Record the approval patch ID from the asserted head SHA

An approval given before a new merge request's diff row exists is stored with a nil patch_id_sha. The approval reset reads nil as invalid, so the next push deletes that approval even though the diff never changed - the reporter sees "approved", then reset approvals from ... by pushing to the branch moments later.

Rather than derive a patch ID on the approver's behalf, this uses the sha the approve API already accepts. That parameter is the caller naming which head it is approving, and check_sha_param! has already rejected the request if it disagrees with the merge request, so it is the one input that can stand in for the missing diff row. Approvals sent without sha - which is every UI approval - behave exactly as they do today.

It is therefore opt-in: the reporter's script needs to start sending sha to benefit. That is deliberate, see below. Behind patch_id_sha_fallback_when_diff_missing, default off.

This replaces the unconditional derivation this MR started with, following the thread with @dskim_gitlab and @phikai.

Related to #395506

Detailed context for AI agents

What changed

• MergeRequest#current_patch_id_sha is back to its master form: it reads the diff row and nothing else. MergeRequests::ResetApprovalsService is its other caller, so the reset path is byte-for-byte unchanged with the flag on or off. The flag now only affects the approve path.
• New MergeRequest#patch_id_sha_for_head(head_sha) resolves the merge base for a given head and returns get_patch_id(base, head). Same-project merge requests resolve in the target repository; forks resolve in the source repository.
• MergeRequests::ApprovalService checks the flag first and, when it is off, does exactly what master does: stamp current_patch_id_sha. With the flag on it stamps the diff-derived value whenever the diff is still for the asserted sha, and only falls back to patch_id_sha_for_head(params[:sha]) when there is no diff or the diff head moved under it. Zero extra Gitaly calls on the normal path. The flag lives only here; the model method itself is unflagged.
• Mcp::Tools::MergeRequests::SaveMergeRequestReviewService#perform_approve already accepted an optional sha and validated it against the head, then discarded it. It now forwards it, so the MCP approve tool gets the same protection.
• patch_id_sha_for_head now returns early if target_branch_sha is blank, before the merge_base call. Without the guard, a target branch deleted while the merge request is still open leaves target_branch_sha nil, encode_binary(nil) sends Gitaly an empty revision, Gitaly answers INVALID_ARGUMENT, and Gitlab::Git::WrapsGitalyErrors maps that to a bare ArgumentError that nothing on the approve path rescues - approval would have failed outright instead of just skipping the fallback.
• doc/api/merge_request_approvals.md gets a > [!flag] note marking the new sha behavior as flag-gated, worded to match the existing approval_group_rules note further down the file. It sits directly below the paragraph it gates rather than at the top of the section, since the flag only gates that paragraph, not the whole approve endpoint.

Root cause

MergeRequests::CreateService sets skip_ensure_merge_request_diff = true and calls mark_as_preparing, deferring diff creation to NewMergeRequestWorker -> MergeRequests::AfterCreateService#ensure_merge_request_diff. So a freshly created merge request has zero merge_request_diffs rows for a short window.

In that window:

1. MergeRequest#merge_request_diff returns MergeRequestDiff.new(merge_request_id: id) - an unpersisted record.
2. MergeRequestDiff#set_patch_id_sha bails on return unless base_commit_sha && head_commit_sha, both nil on a new record, so get_patch_id_sha returns nil.
3. MergeRequests::ApprovalService stamps that nil onto the approval.
4. Approval.with_invalid_patch_id_sha is where.not(patch_id_sha: X).or(where(patch_id_sha: nil)), so a nil approval is always considered invalid.
5. MergeRequests::ResetApprovalsService -> delete_approvals deletes it, even though the patch ID never changed.

There is no preparing? guard on the approve path, so the approval succeeds and posts its system note.

Why it is intermittent

Git::BranchPushService#enqueue_update_mrs returns early when params[:merge_request_branches] excludes the pushed branch. That list comes from MergeRequests::PushedBranchesService, evaluated inside Repositories::PostReceiveWorker. If the branch had no merge request when PostReceive ran, UpdateMergeRequestsWorker is never enqueued and the whole refresh/reset chain is skipped.

Losing the approval therefore needs two races to coincide: PostReceive has to land after merge request creation for the reset to be scheduled at all, and the approval has to land before the first diff row exists to be stamped nil. That dependence on Sidekiq queue latency is why it reproduces on GitLab.com under load but not on an idle instance, and why an attempt to reproduce it on production in 2024 came up empty. It is also why adding sleep 60 between the commit and the merge request creation makes it go away: PostReceive finishes first, so nothing enqueues a reset.

Why the first approach was dropped

The first revision of this MR computed the patch ID from diff_head_sha and diff_base_sha whenever the diff row was missing, with no input from the caller. Review raised the right objection: if there is no diff, what did the approver approve? Deriving the value from the branch answers a different question - "whatever the branch points at right now".

It also generalises badly, and the generalisation is the reason to prefer sha. There is a second, wider window with the same symptom:

1. A merge request already has diff row D1 (head A, patch ID P1).
2. Someone pushes commit B.
3. Before RefreshService#reload_merge_requests creates D2, someone approves. merge_request_diff is still D1, so the approval is stamped P1.
4. execute_async_workers runs after reload_merge_requests, so by the time MergeRequestResetApprovalsWorker fires (+10s) current_patch_id_sha is P2. P1 != P2, approval deleted.

That window is PostReceive latency plus UpdateMergeRequestsWorker latency plus the refresh itself, and it applies to every open merge request rather than only freshly created ones. Extending the derive-from-the-branch approach to cover it would mean stamping approvals with content the approver may never have seen: push a commit, catch a reviewer whose page is stale, and the approval that lands is recorded against the new content and survives the reset that was supposed to clear it. That inverts the purpose of recording the patch ID at all (!131205 (merged), !132000 (merged)), and this area already has security history (!3577 (merged), reset all approvals when the target branch changes).

An asserted sha sidesteps the question rather than answering it wrongly. The caller states what it is approving; API::Helpers#check_sha_param! rejects the request if that disagrees with the merge request; only then is deriving the patch ID a record of the approval rather than a guess about it.

Note the stale-diff case above already behaves correctly for a client that sends sha: diff_head_sha is still D1's head, so asserting B returns 409 Conflict instead of an approval that is quietly doomed. This MR does not change that path.

Why opt-in is the honest shape, and what it costs

The reporter's script does not send sha, so this does not fix an unmodified script, and the UI does not send it either (approvals.vue posts only publish_review). What it does is make a correct way to do this exist, and document it.

The alternative - stamping every approval from the live branch head - fixes the reporter without a client change, at the price of the review-integrity regression described above, on every approval in the product. That trade seems clearly wrong, but it is the trade, and it is the thing to disagree with if you disagree with this MR.

Two other options were considered and rejected:

• Block approval until the merge request is prepared (@dskim_gitlab's suggestion). Cleanest semantics, since it removes the "nothing to stamp" state instead of filling it in. Rejected here only because it is a breaking API change for existing automation, which was @phikai's concern; it is not incompatible with this MR and would make the fallback dead code.
• Treat nil and the first patch ID as equal on reset (@phikai's suggestion). The IS NULL clause in Approval.with_invalid_patch_id_sha is deliberate: it covers approvals created before the column shipped in 16.5, and Gitlab::Import::MergeRequestHelpers#create_approval!, which always writes nil for imported approvals. Treating nil as valid would resurrect stale approvals on those records.

Forks

For a fork, diff_base_sha goes through branch_merge_base_commit -> target_project.merge_base_commit(...), but the target repository has no copy of the source head until create_merge_request_diff runs fetch_ref!. So the merge base cannot be resolved there at all, and patch_id_sha_for_head resolves it in the source repository, which is the only side holding both SHAs. Measured against a real fork, that produces exactly the patch ID the diff goes on to report.

Known limit: this only works while the fork can still see the target branch head. Advancing the target by one commit past the fork point was enough for merge_base to return nil locally, at which point the value falls back to nil as before. Closing that fully would need fetch_ref! on the approve path, which is a write and out of scope here. No error path either way: Repository#merge_base returns nil for an unknown revision rather than raising, and Repository#get_patch_id already rescues Gitaly errors to nil.

Cost

Measured with Gitlab::GitalyClient.get_request_count under :request_store:

Case	Gitaly calls
Approve, diff row exists (the normal path)	0
patch_id_sha_for_head inside the no-diff-row window	3

Three, not the four the earlier revision cost: the caller supplies the head, so there is no source_branch_head lookup. What remains is target_branch_head, merge_base and get_patch_id.

The population paying it is also much smaller than before. It is charged only to approvals that land inside the no-diff-row window and assert a sha, rather than to every approval landing in that window. Everything else - any approval where the diff row exists, which is effectively all of them - reads the persisted patch_id_sha column and issues no Gitaly calls at all, exactly as on master. That 0 is measured, not assumed.

MergeRequests::ResetApprovalsService no longer reaches the fallback under any circumstances, so the reset side's cost is unchanged by the flag.

Deliberately out of scope

1. Reset-side fail-open. MergeRequests::ResetApprovalsService skips filter_approvals entirely when its own mr_patch_id_sha is nil (EE::MergeRequests::BaseService#delete_approvals only filters if patch_id_sha.present?), so delete_approvals falls through to an unfiltered approvals.delete_all. Repository#get_patch_id rescues Gitlab::Git::CommandError and CommandTimedOut to nil, so a transient Gitaly error silently converts "delete invalid approvals" into "delete every approval". Confirmed locally: a correctly stamped, valid approval was deleted this way. This is arguably the larger hole and wants its own issue.

2. Reset scoping. The reset is keyed on (ref, newrev) and resolves its merge requests at execute time, roughly ten seconds later. A merge request created after the push but before the reset runs is swept in even though the push could not have changed it - in the failing runs the pre-refresh and post-refresh patch IDs were identical and the merge request still got an "added 1 commit" note. Deciding the reset per merge request from its own patch-ID transition across the push would be a stronger fix and would also cover the legacy and imported nil approvals. It touches the selective code owner removal path and skip_reset_checks, so it is not folded in here.

Also unchanged: the target-branch-change reset in EE::MergeRequests::UpdateService#delete_approvals_on_target_branch_change still calls delete_approvals with no patch ID, so it keeps wiping all approvals unconditionally. That behaviour is required by the earlier security work and this MR does not touch it.

Verification

Automated, all passing locally:

• spec/models/merge_request_spec.rb, #current_patch_id_sha and #patch_id_sha_for_head - 10 examples. Covers same-project and fork resolution, the derived value matching what the diff goes on to report, no diff row persisted as a side effect, a blank head SHA, a head SHA that is already the merge base, a fork that can no longer see the target head, and a missing source project.
• spec/services/merge_requests/approval_service_spec.rb - the diff-derived value is preferred whenever the diff is for the asserted SHA (asserting patch_id_sha_for_head is never called), the asserted SHA is used when there is no diff or the diff head differs, and with the flag off patch_id_sha_for_head is never called - the diff value is stamped, or nil when there is none.
• ee/spec/services/merge_requests/reset_approvals_service_spec.rb - a regression context that approves while the merge request is still preparing and asserts the approval survives the reset, plus two negative cases: no asserted sha, and the flag off. Both still delete.
• spec/services/mcp/tools/merge_requests/save_merge_request_review_service_spec.rb - two examples added asserting the sha (or nil) the MCP approve tool receives reaches ApprovalService params; 34 examples, 0 failures.
• 145 examples across the three service specs, 10 in the model spec, 0 failures. RuboCop clean on all changed files.

Beyond the committed specs

Three further harnesses were run to answer "does this actually work", all passing, none committed:

1. Full-chain harness (11 examples, :sidekiq_inline) driving real production entry points: MergeRequests::CreateService -> held-back NewMergeRequestWorker -> MergeRequests::ApprovalService -> Repositories::PostReceiveWorker -> Git::ProcessRefChangesService -> MergeRequests::RefreshService -> MergeRequests::Refresh::ApprovalService -> MergeRequestResetApprovalsWorker. Confirms the approval survives with an asserted sha and is still deleted without one, still deleted with the flag off, kept when reset_approvals_on_push is off, kept for a fork, and - importantly - still deleted when it should be: when the branch moves between the assertion and the diff row landing, on a content-changing push after the diff exists, and on a target-branch change. Also that a legacy nil approval is still dropped while a correctly stamped one beside it is kept.
2. API request specs (5 examples) through the real HTTP endpoint, so check_sha_param! runs: a patch ID is recorded when sha is sent, nil when it is omitted, 409 for a SHA that is not the head, and - in the stale-diff window - 409 for the new head while the merge request still reports the old one.
3. Patch ID equivalence probes (12 examples), because the fix is only correct if the derived value equals what the diff row goes on to report. Verified for a single added file, several commits, a modified file, a rename, binary content, and a target branch that advanced after the branch point. Plus robustness: an unknown SHA returns nil without raising, the target head returns nil, and a Gitaly failure degrades to nil.

Manual verification on GDK, real stack

Reproduced against a live GDK over the REST API with real Gitaly, Redis, Postgres and Sidekiq. The queue latency the bug needs was made deterministic by SIGSTOPing Sidekiq for the window rather than fabricating state, and each arm asserted the window was genuinely open (merge_request_diffs count 0, prepared_at NULL) before approving. Identical flow in both arms; the only difference is whether sha was sent:

	sha sent	sha omitted
Diff rows at approve time	0	0
patch_id_sha stamped	d0e03792c3e5...	NULL
Approvals after the reset ran	1	0
System notes	added 1 commit	added 1 commit, reset approvals from @stefani by pushing to the branch

The stamped value matched the patch ID the diff row went on to report, exactly. The sha-omitted arm is the reported bug verbatim: approved this merge request followed by reset approvals from ... by pushing to the branch, which also confirms the reset chain really fired in both arms rather than the approval surviving because nothing ran.

MCP path. Verified the same way through the real MCP endpoint, using a PAT with the mcp scope rather than an LLM agent, sending the JSON-RPC request an agent would send. Same two arms, same outcome: with sha passed the approval was stamped and survived the reset (1 approval, no reset note), with sha omitted it was stamped NULL and reset (0 approvals, reset note posted). The stamped value matched the diff row's later patch ID exactly, and MergeRequestResetApprovalsWorker ran for both branches. Note approve is not gated by the tool's "diff is not ready" check (that only guards submit_review), so an agent can genuinely approve inside the window.

To reproduce on GDK (Sidekiq must be paused before the branch push so the queued PostReceive runs the reset chain against an unchanged head; a later content-changing push legitimately resets):

# 1. mint an mcp-scoped PAT for a user who is not the MR author
#    POST /users/:id/personal_access_tokens  scopes[]=api&scopes[]=mcp
# 2. pause the GDK sidekiq worker (pgrep -f "^sidekiq 7", pick the pid whose lsof shows this GDK)
kill -STOP <sidekiq pid>
# 3. push a branch and create the MR
curl -X POST -H "PRIVATE-TOKEN: $ROOT" -H "Content-Type: application/json"   "$GDK/api/v4/projects/$P/repository/commits"   -d '{"branch":"probe","start_branch":"main","commit_message":"probe","actions":[{"action":"create","file_path":"probe.txt","content":"one
"}]}'
curl -X POST -H "PRIVATE-TOKEN: $ROOT" -H "Content-Type: application/json"   "$GDK/api/v4/projects/$P/merge_requests" -d '{"source_branch":"probe","target_branch":"main","title":"probe"}'
# 4. confirm the window is open
gdk psql -c "select count(*) from merge_request_diffs where merge_request_id=<id>"   # 0
gdk psql -c "select prepared_at from merge_requests where id=<id>"                   # NULL
# 5. approve through the MCP tool with the branch head sha
curl -X POST -H "PRIVATE-TOKEN: $MCP_PAT" -H "Content-Type: application/json"   -H "Accept: application/json, text/event-stream" "$GDK/api/v4/mcp"   -d '{"jsonrpc":"2.0","id":1,"method":"tools/call","params":{"name":"save_merge_request_review","arguments":{"project_id":"'$P'","merge_request_iid":<iid>,"method":"approve","sha":"<head sha>"}}}'
gdk psql -c "select encode(patch_id_sha,'hex') from approvals where merge_request_id=<id>"   # non-NULL with sha, NULL without
# 6. resume sidekiq; the queued PostReceive runs the reset chain
kill -CONT <sidekiq pid>
gdk psql -c "select count(*) from approvals where merge_request_id=<id>"   # 1 with sha, 0 without

Replaying the original reporter's script

The bash script the reporter attached to the issue (#395506 (comment 1356110000)) was replayed in the same shape and sequence against a GDK project, driven entirely through the REST API. Three arms were run with identical timing, varying only whether sha was sent and whether the flag was on.

Arm	sha sent	Flag	Stamped value	Outcome
A	No	On	NULL	Deleted 5 seconds after the push chain ran
B	Yes	On	e57d9d5e...	Still present after 300 seconds
C	Yes	Off	NULL	Deleted 5 seconds after the push chain ran

In every arm, the diff rows that eventually landed had identical base and head commits and exactly one distinct patch ID between them, so the branch content never changed at any point. Arms A and C both stamped NULL under identical timing, which establishes that no diff row exists at the moment of approval. Arm B ran with the same timing and produced a non-null patch ID, and that value is exactly the patch ID those diff rows go on to report, so it can only have come from patch_id_sha_for_head. Arm A also shows the opt-in limitation: a client that does not send sha is still affected.

Known limitation found while testing

A branch whose net diff is empty - a file added and then deleted again, for example - has no patch ID at all: Gitaly reports no difference and Repository#get_patch_id returns nil. An approval on such a merge request is stamped nil and still reset, with or without sha. That is unchanged from master, since the diff row's own patch_id_sha is nil too, but this MR does not fix it.

Rollout

Flag is gitlab_com_derisk, default off, rollout issue #624933. With the flag off, ApprovalService#patch_id_sha_for returns current_patch_id_sha before touching params[:sha] or the diff head, so the approve path is byte-for-byte master. The reset path does not consult the flag at all now. Enable on a single project first, then percentage of actors by project.