Discard merge statuses computed for stale inputs

What does this MR do and why?

Retarget a merge request while a mergeability check is running and the in-flight check overwrites the correct merge status with one computed against the old target branch. The merge request then reports mergeable while merging fails - permanently, because later checks short-circuit on recheck_merge_status? and not even with_merge_status_recheck=true recovers it.

MergeabilityCheckService now captures the source and target a merge status was computed from, and drops the write when the row no longer has them, reporting reason: :stale_merge_status_discarded. The row still awaits a recheck, so the next check answers for the new inputs.

Behind discard_stale_mergeability_verdicts (gitlab_com_derisk, default off). With the flag off there is no added query and no lock.

One behaviour change worth knowing about up front: a discarded merge status makes a merged results pipeline fall back to a detached one for that one race window. Intended - the merge ref was computed for the old target - details below.

How it works, and what a reviewer should look at

The inputs are target_branch plus the diff, which stands in for the source SHA that no column records (diff_head_sha reads it from whichever merge_request_diff is loaded, not necessarily the latest). They are captured in the service before anything touches the merge request, and re-checked at the write.

• The recheck runs inside the transaction that writes, and locks the row. A concurrent retarget updates the same row, so it waits, and cannot move the inputs between the answer and the write. The transition still performs the only UPDATE, so its callbacks and the merge_requests_merge_data dual write stay intact.
• It compares inputs, not merge status. Also requiring the row to still await a status would make the first writer win - a check running on inputs that have since been restored would lock out the correct status behind it, turning a self-correcting race into a permanent wrong answer.
• A discarded merge status downgrades a merged results pipeline to a detached one, since EE::MergeRequests::CreatePipelineService lets only :merge_status_race through and this branch carries no payload. Intended: the merge ref was computed for the old target, and it is the same fallback a conflicted merge request already gets. It lasts one race window, because the row still awaits a recheck. GET .../merge_ref likewise 400s rather than returning a commit from the stale ref. Pinned by a spec in ee/spec/services/ee/merge_requests/create_pipeline_service_spec.rb; a Verify maintainer review is lined up for it.

Database

One added query per merge status write once the flag is on: a primary-key lookup taking a FOR UPDATE lock on that single row, for the duration of the transition's save path. RetargetChainService and the batch recheck paths are what would queue behind it. The flag is gitlab_com_derisk because the lock dirties the page and writes WAL even when the row is unchanged.

Raw SQL and query plan

SELECT "merge_requests"."target_branch", "merge_requests"."latest_merge_request_diff_id"
FROM "merge_requests" WHERE "merge_requests"."id" = $1 LIMIT 1 FOR UPDATE

Limit  (cost=0.57..3.60 rows=1 width=21) (actual time=9.438..9.441 rows=1 loops=1)
  Buffers: shared hit=4 read=6 dirtied=1
  WAL: records=1 fpi=1 bytes=6435
  I/O Timings: read=9.297 write=0.000
  ->  LockRows  (cost=0.57..3.60 rows=1 width=21) (actual time=9.437..9.438 rows=1 loops=1)
        Buffers: shared hit=4 read=6 dirtied=1
        WAL: records=1 fpi=1 bytes=6435
        I/O Timings: read=9.297 write=0.000
        ->  Index Scan using merge_requests_pkey on public.merge_requests
              (cost=0.57..3.59 rows=1 width=21) (actual time=7.480..7.481 rows=1 loops=1)
              Index Cond: (merge_requests.id = 512878411)
              Buffers: shared hit=3 read=5
              I/O Timings: read=7.414 write=0.000

Ten buffers, execution 9.493 ms - almost all of it cold-clone I/O (read=9.297). Production reads this row from shared buffers, on the page the transition is about to update anyway.

The real cost is the WAL line: taking the lock marks the tuple, so it dirties the page and writes WAL even though the row is unchanged. The 6435 bytes here include a full-page image, so steady state is one small record per merge status write rather than 6 KB - but it is not free.

Session: https://console.postgres.ai/gitlab/projects/gitlab-production-main/sessions/54407/commands/157209

Verification on a local instance

Reproduced the reporter's steps end to end, with the check landing in the window DeleteSourceBranchWorker leaves between RetargetChainService and deleting the branch:

Step	Flag off	Flag on
before merge, target parent	mergeable	mergeable
after retarget to main	conflict	conflict
stale check lands	mergeable	conflict
after parent deleted	mergeable	conflict
after with_merge_status_recheck	mergeable	conflict

Retargeting itself is not broken - step 2 shows it invalidates and recomputes correctly.

Remaining gaps

• The target branch is compared by name, not SHA: can_be_merged? resolves the name at Gitaly and discards it, so a caller cannot know which target SHA its merge status used. Worth designing with #605792, which has the same problem for pipelines.
• The check only notices after its Gitaly merge has written a merge ref for the old target and persisted DiffNotePosition rows from it. Tracked in #608481, along with the merged results fallback above.

How to set up and validate locally

bundle exec rspec spec/models/merge_request_spec.rb -e '#merge_status_inputs'
bundle exec rspec spec/services/merge_requests/mergeability_check_service_spec.rb
bundle exec rspec ee/spec/services/ee/merge_requests/create_pipeline_service_spec.rb

New contexts cover a retarget, a superseded source diff, a record refreshed after capture, the broken? path, and a merge status already written for the same inputs (which must still be overwritten). Each context that asserts the bug has a flag-off counterpart showing the stale status winning today.

References

• #606487
• Rollout: #607205
• Follow-up: #608481