Block approval removal while a merge is in flight

Marc Shawrequested to merge
604469-prevent-approval-removal-during-in-flight-merge into master

What does this MR do and why?

Fixes a race where an approval revoked in the short window between auto-merge validating the MR and the git merge completing was silently dropped, producing a merged-but-unapproved MR — an audit gap for projects that require approval.

Root cause

When an MR requires approval and auto-merge is armed, the merge path reads approval state and performs the git merge as separate, non-atomic steps (MergeService#execute):

validate!            # approval re-check (from_source_branch mergeable?) — MR NOT yet locked
in_locked_state do   # MR becomes locked here
  commit             # git write
end

RemoveApprovalService only refused to remove an approval once the MR was already merged?. A reviewer who unapproved during the validate → commit window had their approval row deleted while the merge completed anyway.

The fix — close the race from both sides

  1. RemoveApprovalService refuses to remove an approval while the MR is locked? (the git merge runs inside in_locked_state), mirroring the existing merged? guard. Surfaced as a 404 by the API. This deliberately does not block earlier (e.g. while the merge job is merely enqueued) — unapproval stays possible right up until the merge is actually committing.

  2. EE::MergeRequests::MergeService re-reads approval with a fresh cache inside the lock, immediately before the git write, and aborts the merge if the MR is no longer approved. This catches an unapproval that landed before the lock was taken (the validate! → lock gap).

Together the approval row is held stable for the duration of the lock, and any unapproval that beat the lock is caught by the in-lock re-read. A sub-microsecond DB-commit race remains at the exact lock transition, but it is not user-reachable; eliminating it entirely would require serializing the two paths on a shared lock, which isn't warranted here.

Feature flag

Both changes are gated behind prevent_approval_removal_during_merge (gitlab_com_derisk, default off), with merge_request.project as the actor. When the flag is disabled the prior behaviour is preserved exactly: the locked? guard and the in-lock re-check are skipped, and RemoveApprovalService still logs the deletion via log_approval_deletion_on_merged_or_locked_mr. This gives an instant, code-free rollback and an incremental GitLab.com rollout.

  • Rollout issue: #605128
  • No changelog entry: the change is behind a default-off flag.

Notes for reviewer

  • An earlier revision guarded on the broader merge_ongoing?, but that blocks unapproval from the moment the merge job is enqueued (the Sidekiq queue gap) — earlier than necessary. locked? + the in-lock re-check keeps the block tight to the actual merge window.

Testing

  • spec/services/merge_requests/remove_approval_service_spec.rb — a locked MR blocks removal (no-op, returns nil → 404); a stub_feature_flags(...: false) context asserts the prior behaviour (removal allowed and logged).
  • ee/spec/services/ee/merge_requests/merge_service_spec.rb — the in-lock re-check aborts the merge when the MR is no longer approved, is skipped when approvals are not required, and is a no-op when the flag is disabled.
  • Verified end-to-end on GDK: open MR removal proceeds; locked MR blocked; a merge job that is enqueued-but-not-locked no longer blocks (protection moved to the merge side).

Manual testing — with vs without merge trains

Both paths converge on MergeRequests::MergeService#execute → in_locked_state → commit, so the in-lock re-check (ensure_approved!) fires on both direct and merge-train merges. What differs is the size of the unapproval window:

  • Direct / auto-merge: the MR is locked? almost immediately, so the only gap is the sub-second validate! → lock window. The RemoveApprovalService locked? guard covers the lock; the in-lock re-check covers the gap before it.
  • Merge train: the MR stays opened (not locked?) for the entire train pipeline run — potentially minutes. Approval removal is intentionally still allowed during that time (the locked? guard does not apply on the train), so the in-lock re-check at the final commit is what prevents a merged-but-unapproved result.

Common setup: EE GDK, a project with a rule requiring ≥1 approval; MR author plus a separate approver; the approver approves the MR.

Without merge trains

  1. Flag on. Simulate the merge window by locking the MR (rails console: mr.lock_mr, or set a breakpoint inside in_locked_state).
  2. As the approver, DELETE /projects/:id/merge_requests/:iid/approvals → 404 (removal refused while locked).
  3. mr.unlock_mr and retry the DELETE → 200 (removal allowed when not locked).
  4. Flag off, lock again, DELETE → 200 plus a log_approval_deletion_on_merged_or_locked_mr log line (prior behaviour preserved).

With merge trains (enable merge trains on the project: Settings → Merge requests)

  1. Flag on. Approve, then add the MR to the train. It is now opened with a running train pipeline — confirm mr.locked? is false.
  2. While the pipeline runs, DELETE .../approvals → 200 (allowed; the MR isn't locked yet, so the guard deliberately doesn't fire here).
  3. Let the train reach its merge step. Expected: the in-lock re-check aborts the merge — the MR is not merged and drops off the train with merge error Merge request is not approved (mr.merged? == false).
  4. Flag off, repeat 1–3: the MR merges unapproved, reproducing the merged-but-unapproved audit gap this MR closes.

References

Edited by Marc Shaw

Merge request reports