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

This reverts merge request !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.merged? && !@merge_request.mergeable?
+        elsif !@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,31 +8,18 @@ module MergeRequests
   #
   class PostMergeService < MergeRequests::BaseService
     def execute(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)
+      close_issues(merge_request)
       todo_service.merge_merge_request(merge_request, current_user)
-      end
-
-      notification_service.merge_mr(merge_request, current_user)
+      create_event(merge_request)
       create_note(merge_request)
-      close_issues(merge_request)
+      notification_service.merge_mr(merge_request, current_user)
+      execute_hooks(merge_request, 'merge')
       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,7 +7,9 @@ module MergeRequests
     def execute
       # If performing a squash would result in no change, then
       # immediately return a success message without performing a squash
-      return success(squash_sha: merge_request.diff_head_sha) if squash_redundant?
+      if merge_request.commits_count < 2 && message.nil?
+        return success(squash_sha: merge_request.diff_head_sha)
+      end
 
       return error(s_('MergeRequests|This project does not allow squashing commits when merge requests are accepted.')) if squash_forbidden?
 
@@ -23,12 +25,6 @@ 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

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

app/workers/merge_worker.rb

@@ -7,7 +7,6 @@ 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,23 +64,6 @@ 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',
@@ -305,27 +288,6 @@ 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,6 +19,7 @@ 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,23 +29,5 @@ 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