diff --git a/app/services/merge_requests/merge_service.rb b/app/services/merge_requests/merge_service.rb
index 961a7cb1ef69d9bc9fd7444c2691617d9d2223d2..e631bfc08e131324fcbe31cbb44d5180311cefa5 100644
--- a/app/services/merge_requests/merge_service.rb
+++ b/app/services/merge_requests/merge_service.rb
@@ -55,7 +55,7 @@ def error_check!
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.'
diff --git a/app/services/merge_requests/post_merge_service.rb b/app/services/merge_requests/post_merge_service.rb
index fdf8f442297efefa713c41c2204a6fcff6c1cc92..2d45f2aceb841c44636d9f3a0d4b17d709548146 100644
--- a/app/services/merge_requests/post_merge_service.rb
+++ b/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
diff --git a/app/services/merge_requests/squash_service.rb b/app/services/merge_requests/squash_service.rb
index faa2e9215812610eea0a60ecc6751f1913e8aeae..94ce7098d3a445c386286c281918ec49670e71ae 100644
--- a/app/services/merge_requests/squash_service.rb
+++ b/app/services/merge_requests/squash_service.rb
@@ -7,9 +7,7 @@ class SquashService < MergeRequests::BaseService
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 @@ def execute
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)
diff --git a/app/workers/all_queues.yml b/app/workers/all_queues.yml
index 5148772c8811748be8bbd47091fcf5287384b1a0..3d243cfd00c3bde65e0f3b6102c6661276653191 100644
--- a/app/workers/all_queues.yml
+++ b/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
diff --git a/app/workers/merge_worker.rb b/app/workers/merge_worker.rb
index 270bd831f96938c3f944908c67004540e5c1eb35..fbebaa925b4ee8e7ffdc148abb00ec72d5773d0f 100644
--- a/app/workers/merge_worker.rb
+++ b/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
diff --git a/changelogs/unreleased/32456-make-mergeservice-idempotent.yml b/changelogs/unreleased/32456-make-mergeservice-idempotent.yml
new file mode 100644
index 0000000000000000000000000000000000000000..62186122c69932a239e2d97302742a67aaad64ae
--- /dev/null
+++ b/changelogs/unreleased/32456-make-mergeservice-idempotent.yml
@@ -0,0 +1,5 @@
+---
+title: Make MergeService idempotent
+merge_request: 32456
+author:
+type: changed
diff --git a/spec/services/merge_requests/merge_service_spec.rb b/spec/services/merge_requests/merge_service_spec.rb
index 11e341994f76dbb02c981f46d5dca57b2b86145b..8f3d38bd334578e099cbc1ecd0c42790b82781aa 100644
--- a/spec/services/merge_requests/merge_service_spec.rb
+++ b/spec/services/merge_requests/merge_service_spec.rb
@@ -64,6 +64,23 @@
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 @@
.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
diff --git a/spec/services/merge_requests/post_merge_service_spec.rb b/spec/services/merge_requests/post_merge_service_spec.rb
index a51a896ca96481b7785cad9595cdc8a519e832f9..58e8c250d95fb5444b14cd99c5a726a2d1578bd3 100644
--- a/spec/services/merge_requests/post_merge_service_spec.rb
+++ b/spec/services/merge_requests/post_merge_service_spec.rb
@@ -19,7 +19,6 @@
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
diff --git a/spec/workers/merge_worker_spec.rb b/spec/workers/merge_worker_spec.rb
index 97e8aeb616e3c31940d795a65547192cecc56407..fc0508702251273a2eb0b332f7db8f510b71e943 100644
--- a/spec/workers/merge_worker_spec.rb
+++ b/spec/workers/merge_worker_spec.rb
@@ -29,5 +29,23 @@
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