From 28290e2face82855524e0a3fcfabc018eeec3000 Mon Sep 17 00:00:00 2001 From: Vikhyath Mondreti Date: Sat, 22 Aug 2026 12:42:56 -0700 Subject: [PATCH 1/2] fix(provenance): clear the rows that went unknown after the first repair MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 0005 is finished and will not run again — the runner records a name in script_migrations and never offers it back, which is the contract a run-once repair wants. But it cleared the backlog that existed at the instant it ran, and the writer that produced that backlog kept running until the fix in this branch. Nothing heals such a row in place, so each one goes on reporting on every later read; a few dozen of them account for thousands of log lines a week. A second entry rather than deleting the first's tracking row: the registry is append-only, and a repair that ran twice should say so twice. It shares 0005's walk rather than restating it — the parent-first lock ordering and the status re-check under that lock are subtleties worth having once — and is idempotent, so it costs one empty query if there is nothing left to repair. --- ...rations-paused-billing-attribution.test.ts | 1 + ...epair_unknown_table_row_provenance.test.ts | 29 +++++++++++++ ...005_repair_unknown_table_row_provenance.ts | 42 ++++++++++--------- ...nknown_table_row_provenance_second_pass.ts | 31 ++++++++++++++ packages/db/script-migrations/index.ts | 2 + 5 files changed, 85 insertions(+), 20 deletions(-) create mode 100644 packages/db/script-migrations/0006_repair_unknown_table_row_provenance_second_pass.ts diff --git a/packages/db/script-migrations-paused-billing-attribution.test.ts b/packages/db/script-migrations-paused-billing-attribution.test.ts index edc4ec2a0a9..9468b57a89d 100644 --- a/packages/db/script-migrations-paused-billing-attribution.test.ts +++ b/packages/db/script-migrations-paused-billing-attribution.test.ts @@ -442,6 +442,7 @@ describe('script migration registry', () => { '0003_backfill_workspace_storage_usage', '0004_backfill_fork_kb_file_ownership', '0005_repair_unknown_table_row_provenance', + '0006_repair_unknown_table_row_provenance_second_pass', ]) }) }) diff --git a/packages/db/script-migrations/0005_repair_unknown_table_row_provenance.test.ts b/packages/db/script-migrations/0005_repair_unknown_table_row_provenance.test.ts index 0972f61a5e0..f486ec1f0a9 100644 --- a/packages/db/script-migrations/0005_repair_unknown_table_row_provenance.test.ts +++ b/packages/db/script-migrations/0005_repair_unknown_table_row_provenance.test.ts @@ -4,6 +4,7 @@ import type { Sql } from 'postgres' import { describe, expect, it, vi } from 'vitest' import { repairUnknownTableRowProvenance } from './0005_repair_unknown_table_row_provenance' +import { repairUnknownTableRowProvenanceSecondPass } from './0006_repair_unknown_table_row_provenance_second_pass' function normalizeSql(value: string): string { return value.replace(/\s+/g, ' ').trim() @@ -105,3 +106,31 @@ describe('0005 repair unknown table row provenance', () => { expect(cursors).toEqual(['', 'row-1']) }) }) + +/** + * 0005 is finished — the runner records a name and never offers it again — but it cleared only the + * backlog that existed when it ran, and the writers producing that backlog kept running. The second + * pass exists to clear what accumulated since, and shares the first's implementation because the + * lock ordering and the status re-check are subtleties worth having once. + */ +describe('0006 second pass', () => { + it('repairs on the same walk as the first pass rather than restating it', async () => { + const { sql, statements, cursors } = createSqlHarness([['row-1'], []]) + + await repairUnknownTableRowProvenanceSecondPass.up(sql) + + expect(cursors).toEqual(['', 'row-1']) + const lockIndex = statements.findIndex((statement) => statement.includes('FOR UPDATE')) + const deleteIndex = statements.findIndex((statement) => + statement.startsWith('DELETE FROM user_table_row_secret_provenance') + ) + expect(deleteIndex).toBeGreaterThan(lockIndex) + expect(statements[deleteIndex]).toContain("AND status = 'unknown'") + }) + + it('is a distinct entry so a repair that ran twice is recorded twice', () => { + expect(repairUnknownTableRowProvenanceSecondPass.name).not.toBe( + repairUnknownTableRowProvenance.name + ) + }) +}) diff --git a/packages/db/script-migrations/0005_repair_unknown_table_row_provenance.ts b/packages/db/script-migrations/0005_repair_unknown_table_row_provenance.ts index b04cd4d7053..2cb126ee6df 100644 --- a/packages/db/script-migrations/0005_repair_unknown_table_row_provenance.ts +++ b/packages/db/script-migrations/0005_repair_unknown_table_row_provenance.ts @@ -107,26 +107,28 @@ async function repairUnknownProvenancePage( * past the page instead makes each pass finite and the whole walk terminate on the only condition * that means finished: a page with no candidates left in it. */ +export async function runUnknownTableRowProvenanceRepair(sql: Sql): Promise { + let repaired = 0 + let skipped = 0 + let afterRowId = '' + for (;;) { + const page = await repairUnknownProvenancePage( + sql, + UNKNOWN_PROVENANCE_REPAIR_BATCH_SIZE, + afterRowId + ) + if (page.candidates === 0 || page.lastRowId === null) break + repaired += page.repaired + skipped += page.candidates - page.repaired + afterRowId = page.lastRowId + console.log(` repaired ${repaired} unknown table row(s)`) + } + console.log( + `Unknown table row provenance repair complete: ${repaired} row(s) repaired, ${skipped} left to a concurrent writer.` + ) +} + export const repairUnknownTableRowProvenance: ScriptMigration = { name: '0005_repair_unknown_table_row_provenance', - async up(sql: Sql): Promise { - let repaired = 0 - let skipped = 0 - let afterRowId = '' - for (;;) { - const page = await repairUnknownProvenancePage( - sql, - UNKNOWN_PROVENANCE_REPAIR_BATCH_SIZE, - afterRowId - ) - if (page.candidates === 0 || page.lastRowId === null) break - repaired += page.repaired - skipped += page.candidates - page.repaired - afterRowId = page.lastRowId - console.log(` repaired ${repaired} unknown table row(s)`) - } - console.log( - `Unknown table row provenance repair complete: ${repaired} row(s) repaired, ${skipped} left to a concurrent writer.` - ) - }, + up: runUnknownTableRowProvenanceRepair, } diff --git a/packages/db/script-migrations/0006_repair_unknown_table_row_provenance_second_pass.ts b/packages/db/script-migrations/0006_repair_unknown_table_row_provenance_second_pass.ts new file mode 100644 index 00000000000..d536cabf298 --- /dev/null +++ b/packages/db/script-migrations/0006_repair_unknown_table_row_provenance_second_pass.ts @@ -0,0 +1,31 @@ +import type { Sql } from 'postgres' +import { runUnknownTableRowProvenanceRepair } from './0005_repair_unknown_table_row_provenance' +import type { ScriptMigration } from './types' + +/** + * Clears the rows that went `unknown` after 0005 had already run. + * + * 0005 was correct and is finished: the runner records a name in `script_migrations` and never + * offers it again, which is exactly the contract a run-once repair wants. But it cleared the + * backlog that existed at the instant it ran, and the writers that produced that backlog kept + * running afterwards — a table write whose block could not project one input latched the run's + * registry, so its rows were stored unrecorded, for as long as that bug was live. + * + * Nothing heals such a row in place; a partial cell update keeps it unknown and only a full replace + * carrying complete provenance clears it. So each one goes on reporting on every later read, which + * is why a few dozen rows account for thousands of log lines a week. A second pass is the whole + * remedy. + * + * A new entry rather than deleting 0005's tracking row: the registry is append-only, and a repair + * that ran twice should say so twice. Ordered after the fix that stopped producing these — repairing + * while the writer still creates them only refills the backlog. + * + * Shares 0005's implementation rather than restating it. The walk locks the parent row before the + * sidecar to match the application writer's order, and re-checks `status` under that lock so a + * concurrently committed exact sidecar is never deleted — subtleties worth having once, not twice. + * Idempotent, so it costs one empty query when there is nothing left to repair. + */ +export const repairUnknownTableRowProvenanceSecondPass: ScriptMigration = { + name: '0006_repair_unknown_table_row_provenance_second_pass', + up: (sql: Sql) => runUnknownTableRowProvenanceRepair(sql), +} diff --git a/packages/db/script-migrations/index.ts b/packages/db/script-migrations/index.ts index 8f022c456fe..4bdfa6153d3 100644 --- a/packages/db/script-migrations/index.ts +++ b/packages/db/script-migrations/index.ts @@ -4,6 +4,7 @@ import { backfillPausedBillingAttribution } from './0002_backfill_paused_billing import { backfillWorkspaceStorageUsage } from './0003_backfill_workspace_storage_usage' import { backfillForkKnowledgeBaseFileOwnership } from './0004_backfill_fork_kb_file_ownership' import { repairUnknownTableRowProvenance } from './0005_repair_unknown_table_row_provenance' +import { repairUnknownTableRowProvenanceSecondPass } from './0006_repair_unknown_table_row_provenance_second_pass' import type { ScriptMigration } from './types' export type { ScriptMigration } from './types' @@ -19,6 +20,7 @@ export const scriptMigrations: readonly ScriptMigration[] = [ backfillWorkspaceStorageUsage, backfillForkKnowledgeBaseFileOwnership, repairUnknownTableRowProvenance, + repairUnknownTableRowProvenanceSecondPass, ] /** From ddd35073ed97d70f33b659a68c6f676d34c821d3 Mon Sep 17 00:00:00 2001 From: Vikhyath Mondreti Date: Sat, 22 Aug 2026 13:30:56 -0700 Subject: [PATCH 2/2] chore(provenance): record the deploy ordering the second pass depends on MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit promote-images needs migrate, so a script migration runs while the previous image is still serving. A row an old instance creates between this walk and the end of the rollout sits behind the cursor, and the name is recorded on success, so it is never offered again. Widening the walk would not help — the exposure is the minutes after it returns, not the milliseconds during — so the requirement is to ship it in a release after the writer fix is already promoted, which the file now says. --- ...repair_unknown_table_row_provenance_second_pass.ts | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/packages/db/script-migrations/0006_repair_unknown_table_row_provenance_second_pass.ts b/packages/db/script-migrations/0006_repair_unknown_table_row_provenance_second_pass.ts index d536cabf298..668694c4c91 100644 --- a/packages/db/script-migrations/0006_repair_unknown_table_row_provenance_second_pass.ts +++ b/packages/db/script-migrations/0006_repair_unknown_table_row_provenance_second_pass.ts @@ -17,8 +17,15 @@ import type { ScriptMigration } from './types' * remedy. * * A new entry rather than deleting 0005's tracking row: the registry is append-only, and a repair - * that ran twice should say so twice. Ordered after the fix that stopped producing these — repairing - * while the writer still creates them only refills the backlog. + * that ran twice should say so twice. + * + * It assumes the writer that produced these is already live-fixed, and that is a deploy-ordering + * requirement rather than something this file can enforce. `promote-images` needs `migrate`, so a + * script migration runs while the previous image is still serving: any row an old instance creates + * between this walk and the end of the rollout is behind the cursor, and the name is recorded on + * success, so it is never offered again. Widening the walk would not help — it is the minutes after + * it returns that are exposed, not the milliseconds during. Ship this in a release *after* the + * writer fix is already promoted, and the window closes because nothing is producing rows to miss. * * Shares 0005's implementation rather than restating it. The walk locks the parent row before the * sidecar to match the application writer's order, and re-checks `status` under that lock so a