Resolves race condition for mergeability check serviceosw-adjust-mergeability-check-racecondition

The MergeabilityCheckService can return success even with
an outdated merge ref. That's because the
MergeRequests::RefreshService is called async and it has
the responsibility of flipping the MRs merge_status to
unchecked when they receive updates, or its target branches
receives an update. Now, for callers that need instant
feedback from the repository, we pass a recheck flag to
the service, which will both sync merge-status with
merge-ref altogether, if the ref is outdated.

This can't be done for every scenario given it's an
expensive calculation.

4 files changed, 114 insertions, 24 deletions

diff --git a/app/services/merge_requests/mergeability_check_service.rb b/app/services/merge_requests/mergeability_check_service.rb
index ef833774e65..505c8594ce0 100644
--- a/app/services/merge_requests/mergeability_check_service.rb
+++ b/app/services/merge_requests/mergeability_check_service.rb
@@ -14,23 +14,27 @@ module MergeRequests
     # Updates the MR merge_status. Whenever it switches to a can_be_merged state,
     # the merge-ref is refreshed.
     #
+    # recheck - When given, it'll enforce a merge-ref refresh if it's outdated,
+    # even if the current merge_status is can_be_merged or cannot_be_merged.
+    # Given MergeRequests::RefreshService is called async, it might happen that the target
+    # branch gets updated, but the MergeRequest#merge_status lags behind. So in scenarios
+    # where we need the current state of the merge ref in repository, the `recheck`
+    # argument is required.
+    #
     # Returns a ServiceResponse indicating merge_status is/became can_be_merged
     # and the merge-ref is synced. Success in case of being/becoming mergeable,
     # error otherwise.
-    def execute
+    def execute(recheck: false)
       return ServiceResponse.error(message: 'Invalid argument') unless merge_request
       return ServiceResponse.error(message: 'Unsupported operation') if Gitlab::Database.read_only?
 
+      recheck! if recheck
       update_merge_status
 
       unless merge_request.can_be_merged?
         return ServiceResponse.error(message: 'Merge request is not mergeable')
       end
 
-      unless payload.fetch(:merge_ref_head)
-        return ServiceResponse.error(message: 'Merge ref was not found')
-      end
-
       ServiceResponse.success(payload: payload)
     end
 
@@ -63,13 +67,31 @@ module MergeRequests
     def update_merge_status
       return unless merge_request.recheck_merge_status?
 
-      if can_git_merge?
-        merge_to_ref && merge_request.mark_as_mergeable
+      if can_git_merge? && merge_to_ref
+        merge_request.mark_as_mergeable
       else
         merge_request.mark_as_unmergeable
       end
     end
 
+    def recheck!
+      if !merge_request.recheck_merge_status? && outdated_merge_ref?
+        merge_request.mark_as_unchecked
+      end
+    end
+
+    # Checks if the existing merge-ref is synced with the target branch.
+    #
+    # Returns true if the merge-ref does not exists or is out of sync.
+    def outdated_merge_ref?
+      ref_head = merge_request.merge_ref_head
+
+      return true unless ref_head
+      return true unless target_id = merge_request.target_branch_sha
+
+      ref_head.parent_id != target_id
+    end
+
     def can_git_merge?
       !merge_request.broken? && repository.can_be_merged?(merge_request.diff_head_sha, merge_request.target_branch)
     end
diff --git a/lib/api/merge_requests.rb b/lib/api/merge_requests.rb
index 5bbf6df78b0..bf87e9ec2ff 100644
--- a/lib/api/merge_requests.rb
+++ b/lib/api/merge_requests.rb
@@ -401,7 +401,7 @@ module API
       get ':id/merge_requests/:merge_request_iid/merge_ref' do
         merge_request = find_project_merge_request(params[:merge_request_iid])
 
-        result = ::MergeRequests::MergeabilityCheckService.new(merge_request).execute
+        result = ::MergeRequests::MergeabilityCheckService.new(merge_request).execute(recheck: true)
 
         if result.success?
           present :commit_id, result.payload.dig(:merge_ref_head, :commit_id)
diff --git a/spec/requests/api/merge_requests_spec.rb b/spec/requests/api/merge_requests_spec.rb
index 4cb4fcc890d..76d093e0774 100644
--- a/spec/requests/api/merge_requests_spec.rb
+++ b/spec/requests/api/merge_requests_spec.rb
@@ -1564,13 +1564,28 @@ describe API::MergeRequests do
       expect(json_response['commit_id']).to eq(merge_request.merge_ref_head.sha)
     end
 
-    it "returns 400 if branch can't be merged" do
-      merge_request.update!(merge_status: 'cannot_be_merged')
+    context 'when merge-ref is not synced with merge status' do
+      before do
+        merge_request.update!(merge_status: 'cannot_be_merged')
+      end
 
-      get api(url, user)
+      it 'returns 200 if MR can be merged' do
+        get api(url, user)
 
-      expect(response).to have_gitlab_http_status(400)
-      expect(json_response['message']).to eq('Merge request is not mergeable')
+        expect(response).to have_gitlab_http_status(200)
+        expect(json_response['commit_id']).to eq(merge_request.merge_ref_head.sha)
+      end
+
+      it 'returns 400 if MR cannot be merged' do
+        expect_next_instance_of(MergeRequests::MergeToRefService) do |merge_request|
+          expect(merge_request).to receive(:execute) { { status: :failed } }
+        end
+
+        get api(url, user)
+
+        expect(response).to have_gitlab_http_status(400)
+        expect(json_response['message']).to eq('Merge request is not mergeable')
+      end
     end
 
     context 'when user has no access to the MR' do
diff --git a/spec/services/merge_requests/mergeability_check_service_spec.rb b/spec/services/merge_requests/mergeability_check_service_spec.rb
index aa0485467ed..8800907c304 100644
--- a/spec/services/merge_requests/mergeability_check_service_spec.rb
+++ b/spec/services/merge_requests/mergeability_check_service_spec.rb
@@ -166,21 +166,74 @@ describe MergeRequests::MergeabilityCheckService do
       end
     end
 
-    context 'when MR is mergeable but merge-ref does not exists' do
-      before do
-        merge_request.mark_as_mergeable!
-      end
+    context 'recheck enforced' do
+      subject { described_class.new(merge_request).execute(recheck: true) }
+
+      context 'when MR is mergeable but merge-ref does not exists' do
+        before do
+          merge_request.mark_as_mergeable!
+        end
 
-      it 'keeps merge status as can_be_merged' do
-        expect { subject }.not_to change(merge_request, :merge_status).from('can_be_merged')
+        it_behaves_like 'mergeable merge request'
       end
 
-      it 'returns ServiceResponse.error' do
-        result = subject
+      context 'when MR is mergeable but target-branch is out of sync with merge-ref' do
+        let(:fake_commit) { build(:commit) }
 
-        expect(result).to be_a(ServiceResponse)
-        expect(result.error?).to be(true)
-        expect(result.message).to eq('Merge ref was not found')
+        before do
+          MergeRequests::MergeToRefService.new(project, merge_request.author).execute(merge_request)
+
+          allow(merge_request).to receive(:target_branch_sha) { fake_commit.id }
+
+          merge_request.mark_as_mergeable!
+        end
+
+        context 'when successfully updates the merge-ref' do
+          it 'keeps merge status as can_be_merged' do
+            expect { subject }.not_to change(merge_request, :merge_status).from('can_be_merged')
+          end
+
+          it 'updates the merge ref' do
+            expect_next_instance_of(MergeRequests::MergeToRefService) do |merge_to_ref|
+              expect(merge_to_ref).to receive(:execute).and_call_original
+            end
+
+            subject
+          end
+
+          it 'returns ServiceResponse.success' do
+            result = subject
+
+            expect(result).to be_a(ServiceResponse)
+            expect(result).to be_success
+          end
+
+          it 'ServiceResponse has merge_ref_head payload' do
+            result = subject
+
+            expect(result.payload.keys).to contain_exactly(:merge_ref_head)
+            expect(result.payload[:merge_ref_head].keys)
+              .to contain_exactly(:commit_id, :target_id, :source_id)
+          end
+        end
+
+        context 'when fails to update the merge-ref' do
+          before do
+            expect_next_instance_of(MergeRequests::MergeToRefService) do |merge_to_ref|
+              expect(merge_to_ref).to receive(:execute).and_return(status: :failed)
+            end
+          end
+
+          it_behaves_like 'unmergeable merge request'
+
+          it 'returns ServiceResponse.error' do
+            result = subject
+
+            expect(result).to be_a(ServiceResponse)
+            expect(result.error?).to be(true)
+            expect(result.message).to eq('Merge request is not mergeable')
+          end
+        end
       end
     end
   end