fix(provenance): take the repair's locks in the writer's order

The repair deleted the sidecar and only then updated its parent row,
while mutateTableRowsWithSecretProvenance locks user_table_rows up front
and upserts the sidecar inside the same transaction. Opposite orders, so
an overlapping write deadlocked and Postgres resolved it by aborting
either the deployment or somebody's table write.

Lock the parent first, in id order, matching lockTableRows. Holding that
lock is also what makes the status re-check decisive rather than racy:
the writer commits its sidecar and its marker under the same lock, so
once it is held the write is either wholly done or has not begun.

Author: icecrasher321

packages/db/script-migrations/0005_repair_unknown_table_row_provenance.test.ts

@@ -5,76 +5,100 @@ import type { Sql } from 'postgres'
 import { describe, expect, it, vi } from 'vitest'
 import { repairUnknownTableRowProvenance } from './0005_repair_unknown_table_row_provenance'
 
-interface PageResult {
-  candidates: number
-  repaired: number
-  lastRowId: string | null
-}
-
 function normalizeSql(value: string): string {
   return value.replace(/\s+/g, ' ').trim()
 }
 
-/** Replays a scripted sequence of pages and records the `afterRowId` each pass asked for. */
-function createSqlHarness(pages: PageResult[]): {
+/**
+ * Replays a scripted sequence of candidate pages and records every statement in the order it was
+ * issued, so a test can assert on lock ordering rather than only on the final counts.
+ */
+function createSqlHarness(pages: string[][]): {
   sql: Sql
-  cursors: unknown[]
   statements: string[]
+  cursors: unknown[]
 } {
-  const cursors: unknown[] = []
   const statements: string[] = []
-  let call = 0
-  const query = vi.fn((strings: TemplateStringsArray, ...values: unknown[]) => {
-    statements.push(normalizeSql(strings.join('?')))
-    /** `afterRowId` is interpolated before the page size, so it is the first bound value. */
-    cursors.push(values[0])
-    const page = pages[call] ?? { candidates: 0, repaired: 0, lastRowId: null }
-    call += 1
-    return Promise.resolve([page])
-  })
-  return { sql: query as unknown as Sql, cursors, statements }
+  const cursors: unknown[] = []
+  let page = 0
+
+  const run = (strings: TemplateStringsArray, ...values: unknown[]) => {
+    const text = normalizeSql(strings.join('?'))
+    statements.push(text)
+
+    if (text.startsWith('SELECT row_id AS "rowId"')) {
+      cursors.push(values[0])
+      const rows = (pages[page] ?? []).map((rowId) => ({ rowId }))
+      page += 1
+      return Promise.resolve(rows)
+    }
+    if (text.startsWith('DELETE FROM user_table_row_secret_provenance')) {
+      const ids = (values[0] as string[]) ?? []
+      return Promise.resolve(ids.map((rowId) => ({ rowId })))
+    }
+    if (text.startsWith('UPDATE user_table_rows')) {
+      const ids = (values[0] as string[]) ?? []
+      return Promise.resolve(ids.map((id) => ({ id })))
+    }
+    return Promise.resolve([])
+  }
+
+  const sql = run as unknown as Sql
+  sql.begin = vi.fn(async (callback) => (callback as (tx: Sql) => unknown)(sql)) as Sql['begin']
+  return { sql, statements, cursors }
 }
 
 describe('0005 repair unknown table row provenance', () => {
   /**
-   * A provenance-aware write commits its exact sidecar between this statement's snapshot and its
-   * delete. Matching on the captured id alone would drop that fresh sidecar and clear the marker
-   * behind it, leaving a secret-bearing row reading as legacy — provenance destroyed by the repair
-   * meant to make provenance safe. The re-check is what makes the writer's row stop matching.
+   * `mutateTableRowsWithSecretProvenance` locks `user_table_rows` up front and upserts the sidecar
+   * inside the same transaction. Touching the sidecar first is the opposite order, and an
+   * overlapping write would deadlock — Postgres resolving it by aborting either the deployment or
+   * somebody's table write.
+   */
+  it('locks the parent row before touching the sidecar, in the order writers take them', async () => {
+    const { sql, statements } = createSqlHarness([['row-1', 'row-2'], []])
+
+    await repairUnknownTableRowProvenance.up(sql)
+
+    const lockIndex = statements.findIndex((s) => s.includes('FOR UPDATE'))
+    const deleteIndex = statements.findIndex((s) =>
+      s.startsWith('DELETE FROM user_table_row_secret_provenance')
+    )
+    expect(lockIndex).toBeGreaterThanOrEqual(0)
+    expect(deleteIndex).toBeGreaterThan(lockIndex)
+    expect(statements[lockIndex]).toContain('ORDER BY id')
+  })
+
+  /**
+   * A provenance-aware write commits its exact sidecar and its marker together. Matching on the
+   * captured id alone would drop that fresh sidecar and clear the marker behind it, leaving a
+   * secret-bearing row reading as legacy.
    */
   it('only deletes sidecars still reading unknown', async () => {
-    const { sql, statements } = createSqlHarness([
-      { candidates: 1, repaired: 1, lastRowId: 'row-1' },
-      { candidates: 0, repaired: 0, lastRowId: null },
-    ])
+    const { sql, statements } = createSqlHarness([['row-1'], []])
 
     await repairUnknownTableRowProvenance.up(sql)
 
-    expect(statements[0]).toContain('DELETE FROM user_table_row_secret_provenance')
-    expect(statements[0]).toContain("AND status = 'unknown'")
+    const deleteStatement = statements.find((s) =>
+      s.startsWith('DELETE FROM user_table_row_secret_provenance')
+    )
+    expect(deleteStatement).toContain("AND status = 'unknown'")
   })
 
   /**
    * A page whose rows were all repaired by a concurrent writer clears nothing. Stopping there would
    * have ended the walk and left the rest of the backlog untouched.
    */
   it('keeps walking past a page a concurrent writer already repaired', async () => {
-    const { sql, cursors } = createSqlHarness([
-      { candidates: 2, repaired: 0, lastRowId: 'row-2' },
-      { candidates: 1, repaired: 1, lastRowId: 'row-9' },
-      { candidates: 0, repaired: 0, lastRowId: null },
-    ])
+    const { sql, cursors } = createSqlHarness([['row-1', 'row-2'], ['row-9'], []])
 
     await repairUnknownTableRowProvenance.up(sql)
 
     expect(cursors).toEqual(['', 'row-2', 'row-9'])
   })
 
   it('stops on the first page with no candidates left', async () => {
-    const { sql, cursors } = createSqlHarness([
-      { candidates: 1, repaired: 1, lastRowId: 'row-1' },
-      { candidates: 0, repaired: 0, lastRowId: null },
-    ])
+    const { sql, cursors } = createSqlHarness([['row-1'], []])
 
     await repairUnknownTableRowProvenance.up(sql)
 

packages/db/script-migrations/0005_repair_unknown_table_row_provenance.ts

@@ -15,17 +15,19 @@ interface RepairPage {
 /**
  * Returns one page of `unknown` rows to the untracked state.
  *
- * Both halves belong in one statement. Clearing the marker while a sidecar row survives beside it
- * is the state a derived table transformation reads as unknown, so a split repair would be undone
- * by the next column operation.
+ * Takes the parent row lock before touching the sidecar, in `id` order, because that is the order
+ * `mutateTableRowsWithSecretProvenance` takes them: it locks `user_table_rows` up front, then
+ * upserts the sidecar inside the same transaction. Deleting the sidecar first and only then
+ * updating the parent is the opposite order, so an overlapping write would deadlock — and Postgres
+ * would resolve it by aborting either the deployment or somebody's table write. Sharing the
+ * writer's order means the two serialize instead.
  *
- * The delete re-checks `status` rather than trusting the id the page captured. A provenance-aware
- * write commits its exact sidecar and its version marker together, and can land between this
- * statement's snapshot and its delete; matching on `row_id` alone would drop that fresh exact
- * sidecar and clear the marker behind it, leaving a genuinely secret-bearing row reading as
- * legacy — provenance destroyed by the repair meant to make provenance safe. Under READ COMMITTED
- * the delete re-evaluates its condition against the updated row, so the writer's row no longer
- * matches and is left alone; whichever of the two commits second sees the other's result.
+ * Holding the parent lock is also what makes the status re-check below decisive rather than
+ * racy: a provenance-aware write commits its exact sidecar and its version marker together under
+ * that same lock, so once it is held the write is either wholly done or has not begun. Matching on
+ * the id alone would drop a freshly exact sidecar and clear the marker behind it, leaving a
+ * genuinely secret-bearing row reading as legacy — provenance destroyed by the repair meant to make
+ * provenance safe.
  *
  * `secret_provenance_version` is not a column the demote trigger watches, so this leaves
  * `updated_at` alone and cannot disturb a concurrent write's sidecar binding.
@@ -35,30 +37,44 @@ async function repairUnknownProvenancePage(
   batchSize: number,
   afterRowId: string
 ): Promise<RepairPage> {
-  const [page] = await sql<[RepairPage]>`
-    WITH page AS (
-      SELECT row_id
-      FROM user_table_row_secret_provenance
-      WHERE status = 'unknown' AND row_id > ${afterRowId}
-      ORDER BY row_id
-      LIMIT ${batchSize}
-    ), cleared AS (
+  const candidates = await sql<{ rowId: string }[]>`
+    SELECT row_id AS "rowId"
+    FROM user_table_row_secret_provenance
+    WHERE status = 'unknown' AND row_id > ${afterRowId}
+    ORDER BY row_id
+    LIMIT ${batchSize}
+  `
+  if (candidates.length === 0) return { candidates: 0, repaired: 0, lastRowId: null }
+  const rowIds = candidates.map((candidate) => candidate.rowId)
+
+  const repaired = await sql.begin(async (tx) => {
+    await tx`
+      SELECT id FROM user_table_rows
+      WHERE id = ANY(${rowIds}::text[])
+      ORDER BY id
+      FOR UPDATE
+    `
+    const cleared = await tx<{ rowId: string }[]>`
       DELETE FROM user_table_row_secret_provenance
-      WHERE row_id IN (SELECT row_id FROM page)
+      WHERE row_id = ANY(${rowIds}::text[])
         AND status = 'unknown'
-      RETURNING row_id
-    ), marked AS (
+      RETURNING row_id AS "rowId"
+    `
+    if (cleared.length === 0) return 0
+    const marked = await tx<{ id: string }[]>`
       UPDATE user_table_rows
       SET secret_provenance_version = NULL
-      WHERE id IN (SELECT row_id FROM cleared)
+      WHERE id = ANY(${cleared.map((row) => row.rowId)}::text[])
       RETURNING id
-    )
-    SELECT
-      (SELECT count(*) FROM page)::int AS "candidates",
-      (SELECT count(*) FROM marked)::int AS "repaired",
-      (SELECT max(row_id) FROM page) AS "lastRowId"
-  `
-  return page
+    `
+    return marked.length
+  })
+
+  return {
+    candidates: rowIds.length,
+    repaired: repaired as number,
+    lastRowId: rowIds[rowIds.length - 1],
+  }
 }
 
 /**