fix(relay): scope the stand-down to its statement and count violating transactions

Two changes from a second review round, both of which make the guard safer with
less machinery than the alternatives I was considering.

The stand-down was a transaction-wide latch: one locked read whose projection
omits cell_id muted every statement after it, so a genuinely descending write in
the same transaction went unreported. Scoping it to the statement is strictly
safer and needs no alert policy to compensate, because the only effect of a
missing `held` entry is to REMOVE a report, never to invent one -- the case that
looks like a counterexample, where an unreadable read takes a row and a later
write names it, is a true violation with misleading attribution rather than a
false one. "Select only the columns you need" is a natural future edit, and it
should cost one statement of coverage, not a whole transaction of silence.

Violations now count violating TRANSACTIONS. Merging attempts with a max cannot
inflate, but it can zero out an observed violation: an attempt that violates and
then hits a retryable 55P03 may be followed by a clean attempt down a different
branch, which would erase the only signal this ships for. The gate is
zero-versus-nonzero, so what must survive is "this transaction violated at least
once". Magnitude stays in the distinct-signature log.

Also: ORDER BY now tolerates an alias, because `ORDER BY cell.cell_id ASC` orders
identically and rejecting it would throw in tests on correct SQL. And the comment
claiming a JOIN locks no cell row is wrong -- Postgres FOR UPDATE without OF locks
every table in the FROM list -- so it now records what the narrow reading really
does, along with the WITH ... SELECT shape it cannot see. Both under-report, and
neither exists in relay today.

cloud/apps/relay/src/cell-row-lock-scope.test.ts

@@ -294,8 +294,11 @@ describe.each(backends)('cell row lock scope ($name)', ({ name, open }) => {
       await transaction.query(RESERVE, [1, 0, CELL_A])
     })
 
+    // checked is 1, not 0: standing down is now scoped to the statement it could
+    // not read, so the write after it was still judged. That is the point --
+    // muting a whole transaction over one unreadable projection was the hazard.
     expect(consumeRelayCellRowLockScope(database)).toMatchObject({
-      cellRowLockScopesChecked: 0,
+      cellRowLockScopesChecked: 1,
       cellRowLockScopesStoodDown: 1
     })
   })
@@ -413,10 +416,11 @@ describe.each(backends)('cell row lock scope ($name)', ({ name, open }) => {
     )
   })
 
-  // Why: after standing down the scope has an incomplete picture, so judging
-  // anything further would be guessing. Without this the suppression can be
-  // deleted and no test notices, because the stand-down cases hold nothing yet.
-  it('judges nothing further once it has stood down', async () => {
+  // Why: a statement the scope cannot read must not mute the ones after it. The
+  // read below contributes nothing to `held`, and a missing `held` entry can only
+  // ever remove a report -- so continuing to police is strictly safer than
+  // latching off, and the descending write after it is still caught.
+  it('keeps policing after a statement it could not read', async () => {
     await expect(
       database.transaction(async (transaction) => {
         await transaction.queryLocked(LOCK_ONE, [CELL_C])
@@ -426,13 +430,14 @@ describe.each(backends)('cell row lock scope ($name)', ({ name, open }) => {
         )
         await transaction.query(RESERVE, [1, 0, CELL_A])
       })
-    ).resolves.toBeUndefined()
+    ).rejects.toThrow('out-of-order')
   })
 
   // Why: production is the configuration that actually ships. "Tests throw,
   // production warns" was half-tested -- nothing exercised the warn, nor the
   // once-per-signature dedupe that bounds it.
   it('warns instead of throwing when it is not running under test', async () => {
+    consumeRelayCellRowLockScope(database)
     const previous = process.env.NODE_ENV
     process.env.NODE_ENV = 'production'
     const events: string[] = []

cloud/apps/relay/src/cell-row-lock-scope.ts

@@ -24,16 +24,24 @@ type CellRowLockViolation = {
 export type CellRowLockScopeOutcome = {
   checked: boolean
   stoodDown: boolean
-  violations: number
+  violated: boolean
 }
 
 export function emptyCellRowLockScopeOutcome(): CellRowLockScopeOutcome {
-  return { checked: false, stoodDown: false, violations: 0 }
+  return { checked: false, stoodDown: false, violated: false }
 }
 
-// Why max and not sum: a retry replays the same statements, so a transaction that
-// violated, was retried and violated again is one logical violation seen twice.
-// Summing would make the count rise with contention rather than with breakage.
+// Merged across a transaction's attempts, so the counters describe transactions
+// rather than attempts: a retry replays the same statements, and counting each
+// one made a single logical transaction report up to three times -- inflation
+// worst under exactly the contention these numbers exist to measure.
+//
+// `violated` is any-of, not a sum and not a max. Summing rises with contention
+// rather than with breakage; a max cannot inflate but can still ZERO OUT an
+// observed violation, because an attempt that violates and then hits a retryable
+// 55P03 can be followed by a clean attempt that takes a different branch. The
+// gate here is zero-versus-nonzero, so what has to survive is "this transaction
+// violated at least once". Magnitude lives in the distinct-signature log.
 export function mergeCellRowLockScopeOutcomes(
   left: CellRowLockScopeOutcome,
   right: CellRowLockScopeOutcome
@@ -41,14 +49,15 @@ export function mergeCellRowLockScopeOutcomes(
   return {
     checked: left.checked || right.checked,
     stoodDown: left.stoodDown || right.stoodDown,
-    violations: Math.max(left.violations, right.violations)
+    violated: left.violated || right.violated
   }
 }
 
 // Why: `violations === 0` is only evidence if something was examined. Checked
-// counts transactions where at least one relay_cells write was evaluated, which
-// is the population a per-cell conversion moves; stoodDown counts the ones this
-// guard refused to reason about, and must read as a bug, not as a clean run.
+// counts transactions where at least one cell row lock was evaluated, which is
+// the population a per-cell conversion moves; stoodDown counts the ones holding a
+// statement this guard could not read, and must be a bug report, not a clean run.
+// Violations counts violating TRANSACTIONS, not violating statements.
 export type CellRowLockScopeCounts = {
   cellRowLockScopesChecked: number
   cellRowLockScopesStoodDown: number
@@ -69,7 +78,7 @@ export class CellRowLockScopeSamples {
   record(outcome: CellRowLockScopeOutcome): void {
     if (outcome.checked) this.counts.cellRowLockScopesChecked += 1
     if (outcome.stoodDown) this.counts.cellRowLockScopesStoodDown += 1
-    this.counts.cellRowLockScopeViolations += outcome.violations
+    if (outcome.violated) this.counts.cellRowLockScopeViolations += 1
   }
 
   consumeCounts(): CellRowLockScopeCounts {
@@ -84,13 +93,19 @@ export class CellRowLockScopeSamples {
 const CELL_TABLE = 'relay_cells'
 const CELL_INSERT = /^\s*INSERT\s+INTO\s+relay_cells\s*\(([^)]*)\)\s*VALUES\s*\(/i
 const CELL_ROW_WRITE = /^\s*(?:UPDATE|DELETE\s+FROM)\s+relay_cells\b/i
-// The statement's own relation, not any mention of the table. A locked read of
-// another table that names relay_cells in a JOIN or an EXISTS locks no cell row,
-// and every one of those tables carries a cell_id column that would otherwise be
-// taken for a held row.
+// The statement's own relation, not any mention of the table: relay_assignments,
+// relay_cell_runtime and relay_migrations all carry a cell_id that would
+// otherwise be recorded as a held cell row. The narrow reading is deliberate and
+// it under-reports in two known ways, both of which only ever cost a report and
+// never invent one: a locked read of another table that JOINs relay_cells does
+// lock cell rows, because Postgres FOR UPDATE without OF locks every table in the
+// FROM list; and a WITH ... SELECT does not match the SELECT anchor at all.
+// Neither shape exists in relay today.
 const CELL_TABLE_READ = /^\s*SELECT\b[\s\S]*?\bFROM\s+([A-Za-z_]\w*)/i
 const CELL_ID_EQUALS = /\bcell_id\s*=\s*\?/gi
-const ORDERED_LOCK = /\bORDER\s+BY\s+cell_id\s+ASC\b/i
+// Alias-tolerant: `ORDER BY cell.cell_id ASC` orders identically, and rejecting
+// it would throw in tests on a statement that is correct.
+const ORDERED_LOCK = /\bORDER\s+BY\s+(?:\w+\.)?cell_id\s+ASC\b/i
 // An upsert is not the free set-extension a plain insert is: when the row it
 // names already exists it takes that row's lock and BLOCKS, so it is a waiting
 // acquisition and belongs under the order check. Proven against PostgreSQL 16 --
@@ -102,12 +117,15 @@ const CELL_UPSERT = /\bON\s+CONFLICT\b[\s\S]*\bDO\s+UPDATE\b/i
 
 export class CellRowLockScope {
   private readonly held = new Set<string>()
-  // A locked read whose rows do not name their cell. Nothing produces one today;
-  // policing would then mean guessing, so the scope stands down instead -- but
-  // never silently, or its silence would read the same as a clean transaction.
-  private opaque = false
+  // A statement the scope could not read -- a locked read whose rows do not name
+  // their cell, or one that threw while being parsed. Scoped to the statement,
+  // not the transaction: skipping it and carrying on is strictly safer than
+  // muting everything after it, because the only effect of a missing `held` entry
+  // is to REMOVE reports, never to invent one. A transaction-wide latch turned a
+  // single unreadable projection into a silent transaction.
+  private stoodDown = false
   private checked = false
-  private violations = 0
+  private violated = false
   // Set by a lock that took the whole inventory in order. While it holds, every
   // row that existed is held, so an upsert naming an unheld row is creating one.
   private coveredInventory = false
@@ -121,8 +139,7 @@ export class CellRowLockScope {
       this.inspect(sql, params, lock, rows)
     } catch (error) {
       if (error instanceof CellRowLockScopeViolationError) throw error
-      this.opaque = true
-      reportStandDown(`unreadable:${fingerprint(sql)}`)
+      this.noteStandDown(`unreadable:${fingerprint(sql)}`)
     }
   }
 
@@ -167,7 +184,7 @@ export class CellRowLockScope {
   // never reached or quietly stood down, and the deploy plan is to trust its
   // silence. These give the silence a denominator.
   outcome(): CellRowLockScopeOutcome {
-    return { checked: this.checked, stoodDown: this.opaque, violations: this.violations }
+    return { checked: this.checked, stoodDown: this.stoodDown, violated: this.violated }
   }
 
   // Why reads count too: `checked` is the denominator for "the guard saw the
@@ -176,11 +193,17 @@ export class CellRowLockScope {
   // them when measured across this suite -- including the fleet-wide
   // `SELECT ... FOR UPDATE` the conversion exists to replace, which scored zero.
   private note(): void {
-    if (!this.opaque) this.checked = true
+    this.checked = true
+  }
+
+  // Statement-scoped, and always visible: silence that reads the same as a clean
+  // transaction is the failure this guard exists to avoid.
+  private noteStandDown(statement: string): void {
+    this.stoodDown = true
+    reportStandDown(statement)
   }
 
   private acquireRead(sql: string, lock: CellRowLockKind, rows: SqlRow[]): void {
-    if (this.opaque) return
     const statement = fingerprint(sql)
     // Shape, not row count: a lock that names no single cell may return one row
     // in a fixture and many in production, so judging it by what came back makes
@@ -193,8 +216,9 @@ export class CellRowLockScope {
     for (const row of rows) {
       const cellId = row.cell_id
       if (typeof cellId !== 'string') {
-        this.opaque = true
-        reportStandDown(statement)
+        // Skip this read only. Its rows never enter `held`, which can only cost
+        // a later report, never manufacture one.
+        this.noteStandDown(statement)
         return
       }
       cellIds.push(cellId)
@@ -210,7 +234,7 @@ export class CellRowLockScope {
   // Counted before it is thrown: in production report() only warns, so the count
   // is what carries the magnitude the once-per-signature log deliberately drops.
   private report(violation: CellRowLockViolation): void {
-    this.violations += 1
+    this.violated = true
     report(violation)
   }
 
@@ -219,7 +243,6 @@ export class CellRowLockScope {
     statement: string,
     lock: CellRowLockKind
   ): void {
-    if (this.opaque) return
     if (cellIds === undefined) {
       this.report({ reason: 'unparsed-write', cellIds: [], held: [...this.held], statement })
       return