Commit 8ab5d0db authored by Nick Thomas's avatar Nick Thomas

Fix intermittent 'branch not found' errors when pushing creates an MR

When we push to create a merge request (-omr.create), we kick off an
asynchronous `PostReceive` worker job, then synchronously create a
merge request. This requires the target branch (but not the source
branch) to exist in the repository, so we check when validating the
push options.

The `PostReceive` worker invalidates the branch name cache, but does so
asynchronously, so we may race with it. Until it is invalidated, the
target branch may appear as if it doesn't exist, so we should instead
explicitly invalidate it for ourselves.

This is just dead reckoning at the moment, and is unlikely to solve
cases where the target branch is the project's default branch, but it
seems worth fixing to me.

Changelog: fixed
parent dcef079a
......@@ -65,7 +65,7 @@ module MergeRequests
end
if push_options[:target] && !target_project.repository.branch_exists?(push_options[:target])
errors << "Branch #{push_options[:target]} does not exist"
errors << "Target branch #{target_project.full_path}:#{push_options[:target]} does not exist"
end
end
......
......@@ -17,13 +17,18 @@ class PostReceiveService
response = Gitlab::InternalPostReceive::Response.new
push_options = Gitlab::PushOptions.new(params[:push_options])
mr_options = push_options.get(:merge_request)
response.reference_counter_decreased = Gitlab::ReferenceCounter.new(params[:gl_repository]).decrease
# The PostReceive worker will normally invalidate the cache. However, it
# runs asynchronously. If push options require us to create a new merge
# request synchronously, we can't rely on that, so invalidate the cache here
repository&.expire_branches_cache if mr_options&.key?(:create)
PostReceive.perform_async(params[:gl_repository], params[:identifier],
params[:changes], push_options.as_json)
mr_options = push_options.get(:merge_request)
if mr_options.present?
message = process_mr_push_options(mr_options, params[:changes])
response.add_alert_message(message)
......
......@@ -743,7 +743,7 @@ RSpec.describe MergeRequests::PushOptionsHandlerService do
it 'records an error' do
service.execute
expect(service.errors).to eq(['Branch my-branch does not exist'])
expect(service.errors).to eq(["Target branch #{project.full_path}:my-branch does not exist"])
end
end
......
......@@ -131,6 +131,12 @@ RSpec.describe PostReceiveService do
project.add_developer(user)
end
it 'invalidates the branch name cache' do
expect(service.repository).to receive(:expire_branches_cache).and_call_original
subject
end
it 'invokes MergeRequests::PushOptionsHandlerService' do
expect(MergeRequests::PushOptionsHandlerService).to receive(:new).and_call_original
......
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