Move branch deletion on merge to async worker

A separate idempotent worker can be retried on fail
independently from the main MergeWorker

app/services/merge_requests/merge_service.rb

@@ -107,8 +107,7 @@ module MergeRequests
       log_info("Post merge finished on JID #{merge_jid} with state #{state}")
 
       if delete_source_branch?
-        ::Branches::DeleteService.new(@merge_request.source_project, branch_deletion_user)
-          .execute(merge_request.source_branch)
+        MergeRequests::DeleteSourceBranchWorker.perform_async(@merge_request.id, @merge_request.source_branch_sha, branch_deletion_user.id)
       end
     end
 

app/workers/all_queues.yml

@@ -1808,6 +1808,14 @@
   :weight: 1
   :idempotent: true
   :tags: []
+- :name: merge_requests_delete_source_branch
+  :feature_category: :source_code_management
+  :has_external_dependencies:
+  :urgency: :high
+  :resource_boundary: :unknown
+  :weight: 1
+  :idempotent: true
+  :tags: []
 - :name: metrics_dashboard_prune_old_annotations
   :feature_category: :metrics
   :has_external_dependencies:

app/workers/merge_requests/delete_source_branch_worker.rb

+# frozen_string_literal: true
+
+class MergeRequests::DeleteSourceBranchWorker
+  include ApplicationWorker
+
+  feature_category :source_code_management
+  urgency :high
+  idempotent!
+
+  def perform(merge_request_id, source_branch_sha, user_id)
+    merge_request = MergeRequest.find(merge_request_id)
+    user = User.find(user_id)
+
+    # Source branch changed while it's being removed
+    return if merge_request.source_branch_sha != source_branch_sha
+
+    ::Branches::DeleteService.new(merge_request.source_project, user)
+      .execute(merge_request.source_branch)
+  rescue ActiveRecord::RecordNotFound
+  end
+end

changelogs/unreleased/id-move-delete-branch-to-workers.yml

+---
+title: Move branch deletion on merge to async worker
+merge_request: 55390
+author:
+type: performance

config/sidekiq_queues.yml

@@ -204,6 +204,8 @@
   - 1
 - - merge_request_reset_approvals
   - 1
+- - merge_requests_delete_source_branch
+  - 1
 - - metrics_dashboard_prune_old_annotations
   - 1
 - - metrics_dashboard_sync_dashboards

spec/requests/api/merge_requests_spec.rb

@@ -2537,7 +2537,7 @@ RSpec.describe API::MergeRequests do
       end
     end
 
-    describe "the should_remove_source_branch param" do
+    describe "the should_remove_source_branch param", :sidekiq_inline do
       let(:source_repository) { merge_request.source_project.repository }
       let(:source_branch) { merge_request.source_branch }
 
@@ -2552,7 +2552,7 @@ RSpec.describe API::MergeRequests do
       end
     end
 
-    context "with a merge request that has force_remove_source_branch enabled" do
+    context "with a merge request that has force_remove_source_branch enabled", :sidekiq_inline do
       let(:source_repository) { merge_request.source_project.repository }
       let(:source_branch) { merge_request.source_branch }
 

spec/services/merge_requests/merge_service_spec.rb

@@ -258,9 +258,8 @@ RSpec.describe MergeRequests::MergeService do
           end
 
           it 'removes the source branch using the author user' do
-            expect(::Branches::DeleteService).to receive(:new)
-              .with(merge_request.source_project, merge_request.author)
-              .and_call_original
+            expect(::MergeRequests::DeleteSourceBranchWorker).to receive(:perform_async).with(merge_request.id, merge_request.source_branch_sha, merge_request.author.id)
+
             service.execute(merge_request)
           end
 
@@ -268,7 +267,8 @@ RSpec.describe MergeRequests::MergeService do
             let(:service) { described_class.new(project, user, merge_params.merge('should_remove_source_branch' => false)) }
 
             it 'does not delete the source branch' do
-              expect(::Branches::DeleteService).not_to receive(:new)
+              expect(::MergeRequests::DeleteSourceBranchWorker).not_to receive(:perform_async)
+
               service.execute(merge_request)
             end
           end
@@ -280,9 +280,8 @@ RSpec.describe MergeRequests::MergeService do
           end
 
           it 'removes the source branch using the current user' do
-            expect(::Branches::DeleteService).to receive(:new)
-              .with(merge_request.source_project, user)
-              .and_call_original
+            expect(::MergeRequests::DeleteSourceBranchWorker).to receive(:perform_async).with(merge_request.id, merge_request.source_branch_sha, user.id)
+
             service.execute(merge_request)
           end
         end

spec/workers/merge_requests/delete_source_branch_worker_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe MergeRequests::DeleteSourceBranchWorker do
+  let_it_be(:merge_request) { create(:merge_request) }
+  let_it_be(:user) { create(:user) }
+
+  let(:sha) { merge_request.source_branch_sha }
+  let(:worker) { described_class.new }
+
+  describe '#perform' do
+    context 'with a non-existing merge request' do
+      it 'does nothing' do
+        expect(::Branches::DeleteService).not_to receive(:new)
+
+        worker.perform(non_existing_record_id, sha, user.id)
+      end
+    end
+
+    context 'with a non-existing user' do
+      it 'does nothing' do
+        expect(::Branches::DeleteService).not_to receive(:new)
+
+        worker.perform(merge_request.id, sha, non_existing_record_id)
+      end
+    end
+
+    context 'with existing user and merge request' do
+      it 'calls service to delete source branch' do
+        expect_next_instance_of(::Branches::DeleteService) do |instance|
+          expect(instance).to receive(:execute).with(merge_request.source_branch)
+        end
+
+        worker.perform(merge_request.id, sha, user.id)
+      end
+
+      context 'source branch sha does not match' do
+        it 'does nothing' do
+          expect(::Branches::DeleteService).not_to receive(:new)
+
+          worker.perform(merge_request.id, 'new-source-branch-sha', user.id)
+        end
+      end
+    end
+
+    it_behaves_like 'an idempotent worker' do
+      let(:merge_request) { create(:merge_request) }
+      let(:job_args) { [merge_request.id, sha, user.id] }
+    end
+  end
+end

spec/workers/merge_worker_spec.rb

@@ -14,7 +14,7 @@ RSpec.describe MergeWorker do
       source_project.repository.expire_branches_cache
     end
 
-    it 'clears cache of source repo after removing source branch' do
+    it 'clears cache of source repo after removing source branch', :sidekiq_inline do
       expect(source_project.repository.branch_names).to include('markdown')
 
       described_class.new.perform(