Stop auto-merge hanging on a finished merge train pipeline

When a merge train pipeline fails, the merge request is dropped from the train but diff_head_pipeline keeps pointing at the failed pipeline. This MR fixes both halves of the resulting bug: the widget was offering the wrong button ("Add to merge train when all merge checks pass" instead of "Re-add to merge train"), and even the strategy it did offer could never complete, because a completed merge train pipeline is never retryable. MergeTrainService now stays preferred so the correct button comes back, and AddToMergeTrainWhenChecksPassService no longer gets stuck when it is reached anyway.

Flag auto_merge_readd_to_train_after_completed_pipeline (gitlab_com_derisk, default off, project actor, 19.4) covers both halves - it is a single kill switch. Rollout: #627754. Issue: #627635. Observed on !252202 (merged) and !248460 (merged).

Detailed context for AI agents

The bug

When a merge train pipeline fails, the merge request is dropped from the train, but diff_head_pipeline still points at that failed train pipeline.

AutoMergeService#available_strategies tries merge_train first. EE::MergeRequest#skipped_auto_merge_checks skips the rebase check (and conditionally the CI check) for that strategy, so all 17 other mergeability checks must pass. add_to_merge_train_when_checks_pass (AMTWCP) only requires not_open and commits_status - it is the catch-all for "not ready yet". The moment any one of merge_train's checks is transiently not successful, merge_train drops out and AMTWCP is offered instead. There is no "neither" state.

The most common trigger on a busy project: every push to the target branch runs MergeRequests::RefreshService#reload_merge_requests, which calls batch_mark_as_unchecked on all matching merge requests. CheckConflictStatusService returns checking for any merge_status that is not can_be_merged/cannot_be_merged, and nothing recomputes it for a dropped merge request because enqueue_auto_merge_for_unchecked only covers merge requests that still have auto-merge on. Other triggers, such as an unresolved thread, are not races at all.

Issue's own stated fix direction

Issue #627635 frames the bug as the wrong widget button and states the expected fix directly: do not offer add_to_merge_train_when_checks_pass when the head pipeline is a failed merge train pipeline; always offer "Re-add to merge train" instead. This MR's second commit implements exactly that, by keeping merge_train preferred rather than by changing the widget.

Why the wrong button appears, and why it's intermittent

MergeRequestPollWidgetEntity supplies the strategy list (availableAutoMergeStrategies / preferredAutoMergeStrategy) over the REST widget poll, while detailedMergeStatus comes from a separate GraphQL query and subscription. MergeRequestStore keeps the previous detailedMergeStatus when a REST poll response lands. So the widget can be rendering its ready state from a good, slightly-stale detailedMergeStatus while the button text comes from a strategy list computed moments earlier, when merge_status was transiently unchecked. That gap between the two data sources is why the bug is intermittent (the issue reports roughly 1 in 3) and why the documented workaround is to cancel auto-merge and click again.

Once the wrong button is clicked, add_to_merge_train_when_checks_pass is persisted as the merge request's strategy, and AutoMergeService#process dispatches forever on that persisted value - it never re-asks which strategy the merge request should have had. That persisted-strategy dispatch is exactly what the first commit's readd_to_train? predicate unsticks.

Why it hangs forever (first commit's problem)

AMTWCP's #process gate was merge_request.mergeable?(skip_conflict_check: true, use_cache: false), which requires a successful head pipeline. The head pipeline is the failed merge train pipeline, and completed merge train pipelines have retryable? == false, so the gate can never pass. #process returns silently on failure: no abort, no system note, no todo. It no-ops on every subsequent event forever.

Confirmed in the stuck state: MergeTrainService.available_for? == true while mergeable?(skip_conflict_check: true) == false. The system already knows the merge request can go back on the train; AMTWCP just never asks.

The first commit's fix

AddToMergeTrainWhenChecksPassService#process now passes skip_ci_check: into its mergeability gate, computed by a private predicate on the service:

def readd_to_train?(merge_request)
  return false unless Feature.enabled?(:auto_merge_readd_to_train_after_completed_pipeline, project)

  !merge_request.on_train? &&
    !merge_request.pipeline_creating? &&
    merge_request.diff_head_pipeline&.completed_merge_train_pipeline?.present?
end

It keys off Ci::Pipeline#completed_merge_train_pipeline?, the same fact retryable? uses to refuse retries, rather than the looser merge_train_pipeline?. Without the complete? term, a merge request whose stale train pipeline is still running would have its CI check skipped, clear the gate, then fail MergeTrainService.available_for? on incomplete_diff_head_pipeline and get aborted where today it correctly waits.

The predicate is private to this service. EE::MergeRequest#skipped_auto_merge_checks and the merge_train strategy are deliberately untouched, so nothing outside this strategy changes behaviour. No new policy is introduced: the merge_train strategy already treats a failed train pipeline as "CI has already run against the merged result". This just stops AMTWCP disagreeing with it.

Concurrency guard (first commit)

return if merge_request.reset.on_train? is placed immediately before the abort in #process. Found on a local GDK driving the real API, not from specs.

One click fans out into two AutoMergeProcessWorker runs with different args, so deduplicate :until_executed does not collapse them. Both clear the pre-gate. The winner adds the car, which flips on_train? to true, so the predicate goes false, so the CI check comes back and fails against the still-failed train pipeline. The loser then aborts and destroys the car about 150ms later.

Observed log: run 2 adds to train at 13:44:29.109, run 1 aborts at 13:44:29.260, with the system note "removed this merge request from the merge train because the merge request cannot be merged. The pipeline must succeed."

Without the guard, the user clicks the button and auto-merge silently cancels itself a second later, worse than the original hang. This race is introduced by the CI skip, so the guard has to ship with it. A guard at the top of #process does not work, because the losing run passes it before the winner commits - it has to be re-read right before the destructive step. The guard is gated on the same flag as the CI skip, so it is inert with the flag off and the whole diff has a single kill switch.

The second commit's fix: keep merge_train preferred

AutoMerge::MergeTrainService#skippable_available_for_checks previously set skip_conflict_check: true only when the merge request's persisted auto_merge_strategy was already add_to_merge_train_when_checks_pass. It now also sets it when a new private predicate, readd_after_completed_train_pipeline?, is true: the auto_merge_readd_to_train_after_completed_pipeline flag is enabled, the merge request is not on the train, and diff_head_pipeline&.completed_merge_train_pipeline?.

This is deliberately the same flag as the first commit - the two halves are one fix, and the flag is a single kill switch for both. The flag YAML description was reworded to cover both behaviours.

Because getting merge_train preferred is sufficient, no frontend change was needed: showReAddToMergeTrain and showFailedPipelineModalMergeTrain in ee/app/assets/javascripts/vue_merge_request_widget/mixins/ready_to_merge.js are both gated on preferredAutoMergeStrategy === 'merge_train' plus a train-ref pipeline, so the "Re-add to merge train" helper text and the "The latest pipeline failed for this merge request. Do you still want to merge?" confirmation dialog both come back once merge_train wins again.

Why both halves are kept - verified, not assumed

Verified manually on a local development instance:

• Flag off: strategies offered ["add_to_merge_train_when_checks_pass"]; an already-stuck merge request stays stuck across repeated #process calls.
• Flag on: strategies offered ["merge_train", "add_to_merge_train_when_checks_pass"], so merge_train is preferred; an already-stuck merge request is added to the train on the next #process.
• The conflict skip alone (second commit without the first) is not sufficient. Two cases were reproduced with it enabled and the AMTWCP fix disabled:
  • (a) A merge request already carrying the persisted add_to_merge_train_when_checks_pass strategy stays stuck, because #process dispatches on the persisted strategy and never re-evaluates it.
  • (b) A merge request dropped from the train and then set to auto-merge while an unresolved discussion exists is correctly offered add_to_merge_train_when_checks_pass (merge_train does not skip the discussions check), and once the discussion is resolved it still hangs forever on the dead pipeline. Case (b) is not a race at all. Every check that merge_train does not skip but AMTWCP does is such a doorway: discussions, approvals, external status checks, security policies, Jira, locked paths, draft, blocked-by.
• MergeRequests::Mergeability::CheckApprovedService also returns checking while temporarily_unapproved? is set, which demotes merge_train identically and is not addressed by the conflict skip. Noted as a known remaining gap.

Trade-off

Skipping the conflict check means a merge request with a genuine conflict can be offered the merge train and added to it, then dropped when the train ref cannot be built. That matches the reasoning already in the existing comment on skippable_available_for_checks - the conflict surfaces at train ref build time with a more informative message.

What changed from the previous revision (first commit)

An earlier revision extracted the rule onto EE::MergeRequest#merge_train_retry? and rewired skipped_auto_merge_checks to call it, which switched the merge_train strategy from merge_train_pipeline? to completed_merge_train_pipeline?. That half was not behind the flag.

That was reverted because it changed behaviour for the merge_train strategy. AutoMerge::BaseService#availability_details runs the mergeability checks before yielding to the subclass block, so MergeTrainService's own pipeline.complete? test never gets the chance to compensate, and that block has a canceling? || blocked? branch that deliberately allows an incomplete pipeline.

Reproduced effect: with only_allow_merge_if_pipeline_succeeds = false, CI enabled, stored strategy AMTWCP, and a head pipeline on a train ref in manual or canceling state, merge_train dropped out of availableAutoMergeStrategies - the widget lost a button it should show. It also broke 25 examples in ee/spec/features/merge_trains/two_merge_requests_on_train_spec.rb, whose instance_double(Ci::Pipeline, ...) stubs merge_train_pipeline? but not completed_merge_train_pipeline?.

Keeping the predicate private to the AMTWCP service removes all of that for the first commit. The second commit does touch MergeTrainService, but only through the same skippable_available_for_checks path already used for the persisted-AMTWCP case, gated the same way.

Verification (first commit's matrix)

A 36-row matrix was run over pipeline state (none, success/failed on a normal ref, and failed/canceled/success/running/manual/canceling on a train ref) crossed with only_allow_merge_if_pipeline_succeeds true/false, crossed with stored auto-merge strategy none/AMTWCP. For each row it recorded skipped_auto_merge_checks(auto_merge_strategy: 'merge_train')[:skip_ci_check], MergeTrainService#available_for?, the unavailable_reason, MergeRequest#auto_merge_eligible?(strategy: 'merge_train') and AutoMergeService#available_strategies. The matrix was run against this branch and against its master parent, in two variants (project CI enabled and not). Output is byte-identical to the parent in both variants.

Because the guard is gated too, flag-off is now master's behaviour for the whole diff.

That matrix covers the merge_train strategy path, not the gate the first commit actually changes, so the AMTWCP gate was measured separately. The table below evaluates the exact expression from #process against a real merge request on a GDK, with the flag genuinely toggled, in a rolled-back transaction:

scenario                     flag   | predicate GATE     avail    outcome
failed/TRAIN-ref (the bug)   off    | F        F        T        returns early, keeps waiting
failed/TRAIN-ref (the bug)   ON     | T        T        T        ADDS TO TRAIN
canceled/TRAIN-ref           off    | F        F        T        returns early, keeps waiting
canceled/TRAIN-ref           ON     | T        T        T        ADDS TO TRAIN
running/TRAIN-ref            off    | F        F        F        returns early, keeps waiting
running/TRAIN-ref            ON     | F        F        F        returns early, keeps waiting
failed/normal-ref            off    | F        F        F        returns early, keeps waiting
failed/normal-ref            ON     | F        F        F        returns early, keeps waiting

predicate is readd_to_train?, GATE is the mergeable? call in #process, avail is MergeTrainService#available_for?.

The first row is the bug: the gate is false while available_for? is true, which is the stuck state described above. Three further points come out of it. Canceled train pipelines hang in exactly the same way and are fixed by the same change. A still-running train pipeline keeps the predicate false in both flag states, and since available_for? is also false there, a looser predicate would have cleared the gate and then aborted the merge request - which is what the complete? term prevents. Pipelines on a normal ref are unaffected in both states.

Each new example was checked against unpatched code and confirmed to fail there in the expected way, rather than passing for the wrong reason:

• Reverting the service to its master version fails "adds the merge request back to the train".
• Deleting the guard fails the flag-on concurrency example.
• Leaving the guard in but removing its flag check fails the flag-off example.

Specs: 688 examples across ee/spec/services/auto_merge, ee/spec/services/merge_trains, spec/services/auto_merge and ee/spec/models/merge_request_spec.rb, 0 failures. Rubocop clean.

End-to-end on a local GDK with a real merge train, on a project with merge trains, merged results pipelines and only-allow-merge-if-pipeline-succeeds enabled, with CI rigged to fail only when $CI_MERGE_REQUEST_EVENT_TYPE == merge_train. Readings taken from the GraphQL availableAutoMergeStrategies field. Before the fix, pushing a commit to master flipped the merge request to merge_status=UNCHECKED and left only add_to_merge_train_when_checks_pass available, which never self-corrected and left it stuck through 55 seconds of polling and 3 direct AutoMergeProcessWorker runs. After the fix, clicking the button puts the merge request back on the train immediately, a new train pipeline runs, and it merges. This run was performed against the earlier revision; the AMTWCP gate logic is unchanged since.

Tests added

First commit, all in ee/spec/services/auto_merge/add_to_merge_train_when_checks_pass_service_spec.rb:

• A failed merge train pipeline adds the merge request back to the train.
• A concurrent run does not tear down the existing car, with the flag on.
• With the flag off, a concurrently created car is still torn down, that is, master's behaviour is unchanged.
• A still-running train pipeline keeps waiting and does not initialise MergeTrainService.
• A pipeline being created still waits.
• Flag off keeps the old behaviour.

Second commit:

• ee/spec/services/auto_merge/merge_train_service_spec.rb: the conflict-check skip is applied only when the flag is on, the merge request is off the train, and the train pipeline is complete. Two existing instance_double(Ci::Pipeline, ...) in this spec gained completed_merge_train_pipeline?: false so they keep exercising the old, unaffected path.
• ee/spec/services/ee/auto_merge_service_spec.rb: with a stale merge_status, merge_train is still offered and preferred; with the flag off it is dropped (reproducing the bug's own symptom in a spec).

Remaining gaps, deliberately out of scope

• MergeRequests::Mergeability::CheckApprovedService returns checking while temporarily_unapproved? is set, which demotes merge_train the same way the conflict check does, and this MR does not extend the skip to cover it.
• The guard narrows the race window but does not lock it. Properly closing it would need the AMTWCP-to-merge_train transition under an exclusive lease, which is a bigger change than this bug warrants.
• MergeTrainService#execute's own return if merge_request.on_train? reads a merge_train_car association that was already cached earlier in #process, so two runs that both pass available_for? can both reach build_merge_train_car. This is pre-existing rather than introduced here, since the old code also called on_train? at the same point.
• The root cause behind both this bug and the CheckApprovedService gap is that checking means "not known yet", but available_strategies treats it as unavailable and silently falls through to a weaker strategy - there is no "neither" state. Fixing that properly, either by not offering any strategy while a check is checking or by holding the widget button in a loading state, would close the whole class of bugs and is worth a separate issue.