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#execute short-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_count reads the persisted merge_request_diffs.commits_count column (app/models/merge_request.rb:1080). merge_request.first_commit resolves the SHAs recorded for the diff and looks them up in the repository (app/models/merge_request_diff.rb:488 and :1035).
  • first_commit is nil whenever that lookup comes back empty, and the persisted count is not recomputed to match, so the two disagree and nil.safe_message raised NoMethodError. The rest of that line already used nil-safe navigation; first_commit was 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 collected state.
  • Mechanism: with the mr_diff_commits_read_new_table flag on and merge_request_diff_commits partitioned, commit SHAs are resolved exclusively through merge_request_commits_metadata, joined on merge_request_diff_commits.merge_request_commits_metadata_id and filtered by project_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 persisted commits_count still 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_id or sha is 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::MergeService only rescues MergeError and StrategyError (app/services/merge_requests/merge_service.rb:44), so the NoMethodError escaped MergeWorker entirely. The job burned its three Sidekiq retries and died, and nothing was written to the merge request's merge_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_request publishes MergeRequests::MergeableEvent when auto-merge is enabled and mergeability checks pass (app/controllers/projects/merge_requests_controller.rb:474), which runs MergeRequests::ProcessAutoMergeFromEventWorker, then AutoMergeService#process, which enqueues MergeWorker. Every reload of the merge request page re-triggered the crash.

Fix and resulting behaviour

  • With the guard nil-safe, a nil first_commit simply 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_sha and diff_head_sha still 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 StrategyError recorded on merge_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 identical NoMethodError and 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, stubbing first_commit to nil and asserting the service squashes.
  • Full file passes: 44 examples, 0 failures. Reverting only the service file makes the new example fail with NoMethodError in all 5 contexts that use the shared example, confirming the test is meaningful.
  • spec/services/merge_requests/merge_strategies/from_source_branch_spec.rb also 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 after validate_strategy! in app/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. ProcessAutoMergeFromEventWorker is idempotent! with the default until_executing strategy, which releases its lock before the job runs, and MergeWorker uses until_executed but deletes its key in an ensure, so a raised error releases it too.
Edited by Marc Shaw

Merge request reports

Loading
Loading