Merge branch '32737-make-mergeservice-idempotent-2' into 'master'

Make MergeService idempotent and prevent duplicate notifications

Closes #32737

See merge request gitlab-org/gitlab!32456

app/services/merge_requests/merge_service.rb

@@ -55,7 +55,7 @@ module MergeRequests
       error =
         if @merge_request.should_be_rebased?
           'Only fast-forward merge is allowed for your project. Please update your source branch'
-        elsif !@merge_request.mergeable?
+        elsif !@merge_request.merged? && !@merge_request.mergeable?
           'Merge request is not mergeable'
         elsif !@merge_request.squash && project.squash_always?
           'This project requires squashing commits when merge requests are accepted.'

app/services/merge_requests/post_merge_service.rb

@@ -8,18 +8,31 @@ module MergeRequests
   #
   class PostMergeService < MergeRequests::BaseService
     def execute(merge_request)
-      merge_request.mark_as_merged
-      close_issues(merge_request)
-      todo_service.merge_merge_request(merge_request, current_user)
-      create_event(merge_request)
-      create_note(merge_request)
+      return if merge_request.merged?
+
+      # These operations need to happen transactionally
+      ActiveRecord::Base.transaction(requires_new: true) do
+        merge_request.mark_as_merged
+
+        # These options do not call external services and should be
+        # quick enough to put in a transaction
+        create_event(merge_request)
+        todo_service.merge_merge_request(merge_request, current_user)
+      end
+
       notification_service.merge_mr(merge_request, current_user)
-      execute_hooks(merge_request, 'merge')
+      create_note(merge_request)
+      close_issues(merge_request)
       invalidate_cache_counts(merge_request, users: merge_request.assignees)
       merge_request.update_project_counter_caches
       delete_non_latest_diffs(merge_request)
       cancel_review_app_jobs!(merge_request)
       cleanup_environments(merge_request)
+
+      # Anything after this point will be executed at-most-once. Less important activity only
+      # TODO: make all the work in here a separate sidekiq job so it can go in the transaction
+      # Issue: https://gitlab.com/gitlab-org/gitlab/-/issues/228803
+      execute_hooks(merge_request, 'merge')
     end
 
     private

app/services/merge_requests/squash_service.rb

@@ -7,9 +7,7 @@ module MergeRequests
     def execute
       # If performing a squash would result in no change, then
       # immediately return a success message without performing a squash
-      if merge_request.commits_count < 2 && message.nil?
-        return success(squash_sha: merge_request.diff_head_sha)
-      end
+      return success(squash_sha: merge_request.diff_head_sha) if squash_redundant?
 
       return error(s_('MergeRequests|This project does not allow squashing commits when merge requests are accepted.')) if squash_forbidden?
 
@@ -25,6 +23,12 @@ module MergeRequests
 
     private
 
+    def squash_redundant?
+      return true if merge_request.merged?
+
+      merge_request.commits_count < 2 && message.nil?
+    end
+
     def squash!
       squash_sha = repository.squash(current_user, merge_request, message || merge_request.default_squash_commit_message)
 

app/workers/all_queues.yml

@@ -1426,7 +1426,7 @@
   :urgency: :high
   :resource_boundary: :unknown
   :weight: 5
-  :idempotent: 
+  :idempotent: true
   :tags: []
 - :name: merge_request_mergeability_check
   :feature_category: :source_code_management

app/workers/merge_worker.rb

@@ -7,6 +7,7 @@ class MergeWorker # rubocop:disable Scalability/IdempotentWorker
   urgency :high
   weight 5
   loggable_arguments 2
+  idempotent!
 
   def perform(merge_request_id, current_user_id, params)
     params = params.with_indifferent_access

changelogs/unreleased/32456-make-mergeservice-idempotent.yml

+---
+title: Make MergeService idempotent
+merge_request: 32456
+author:
+type: changed

spec/services/merge_requests/merge_service_spec.rb

@@ -64,6 +64,23 @@ RSpec.describe MergeRequests::MergeService do
         end
       end
 
+      it 'is idempotent' do
+        repository = project.repository
+        commit_count = repository.commit_count
+        merge_commit = merge_request.merge_commit.id
+
+        # a first invocation of execute is performed on the before block
+        service.execute(merge_request)
+
+        expect(merge_request.merge_error).to be_falsey
+        expect(merge_request).to be_valid
+        expect(merge_request).to be_merged
+
+        expect(repository.commits_by(oids: [merge_commit]).size).to eq(1)
+        expect(repository.commit_count).to eq(commit_count)
+        expect(merge_request.in_progress_merge_commit_sha).to be_nil
+      end
+
       context 'when squashing' do
         let(:merge_params) do
           { commit_message: 'Merge commit message',
@@ -288,6 +305,27 @@ RSpec.describe MergeRequests::MergeService do
               .and_call_original
             service.execute(merge_request)
           end
+
+          it 'does not fail to be idempotent when there is a Gitaly error' do
+            # This arose from an issue where Gitaly failed at a certain point
+            # and MergeService kept running PostMergeService and creating
+            # additional notifications. This spec makes sure if a Gitaly error
+            # does happen, MergeService will just quietly keep trying until
+            # the branch is removed.
+            # https://gitlab.com/gitlab-org/gitlab/-/issues/213620#note_331782036
+
+            # This simulates a Gitaly error when trying to delete a branch
+            expect_any_instance_of(Gitlab::GitalyClient::OperationService)
+              .to receive(:user_delete_branch).exactly(4).times
+              .and_raise(GRPC::FailedPrecondition)
+
+            # Only one notification should be sent out:
+            expect(NotificationRecipients::BuildService)
+              .to receive(:build_recipients)
+              .exactly(:once).and_call_original
+
+            4.times { expect { service.execute(merge_request) }.to raise_error(Gitlab::Git::CommandError) }
+          end
         end
       end
     end

spec/services/merge_requests/post_merge_service_spec.rb

@@ -19,7 +19,6 @@ RSpec.describe MergeRequests::PostMergeService do
     it 'refreshes the number of open merge requests for a valid MR', :use_clean_rails_memory_store_caching do
       # Cache the counter before the MR changed state.
       project.open_merge_requests_count
-      merge_request.update!(state: 'merged')
 
       expect { subject }.to change { project.open_merge_requests_count }.from(1).to(0)
     end

spec/workers/merge_worker_spec.rb

@@ -29,5 +29,23 @@ RSpec.describe MergeWorker do
       source_project.repository.expire_branches_cache
       expect(source_project.repository.branch_names).not_to include('markdown')
     end
+
+    it_behaves_like 'an idempotent worker' do
+      let(:job_args) do
+        [
+          merge_request.id,
+          merge_request.author_id,
+          commit_message: 'wow such merge',
+          sha: merge_request.diff_head_sha
+        ]
+      end
+
+      it 'the merge request is still shown as merged' do
+        subject
+
+        merge_request.reload
+        expect(merge_request).to be_merged
+      end
+    end
   end
 end