Prevent worker that updates merge requests head pipeline from failing jobsissue_41021

9 files changed, 64 insertions, 8 deletions

diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index 422f138c4ea..c40e2a09012 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -83,6 +83,12 @@ class MergeRequest < ActiveRecord::Base
       transition locked: :opened
     end
 
+    before_transition any => :opened do |merge_request|
+      Sidekiq::Worker.skipping_transaction_check do
+        UpdateHeadPipelineForMergeRequestWorker.perform_async(merge_request.id)
+      end
+    end
+
     state :opened
     state :closed
     state :merged
diff --git a/app/services/ci/create_pipeline_service.rb b/app/services/ci/create_pipeline_service.rb
index 85db2760e23..c8b112132b3 100644
--- a/app/services/ci/create_pipeline_service.rb
+++ b/app/services/ci/create_pipeline_service.rb
@@ -81,7 +81,7 @@ module Ci
     end
 
     def related_merge_requests
-      MergeRequest.where(source_project: pipeline.project, source_branch: pipeline.ref)
+      MergeRequest.opened.where(source_project: pipeline.project, source_branch: pipeline.ref)
     end
   end
 end
diff --git a/app/workers/stuck_merge_jobs_worker.rb b/app/workers/stuck_merge_jobs_worker.rb
index 36d2a2e6466..e31fa0fea47 100644
--- a/app/workers/stuck_merge_jobs_worker.rb
+++ b/app/workers/stuck_merge_jobs_worker.rb
@@ -23,7 +23,13 @@ class StuckMergeJobsWorker
     merge_requests = MergeRequest.where(id: completed_ids)
 
     merge_requests.where.not(merge_commit_sha: nil).update_all(state: :merged)
-    merge_requests.where(merge_commit_sha: nil).update_all(state: :opened, merge_jid: nil)
+
+    merge_requests_to_reopen = merge_requests.where(merge_commit_sha: nil)
+    merge_requests_to_reopen.update_all(merge_jid: nil)
+
+    # Do not reopen merge requests using direct queries.
+    # We rely on state machine callbacks to update head_pipeline_id
+    merge_requests_to_reopen.map(&:unlock_mr)
 
     Rails.logger.info("Updated state of locked merge jobs. JIDs: #{completed_jids.join(', ')}")
   end
diff --git a/app/workers/update_head_pipeline_for_merge_request_worker.rb b/app/workers/update_head_pipeline_for_merge_request_worker.rb
index 0a2e9b63578..9614daf1976 100644
--- a/app/workers/update_head_pipeline_for_merge_request_worker.rb
+++ b/app/workers/update_head_pipeline_for_merge_request_worker.rb
@@ -8,8 +8,19 @@ class UpdateHeadPipelineForMergeRequestWorker
     pipeline = Ci::Pipeline.where(project: merge_request.source_project, ref: merge_request.source_branch).last
 
     return unless pipeline && pipeline.latest?
-    raise ArgumentError, 'merge request sha does not equal pipeline sha' if merge_request.diff_head_sha != pipeline.sha
+
+    if merge_request.diff_head_sha != pipeline.sha
+      log_error_message_for(merge_request)
+
+      return
+    end
 
     merge_request.update_attribute(:head_pipeline_id, pipeline.id)
   end
+
+  def log_error_message_for(merge_request)
+    Gitlab::EnvironmentLogger.error(
+      "Outdated head pipeline for active merge request: id=#{merge_request.id}, source_branch=#{merge_request.source_branch}, diff_head_sha=#{merge_request.diff_head_sha}"
+    )
+  end
 end
diff --git a/changelogs/unreleased/issue_41021.yml b/changelogs/unreleased/issue_41021.yml
new file mode 100644
index 00000000000..d94f4b2fecd
--- /dev/null
+++ b/changelogs/unreleased/issue_41021.yml
@@ -0,0 +1,5 @@
+---
+title: Prevent worker that updates merge requests head pipeline from failing jobs
+merge_request:
+author:
+type: fixed
diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb
index 30a5a3bbff7..13a6d125ebc 100644
--- a/spec/models/merge_request_spec.rb
+++ b/spec/models/merge_request_spec.rb
@@ -1855,4 +1855,18 @@ describe MergeRequest do
   it_behaves_like 'throttled touch' do
     subject { create(:merge_request, updated_at: 1.hour.ago) }
   end
+
+  context 'state machine transitions' do
+    describe '#unlock_mr' do
+      subject { create(:merge_request, state: 'locked') }
+
+      it 'updates merge request head pipeline' do
+        pipeline = create(:ci_empty_pipeline, project: subject.project, ref: subject.source_branch, sha: subject.source_branch_sha)
+
+        subject.unlock_mr
+
+        expect(subject.reload.head_pipeline).to eq(pipeline)
+      end
+    end
+  end
 end
diff --git a/spec/services/ci/create_pipeline_service_spec.rb b/spec/services/ci/create_pipeline_service_spec.rb
index 41ce81e0651..267258b33a8 100644
--- a/spec/services/ci/create_pipeline_service_spec.rb
+++ b/spec/services/ci/create_pipeline_service_spec.rb
@@ -64,6 +64,18 @@ describe Ci::CreatePipelineService do
           create(:merge_request, source_branch: 'master', target_branch: "branch_2", source_project: project)
         end
 
+        context 'when related merge request is already merged' do
+          let!(:merged_merge_request) do
+            create(:merge_request, source_branch: 'master', target_branch: "branch_2", source_project: project, state: 'merged')
+          end
+
+          it 'does not schedule update head pipeline job' do
+            expect(UpdateHeadPipelineForMergeRequestWorker).not_to receive(:perform_async).with(merged_merge_request.id)
+
+            execute_service
+          end
+        end
+
         context 'when the head pipeline sha equals merge request sha' do
           it 'updates head pipeline of each merge request' do
             merge_request_1
@@ -77,13 +89,13 @@ describe Ci::CreatePipelineService do
         end
 
         context 'when the head pipeline sha does not equal merge request sha' do
-          it 'raises the ArgumentError error from worker and does not update the head piepeline of MRs' do
+          it 'does not update the head piepeline of MRs' do
             merge_request_1
             merge_request_2
 
             allow_any_instance_of(Ci::Pipeline).to receive(:latest?).and_return(true)
 
-            expect { execute_service(after: 'ae73cb07c9eeaf35924a10f713b364d32b2dd34f') }.to raise_error(ArgumentError)
+            expect { execute_service(after: 'ae73cb07c9eeaf35924a10f713b364d32b2dd34f') }.not_to raise_error
 
             last_pipeline = Ci::Pipeline.last
 
diff --git a/spec/workers/stuck_merge_jobs_worker_spec.rb b/spec/workers/stuck_merge_jobs_worker_spec.rb
index f8b55e873df..c2c2a5f9121 100644
--- a/spec/workers/stuck_merge_jobs_worker_spec.rb
+++ b/spec/workers/stuck_merge_jobs_worker_spec.rb
@@ -14,7 +14,6 @@ describe StuckMergeJobsWorker do
 
         mr_with_sha.reload
         mr_without_sha.reload
-
         expect(mr_with_sha).to be_merged
         expect(mr_without_sha).to be_opened
         expect(mr_with_sha.merge_jid).to be_present
@@ -24,10 +23,13 @@ describe StuckMergeJobsWorker do
       it 'updates merge request to opened when locked but has not been merged' do
         allow(Gitlab::SidekiqStatus).to receive(:completed_jids).and_return(%w(123))
         merge_request = create(:merge_request, :locked, merge_jid: '123', state: :locked)
+        pipeline = create(:ci_empty_pipeline, project: merge_request.project, ref: merge_request.source_branch, sha: merge_request.source_branch_sha)
 
         worker.perform
 
-        expect(merge_request.reload).to be_opened
+        merge_request.reload
+        expect(merge_request).to be_opened
+        expect(merge_request.head_pipeline).to eq(pipeline)
       end
 
       it 'logs updated stuck merge job ids' do
diff --git a/spec/workers/update_head_pipeline_for_merge_request_worker_spec.rb b/spec/workers/update_head_pipeline_for_merge_request_worker_spec.rb
index 522e1566271..9adde5fc21a 100644
--- a/spec/workers/update_head_pipeline_for_merge_request_worker_spec.rb
+++ b/spec/workers/update_head_pipeline_for_merge_request_worker_spec.rb
@@ -22,7 +22,7 @@ describe UpdateHeadPipelineForMergeRequestWorker do
         end
 
         it 'does not update head_pipeline_id' do
-          expect { subject.perform(merge_request.id) }.to raise_error(ArgumentError)
+          expect { subject.perform(merge_request.id) }.not_to raise_error
 
           expect(merge_request.reload.head_pipeline_id).to eq(nil)
         end