Restore missing MR diff commit rows from the archived table
Diffs whose commit rows were skipped by the partitioning backfill still report a commits_count but list no commits, so the Commits tab, REST /commits and GraphQL commits come back empty. The rows still exist in merge_request_diff_commits_archived with their metadata pointer. When a read finds no rows for such a diff, this enqueues a worker that copies them back for that one diff, using the same statement as the corrected backfill in !255692 (merged). Behind restore_missing_mr_diff_commits, default off.
Related to https://gitlab.com/gitlab-org/gitlab/-/work_items/628194 and https://gitlab.com/gitlab-com/request-for-help/-/work_items/5353.
Query plan for the restore statement on one affected diff: https://console.postgres.ai/gitlab/projects/gitlab-production-main/sessions/57862/commands/162530
Detailed context for AI agents
Why rows, not a read-time fallback
!256341 (closed) rebuilds the list from Gitaly at read time. That leaves commit_shas readers (pipeline association, includes_any_commits?, approval committer filtering, MergeRequestDiff.ids_including_any_commits) inconsistent unless every one of them gains the fallback, adds an unmemoised Gitaly call to hot paths for broken diffs, raises on a missing ref for closed MRs whose refs are gone, and masks which rows the backfill still owes. Restoring the rows fixes every reader from the next request on and needs no Gitaly or branch. It also covers closed and merged MRs.
Data flow
- Detection:
MergeRequests::RestoreDiffCommitsWorker.schedule_for(diff) { predicate }, called only fromMergeRequestDiff#load_commits, which every visible commit list (REST/commits, GraphQLcommits, Commits tab) goes through via#commits. The predicate is empty result on the first page, and it can tell rows missing apart from Gitaly returning nothing for stored SHAs.Feature.enabled?is the first statement and the predicate block only runs when it passes, so with the flag off nothing else executes. Thenproject_idpresent,commits_count > 0,read_new_commits_table?, and not inside a transaction (Sidekiq forbids enqueueing there).commit_shasreaders are deliberately not hooked: they are internal helpers that cope with an empty list, and some run inside the diff creation transaction.commits_countis set from the rows when they are written, so rows missing with a count is a reliable corruption signal. Enqueues once per request viaSafeRequestStore. - Worker:
MergeRequests::RestoreDiffCommitsWorker, idempotent,deduplicate :until_executed, including_scheduled: true,data_consistency :sticky,urgency :low, deferred onmerge_request_diff_commitshealth. Re-checks flag, archived table existence and that rows are still missing, then callsMergeRequestDiffCommit.restore_from_archived(diff.id)and logsinserted_rows. - SQL:
MergeRequestDiffCommit.restore_from_archivedis the backfill's per-batch statement withmerge_request_diff_id = ?in place of the cursor window: four columns,project_idfrommerge_request_diffs(NOT NULL there, nullable on the archived table, and the partition key),merge_request_commits_metadata_id IS NOT NULL,ON CONFLICT DO NOTHING. No conflict target so it works on both the partitioned PK(merge_request_diff_id, relative_order, project_id)and the pre-swap PK. The only trigger that fires is the post-swap reverse sync into the archived table, which conflicts on every row and writes nothing. - No cache invalidation needed: the commit list is only instance-memoised,
commits_countis already correct, and nothing in Redis derives from the rows.
Scope
- GitLab.com only in effect: the archived table exists only where
SwapMergeRequestDiffCommitsTableran. Elsewhere the worker returns before the insert. Self-managed reads still go to the old table and are unaffected. - Does not fix the backfill predicate or the wider population. That is !255692 (merged). Both use
ON CONFLICT DO NOTHING, so they are idempotent against each other. Once that lands, the two can share the CTEs. - Console use for a known list is
MergeRequestDiffCommit.restore_from_archived(diff_id)per diff.
Rollout
Enable per project or group for the affected customers first. Enabling it before a console reload_diff run makes that script report MRs as already readable after the first request, so sequence accordingly.
Testing
Live run on GDK2 against a public MR with 10 commits: created merge_request_diff_commits_archived as a copy, moved the diff's rows into it with project_id NULL as on production, and confirmed REST /commits returned X-Total: 10 with an empty body, GraphQL commitCount 10 with commits empty, and the Commits tab count 10 with an empty list. With the read path told the table is partitioned, one read enqueued the worker, Sidekiq restored 10 rows in 143 ms with inserted_rows: 10 logged, and all three surfaces returned the same commits in the original order. Also verified live: flag off enqueues nothing, a read inside a transaction enqueues nothing, a later page enqueues nothing, two concurrent restore_from_archived calls returned 10 and 0 with no error, two concurrent worker runs on a re-broken diff produced exactly 10 rows, and repeat runs are no-ops.
Specs create a temporary merge_request_diff_commits_archived as a copy of the current table (shared context with archived merge_request_diff_commits) and move one diff's rows into it. Covered: restore order and count, idempotency, project_id sourced from the diff, rows without a pointer skipped, other diffs untouched, worker no-ops (flag off, table missing, diff missing, rows present), detection from #commits with and without load_from_gitaly, flag off runs only the flag check, once per request, no enqueue for later pages, flag off, old table, zero commits, healthy rows.