diff --git a/cloud/apps/relay/src/cell-inventory-lock-census.test.ts b/cloud/apps/relay/src/cell-inventory-lock-census.test.ts index 929173eb9da..6a02b6ed43a 100644 --- a/cloud/apps/relay/src/cell-inventory-lock-census.test.ts +++ b/cloud/apps/relay/src/cell-inventory-lock-census.test.ts @@ -1,4 +1,6 @@ -import { readFileSync } from 'node:fs' +import { readdirSync, readFileSync } from 'node:fs' +import { join, relative } from 'node:path' +import { fileURLToPath } from 'node:url' import { describe, expect, it } from 'vitest' import { cellInventoryLockOptions, type CellInventoryLockMode } from './assignment-store.js' @@ -55,28 +57,44 @@ const CENSUS: CensusEntry[] = [ // second selects from the inventory its single caller has already locked. ] -// Every inline `FROM relay_cells ... FOR UPDATE` outside the named lock helpers, -// in source order: whole-table locks in reconciliation and sticky placement, -// and single-row locks for a cell the method is already scoped to (heartbeat, -// fence, drain generation, configuration, or a reservation adjust that runs -// under a lock its caller already holds). A new inline lock fails the census +// Every statement outside the named lock helpers that takes a relay_cells row +// lock, as `file:method` in file-then-source order: whole-table locks in +// reconciliation, sticky placement and the admission boundary, and per-cell +// locks or writes for a cell the method is already scoped to (heartbeat, fence, +// drain generation, configuration, cell registration, or a reservation adjust +// that runs under a lock its caller already holds). A new one fails the census // below until it is listed here; per-connection paths that touch more than one // cell go through lockCellRows so the order is fixed. const NAMED_LOCK_HELPERS = ['lockCellInventory', 'lockGeneralCellInventory', 'lockCellRows'] -const INLINE_CELL_LOCK_SITES = [ - 'reconcileCellsWithOptions', - 'assignStickyOnce', - 'recordCellHeartbeat', - 'attestCellFence', - 'adoptLegacyCellFence', - 'commitLegacyCellFenceAdoption', - 'prepareCellFenceAttempt', - 'attestCellFenceAttempt', - 'attestCellFenceAttempt', - 'configureCell', - 'assertDrainCellGeneration', - 'adjustCellReservation' +const CELL_ROW_LOCK_SITES = [ + 'assignment-store.ts:reconcileCellsWithOptions', + 'assignment-store.ts:reconcileCellsWithOptions', + 'assignment-store.ts:assignStickyOnce', + 'assignment-store.ts:recordCellHeartbeat', + 'assignment-store.ts:recordCellHeartbeat', + 'assignment-store.ts:attestCellFence', + 'assignment-store.ts:adoptLegacyCellFence', + 'assignment-store.ts:commitLegacyCellFenceAdoption', + 'assignment-store.ts:prepareCellFenceAttempt', + 'assignment-store.ts:attestCellFenceAttempt', + 'assignment-store.ts:attestCellFenceAttempt', + 'assignment-store.ts:configureCell', + 'assignment-store.ts:configureCell', + 'assignment-store.ts:reconcileReservationAccounting', + 'assignment-store.ts:assertDrainCellGeneration', + 'assignment-store.ts:removeSupersededSameCellControls', + 'assignment-store.ts:adjustCellReservation', + 'assignment-store.ts:adjustCellReservation', + 'assignment-store.ts:adjustCellReservationAtomically', + 'cell-admission-migration-registration.ts:add', + 'cell-admission-migration-registration.ts:insertCell', + 'cell-admission-selector.ts:setCellAdmissionBeforeBoundary', + 'cell-admission-selector.ts:setCellAdmissionBeforeBoundary', + 'cell-admission-selector.ts:apply', + 'cell-admission-selector.ts:apply', + 'cell-admission-selector.ts:inspect', + 'cell-admission-selector.ts:persistIntent' ] // The background sweeps, and nothing else. A method reachable from one of these @@ -92,9 +110,23 @@ const REQUEST_ENTRY_FILES = [ ] const DECLARATION = /^ {2}(?:private |public )?(?:static )?(?:async )?([A-Za-z_][\w]*)[(<]/ +// The selector and the registrar are modules of free functions, not store methods. +const FUNCTION_DECLARATION = /^(?:export )?(?:async )?function ([A-Za-z_][\w]*)[(<]/ +const SOURCE_DIRECTORY = fileURLToPath(new URL('.', import.meta.url)) + +// The lock census is store-scoped on purpose: lockCellInventory is private, so +// only this class can reach it, and the call graph below is same-class. function storeSource(): string[] { - return readFileSync(new URL('./assignment-store.ts', import.meta.url), 'utf8').split('\n') + return readFileSync(join(SOURCE_DIRECTORY, 'assignment-store.ts'), 'utf8').split('\n') +} + +function relaySourceFiles(): string[] { + return readdirSync(SOURCE_DIRECTORY, { recursive: true, withFileTypes: true }) + .filter((entry) => entry.isFile() && entry.name.endsWith('.ts')) + .filter((entry) => !entry.name.endsWith('.test.ts')) + .map((entry) => relative(SOURCE_DIRECTORY, join(entry.parentPath, entry.name))) + .sort() } function entryPoints(files: string[]): string[] { @@ -158,6 +190,42 @@ function derivedReachability(lines: string[]): (method: string) => Reachability : 'orphan' } +// Whole statements, not a fixed window: a wide column list or a raw FOR UPDATE +// inside query() must not slip past. +const TICK = String.fromCharCode(96) +const STATEMENT_CALL = '\\.(queryLocked|query)\\(\\s*' + TICK + '([^' + TICK + ']*)' + TICK +const CELL_TABLE_READ = /\bFROM\s+relay_cells\b/ +// A bare write takes the same row lock with no SELECT in front of it, so a +// FROM-only detector is blind to exactly the statements a per-cell conversion +// has to move. +const CELL_ROW_WRITE = /^\s*(?:UPDATE|INSERT INTO|DELETE FROM)\s+relay_cells\b/ + +function cellRowLockSites(file: string, source: string): string[] { + const lines = source.split('\n') + const bounds: { name: string; start: number }[] = [] + lines.forEach((line, index) => { + const declaration = DECLARATION.exec(line) ?? FUNCTION_DECLARATION.exec(line) + if (declaration) bounds.push({ name: declaration[1]!, start: index }) + }) + const methodAt = (offset: number): string => { + const lineIndex = source.slice(0, offset).split('\n').length - 1 + let name = '' + for (const bound of bounds) if (bound.start <= lineIndex) name = bound.name + return name + } + const sites: string[] = [] + for (const call of source.matchAll(new RegExp(STATEMENT_CALL, 'g'))) { + const statement = call[2]! + const writes = CELL_ROW_WRITE.test(statement) + if (!writes && !CELL_TABLE_READ.test(statement)) continue + if (!writes && call[1] !== 'queryLocked' && !/\bFOR\s+UPDATE\b/.test(statement)) continue + const method = methodAt(call.index) + if (NAMED_LOCK_HELPERS.includes(method)) continue + sites.push(`${file}:${method}`) + } + return sites +} + function readCallSites(): { method: string; mode: CensusMode }[] { const sites: { method: string; mode: CensusMode }[] = [] let method = '' @@ -180,37 +248,11 @@ describe('cell inventory lock call-site census', () => { // Why: the census only sees lockCellInventory calls, so a hand-written // `relay_cells ... FOR UPDATE` would escape classification entirely. it('routes every relay_cells row lock through a named lock helper', () => { - const lines = storeSource() - const rawSites: string[] = [] - // Whole statements, not a fixed window: a wide column list or a raw - // FOR UPDATE inside query() must not slip past. - const source = lines.join('\n') - const bounds: { name: string; start: number }[] = [] - lines.forEach((line, index) => { - const declaration = DECLARATION.exec(line) - if (declaration) bounds.push({ name: declaration[1]!, start: index }) - }) - const methodAt = (offset: number): string => { - const lineIndex = source.slice(0, offset).split('\n').length - 1 - let name = '' - for (const bound of bounds) if (bound.start <= lineIndex) name = bound.name - return name - } - const tick = String.fromCharCode(96) - const statementCall = new RegExp( - '\\.(queryLocked|query)\\(\\s*' + tick + '([^' + tick + ']*)' + tick, - 'g' + const sites = relaySourceFiles().flatMap((file) => + cellRowLockSites(file, readFileSync(join(SOURCE_DIRECTORY, file), 'utf8')) ) - for (const call of source.matchAll(statementCall)) { - const statement = call[2]! - if (!/\bFROM\s+relay_cells\b/.test(statement)) continue - const locks = call[1] === 'queryLocked' || /\bFOR\s+UPDATE\b/.test(statement) - if (!locks) continue - const method = methodAt(call.index) - if (NAMED_LOCK_HELPERS.includes(method)) continue - rawSites.push(method) - } - expect(rawSites).toEqual(INLINE_CELL_LOCK_SITES) + + expect(sites).toEqual(CELL_ROW_LOCK_SITES) }) it('leaves no call site taking the inventory without naming a mode', () => {