fix(provenance): close a repair race and stop memory double-reporting

The repair matched sidecars by the id its page captured, so a
provenance-aware write committing between the snapshot and the delete
had its fresh exact sidecar removed and its marker cleared behind it —
a secret-bearing row left reading as legacy. The delete now re-checks
status, which under READ COMMITTED re-evaluates against the writer's
committed row so it no longer matches.

Walk the candidate set by keyset over row_id. A page whose rows were
all repaired concurrently clears nothing, and terminating on "cleared
nothing" ended the walk with the rest of the backlog untouched.

Memory reported unrecorded provenance twice, and counted records even
when the surface was enforced — auditing a fail-open read that had
actually failed closed.

Author: icecrasher321

apps/sim/app/api/memory/secret-provenance.test.ts

@@ -5,7 +5,19 @@
 import { memorySecretProvenance } from '@sim/db/schema'
 import { dbChainMockFns, queueTableRows, resetDbChainMock } from '@sim/testing'
 import { NextRequest } from 'next/server'
-import { beforeEach, describe, expect, it } from 'vitest'
+import { beforeEach, describe, expect, it, vi } from 'vitest'
+
+const { mockIsEnforced, mockReport } = vi.hoisted(() => ({
+  mockIsEnforced: vi.fn(() => false),
+  mockReport: vi.fn(),
+}))
+
+vi.mock('@/lib/execution/durable-secret-provenance-enforcement', () => ({
+  DURABLE_SECRET_PROVENANCE_SURFACES: ['memory', 'table-row', 'knowledge'],
+  isDurableSecretProvenanceEnforced: mockIsEnforced,
+  reportUnrecordedDurableProvenance: mockReport,
+}))
+
 import { AuthType } from '@/lib/auth/hybrid'
 import {
   PRIVATE_SECRET_PROVENANCE_BUNDLE_V1,
@@ -47,6 +59,8 @@ function privateMemoryWrite(
 describe('memory write secret provenance', () => {
   beforeEach(() => {
     resetDbChainMock()
+    mockReport.mockClear()
+    mockIsEnforced.mockReturnValue(false)
   })
   it('classifies a headerless external write as exact-empty', () => {
     const request = new NextRequest('http://localhost/api/memory', { method: 'POST' })
@@ -267,4 +281,63 @@ describe('memory write secret provenance', () => {
       [RESOLVED_SECRET_PROVENANCE_FIELD]: { version: 1, complete: true, entries: [] },
     })
   })
+  /**
+   * One entry for the read, not one per record: the per-record import knows no workspace, so its
+   * report can only ever be a log line, and passing the workspace down instead would write
+   * thousands of audit rows for a single event.
+   */
+  it('reports one aggregated entry for a read that proceeded unvouched', async () => {
+    const request = new NextRequest('http://localhost/api/memory', {
+      headers: {
+        [PRIVATE_TOOL_METADATA_REQUEST_HEADER]: RESOLVED_SECRET_PROVENANCE_METADATA_V1,
+      },
+    })
+
+    await createMemoryResponse({
+      request,
+      authType: AuthType.INTERNAL_JWT,
+      userId: 'user-1',
+      workspaceId: 'workspace-1',
+      body: { success: true },
+      memories: [
+        { id: 'memory-1', data: 'value', secretProvenanceVersion: 1 },
+        { id: 'memory-2', data: 'value', secretProvenanceVersion: 1 },
+      ],
+    })
+
+    expect(mockReport).toHaveBeenCalledTimes(1)
+    expect(mockReport).toHaveBeenCalledWith(
+      expect.objectContaining({
+        surface: 'memory',
+        cause: 'durable-provenance-unknown',
+        affectedCount: 2,
+        workspaceId: 'workspace-1',
+      })
+    )
+  })
+
+  /**
+   * Under enforcement the import fails the registry closed rather than proceeding, so there is no
+   * fail-open read to record. Counting those records anyway would audit something that never
+   * happened, in the one trail whose whole purpose is to say a read went ahead unvouched.
+   */
+  it('records nothing when the surface is enforced and the read fails closed', async () => {
+    mockIsEnforced.mockReturnValue(true)
+    const request = new NextRequest('http://localhost/api/memory', {
+      headers: {
+        [PRIVATE_TOOL_METADATA_REQUEST_HEADER]: RESOLVED_SECRET_PROVENANCE_METADATA_V1,
+      },
+    })
+
+    await createMemoryResponse({
+      request,
+      authType: AuthType.INTERNAL_JWT,
+      userId: 'user-1',
+      workspaceId: 'workspace-1',
+      body: { success: true },
+      memories: [{ id: 'memory-1', data: 'value', secretProvenanceVersion: 1 }],
+    })
+
+    expect(mockReport).not.toHaveBeenCalled()
+  })
 })

apps/sim/app/api/memory/secret-provenance.ts

@@ -9,7 +9,10 @@ import {
   EXACT_EMPTY_DURABLE_SECRET_PROVENANCE,
   importDurableSecretProvenance,
 } from '@/lib/execution/durable-secret-provenance'
-import { reportUnrecordedDurableProvenance } from '@/lib/execution/durable-secret-provenance-enforcement'
+import {
+  isDurableSecretProvenanceEnforced,
+  reportUnrecordedDurableProvenance,
+} from '@/lib/execution/durable-secret-provenance-enforcement'
 import {
   inspectPrivateSecretProvenanceRequest,
   isPrivateSecretProvenanceBundleV1,
@@ -121,6 +124,12 @@ export async function createMemoryResponse(options: {
     matching.push(memory)
     memoriesById.set(memory.id, matching)
   }
+  /**
+   * Counted only while the surface is open. Under enforcement the import fails the registry closed
+   * instead of proceeding, so counting those records would audit a fail-open read that never
+   * happened — and this entry exists precisely to say a read went ahead unvouched.
+   */
+  const memoryEnforced = isDurableSecretProvenanceEnforced('memory')
   let unrecordedMemoryCount = 0
   for (let index = 0; index < ids.length; index += PRIVATE_MEMORY_QUERY_CHUNK_SIZE) {
     const pageIds = ids.slice(index, index + PRIVATE_MEMORY_QUERY_CHUNK_SIZE)
@@ -139,8 +148,10 @@ export async function createMemoryResponse(options: {
           status: sidecar?.status ?? null,
           entries: sidecar?.entries,
         })
-        if (provenance.status === 'unknown') unrecordedMemoryCount += 1
-        await importDurableSecretProvenance(registry, provenance, record.data, 'memory')
+        if (provenance.status === 'unknown' && !memoryEnforced) unrecordedMemoryCount += 1
+        await importDurableSecretProvenance(registry, provenance, record.data, 'memory', {
+          reportUnrecorded: false,
+        })
       }
     }
   }

apps/sim/lib/execution/durable-secret-provenance.ts

@@ -209,11 +209,22 @@ export async function importDurableSecretProvenance(
   registry: ResolvedSecretTraceRegistry,
   provenance: DurableSecretProvenance,
   value?: unknown,
-  surface?: DurableSecretProvenanceSurface
+  surface?: DurableSecretProvenanceSurface,
+  /**
+   * Set by a caller that reports the whole read itself.
+   *
+   * This function sees one record and knows no workspace, so its report can only ever be a log
+   * line, one per record. A caller reading a page can say the same thing once, with the workspace
+   * and the count — which is the entry that reaches the people who own the secrets. Both reporting
+   * would double-count the same event at two different granularities.
+   */
+  options: { reportUnrecorded?: boolean } = {}
 ): Promise<boolean> {
   if (provenance.status === 'unknown') {
     if (surface && !isDurableSecretProvenanceEnforced(surface)) {
-      reportUnrecordedDurableProvenance({ surface, cause: 'durable-provenance-unknown' })
+      if (options.reportUnrecorded !== false) {
+        reportUnrecordedDurableProvenance({ surface, cause: 'durable-provenance-unknown' })
+      }
       return true
     }
     registry.markIncomplete('durable-provenance-unknown')

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

@@ -0,0 +1,83 @@
+/**
+ * @vitest-environment node
+ */
+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[]): {
+  sql: Sql
+  cursors: unknown[]
+  statements: string[]
+} {
+  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 }
+}
+
+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.
+   */
+  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 },
+    ])
+
+    await repairUnknownTableRowProvenance.up(sql)
+
+    expect(statements[0]).toContain('DELETE FROM user_table_row_secret_provenance')
+    expect(statements[0]).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 },
+    ])
+
+    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 },
+    ])
+
+    await repairUnknownTableRowProvenance.up(sql)
+
+    expect(cursors).toEqual(['', 'row-1'])
+  })
+})

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

@@ -3,34 +3,62 @@ import type { ScriptMigration } from './types'
 
 export const UNKNOWN_PROVENANCE_REPAIR_BATCH_SIZE = 1000
 
+interface RepairPage {
+  /** Rows still reading `unknown` when the page was selected; zero means the walk is done. */
+  candidates: number
+  /** Rows actually returned to untracked. Lower than `candidates` when a writer got there first. */
+  repaired: number
+  /** Highest `row_id` in the page, so the next pass resumes past it. */
+  lastRowId: string | null
+}
+
 /**
- * Returns one page of `unknown` rows to the untracked state, and reports how many it cleared.
+ * 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.
  *
- * Both halves are required and 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.
+ * 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.
  *
  * `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.
  */
-async function repairUnknownProvenancePage(sql: Sql, batchSize: number): Promise<number> {
-  const repaired = await sql<{ id: string }[]>`
+async function repairUnknownProvenancePage(
+  sql: Sql,
+  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'
+      WHERE status = 'unknown' AND row_id > ${afterRowId}
+      ORDER BY row_id
       LIMIT ${batchSize}
     ), cleared AS (
       DELETE FROM user_table_row_secret_provenance
       WHERE row_id IN (SELECT row_id FROM page)
+        AND status = 'unknown'
       RETURNING row_id
+    ), marked AS (
+      UPDATE user_table_rows
+      SET secret_provenance_version = NULL
+      WHERE id IN (SELECT row_id FROM cleared)
+      RETURNING id
     )
-    UPDATE user_table_rows
-    SET secret_provenance_version = NULL
-    WHERE id IN (SELECT row_id FROM cleared)
-    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 repaired.length
+  return page
 }
 
 /**
@@ -56,17 +84,33 @@ async function repairUnknownProvenancePage(sql: Sql, batchSize: number): Promise
  * Idempotent and resumable: a repaired row no longer has a sidecar, so it leaves the candidate set
  * and a re-run after a crash resumes on what remains. Rows that become unknown after this runs are
  * simply left for the writers now instrumented to report them.
+ *
+ * Walked by keyset over `row_id` rather than by re-selecting the head of the candidate set. A page
+ * whose rows were all repaired by a concurrent writer clears nothing, and terminating on "cleared
+ * nothing" would have ended the walk there and left the rest of the backlog untouched. Advancing
+ * 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 const repairUnknownTableRowProvenance: ScriptMigration = {
   name: '0005_repair_unknown_table_row_provenance',
   async up(sql: Sql): Promise<void> {
     let repaired = 0
+    let skipped = 0
+    let afterRowId = ''
     for (;;) {
-      const page = await repairUnknownProvenancePage(sql, UNKNOWN_PROVENANCE_REPAIR_BATCH_SIZE)
-      if (page === 0) break
-      repaired += page
+      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).`)
+    console.log(
+      `Unknown table row provenance repair complete: ${repaired} row(s) repaired, ${skipped} left to a concurrent writer.`
+    )
   },
 }