From 100b98d14f2ef17dbfee4a5568233dde06398491 Mon Sep 17 00:00:00 2001 From: Phil Hughes Date: Tue, 14 Oct 2025 15:48:43 +0100 Subject: [PATCH] Prevent closing of merged merge requests Updates the merge request close service to always read from primary and reset the merge request object before attempting to close the merge request. https://gitlab.com/gitlab-org/gitlab/-/issues/510563 --- app/services/issuable_base_service.rb | 11 ++++- app/services/merge_requests/close_service.rb | 48 ++++++++++--------- .../ee/merge_requests/close_service.rb | 2 +- .../merge_requests/close_service_spec.rb | 17 +++++++ .../services/issuable_shared_examples.rb | 10 +++- .../services/merge_request_shared_examples.rb | 5 +- 6 files changed, 65 insertions(+), 28 deletions(-) diff --git a/app/services/issuable_base_service.rb b/app/services/issuable_base_service.rb index 89a48c598b6721..9ba6df5f490c7c 100644 --- a/app/services/issuable_base_service.rb +++ b/app/services/issuable_base_service.rb @@ -468,7 +468,8 @@ def change_additional_attributes(issuable) end def change_state(issuable) - case params.delete(:state_event) + state_event = params.delete(:state_event) + case state_event when 'reopen' service_class = reopen_service when 'close' @@ -476,7 +477,13 @@ def change_state(issuable) end if service_class - service_class.new(**service_class.constructor_container_arg(project), current_user: current_user).execute(issuable) + if issuable.is_a?(MergeRequest) && state_event == 'close' + service_class.new(**service_class.constructor_container_arg(project), current_user: current_user) + .execute(issuable, skip_reset: true) + else + service_class.new(**service_class.constructor_container_arg(project), current_user: current_user) + .execute(issuable) + end end end diff --git a/app/services/merge_requests/close_service.rb b/app/services/merge_requests/close_service.rb index b9d2098eba395b..dda1dc7523f88d 100644 --- a/app/services/merge_requests/close_service.rb +++ b/app/services/merge_requests/close_service.rb @@ -4,7 +4,7 @@ module MergeRequests class CloseService < MergeRequests::BaseService include RemovesRefs - def execute(merge_request, commit = nil) + def execute(merge_request, commit = nil, skip_reset: false) if Feature.enabled?(:destroy_fork_network_on_archive, project) unless params[:skip_authorization].present? || can?(current_user, :update_merge_request, merge_request) return merge_request @@ -13,28 +13,32 @@ def execute(merge_request, commit = nil) return merge_request unless can?(current_user, :update_merge_request, merge_request) end - # If we close MergeRequest we want to ignore validation - # so we can close broken one (Ex. fork project removed) - merge_request.allow_broken = true - - if merge_request.close - expire_unapproved_key(merge_request) - create_event(merge_request) - merge_request_activity_counter.track_close_mr_action(user: current_user) - create_note(merge_request) - notification_service.async.close_mr(merge_request, current_user) - todo_service.close_merge_request(merge_request, current_user) - execute_hooks(merge_request, 'close') - invalidate_all_users_cache_count(merge_request) - merge_request.invalidate_project_counter_caches - cleanup_environments(merge_request) - deactivate_pages_deployments(merge_request) - abort_auto_merge(merge_request, 'merge request was closed') - cleanup_refs(merge_request) - trigger_merge_request_merge_status_updated(merge_request) - end + ::Gitlab::Database::LoadBalancing::SessionMap.current(merge_request.load_balancer).use_primary do + merge_request.reset unless skip_reset + + # If we close MergeRequest we want to ignore validation + # so we can close broken one (Ex. fork project removed) + merge_request.allow_broken = true - merge_request + if merge_request.close + expire_unapproved_key(merge_request) + create_event(merge_request) + merge_request_activity_counter.track_close_mr_action(user: current_user) + create_note(merge_request) + notification_service.async.close_mr(merge_request, current_user) + todo_service.close_merge_request(merge_request, current_user) + execute_hooks(merge_request, 'close') + invalidate_all_users_cache_count(merge_request) + merge_request.invalidate_project_counter_caches + cleanup_environments(merge_request) + deactivate_pages_deployments(merge_request) + abort_auto_merge(merge_request, 'merge request was closed') + cleanup_refs(merge_request) + trigger_merge_request_merge_status_updated(merge_request) + end + + merge_request + end end private diff --git a/ee/app/services/ee/merge_requests/close_service.rb b/ee/app/services/ee/merge_requests/close_service.rb index 2d47a9d047144f..f8edf726d5d413 100644 --- a/ee/app/services/ee/merge_requests/close_service.rb +++ b/ee/app/services/ee/merge_requests/close_service.rb @@ -6,7 +6,7 @@ module CloseService extend ::Gitlab::Utils::Override override :execute - def execute(merge_request, commit = nil) + def execute(merge_request, commit = nil, skip_reset: false) super.tap do if current_user.project_bot? log_audit_event(merge_request, 'merge_request_closed_by_project_bot', diff --git a/spec/services/merge_requests/close_service_spec.rb b/spec/services/merge_requests/close_service_spec.rb index 55c9b3ff1d0b90..a524ef79be85c0 100644 --- a/spec/services/merge_requests/close_service_spec.rb +++ b/spec/services/merge_requests/close_service_spec.rb @@ -66,6 +66,23 @@ def execute end end + context 'when the merge request is merged concurrently in another process' do + it 'does not close the merge request' do + stale_instance = MergeRequest.find(merge_request.id) + fresh_instance = MergeRequest.find(merge_request.id) + + fresh_instance.mark_as_merged! + + result = service.execute(stale_instance) + + expect(result).to eq(stale_instance) + expect(result).to be_merged + expect(result).not_to be_closed + + expect(merge_request.reload).to be_merged + end + end + it 'updates metrics' do metrics = merge_request.metrics metrics_service = double(MergeRequestMetricsService) diff --git a/spec/support/shared_examples/services/issuable_shared_examples.rb b/spec/support/shared_examples/services/issuable_shared_examples.rb index 2357dabf350f8e..97c78a08d917e7 100644 --- a/spec/support/shared_examples/services/issuable_shared_examples.rb +++ b/spec/support/shared_examples/services/issuable_shared_examples.rb @@ -2,13 +2,19 @@ RSpec.shared_examples 'cache counters invalidator' do it 'invalidates counter cache for assignees' do - expect(merge_request.assignees).to all(receive(:invalidate_merge_request_cache_counts)) + assignees = merge_request.assignees.to_a + allow(merge_request).to receive(:assignees).and_return(assignees) + + expect(assignees).to all(receive(:invalidate_merge_request_cache_counts)) described_class.new(project: project, current_user: user).execute(merge_request) end it 'invalidates counter cache for author' do - expect(merge_request.author).to receive(:invalidate_merge_request_cache_counts) + author = merge_request.author + allow(merge_request).to receive(:author).and_return(author) + + expect(author).to receive(:invalidate_merge_request_cache_counts) described_class.new(project: project, current_user: user).execute(merge_request) end diff --git a/spec/support/shared_examples/services/merge_request_shared_examples.rb b/spec/support/shared_examples/services/merge_request_shared_examples.rb index 76526f1a99fe14..146100d40ff5a3 100644 --- a/spec/support/shared_examples/services/merge_request_shared_examples.rb +++ b/spec/support/shared_examples/services/merge_request_shared_examples.rb @@ -52,7 +52,10 @@ end it 'invalidates counter cache for reviewers' do - expect(merge_request.reviewers).to all(receive(:invalidate_merge_request_cache_counts)) + reviewers = merge_request.reviewers.to_a + allow(merge_request).to receive(:reviewers).and_return(reviewers) + + expect(reviewers).to all(receive(:invalidate_merge_request_cache_counts)) described_class.new(project: project, current_user: user).execute(merge_request) end -- GitLab