Fix merge-train pipeline canceled on skip_merge_train race

Bo Carbonellrequested to merge
duo/fix/741014-merge-train-skip-merge-race into master

What does this MR do and why?

Fixes a race condition (Zendesk ticket 741014) where an active merge-train pipeline is incorrectly canceled when an off-train MR is merged with skip_merge_train=true.

Root cause

When MergeService merges an off-train MR with skip_merge_train=true, the call order was:

  1. execute_git_merge! -> git push to target branch -> Gitaly post-receive hook -> PostReceiveWorker enqueued (async)
  2. merge_request.update!(merged_commit_sha: ...) -- SHA persisted to DB
  3. EE::MergeRequests::MergeService#after_merge -> MergeTrains::Car.insert_skip_merged_car_for -- skip_merged car created

UpdateMergeRequestsWorker (urgency: high) is enqueued in step 1 and can execute before step 3 completes. When it runs, it calls MergeRequests::RefreshService -> EE::MergeRequests::RefreshService#check_merge_train_status -> MergeTrains::CheckStatusService#execute -> MergeTrains::Train#sha_exists_in_history?(newrev).

Until step 3 completes there is no completed car for the off-train MR, so sha_exists_in_history? returns false, causing first_active_car.outdate_pipeline to cancel and recreate the running merge-train pipeline.

Fix

Move insert_skip_merged_car_for before the git push.

EE::MergeRequests::MergeService#commit now calls insert_skip_merged_car_for BEFORE calling super (which does the git merge and push). The MR is already locked at this point (inside in_locked_state), satisfying the guard in insert_skip_merged_car_for.

No query changes are needed: completed_cars already includes skip_merged cars, and the existing SHA condition in sha_exists_in_history? matches in_progress_merge_commit_sha, which is set during the Gitaly merge (before the push) and only cleared after merged_commit_sha is persisted. With the car created before the push, the existing check returns true throughout the race window. ee/app/models/merge_trains/train.rb is unchanged by this MR.

Rollback on merge failure.

If the git merge raises (MergeRequests::MergeBaseService::MergeError, or MergeRequests::MergeStrategies::StrategyError, which try_merge re-raises unchanged to preserve its user-facing message), the skip_merged car is destroyed before re-raising, so a failed merge leaves no stale car in completed history. Both error classes are raised only before anything lands on the target branch. Post-push failures (for example a transient DB error while persisting merged_commit_sha) intentionally keep the car, because the commit is already on the branch and the car legitimately belongs in train history. Destroying a pipeline-less skip_merged car is safe: Ci::CancelPipelineService#force_execute returns early for a nil pipeline.

Remaining limitation

The sub-millisecond window before the MR is locked (before in_progress_merge_commit_sha is set by the Gitaly merge) is not covered. This window predates the git push, so UpdateMergeRequestsWorker cannot have been enqueued yet. The dangerous window (between git push and merged_commit_sha persistence) is fully covered.

References

Screenshots or screen recordings

N/A - backend race condition fix.

How to set up and validate locally

  1. Create a project with merge pipelines enabled, merge trains enabled, merge_trains_skip_train_allowed: true, merge method = merge commits.
  2. Create MR A targeting default branch; add to merge train; ensure pipeline is running (sleep in CI).
  3. Create MR B targeting same branch (not on train).
  4. Merge MR B via API with skip_merge_train=true.
  5. Observe MR A's pipeline continues running (was: canceled and recreated).

Test coverage

Regression specs in ee/spec/models/merge_trains/train_spec.rb, ee/spec/services/merge_trains/check_status_service_spec.rb, ee/spec/services/ee/merge_requests/merge_service_spec.rb, and ee/spec/models/merge_trains/car_spec.rb:

  1. Race window: skip_merged car exists, in_progress_merge_commit_sha == newrev, SHA not yet on MR -> sha_exists_in_history? returns true (train not invalidated).
  2. Completed skip-merge: car exists, SHA on MR, MR merged -> returns true.
  3. Normal off-train merge (no skip_merged car): returns false (train invalidation preserved).
  4. Unexpected commit (newrev differs from in_progress_merge_commit_sha): returns false (train invalidated).
  5. Different target branch/project: skip_merged car elsewhere does not suppress invalidation for this branch's train.
  6. Rollback: if the git merge raises after the skip_merged car is created (MergeError, or a StrategyError that try_merge re-raises unchanged), the car is destroyed (no stale car remains); destroying a pipeline-less skip_merged car does not raise.

MR acceptance checklist

Evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.

Database review

MergeTrains::Car.insert_skip_merged_car_for (now called before the git push)

has_one load performed by create_merge_train_car:

SELECT "merge_trains".* FROM "merge_trains" WHERE "merge_trains"."merge_request_id" = 517595598 LIMIT 1

INSERT of the skip_merged car:

INSERT INTO "merge_trains" ("merge_request_id", "user_id", "created_at", "updated_at", "target_project_id", "target_branch", "status", "merged_at")
VALUES (517595598, 8410247, now(), now(), 278964, 'master', 5, now())
RETURNING "id"

skip_merged_car&.destroy (new rollback path when the git merge raises MergeError or StrategyError)

DELETE FROM "merge_trains" WHERE "merge_trains"."id" = 10181056

after_destroy → try_cleanup_ref(async: false) → MergeRequests::GeneratedRefCommit.where(project:, merge_request:).delete_all:

DELETE FROM "p_generated_ref_commits"
WHERE "p_generated_ref_commits"."project_id" = 278964
  AND "p_generated_ref_commits"."merge_request_iid" = 249384
  AND "p_generated_ref_commits"."project_id" = 278964

MergeTrains::Train#sha_exists_in_history? (unchanged; the fix relies on its timing)

SELECT 1 AS one
FROM "merge_requests"
WHERE "merge_requests"."id" IN (
  SELECT "merge_trains"."merge_request_id"
  FROM "merge_trains"
  WHERE "merge_trains"."target_project_id" = 278964
    AND "merge_trains"."target_branch" = 'master'
    AND "merge_trains"."status" IN (1, 4, 5)
  ORDER BY "merge_trains"."id" DESC
  LIMIT 20
)
AND (
  merge_commit_sha = '36c7684e65432abcacc13eac860f62db0a73c2c0'
  OR in_progress_merge_commit_sha = '36c7684e65432abcacc13eac860f62db0a73c2c0'
  OR squash_commit_sha = '36c7684e65432abcacc13eac860f62db0a73c2c0'
  OR merged_commit_sha = '36c7684e65432abcacc13eac860f62db0a73c2c0'
)
LIMIT 1
Edited by Bo Carbonell

Merge request reports