Merge branch 'fix_race_condition_issue_on_merge_requests_create_service' into 'master'

Fix race condition issue on MergeRequests::CreateService

See merge request gitlab-org/gitlab!39587

app/services/merge_requests/after_create_service.rb

@@ -14,3 +14,5 @@ module MergeRequests
     end
   end
 end
+
+MergeRequests::AfterCreateService.prepend_if_ee('EE::MergeRequests::AfterCreateService')

ee/app/services/ee/merge_requests/after_create_service.rb

+# frozen_string_literal: true
+
+module EE
+  module MergeRequests
+    module AfterCreateService
+      extend ::Gitlab::Utils::Override
+
+      override :execute
+      def execute(merge_request)
+        super
+
+        schedule_sync_for(merge_request.head_pipeline_id)
+      end
+
+      private
+
+      def schedule_sync_for(pipeline_id)
+        ::SyncSecurityReportsToReportApprovalRulesWorker.perform_async(pipeline_id) if pipeline_id
+      end
+    end
+  end
+end

ee/app/services/ee/merge_requests/create_service.rb

@@ -12,12 +12,6 @@ module EE
         ::MergeRequests::SyncCodeOwnerApprovalRules.new(issuable).execute
         ::MergeRequests::SyncReportApproverApprovalRules.new(issuable).execute
 
-        # Attempt to sync reports if pipeline has finished before MR has been created
-        pipeline = issuable.find_actual_head_pipeline
-        if pipeline
-          ::SyncSecurityReportsToReportApprovalRulesWorker.perform_async(pipeline.id)
-        end
-
         ::MergeRequests::UpdateBlocksService
           .new(issuable, current_user, blocking_merge_requests_params)
           .execute

ee/changelogs/unreleased/fix_race_condition_issue_on_merge_requests_create_service.yml

+---
+title: Fix the `Vulnerability-Check` and `License-Check` approval rules to be synched correctly after creating a merge request.
+merge_request: 39587
+author:
+type: fixed

ee/spec/services/ee/merge_requests/after_create_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe MergeRequests::AfterCreateService do
+  let_it_be(:merge_request) { create(:merge_request) }
+
+  let(:service_object) { described_class.new(merge_request.target_project, merge_request.author) }
+
+  describe '#execute' do
+    subject(:execute) { service_object.execute(merge_request) }
+
+    before do
+      allow(SyncSecurityReportsToReportApprovalRulesWorker).to receive(:perform_async)
+    end
+
+    context 'when the merge request has actual_head_pipeline' do
+      let(:pipeline_id) { 1881 }
+
+      before do
+        allow(merge_request).to receive(:head_pipeline_id).and_return(pipeline_id)
+        allow(merge_request).to receive(:update_head_pipeline).and_return(true)
+      end
+
+      it 'schedules a background job to sync security reports to approval rules' do
+        execute
+
+        expect(merge_request).to have_received(:update_head_pipeline).ordered
+        expect(SyncSecurityReportsToReportApprovalRulesWorker).to have_received(:perform_async).ordered.with(pipeline_id)
+      end
+    end
+
+    context 'when the merge request does not have actual_head_pipeline' do
+      it 'does not schedule a background job to sync security reports to approval rules' do
+        execute
+
+        expect(SyncSecurityReportsToReportApprovalRulesWorker).not_to have_received(:perform_async)
+      end
+    end
+  end
+end

ee/spec/services/ee/merge_requests/create_service_spec.rb

@@ -39,9 +39,6 @@ RSpec.describe MergeRequests::CreateService do
     end
 
     context 'report approvers' do
-      let(:sha) { project.repository.commits(opts[:source_branch], limit: 1).first.id }
-      let(:pipeline) { instance_double(Ci::Pipeline, id: 42, project_id: project.id, merge_request?: true) }
-
       it 'refreshes report approvers for the merge request' do
         expect_next_instance_of(::MergeRequests::SyncReportApproverApprovalRules) do |service|
           expect(service).to receive(:execute)
@@ -49,24 +46,6 @@ RSpec.describe MergeRequests::CreateService do
 
         service.execute
       end
-
-      it 'enqueues approval rule report syncing when pipeline exists' do
-        expect_next_instance_of(MergeRequest) do |merge_request|
-          allow(merge_request).to receive(:find_actual_head_pipeline).and_return(pipeline)
-          allow(merge_request).to receive(:update_head_pipeline).and_return(true)
-        end
-        expect(::SyncSecurityReportsToReportApprovalRulesWorker)
-          .to receive(:perform_async)
-
-        service.execute
-      end
-
-      it 'wont enqueue approval rule report syncing without pipeline' do
-        expect(::SyncSecurityReportsToReportApprovalRulesWorker)
-          .not_to receive(:perform_async)
-
-        service.execute
-      end
     end
 
     it_behaves_like 'new issuable with scoped labels' do