Check if head_pipeline is correct before merging

app/controllers/projects/merge_requests_controller.rb

@@ -283,15 +283,15 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
     @merge_request.update(merge_error: nil)
     if params[:merge_when_pipeline_succeeds].present?
-      return :failed unless @merge_request.head_pipeline
-      if @merge_request.head_pipeline.active?
+      return :failed unless @merge_request.current_head_pipeline
+      if @merge_request.current_head_pipeline.active?
         ::MergeRequests::MergeWhenPipelineSucceedsService
           .new(@project, current_user, merge_params)
           .execute(@merge_request)
         :merge_when_pipeline_succeeds
-      elsif @merge_request.head_pipeline.success?
+      elsif @merge_request.current_head_pipeline.success?
         # This can be triggered when a user clicks the auto merge button while
         # the tests finish at about the same time
         @merge_request.merge_async(current_user.id, params)

app/models/merge_request.rb

@@ -145,8 +145,11 @@ class MergeRequest < ActiveRecord::Base
     '!'
   end
-  def head_pipeline
-    super&.sha == diff_head_sha ? super : nil
+  # Use this method whenever you need to make sure the head_pipeline is synced with the
+  # branch head commit, for example checking if a merge request can be merged.
+  # For more information check: https://gitlab.com/gitlab-org/gitlab-ce/issues/40004
+  def current_head_pipeline
+    head_pipeline&.sha == diff_head_sha ? head_pipeline : nil
   end
   # Pattern used to extract `!123` merge request references from text
@@ -826,8 +829,9 @@ class MergeRequest < ActiveRecord::Base
   def mergeable_ci_state?
     return true unless project.only_allow_merge_if_pipeline_succeeds?
-    !head_pipeline || head_pipeline.success? || head_pipeline.skipped?
+    return true unless head_pipeline
+    current_head_pipeline&.success? || current_head_pipeline&.skipped?
   end
   def environments_for(current_user)
@@ -1001,7 +1005,7 @@ class MergeRequest < ActiveRecord::Base
     return true if autocomplete_precheck
     return false unless mergeable?(skip_ci_check: true)
-    return false if head_pipeline && !(head_pipeline.success? || head_pipeline.active?)
+    return false if current_head_pipeline && !(current_head_pipeline.success? || current_head_pipeline.active?)
     return false if last_diff_sha != diff_head_sha
     true

spec/controllers/projects/merge_requests_controller_spec.rb

@@ -324,12 +324,12 @@ describe Projects::MergeRequestsController do
       end
       context 'when the pipeline succeeds is passed' do
-        def merge_when_pipeline_succeeds
-          post :merge, base_params.merge(sha: merge_request.diff_head_sha, merge_when_pipeline_succeeds: '1')
+        let!(:head_pipeline) do
+          create(:ci_empty_pipeline, project: project, sha: merge_request.diff_head_sha, ref: merge_request.source_branch, head_pipeline_of: merge_request)
         end
-        before do
-          create(:ci_empty_pipeline, project: project, sha: merge_request.diff_head_sha, ref: merge_request.source_branch, head_pipeline_of: merge_request)
+        def merge_when_pipeline_succeeds
+          post :merge, base_params.merge(sha: merge_request.diff_head_sha, merge_when_pipeline_succeeds: '1')
         end
         it 'returns :merge_when_pipeline_succeeds' do
@@ -354,6 +354,18 @@ describe Projects::MergeRequestsController do
             project.update_column(:only_allow_merge_if_pipeline_succeeds, true)
           end
+          context 'and head pipeline is not the current one' do
+            before do
+              head_pipeline.update(sha: 'not_current_sha')
+            end
+
+            it 'returns :failed' do
+              merge_when_pipeline_succeeds
+
+              expect(json_response).to eq('status' => 'failed')
+            end
+          end
+
           it 'returns :merge_when_pipeline_succeeds' do
             merge_when_pipeline_succeeds

spec/models/merge_request_spec.rb

@@ -827,29 +827,34 @@ describe MergeRequest do
     end
   end
-  describe '#head_pipeline' do
+  context 'head pipeline' do
     before do
       allow(subject).to receive(:diff_head_sha).and_return('lastsha')
     end
-    it 'returns nil for MR without head_pipeline_id' do
-      subject.update_attribute(:head_pipeline_id, nil)
-      expect(subject.head_pipeline).to be_nil
+    describe '#head_pipeline' do
+      it 'returns nil for MR without head_pipeline_id' do
+        subject.update_attribute(:head_pipeline_id, nil)
+        expect(subject.head_pipeline).to be_nil
+      end
     end
-    it 'returns nil for MR with old pipeline' do
-      pipeline = create(:ci_empty_pipeline, sha: 'notlatestsha')
-      subject.update_attribute(:head_pipeline_id, pipeline.id)
-      expect(subject.head_pipeline).to be_nil
-    end
-    it 'returns the pipeline for MR with recent pipeline' do
-      pipeline = create(:ci_empty_pipeline, sha: 'lastsha')
-      subject.update_attribute(:head_pipeline_id, pipeline.id)
-      expect(subject.head_pipeline).to eq(pipeline)
+    describe '#current_head_pipeline' do
+      it 'returns nil for MR with old pipeline' do
+        pipeline = create(:ci_empty_pipeline, sha: 'notlatestsha')
+        subject.update_attribute(:head_pipeline_id, pipeline.id)
+        expect(subject.current_head_pipeline).to be_nil
+      end
+      it 'returns the pipeline for MR with recent pipeline' do
+        pipeline = create(:ci_empty_pipeline, sha: 'lastsha')
+        subject.update_attribute(:head_pipeline_id, pipeline.id)
+        expect(subject.current_head_pipeline).to eq(subject.head_pipeline)
+        expect(subject.current_head_pipeline).to eq(pipeline)
+      end
     end
   end
@@ -1187,7 +1192,7 @@ describe MergeRequest do
     context 'when it is only allowed to merge when build is green' do
       context 'and a failed pipeline is associated' do
         before do
-          pipeline.update(status: 'failed')
+          pipeline.update(status: 'failed', sha: subject.diff_head_sha)
           allow(subject).to receive(:head_pipeline) { pipeline }
         end
@@ -1196,7 +1201,7 @@ describe MergeRequest do
       context 'and a successful pipeline is associated' do
         before do
-          pipeline.update(status: 'success')
+          pipeline.update(status: 'success', sha: subject.diff_head_sha)
           allow(subject).to receive(:head_pipeline) { pipeline }
         end
@@ -1205,7 +1210,7 @@ describe MergeRequest do
       context 'and a skipped pipeline is associated' do
         before do
-          pipeline.update(status: 'skipped')
+          pipeline.update(status: 'skipped', sha: subject.diff_head_sha)
           allow(subject).to receive(:head_pipeline) { pipeline }
         end