From d9cb7d3b23c39dc73c1c847024b1b45d507decc9 Mon Sep 17 00:00:00 2001 From: Hussein Mohamed Date: Sat, 5 Sep 2026 12:28:30 +0300 Subject: [PATCH 1/3] test(api): make the durable analysis cache own its database setup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The suite required `engine_analysis_cache` and never built it. On a genuinely fresh database it failed 6 of 10 tests — three throwing SQLSTATE 42P01 from its own statements, three asserting on a durable row that was never written, because the cache absorbs a missing table as a fault and degrades to computing. It passed only because the persistence package migrates the shared database and runs first, in both the root test script and the postgres-integration CI job. On a migrated database it passed and left five rows behind every run: minting a fresh FEN per test is collision-avoidance, not cleanup. It now applies the canonical migrations itself, once per file, from a path anchored on its own location rather than the working directory, and every test runs inside the `withSharedDatabase` contract with cleanup scoped to the exact FENs it minted — recorded before the statement that creates the row, so a body that throws after a commit still surrenders it. A new ownership regression runs the compiled suite as a child process against a disposable, unmigrated database and reads the result from outside: it passes with nothing prepared for it, any one of its tests can be the only one that runs, and it hands a migrated table back exactly as it found it, stranger's row included. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01CUPqvu66J5ZVyiv4nJ797r --- ...ache-durable-ownership.integration.test.ts | 201 ++++++ ...analysis-cache-durable.integration.test.ts | 601 +++++++++++------- 2 files changed, 577 insertions(+), 225 deletions(-) create mode 100644 packages/api/test/analysis-cache-durable-ownership.integration.test.ts diff --git a/packages/api/test/analysis-cache-durable-ownership.integration.test.ts b/packages/api/test/analysis-cache-durable-ownership.integration.test.ts new file mode 100644 index 00000000..be27845c --- /dev/null +++ b/packages/api/test/analysis-cache-durable-ownership.integration.test.ts @@ -0,0 +1,201 @@ +/** + * What `analysis-cache-durable.integration.test.ts` owes the database it runs against. + * + * That suite drives the production cache composition against a real PostgreSQL server, and it made + * two assumptions about the database that nothing enforced: that `engine_analysis_cache` already + * existed, and that the rows it wrote were somebody else's problem. Both held only by accident of + * ordering — the persistence package's suites migrate `DATABASE_URL` and run first in both the root + * `test` script and the `postgres-integration` CI job — and neither is visible from inside the + * suite itself, which is why this file sits beside it rather than in it. + * + * Measured against the code this replaced: on a genuinely fresh database the suite failed 6 of its + * 10 tests, three of them by throwing SQLSTATE 42P01 from its own statements and three by asserting + * on a durable row that was never written — because the cache absorbs a missing table as a fault + * and degrades to computing, so a suite about durability went on running with no durability at all. + * On a migrated database it passed, and left 5 rows behind every time it did. + * + * Both readings have to be taken from outside the suite. Its identities are minted per run, so no + * residue it leaves ever collides with anything it later asks for, and every assertion it makes + * passes on the hundredth run exactly as on the first. + */ +import assert from 'node:assert/strict'; +import { test } from 'node:test'; +import { execFile } from 'node:child_process'; +import { join } from 'node:path'; +import { promisify } from 'node:util'; +import { migrate } from '@chess-platform/persistence/pg'; +import { withTestDatabase } from '@chess-platform/persistence/test-support'; +import type { Pool } from 'pg'; + +const DATABASE_URL = process.env['DATABASE_URL']; +const skip = DATABASE_URL ? false : 'DATABASE_URL not set'; + +const SUITE = join(__dirname, 'analysis-cache-durable.integration.test.js'); +const MIGRATIONS = join(__dirname, '../../../persistence/migrations'); +/** Deliberately not `packages/api` — see `runSuiteAgainst`. */ +const REPO_ROOT = join(__dirname, '..', '..', '..', '..'); + +const run = promisify(execFile); + +/** + * The environment the suite under test runs in, as a fresh test-runner process. + * + * `NODE_TEST_CONTEXT` has to go. Node sets it inside a test process, and a child that inherits it + * believes it is a nested runner: it declines to execute the file at all, warns that `run()` was + * called recursively, and exits 0 having produced nothing. Inheriting it would make both tests here + * pass for the worst possible reason — a suite that never ran neither fails nor leaves residue. + * + * The reporter is pinned for the same class of reason. Node chooses `spec` or `tap` by whether + * stdout is a TTY, and only `tap` prints the counts read below, so leaving the choice to the + * environment would make these checks depend on how the outer suite happened to be invoked. + */ +function childEnv(connectionString: string): NodeJS.ProcessEnv { + const env: NodeJS.ProcessEnv = { ...process.env, DATABASE_URL: connectionString }; + delete env['NODE_TEST_CONTEXT']; + return env; +} + +/** + * One `# key N` line of a TAP summary, or `undefined` when the run produced no such line. + * + * `\r?` because this runs on Windows too, where a carriage return sits between the digits and the + * line end that `$` matches — without it every count reads as absent and every assertion below + * fails for a reason that has nothing to do with the database. + */ +function tally(stdout: string, key: string): number | undefined { + const match = new RegExp(`^# ${key} (\\d+)\\r?$`, 'm').exec(stdout); + return match ? Number(match[1]) : undefined; +} + +/** + * Run the suite under test against `connectionString`, and insist it really ran. + * + * `# fail 0` alone is not evidence: a run that executed nothing prints it, and so does a run whose + * every test self-skipped because `DATABASE_URL` never reached the child. The other counts are what + * separate "passed" from "did not happen" — every test that ran passed, none were skipped, and at + * least one ran. Not `# pass 10`: the count is a property of the other file, and pinning it here + * would make adding a test there a failure in this one, which is not a fact about the database. + * + * The child runs from the repository root, not from `packages/api`. That is the point: the suite + * has to find the migrations it applies from its own location, so a working directory it does not + * control cannot decide whether its schema gets built. + */ +async function runSuiteAgainst(connectionString: string, ...extraArgs: string[]): Promise { + const child = await run( + process.execPath, + ['--test', '--test-concurrency=1', '--test-reporter=tap', ...extraArgs, SUITE], + { cwd: REPO_ROOT, env: childEnv(connectionString) }, + ).catch((error: unknown) => { + // The child's own report, not just "command failed": a non-zero exit is where the interesting + // detail lives, and `execFile` puts it on the rejection rather than in the message. + const output = typeof (error as { stdout?: unknown }).stdout === 'string' ? (error as { stdout: string }).stdout : ''; + const detail = error instanceof Error ? error.message : String(error); + assert.fail(`the suite under test must pass against this database:\n${detail}\n${output}`); + }); + + assert.equal(tally(child.stdout, 'fail'), 0, 'the suite under test reported a failure'); + assert.equal(tally(child.stdout, 'skipped'), 0, 'a skipped suite proves nothing about the database'); + assert.ok((tally(child.stdout, 'pass') ?? 0) > 0, 'the suite under test ran no tests at all'); + assert.equal( + tally(child.stdout, 'pass'), + tally(child.stdout, 'tests'), + 'every test the child started must also have finished', + ); +} + +/** + * Every row of `engine_analysis_cache` by identity, ordered in SQL so two readings compare directly. + * + * Identity rather than count: a count would let a cleanup that removed one row and left another + * compare equal, and could not show *which* row a too-broad delete took. + */ +async function readCacheRows(pool: Pool): Promise { + const rows = await pool.query<{ value: string }>( + `SELECT fingerprint || ':' || variant || ':' || multi_pv::text || ':' || fen AS value + FROM engine_analysis_cache ORDER BY fingerprint, variant, multi_pv, fen`, + ); + return rows.rows.map((row) => row.value); +} + +/** + * The suite has to work on a database no other test has prepared for it. + * + * `withTestDatabase` hands back a database that has been created and nothing else — no migrations, + * no rows — which is exactly the condition the defect needed: the fresh server a developer starts + * for one file, and the state a CI job would be in if it ever ran this suite without the + * persistence package ahead of it. Nothing here tells the child what schema to build; that the + * child arrives at a working one is the whole assertion. + */ +test('the durable cache suite passes against a database nothing else has prepared', { skip }, async () => { + await withTestDatabase( + async ({ pool, connectionString }) => { + await runSuiteAgainst(connectionString); + + // Nothing but the child ever touched this database, so anything in the table is its residue — + // and the table exists to be read only because the child built it, which is the other half of + // the claim. The check below is worth making here as well as on a migrated database: cleanup + // and schema establishment are different code, and the fresh path is the one nothing else + // covers. + assert.deepEqual(await readCacheRows(pool), [], 'the suite must leave a fresh database empty'); + }, + { connectionString: DATABASE_URL, max: 2 }, + ); +}); + +/** + * And any one of its tests can be the only one that runs. + * + * Establishing the schema in the first test would satisfy the check above while leaving every other + * test dependent on that one having gone first — the same defect at a smaller scale, and the shape + * a developer meets first, because `--test-name-pattern` is how you rerun the one test that failed. + * The last test in the file is the subject here: it is the furthest from any setup the first test + * might have done, and it both reads and writes the table. + */ +test('any single test of the durable cache suite can run alone on a fresh database', { skip }, async () => { + await withTestDatabase( + async ({ connectionString }) => { + await runSuiteAgainst( + connectionString, + '--test-name-pattern', + 'the retention sweep runs against the composed cache', + ); + }, + { connectionString: DATABASE_URL, max: 2 }, + ); +}); + +/** + * And it hands the table back exactly as it found it. + * + * The stranger row is the half a residue check on its own would miss. "The suite removed its own + * rows" and "the suite emptied the table" are the same reading unless something it does not own is + * sitting there to tell them apart — and an unqualified `DELETE FROM engine_analysis_cache` would + * satisfy every assertion the suite makes about itself while destroying a concurrent suite's + * fixture. + */ +test('the durable cache suite leaves engine_analysis_cache exactly as it found it', { skip }, async () => { + await withTestDatabase( + async ({ pool, connectionString }) => { + await migrate(pool, MIGRATIONS); + // `achieved_depth` is not decoration: 0026 refuses a row whose three achieved_* columns are + // all NULL, because such a row could never answer any request. + await pool.query( + `INSERT INTO engine_analysis_cache + (fingerprint, variant, multi_pv, fen, achieved_depth, payload_version, results, + created_at, updated_at) + VALUES ('stranger-fingerprint', 'standard', 1, $1, 10, 1, '[]'::jsonb, now(), now())`, + ['rnbqkbnr/pppppppp/8/8/8/8/PPPPPPPP/RNBQKBNR w KQkq - 0 1'], + ); + + const before = await readCacheRows(pool); + await runSuiteAgainst(connectionString); + + assert.deepEqual( + await readCacheRows(pool), + before, + 'the suite must remove every row it wrote, and only those', + ); + }, + { connectionString: DATABASE_URL, max: 2 }, + ); +}); diff --git a/packages/api/test/analysis-cache-durable.integration.test.ts b/packages/api/test/analysis-cache-durable.integration.test.ts index bee9d194..3c278c06 100644 --- a/packages/api/test/analysis-cache-durable.integration.test.ts +++ b/packages/api/test/analysis-cache-durable.integration.test.ts @@ -15,16 +15,19 @@ * this job cannot install; `FakeEngineTransport` is the engine package's own, and counting its `go` * commands is what makes "the engine did not run again" an assertion rather than a hope. */ -import test from 'node:test'; +import test, { after } from 'node:test'; import assert from 'node:assert/strict'; import { randomInt } from 'node:crypto'; +import { join } from 'node:path'; +import type { Pool } from 'pg'; import { EngineManager, FakeEngineTransport, stockfishPlugin, type EngineResult, } from '@chess-platform/engine'; -import { createPool, PgAnalysisCache } from '@chess-platform/persistence/pg'; +import { migrate, PgAnalysisCache } from '@chess-platform/persistence/pg'; +import { withSharedDatabase } from '@chess-platform/persistence/test-support/fixtures'; import { createAnalysisCacheComposition } from '../src/analysis/durable-cache'; import type { AnalysisCacheComposition } from '../src/analysis/composition'; import { JsonLogger } from '../src/ports/logger'; @@ -43,15 +46,129 @@ const STOCKFISH_OPTIONS = [ const INFO = 'info depth 10 seldepth 12 nodes 12345 nps 50000 time 200 score cp 20 multipv 1 pv e2e4'; /** - * A position unique to this run. + * The canonical ledger, from the package that owns it — never a schema this file invents. + * + * Anchored on this file's own location rather than `process.cwd()`, which several suites here still + * use: the working directory is the package root under `npm test` and the repository root under an + * IDE runner or a hand-written `node --test packages/api/...`, and a suite that can only find its + * schema from one of those has swapped one hidden precondition for another. + */ +const MIGRATIONS = join(__dirname, '../../../persistence/migrations'); + +/** + * Every FEN minted so far and not yet cleaned up, which is this file's claim on the table. + * + * A FEN recorded here was generated by this process and appears in no other suite, which is what + * makes deleting by exact equality an ownership claim: matching the shared `rnbqkbnr/...` placement + * as a prefix would sweep up every standard starting position in the database, most of which belong + * to somebody else. The fullmove counters below start at a million to keep that true — the + * persistence package's own cache suite keys its rows on the canonical `... 0 1`, and this file + * deletes by FEN without regard to fingerprint, so a counter of 1 minted here would take that row + * with it. + * + * Draining rather than reading assumes these tests run one at a time, which is what + * `--test-concurrency=1` and node's sequential top-level tests give: a test that opted itself into + * concurrency would have its rows deleted by a neighbour's cleanup. The sibling suite for this same + * table makes the same assumption, at `packages/persistence/test/analysis-cache.integration.test.ts`. + */ +const mintedFens: string[] = []; + +/** The same FENs, kept for the whole file rather than drained, so `after` can audit the cleanup. */ +const allFens: string[] = []; + +/** + * A position unique to this run, recorded before it can be used. * * The cache identity includes the FEN, and these suites share a database with every other * integration file and with previous runs of themselves. Varying the fullmove counter keeps the FEN * structurally valid while making each test's identity its own, so a leftover row can neither * satisfy a request this test meant to miss nor be mistaken for one it wrote. + * + * Uniqueness was mistaken for cleanup, and it is not: minting a fresh identity means the *next* run + * never collides, not that this one took its rows back. It left five rows in `engine_analysis_cache` + * every time it ran. Recording happens here, before the caller can analyse anything, so a body that + * throws between minting the FEN and writing the row still hands cleanup an identity it owns. */ function freshFen(): string { - return `rnbqkbnr/pppppppp/8/8/8/8/PPPPPPPP/RNBQKBNR w KQkq - 0 ${randomInt(1, 2_000_000)}`; + const fen = `rnbqkbnr/pppppppp/8/8/8/8/PPPPPPPP/RNBQKBNR w KQkq - 0 ${randomInt(1_000_000, 2_000_000)}`; + mintedFens.push(fen); + allFens.push(fen); + return fen; +} + +/** + * Say out loud that the file left nothing behind. + * + * Cleanup that quietly stopped deleting would go on passing indefinitely otherwise: every identity + * is minted fresh, so residue never collides with anything a later run asks for. The ownership + * regression beside this file makes the same reading from outside, and more strictly — it can also + * see rows *wrongly* removed. This one is here because it is the reading a developer running only + * this file still gets. + */ +after(async () => { + if (!DATABASE_URL || allFens.length === 0) return; + await withSharedDatabase({ max: 2, cleanup: async () => undefined }, async (pool) => { + const left = await pool.query<{ n: string }>( + 'SELECT count(*)::text AS n FROM engine_analysis_cache WHERE fen = ANY($1::text[])', + [allFens], + ); + assert.equal(left.rows[0]?.n, '0', 'every row this file wrote was removed again'); + }); +}); + +/** + * Apply the ledger the first time only; later calls on later pools are no-ops. + * + * The suite needs `engine_analysis_cache` (0026) and its `updated_at` index (0027), and used to + * assume some earlier package had built them: the persistence suites migrate `DATABASE_URL` and run + * before this one in both the root `test` script and the `postgres-integration` CI job. Run against + * a database nobody had prepared, six of these ten tests failed — three throwing SQLSTATE 42P01 and + * three quietly asserting on a durable row that was never written, because the cache absorbs a + * missing table as a fault and degrades to computing. + * + * A flag rather than a memoized promise: a promise would stay bound to the first test's pool, which + * `withDatabase` closes on the way out, so a later caller awaiting it for its own connection would + * be waiting on a pool that no longer exists. + */ +let migrated = false; + +async function ensureMigrated(pool: Pool): Promise { + if (migrated) return; + await migrate(pool, MIGRATIONS); + migrated = true; +} + +/** + * Run one test against the shared database, then take back the rows it wrote. + * + * `withSharedDatabase` rather than a `try/finally` around the `DELETE`: a `finally` that awaits + * cleanup lets a failing `DELETE` replace the assertion that actually failed, and cleanup here runs + * on the path where it matters most — a failing test is the one someone reruns. It says nothing + * about the `finally` blocks inside the bodies below, which shut engines down: a `shutdown()` that + * rejects while an assertion is already failing still replaces it, as it did before this change. + */ +async function withDatabase(body: (pool: Pool) => Promise): Promise { + // Two connections for *this* pool — enough for `migrate`, which holds its advisory lock on a + // dedicated client while running its statements on another. It is not a budget for the file: each + // `instance()` builds the production cache pool, which is four more. + await withSharedDatabase({ max: 2, cleanup: deleteMintedRows }, async (pool) => { + await ensureMigrated(pool); + await body(pool); + }); +} + +/** + * Remove every row keyed by a FEN minted so far, then forget them. + * + * The list is drained rather than read, so a test that failed partway hands the next one an empty + * ledger instead of re-deleting rows that are already gone. Deleting by FEN alone rather than by the + * whole composite key is deliberate: one FEN can carry rows under several engine fingerprints — the + * upgrade case is a whole test here — and this file minted every one of them. + */ +async function deleteMintedRows(pool: Pool): Promise { + const fens = mintedFens.splice(0, mintedFens.length); + if (fens.length === 0) return; + await pool.query('DELETE FROM engine_analysis_cache WHERE fen = ANY($1::text[])', [fens]); } interface Instance { @@ -126,22 +243,24 @@ function counter(metrics: InMemoryMetrics, series: string): number { } test('a cold position is computed once and then served from the database', { skip }, async () => { - const node = instance(); - const fen = freshFen(); - try { - const first = await analyze(node, fen); - assert.equal(node.searches(), 1, 'a cold identity must reach the engine'); - - const second = await analyze(node, fen); - assert.deepEqual(second, first, 'the cached analysis is the analysis, not an approximation'); - assert.equal(node.searches(), 1, 'the second request must not run a search'); - - assert.equal(counter(node.metrics, 'analysis_cache_events_total{event="cache_hit"}'), 1); - assert.equal(counter(node.metrics, 'analysis_cache_events_total{event="cache_miss"}'), 1); - assert.equal(counter(node.metrics, 'analysis_cache_faults_total{fault="read"}'), 0); - } finally { - await node.shutdown(); - } + await withDatabase(async () => { + const node = instance(); + const fen = freshFen(); + try { + const first = await analyze(node, fen); + assert.equal(node.searches(), 1, 'a cold identity must reach the engine'); + + const second = await analyze(node, fen); + assert.deepEqual(second, first, 'the cached analysis is the analysis, not an approximation'); + assert.equal(node.searches(), 1, 'the second request must not run a search'); + + assert.equal(counter(node.metrics, 'analysis_cache_events_total{event="cache_hit"}'), 1); + assert.equal(counter(node.metrics, 'analysis_cache_events_total{event="cache_miss"}'), 1); + assert.equal(counter(node.metrics, 'analysis_cache_faults_total{fault="read"}'), 0); + } finally { + await node.shutdown(); + } + }); }); /** @@ -152,24 +271,26 @@ test('a cold position is computed once and then served from the database', { ski * has its own pool, its own manager and its own workers, and shares nothing with A but the table. */ test('a second instance reuses what the first stored, with no engine of its own', { skip }, async () => { - const fen = freshFen(); - const first = instance(); - try { - await analyze(first, fen); - assert.equal(first.searches(), 1); - } finally { - await first.shutdown(); - } - - const second = instance(); - try { - const results = await analyze(second, fen); - assert.equal(second.searches(), 0, 'the durable row must answer without a search'); - assert.equal(results[0]?.depth, 10, 'and must carry the analysis the first instance found'); - assert.equal(counter(second.metrics, 'analysis_cache_events_total{event="cache_hit"}'), 1); - } finally { - await second.shutdown(); - } + await withDatabase(async () => { + const fen = freshFen(); + const first = instance(); + try { + await analyze(first, fen); + assert.equal(first.searches(), 1); + } finally { + await first.shutdown(); + } + + const second = instance(); + try { + const results = await analyze(second, fen); + assert.equal(second.searches(), 0, 'the durable row must answer without a search'); + assert.equal(results[0]?.depth, 10, 'and must carry the analysis the first instance found'); + assert.equal(counter(second.metrics, 'analysis_cache_events_total{event="cache_hit"}'), 1); + } finally { + await second.shutdown(); + } + }); }); /** @@ -178,23 +299,25 @@ test('a second instance reuses what the first stored, with no engine of its own' * different row — so an upgrade cannot serve yesterday's analysis as today's. */ test('a different engine build does not read the first build\'s rows', { skip }, async () => { - const fen = freshFen(); - const sixteen = instance({ engineName: 'Stockfish 16' }); - try { - await analyze(sixteen, fen); - assert.equal(sixteen.searches(), 1); - } finally { - await sixteen.shutdown(); - } - - const seventeen = instance({ engineName: 'Stockfish 17' }); - try { - await analyze(seventeen, fen); - assert.equal(seventeen.searches(), 1, 'a new build must compute rather than inherit'); - assert.equal(counter(seventeen.metrics, 'analysis_cache_events_total{event="cache_hit"}'), 0); - } finally { - await seventeen.shutdown(); - } + await withDatabase(async () => { + const fen = freshFen(); + const sixteen = instance({ engineName: 'Stockfish 16' }); + try { + await analyze(sixteen, fen); + assert.equal(sixteen.searches(), 1); + } finally { + await sixteen.shutdown(); + } + + const seventeen = instance({ engineName: 'Stockfish 17' }); + try { + await analyze(seventeen, fen); + assert.equal(seventeen.searches(), 1, 'a new build must compute rather than inherit'); + assert.equal(counter(seventeen.metrics, 'analysis_cache_events_total{event="cache_hit"}'), 0); + } finally { + await seventeen.shutdown(); + } + }); }); /** @@ -207,46 +330,50 @@ test('a different engine build does not read the first build\'s rows', { skip }, * can tell those apart. */ test('a dead database costs a recomputation, not a failed analysis', { skip }, async () => { - const node = instance(); - const fen = freshFen(); - try { - // Kill the cache underneath a live manager: every subsequent get and set fails. - await node.tier.shutdown(); - - const results = await analyze(node, fen); - - assert.equal(results.length, 1, 'the caller still gets a real analysis'); - assert.equal(results[0]?.evaluation.value, 20); - assert.equal(node.searches(), 1, 'exactly one search, from the one request'); - assert.equal(counter(node.metrics, 'analysis_cache_faults_total{fault="read"}'), 1); - assert.equal(counter(node.metrics, 'analysis_cache_faults_total{fault="write"}'), 1); - assert.equal( - counter(node.metrics, 'analysis_cache_events_total{event="cache_write_completed"}'), - 1, - 'the engine cannot see that the write failed — which is precisely why the fault counter exists', - ); - } finally { - await node.manager.shutdown({ deadlineMs: 2_000 }); - } + await withDatabase(async () => { + const node = instance(); + const fen = freshFen(); + try { + // Kill the cache underneath a live manager: every subsequent get and set fails. + await node.tier.shutdown(); + + const results = await analyze(node, fen); + + assert.equal(results.length, 1, 'the caller still gets a real analysis'); + assert.equal(results[0]?.evaluation.value, 20); + assert.equal(node.searches(), 1, 'exactly one search, from the one request'); + assert.equal(counter(node.metrics, 'analysis_cache_faults_total{fault="read"}'), 1); + assert.equal(counter(node.metrics, 'analysis_cache_faults_total{fault="write"}'), 1); + assert.equal( + counter(node.metrics, 'analysis_cache_events_total{event="cache_write_completed"}'), + 1, + 'the engine cannot see that the write failed — which is precisely why the fault counter exists', + ); + } finally { + await node.manager.shutdown({ deadlineMs: 2_000 }); + } + }); }); test('a storm of identical requests against a dead cache still runs one search', { skip }, async () => { - const node = instance(); - const fen = freshFen(); - try { - await node.tier.shutdown(); - - const results = await Promise.all(Array.from({ length: 8 }, () => analyze(node, fen))); - - // Single-flight is process-local and unaffected by the cache being gone: eight callers that ask - // for the same identity join one flight, so a cache outage cannot turn request volume into - // engine volume. - assert.equal(node.searches(), 1, 'eight callers, one search'); - assert.equal(counter(node.metrics, 'analysis_cache_events_total{event="request_coalesced"}'), 7); - for (const result of results) assert.equal(result[0]?.depth, 10); - } finally { - await node.manager.shutdown({ deadlineMs: 2_000 }); - } + await withDatabase(async () => { + const node = instance(); + const fen = freshFen(); + try { + await node.tier.shutdown(); + + const results = await Promise.all(Array.from({ length: 8 }, () => analyze(node, fen))); + + // Single-flight is process-local and unaffected by the cache being gone: eight callers that + // ask for the same identity join one flight, so a cache outage cannot turn request volume + // into engine volume. + assert.equal(node.searches(), 1, 'eight callers, one search'); + assert.equal(counter(node.metrics, 'analysis_cache_events_total{event="request_coalesced"}'), 7); + for (const result of results) assert.equal(result[0]?.depth, 10); + } finally { + await node.manager.shutdown({ deadlineMs: 2_000 }); + } + }); }); /** @@ -259,44 +386,56 @@ test('a storm of identical requests against a dead cache still runs one search', * search on a cold miss, which is the price of not adding a distributed lock. */ test('two live instances racing a cold position both compute it', { skip }, async () => { - const fen = freshFen(); - const a = instance(); - const b = instance(); - try { - await Promise.all([analyze(a, fen), analyze(b, fen)]); - - assert.equal(a.searches() + b.searches(), 2, 'cross-process single-flight does not exist'); - - // And the race resolves to one row that either of them can then read. - const reader = instance(); + await withDatabase(async () => { + const fen = freshFen(); + const a = instance(); + const b = instance(); try { - await analyze(reader, fen); - assert.equal(reader.searches(), 0, 'the duplicated work still leaves one usable row'); + await Promise.all([analyze(a, fen), analyze(b, fen)]); + + assert.equal(a.searches() + b.searches(), 2, 'cross-process single-flight does not exist'); + + // And the race resolves to one row that either of them can then read. + const reader = instance(); + try { + await analyze(reader, fen); + assert.equal(reader.searches(), 0, 'the duplicated work still leaves one usable row'); + } finally { + await reader.shutdown(); + } } finally { - await reader.shutdown(); + // Nested rather than sequential: a rejection from the first `shutdown` would otherwise skip + // the second, leaving the other instance's pool and engine worker behind. + try { + await a.shutdown(); + } finally { + await b.shutdown(); + } } - } finally { - await a.shutdown(); - await b.shutdown(); - } + }); }); test('a cache pool that cannot connect degrades to computing, and says so', { skip }, async () => { - // Port 1 is not a PostgreSQL server, so every statement fails at connection time — the shape of a - // misconfigured or unreachable database, as opposed to one that was closed. - const node = instance({ connectionString: 'postgres://nobody:nobody@127.0.0.1:1/none' }); - try { - const results = await analyze(node, freshFen()); - - assert.equal(results.length, 1, 'analysis is unaffected by a cache it cannot reach'); - assert.equal(node.searches(), 1); - assert.ok( - counter(node.metrics, 'analysis_cache_faults_total{fault="read"}') >= 1, - 'an unreachable cache must be reported, not merely missed', - ); - } finally { - await node.shutdown(); - } + // Wrapped like the rest even though its cache never reaches a database: the FEN it mints is + // registered the moment it is minted, so it belongs to a cleanup somewhere, and a `DELETE` that + // matches nothing is the correct outcome rather than a special case. + await withDatabase(async () => { + // Port 1 is not a PostgreSQL server, so every statement fails at connection time — the shape of + // a misconfigured or unreachable database, as opposed to one that was closed. + const node = instance({ connectionString: 'postgres://nobody:nobody@127.0.0.1:1/none' }); + try { + const results = await analyze(node, freshFen()); + + assert.equal(results.length, 1, 'analysis is unaffected by a cache it cannot reach'); + assert.equal(node.searches(), 1); + assert.ok( + counter(node.metrics, 'analysis_cache_faults_total{fault="read"}') >= 1, + 'an unreachable cache must be reported, not merely missed', + ); + } finally { + await node.shutdown(); + } + }); }); /** @@ -307,30 +446,42 @@ test('a cache pool that cannot connect degrades to computing, and says so', { sk * the analysis would wait for the lock rather than the bound, and this test would stop finishing. */ test('a lookup that would hang is bounded by the pool the factory built', { skip }, async () => { - const node = instance(); - const blocker = createPool({ max: 1 }); - const holder = await blocker.connect(); - try { - await holder.query('BEGIN'); - await holder.query('LOCK TABLE engine_analysis_cache IN ACCESS EXCLUSIVE MODE'); - - const startedAt = Date.now(); - const results = await analyze(node, freshFen()); - const elapsed = Date.now() - startedAt; - - assert.equal(results.length, 1, 'the analysis still completes'); - assert.equal(node.searches(), 1); - assert.ok(elapsed < 10_000, `the cache must not wait on the lock (took ${elapsed}ms)`); - assert.ok( - counter(node.metrics, 'analysis_cache_faults_total{fault="read"}') >= 1, - 'the bounded read is reported as a fault, not passed off as a cold cache', - ); - } finally { - await holder.query('ROLLBACK').catch(() => {}); - holder.release(); - await blocker.end(); - await node.shutdown(); - } + await withDatabase(async (pool) => { + const node = instance(); + // The engine is shut down however this exits, acquisition included: a pool that could not hand + // out a connection would otherwise leave the composition's own pool and retention timer behind, + // and an open handle is how a failing test becomes a hanging one. + try { + // The lock is held on a connection from this test's own pool rather than a third one built + // for the purpose: the cache under test has a pool of its own, so these are already different + // backends, which is all the lock needs to bite. + const holder = await pool.connect(); + try { + await holder.query('BEGIN'); + await holder.query('LOCK TABLE engine_analysis_cache IN ACCESS EXCLUSIVE MODE'); + + const startedAt = Date.now(); + const results = await analyze(node, freshFen()); + const elapsed = Date.now() - startedAt; + + assert.equal(results.length, 1, 'the analysis still completes'); + assert.equal(node.searches(), 1); + assert.ok(elapsed < 10_000, `the cache must not wait on the lock (took ${elapsed}ms)`); + assert.ok( + counter(node.metrics, 'analysis_cache_faults_total{fault="read"}') >= 1, + 'the bounded read is reported as a fault, not passed off as a cold cache', + ); + } finally { + // Released before cleanup runs, and rolled back first: a connection handed back mid + // transaction would still hold the exclusive lock, and the `DELETE` that follows would wait + // on it forever. + await holder.query('ROLLBACK').catch(() => {}); + holder.release(); + } + } finally { + await node.shutdown(); + } + }); }); /** @@ -344,90 +495,90 @@ test('a lookup that would hang is bounded by the pool the factory built', { skip * retention expire it — so if the bound ever stopped holding, this is where it would show. */ test('a hot entry outlives the row it came from, by a bounded amount', { skip }, async () => { - const node = instance(); - const fen = freshFen(); - const pool = createPool({ max: 1 }); - try { - await analyze(node, fen); - assert.equal(node.searches(), 1); - - // Asserted, because a zero-row delete would make the rest of this test vacuous: if the durable - // write had never landed, the hot tier would answer for the ordinary reason and the fresh - // instance would recompute for the ordinary reason, and nothing about staleness would have been - // shown. Raised in the CodeRabbit review of PR #19. - const removed = await pool.query('DELETE FROM engine_analysis_cache WHERE fen = $1', [fen]); - assert.equal(removed.rowCount, 1, 'the durable row must have existed for its loss to mean anything'); - - await analyze(node, fen); - assert.equal(node.searches(), 1, 'memory answers without consulting the table it no longer needs'); - - // A process that never cached it sees the truth at once, so the staleness is this one process's - // memory and nothing wider — no other replica can inherit it. - const cold = instance(); + await withDatabase(async (pool) => { + const node = instance(); + const fen = freshFen(); try { - await analyze(cold, fen); - assert.equal(cold.searches(), 1, 'the row really is gone everywhere else'); + await analyze(node, fen); + assert.equal(node.searches(), 1); + + // Asserted, because a zero-row delete would make the rest of this test vacuous: if the + // durable write had never landed, the hot tier would answer for the ordinary reason and the + // fresh instance would recompute for the ordinary reason, and nothing about staleness would + // have been shown. Raised in the CodeRabbit review of PR #19. + const removed = await pool.query('DELETE FROM engine_analysis_cache WHERE fen = $1', [fen]); + assert.equal(removed.rowCount, 1, 'the durable row must have existed for its loss to mean anything'); + + await analyze(node, fen); + assert.equal(node.searches(), 1, 'memory answers without consulting the table it no longer needs'); + + // A process that never cached it sees the truth at once, so the staleness is this one + // process's memory and nothing wider — no other replica can inherit it. + const cold = instance(); + try { + await analyze(cold, fen); + assert.equal(cold.searches(), 1, 'the row really is gone everywhere else'); + } finally { + await cold.shutdown(); + } } finally { - await cold.shutdown(); + // The row `cold` wrote back is not deleted here: it is keyed by a FEN this file minted, so + // the cleanup `withDatabase` runs takes it, on the failure path as well as this one. + await node.shutdown(); } - } finally { - await pool.query('DELETE FROM engine_analysis_cache WHERE fen = $1', [fen]).catch(() => {}); - await pool.end(); - await node.shutdown(); - } + }); }); test('the retention sweep runs against the composed cache without disturbing it', { skip }, async () => { - const node = instance(); - const fen = freshFen(); - const pool = createPool({ max: 1 }); - try { - await analyze(node, fen); - assert.equal(node.searches(), 1); - - // File the row in the last century and sweep with a cutoff at the millennium. The sweep is - // deliberately table-wide, so a cutoff of "a day ago" would also match anything an earlier run - // left behind and the exact count below would be a property of the database rather than of the - // sweep. Nothing else ever writes a row this old. - await pool.query( - `UPDATE engine_analysis_cache SET updated_at = TIMESTAMPTZ '1999-01-01' WHERE fen = $1`, - [fen], - ); - // Swept through the adapter, which is exactly what `AnalysisCacheRetention` holds in production: - // the durable half of the tier, not the hot cache in front of it (ADR-0139 §2). `deleteExpired` - // is the adapter's own and never was part of the `AnalysisCache` port. - // At least one, not exactly one. Nothing else *writes* a row this old, but a previous run that - // died between the back-dating above and its cleanup below would leave one, and then an exact - // count would be a property of the database rather than of the sweep. That this row in - // particular went is proven below, by a process that has to reach the table to find out. - const durable = new PgAnalysisCache(pool); - assert.ok((await durable.deleteExpired(new Date('2000-01-01T00:00:00Z'), 100)) >= 1); - - // The identity is gone from the table, so a process that never cached it recomputes — an expired - // entry costs one search, and the cache heals itself. It is asked on a *fresh* instance because - // `node` still holds the position in its own hot tier; that entry outliving the row it came from - // is ADR-0139's bounded staleness, and the test below is where that is pinned. - const cold = instance(); + await withDatabase(async (pool) => { + const node = instance(); + const fen = freshFen(); try { - await analyze(cold, fen); - assert.equal(cold.searches(), 1, 'an expired row must not still answer'); + await analyze(node, fen); + assert.equal(node.searches(), 1); + + // File the row in the last century and sweep with a cutoff at the millennium. The sweep is + // deliberately table-wide, so a cutoff of "a day ago" would also match anything an earlier + // run left behind and the exact count below would be a property of the database rather than + // of the sweep. Nothing else ever writes a row this old. + await pool.query( + `UPDATE engine_analysis_cache SET updated_at = TIMESTAMPTZ '1999-01-01' WHERE fen = $1`, + [fen], + ); + // Swept through the adapter, which is exactly what `AnalysisCacheRetention` holds in + // production: the durable half of the tier, not the hot cache in front of it (ADR-0139 §2). + // `deleteExpired` is the adapter's own and never was part of the `AnalysisCache` port. + // At least one, not exactly one. Nothing else *writes* a row this old, but a previous run + // that died between the back-dating above and its cleanup below would leave one, and then an + // exact count would be a property of the database rather than of the sweep. That this row in + // particular went is proven below, by a process that has to reach the table to find out. + const durable = new PgAnalysisCache(pool); + assert.ok((await durable.deleteExpired(new Date('2000-01-01T00:00:00Z'), 100)) >= 1); + + // The identity is gone from the table, so a process that never cached it recomputes — an + // expired entry costs one search, and the cache heals itself. It is asked on a *fresh* + // instance because `node` still holds the position in its own hot tier; that entry outliving + // the row it came from is ADR-0139's bounded staleness, pinned by the test above. + const cold = instance(); + try { + await analyze(cold, fen); + assert.equal(cold.searches(), 1, 'an expired row must not still answer'); + } finally { + await cold.shutdown(); + } + + // And the recomputation really did land in the table, rather than only in the process that + // made it — which a second lookup on `cold` could not have shown, because its own hot tier + // would have answered before PostgreSQL was ever asked. + const reader = instance(); + try { + await analyze(reader, fen); + assert.equal(reader.searches(), 0, 'the recomputed row must be durable, not merely in memory'); + } finally { + await reader.shutdown(); + } } finally { - await cold.shutdown(); + await node.shutdown(); } - - // And the recomputation really did land in the table, rather than only in the process that made - // it — which a second lookup on `cold` could not have shown, because its own hot tier would have - // answered before PostgreSQL was ever asked. - const reader = instance(); - try { - await analyze(reader, fen); - assert.equal(reader.searches(), 0, 'the recomputed row must be durable, not merely in memory'); - } finally { - await reader.shutdown(); - } - } finally { - await pool.query('DELETE FROM engine_analysis_cache WHERE fen = $1', [fen]).catch(() => {}); - await pool.end(); - await node.shutdown(); - } + }); }); From 7f64b9eee15cf5ab29bd709b45b4850e3463d73b Mon Sep 17 00:00:00 2001 From: Hussein Mohamed Date: Sat, 5 Sep 2026 12:28:40 +0300 Subject: [PATCH 2/3] docs: record M15 Increment 49 cache test isolation fix MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Records the reproduction on four databases, the proven root cause and the measured masking order, the ownership design and the two candidates rejected, the RED regression, the fresh-database acceptance matrix, and 7 of 9 mutations killed with both survivors named and explained. Marks the analysis-cache-durable fresh-database dependency RESOLVED, and keeps Signature B and the test:counts/services/gateway workspace issue open — the latter re-measured here as exit 1, unchanged in kind. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01CUPqvu66J5ZVyiv4nJ797r --- docs/PROJECT_STATE.md | 152 +++++++++++++++++++++++++++++++++++++++++- docs/ROADMAP.md | 6 +- 2 files changed, 154 insertions(+), 4 deletions(-) diff --git a/docs/PROJECT_STATE.md b/docs/PROJECT_STATE.md index 26359daf..0e053d3e 100644 --- a/docs/PROJECT_STATE.md +++ b/docs/PROJECT_STATE.md @@ -4,7 +4,157 @@ > to read **only this file** and continue immediately. Updated after every > milestone and every significant architectural step. -_Last updated: 2026-09-05 — M15 Increment 48: shared-database ownership for API pg-security integration tests._ +_Last updated: 2026-09-05 — M15 Increment 49: the durable analysis-cache suite establishes its own database._ + + +## M15 Increment 49 — the durable analysis-cache suite establishes its own database + +**Status: RESOLVED.** The defect Increment 48 recorded as out of its own scope is closed. No +production code, migration, constraint, foreign key or repository semantic changed; the whole change +is test lifecycle. + +**The defect, re-proven on current `main` before any edit.** Measured on PostgreSQL 16.14 +(`pgvector/pgvector:pg16`), Node v24.15.0, `pg` 8.22.0, at `origin/main` +`771b1f93c05585294474e95fcb24bf116766db3d`: + +- **A — a genuinely fresh, never-migrated database:** `packages/api/test/analysis-cache-durable.integration.test.ts` + run alone gave **10 tests, 4 pass, 6 fail**. Three failed by throwing SQLSTATE **42P01** + (`relation "engine_analysis_cache" does not exist`) from the test's own statements — the + `LOCK TABLE` in "a lookup that would hang is bounded by the pool the factory built", the `DELETE` + in "a hot entry outlives the row it came from", and the `UPDATE` in "the retention sweep runs + against the composed cache". The other three failed as ordinary assertion failures: "a cold + position is computed once and then served from the database", "a second instance reuses what the + first stored" and "two live instances racing a cold position both compute it" each expected a + durable row and got a recomputation instead. +- **The four that passed did so vacuously.** `PgAnalysisCache` absorbs a database fault, reports it + through `onError`, and returns a miss, so the engine recomputes: a suite about durability went on + running with no durability at all. "A different engine build does not read the first build's rows" + asserts two searches and no cache hit, which a table that does not exist satisfies perfectly; the + two dead-cache tests shut the tier down before touching the table; and the unreachable-server test + never uses `DATABASE_URL` at all. +- **B — an already-migrated database:** **10 tests, 10 pass, 0 fail** — and **5 rows left behind in + `engine_analysis_cache`** every run, one each from the two durable-read tests and the race test and + two from the engine-upgrade test (same FEN, two fingerprints). The four remaining tests never + successfully wrote. +- **C — the masking order, measured rather than assumed.** The whole `packages/api` package against a + fresh database gave **993 tests, 977 pass, 6 fail** — the same six, so no sibling API test masks + it. Running `packages/persistence` first against the same fresh database (**186 pass**) and then + the target file gave **10 pass, 0 fail**. The masking is entirely `packages/persistence`: seventeen + of its suites call `migrate()` on the shared `DATABASE_URL`, and both the root `test` script + (`package.json`, persistence before api) and the CI `postgres-integration` job (`npm test + --workspace @chess-platform/persistence` before `--workspace @chess-platform/api`) run that package + first. + +**Root cause.** The suite required schema it did not establish. `engine_analysis_cache` is created by +`packages/persistence/migrations/0026_engine_analysis_cache.sql` and indexed by `0027`; the file +never called `migrate()`, never used `withTestDatabase` or `withSharedDatabase`, and opened pools +straight onto `DATABASE_URL`. It was the only file in the repository operating on application tables +while neither migrating nor creating scratch DDL of its own. The residue is the same ownership +question from the other side: `freshFen()` minted a unique position per test, which is +collision-avoidance and not cleanup — a fresh identity means the *next* run never collides, not that +this one took its rows back. + +**Ownership and lifecycle design.** Three candidates were compared against the repository's own +patterns before anything was written: (A) migrate the shared `DATABASE_URL` once per file and own the +rows; (B) a disposable migrated database per test via `withTestDatabase`; (C) migrate inside every +test body. **A was chosen.** It is the shape of the sibling suite for this exact table, +`packages/persistence/test/analysis-cache.integration.test.ts` — a file-scoped `migrated` flag, an +`ensureMigrated(pool)`, `withSharedDatabase`, and cleanup scoped to minted identities. It matches +`packages/api/test/analysis-real-stack.test.ts`, which runs the same composition and applies the +canonical migrations itself, and which CI already proves against a never-migrated database in the +`analysis-smoke` job. B was rejected on cost and blast radius: ten `CREATE DATABASE` statements, ten +passes over 31 migrations, ten quiescence waits and ten drops, for a suite that needs one table, and +a crash mid-run would leave orphaned `test_db_*` databases behind. C was rejected as A without the +flag — the same semantics, re-walking 31 migrations ten times. + +**Regression first, proven RED for the right reason.** +`packages/api/test/analysis-cache-durable-ownership.integration.test.ts` was written before the fix +and run against the unmodified suite: **3 tests, 0 pass, 3 fail**, with +`relation "engine_analysis_cache" does not exist` appearing four times in the output and the residue +check reporting the exact five surplus rows by identity. It follows the parent/child harness +Increment 48 established: a disposable database from `withTestDatabase` — which creates a database +and applies nothing, so it *is* the fresh condition — and the compiled suite spawned as a child with +`NODE_TEST_CONTEXT` deleted (a child that inherits it silently declines to run the file and exits 0) +and `--test-reporter=tap` pinned. The three readings are: the suite passes against a database nothing +else prepared and leaves it empty; any single one of its tests can be the only one that runs +(`--test-name-pattern`, so that establishing the schema in the first test would not satisfy the +check); and against a migrated database carrying a stranger's row the table is byte-for-byte as it +was found. The child is deliberately spawned from the repository root, not from `packages/api`, so a +working directory the suite does not control cannot decide whether its schema gets built. + +**Implementation.** `MIGRATIONS` resolves from the file's own location rather than `process.cwd()`; +`ensureMigrated(pool)` applies the canonical ledger once behind a file-scoped flag; every test now +runs inside `withDatabase`, which is `withSharedDatabase({ max: 2, cleanup: deleteMintedRows })`; +`freshFen()` records each identity before returning it, so a body that throws after a commit still +hands cleanup something it owns; and cleanup deletes by exact FEN equality +(`WHERE fen = ANY($1::text[])`), never by the `rnbqkbnr/...` prefix every standard starting position +shares. The minted counters start at a million because the persistence package's own cache suite keys +its rows on the canonical `... 0 1` and this file deletes by FEN without regard to fingerprint. Tests +that previously built their own single-connection pools use the one `withDatabase` hands them, and +the two ad-hoc `finally` deletes — which swallowed their own failures with `.catch(() => {})` — are +gone, because cleanup now owns those rows on the failure path as well. An `after` hook audits that +the file left nothing behind, which is the reading a developer running only this file still gets. +Two lifecycle holes found in adversarial review were closed while the code was open: the lock-holding +test acquired its client outside the `try` that guarantees the engine is shut down, and the racing +test awaited two shutdowns in sequence so that a rejection from the first skipped the second. + +**Acceptance, on the final code.** Three independent brand-new empty databases: **10/10, 10/10, +10/10**, with **0 rows** left in `engine_analysis_cache` in each. An already-migrated database +carrying five unrelated rows: **10/10**, and all five still there afterwards — cleanup is scoped to +what the run owns, not to the table. The whole `packages/api` package against a brand-new database, +with no other package run first: **996 tests, 986 pass, 0 fail, 10 skipped** (the engine-binary smoke +files, which self-skip without `STOCKFISH_PATH`), **0** rows of residue and **0** leaked `test_db_*` +databases. + +**Falsification: 7 of 9 mutations killed, and the two survivors are reported as they are.** Killed: +removing the schema establishment; asserting the ledger was already applied; pointing the composition +at no database so the durable tier silently switches off; skipping teardown; widening cleanup to +`DELETE FROM engine_analysis_cache`; resolving the migrations from the working directory again; and +recording the minted identity for the audit but not for the cleanup that has to act on it. +**Survivor 1** lets a teardown failure replace the body failure — that precedence contract belongs to +`withSharedDatabase`, not to this file, and the same mutation is killed by the suite that owns it +(`packages/persistence/test/reused-database.integration.test.ts`, 6 failing). **Survivor 2** migrates +only on the first body to run instead of behind the flag; it is an equivalent mutant, because with +tests running one at a time the two are the same observable behaviour. Every mutated source was +restored and verified byte-identical by SHA-256. + +**Validation, run fresh on the final code.** `npm run build` and `npm run lint` clean. The ownership +regression 3/3. The full repository suite against one PostgreSQL 16.14 database: **exit 0, 19 +packages, 3304 tests, 3276 pass, 0 fail, 28 skipped**, with **0** suites self-skipping for a missing +`DATABASE_URL` — so the database-backed tests, which are the ones this increment touches, actually +ran. `check:ci-parity`, `check:variant-parity`, `check:adr-claims`, `check:engine-pin-parity`, +`check:observability` and `test:scripts` all exit 0; `git diff --check` clean. + +### Known limits recorded rather than fixed + +- The retention test calls `deleteExpired(new Date('2000-01-01'), 100)`, which is table-wide by + design — it is the production sweep under test, not cleanup this increment added. Nothing else + writes a row dated 1999, but on a database shared with a suite that does, this delete is not + identity-scoped. Pre-existing and unchanged. +- `mintedFens` is module-scoped and drained by whichever cleanup runs next, which assumes tests run + one at a time. That is what `--test-concurrency=1` and node's sequential top-level tests give, and + it is the same assumption the sibling persistence suite makes; a test that opted itself into + concurrency would have its rows removed by a neighbour's cleanup. Now stated in the file rather + than implied. +- The `finally` blocks that shut engines down are still plain `try/finally`, so a `shutdown()` that + rejects while an assertion is already failing replaces it. Pre-existing, unchanged, and now said + out loud where the opposite could have been read into the cleanup docstring. + +### Still open after Increment 49 + +- **Signature B — UNRESOLVED.** Increment 49 recorded **0 occurrences** across its baseline + reproduction, acceptance runs, two mutation rounds and full-repository validation. Like Increment + 47's 26 bounded runs and Increment 48's zero, that bounds the rate under those conditions and + resolves nothing. +- **`npm run test:counts` exits non-zero because `services/gateway` sits outside the npm + workspaces — OPEN, not fixed here.** Increment 49 measured it again, without piping the command + through anything that could swallow its status: it **exited 1**, reporting **3256 tests (113 + skipped)** and `gateway-service: ERROR` — six `TS2307: Cannot find module 'ioredis'` diagnostics, + because the root `workspaces` field is `packages/*` and that service's dependencies are never + installed by a root `npm install`. Its own totals are lower than the suite's above because it runs + without `DATABASE_URL`, so the database suites self-skip; that is a property of how the script is + invoked, not a change in coverage. No gateway dependency was installed or mutated in this + increment, and the remedy is still not assumed to be "add it to the workspaces". ## M15 Increment 48 — shared-database ownership for API pg-security integration tests diff --git a/docs/ROADMAP.md b/docs/ROADMAP.md index 43144658..335ee7de 100644 --- a/docs/ROADMAP.md +++ b/docs/ROADMAP.md @@ -1349,12 +1349,12 @@ Debt observed during M14. Each states what is known, not what is planned; items - **The published capability document could omit a composed feature, and the client offered controls that could not work (RESOLVED in M15 Increment 24 / ADR-0132).** Left open by Increment 23 above. `capabilitiesView` (`packages/api/src/presenters.ts`) took a hand-written `Pick`, so a feature never added to it was invisible to `GET /v1/capabilities` and nothing complained. ADR-0131 judged that "a narrower failure than a 503 — the feature works for anyone who calls the route directly", and deferred it on the grounds that closing it meant first deciding which optional dependencies are user-facing capabilities. **Both halves of that were wrong.** The failure is narrower only when the client can still reach the feature, and a live instance already existed where it could not: `GET /v1/search` served three modes from two independently-gated dependency sets behind one published `search` flag, so a deployment running the Helm chart’s `search.semanticEnabled: false` advertised search while the client offered two mode buttons whose every request answered 503. And the judgement, while genuinely underivable, can be made **unskippable**, which was the property wanted. `semanticSearch` is now published from both of its dependencies, `Exclude[0] | NotAPublishedCapability>` must be `never` so a new optional dependency cannot compile until someone gives it a flag or records why it has none, and a behavioural guard requires every capability source to change what the document publishes — because a key can sit in that parameter unread, which the compile-time half cannot see. The mutation ledger lives in ADR-0132 and is not restated here. - **The search surface was not gated on `capabilities.search`, so an absolute kill switch still showed a search box (RESOLVED in M15 Increment 24 / ADR-0132 §5).** `SEARCH_ENABLED=0` — the chart's `search.enabled: false`, an absolute kill switch per ADR-0055 — leaves `searchRepository` unconstructed and `GET /v1/search` answering 503 on every mode, keyword included. The entry point was the persistent header form in `packages/web/index.html`, present on every page; being a `
` rather than an `a[data-route]`, `NAV_CAPABILITY_MAP` could not reach it, which is why the first pass at Increment 24 gated the semantic and hybrid *modes* and left keyword ungated — the same defect class one mode over. Raised by the Qodo review of PR #155. **Fixed in the same increment rather than deferred:** the form now ships `hidden` and is revealed by `applySearchCapability` only on an explicit `search: true`; the route renders an honest unavailable notice and issues no request; and keyword search waits for the capability answer, reversing this increment's own earlier latency decision, because knowing whether a request is pointless requires having asked. A markup-contract test pins the `hidden` attribute, since the gate depends on it and every other test passes without it. - **Clicking a search mode discarded text typed since the page loaded (RESOLVED in a follow-up to M15 Increment 24 / ADR-0132).** `createModeInput` in `packages/web/src/app/search-mount.ts` closed over the query captured when the route mounted, so `navigateToSearchMode` navigated with the old term and the remount reset the input to match it. Type a new term into the header field, click **Semantic** without pressing enter, and the typed text was gone with no indication it had been discarded. Pre-existing — the closure predated Increment 24 and was untouched by it — and found by the adversarial review of PR #155 while reviewing the capability gate wrapped around the same control. **Resolved:** the query is now a `() => string` read when a mode is chosen rather than a string captured when the selector renders, matching what `main.ts`'s submit handler already does, and falling back to the mounted query only where the document has no header input. Two regression tests, one of which fails against the exact pre-fix closure. -- **`startHarness` drew ports `fetch` refuses (RESOLVED in ADR-0140); the unexplained whole-file failure is still open.** Two signatures were filed here, deliberately not as one cause, and that judgement held. **Signature A is resolved.** WHATWG Fetch blocks eighty-two ports and undici enforces the list on the port number alone, before opening a socket — so `server.listen(0)` could bind, listen and answer raw TCP while `fetch` still refused, surfacing as `TypeError: fetch failed` / `Error: bad port` at the harness’s first request rather than at the listen that caused it. Whether it can happen at all is a property of the host’s dynamic port range: a typical Linux CI range (32768–60999) contains no blocked port, while the Windows range in use here (1024–15000) contains nineteen, which is the whole of "green on CI, flaky locally". The guard that already existed was incomplete in a way that still failed — its hand-observed set of eighteen ports was the spec list intersected with one machine’s range **minus `6679`** — and it retried unboundedly, had no behaviour on exhaustion, and had been copy-pasted into `auth-signin-schema.integration.test.ts`, so the missing port had to be found twice. `packages/api/test/listen.ts` now owns port acquisition for both sites: the spec-complete eighty-two ports (verified by sweeping all 65535 through the real `fetch` on Node v24.15.0), a bounded twenty attempts, each rejected listener closed before the next is asked for, a guarded `address()` read in place of the `as AddressInfo` cast, and an exhaustion error naming the attempts and rejected ports and nothing else. **Signature B is not resolved and was not folded in.** A file still fails with `'test failed'`, no assertion, no stack and none of its own tests reported. Twenty consecutive full runs gave five failures: on pre-fix code one signature A (`auth.test.js`) and three signature B; on post-fix code one signature B and no signature A. The port fix removes A and leaves B exactly where it was, which is the evidence that they are two defects. Four different files were hit (`move-explanation-route`, `tournament-commentary-route`, `bot-detection-analyze`, `anti-cheat-analysis`), sharing no import beyond `./helpers`; each died in 589–703 ms with no test of its own reporting and no stderr. Refuted with evidence: ephemeral-port exhaustion (113 sockets in TIME_WAIT against a 13977-port range), a `Promise.race` loser becoming an unhandled rejection (`race` subscribes to every promise, confirmed on v24.15.0), a throwing `after`/`afterEach` hook (the affected files use none), and a double `close()` rejecting (awaited in a `finally`, it would be attributed to that test with a stack). A second bounded pass — twelve more full runs under the TAP reporter with a preload recording `uncaughtException`, `unhandledRejection` and any non-zero exit — produced twelve clean runs and captured nothing at the time. **A follow-up increment (`claude/node-test-signature-b`) then captured the defect directly, three more times, on three files never previously implicated** (`rate-limit-atomicity`, `dependency-parity`, `studies-api`) — seven distinct files observed with this symptom to date. Occurrences across seven distinct files make a shared or cross-cutting path more plausible and make a defect confined to one test file less likely, but do not exclude file-specific inputs or lifecycle interactions. An instrumented preload (`packages/api/test/diagnostics/signature-b-preload.cjs`) hooking process-level events — `process.exit`, `process.abort`, `process.kill`, `uncaughtExceptionMonitor` (passively observing uncaught exceptions and fatal unhandled rejections), `warning`, `beforeExit`, and Node’s own unconditional `exit` — showed **none of the hooks active at the time fired** on any of the three historical captures (though `process.abort()` was not wrapped in those initial runs and is now covered for future occurrences). A synthetic `process.exit(1)`-before-registration fixture reproduces the identical silent shape; every other synthetic mechanism tried (a post-test async throw, an emitter `'error'` with its listener removed, a synchronous module-load throw, a delayed `SIGKILL`) prints a visibly different diagnostic line, stack, or partial test output that the real defect never shows. This narrows the investigated possibilities while leaving the root cause unresolved: the per-file child process (`node --test` spawns one per file, confirmed by distinct PIDs) was not terminated by `process.exit`, uncaught exceptions, or fatal unhandled rejections, and future runs with `process.abort` instrumentation will record whether abort was called through JS; an absent record narrows in-runtime JS termination but cannot alone prove external termination without corroborating child exit status/signal data or OS-level crash evidence (e.g. distinguishing an external kill or uncatchable signal from a native C++/V8 crash). The machine had roughly 2.5 GB of 15.7 GB RAM free at capture time with several other agents’ processes concurrently running, which is circumstantially consistent with resource contention, but no crash was recorded in the Windows Application or System event logs in that window, so the exact external trigger is still not established. No fix was invented — the forbidden responses (sleeps, whole-file retries, lowering concurrency) would only hide the unresolved root cause, whose origin is not yet established. **A further increment then crossed the parent/child boundary the earlier work stopped at, and found the evidence had been there all along:** Node's runner attaches the child's `exitCode` and `signal` to the `ERR_TEST_FAILURE` it throws, and the `spec` reporter discards them — `formatError` replaces the error with `error.cause`, the bare string `'test failed'` — while the built-in `tap` reporter serializes them, so running `spec` to stdout and `tap` to a file recovers the exit status with no custom reporter and no patched internals. Exit codes were measured on this platform rather than assumed: `process.abort()` gives `134`, `Stop-Process -Force` gives `4294967295`, NTSTATUS faults surface as raw unsigned values such as `3221225477` (`0xC0000005`) — and `1` is produced alike by an uncaught exception, `process.exit(1)`, `taskkill /F` and `process.kill`, so it identifies nothing on its own and is classified `inconclusive`. `signature-b-correlate.cjs` joins the parent's TAP record to the child's JSONL log on the test file path (which also yields the child PID) and states what the pair does and does not establish; where the exit code is ambiguous, a child that reached `preload-installed` and then logged nothing still excludes `process.exit` and an uncaught exception, because both would have left a record and fired Node's `exit` event. A bounded pass of 20 runs under this instrumentation produced 0 captures — which bounds the rate and proves nothing: treating the historical ~1-in-5 as an independent per-run rate, zero captures in 20 runs has probability `(4/5)^20 ≈ 1.2%`, and independence is an assumption rather than an established fact; it ran at 3084–3834 MB free against roughly 2.5 GB at the historical captures, consistent with the resource-contention hypothesis but not evidence for it. **Signature B stays UNRESOLVED**; what changed is that the next occurrence is readable rather than silent. **During M15 Increment 46 validation, Signature B was directly observed three more times, broadening the observed scope to `packages/persistence`:** `search-backfill.integration.test.ts`, `learning.integration.test.ts`, and `test-database.integration.test.ts` each died with the documented bare whole-file `'test failed'` with zero tests reporting and no assertion or stack, and each passed standalone against the exact database state it died on. This confirms the defect is not specific to `packages/api` or its HTTP server test harness. **M15 Increment 47 hardened the diagnostic correlator across 26 bounded runs:** 1 baseline full-suite run, 20 sequential runs under `run-signature-b-pass.mjs`, and 5 concurrent repository-native load runs produced 0 captures (which bounds the rate under those conditions and does not resolve the defect). Code inspection and falsification identified and fixed four correlator blind spots in `packages/api/test/diagnostics/signature-b-correlate.cjs`: (1) cross-directory basename fallback could falsely match a child log from another directory when the target had slashes; (2) Windows path case folding was host-dependent (`process.platform === 'win32'`), breaking cross-platform correlation when logs were analyzed on a different OS; (3) signed 32-bit Windows NTSTATUS exit codes (e.g. `0xC0000005` surfacing as `-1073741819`) were not normalized to unsigned values; and (4) quoted TAP YAML scalar tokens lost exit code or duration numbers. The targeted test suite was expanded from 23 to 41 tests (39 pass, 2 skip, 0 fail), killing 4 falsification mutations. **Signature B remains UNRESOLVED;** no production fix was invented, and the next occurrence will be correlated without cross-platform or exit-code blind spots. **M15 Increment 48 recorded 0 Signature B occurrences** across its baseline reproduction, A/B/C acceptance, mutation rounds and full-repository validation — which, like Increment 47's 26 bounded runs, bounds the rate under those conditions and resolves nothing. It stays an open tracked defect. See ADR-0140 §4. +- **`startHarness` drew ports `fetch` refuses (RESOLVED in ADR-0140); the unexplained whole-file failure is still open.** Two signatures were filed here, deliberately not as one cause, and that judgement held. **Signature A is resolved.** WHATWG Fetch blocks eighty-two ports and undici enforces the list on the port number alone, before opening a socket — so `server.listen(0)` could bind, listen and answer raw TCP while `fetch` still refused, surfacing as `TypeError: fetch failed` / `Error: bad port` at the harness’s first request rather than at the listen that caused it. Whether it can happen at all is a property of the host’s dynamic port range: a typical Linux CI range (32768–60999) contains no blocked port, while the Windows range in use here (1024–15000) contains nineteen, which is the whole of "green on CI, flaky locally". The guard that already existed was incomplete in a way that still failed — its hand-observed set of eighteen ports was the spec list intersected with one machine’s range **minus `6679`** — and it retried unboundedly, had no behaviour on exhaustion, and had been copy-pasted into `auth-signin-schema.integration.test.ts`, so the missing port had to be found twice. `packages/api/test/listen.ts` now owns port acquisition for both sites: the spec-complete eighty-two ports (verified by sweeping all 65535 through the real `fetch` on Node v24.15.0), a bounded twenty attempts, each rejected listener closed before the next is asked for, a guarded `address()` read in place of the `as AddressInfo` cast, and an exhaustion error naming the attempts and rejected ports and nothing else. **Signature B is not resolved and was not folded in.** A file still fails with `'test failed'`, no assertion, no stack and none of its own tests reported. Twenty consecutive full runs gave five failures: on pre-fix code one signature A (`auth.test.js`) and three signature B; on post-fix code one signature B and no signature A. The port fix removes A and leaves B exactly where it was, which is the evidence that they are two defects. Four different files were hit (`move-explanation-route`, `tournament-commentary-route`, `bot-detection-analyze`, `anti-cheat-analysis`), sharing no import beyond `./helpers`; each died in 589–703 ms with no test of its own reporting and no stderr. Refuted with evidence: ephemeral-port exhaustion (113 sockets in TIME_WAIT against a 13977-port range), a `Promise.race` loser becoming an unhandled rejection (`race` subscribes to every promise, confirmed on v24.15.0), a throwing `after`/`afterEach` hook (the affected files use none), and a double `close()` rejecting (awaited in a `finally`, it would be attributed to that test with a stack). A second bounded pass — twelve more full runs under the TAP reporter with a preload recording `uncaughtException`, `unhandledRejection` and any non-zero exit — produced twelve clean runs and captured nothing at the time. **A follow-up increment (`claude/node-test-signature-b`) then captured the defect directly, three more times, on three files never previously implicated** (`rate-limit-atomicity`, `dependency-parity`, `studies-api`) — seven distinct files observed with this symptom to date. Occurrences across seven distinct files make a shared or cross-cutting path more plausible and make a defect confined to one test file less likely, but do not exclude file-specific inputs or lifecycle interactions. An instrumented preload (`packages/api/test/diagnostics/signature-b-preload.cjs`) hooking process-level events — `process.exit`, `process.abort`, `process.kill`, `uncaughtExceptionMonitor` (passively observing uncaught exceptions and fatal unhandled rejections), `warning`, `beforeExit`, and Node’s own unconditional `exit` — showed **none of the hooks active at the time fired** on any of the three historical captures (though `process.abort()` was not wrapped in those initial runs and is now covered for future occurrences). A synthetic `process.exit(1)`-before-registration fixture reproduces the identical silent shape; every other synthetic mechanism tried (a post-test async throw, an emitter `'error'` with its listener removed, a synchronous module-load throw, a delayed `SIGKILL`) prints a visibly different diagnostic line, stack, or partial test output that the real defect never shows. This narrows the investigated possibilities while leaving the root cause unresolved: the per-file child process (`node --test` spawns one per file, confirmed by distinct PIDs) was not terminated by `process.exit`, uncaught exceptions, or fatal unhandled rejections, and future runs with `process.abort` instrumentation will record whether abort was called through JS; an absent record narrows in-runtime JS termination but cannot alone prove external termination without corroborating child exit status/signal data or OS-level crash evidence (e.g. distinguishing an external kill or uncatchable signal from a native C++/V8 crash). The machine had roughly 2.5 GB of 15.7 GB RAM free at capture time with several other agents’ processes concurrently running, which is circumstantially consistent with resource contention, but no crash was recorded in the Windows Application or System event logs in that window, so the exact external trigger is still not established. No fix was invented — the forbidden responses (sleeps, whole-file retries, lowering concurrency) would only hide the unresolved root cause, whose origin is not yet established. **A further increment then crossed the parent/child boundary the earlier work stopped at, and found the evidence had been there all along:** Node's runner attaches the child's `exitCode` and `signal` to the `ERR_TEST_FAILURE` it throws, and the `spec` reporter discards them — `formatError` replaces the error with `error.cause`, the bare string `'test failed'` — while the built-in `tap` reporter serializes them, so running `spec` to stdout and `tap` to a file recovers the exit status with no custom reporter and no patched internals. Exit codes were measured on this platform rather than assumed: `process.abort()` gives `134`, `Stop-Process -Force` gives `4294967295`, NTSTATUS faults surface as raw unsigned values such as `3221225477` (`0xC0000005`) — and `1` is produced alike by an uncaught exception, `process.exit(1)`, `taskkill /F` and `process.kill`, so it identifies nothing on its own and is classified `inconclusive`. `signature-b-correlate.cjs` joins the parent's TAP record to the child's JSONL log on the test file path (which also yields the child PID) and states what the pair does and does not establish; where the exit code is ambiguous, a child that reached `preload-installed` and then logged nothing still excludes `process.exit` and an uncaught exception, because both would have left a record and fired Node's `exit` event. A bounded pass of 20 runs under this instrumentation produced 0 captures — which bounds the rate and proves nothing: treating the historical ~1-in-5 as an independent per-run rate, zero captures in 20 runs has probability `(4/5)^20 ≈ 1.2%`, and independence is an assumption rather than an established fact; it ran at 3084–3834 MB free against roughly 2.5 GB at the historical captures, consistent with the resource-contention hypothesis but not evidence for it. **Signature B stays UNRESOLVED**; what changed is that the next occurrence is readable rather than silent. **During M15 Increment 46 validation, Signature B was directly observed three more times, broadening the observed scope to `packages/persistence`:** `search-backfill.integration.test.ts`, `learning.integration.test.ts`, and `test-database.integration.test.ts` each died with the documented bare whole-file `'test failed'` with zero tests reporting and no assertion or stack, and each passed standalone against the exact database state it died on. This confirms the defect is not specific to `packages/api` or its HTTP server test harness. **M15 Increment 47 hardened the diagnostic correlator across 26 bounded runs:** 1 baseline full-suite run, 20 sequential runs under `run-signature-b-pass.mjs`, and 5 concurrent repository-native load runs produced 0 captures (which bounds the rate under those conditions and does not resolve the defect). Code inspection and falsification identified and fixed four correlator blind spots in `packages/api/test/diagnostics/signature-b-correlate.cjs`: (1) cross-directory basename fallback could falsely match a child log from another directory when the target had slashes; (2) Windows path case folding was host-dependent (`process.platform === 'win32'`), breaking cross-platform correlation when logs were analyzed on a different OS; (3) signed 32-bit Windows NTSTATUS exit codes (e.g. `0xC0000005` surfacing as `-1073741819`) were not normalized to unsigned values; and (4) quoted TAP YAML scalar tokens lost exit code or duration numbers. The targeted test suite was expanded from 23 to 41 tests (39 pass, 2 skip, 0 fail), killing 4 falsification mutations. **Signature B remains UNRESOLVED;** no production fix was invented, and the next occurrence will be correlated without cross-platform or exit-code blind spots. **M15 Increment 48 recorded 0 Signature B occurrences** across its baseline reproduction, A/B/C acceptance, mutation rounds and full-repository validation — which, like Increment 47's 26 bounded runs, bounds the rate under those conditions and resolves nothing. It stays an open tracked defect. **M15 Increment 49 likewise recorded 0 occurrences** across its pre-fix reproduction on four databases, its fresh-database acceptance runs, two rounds of falsification and a full-repository run of 3304 tests. Three zero-observation increments in a row bound the rate and establish nothing about the cause; it stays an open tracked defect. See ADR-0140 §4. - **Isolated-database test teardown dropped databases out from under connections that had not finished closing (RESOLVED in M15 Increment 45).** `withDatabase` in `packages/persistence/test/variant-migrations.integration.test.ts` ended its pool and then immediately ran `DROP DATABASE ... WITH (FORCE)`. `pool.end()` does not wait for its clients to close: in pg 8.22.0 `_pulseQueue` reaches the end callback in the same synchronous turn in which `_remove` filters the last client out of `_clients`, while `client.end()` has only queued the Terminate byte — instrumentation recorded **zero of four `remove` events fired at the moment `end()` resolved**. The drop could therefore still find a backend attached; `FORCE` terminated it, and the resulting `FATAL` arrived on a socket whose pool still had `idleListener` attached, which `pg` re-emitted as `pool.emit('error')` — an unhandled EventEmitter error that `node:test` attributed to whichever test was running rather than to the teardown that caused it. It surfaced as intermittent `terminating connection due to administrator command` failures in `postgres integration (persistence)` during M15 Increment 44, on a different test each run, which is the signature of a race rather than a broken assertion. The same shape existed in `packages/api/test/auth-signin-schema.integration.test.ts`, which had absorbed SQLSTATE 57P01 with a `pool.on('error', ...)` listener — a symptom fix for the same cause. **Resolved in Increment 45:** a shared `withTestDatabase` helper (`@chess-platform/persistence/test-support`) ends the pool under a bound, waits for `pg_stat_activity` to report the database unused, and drops it *without* `FORCE`. Measured on PostgreSQL 16.14, a plain drop against a still-attached backend fails with SQLSTATE 55006 and leaves that connection untouched, where `FORCE` succeeds by killing it — so the change trades a quiet, harmful success for a loud, harmless failure. FORCE remains only on the emergency path that guarantees the disposable database is still dropped once teardown has already failed — best effort, since that last drop runs inside a `catch` so it cannot bury the error being reported. The 57P01 absorber is deleted, because the corrected lifecycle never causes one. `createPool` and `migrate` are unchanged, and no migration was added. - **The persistence integration suite was not idempotent against a reused database (RESOLVED in M15 Increment 46).** Recorded as a known defect by Increment 45 and left open there. Against a fresh PostgreSQL 16 database the suite passed; a second run against the *same* database failed nine tests, deterministically — measured on 16.14 before any edit as **173 pass / 0 fail** then **164 pass / 9 fail**. CI provisions a fresh server per run, so it never surfaced there. One contract was being broken in two directions: a suite sharing `chess_test` must remove every row it created and remove nothing else. `achievements.integration.test.ts` broke the second half with an unqualified `DELETE FROM users` in `beforeEach` — `games.white_id` and `games.black_id` are the only references to `users` without `ON DELETE CASCADE` (thirty-one FKs point at `users`; twenty-nine cascade; the two that do not are both on `games`), so one game left behind by `pg.integration.test.ts` aborted the wipe with SQLSTATE 23503 before any assertion ran, and the same statement destroyed the bot accounts migration 0021 seeds, which nothing restores because `migrate` has already recorded 0021 as applied. `pg/identity-tokens.test.ts` and `tournaments.pg.integration.test.ts` broke the first half, leaving fixed primary keys behind and colliding on `users_pkey` and `tournaments_pkey` (the latter surfacing through the repository's compare-and-set as `VersionConflictError`). **Resolved in Increment 46:** suites that legitimately share the database delete exactly their own rows through `withSharedDatabase` (`packages/persistence/src/test-support/fixtures.ts`), the sibling of Increment 45's `withTestDatabase` and heir to its precedence rule — a cleanup failure never replaces the assertion that actually failed, and never disappears either. `pg.integration.test.ts` moved to disposable databases instead, because it cannot meet the cleanup half at all: it appends to `game_events`, which is append-only by production trigger, so cleaning up after itself would have meant weakening a production safety rule to suit a test. `users-batch`, `anti-cheat`, `bot-reports` and `analysis-cache` were corrected for the same contract though none of them ever failed — fresh `uuidv7()` ids meant their leaked rows could not collide, so the tables merely grew on every run. Serialization was not the fix and was not introduced: `--test-concurrency=1` is a pre-existing documented invariant and the second run failed identically under it. Acceptance is three consecutive runs against one database with no reset between them (186/186/186), after which only the three migration-seeded bot accounts remain, with no leaked disposable databases and no lingering backends; falsification killed 17 of 20 mutations. No production code, migration, checksum, constraint or repository conflict semantic changed, and no migration was added. - **The API pg-security integration suite leaked every row it created into the shared database (RESOLVED in M15 Increment 48).** Recorded as a known defect by Increment 46 and deliberately left open there. `packages/api/test/pg-security.integration.test.ts` created users, password credentials, roles, sessions and rate-limit buckets through real repositories and closed its pools without removing any of them. Because every identifier it mints is a fresh `uuidv7()`, no run collided with another, so all 11 tests passed indefinitely while the database grew — measured on PostgreSQL 16.14 before any edit as **11/11 passing and 25 rows leaked on the first run (4 users, 4 credentials, 4 roles, 4 sessions, 9 rate-limit buckets), then 11/11 again and an identical further 25 on a second run against the same database**. The failure mode was silent accumulation, not a failing test, which is why nothing in the file could see it. `rate_limit_buckets` is the sharpest case: no foreign key references it, so no cascade can ever reach those rows, and `PgRateLimiter.sweep` only evicts buckets that expired over an hour ago. **Resolved in Increment 48:** every test runs inside Increment 46's `withSharedDatabase` contract and names what it owns — users deleted by exact owned id, with the proven `ON DELETE CASCADE` foreign keys removing their credentials, roles and sessions (the only non-cascading references to `users` are `games.white_id` and `games.black_id`, and this file creates no games), and rate-limit buckets deleted by exact owned key. Identifiers are recorded before the statement that creates the row, so a body that throws after a commit still surrenders it. Prefix-matched cleanup, `TRUNCATE`, broad `DELETE`, random-identifier workarounds, retries, conflict suppression and serialization were all rejected; `./test-support/fixtures` was added to the persistence package's `exports` so the canonical helper could be reused rather than duplicated. A second defect was fixed in the same increment: the bucket-creation race test read backend PIDs before the `try` whose `finally` releases the leased client, and a pool with a client still checked out never settles `pool.end()`, so a failure there hung the file instead of reporting the error — both reads now sit inside the protected region. Acceptance is three consecutive runs against one migrated database with no reset between them (**11/11, 11/11, 11/11**), after which the database state is identical to its pre-run reading, migration seeds and unrelated sentinels are preserved, no `test_db_*` databases are leaked and no backends linger; falsification killed **8 of 9 mutations**, the survivor being the `backendPid` error path, which needs fault injection no passing suite performs. No production code, migration, constraint, foreign key or repository semantic changed. -- **`analysis-cache-durable.integration.test.ts` depends on schema it does not establish (OPEN, found during M15 Increment 48).** `packages/api/test/analysis-cache-durable.integration.test.ts` never calls `migrate()` — it opens pools and assumes the schema is already present. Inside the full repository suite it passes, because `packages/persistence` migrates the shared database first; run on its own against a fresh database it **fails 6 of 10**. Proven by running that one file against three databases: a fresh database gave 6 failures, and two previously migrated databases gave 0 each. This is the mirror image of the defect Increment 48 closed — a suite depending on state it does not establish, rather than leaking state it does not remove — and belongs to the same ownership family. **Open:** a bounded follow-up, to be fixed in its own PR; deliberately not fixed in Increment 48 so that increment's scope stayed on the leak it was opened for. -- **`npm run test:counts` exits non-zero because `services/gateway` sits outside the npm workspaces (OPEN, observed during M15 Increment 48).** During Increment 48 the script **exited 1**, reporting **3286 tests (28 skipped)** at the implementation HEAD and **3301 tests (28 skipped)** after that branch was synchronized with `main` — the exit code and its cause unchanged by the merge. The non-zero exit came entirely from `services/gateway`: the root `workspaces` field is `packages/*`, so that service's dependencies are never installed by a root `npm install` and its local `ioredis` module resolution fails during the counts run. No gateway dependency was installed or mutated while closing Increment 48, because that would modify `services/gateway/package-lock.json` outside the increment's scope. **Open:** a bounded tooling/workspace investigation in its own right. The remedy is not assumed to be "add it to the workspaces" — what the service's exclusion is for, and what CI relies on, has to be established before changing it. +- **`analysis-cache-durable.integration.test.ts` depended on schema it did not establish (RESOLVED in M15 Increment 49).** Recorded as a known defect by Increment 48 and deliberately left open there. `packages/api/test/analysis-cache-durable.integration.test.ts` never called `migrate()`: it opened pools straight onto `DATABASE_URL` and assumed `engine_analysis_cache` — created by migration `0026` and indexed by `0027` — was already there. Re-proven on current `main` before any edit, on PostgreSQL 16.14: against a genuinely fresh, never-migrated database it was **10 tests, 4 pass, 6 fail**, three of them throwing SQLSTATE **42P01** from the suite's own `LOCK TABLE`, `DELETE` and `UPDATE`, and three failing as assertions because the durable row they expected was never written. The four that passed did so vacuously: `PgAnalysisCache` absorbs a database fault and returns a miss, so a suite about durability ran with no durability at all. Against an already-migrated database it was **10/10 — and left 5 rows behind every run**, `freshFen()` being collision-avoidance rather than cleanup. The masking was measured, not assumed: the whole `packages/api` package on a fresh database gave the same **6 failures**, while running `packages/persistence` first (186 pass) and then the file gave **10/10** — seventeen persistence suites migrate the shared `DATABASE_URL`, and both the root `test` script and the CI `postgres-integration` job run that package first. **Resolved in Increment 49:** the suite applies the canonical migrations itself, once per file behind a flag, from a path anchored on its own location rather than `process.cwd()`; every test runs inside Increment 46's `withSharedDatabase` contract; each FEN is recorded before the statement that creates its row; and cleanup deletes by exact FEN equality, never by the placement prefix every standard starting position shares. A disposable database per test was considered and rejected on cost and orphan risk; the shape chosen is the one the sibling suite for the same table already uses. Acceptance is three independent brand-new empty databases (**10/10, 10/10, 10/10**, 0 rows of residue each), an already-migrated database whose five unrelated rows survive untouched, and the whole API package on a brand-new database with no other package first (**996 tests, 986 pass, 0 fail, 10 skipped**, 0 residue, 0 leaked `test_db_*`). Falsification killed **7 of 9 mutations**; the two survivors are a failure-precedence contract owned by `withSharedDatabase` and killed by its own suite, and an equivalent mutant. No production code, migration or repository semantic changed. +- **`npm run test:counts` exits non-zero because `services/gateway` sits outside the npm workspaces (OPEN, observed during M15 Increment 48).** During Increment 48 the script **exited 1**, reporting **3286 tests (28 skipped)** at the implementation HEAD and **3301 tests (28 skipped)** after that branch was synchronized with `main` — the exit code and its cause unchanged by the merge. The non-zero exit came entirely from `services/gateway`: the root `workspaces` field is `packages/*`, so that service's dependencies are never installed by a root `npm install` and its local `ioredis` module resolution fails during the counts run. No gateway dependency was installed or mutated while closing Increment 48, because that would modify `services/gateway/package-lock.json` outside the increment's scope. **M15 Increment 49 measured it again on its own branch and it is unchanged in kind: exit 1, 3256 tests (113 skipped), `gateway-service: ERROR` with six `TS2307: Cannot find module 'ioredis'` diagnostics.** **Open:** a bounded tooling/workspace investigation in its own right. The remedy is not assumed to be "add it to the workspaces" — what the service's exclusion is for, and what CI relies on, has to be established before changing it. - **`ApiServer.listen` registered no `'error'` handler, so a failed bind hung and raised an uncaught event (RESOLVED in ADR-0140 §5).** `packages/api/src/server.ts` resolved its promise from the `listening` callback only and built it with no reject path. A bind failing asynchronously (`EADDRINUSE`, `EMFILE`) left the promise pending forever and, with no `'error'` listener on the `http.Server`, was re-raised as an uncaught exception. Found while investigating ADR-0140 and independently raised by the Qodo review of PR #21. **Resolved in the same increment** rather than deferred, because ADR-0140 §2's bounded, diagnosable acquisition is not true without it — the retry can only report a bind error if the listener it is handed rejects. A one-shot `'error'` listener now rejects and is removed once listening, so later server errors keep their previous semantics rather than being swallowed by a `reject` on a settled promise. The regression test fails against the exact pre-fix code, through an uncaught `ERR_UNHANDLED_ERROR`. - **`main.ts`'s controller-disposal list is manual, untested, and silently incomplete when a section is added (RESOLVED in Increment 25 / ADR-0092).** `run()` in `packages/web/src/main.ts` disposes the previous route's controllers by name, and its own comment says doing so "is what makes re-bootstrapping safe" — but adding a section to `bootstrap` and forgetting to add it there compiles, passes every gate, and leaks. Increment 23 shipped exactly that omission for `LearningController` and it was caught in PR review, not by a test. `main.ts` has no test coverage of any kind, so no section's disposal is verified. A structural fix (bootstrap returning its disposables as a collection, or a type-level exhaustiveness check keyed off the result type) would make the next omission a compile error; it is a refactor across ~15 return sites and belongs in its own increment. **Resolved in Increment 25 (ADR-0092):** extracted `createLifecycle` run loop in `lifecycle.ts`, defined `BootstrappedDisposables` and `DisposableKey` driving `DISPOSABLE_TEARDOWN_MAP: Record` for compile-time exhaustiveness, normalised `.dispose()` verb across all disposables, and cascaded `GameController.stop()` to `gameSync.stop()`. - **`stepView` sends the answers to the learner (RESOLVED in Increment 29 / ADR-0095).** `packages/api/src/presenters.ts` emits `expectedSan` on a move step and `correctIndex` on a quiz step, and `GET /v1/lessons/:id/steps` is the route the learner's own lesson page calls. Increment 23 omits both from the client-side types (`packages/web/src/api/models.ts`), so the app cannot render or grade against them and a future edit that tries becomes a compile error — but the fields are still on the wire and readable in devtools. The authoring routes legitimately need them returned to the author, so the fix is a learner-scoped step view (or a caller-dependent projection), not a deletion: an API contract decision with its own ADR. Nothing rated or rewarded depends on step progress today, so this is a wart rather than a breach. **Resolved in Increment 29 (ADR-0095):** added `LearnerStepView` / `learnerStepView` in `packages/api/src/presenters.ts` omitting `expectedSan` and `correctIndex`. `GET /v1/lessons/:id/steps` and `GET /v1/steps/:id` now check course authorship via `repo.getLesson` / `repo.getCourse`, returning full `stepView` to the author and `learnerStepView` to learners and anonymous callers. Updated OpenAPI schema and web model comments. The first attempt resolved authorship with a separate `getLesson` + `getCourse` after the step read, which doubled both routes from 3 SQL queries to 6 because `listSteps` / `getStep` had already made those reads internally and discarded the course; caught in the PR #92 review and fixed by adding `getStepWithCourse` / `listStepsWithCourse` to `LearningRepository`, which return what was already loaded. Both routes now make exactly one repository call, pinned by a counting-proxy test. From daa430cec752cbc17e29a632b63d75bb38fc469a Mon Sep 17 00:00:00 2001 From: Hussein Mohamed Date: Sat, 5 Sep 2026 12:54:02 +0300 Subject: [PATCH 3/3] fix(api-test): ask the persistence package where its migrations are MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Qodo found the migrations path encoded the emitted `dist-test` layout — `join(__dirname, '../../../persistence/migrations')` is only correct one directory down from the source tree. Its stated failure mode, running the TypeScript source directly, is not reachable here: every test file, untouched ones on `main` included, fails first at ESM resolution of its extensionless relative imports. The concern was right regardless, and the answer already existed. `migrationsDir()` walks up from the persistence module itself and was added in response to an earlier path-portability finding, so the file now knows nothing about any layout. Verified both ways round: the compiled suite passes on a fresh database run from `packages/api` and from the repository root. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01CUPqvu66J5ZVyiv4nJ797r --- docs/PROJECT_STATE.md | 18 +++++++++++++++++- docs/ROADMAP.md | 2 +- ...cache-durable-ownership.integration.test.ts | 5 +++-- .../analysis-cache-durable.integration.test.ts | 17 ++++++++++------- 4 files changed, 31 insertions(+), 11 deletions(-) diff --git a/docs/PROJECT_STATE.md b/docs/PROJECT_STATE.md index 0e053d3e..40d24840 100644 --- a/docs/PROJECT_STATE.md +++ b/docs/PROJECT_STATE.md @@ -82,7 +82,8 @@ check); and against a migrated database carrying a stranger's row the table is b was found. The child is deliberately spawned from the repository root, not from `packages/api`, so a working directory the suite does not control cannot decide whether its schema gets built. -**Implementation.** `MIGRATIONS` resolves from the file's own location rather than `process.cwd()`; +**Implementation.** `MIGRATIONS` asks the persistence package for the directory it ships, through +its own `migrationsDir()`, instead of assembling a path from `process.cwd()`; `ensureMigrated(pool)` applies the canonical ledger once behind a file-scoped flag; every test now runs inside `withDatabase`, which is `withSharedDatabase({ max: 2, cleanup: deleteMintedRows })`; `freshFen()` records each identity before returning it, so a body that throws after a commit still @@ -125,6 +126,21 @@ packages, 3304 tests, 3276 pass, 0 fail, 28 skipped**, with **0** suites self-sk ran. `check:ci-parity`, `check:variant-parity`, `check:adr-claims`, `check:engine-pin-parity`, `check:observability` and `test:scripts` all exit 0; `git diff --check` clean. +**Review findings, and what they turned out to be.** Adversarial review found four things worth +acting on and two worth stating: the sentinel row in the ownership regression used the canonical +`... 0 1`, which the suite's own minting could produce roughly once in two million and would then +have deleted, so minted counters now start at a million; the lock-holding test acquired its client +outside the `try` that guarantees a shutdown; the racing test awaited two shutdowns in sequence; +and the TAP tally parser anchored on `$` without allowing the carriage return Windows puts before +it. Qodo then flagged the migrations path as encoding the emitted `dist-test` layout. Its stated +failure mode — running the TypeScript source directly — is not reachable in this repository: every +test file, including untouched ones on `main`, fails first at ESM resolution of its extensionless +relative imports, so Node never reaches the path. The underlying concern was right regardless, and +the answer already existed: `migrationsDir()` walks up from the persistence module itself and was +added in response to an earlier path-portability finding, so the file now knows nothing about any +layout. Verified both ways round — the compiled suite passes on a fresh database run from +`packages/api` and from the repository root. + ### Known limits recorded rather than fixed - The retention test calls `deleteExpired(new Date('2000-01-01'), 100)`, which is table-wide by diff --git a/docs/ROADMAP.md b/docs/ROADMAP.md index 335ee7de..57abfdaa 100644 --- a/docs/ROADMAP.md +++ b/docs/ROADMAP.md @@ -1353,7 +1353,7 @@ Debt observed during M14. Each states what is known, not what is planned; items - **Isolated-database test teardown dropped databases out from under connections that had not finished closing (RESOLVED in M15 Increment 45).** `withDatabase` in `packages/persistence/test/variant-migrations.integration.test.ts` ended its pool and then immediately ran `DROP DATABASE ... WITH (FORCE)`. `pool.end()` does not wait for its clients to close: in pg 8.22.0 `_pulseQueue` reaches the end callback in the same synchronous turn in which `_remove` filters the last client out of `_clients`, while `client.end()` has only queued the Terminate byte — instrumentation recorded **zero of four `remove` events fired at the moment `end()` resolved**. The drop could therefore still find a backend attached; `FORCE` terminated it, and the resulting `FATAL` arrived on a socket whose pool still had `idleListener` attached, which `pg` re-emitted as `pool.emit('error')` — an unhandled EventEmitter error that `node:test` attributed to whichever test was running rather than to the teardown that caused it. It surfaced as intermittent `terminating connection due to administrator command` failures in `postgres integration (persistence)` during M15 Increment 44, on a different test each run, which is the signature of a race rather than a broken assertion. The same shape existed in `packages/api/test/auth-signin-schema.integration.test.ts`, which had absorbed SQLSTATE 57P01 with a `pool.on('error', ...)` listener — a symptom fix for the same cause. **Resolved in Increment 45:** a shared `withTestDatabase` helper (`@chess-platform/persistence/test-support`) ends the pool under a bound, waits for `pg_stat_activity` to report the database unused, and drops it *without* `FORCE`. Measured on PostgreSQL 16.14, a plain drop against a still-attached backend fails with SQLSTATE 55006 and leaves that connection untouched, where `FORCE` succeeds by killing it — so the change trades a quiet, harmful success for a loud, harmless failure. FORCE remains only on the emergency path that guarantees the disposable database is still dropped once teardown has already failed — best effort, since that last drop runs inside a `catch` so it cannot bury the error being reported. The 57P01 absorber is deleted, because the corrected lifecycle never causes one. `createPool` and `migrate` are unchanged, and no migration was added. - **The persistence integration suite was not idempotent against a reused database (RESOLVED in M15 Increment 46).** Recorded as a known defect by Increment 45 and left open there. Against a fresh PostgreSQL 16 database the suite passed; a second run against the *same* database failed nine tests, deterministically — measured on 16.14 before any edit as **173 pass / 0 fail** then **164 pass / 9 fail**. CI provisions a fresh server per run, so it never surfaced there. One contract was being broken in two directions: a suite sharing `chess_test` must remove every row it created and remove nothing else. `achievements.integration.test.ts` broke the second half with an unqualified `DELETE FROM users` in `beforeEach` — `games.white_id` and `games.black_id` are the only references to `users` without `ON DELETE CASCADE` (thirty-one FKs point at `users`; twenty-nine cascade; the two that do not are both on `games`), so one game left behind by `pg.integration.test.ts` aborted the wipe with SQLSTATE 23503 before any assertion ran, and the same statement destroyed the bot accounts migration 0021 seeds, which nothing restores because `migrate` has already recorded 0021 as applied. `pg/identity-tokens.test.ts` and `tournaments.pg.integration.test.ts` broke the first half, leaving fixed primary keys behind and colliding on `users_pkey` and `tournaments_pkey` (the latter surfacing through the repository's compare-and-set as `VersionConflictError`). **Resolved in Increment 46:** suites that legitimately share the database delete exactly their own rows through `withSharedDatabase` (`packages/persistence/src/test-support/fixtures.ts`), the sibling of Increment 45's `withTestDatabase` and heir to its precedence rule — a cleanup failure never replaces the assertion that actually failed, and never disappears either. `pg.integration.test.ts` moved to disposable databases instead, because it cannot meet the cleanup half at all: it appends to `game_events`, which is append-only by production trigger, so cleaning up after itself would have meant weakening a production safety rule to suit a test. `users-batch`, `anti-cheat`, `bot-reports` and `analysis-cache` were corrected for the same contract though none of them ever failed — fresh `uuidv7()` ids meant their leaked rows could not collide, so the tables merely grew on every run. Serialization was not the fix and was not introduced: `--test-concurrency=1` is a pre-existing documented invariant and the second run failed identically under it. Acceptance is three consecutive runs against one database with no reset between them (186/186/186), after which only the three migration-seeded bot accounts remain, with no leaked disposable databases and no lingering backends; falsification killed 17 of 20 mutations. No production code, migration, checksum, constraint or repository conflict semantic changed, and no migration was added. - **The API pg-security integration suite leaked every row it created into the shared database (RESOLVED in M15 Increment 48).** Recorded as a known defect by Increment 46 and deliberately left open there. `packages/api/test/pg-security.integration.test.ts` created users, password credentials, roles, sessions and rate-limit buckets through real repositories and closed its pools without removing any of them. Because every identifier it mints is a fresh `uuidv7()`, no run collided with another, so all 11 tests passed indefinitely while the database grew — measured on PostgreSQL 16.14 before any edit as **11/11 passing and 25 rows leaked on the first run (4 users, 4 credentials, 4 roles, 4 sessions, 9 rate-limit buckets), then 11/11 again and an identical further 25 on a second run against the same database**. The failure mode was silent accumulation, not a failing test, which is why nothing in the file could see it. `rate_limit_buckets` is the sharpest case: no foreign key references it, so no cascade can ever reach those rows, and `PgRateLimiter.sweep` only evicts buckets that expired over an hour ago. **Resolved in Increment 48:** every test runs inside Increment 46's `withSharedDatabase` contract and names what it owns — users deleted by exact owned id, with the proven `ON DELETE CASCADE` foreign keys removing their credentials, roles and sessions (the only non-cascading references to `users` are `games.white_id` and `games.black_id`, and this file creates no games), and rate-limit buckets deleted by exact owned key. Identifiers are recorded before the statement that creates the row, so a body that throws after a commit still surrenders it. Prefix-matched cleanup, `TRUNCATE`, broad `DELETE`, random-identifier workarounds, retries, conflict suppression and serialization were all rejected; `./test-support/fixtures` was added to the persistence package's `exports` so the canonical helper could be reused rather than duplicated. A second defect was fixed in the same increment: the bucket-creation race test read backend PIDs before the `try` whose `finally` releases the leased client, and a pool with a client still checked out never settles `pool.end()`, so a failure there hung the file instead of reporting the error — both reads now sit inside the protected region. Acceptance is three consecutive runs against one migrated database with no reset between them (**11/11, 11/11, 11/11**), after which the database state is identical to its pre-run reading, migration seeds and unrelated sentinels are preserved, no `test_db_*` databases are leaked and no backends linger; falsification killed **8 of 9 mutations**, the survivor being the `backendPid` error path, which needs fault injection no passing suite performs. No production code, migration, constraint, foreign key or repository semantic changed. -- **`analysis-cache-durable.integration.test.ts` depended on schema it did not establish (RESOLVED in M15 Increment 49).** Recorded as a known defect by Increment 48 and deliberately left open there. `packages/api/test/analysis-cache-durable.integration.test.ts` never called `migrate()`: it opened pools straight onto `DATABASE_URL` and assumed `engine_analysis_cache` — created by migration `0026` and indexed by `0027` — was already there. Re-proven on current `main` before any edit, on PostgreSQL 16.14: against a genuinely fresh, never-migrated database it was **10 tests, 4 pass, 6 fail**, three of them throwing SQLSTATE **42P01** from the suite's own `LOCK TABLE`, `DELETE` and `UPDATE`, and three failing as assertions because the durable row they expected was never written. The four that passed did so vacuously: `PgAnalysisCache` absorbs a database fault and returns a miss, so a suite about durability ran with no durability at all. Against an already-migrated database it was **10/10 — and left 5 rows behind every run**, `freshFen()` being collision-avoidance rather than cleanup. The masking was measured, not assumed: the whole `packages/api` package on a fresh database gave the same **6 failures**, while running `packages/persistence` first (186 pass) and then the file gave **10/10** — seventeen persistence suites migrate the shared `DATABASE_URL`, and both the root `test` script and the CI `postgres-integration` job run that package first. **Resolved in Increment 49:** the suite applies the canonical migrations itself, once per file behind a flag, from a path anchored on its own location rather than `process.cwd()`; every test runs inside Increment 46's `withSharedDatabase` contract; each FEN is recorded before the statement that creates its row; and cleanup deletes by exact FEN equality, never by the placement prefix every standard starting position shares. A disposable database per test was considered and rejected on cost and orphan risk; the shape chosen is the one the sibling suite for the same table already uses. Acceptance is three independent brand-new empty databases (**10/10, 10/10, 10/10**, 0 rows of residue each), an already-migrated database whose five unrelated rows survive untouched, and the whole API package on a brand-new database with no other package first (**996 tests, 986 pass, 0 fail, 10 skipped**, 0 residue, 0 leaked `test_db_*`). Falsification killed **7 of 9 mutations**; the two survivors are a failure-precedence contract owned by `withSharedDatabase` and killed by its own suite, and an equivalent mutant. No production code, migration or repository semantic changed. +- **`analysis-cache-durable.integration.test.ts` depended on schema it did not establish (RESOLVED in M15 Increment 49).** Recorded as a known defect by Increment 48 and deliberately left open there. `packages/api/test/analysis-cache-durable.integration.test.ts` never called `migrate()`: it opened pools straight onto `DATABASE_URL` and assumed `engine_analysis_cache` — created by migration `0026` and indexed by `0027` — was already there. Re-proven on current `main` before any edit, on PostgreSQL 16.14: against a genuinely fresh, never-migrated database it was **10 tests, 4 pass, 6 fail**, three of them throwing SQLSTATE **42P01** from the suite's own `LOCK TABLE`, `DELETE` and `UPDATE`, and three failing as assertions because the durable row they expected was never written. The four that passed did so vacuously: `PgAnalysisCache` absorbs a database fault and returns a miss, so a suite about durability ran with no durability at all. Against an already-migrated database it was **10/10 — and left 5 rows behind every run**, `freshFen()` being collision-avoidance rather than cleanup. The masking was measured, not assumed: the whole `packages/api` package on a fresh database gave the same **6 failures**, while running `packages/persistence` first (186 pass) and then the file gave **10/10** — seventeen persistence suites migrate the shared `DATABASE_URL`, and both the root `test` script and the CI `postgres-integration` job run that package first. **Resolved in Increment 49:** the suite applies the canonical migrations itself, once per file behind a flag, asking the persistence package for the directory it ships (`migrationsDir()`) instead of assembling one from `process.cwd()`; every test runs inside Increment 46's `withSharedDatabase` contract; each FEN is recorded before the statement that creates its row; and cleanup deletes by exact FEN equality, never by the placement prefix every standard starting position shares. A disposable database per test was considered and rejected on cost and orphan risk; the shape chosen is the one the sibling suite for the same table already uses. Acceptance is three independent brand-new empty databases (**10/10, 10/10, 10/10**, 0 rows of residue each), an already-migrated database whose five unrelated rows survive untouched, and the whole API package on a brand-new database with no other package first (**996 tests, 986 pass, 0 fail, 10 skipped**, 0 residue, 0 leaked `test_db_*`). Falsification killed **7 of 9 mutations**; the two survivors are a failure-precedence contract owned by `withSharedDatabase` and killed by its own suite, and an equivalent mutant. No production code, migration or repository semantic changed. - **`npm run test:counts` exits non-zero because `services/gateway` sits outside the npm workspaces (OPEN, observed during M15 Increment 48).** During Increment 48 the script **exited 1**, reporting **3286 tests (28 skipped)** at the implementation HEAD and **3301 tests (28 skipped)** after that branch was synchronized with `main` — the exit code and its cause unchanged by the merge. The non-zero exit came entirely from `services/gateway`: the root `workspaces` field is `packages/*`, so that service's dependencies are never installed by a root `npm install` and its local `ioredis` module resolution fails during the counts run. No gateway dependency was installed or mutated while closing Increment 48, because that would modify `services/gateway/package-lock.json` outside the increment's scope. **M15 Increment 49 measured it again on its own branch and it is unchanged in kind: exit 1, 3256 tests (113 skipped), `gateway-service: ERROR` with six `TS2307: Cannot find module 'ioredis'` diagnostics.** **Open:** a bounded tooling/workspace investigation in its own right. The remedy is not assumed to be "add it to the workspaces" — what the service's exclusion is for, and what CI relies on, has to be established before changing it. - **`ApiServer.listen` registered no `'error'` handler, so a failed bind hung and raised an uncaught event (RESOLVED in ADR-0140 §5).** `packages/api/src/server.ts` resolved its promise from the `listening` callback only and built it with no reject path. A bind failing asynchronously (`EADDRINUSE`, `EMFILE`) left the promise pending forever and, with no `'error'` listener on the `http.Server`, was re-raised as an uncaught exception. Found while investigating ADR-0140 and independently raised by the Qodo review of PR #21. **Resolved in the same increment** rather than deferred, because ADR-0140 §2's bounded, diagnosable acquisition is not true without it — the retry can only report a bind error if the listener it is handed rejects. A one-shot `'error'` listener now rejects and is removed once listening, so later server errors keep their previous semantics rather than being swallowed by a `reject` on a settled promise. The regression test fails against the exact pre-fix code, through an uncaught `ERR_UNHANDLED_ERROR`. - **`main.ts`'s controller-disposal list is manual, untested, and silently incomplete when a section is added (RESOLVED in Increment 25 / ADR-0092).** `run()` in `packages/web/src/main.ts` disposes the previous route's controllers by name, and its own comment says doing so "is what makes re-bootstrapping safe" — but adding a section to `bootstrap` and forgetting to add it there compiles, passes every gate, and leaks. Increment 23 shipped exactly that omission for `LearningController` and it was caught in PR review, not by a test. `main.ts` has no test coverage of any kind, so no section's disposal is verified. A structural fix (bootstrap returning its disposables as a collection, or a type-level exhaustiveness check keyed off the result type) would make the next omission a compile error; it is a refactor across ~15 return sites and belongs in its own increment. **Resolved in Increment 25 (ADR-0092):** extracted `createLifecycle` run loop in `lifecycle.ts`, defined `BootstrappedDisposables` and `DisposableKey` driving `DISPOSABLE_TEARDOWN_MAP: Record` for compile-time exhaustiveness, normalised `.dispose()` verb across all disposables, and cascaded `GameController.stop()` to `gameSync.stop()`. diff --git a/packages/api/test/analysis-cache-durable-ownership.integration.test.ts b/packages/api/test/analysis-cache-durable-ownership.integration.test.ts index be27845c..77d82484 100644 --- a/packages/api/test/analysis-cache-durable-ownership.integration.test.ts +++ b/packages/api/test/analysis-cache-durable-ownership.integration.test.ts @@ -23,7 +23,7 @@ import { test } from 'node:test'; import { execFile } from 'node:child_process'; import { join } from 'node:path'; import { promisify } from 'node:util'; -import { migrate } from '@chess-platform/persistence/pg'; +import { migrate, migrationsDir } from '@chess-platform/persistence/pg'; import { withTestDatabase } from '@chess-platform/persistence/test-support'; import type { Pool } from 'pg'; @@ -31,7 +31,8 @@ const DATABASE_URL = process.env['DATABASE_URL']; const skip = DATABASE_URL ? false : 'DATABASE_URL not set'; const SUITE = join(__dirname, 'analysis-cache-durable.integration.test.js'); -const MIGRATIONS = join(__dirname, '../../../persistence/migrations'); +/** Resolved by the package that ships them, so it is right whatever the working directory is. */ +const MIGRATIONS = migrationsDir(); /** Deliberately not `packages/api` — see `runSuiteAgainst`. */ const REPO_ROOT = join(__dirname, '..', '..', '..', '..'); diff --git a/packages/api/test/analysis-cache-durable.integration.test.ts b/packages/api/test/analysis-cache-durable.integration.test.ts index 3c278c06..b3958180 100644 --- a/packages/api/test/analysis-cache-durable.integration.test.ts +++ b/packages/api/test/analysis-cache-durable.integration.test.ts @@ -18,7 +18,6 @@ import test, { after } from 'node:test'; import assert from 'node:assert/strict'; import { randomInt } from 'node:crypto'; -import { join } from 'node:path'; import type { Pool } from 'pg'; import { EngineManager, @@ -26,7 +25,7 @@ import { stockfishPlugin, type EngineResult, } from '@chess-platform/engine'; -import { migrate, PgAnalysisCache } from '@chess-platform/persistence/pg'; +import { migrate, migrationsDir, PgAnalysisCache } from '@chess-platform/persistence/pg'; import { withSharedDatabase } from '@chess-platform/persistence/test-support/fixtures'; import { createAnalysisCacheComposition } from '../src/analysis/durable-cache'; import type { AnalysisCacheComposition } from '../src/analysis/composition'; @@ -48,12 +47,16 @@ const INFO = 'info depth 10 seldepth 12 nodes 12345 nps 50000 time 200 score cp /** * The canonical ledger, from the package that owns it — never a schema this file invents. * - * Anchored on this file's own location rather than `process.cwd()`, which several suites here still - * use: the working directory is the package root under `npm test` and the repository root under an - * IDE runner or a hand-written `node --test packages/api/...`, and a suite that can only find its - * schema from one of those has swapped one hidden precondition for another. + * Asked for by that package rather than assembled here from `process.cwd()`, which several suites + * still do: the working directory is the package root under `npm test` and the repository root + * under an IDE runner or a hand-written `node --test packages/api/...`, and a suite that can only + * find its schema from one of those has swapped one hidden precondition for another. A relative + * path from this file would have the same shape of problem one level down, since it would have to + * know whether it was running as source or as the emitted `dist-test` copy. `migrationsDir` walks + * up from the persistence module itself, so it is right in every layout and this file has to know + * nothing about any of them. */ -const MIGRATIONS = join(__dirname, '../../../persistence/migrations'); +const MIGRATIONS = migrationsDir(); /** * Every FEN minted so far and not yet cleaned up, which is this file's claim on the table.