Reconcile silently-merged MRs instead of aborting the train
What does this MR do and why?
Fixes #598924. Fast-forward merge trains (merge_method: ff /
rebase_merge) use the FromTrainRef merge strategy, which fast-forwards the commit
onto the target branch; MergeRequests::PostMergeService then finalises the merge
(marks it merged, sets merged_at, fires hooks/notifications, closes issues, deletes
the source branch).
If a transient error (e.g. a statement timeout or a Sidekiq shutdown) interrupted that
post-merge, MergeTrains::RefreshMergeRequestService's broad rescue StandardError
treated it like any other failure and called abort, ejecting the MR from the train and
destroying the merge train car. But the commit had already landed on the target branch,
so the MR was left open with merged_at: null — inconsistent with git. Re-adding it to
the train then reported "no changes to merge after rebasing onto the target branch", and
required manual intervention.
RefreshMergeRequestService#merge! now rescues an interrupted merge and reconciles it
instead of aborting, covering three windows:
- Interrupted after ff_merge landed the commit, before the post-merge finished →
run the full post-merge follow-through via
MergeRequests::PostMergeService(idempotent through its ownmerged?guard) plus source-branch deletion. This setsmerged_at, fires hooks/notifications and closes issues — a real merge, not a state flip. Reconciliation only runs when the landed commit is actually reachable from the target branch head (mirrorsMergeRequest#squash_commit_reachable_from_target_branch?), so a stale sha from a prior aborted attempt cannot qualify. For the FromTrainRef strategy the landed commit is the current train ref sha; other strategies use the recordedmerged_commit_sha. That landed sha is also persisted asmerged_commit_shabefore the post-merge (as the manual-merge refresh path does), sinceMergeServicemay not have recorded it before the interruption — otherwise consumers fall back todiff_head_sha, the un-rebased source tip. - Interrupted after the MR was already marked merged (a later post-merge step timed
out) → finish the car (mirrors
handle_merged_stuck_car); never abort a merged MR. merge!'s ownProcessError(a soft merge failure) is re-raised so it aborts as before rather than being reconciled.
If the reconciliation itself keeps failing transiently, a ReconcileError makes the
worker crash and retry without aborting, so the car is preserved and finalised on a
later run.
Both reconcile paths — the immediate one here and the stuck-car unstick path
handle_silently_merged_stuck_car — now share finalize_silently_merged_merge!, so a
reconciled merge always runs the same full post-merge follow-through. This also closes
#627553.
How to set up and validate locally
Setup
- Configure a GDK project with
merge_method: ffin its merge request settings. - Enable merge pipelines and merge trains on the project.
- Ensure an active EE license is loaded.
- Create an open merge request from a feature branch into the project's default branch (one or two users acting as author and maintainer is sufficient).
Validate Bug A: Reconcile interrupted post-merge
-
In a
rails consolesession, monkey-patchMergeRequests::PostMergeService#executeto raise an error after calling the original implementation:original_execute = MergeRequests::PostMergeService.instance_method(:execute) error_raised = false MergeRequests::PostMergeService.define_method(:execute) do result = original_execute.bind(self).call unless error_raised error_raised = true raise StandardError, "Simulated post-merge timeout" end result endAlternatively, add a temporary
raisestatement inapp/services/merge_requests/post_merge_service.rbimmediately after the line callingmark_as_merged, then revert it after testing. -
Add the merge request to the merge train via the UI or API.
-
Allow
MergeTrains::RefreshMergeRequestWorkerto run (or trigger it manually). Theff_mergeoperation pushes the commit to the target branch, then the injected error fires during post-merge finalization. -
Expected behavior before this fix: The MR is ejected from the train, the merge train car is destroyed, the MR remains open with
merged_at: null, and re-adding it reports "no changes to merge after rebasing onto the target branch". -
Expected behavior with this fix: The MR transitions to
mergedwithmerged_atset, and the car is finished. Manual re-addition is not required. A before/after screenshot of the MR widget is helpful here.
Validate Bug B: Finish stuck cars with post-merge follow-through
-
Create a merge train car in the
mergingstate withupdated_atset earlier than the stuck-car timeout threshold, while the underlying MR is already merged in the repository. -
Trigger the refresh by allowing
MergeTrains::RefreshMergeRequestWorkerto run, or by calling directly inrails console:MergeTrains::RefreshMergeRequestService.new(project, user).execute(merge_request) -
Expected result: The car finishes and the full post-merge follow-through runs, including source branch deletion, closing of related issues, and sending notifications. The fix ensures these side effects occur instead of a bare state-column flip.
Automated test coverage: bundle exec rspec ee/spec/services/merge_trains/refresh_merge_request_service_spec.rb
Known limitations
- The stuck-car unstick path (
handle_silently_merged_stuck_car) can still abort before the MR is marked merged. IfPostMergeServicefails before it sets the MR to merged (for example, a timeout at that exact point), the code falls through tohandle_locked_stuck_car, which unlocks the MR and aborts, destroying the car. This is existing behavior: the newmerged?guard only covers the case where the MR is already merged, and the fallback exists to avoid an infinite retry loop. This narrow window is not widened by this MR. - Partial post-merge side effects are not retryable.
PostMergeService#executestarts withreturn if merge_request.merged?, so its idempotency means "do nothing if already merged," not "resume where it left off." If it fails partway through, after marking the MR merged but before completing steps such as closing issues, sending notifications, or firing webhooks, those remaining steps are lost and cannot be retried, since the guard short-circuits on subsequent calls. This affects the normal merge path as well, not just merge trains, and is tracked separately in #628217.