Move approval reset to new service

Separates the approval reset into a new service, which is then
executed in a new worker in order to separate it from the
existing flow.

config/sidekiq_queues.yml

@@ -160,6 +160,8 @@
   - 1
 - - merge_request_mergeability_check
   - 1
+- - merge_request_reset_approvals
+  - 3
 - - metrics_dashboard_prune_old_annotations
   - 1
 - - migrate_external_diffs

ee/app/services/ee/merge_requests/refresh_service.rb

@@ -20,12 +20,7 @@ module EE
 
       # Note: Closed merge requests also need approvals reset.
       def reset_approvals_for_merge_requests(ref, newrev)
-        branch_name = ::Gitlab::Git.ref_name(ref)
-        merge_requests = merge_requests_for(branch_name, mr_states: [:opened, :closed])
-
-        merge_requests.each do |merge_request|
-          reset_approvals(merge_request, newrev)
-        end
+        MergeRequestResetApprovalsWorker.perform_async(project.id, current_user.id, ref, newrev)
       end
 
       # @return [Hash<Integer, MergeRequestDiff>] Diffs prior to code push, mapped from merge request id
@@ -78,10 +73,6 @@ module EE
           .including_merge_train
       end
       # rubocop:enable Gitlab/ModuleWithInstanceVariables
-
-      def reset_approvals?(merge_request, newrev)
-        super && merge_request.rebase_commit_sha != newrev
-      end
     end
   end
 end

ee/app/services/ee/merge_requests/reset_approvals_service.rb

+# frozen_string_literal: true
+
+module EE
+  module MergeRequests
+    class ResetApprovalsService < ::MergeRequests::BaseService
+      def execute(ref, newrev)
+        reset_approvals_for_merge_requests(ref, newrev)
+      end
+
+      private
+
+      # Note: Closed merge requests also need approvals reset.
+      def reset_approvals_for_merge_requests(ref, newrev)
+        branch_name = ::Gitlab::Git.ref_name(ref)
+        merge_requests = merge_requests_for(branch_name, mr_states: [:opened, :closed])
+
+        merge_requests.each do |merge_request|
+          reset_approvals(merge_request, newrev)
+        end
+      end
+
+      def reset_approvals?(merge_request, newrev)
+        super && merge_request.rebase_commit_sha != newrev
+      end
+    end
+  end
+end

ee/app/workers/all_queues.yml

@@ -635,6 +635,14 @@
   :weight: 2
   :idempotent:
   :tags: []
+- :name: merge_request_reset_approvals
+  :feature_category: :source_code_management
+  :has_external_dependencies:
+  :urgency: :high
+  :resource_boundary: :cpu
+  :weight: 3
+  :idempotent:
+  :tags: []
 - :name: new_epic
   :feature_category: :epics
   :has_external_dependencies:

ee/app/workers/merge_request_reset_approvals_worker.rb

+# frozen_string_literal: true
+
+class MergeRequestResetApprovalsWorker # rubocop:disable Scalability/IdempotentWorker
+  include ApplicationWorker
+
+  feature_category :source_code_management
+  urgency :high
+  worker_resource_boundary :cpu
+  weight 3
+  loggable_arguments 2, 3
+
+  LOG_TIME_THRESHOLD = 90 # seconds
+
+  # rubocop: disable CodeReuse/ActiveRecord
+  def perform(project_id, user_id, ref, newrev)
+    project = Project.find_by(id: project_id)
+    return unless project
+
+    user = User.find_by(id: user_id)
+    return unless user
+
+    EE::MergeRequests::ResetApprovalsService.new(project, user).execute(ref, newrev)
+  end
+  # rubocop: enable CodeReuse/ActiveRecord
+end

ee/changelogs/unreleased/refresh-service-optimisations.yml

+---
+title: Move approval reset to new service and worker
+merge_request: 42001
+author:
+type: performance

ee/spec/services/ee/merge_requests/refresh_service_spec.rb

@@ -13,6 +13,7 @@ RSpec.describe MergeRequests::RefreshService do
   let(:fork_user) { create(:user) }
 
   let(:source_branch) { 'between-create-delete-modify-move' }
+
   let(:merge_request) do
     create(:merge_request,
       source_project: project,
@@ -353,6 +354,7 @@ RSpec.describe MergeRequests::RefreshService do
         it 'resets approvals' do
           service.execute(oldrev, newrev, 'refs/heads/master')
           reload_mrs
+          MergeRequestResetApprovalsWorker.drain
 
           expect(merge_request.approvals).to be_empty
           expect(forked_merge_request.approvals).not_to be_empty
@@ -451,6 +453,7 @@ RSpec.describe MergeRequests::RefreshService do
             allow(service).to receive(:execute_hooks)
             service.execute(oldrev, newrev, 'refs/heads/master')
             reload_mrs
+            MergeRequestResetApprovalsWorker.drain
           end
 
           it 'resets approvals' do
@@ -494,6 +497,7 @@ RSpec.describe MergeRequests::RefreshService do
               allow(service).to receive(:execute_hooks)
               service.execute(oldrev, newrev, 'refs/heads/master')
               reload_mrs
+              MergeRequestResetApprovalsWorker.drain
             end
 
             it 'resets the approvals' do
@@ -507,6 +511,7 @@ RSpec.describe MergeRequests::RefreshService do
               allow(service).to receive(:execute_hooks)
               service.execute(oldrev, newrev, 'refs/heads/master')
               reload_mrs
+              MergeRequestResetApprovalsWorker.drain
             end
 
             it 'resets the approvals' do

ee/spec/services/ee/merge_requests/reset_approvals_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe EE::MergeRequests::ResetApprovalsService do
+  include ProjectForksHelper
+  include ProjectHelpers
+
+  let(:service) { described_class.new(project, current_user) }
+  let(:current_user) { merge_request.author }
+  let(:group) { create(:group) }
+  let(:user) { create(:user) }
+  let(:project) { create(:project, :repository, namespace: group, approvals_before_merge: 1, reset_approvals_on_push: true) }
+
+  let(:merge_request) do
+    create(:merge_request,
+      source_project: project,
+      source_branch: 'master',
+      target_branch: 'feature',
+      target_project: project,
+      merge_when_pipeline_succeeds: true,
+      merge_user: user)
+  end
+
+  let(:commits) { merge_request.commits }
+  let(:oldrev) { commits.last.id }
+  let(:newrev) { commits.first.id }
+  let(:approver) { create(:user) }
+  let(:notification_service) { spy('notification_service') }
+
+  def approval_todos(merge_request)
+    Todo.where(action: Todo::APPROVAL_REQUIRED, target: merge_request)
+  end
+
+  describe "#execute" do
+    before do
+      merge_request.approvals.create!(user_id: user.id)
+      allow(service).to receive(:execute_hooks)
+      allow(NotificationService).to receive(:new) { notification_service }
+      project.add_developer(approver)
+
+      perform_enqueued_jobs do
+        merge_request.update!(approver_ids: [approver].map(&:id).join(','))
+      end
+    end
+
+    it 'resets approvals' do
+      service.execute("refs/heads/master", newrev)
+      merge_request.reload
+
+      expect(merge_request.approvals).to be_empty
+      expect(approval_todos(merge_request).map(&:user)).to contain_exactly(approver)
+    end
+  end
+end

ee/spec/workers/merge_request_reset_approvals_worker_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe MergeRequestResetApprovalsWorker do
+  include RepoHelpers
+
+  let(:project) { create(:project, :repository) }
+  let(:user) { create(:user) }
+
+  subject { described_class.new }
+
+  describe '#perform' do
+    let(:newrev) { "789012" }
+    let(:ref)    { "refs/heads/test" }
+
+    def perform
+      subject.perform(project.id, user.id, ref, newrev)
+    end
+
+    it 'executes MergeRequests::RefreshService with expected values' do
+      expect_next_instance_of(EE::MergeRequests::ResetApprovalsService, project, user) do |refresh_service|
+        expect(refresh_service).to receive(:execute).with(ref, newrev)
+      end
+
+      perform
+    end
+  end
+end