Merge branch 'pks-operations-merges-allow-structured-errors' into 'master'

gitaly: Handle newly introduced ReferenceUpdateErrors

See merge request gitlab-org/gitlab!78366

Gemfile

@@ -473,7 +473,7 @@ gem 'ssh_data', '~> 1.2'
 gem 'spamcheck', '~> 0.1.0'
 
 # Gitaly GRPC protocol definitions
-gem 'gitaly', '~> 14.6.0.pre.rc1'
+gem 'gitaly', '~> 14.8.0.pre.rc1'
 
 # KAS GRPC protocol definitions
 gem 'kas-grpc', '~> 0.0.2'

Gemfile.lock

@@ -449,7 +449,7 @@ GEM
       rails (>= 3.2.0)
     git (1.7.0)
       rchardet (~> 1.8)
-    gitaly (14.6.0.pre.rc1)
+    gitaly (14.8.0.pre.rc1)
       grpc (~> 1.0)
     github-markup (1.7.0)
     gitlab (4.16.1)
@@ -1466,7 +1466,7 @@ DEPENDENCIES
   gettext (~> 3.3)
   gettext_i18n_rails (~> 1.8.0)
   gettext_i18n_rails_js (~> 1.3)
-  gitaly (~> 14.6.0.pre.rc1)
+  gitaly (~> 14.8.0.pre.rc1)
   github-markup (~> 1.7.0)
   gitlab-chronic (~> 0.10.5)
   gitlab-dangerfiles (~> 2.8.0)

lib/gitlab/git/reference_update_error.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module Git
+    # ReferenceUpdateError represents an error that happen when trying to
+    # update a Git reference.
+    class ReferenceUpdateError < StandardError
+      def initialize(message, reference, old_oid, new_oid)
+        @message = message
+        @reference = reference
+        @old_oid = old_oid
+        @new_oid = new_oid
+      end
+    end
+  end
+end

lib/gitlab/gitaly_client/operation_service.rb

@@ -168,8 +168,12 @@ module Gitlab
 
         raise unless decoded_error.present?
 
-        raise decoded_error
+        # We simply ignore any reference update errors which are typically an
+        # indicator of multiple RPC calls trying to update the same reference
+        # at the same point in time.
+        return if decoded_error.is_a?(Gitlab::Git::ReferenceUpdateError)
 
+        raise decoded_error
       ensure
         request_enum.close
       end
@@ -495,6 +499,12 @@ module Gitlab
           access_check_error = detailed_error.access_check
           # These messages were returned from internal/allowed API calls
           Gitlab::Git::PreReceiveError.new(fallback_message: access_check_error.error_message)
+        when :reference_update
+          reference_update_error = detailed_error.reference_update
+          Gitlab::Git::ReferenceUpdateError.new(err.details,
+                                                reference_update_error.reference_name,
+                                                reference_update_error.old_oid,
+                                                reference_update_error.new_oid)
         else
           # We're handling access_check only for now, but we'll add more detailed error types
           nil

spec/lib/gitlab/gitaly_client/operation_service_spec.rb

@@ -2,6 +2,9 @@
 
 require 'spec_helper'
 
+require 'google/rpc/status_pb'
+require 'google/protobuf/well_known_types'
+
 RSpec.describe Gitlab::GitalyClient::OperationService do
   let_it_be(:user) { create(:user) }
   let_it_be(:project) { create(:project, :repository) }
@@ -185,11 +188,16 @@ RSpec.describe Gitlab::GitalyClient::OperationService do
 
     context 'with an exception with the UserMergeBranchError' do
       let(:permission_error) do
-        GRPC::PermissionDenied.new(
+        new_detailed_error(
+          GRPC::Core::StatusCodes::PERMISSION_DENIED,
           "GitLab: You are not allowed to push code to this project.",
-          { "grpc-status-details-bin" =>
-           "\b\a\x129GitLab: You are not allowed to push code to this project.\x1A\xDE\x01\n/type.googleapis.com/gitaly.UserMergeBranchError\x12\xAA\x01\n\xA7\x01\n1You are not allowed to push code to this project.\x12\x03web\x1A\auser-15\"df15b32277d2c55c6c595845a87109b09c913c556 5d6e0f935ad9240655f64e883cd98fad6f9a17ee refs/heads/master\n" }
-        )
+          Gitaly::UserMergeBranchError.new(
+            access_check: Gitaly::AccessCheckError.new(
+              error_message: "You are not allowed to push code to this project.",
+              protocol: "web",
+              user_id: "user-15",
+              changes: "df15b32277d2c55c6c595845a87109b09c913c556 5d6e0f935ad9240655f64e883cd98fad6f9a17ee refs/heads/master\n"
+            )))
       end
 
       it 'raises PreRecieveError with the error message' do
@@ -217,6 +225,27 @@ RSpec.describe Gitlab::GitalyClient::OperationService do
         expect { subject }.to raise_error(GRPC::PermissionDenied)
       end
     end
+
+    context 'with ReferenceUpdateError' do
+      let(:reference_update_error) do
+        new_detailed_error(GRPC::Core::StatusCodes::FAILED_PRECONDITION,
+                           "some ignored error message",
+                           Gitaly::UserMergeBranchError.new(
+                             reference_update: Gitaly::ReferenceUpdateError.new(
+                               reference_name: "refs/heads/something",
+                               old_oid: "1234",
+                               new_oid: "6789"
+                             )))
+      end
+
+      it 'returns nil' do
+        expect_any_instance_of(Gitaly::OperationService::Stub)
+          .to receive(:user_merge_branch).with(kind_of(Enumerator), kind_of(Hash))
+          .and_raise(reference_update_error)
+
+        expect(subject).to be_nil
+      end
+    end
   end
 
   describe '#user_ff_branch' do
@@ -478,4 +507,14 @@ RSpec.describe Gitlab::GitalyClient::OperationService do
       end
     end
   end
+
+  def new_detailed_error(error_code, error_message, details)
+    status_error = Google::Rpc::Status.new(
+      code: error_code,
+      message: error_message,
+      details: [Google::Protobuf::Any.pack(details)]
+    )
+
+    GRPC::BadStatus.new(error_code, error_message, { "grpc-status-details-bin" => Google::Rpc::Status.encode(status_error) })
+  end
 end