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.
- Rollout issue: #605128 (closed)
- Feature issue: #604469 (closed)
What the feature does (now unconditional)
It closes a race where a merge request could end up merged but unapproved, via two guards:
MergeRequests::RemoveApprovalService(app/services/merge_requests/remove_approval_service.rb) refuses to remove an approval while the merge request islocked?— i.e. while the git merge is running insideMergeService#in_locked_state. The unapprove REST API surfaces this as a 404, mirroring the existingmerged?guard.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 theif Feature.enabled?(...) / elsebranch with a plainreturn if merge_request.locked?. This drops the flag-disabled fallback path, which used to allow the removal but callmerge_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; thelog_approval_deletion_on_merged_or_locked_mrmodel 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 thereturn unless ::Feature.enabled?(...)guard fromensure_approved!. Theapproval_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-environmentErrno::ECONNREFUSEDfailures connecting to localhost:80 from post-merge webhook/Jira integration code, unrelated to this change.bundle exec rubocopon all 5 changed Ruby files — no offenses.- Grep confirms zero remaining references to
prevent_approval_removal_during_mergeanywhere 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.