Merge branch '324369-api-jobs-request-the-assign_runner-likely-has-n-1-problem' into 'master'

Move merge request build failure todo creation to an async worker [RUN ALL RSPEC] [RUN AS-IF-FOSS]

See merge request gitlab-org/gitlab!57490

app/models/commit_status.rb

@@ -56,6 +56,7 @@ class CommitStatus < ApplicationRecord
   scope :by_name, -> (name) { where(name: name) }
   scope :in_pipelines, ->(pipelines) { where(pipeline: pipelines) }
   scope :eager_load_pipeline, -> { eager_load(:pipeline, project: { namespace: :route }) }
+  scope :with_pipeline, -> { joins(:pipeline) }
 
   scope :for_project_paths, -> (paths) do
     where(project: Project.where_full_path_in(Array(paths)))
@@ -180,6 +181,7 @@ class CommitStatus < ApplicationRecord
     end
 
     after_transition any => :failed do |commit_status|
+      next if Feature.enabled?(:async_add_build_failure_todo, commit_status.project, default_enabled: :yaml)
       next unless commit_status.project
 
       # rubocop: disable CodeReuse/ServiceClass

app/services/ci/retry_build_service.rb

@@ -21,7 +21,7 @@ module Ci
 
         Gitlab::OptimisticLocking.retry_lock(new_build, name: 'retry_build', &:enqueue)
 
-        MergeRequests::AddTodoWhenBuildFailsService
+        ::MergeRequests::AddTodoWhenBuildFailsService
           .new(project, current_user)
           .close(new_build)
       end

app/services/ci/retry_pipeline_service.rb

@@ -28,7 +28,7 @@ module Ci
 
       pipeline.reset_ancestor_bridges!
 
-      MergeRequests::AddTodoWhenBuildFailsService
+      ::MergeRequests::AddTodoWhenBuildFailsService
         .new(project, current_user)
         .close_all(pipeline)
 

app/services/merge_requests/base_service.rb

@@ -164,7 +164,7 @@ module MergeRequests
 
     def pipeline_merge_requests(pipeline)
       pipeline.all_merge_requests.opened.each do |merge_request|
-        next unless pipeline == merge_request.head_pipeline
+        next unless pipeline.id == merge_request.head_pipeline_id
 
         yield merge_request
       end

app/workers/all_queues.yml

@@ -1235,6 +1235,14 @@
   :weight: 3
   :idempotent:
   :tags: []
+- :name: pipeline_default:ci_merge_requests_add_todo_when_build_fails
+  :feature_category: :continuous_integration
+  :has_external_dependencies:
+  :urgency: :low
+  :resource_boundary: :unknown
+  :weight: 3
+  :idempotent: true
+  :tags: []
 - :name: pipeline_default:ci_pipeline_bridge_status
   :feature_category: :continuous_integration
   :has_external_dependencies:

app/workers/build_finished_worker.rb

@@ -37,6 +37,10 @@ class BuildFinishedWorker # rubocop:disable Scalability/IdempotentWorker
     ExpirePipelineCacheWorker.perform_async(build.pipeline_id)
     ChatNotificationWorker.perform_async(build.id) if build.pipeline.chat?
 
+    if build.failed? && Feature.enabled?(:async_add_build_failure_todo, build.project, default_enabled: :yaml)
+      ::Ci::MergeRequests::AddTodoWhenBuildFailsWorker.perform_async(build.id)
+    end
+
     ##
     # We want to delay sending a build trace to object storage operation to
     # validate that this fixes a race condition between this and flushing live

app/workers/ci/merge_requests/add_todo_when_build_fails_worker.rb

+# frozen_string_literal: true
+module Ci
+  module MergeRequests
+    class AddTodoWhenBuildFailsWorker
+      include ApplicationWorker
+      include PipelineQueue
+
+      urgency :low
+      idempotent!
+
+      def perform(job_id)
+        job = ::CommitStatus.with_pipeline.find_by_id(job_id)
+        project = job&.project
+
+        return unless job && project
+
+        ::MergeRequests::AddTodoWhenBuildFailsService.new(job.project, nil).execute(job)
+      end
+    end
+  end
+end

config/feature_flags/development/async_add_build_failure_todo.yml

+---
+name: async_add_build_failure_todo
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/57490/diffs
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/326726
+milestone: '13.11'
+type: development
+group: group::continuous integration
+default_enabled: false

spec/models/ci/build_spec.rb

@@ -3582,10 +3582,10 @@ RSpec.describe Ci::Build do
   end
 
   describe 'state transition when build fails' do
-    let(:service) { MergeRequests::AddTodoWhenBuildFailsService.new(project, user) }
+    let(:service) { ::MergeRequests::AddTodoWhenBuildFailsService.new(project, user) }
 
     before do
-      allow(MergeRequests::AddTodoWhenBuildFailsService).to receive(:new).and_return(service)
+      allow(::MergeRequests::AddTodoWhenBuildFailsService).to receive(:new).and_return(service)
       allow(service).to receive(:close)
     end
 
@@ -3670,15 +3670,42 @@ RSpec.describe Ci::Build do
         subject.drop!
       end
 
-      it 'creates a todo' do
-        project.add_developer(user)
+      context 'when async_add_build_failure_todo flag enabled' do
+        it 'creates a todo async', :sidekiq_inline do
+          project.add_developer(user)
+
+          expect_next_instance_of(TodoService) do |todo_service|
+            expect(todo_service)
+              .to receive(:merge_request_build_failed).with(merge_request)
+          end
 
-        expect_next_instance_of(TodoService) do |todo_service|
-          expect(todo_service)
-            .to receive(:merge_request_build_failed).with(merge_request)
+          subject.drop!
         end
 
-        subject.drop!
+        it 'does not create a sync todo' do
+          project.add_developer(user)
+
+          expect(TodoService).not_to receive(:new)
+
+          subject.drop!
+        end
+      end
+
+      context 'when async_add_build_failure_todo flag disabled' do
+        before do
+          stub_feature_flags(async_add_build_failure_todo: false)
+        end
+
+        it 'creates a todo sync' do
+          project.add_developer(user)
+
+          expect_next_instance_of(TodoService) do |todo_service|
+            expect(todo_service)
+              .to receive(:merge_request_build_failed).with(merge_request)
+          end
+
+          subject.drop!
+        end
       end
     end
 

spec/services/ci/retry_build_service_spec.rb

@@ -181,7 +181,7 @@ RSpec.describe Ci::RetryBuildService do
       end
 
       it 'resolves todos for old build that failed' do
-        expect(MergeRequests::AddTodoWhenBuildFailsService)
+        expect(::MergeRequests::AddTodoWhenBuildFailsService)
           .to receive_message_chain(:new, :close)
 
         service.execute(build)

spec/services/ci/retry_pipeline_service_spec.rb

@@ -272,7 +272,7 @@ RSpec.describe Ci::RetryPipelineService, '#execute' do
     end
 
     it 'closes all todos about failed jobs for pipeline' do
-      expect(MergeRequests::AddTodoWhenBuildFailsService)
+      expect(::MergeRequests::AddTodoWhenBuildFailsService)
         .to receive_message_chain(:new, :close_all)
 
       service.execute(pipeline)

spec/services/merge_requests/add_todo_when_build_fails_service_spec.rb

@@ -2,7 +2,7 @@
 
 require 'spec_helper'
 
-RSpec.describe MergeRequests::AddTodoWhenBuildFailsService do
+RSpec.describe ::MergeRequests::AddTodoWhenBuildFailsService do
   let(:user) { create(:user) }
   let(:project) { create(:project, :repository) }
   let(:sha) { '1234567890abcdef1234567890abcdef12345678' }
@@ -24,8 +24,8 @@ RSpec.describe MergeRequests::AddTodoWhenBuildFailsService do
 
   before do
     allow_any_instance_of(MergeRequest)
-      .to receive(:head_pipeline)
-      .and_return(pipeline)
+      .to receive(:head_pipeline_id)
+      .and_return(pipeline.id)
 
     allow(service).to receive(:todo_service).and_return(todo_service)
   end

spec/workers/build_finished_worker_spec.rb

@@ -6,10 +6,8 @@ RSpec.describe BuildFinishedWorker do
   subject { described_class.new.perform(build.id) }
 
   describe '#perform' do
-    let(:build) { create(:ci_build, :success, pipeline: create(:ci_pipeline)) }
-
     context 'when build exists' do
-      let!(:build) { create(:ci_build) }
+      let_it_be(:build) { create(:ci_build, :success, pipeline: create(:ci_pipeline)) }
 
       before do
         expect(Ci::Build).to receive(:find_by).with(id: build.id).and_return(build)
@@ -30,6 +28,42 @@ RSpec.describe BuildFinishedWorker do
 
         subject
       end
+
+      context 'when build is failed' do
+        before do
+          build.update!(status: :failed)
+        end
+
+        it 'adds a todo' do
+          expect(::Ci::MergeRequests::AddTodoWhenBuildFailsWorker).to receive(:perform_async)
+
+          subject
+        end
+
+        context 'when async_add_build_failure_todo disabled' do
+          before do
+            stub_feature_flags(async_add_build_failure_todo: false)
+          end
+
+          it 'does not add a todo' do
+            expect(::Ci::MergeRequests::AddTodoWhenBuildFailsWorker).not_to receive(:perform_async)
+
+            subject
+          end
+        end
+      end
+
+      context 'when build has a chat' do
+        before do
+          build.pipeline.update!(source: :chat)
+        end
+
+        it 'schedules a ChatNotification job' do
+          expect(ChatNotificationWorker).to receive(:perform_async).with(build.id)
+
+          subject
+        end
+      end
     end
 
     context 'when build does not exist' do
@@ -38,15 +72,5 @@ RSpec.describe BuildFinishedWorker do
           .not_to raise_error
       end
     end
-
-    context 'when build has a chat' do
-      let(:build) { create(:ci_build, :success, pipeline: create(:ci_pipeline, source: :chat)) }
-
-      it 'schedules a ChatNotification job' do
-        expect(ChatNotificationWorker).to receive(:perform_async).with(build.id)
-
-        subject
-      end
-    end
   end
 end

spec/workers/ci/merge_requests/add_todo_when_build_fails_worker_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Ci::MergeRequests::AddTodoWhenBuildFailsWorker do
+  describe '#perform' do
+    let_it_be(:project) { create(:project) }
+    let_it_be(:pipeline) { create(:ci_pipeline, :detached_merge_request_pipeline) }
+    let_it_be(:job) { create(:ci_build, project: project, pipeline: pipeline, status: :failed) }
+
+    let(:job_args) { job.id }
+
+    subject(:perform_twice) { perform_multiple(job_args, exec_times: 2) }
+
+    include_examples 'an idempotent worker' do
+      it 'executes todo service' do
+        service = double
+        expect(::MergeRequests::AddTodoWhenBuildFailsService).to receive(:new).with(project, nil).and_return(service).twice
+        expect(service).to receive(:execute).with(job).twice
+
+        perform_twice
+      end
+    end
+
+    context 'when job does not exist' do
+      let(:job_args) { 0 }
+
+      it 'returns nil' do
+        expect(described_class.new.perform(job_args)).to eq(nil)
+      end
+    end
+
+    context 'when project does not exist' do
+      before do
+        job.update!(project_id: nil)
+      end
+
+      it 'returns nil' do
+        expect(described_class.new.perform(job_args)).to eq(nil)
+      end
+    end
+
+    context 'when pipeline does not exist' do
+      before do
+        job.update_attribute('pipeline_id', nil)
+      end
+
+      it 'returns nil' do
+        expect(described_class.new.perform(job_args)).to eq(nil)
+      end
+    end
+  end
+end