Loading
Fix crash when squashing a merge request with a missing commit
Fixes a NoMethodError that killed MergeWorker when a merge request's first commit could not be resolved.
The no-op-squash guard compares a persisted commit count against a live lookup of that commit's message, and the lookup can return nil while the count still says 1. nil.safe_message then raised inside the merge worker, nothing was recorded on the merge request, and auto-merge sat there silently. This adds the missing nil-safe navigation so the guard falls through to a real squash instead, plus a spec covering that case.
Detailed context for AI agents
Root cause
MergeRequests::SquashService#executeshort-circuits when a squash would be a no-op: a single commit, and the squash message already equal to that commit's message (app/services/merge_requests/squash_service.rb:18).- The two halves of that guard read from different sources.
merge_request.commits_countreads the persistedmerge_request_diffs.commits_countcolumn (app/models/merge_request.rb:1080).merge_request.first_commitresolves the SHAs recorded for the diff and looks them up in the repository (app/models/merge_request_diff.rb:488and:1035). first_commitis nil whenever that lookup comes back empty, and the persisted count is not recomputed to match, so the two disagree andnil.safe_messageraisedNoMethodError. The rest of that line already used nil-safe navigation;first_commitwas the one call that did not.- Two things make the lookup come back empty. Either the repository no longer has the object, or the SHA list itself is empty because the diff's commit rows are not readable.
- The second one is what happens in practice on GitLab.com. Some merge request diffs currently return zero commits even though their commits exist in the repository. Public example: !239766 (merged), where all three diff versions return zero commits from the API, including the latest one in
collectedstate. - Mechanism: with the
mr_diff_commits_read_new_tableflag on andmerge_request_diff_commitspartitioned, commit SHAs are resolved exclusively throughmerge_request_commits_metadata, joined onmerge_request_diff_commits.merge_request_commits_metadata_idand filtered byproject_id(app/models/merge_request_diff_commit.rb:192). It is an INNER JOIN, so a diff whose rows are absent, or present without a metadata id, yields zero SHAs while the persistedcommits_countstill says one. - Why some rows are in that state is a separate question and out of scope here. Contributing factors visible in the code: the backfill deliberately skips legacy rows where
commit_author_id,committer_idorshais NULL because the metadata table declares those columns NOT NULL (lib/gitlab/background_migration/backfill_merge_request_diff_commits_to_partitioned.rb:115), and the pre-swap sync trigger only mirrored rows that already had a metadata id.
Why the crash escaped
MergeRequests::MergeServiceonly rescuesMergeErrorandStrategyError(app/services/merge_requests/merge_service.rb:44), so theNoMethodErrorescapedMergeWorkerentirely. The job burned its three Sidekiq retries and died, and nothing was written to the merge request'smerge_error, so an auto-merge sat there with no explanation for the user.- The path is reachable from an ordinary page view.
Projects::MergeRequestsController#show_merge_requestpublishesMergeRequests::MergeableEventwhen auto-merge is enabled and mergeability checks pass (app/controllers/projects/merge_requests_controller.rb:474), which runsMergeRequests::ProcessAutoMergeFromEventWorker, thenAutoMergeService#process, which enqueuesMergeWorker. Every reload of the merge request page re-triggered the crash.
Fix and resulting behaviour
- With the guard nil-safe, a nil
first_commitsimply fails the equality check, so the service falls through to the real squash instead of raising. - In the case observed on GitLab.com, where the commit rows are unreadable but the git objects are fine,
diff_start_shaanddiff_head_shastill resolve, so the squash succeeds and the merge proceeds normally. - If the revisions genuinely cannot be resolved, the squash fails cleanly and returns an error that surfaces as a
StrategyErrorrecorded onmerge_error. Either way the worker no longer dies unhandled.
Verification performed
- Reproduced the crash locally on a GDK by leaving a diff pointing at a commit its repository could not resolve, then running
MergeWorker. Got the identicalNoMethodErrorand backtrace frames. With the fix applied the same run no longer raises. - New spec example in
spec/services/merge_requests/squash_service_spec.rb, inside the existing single-commit context, stubbingfirst_committo nil and asserting the service squashes. - Full file passes: 44 examples, 0 failures. Reverting only the service file makes the new example fail with
NoMethodErrorin all 5 contexts that use the shared example, confirming the test is meaningful. spec/services/merge_requests/merge_strategies/from_source_branch_spec.rbalso passes: 26 examples, 0 failures.- RuboCop clean on both files.
Deliberately out of scope
- Why some diffs have unreadable commit rows. That belongs with the partitioning work, not with this fix.
- The error message shown when the revisions genuinely cannot be resolved is misleading: the user is told to resolve conflicts locally. The check that would say something honest,
updated_check!("Branch has been updated since the merge was requested"), runs aftervalidate_strategy!inapp/services/merge_requests/merge_service.rb:57-58, so it is never reached. - Nothing stops the merge request page from re-triggering the whole chain on every load.
ProcessAutoMergeFromEventWorkerisidempotent!with the defaultuntil_executingstrategy, which releases its lock before the job runs, andMergeWorkerusesuntil_executedbut deletes its key in anensure, so a raised error releases it too.
Edited by Marc Shaw