Merge branch 'revert-c0d092b9' into 'master'

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

See merge request gitlab-org/gitlab!28979

app/services/merge_requests/merge_service.rb

@@ -27,7 +27,6 @@ module MergeRequests
           success
         end
       end
-
       log_info("Merge process finished on JID #{merge_jid} with state #{state}")
     rescue MergeError => e
       handle_merge_error(log_message: e.message, save_message_on_model: true)
@@ -55,7 +54,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'
         end
 

app/services/merge_requests/post_merge_service.rb

@@ -8,28 +8,17 @@ module MergeRequests
   #
   class PostMergeService < MergeRequests::BaseService
     def execute(merge_request)
-      # 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
-        # relatively 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
-
-      # These operations are idempotent so can be safely run multiple times
-      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)
       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
-      execute_hooks(merge_request, 'merge')
     end
 
     private

app/services/merge_requests/squash_service.rb

@@ -4,14 +4,12 @@ module MergeRequests
   class SquashService < MergeRequests::BaseService
     include Git::Logger
 
-    def idempotent?
-      true
-    end
-
     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
 
       if merge_request.squash_in_progress?
         return error(s_('MergeRequests|Squash task canceled: another squash is already in progress.'))
@@ -22,12 +20,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)
 

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

----
-title: Make MergeService idempotent
-merge_request: 24708
-author:
-type: changed

spec/services/merge_requests/merge_service_spec.rb

@@ -47,23 +47,6 @@ describe MergeRequests::MergeService do
         expect(note.note).to include 'merged'
       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',

spec/services/merge_requests/post_merge_service_spec.rb

@@ -17,6 +17,7 @@ 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')
 
       service = described_class.new(project, user, {})
 

spec/services/merge_requests/squash_service_spec.rb

@@ -137,24 +137,6 @@ describe MergeRequests::SquashService do
       include_examples 'the squash succeeds'
     end
 
-    context 'when the merge request has already been merged' do
-      let(:merge_request) { merge_request_with_one_commit }
-
-      it 'checks the side-effects for multiple calls' do
-        merge_request.mark_as_merged
-
-        expect(service).to be_idempotent
-        expect { IdempotentWorkerHelper::WORKER_EXEC_TIMES.times { service.execute } }.not_to raise_error
-      end
-
-      it 'idempotently returns a success' do
-        merge_request.mark_as_merged
-        result = service.execute
-
-        expect(result).to match(status: :success, squash_sha: merge_request.diff_head_sha)
-      end
-    end
-
     context 'git errors' do
       let(:merge_request) { merge_request_with_only_new_files }
       let(:error) { 'A test error' }