From 78300c1f0441683e927967ce4b5637a0c40cad8a 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/merge_requests/base_service.rb | 12 +++++ app/services/merge_requests/close_service.rb | 54 +++++++++++-------- .../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, 74 insertions(+), 26 deletions(-) diff --git a/app/services/merge_requests/base_service.rb b/app/services/merge_requests/base_service.rb index 02e36cb544a035..97e0e83806a6fb 100644 --- a/app/services/merge_requests/base_service.rb +++ b/app/services/merge_requests/base_service.rb @@ -330,6 +330,18 @@ def duo_code_review_bot def invalidate_all_users_cache_count(merge_request) invalidate_cache_counts(merge_request, users: [*merge_request.assignees, *merge_request.reviewers, merge_request.author].uniq) end + + override :change_state + def change_state(merge_request) + case params[:state_event] + when 'close' + close_service.new(**close_service.constructor_container_arg(project), current_user: current_user) + .execute(merge_request, skip_reset: true) + params.delete(:state_event) + else + super + end + end end end diff --git a/app/services/merge_requests/close_service.rb b/app/services/merge_requests/close_service.rb index b9d2098eba395b..6707eaff17ae98 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,32 +13,42 @@ 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 + use_primary(merge_request, skip_reset) do |merge_request| + # 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 - merge_request + merge_request + end end private + def use_primary(merge_request, skip_reset) + return yield(merge_request) if skip_reset + + ::Gitlab::Database::LoadBalancing::SessionMap.current(merge_request.load_balancer).use_primary do + yield(merge_request.reset) + end + end + def create_event(merge_request) # Making sure MergeRequest::Metrics updates are in sync with # Event creation. 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