Verify fast-forward merge advances the target branch

Marc Shawrequested to merge
598820-ff-merge-advancement-check into master

What does this MR do and why?

Fixes silent data loss on the fast-forward / semi-linear + automatic-rebase merge path. See https://gitlab.com/gitlab-org/gitlab/-/work_items/598820.

MergeRequests::MergeStrategies::FromSourceBranch#fast_forward! recorded whatever SHA Gitaly returned as the merge result without verifying that the target branch actually advanced. When the fast-forward is a no-op — for example a rebase that collapses onto the target tip, or a stale/concurrent ref — the merge request was still persisted as merged with merge_commit_sha = nil, and (with source-branch deletion enabled) the source branch was deleted, silently losing the MR's commits.

This MR captures the prior target tip and passes it as target_sha (Gitaly optimistic lock), then raises unless the returned SHA is present and differs from that tip. It mirrors the guard the merge-train strategy already has in FromTrainRef#execute_git_merge!, and covers both the auto-rebase and the plain fast-forward-only paths.

Feature flag

Gated behind verify_ff_merge_advancement (gitlab_com_derisk, default off), so it can be rolled out gradually and disabled instantly if the guard ever fires on a legitimate merge. Rollout issue: #602293

Verification

Automated

Regression specs in spec/services/merge_requests/merge_strategies/from_source_branch_spec.rb cover no-advance → raise, blank → raise, and flag-disabled → legacy behavior. They fail without the fix and pass with it.

Manual — end-to-end through the REST API

Reproduced the customer's exact data-loss signature from the work item on GDK, driving the real merge endpoint (PUT /projects/:id/merge_requests/:iid/merge → MergeRequests::MergeService) rather than the service layer directly. Project, branches, commits, MRs, the merge, and verification all go through the API; the flag is toggled with POST / DELETE /api/v4/features/verify_ff_merge_advancement. Two things have no REST surface and are handled out-of-band: the Enable automatic rebase prior to merge project setting (project_setting.automatic_rebase_enabled, set via UI/Rails) and reading back merged_commit_sha (not exposed in the MR entity, read via console).

That project setting is the trigger: with it on, CheckRebaseStatusService returns inactive, so a diverged MR still reports mergeable? and the merge proceeds — and the merge-time rebase then collapses it to a no-op.

Setup (mirrors the customer's A / B / C sequence):

  1. Project on merge_method = rebase_merge with automatic rebase prior to merge enabled.
  2. MR A — feat-a with two commits, squash-merged → target advances to A'.
  3. Branch dup-b from A's source, then toggle merge_method to ff. After A merged, dup-b's commits are content-equivalent to A', so the merge-time auto-rebase collapses to a no-op.
  4. MR B — merge dup-b with source-branch removal. This is the data-loss path.
  5. MR C — independent commits on rebase_merge — the legitimate control.

Run the sequence once with the flag off, once on:

MR flag off (bug) flag on (fix)
A (legitimate) merged · merge_commit_sha == merged_commit_sha · target advanced identical
B (no-op collapse) merged · merge_commit_sha = nil · merged_commit_sha = A' · target NOT advanced · source branch deleted opened · merge_commit_sha = merged_commit_sha = nil · source branch intact · merge_error set
C (legitimate) merged · real merge commit · target advanced · first-parent = A' identical

The decisive field is B's merged_commit_sha: A' with the flag off (B recorded as merged carrying A's commit, then its branch deleted — data loss) versus nil with it on (B rejected, stays open, branch preserved). MRs A and C are byte-for-byte unchanged between the two runs, confirming the guard fires only on the pathological no-op fast-forward and leaves legitimate merges (squash, ff, and semi-linear merge commits) untouched.

On rejection the user sees the generic An error occurred while merging; the merge service log records the specific reason — Fast-forward merge did not advance the target branch.

Related

MR acceptance checklist

  • Tests added (regression specs; fail without the fix)
  • Feature flag added (verify_ff_merge_advancement, default off) with rollout issue
  • Backports to 18.10 / 18.11 stable (follow-up)
Edited by Marc Shaw

Merge request reports