Commit dd88c580 authored by Patrick Steinhardt's avatar Patrick Steinhardt

gitaly: Handle newly introduced ReferenceUpdateErrors

When updating references in UserMergeBranch failed due to a concurrent
update of the same reference, then Gitaly used to return no error at all
but instead just return an empty gRPC response. This behaviour isn't
obvious at all and easy to get wrong. Furthermore, Rails didn't have any
information about what went wrong.

To fix this, Gitaly has introduced a new error type which is returned in
case there was a race updating the reference. Instead of returning
successful, it now returns an error with a ReferenceUpdateError embedded
inside of it. This allow us to extract this error and thus know exactly
what happened, including the reference that failed to update with both
its old and new expected object ID values.

Update the Gitaly client to handle this error in a backwards-compatible
fashion. For now this means that we silently ignore it like we used to
do with the old error as well.
parent e5b2feee
# 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
...@@ -168,8 +168,12 @@ module Gitlab ...@@ -168,8 +168,12 @@ module Gitlab
raise unless decoded_error.present? 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 ensure
request_enum.close request_enum.close
end end
...@@ -495,6 +499,12 @@ module Gitlab ...@@ -495,6 +499,12 @@ module Gitlab
access_check_error = detailed_error.access_check access_check_error = detailed_error.access_check
# These messages were returned from internal/allowed API calls # These messages were returned from internal/allowed API calls
Gitlab::Git::PreReceiveError.new(fallback_message: access_check_error.error_message) 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 else
# We're handling access_check only for now, but we'll add more detailed error types # We're handling access_check only for now, but we'll add more detailed error types
nil nil
......
...@@ -225,6 +225,27 @@ RSpec.describe Gitlab::GitalyClient::OperationService do ...@@ -225,6 +225,27 @@ RSpec.describe Gitlab::GitalyClient::OperationService do
expect { subject }.to raise_error(GRPC::PermissionDenied) expect { subject }.to raise_error(GRPC::PermissionDenied)
end end
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 end
describe '#user_ff_branch' do describe '#user_ff_branch' do
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment