Remove prevent_approval_removal_during_merge feature flag

Removes the prevent_approval_removal_during_merge feature flag now that it's at 100% on GitLab.com, making its behaviour the default. The flag prevented an approval from being removed while a merge request is locked (mid-merge), and re-checked approval state inside the merge lock right before the git write, closing a race where an MR could end up merged but unapproved.

Detailed context for AI agents

Background

prevent_approval_removal_during_merge was a gitlab_com_derisk flag introduced in milestone 19.2 by !243660 (merged). It is now enabled at 100% on GitLab.com, so this MR removes it and makes the guarded behaviour unconditional.

What the feature does (now unconditional)

It closes a race where a merge request could end up merged but unapproved, via two guards:

  1. MergeRequests::RemoveApprovalService (app/services/merge_requests/remove_approval_service.rb) refuses to remove an approval while the merge request is locked? — i.e. while the git merge is running inside MergeService#in_locked_state. The unapprove REST API surfaces this as a 404, mirroring the existing merged? guard.
  2. EE::MergeRequests::MergeService#ensure_approved! (ee/app/services/ee/merge_requests/merge_service.rb) re-reads approval state with a fresh cache (reset_approval_cache!) inside the merge lock, immediately before the git write, and aborts the merge with "Merge request is not approved" if the MR is no longer approved. Approval is also validated earlier in #validate!, but that runs before the MR is locked, hence this in-lock re-check.

Changes

  • app/services/merge_requests/remove_approval_service.rb: replaced the if Feature.enabled?(...) / else branch with a plain return if merge_request.locked?. This drops the flag-disabled fallback path, which used to allow the removal but call merge_request.log_approval_deletion_on_merged_or_locked_mr(source: 'MergeRequests::RemoveApprovalService', ...) to keep the merged-but-unapproved case observable. That call site is gone; the log_approval_deletion_on_merged_or_locked_mr model method itself stays, since three other call sites still use it (ee/app/services/ee/merge_requests/base_service.rb, ee/app/services/merge_requests/reset_approvals_service.rb, lib/api/merge_request_approvals.rb).
  • ee/app/services/ee/merge_requests/merge_service.rb: removed the return unless ::Feature.enabled?(...) guard from ensure_approved!. The approval_feature_available? guard remains, so the re-check is still skipped when approvals aren't in play.
  • Deleted config/feature_flags/gitlab_com_derisk/prevent_approval_removal_during_merge.yml.
  • Removed the three flag-disabled spec contexts (spec/services/merge_requests/remove_approval_service_spec.rb, spec/requests/api/merge_request_approvals_spec.rb, ee/spec/services/ee/merge_requests/merge_service_spec.rb). Flag-enabled coverage stays and is now the only path tested.
  • Net diff: 1 insertion, 71 deletions across 6 files.
  • Commit trailer: Changelog: fixed.

Verification

  • bundle exec rspec spec/services/merge_requests/remove_approval_service_spec.rb — 31 examples, 0 failures.
  • bundle exec rspec spec/requests/api/merge_request_approvals_spec.rb — 48 examples, 0 failures (4 pending, unrelated granular-token shared examples).
  • bundle exec rspec ee/spec/services/ee/merge_requests/merge_service_spec.rb — 43 examples, 4 failures, confirmed pre-existing (reproduce identically on unmodified master with changes stashed). They're local-environment Errno::ECONNREFUSED failures connecting to localhost:80 from post-merge webhook/Jira integration code, unrelated to this change.
  • bundle exec rubocop on all 5 changed Ruby files — no offenses.
  • Grep confirms zero remaining references to prevent_approval_removal_during_merge anywhere in the repo.

Rollback

No flag left to flip. Reverting this MR is the rollback path.

Follow-up noted during rollout (out of scope here)

For projects that require zero approvals, approval_feature_available? is still true but approved? is always true, so the reset_approval_cache! round-trip on the merge path is wasted work. Guarding the re-check with approvals_required > 0 would skip it. Not done here; MergeWorker performance was watched during rollout and the re-check did not show up as a problem.

Merge request reports

Loading
Loading