diff --git a/docs/PROJECT_STATE.md b/docs/PROJECT_STATE.md index db8e2c78..b5776790 100644 --- a/docs/PROJECT_STATE.md +++ b/docs/PROJECT_STATE.md @@ -4,7 +4,79 @@ > to read **only this file** and continue immediately. Updated after every > milestone and every significant architectural step. -_Last updated: 2026-09-03 — M15 Increment 45: PostgreSQL isolated-database teardown race._ +_Last updated: 2026-09-04 — M15 Increment 46: reused-database idempotence in the persistence suite._ + + +## M15 Increment 46 — the persistence suite is idempotent on a reused database + +The defect Increment 45 recorded and deliberately left open is closed. Against a fresh PostgreSQL 16 +database the persistence suite passed; run a second time against the *same* database it failed nine +tests, every time, in the three families Increment 45 named. Measured here on PostgreSQL 16.14 +(`pgvector/pgvector:pg16`, matching CI) before any edit: **run 1 = 173 pass / 0 fail, run 2 on the +same database = 164 pass / 9 fail**. CI provisions a fresh server per run, which is the only reason +this never showed there. + +**One contract was being broken, in two directions.** Suites share `chess_test` because they need a +migrated schema, not a private one, and that only works while each suite removes every row it +created and removes nothing else. Each failure family is one half of that: + +| Failing tests | Where | PostgreSQL error | Why | +|---|---|---|---| +| 5 | `achievements.integration.test.ts` `beforeEach` | `23503` on `games_white_id_fkey` | it deleted rows it did not own | +| 2 | `pg/identity-tokens.test.ts` | `23505` on `users_pkey` | it never removed the rows it did own | +| 2 | `tournaments.pg.integration.test.ts` | `23505` on `tournaments_pkey`, surfaced as `VersionConflictError` | same | + +The achievements case is the one worth remembering. Its `beforeEach` ran an unqualified +`DELETE FROM users`, which is a claim to own the whole database. `games.white_id` and +`games.black_id` are the **only** references to `users` without `ON DELETE CASCADE` — verified against +the live catalogue: thirty-one foreign keys point at `users`, twenty-nine cascade, and the two that do +not are both on `games` — so a single game left behind by `pg.integration.test.ts` made the wipe abort +before any assertion ran. The same statement also destroyed the bot accounts migration 0021 seeds, and +nothing puts them back: `migrate` has already recorded 0021 as applied, so a database that had run the +suite once was permanently missing them. + +**The fix is ownership, not a bigger reset.** Suites that legitimately share the database now delete +exactly their own rows through `withSharedDatabase` in +`packages/persistence/src/test-support/fixtures.ts` — the sibling of Increment 45's +`withTestDatabase`, inheriting that increment's 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. It appends to `game_events`, which is append-only by production trigger +(`game_events_block_mutate`), so `DELETE` raises; cleaning up after itself would have meant weakening +a production safety rule to suit a test. Two of its tests also edit `schema_migrations` deliberately — +that used to rest on `--test-concurrency=1` keeping other files away from a corrupted ledger, and now +rests on nothing, because no other file can reach the database. + +`--test-concurrency=1` is an existing, documented invariant, not something introduced here, and it was +never the fix: the second run failed identically with files already serialized. + +**Also corrected, same contract, no failure of their own:** `users-batch`, `anti-cheat`, `bot-reports` +and `analysis-cache` wrote rows they never removed. Fresh `uuidv7()` ids meant they could not collide, +so nothing ever failed — the tables simply grew on every run against a database anyone reuses. Unique +keys stop the *next* run failing; they are not cleanup. + +**Verified.** Thirteen regression tests in `reused-database.integration.test.ts` pin the mechanism, each +in its own disposable database so it can make claims about what a database contains. Acceptance is +three consecutive runs against one PostgreSQL 16.14 database with no reset between them: +**186 / 186 / 186, zero failures**, after which the database holds no test rows at all — only the +three migration-seeded bot accounts — with zero leaked disposable databases and zero lingering +backends. Falsification killed **17 of 20** mutations; the survivors are named in the PR. + +No production code, migration, checksum, constraint or repository conflict semantic changed. + +**Signature B remains UNRESOLVED**, and was observed three times during this increment — on the +`search-backfill`, `learning` and `test-database` integration files. Each was a whole file failing +with a bare `'test failed'`, no assertion, no stack and none of its own tests reported: the +documented Signature B shape, now seen in **`packages/persistence` as well as `packages/api`**, which +is new information about a defect previously recorded only in the API suite. None recurred — each +file passes standalone against the exact database state it died on, and re-running the same command +was green. Nothing here touches it. + +**Found and not fixed here:** `packages/api/test/pg-security.integration.test.ts` leaks users into +the shared database (`rotate…`, `revoke…`, `race…`, `meta…` handles, minted with `uuidv7()` so they +never collide). That is the same ownership contract this increment corrected, in a package this +increment's scope did not cover — recorded rather than silently widened into. ## M15 Increment 45 — PostgreSQL isolated-database teardown race @@ -65,7 +137,9 @@ before force-dropping, because a client the callback checked out and never relea database. A second consecutive run against the same server fails — 12 tests on `b95065f`, 9 with this change — in the achievements, identity-tokens and tournaments repositories, which share `chess_test` rather than taking a database of their own. CI provisions a fresh server every run, so -it has never surfaced there. Pre-existing, and out of scope for this increment. +it has never surfaced there. Pre-existing, and out of scope for this increment. **Closed in +Increment 46**; the finding above is left as written, because it is what was true when this +increment shipped. **Signature B remains UNRESOLVED.** It is a separate defect and nothing here touches it. diff --git a/docs/ROADMAP.md b/docs/ROADMAP.md index b0ff9707..555326ac 100644 --- a/docs/ROADMAP.md +++ b/docs/ROADMAP.md @@ -1351,6 +1351,7 @@ Debt observed during M14. Each states what is known, not what is planned; items - **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. 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. - **`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. diff --git a/packages/persistence/src/test-support/fixtures.ts b/packages/persistence/src/test-support/fixtures.ts new file mode 100644 index 00000000..324ca2bc --- /dev/null +++ b/packages/persistence/src/test-support/fixtures.ts @@ -0,0 +1,202 @@ +/** + * @packageDocumentation + * Owning your own rows in a database you share. + * + * {@link ../test-support/database!withTestDatabase} answers the other half of the same question: + * a suite that needs an empty or exclusive database gets a disposable one. This file is for the + * suites that legitimately share the database `DATABASE_URL` points at — they need a migrated + * schema, not a private one — and whose only obligation is to leave it as they found it. + * + * The contract those suites must meet is narrow and was not being met: + * + * - remove every row the suite created that a later run could collide with; + * - never remove a row the suite did not create. + * + * Both halves matter. A suite that skips the first leaves its fixed primary keys behind and the + * next run fails on `users_pkey` or `tournaments_pkey`. A suite that ignores the second reaches + * for an unqualified `DELETE FROM users`, which destroys other suites' fixtures and, once any + * `games` row exists, cannot even succeed — `games.white_id` and `games.black_id` are the only + * references to `users` without `ON DELETE CASCADE`, so the wipe aborts with SQLSTATE 23503. + * + * Test-only, like its sibling: nothing under `src/pg` imports it and no production entry point + * re-exports it. + */ + +import type { Pool } from 'pg'; +import { createPool } from '../pg/pool'; + +export interface SharedDatabaseOptions { + /** Server and database to connect to. Defaults to `DATABASE_URL`, like {@link createPool}. */ + readonly connectionString?: string; + /** + * Pool size. These suites share a server with every other integration file, so the driver + * default of ten connections per suite is pressure none of them needs. + */ + readonly max?: number; + /** + * Removes exactly the rows the body created, and nothing else. + * + * It runs whether the body passed or failed, so it must tolerate rows that were never created — + * a body that failed halfway leaves a partial fixture, and a `DELETE` that matches nothing is + * the correct outcome, not an error. + */ + readonly cleanup: (pool: Pool) => Promise; +} + +/** + * Run `body` against the shared database, then remove the rows it owns. + * + * Failure precedence is the point of this helper, and it is the part that a plain `try/finally` + * gets wrong. A `finally` that awaits cleanup lets a cleanup rejection *replace* the assertion + * that actually failed, so the run reports a `DELETE` that could not run instead of the defect + * that made it necessary. Here the body's failure always wins — the value the body rejected with + * is the value rethrown, unchanged — and cleanup failing on its own is reported rather than + * swallowed, because silence there would let a suite quietly stop cleaning up and reintroduce + * exactly this defect. + * + * When both fail, the teardown error rides along on the body error's `cause` chain, at the first + * free link rather than only the first: an error that already carries a `cause` used to drop the + * teardown failure entirely. A body that rejects with a primitive has nowhere to hang a property, + * and wrapping the value or throwing an `AggregateError` to make room would change what the caller + * catches — the one thing this helper exists to keep stable. That value is rethrown exactly as it + * was, and the teardown failure goes out as a process warning instead, so the one case that cannot + * carry a cause is still not a silent one. + */ +export async function withSharedDatabase( + options: SharedDatabaseOptions, + body: (pool: Pool) => Promise, +): Promise { + // Both are safe to pass through undefined: `createPool` falls back to `DATABASE_URL`, and `pg` + // applies its own default pool size. + const pool = createPool({ connectionString: options.connectionString, max: options.max }); + + // `undefined` is a legal rejection value, so the flags carry whether something failed and the + // paired variable carries only what it failed with. + let bodyFailed = false; + let bodyError: unknown; + let teardownFailed = false; + let teardownError: unknown; + let result!: T; + + try { + result = await body(pool); + } catch (error) { + bodyFailed = true; + bodyError = error; + } + + try { + await options.cleanup(pool); + } catch (error) { + teardownFailed = true; + teardownError = error; + } + + try { + await pool.end(); + } catch (error) { + if (!teardownFailed) { + teardownFailed = true; + teardownError = error; + } + } + + if (bodyFailed) { + // Nothing between here and the throw may escape. Recording a *secondary* failure must never + // cost the primary one, and every step of it — walking the error, describing it, emitting the + // warning — touches something this helper did not create. Each of those guards its own known + // failure so the teardown error still lands where it can; this one is the backstop that makes + // "the body's failure is what surfaces" true unconditionally rather than case by case. + if (teardownFailed) { + try { + if (!tryAttachCause(bodyError, teardownError)) { + reportUnattachableTeardownFailure(teardownError); + } + } catch { + // Nowhere left to put it. The body's failure still gets out, which is the guarantee. + } + } + throw bodyError; + } + if (teardownFailed) { + throw teardownError; + } + return result; +} + +/** The `type` on the warning emitted when a teardown failure has nowhere to be attached. */ +export const UNATTACHABLE_TEARDOWN_WARNING = 'PersistenceTestTeardownFailure'; + +/** + * Hang `addition` off the first free `cause` link under `error`. Reports whether it found one. + * + * Setting only `error.cause` drops the addition whenever the primary error already has one, which + * is common: repositories wrap driver errors and keep the original as the cause. Walking to the + * end keeps both. The `seen` set is not theoretical tidiness — an error whose cause chain loops + * back on itself would otherwise spin here forever, inside teardown, with no test to blame. + */ +function tryAttachCause(error: unknown, addition: unknown): boolean { + // The whole walk is guarded, not just the write. Every step touches an object this helper did + // not create: reading `cause` can run someone else's getter, assigning it can run a setter or be + // refused by a frozen error — with a `TypeError` in strict mode, silently otherwise. None of + // those may escape, because an exception raised while *annotating* a failure would replace the + // failure, which is the one loss this helper exists to prevent. Anything unexpected is reported + // as "no free link", which routes the teardown failure to the warning path instead. + try { + const seen = new Set(); + let link = error; + while (link instanceof Error && !seen.has(link)) { + if (link.cause === undefined) { + link.cause = addition; + // Re-read rather than trust the write: a sealed error refuses it without complaining. + return Object.is(link.cause, addition); + } + seen.add(link); + link = link.cause; + } + } catch { + return false; + } + return false; +} + +/** + * Say out loud that a cleanup failed, when the failure it happened alongside cannot carry it. + * + * A warning rather than a throw, because the body's failure is the one the run must report. It + * leaves the thrown value untouched and still puts the cleanup failure — and its stack — in front + * of whoever reads the output, which is the whole difference between a documented limitation and + * a swallowed error. + */ +function reportUnattachableTeardownFailure(teardownError: unknown): void { + // Describing the failure reads properties, and `String()` runs `toString`, on an object this + // helper did not create. If even that throws there is still something worth saying, and saying + // it beats emitting nothing because the description of the problem was itself a problem. + let detail: string; + try { + detail = + teardownError instanceof Error + ? (teardownError.stack ?? `${teardownError.name}: ${teardownError.message}`) + : `cleanup rejected with a non-Error value: ${String(teardownError)}`; + } catch { + detail = 'cleanup failed, and describing the failure threw as well'; + } + process.emitWarning(detail, { + type: UNATTACHABLE_TEARDOWN_WARNING, + detail: 'the body rejected with a value that cannot carry a cause, so this could not ride along', + }); +} + +/** + * Delete these users and the rows that depend on them. + * + * Almost everything referencing `users` cascades, so deleting the user is enough — but `games` + * does not cascade, and a fixture that gave its users a game cannot be removed until the game is. + * Callers that never create games get the same answer from the second statement alone; doing both + * unconditionally means a caller does not have to know which of the two it is. + */ +export async function deleteFixtureUsers(pool: Pool, userIds: readonly string[]): Promise { + const ids = [...userIds]; + await pool.query('DELETE FROM games WHERE white_id = ANY($1::uuid[]) OR black_id = ANY($1::uuid[])', [ids]); + await pool.query('DELETE FROM users WHERE id = ANY($1::uuid[])', [ids]); +} diff --git a/packages/persistence/test/achievements.integration.test.ts b/packages/persistence/test/achievements.integration.test.ts index 987751e5..9e1179ba 100644 --- a/packages/persistence/test/achievements.integration.test.ts +++ b/packages/persistence/test/achievements.integration.test.ts @@ -7,26 +7,87 @@ import { createPool, migrate, PgAchievementsRepository } from '../src/pg/index.j const databaseUrl = process.env.DATABASE_URL; +/** + * Every user this file creates. Cleanup removes exactly these and nothing else. + * + * This suite used to open each test with an unqualified `DELETE FROM achievement_progress` and + * `DELETE FROM users`, which is a claim to own the whole shared database. It does not own it: + * those statements destroyed rows belonging to other suites, and against a database that had + * already been used they failed outright. `games.white_id` and `games.black_id` are the only + * references to `users` without `ON DELETE CASCADE`, so one game left behind by another file made + * the wipe abort with SQLSTATE 23503 in `beforeEach`, before any assertion in this file ran. + * + * Deleting this suite's own user is what it actually needed: `achievement_progress` cascades from + * `users`, so the fixture goes with it. + */ +const FIXTURE_USER_IDS = ['018f3a5b-7c9d-7000-8000-000000000001']; + +/** The bot accounts migration 0021 seeds. They belong to the schema, not to this suite. */ +const SEEDED_BOT_HANDLES = ['gambit-novice', 'gambit-club', 'gambit-master']; + describe('PgAchievementsRepository (integration)', { skip: !databaseUrl }, () => { let pool: Pool; let repo: PgAchievementsRepository; + /** Which seeded bot accounts this database actually had before the suite touched anything. */ + let seedBaseline: string[] = []; + + /** + * The seeded bot handles present right now, ordered so two readings compare directly. + * + * Read twice — once in `before` for the baseline, once per cleanup — because the question is + * whether cleanup changed the set, not how large it is. + */ + const seededBotsNow = async (): Promise => { + const { rows } = await pool.query<{ handle: string }>( + 'SELECT handle FROM users WHERE handle = ANY($1::citext[]) ORDER BY handle', + [SEEDED_BOT_HANDLES], + ); + return rows.map((row) => row.handle); + }; + + /** Remove this suite's own rows. Safe when they are already gone, so it runs before and after. */ + const deleteFixtures = async (): Promise => { + await pool.query('DELETE FROM users WHERE id = ANY($1::uuid[])', [FIXTURE_USER_IDS]); + + // The second half of the contract, checked where it would be broken. The wipe this replaced + // did not only destroy other suites' fixtures: it took the bot accounts migration 0021 seeds, + // and nothing restores them — `migrate` has recorded 0021 as applied, so the database simply + // stays without them. Widening the delete again fails here instead of silently years later. + // + // Compared against what this database had at `before`, not against all three. A database that + // already ran the old suite is *already* missing them, permanently, and demanding three here + // would fail every run on that database for a reason this suite did not cause and must not try + // to repair — restoring schema-owned rows is not cleanup's job. Holding the count steady still + // catches a widened delete, which is the whole point. + assert.deepEqual( + await seededBotsNow(), + seedBaseline, + 'cleanup must leave rows this suite did not create, including the migration seed', + ); + }; + before(async () => { pool = createPool({ connectionString: databaseUrl }); await migrate(pool, join(process.cwd(), 'migrations')); + seedBaseline = await seededBotsNow(); repo = new PgAchievementsRepository(pool); }); after(async () => { - if (pool) { + if (!pool) return; + // Leave the shared database as this suite found it, so the next run begins from the same + // preconditions this one did — but close the pool whatever that delete does. Awaiting the + // cleanup first and closing second would skip `end()` on exactly the runs where cleanup + // failed, leaving a backend attached and the worker unable to exit cleanly. + try { + await deleteFixtures(); + } finally { await pool.end(); } }); - beforeEach(async () => { - await pool.query('DELETE FROM achievement_progress'); - await pool.query('DELETE FROM users'); - }); + beforeEach(deleteFixtures); async function createTestUser(id: string, handle: string): Promise { await pool.query( diff --git a/packages/persistence/test/analysis-cache.integration.test.ts b/packages/persistence/test/analysis-cache.integration.test.ts index 7eff4462..d839aad8 100644 --- a/packages/persistence/test/analysis-cache.integration.test.ts +++ b/packages/persistence/test/analysis-cache.integration.test.ts @@ -4,15 +4,16 @@ * `ON CONFLICT DO UPDATE ... WHERE` guard — and neither can be exercised by a fake pool. * * Every test namespaces its rows with a unique fingerprint, so the file is safe to run against a - * database shared with the other integration suites and against itself. + * database shared with the other integration suites and against itself — and then removes them, + * so it is also safe to run against a database that has already been used. */ -import { describe, it } from 'node:test'; +import { after, describe, it } from 'node:test'; import * as assert from 'node:assert/strict'; import { randomUUID } from 'node:crypto'; import { join } from 'node:path'; import type { Pool } from 'pg'; import type { AnalysisKey, EngineResult } from '@chess-platform/engine'; -import { createPool } from '../src/pg/pool'; +import { withSharedDatabase } from '../src/test-support/fixtures'; import { migrate } from '../src/pg/migrate'; import { ANALYSIS_CACHE_PAYLOAD_VERSION, encodeAnalysisPayload } from '../src/analysis-cache'; import { PgAnalysisCache, type AnalysisCacheFault } from '../src/pg/analysis-cache'; @@ -50,10 +51,48 @@ function otherLine(): EngineResult { }; } +/** + * Every fingerprint this file mints, so cleanup can remove exactly the rows they own. + * + * A unique fingerprint per test is what keeps the suites from colliding, and that was mistaken for + * a cleanup strategy: unique keys mean the *next* run does not fail, not that this run left + * nothing behind. Against a database anyone reuses, `engine_analysis_cache` grew by twenty-odd + * rows every time the file ran. + */ +const mintedFingerprints: string[] = []; + +/** + * The same fingerprints, kept for the whole file rather than drained per test. + * + * `after` uses this to check the file actually left nothing behind. Without that check the cleanup + * is only assumed to work: unique fingerprints mean no later run ever collides, so a cleanup that + * quietly stopped deleting anything would go on passing indefinitely. + */ +const allFingerprints: string[] = []; + +/** Mint a fingerprint nothing else uses, and remember it for cleanup. */ +function freshFingerprint(): string { + const fingerprint = `fp-${randomUUID()}`; + mintedFingerprints.push(fingerprint); + allFingerprints.push(fingerprint); + return fingerprint; +} + +after(async () => { + if (!DATABASE_URL || allFingerprints.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 fingerprint = ANY($1::text[])', + [allFingerprints], + ); + assert.equal(left.rows[0]?.n, '0', 'every row this file wrote was removed again'); + }); +}); + /** A fresh identity per test, so suites sharing one database cannot collide. */ function freshKey(overrides: Partial = {}): AnalysisKey { return { - fingerprint: `fp-${randomUUID()}`, + fingerprint: freshFingerprint(), fen: START_FEN, variant: 'standard', multiPv: 1, @@ -72,27 +111,51 @@ function freshKey(overrides: Partial = {}): AnalysisKey { */ let migrated = false; +/** Apply the ledger the first time only; later calls on later pools are no-ops. */ async function ensureMigrated(pool: Pool): Promise { if (migrated) return; await migrate(pool, join(process.cwd(), 'migrations')); migrated = true; } +/** + * Run one test against a cache on the shared database, then take back the rows it wrote. + * + * The fault array is handed to the body so a test can assert on errors the cache reported rather + * than threw — it is collected here, outside the callback, so it survives a body that fails. + */ async function withCache( run: (cache: PgAnalysisCache, pool: Pool, faults: AnalysisCacheFault[]) => Promise, ): Promise { + const faults: AnalysisCacheFault[] = []; // A small pool per test: these suites share a server with every other integration file, and // the default of ten connections each is pressure none of them needs. - const pool = createPool({ max: 2 }); - const faults: AnalysisCacheFault[] = []; - try { + await withSharedDatabase({ max: 2, cleanup: deleteMintedRows }, async (pool) => { await ensureMigrated(pool); await run(new PgAnalysisCache(pool, { onError: (fault) => faults.push(fault) }), pool, faults); - } finally { - await pool.end(); - } + }); +} + +/** + * Remove every row keyed by a fingerprint minted so far, then forget them. + * + * The list is drained rather than read, so a test that fails partway still hands the next one an + * empty ledger instead of re-deleting rows that are already gone. + */ +async function deleteMintedRows(pool: Pool): Promise { + const fingerprints = mintedFingerprints.splice(0, mintedFingerprints.length); + if (fingerprints.length === 0) return; + await pool.query('DELETE FROM engine_analysis_cache WHERE fingerprint = ANY($1::text[])', [ + fingerprints, + ]); } +/** + * The limits actually recorded for an identity, read past the cache rather than through it. + * + * Asserts exactly one row: the identity is a composite key, so two would mean the write path had + * inserted where it should have updated, which no assertion through the cache API would show. + */ async function storedLimitsOf(pool: Pool, key: AnalysisKey): Promise> { const result = await pool.query( `SELECT achieved_depth, achieved_nodes, achieved_time_ms, payload_version @@ -191,7 +254,7 @@ describe('PgAnalysisCache identity isolation', { skip }, () => { const key = freshKey(); await cache.set(key, [line(1)], { limits: { depth: 20 } }); - const otherBuild = { ...key, fingerprint: `fp-${randomUUID()}` }; + const otherBuild = { ...key, fingerprint: freshFingerprint() }; assert.equal(await cache.get(otherBuild, { depth: 20 }), undefined); }); }); diff --git a/packages/persistence/test/anti-cheat.integration.test.ts b/packages/persistence/test/anti-cheat.integration.test.ts index 5612c8b3..d5f7152a 100644 --- a/packages/persistence/test/anti-cheat.integration.test.ts +++ b/packages/persistence/test/anti-cheat.integration.test.ts @@ -2,14 +2,35 @@ import test from 'node:test'; import assert from 'node:assert/strict'; import { join } from 'node:path'; import type { PlayerCorrelationReport, StoredPlayerReport } from '@chess-platform/anti-cheat'; -import { createPool } from '../src/pg/pool'; +import type { Pool } from 'pg'; import { migrate } from '../src/pg/migrate'; import { PgAntiCheatReportRepository } from '../src/pg/anti-cheat'; import { uuidv7 } from '../src/ids'; +import { withSharedDatabase } from '../src/test-support/fixtures'; const DATABASE_URL = process.env['DATABASE_URL']; const skip = DATABASE_URL ? false : 'DATABASE_URL not set'; +/** + * Remove the reports a test wrote. + * + * `anti_cheat_reports` references nothing, and these tests mint fresh `uuidv7()` ids, so leaving + * the rows behind never collided with anything — which is exactly why it went unnoticed while the + * table grew on every run against a database anyone reuses. Fresh ids are not a substitute for + * cleaning up; they only hide the omission. + */ +const deleteReportsForGames = + (gameIds: readonly string[]) => + async (pool: Pool): Promise => { + await pool.query('DELETE FROM anti_cheat_reports WHERE game_id = ANY($1::uuid[])', [[...gameIds]]); + }; + +/** + * A correlation report whose fields are internally consistent, varying only by suspicion band. + * + * The numbers matter less than that they round-trip: the report is stored as JSONB, so a field + * dropped or renamed by the mapping shows up as a mismatch rather than a type error. + */ function makeReport(suspicion: 'clean' | 'review' | 'high' = 'clean'): PlayerCorrelationReport { return { suspicion, @@ -30,12 +51,13 @@ function makeReport(suspicion: 'clean' | 'review' | 'high' = 'clean'): PlayerCor } test('anti-cheat reports pg repository: migrate, saveBatch, listByPlayer, and upsert', { skip }, async () => { - const pool = createPool(); - try { + const gameIds: string[] = []; + await withSharedDatabase({ cleanup: deleteReportsForGames(gameIds) }, async (pool) => { await migrate(pool, join(process.cwd(), 'migrations')); const repo = new PgAntiCheatReportRepository(pool); const gameId = uuidv7(); + gameIds.push(gameId); const whitePlayerId = uuidv7(); const blackPlayerId = uuidv7(); @@ -82,7 +104,5 @@ test('anti-cheat reports pg repository: migrate, saveBatch, listByPlayer, and up const whiteStoredUpdated = await repo.listByPlayer(whitePlayerId); assert.equal(whiteStoredUpdated.length, 1, 'upsert replaces prior record, does not duplicate'); assert.equal(whiteStoredUpdated[0]?.report.suspicion, 'high'); - } finally { - await pool.end(); - } + }); }); diff --git a/packages/persistence/test/bot-reports.integration.test.ts b/packages/persistence/test/bot-reports.integration.test.ts index c02c2bfe..cfbd7bea 100644 --- a/packages/persistence/test/bot-reports.integration.test.ts +++ b/packages/persistence/test/bot-reports.integration.test.ts @@ -2,14 +2,21 @@ import test from 'node:test'; import assert from 'node:assert/strict'; import { join } from 'node:path'; import type { BotBehaviorReport, StoredBotReport } from '@chess-platform/anti-cheat'; -import { createPool } from '../src/pg/pool'; +import type { Pool } from 'pg'; import { migrate } from '../src/pg/migrate'; import { PgBotBehaviorReportRepository } from '../src/pg/bot-reports'; import { uuidv7 } from '../src/ids'; +import { withSharedDatabase } from '../src/test-support/fixtures'; const DATABASE_URL = process.env['DATABASE_URL']; const skip = DATABASE_URL ? false : 'DATABASE_URL not set'; +/** + * A behaviour report varying only by suspicion band. + * + * Stored as JSONB, so the point of the fixed fields is that they come back unchanged — a mapping + * that dropped or renamed one would pass a type check and fail here. + */ function makeReport(suspicion: 'clean' | 'review' | 'high' = 'clean'): BotBehaviorReport { return { suspicion, @@ -25,13 +32,24 @@ function makeReport(suspicion: 'clean' | 'review' | 'high' = 'clean'): BotBehavi }; } +/** + * Remove the reports a test wrote. `bot_reports` references nothing and the ids are freshly + * minted, so the leak was silent: the table simply grew on every run against a reused database. + */ +const deleteReportsForGames = + (gameIds: readonly string[]) => + async (pool: Pool): Promise => { + await pool.query('DELETE FROM bot_reports WHERE game_id = ANY($1::uuid[])', [[...gameIds]]); + }; + test('bot reports pg repository: migrate, saveBatch, listByPlayer, and upsert', { skip }, async () => { - const pool = createPool(); - try { + const gameIds: string[] = []; + await withSharedDatabase({ cleanup: deleteReportsForGames(gameIds) }, async (pool) => { await migrate(pool, join(process.cwd(), 'migrations')); const repo = new PgBotBehaviorReportRepository(pool); const gameId = uuidv7(); + gameIds.push(gameId); const whitePlayerId = uuidv7(); const blackPlayerId = uuidv7(); @@ -78,7 +96,5 @@ test('bot reports pg repository: migrate, saveBatch, listByPlayer, and upsert', const whiteStoredUpdated = await repo.listByPlayer(whitePlayerId); assert.equal(whiteStoredUpdated.length, 1, 'upsert replaces prior record, does not duplicate'); assert.equal(whiteStoredUpdated[0]?.report.suspicion, 'high'); - } finally { - await pool.end(); - } + }); }); diff --git a/packages/persistence/test/pg.integration.test.ts b/packages/persistence/test/pg.integration.test.ts index 32947017..23d5cdb3 100644 --- a/packages/persistence/test/pg.integration.test.ts +++ b/packages/persistence/test/pg.integration.test.ts @@ -9,15 +9,33 @@ import { PostgresEventStore } from '../src/pg/event-store'; import { PgGamesRepository, PgSeeksRepository, PgSeekAcceptor, PgGameStarter, PgUsersRepository } from '../src/pg/repositories'; import { uuidv7 } from '../src/ids'; import { ConcurrencyError } from '../src/errors'; +import { withTestDatabase } from '../src/test-support/database'; // Integration tests need a real Postgres. They SKIP (not fail) when DATABASE_URL // is unset, so dependency-free suites still run everywhere (incl. CI before a DB). const DATABASE_URL = process.env['DATABASE_URL']; const skip = DATABASE_URL ? false : 'DATABASE_URL not set'; +/** + * Every test here that writes takes a disposable database of its own. + * + * This file cannot meet the obligation the other shared-database suites meet — remove what you + * created — because it appends to `game_events`, and that table is append-only by production + * trigger (`game_events_block_mutate`, migration 0001): `DELETE` raises. Cleaning up after itself + * would mean weakening a production safety rule to suit a test, so the honest alternative is to + * stop writing into a database it shares. The rows it used to leave behind were not harmless: a + * `games` row referencing one of its users is what made the achievements suite's cleanup abort + * with SQLSTATE 23503 on every reused database. + * + * Two of these tests also edit `schema_migrations` deliberately — setting a checksum the runner + * must reject, or marking an online index pending — which no other suite may observe. That used + * to rest on `--test-concurrency=1` keeping files apart. On a database nobody else can reach, it + * rests on nothing. + */ +const isolated = { connectionString: DATABASE_URL, max: 4 } as const; + test('migrations apply and are idempotent', { skip }, async () => { - const pool = createPool(); - try { + await withTestDatabase(async ({ pool }) => { const dir = join(process.cwd(), 'migrations'); await migrate(pool, dir); @@ -55,47 +73,39 @@ test('migrations apply and are idempotent', { skip }, async () => { 'SELECT state FROM schema_migrations WHERE version = 23', ); assert.equal(migration.rows[0]?.state, 'applied'); - } finally { - await pool.end(); - } + }, isolated); }); test('the ledger is portable across checkouts but still rejects edits', { skip }, async () => { - const pool = createPool(); - const dir = join(process.cwd(), 'migrations'); - const file = '0023_community_pending_join_requests_index.sql'; - const version = 23; - const canonical = migrationChecksum(readMigrationSql(dir, file)); - /** The checksum recorded for this migration, or undefined if it has no row. */ - const readChecksum = async (): Promise => - ( - await pool.query<{ checksum: string }>( - 'SELECT checksum FROM schema_migrations WHERE version = $1', - [version], - ) - ).rows[0]?.checksum; - - let ledgerMutated = false; - /** - * Overwrite this migration's recorded checksum, and remember that the ledger - * now needs restoring — cleanup keys off that flag so a failed write is not - * followed by a doomed restore that would bury the real error. - * - * This briefly leaves a checksum the runner must reject, so nothing else may - * migrate against the database meanwhile. The suite guarantees that with - * `node --test --test-concurrency=1`, which runs test files one at a time. - * Taking the runner's own advisory lock here instead would deadlock: migrate() - * acquires that same key on its own connection and would wait on this one. - */ - const setChecksum = async (checksum: string): Promise => { - await pool.query('UPDATE schema_migrations SET checksum = $2 WHERE version = $1', [ - version, - checksum, - ]); - ledgerMutated = true; - }; + await withTestDatabase(async ({ pool }) => { + const dir = join(process.cwd(), 'migrations'); + const file = '0023_community_pending_join_requests_index.sql'; + const version = 23; + const canonical = migrationChecksum(readMigrationSql(dir, file)); + /** The checksum recorded for this migration, or undefined if it has no row. */ + const readChecksum = async (): Promise => + ( + await pool.query<{ checksum: string }>( + 'SELECT checksum FROM schema_migrations WHERE version = $1', + [version], + ) + ).rows[0]?.checksum; + + /** + * Overwrite this migration's recorded checksum. + * + * This deliberately leaves a checksum the runner must reject. It used to need restoring in a + * `finally`, and a comment explaining that `--test-concurrency=1` was what kept any other file + * from migrating against the corrupted ledger meanwhile. The database is this test's own now + * and is dropped when it returns, so there is nothing to restore and nobody to protect. + */ + const setChecksum = async (checksum: string): Promise => { + await pool.query('UPDATE schema_migrations SET checksum = $2 WHERE version = $1', [ + version, + checksum, + ]); + }; - try { await migrate(pool, dir); assert.equal(await readChecksum(), canonical, 'a fresh run records the canonical checksum'); @@ -114,22 +124,11 @@ test('the ledger is portable across checkouts but still rejects edits', { skip } // An actual edit to an applied migration matches neither rendering. await setChecksum(createHash('sha256').update('edited migration', 'utf8').digest('hex')); await assert.rejects(migrate(pool, dir), /changed after being applied; history is immutable/); - } finally { - // Restore only what this test actually changed: if the first migrate() threw - // before schema_migrations existed, an UPDATE here would throw too and bury - // the real failure. Never let the restore leak the pool either — every later - // integration file migrates against this same database. - try { - if (ledgerMutated) await setChecksum(canonical); - } finally { - await pool.end(); - } - } + }, isolated); }); test('postgres event store: round-trip and optimistic concurrency', { skip }, async () => { - const pool = createPool(); - try { + await withTestDatabase(async ({ pool }) => { await migrate(pool, join(process.cwd(), 'migrations')); const store = new PostgresEventStore(pool); const gameId = uuidv7(); @@ -157,11 +156,12 @@ test('postgres event store: round-trip and optimistic concurrency', { skip }, as // A second append at a stale head is rejected. await assert.rejects(store.append(gameId, -1, events), ConcurrencyError); - } finally { - await pool.end(); - } + }, isolated); }); +// The one test in this file that writes nothing. It reads a row that cannot exist, so it needs a +// migrated schema and nothing else — and paying for a disposable database to prove a lookup +// returns null would buy nothing. test('postgres games repository treats a malformed public id as not found', { skip }, async () => { const pool = createPool(); try { @@ -174,8 +174,7 @@ test('postgres games repository treats a malformed public id as not found', { sk }); test('postgres seek acceptance: optimistic concurrency', { skip }, async () => { - const pool = createPool(); - try { + await withTestDatabase(async ({ pool }) => { await migrate(pool, join(process.cwd(), 'migrations')); const seeks = new PgSeeksRepository(pool); const acceptor = new PgSeekAcceptor(pool); @@ -310,14 +309,11 @@ test('postgres seek acceptance: optimistic concurrency', { skip }, async () => { assert.equal(finalGame.rowCount, 0); assert.equal(finalEvents.rowCount, 0); } - } finally { - await pool.end(); - } + }, isolated); }); test('PgGameStarter: creates game and handles duplicate id cleanly', { skip }, async () => { - const pool = createPool(); - try { + await withTestDatabase(async ({ pool }) => { await migrate(pool, join(process.cwd(), 'migrations')); const starter = new PgGameStarter(pool); const users = new PgUsersRepository(pool); @@ -355,8 +351,6 @@ test('PgGameStarter: creates game and handles duplicate id cleanly', { skip }, a const second = await starter.start(gameId, events, gameStart); assert.equal(second, false, 'duplicate gameId must return false without throwing'); - } finally { - await pool.end(); - } + }, isolated); }); diff --git a/packages/persistence/test/pg/identity-tokens.test.ts b/packages/persistence/test/pg/identity-tokens.test.ts index ebc7b9cf..a3cd9965 100644 --- a/packages/persistence/test/pg/identity-tokens.test.ts +++ b/packages/persistence/test/pg/identity-tokens.test.ts @@ -2,23 +2,34 @@ import { describe, it } from 'node:test'; import * as assert from 'node:assert/strict'; import { randomBytes, createHash } from 'node:crypto'; import { join } from 'node:path'; -import { createPool } from '../../src/pg/pool'; import { migrate } from '../../src/pg/migrate'; import { PgIdentityTokensRepository, PgUsersRepository } from '../../src/pg/repositories'; +import { deleteFixtureUsers, withSharedDatabase } from '../../src/test-support/fixtures'; const DATABASE_URL = process.env['DATABASE_URL']; const skip = DATABASE_URL ? false : 'DATABASE_URL not set'; +/** + * These ids are fixed on purpose — they keep the fixtures readable — but a fixed primary key is + * only safe while the suite removes it again. Neither test used to, so a second run against the + * same database re-inserted them and died on `users_pkey` (SQLSTATE 23505) before reaching an + * assertion. The tokens themselves need no cleanup of their own: `identity_tokens.user_id` + * cascades from `users`. + */ +const TOKEN_USER_ID = '01918300-0000-0000-0000-000000000000'; +const RACE_USER_ID = '01918300-0000-0000-0000-000000000001'; + describe('PgIdentityTokensRepository', { skip }, () => { it('creates and atomically consumes a token', async () => { - const pool = createPool(); - try { + await withSharedDatabase({ + cleanup: (pool) => deleteFixtureUsers(pool, [TOKEN_USER_ID]), + }, async (pool) => { await migrate(pool, join(process.cwd(), 'migrations')); const users = new PgUsersRepository(pool); const tokens = new PgIdentityTokensRepository(pool); const user = await users.create({ - id: '01918300-0000-0000-0000-000000000000', + id: TOKEN_USER_ID, handle: 'tokenuser', email: 'tokenuser@example.com', emailHash: createHash('sha256').update('tokenuser@example.com').digest(), @@ -82,19 +93,18 @@ describe('PgIdentityTokensRepository', { skip }, () => { tokens.consume(replacement, 'email_verify', new Date()), )); assert.equal(consumedReplacements.filter(Boolean).length, 1); - } finally { - await pool.end(); - } + }); }); it('serializes email verification against replacement issuance', async () => { - const pool = createPool(); - try { + await withSharedDatabase({ + cleanup: (pool) => deleteFixtureUsers(pool, [RACE_USER_ID]), + }, async (pool) => { await migrate(pool, join(process.cwd(), 'migrations')); const users = new PgUsersRepository(pool); const tokens = new PgIdentityTokensRepository(pool); const user = await users.create({ - id: '01918300-0000-0000-0000-000000000001', + id: RACE_USER_ID, handle: 'verificationrace', email: 'verificationrace@example.com', emailHash: createHash('sha256').update('verificationrace@example.com').digest(), @@ -134,8 +144,6 @@ describe('PgIdentityTokensRepository', { skip }, () => { userId: user.id, expiresAt, }, new Date()), null, 'verified users cannot receive another verification token'); - } finally { - await pool.end(); - } + }); }); }); diff --git a/packages/persistence/test/reused-database.integration.test.ts b/packages/persistence/test/reused-database.integration.test.ts new file mode 100644 index 00000000..92fa72fc --- /dev/null +++ b/packages/persistence/test/reused-database.integration.test.ts @@ -0,0 +1,441 @@ +/** + * The contract that makes the persistence integration suite runnable twice. + * + * Every suite here shares one database. That is deliberate — they need a migrated schema, not a + * private one — and it only works while each suite obeys two rules: + * + * 1. remove every row it created that a later run could collide with; + * 2. never remove a row it did not create. + * + * Both were being broken, and the second run of the suite failed nine tests as a result. The + * acceptance proof for that is the whole suite run three times against one database; what this + * file pins is the *mechanism*, so the contract cannot be quietly dropped again without a test + * saying so. + * + * Each test runs inside its own disposable database, so this file can make claims about what a + * database contains — counts, absences, seed rows — that would be meaningless against a database + * shared with twenty other suites. + */ + +import test from 'node:test'; +import assert from 'node:assert/strict'; +import { join } from 'node:path'; +import type { Pool } from 'pg'; +import { migrate } from '../src/pg/migrate'; +import { PgUsersRepository } from '../src/pg/repositories'; +import { uuidv7 } from '../src/ids'; +import { withTestDatabase } from '../src/test-support/database'; +import { + deleteFixtureUsers, + UNATTACHABLE_TEARDOWN_WARNING, + withSharedDatabase, +} from '../src/test-support/fixtures'; + +const DATABASE_URL = process.env['DATABASE_URL']; +const skip = DATABASE_URL ? false : 'DATABASE_URL not set'; + +const isolated = { connectionString: DATABASE_URL, max: 4 } as const; + +/** A fixed id, like the ones the real suites use — the whole point is that reuse must be safe. */ +const FIXED_USER_ID = '01918300-0000-0000-0000-0000000000ff'; +const FIXED_HANDLE = 'reuse-fixture'; + +/** The bot accounts migration 0021 seeds. They belong to the schema, not to any suite. */ +const SEEDED_BOT_HANDLES = ['gambit-novice', 'gambit-club', 'gambit-master']; + +/** + * Bring a disposable database up to the same schema the shared one has. + * + * The same call every real suite makes on its way in, so what these tests exercise is the schema + * as shipped — constraints, triggers and seed rows included — rather than a convenient subset. + */ +async function migrated(pool: Pool): Promise { + await migrate(pool, join(process.cwd(), 'migrations')); +} + +/** + * How many rows carry this id — 0 or 1, since it is the primary key. + * + * Counted rather than selected so an absent row is `0` instead of something falsy that an + * assertion could confuse with a row that exists. `-1` is unreachable: `count(*)` always returns a + * row, and seeing it would mean the query itself was wrong rather than the database empty. + */ +async function countUsers(pool: Pool, id: string): Promise { + const { rows } = await pool.query<{ n: string }>( + 'SELECT count(*)::text AS n FROM users WHERE id = $1', + [id], + ); + return Number(rows[0]?.n ?? '-1'); +} + +/** Insert a game between two users, the one child of `users` that does not cascade. */ +async function insertGame(pool: Pool, whiteId: string, blackId: string): Promise { + const gameId = uuidv7(); + await pool.query( + `INSERT INTO games (id, variant, rated, speed, white_id, black_id, started_at) + VALUES ($1, 'standard', false, 'blitz', $2, $3, now())`, + [gameId, whiteId, blackId], + ); + return gameId; +} + +test('a suite that cleans up hands the next run the preconditions it needs', { skip }, async () => { + await withTestDatabase(async ({ pool }) => { + await migrated(pool); + const users = new PgUsersRepository(pool); + + // Twice, with no reset in between. The second pass is the one that used to fail: the first + // left a row on a fixed primary key and nothing removed it, so `users.create` hit + // `users_pkey` (SQLSTATE 23505) before any assertion ran. + for (const pass of [1, 2]) { + await users.create({ id: FIXED_USER_ID, handle: FIXED_HANDLE }); + assert.equal(await countUsers(pool, FIXED_USER_ID), 1, `pass ${pass} created its fixture`); + await deleteFixtureUsers(pool, [FIXED_USER_ID]); + assert.equal(await countUsers(pool, FIXED_USER_ID), 0, `pass ${pass} removed its fixture`); + } + }, isolated); +}); + +test('cleanup removes a fixture that owns a game, which does not cascade', { skip }, async () => { + await withTestDatabase(async ({ pool }) => { + await migrated(pool); + const users = new PgUsersRepository(pool); + const white = await users.create({ id: uuidv7(), handle: `fk-white-${uuidv7().slice(0, 8)}` }); + const black = await users.create({ id: uuidv7(), handle: `fk-black-${uuidv7().slice(0, 8)}` }); + await insertGame(pool, white.id, black.id); + + // The unqualified form fails here, and this is exactly the failure the achievements suite hit + // on every reused database: `games.white_id` references `users` with no ON DELETE clause. + await assert.rejects( + pool.query('DELETE FROM users WHERE id = $1', [white.id]), + (error: unknown) => { + assert.equal((error as { code?: string }).code, '23503'); + assert.equal((error as { constraint?: string }).constraint, 'games_white_id_fkey'); + return true; + }, + 'a plain user delete must still be refused while a game references it', + ); + + // Ordering the delete is the whole job: the game goes first, then the users. + await deleteFixtureUsers(pool, [white.id, black.id]); + assert.equal(await countUsers(pool, white.id), 0); + assert.equal(await countUsers(pool, black.id), 0); + }, isolated); +}); + +test('cleanup takes only its own rows, and leaves the schema seed alone', { skip }, async () => { + await withTestDatabase(async ({ pool }) => { + await migrated(pool); + const users = new PgUsersRepository(pool); + const mine = await users.create({ id: uuidv7(), handle: `mine-${uuidv7().slice(0, 8)}` }); + const theirs = await users.create({ id: uuidv7(), handle: `theirs-${uuidv7().slice(0, 8)}` }); + const otherGame = await insertGame(pool, theirs.id, theirs.id); + + await deleteFixtureUsers(pool, [mine.id]); + + assert.equal(await countUsers(pool, mine.id), 0, 'its own row is gone'); + assert.equal(await countUsers(pool, theirs.id), 1, "another suite's user is untouched"); + const survivingGame = await pool.query('SELECT id FROM games WHERE id = $1', [otherGame]); + assert.equal(survivingGame.rowCount, 1, "another suite's game is untouched"); + + // The unqualified `DELETE FROM users` this replaced did not only destroy other suites' rows. + // It destroyed the bot accounts migration 0021 seeds, and nothing puts them back: `migrate` + // has already recorded 0021 as applied, so the database stayed missing them for good. + const seeded = await pool.query<{ handle: string }>( + 'SELECT handle FROM users WHERE handle = ANY($1::citext[]) ORDER BY handle', + [SEEDED_BOT_HANDLES], + ); + assert.equal(seeded.rowCount, SEEDED_BOT_HANDLES.length, 'migration seed rows survive a suite'); + }, isolated); +}); + +test('a fixed handle from an earlier run cannot poison the next one', { skip }, async () => { + await withTestDatabase(async ({ pool }) => { + await migrated(pool); + const users = new PgUsersRepository(pool); + + // A different id but the same handle is the other half of the collision: `users.handle` is + // UNIQUE, so cleanup that removed the row by id only would still leave the handle taken. + await users.create({ id: FIXED_USER_ID, handle: FIXED_HANDLE }); + await deleteFixtureUsers(pool, [FIXED_USER_ID]); + const reused = await users.create({ id: uuidv7(), handle: FIXED_HANDLE }); + assert.equal(reused.handle, FIXED_HANDLE, 'the handle is free again'); + }, isolated); +}); + +test('cleanup runs even when the body threw, and does not replace its error', { skip }, async () => { + await withTestDatabase(async ({ pool: owned, connectionString }) => { + await migrated(owned); + const shared = { connectionString, max: 2 }; + const userId = FIXED_USER_ID; + let cleaned = 0; + + const boom = new Error('the assertion that actually failed'); + await assert.rejects( + withSharedDatabase({ + ...shared, + cleanup: async (pool) => { + cleaned += 1; + await deleteFixtureUsers(pool, [userId]); + }, + }, async (pool) => { + await new PgUsersRepository(pool).create({ id: userId, handle: FIXED_HANDLE }); + throw boom; + }), + (error: unknown) => { + assert.equal(error, boom, 'the body error is what surfaces, not a teardown error'); + return true; + }, + ); + assert.equal(cleaned, 1, 'cleanup ran despite the failure'); + + // And the half-built fixture really is gone, so the next run starts where this one did. + assert.equal(await countUsers(owned, userId), 0, 'the failed run kept no residue'); + }, isolated); +}); + +test('when the body and the cleanup both fail, the body error still wins', { skip }, async () => { + await withTestDatabase(async ({ connectionString }) => { + // The test above only ever had cleanup succeed, so it could not tell the precedence rule from + // its absence — swapping the two throws left it passing. This is the case that separates them. + const boom = new Error('the assertion that actually failed'); + const cleanupFailure = new Error('and cleanup could not run either'); + + await assert.rejects( + withSharedDatabase({ + connectionString, + max: 2, + cleanup: () => Promise.reject(cleanupFailure), + }, () => Promise.reject(boom)), + (error: unknown) => { + assert.equal(error, boom, 'losing the real failure to a teardown error is the worse trade'); + assert.equal((error as Error).cause, cleanupFailure, 'the teardown failure is kept as cause'); + return true; + }, + ); + }, isolated); +}); + +test('a body error that already has a cause still carries the teardown failure', { skip }, async () => { + await withTestDatabase(async ({ connectionString }) => { + // Repositories wrap driver errors and keep the original as `cause`, so a body failing inside + // one arrives here with that link already taken. Writing only to `error.cause` dropped the + // teardown failure in exactly that case — the common one, not an edge. + const original = new Error('the driver error underneath'); + const boom = new Error('the assertion that actually failed', { cause: original }); + const cleanupFailure = new Error('and cleanup could not run either'); + + await assert.rejects( + withSharedDatabase({ + connectionString, + max: 2, + cleanup: () => Promise.reject(cleanupFailure), + }, () => Promise.reject(boom)), + (error: unknown) => { + assert.equal(error, boom, 'the body error is still what surfaces'); + assert.equal((error as Error).cause, original, 'and keeps the cause it arrived with'); + assert.equal(original.cause, cleanupFailure, 'the teardown failure took the next free link'); + return true; + }, + ); + }, isolated); +}); + +test('a teardown failure with nowhere to attach is warned about, not dropped', { skip }, async () => { + await withTestDatabase(async ({ connectionString }) => { + // A primitive has nowhere to hang a cause, so the teardown failure cannot ride along. What + // must not change is the value the caller catches: wrapping it to make room would break every + // `assert.rejects` predicate that compares identity. So it goes out as a warning instead — + // the difference between a limitation and a swallowed error. + const warnings: Error[] = []; + const collect = (warning: Error): void => { + warnings.push(warning); + }; + process.on('warning', collect); + try { + await assert.rejects( + withSharedDatabase({ + connectionString, + max: 2, + cleanup: () => Promise.reject(new Error('cleanup could not run')), + }, () => Promise.reject('a bare string')), + (error: unknown) => { + assert.equal(error, 'a bare string', 'the rejected value is rethrown untouched'); + return true; + }, + ); + // `emitWarning` defers to `process.nextTick`, which runs before any `setImmediate` — so this + // is an ordering guarantee rather than a wait, and the assertion below cannot race it. + await new Promise((resolve) => setImmediate(resolve)); + } finally { + process.off('warning', collect); + } + + const reported = warnings.filter((w) => w.name === UNATTACHABLE_TEARDOWN_WARNING); + assert.equal(reported.length, 1, 'the cleanup failure was reported exactly once'); + assert.match(reported[0]?.message ?? '', /cleanup could not run/); + }, isolated); +}); + +test('a frozen body error is rethrown, not replaced by the attempt to annotate it', { skip }, async () => { + await withTestDatabase(async ({ connectionString }) => { + // Writing `cause` on a frozen error throws a TypeError in strict mode. Escaping, that TypeError + // would replace the body failure with a complaint about a property assignment — the exact loss + // this helper exists to prevent, introduced by the code that was meant to preserve more. + const boom = Object.freeze(new Error('the assertion that actually failed')); + const cleanupFailure = new Error('and cleanup could not run either'); + + const warnings: Error[] = []; + const collect = (warning: Error): void => { + warnings.push(warning); + }; + process.on('warning', collect); + try { + await assert.rejects( + withSharedDatabase({ + connectionString, + max: 2, + cleanup: () => Promise.reject(cleanupFailure), + }, () => Promise.reject(boom)), + (error: unknown) => { + assert.equal(error, boom, 'the body error survives an error that cannot be annotated'); + return true; + }, + ); + await new Promise((resolve) => setImmediate(resolve)); + } finally { + process.off('warning', collect); + } + + // And the teardown failure is not lost just because it had nowhere to go. + const reported = warnings.filter((w) => w.name === UNATTACHABLE_TEARDOWN_WARNING); + assert.equal(reported.length, 1, 'the cleanup failure fell through to the warning path'); + }, isolated); +}); + +test('a body error whose cause getter throws is still the error that surfaces', { skip }, async () => { + await withTestDatabase(async ({ connectionString }) => { + // Walking the chain reads `cause` on an object this helper did not create, so the read itself + // can run someone else's getter. Frozen errors were the write half of the same problem; this + // is the read half, and it escaped the guard that only wrapped the assignment. + const boom = new Error('the assertion that actually failed'); + Object.defineProperty(boom, 'cause', { + configurable: true, + get() { + throw new Error('reading the cause blew up'); + }, + }); + const cleanupFailure = new Error('and cleanup could not run either'); + + const warnings: Error[] = []; + const collect = (warning: Error): void => { + warnings.push(warning); + }; + process.on('warning', collect); + try { + await assert.rejects( + withSharedDatabase({ + connectionString, + max: 2, + cleanup: () => Promise.reject(cleanupFailure), + }, () => Promise.reject(boom)), + (error: unknown) => { + assert.equal(error, boom, 'annotating must never replace the failure being annotated'); + return true; + }, + ); + await new Promise((resolve) => setImmediate(resolve)); + } finally { + process.off('warning', collect); + } + + const reported = warnings.filter((w) => w.name === UNATTACHABLE_TEARDOWN_WARNING); + assert.equal(reported.length, 1, 'the cleanup failure fell through to the warning path'); + }, isolated); +}); + +test('a teardown failure that cannot even be described still loses to the body error', { skip }, async () => { + await withTestDatabase(async ({ connectionString }) => { + // The last place an exception could still be raised while recording a secondary failure: + // describing it. Reading `stack` runs a getter on someone else's object, and this one throws. + const cleanupFailure = new Error('cleanup could not run'); + Object.defineProperty(cleanupFailure, 'stack', { + configurable: true, + get() { + throw new Error('and reading its stack blew up too'); + }, + }); + + const warnings: Error[] = []; + const collect = (warning: Error): void => { + warnings.push(warning); + }; + process.on('warning', collect); + try { + await assert.rejects( + withSharedDatabase({ + connectionString, + max: 2, + cleanup: () => Promise.reject(cleanupFailure), + // A primitive, so the failure has to go down the describe-and-warn path. + }, () => Promise.reject('a bare string')), + (error: unknown) => { + assert.equal(error, 'a bare string', 'the body failure survives all of this'); + return true; + }, + ); + await new Promise((resolve) => setImmediate(resolve)); + } finally { + process.off('warning', collect); + } + + // It still says something, rather than emitting nothing because the description failed. + const reported = warnings.filter((w) => w.name === UNATTACHABLE_TEARDOWN_WARNING); + assert.equal(reported.length, 1, 'the cleanup failure was still reported'); + assert.match(reported[0]?.message ?? '', /describing the failure threw/); + }, isolated); +}); + +test('a cleanup that fails is reported rather than swallowed', { skip }, async () => { + await withTestDatabase(async ({ pool, connectionString }) => { + await migrated(pool); + const failure = new Error('cleanup could not run'); + await assert.rejects( + withSharedDatabase({ + connectionString, + max: 2, + cleanup: () => Promise.reject(failure), + }, async () => undefined), + (error: unknown) => { + assert.equal(error, failure, 'a passing body must not hide a broken cleanup'); + return true; + }, + ); + }, isolated); +}); + +test('re-migrating a used database leaves the ledger byte-for-byte alone', { skip }, async () => { + await withTestDatabase(async ({ pool }) => { + const dir = join(process.cwd(), 'migrations'); + const ledger = async (): Promise => { + const { rows } = await pool.query<{ row: string }>( + `SELECT version || ':' || checksum || ':' || state AS row + FROM schema_migrations ORDER BY version`, + ); + return rows.map((r) => r.row).join('\n'); + }; + + assert.ok((await migrate(pool, dir)) > 0, 'the first run applies the ledger'); + const afterFirst = await ledger(); + assert.ok(afterFirst.length > 0, 'the ledger recorded something to compare against'); + + // Nearly every suite calls `migrate()` on the way in, so on a reused database it runs many + // times over. `pg.integration.test.ts` already pins that it applies nothing; what matters + // here is that it also *rewrites* nothing — a re-run that restamped checksums or flipped a + // state would corrupt the ledger for every suite that migrated after it. + assert.equal(await migrate(pool, dir), 0, 're-running applies nothing'); + assert.equal(await ledger(), afterFirst, 'and records nothing new'); + assert.equal(await migrate(pool, dir), 0, 'still nothing the third time'); + assert.equal(await ledger(), afterFirst, 'the ledger is unchanged after repeated runs'); + }, isolated); +}); diff --git a/packages/persistence/test/tournaments.pg.integration.test.ts b/packages/persistence/test/tournaments.pg.integration.test.ts index 9f59f388..4cb066b1 100644 --- a/packages/persistence/test/tournaments.pg.integration.test.ts +++ b/packages/persistence/test/tournaments.pg.integration.test.ts @@ -1,6 +1,7 @@ import test from 'node:test'; import assert from 'node:assert/strict'; import { join } from 'node:path'; +import type { Pool } from 'pg'; import { createPool } from '../src/pg/pool'; import { migrate } from '../src/pg/migrate'; import { PgTournamentsRepository } from '../src/pg/repositories'; @@ -9,10 +10,26 @@ import { Tournament } from '@chess-platform/tournament'; import { createPairingStrategy } from '@chess-platform/tournament'; import type { TournamentConfig } from '@chess-platform/tournament'; import { VersionConflictError } from '../src/errors'; +import { withSharedDatabase } from '../src/test-support/fixtures'; const DATABASE_URL = process.env['DATABASE_URL']; const skip = DATABASE_URL ? false : 'DATABASE_URL not set'; +/** + * Remove exactly the tournaments a test created. Nothing references `tournaments`, so there is no + * ordering to respect here — only the obligation to do it at all. + * + * `save(snapshot, 0)` means "create this": expected version 0 says the row does not exist yet, and + * the repository turns the resulting `tournaments_pkey` violation into a `VersionConflictError`. + * These ids are fixed, so leaving the rows behind made the *second* run of this file report a + * version conflict on a tournament nobody was concurrently updating. + */ +const deleteTournaments = + (ids: readonly string[]) => + async (pool: Pool): Promise => { + await pool.query('DELETE FROM tournaments WHERE id = ANY($1::text[])', [[...ids]]); + }; + test('tournaments repository: migrations apply and are idempotent', { skip }, async () => { const pool = createPool(); try { @@ -25,8 +42,7 @@ test('tournaments repository: migrations apply and are idempotent', { skip }, as }); test('tournaments repository: round-trip a round-robin mid-flight snapshot', { skip }, async () => { - const pool = createPool(); - try { + await withSharedDatabase({ cleanup: deleteTournaments(['t-rr-test']) }, async (pool) => { await migrate(pool, join(process.cwd(), 'migrations')); const repo = new PgTournamentsRepository(pool); @@ -85,14 +101,13 @@ test('tournaments repository: round-trip a round-robin mid-flight snapshot', { s assert.equal(summary.format, 'round_robin'); assert.equal(summary.state, 'finished'); assert.equal(summary.participantCount, 2); - } finally { - await pool.end(); - } + }); }); test('tournaments repository: round-trip a swiss mid-flight snapshot', { skip }, async () => { - const pool = createPool(); - try { + await withSharedDatabase({ + cleanup: deleteTournaments(['t-swiss-test', 't-swiss-test-2']), + }, async (pool) => { await migrate(pool, join(process.cwd(), 'migrations')); const repo = new PgTournamentsRepository(pool); @@ -152,15 +167,11 @@ test('tournaments repository: round-trip a swiss mid-flight snapshot', { skip }, assert.ok(i1 !== -1 && i2 !== -1); // created_at is only set on INSERT, so t2 is newer assert.ok(i2 < i1, 't2 should be newer than t1'); - - } finally { - await pool.end(); - } + }); }); test('tournaments repository: pre-migration rows (no explicit version) stay updatable', { skip }, async () => { - const pool = createPool(); - try { + await withSharedDatabase({ cleanup: deleteTournaments(['t-pre-migration']) }, async (pool) => { await migrate(pool, join(process.cwd(), 'migrations')); const repo = new PgTournamentsRepository(pool); @@ -196,7 +207,5 @@ test('tournaments repository: pre-migration rows (no explicit version) stay upda const after = await repo.findById(config.id); assert.equal(after!.snapshot.state, 'running'); assert.equal(after!.version, 2); - } finally { - await pool.end(); - } + }); }); diff --git a/packages/persistence/test/users-batch.integration.test.ts b/packages/persistence/test/users-batch.integration.test.ts index b5aaf0d0..dc3aa062 100644 --- a/packages/persistence/test/users-batch.integration.test.ts +++ b/packages/persistence/test/users-batch.integration.test.ts @@ -11,23 +11,29 @@ import test from 'node:test'; import assert from 'node:assert/strict'; import { join } from 'node:path'; -import { createPool } from '../src/pg/pool'; import { migrate } from '../src/pg/migrate'; import { PgUsersRepository } from '../src/pg/repositories'; import { uuidv7 } from '../src/ids'; +import { deleteFixtureUsers, withSharedDatabase } from '../src/test-support/fixtures'; const DATABASE_URL = process.env['DATABASE_URL']; const skip = DATABASE_URL ? false : 'DATABASE_URL not set'; test('users pg repository: findByIds batches, drops unknown ids, and survives malformed ones', { skip }, async () => { - const pool = createPool(); - try { + // Filled as rows are created, so cleanup removes exactly what this test made — including the + // partial fixture a failure halfway through would otherwise leave in the shared database. + const createdUserIds: string[] = []; + await withSharedDatabase({ + cleanup: (pool) => deleteFixtureUsers(pool, createdUserIds), + }, async (pool) => { await migrate(pool, join(process.cwd(), 'migrations')); const repo = new PgUsersRepository(pool); const suffix = uuidv7().slice(0, 8); const alice = await repo.create({ id: uuidv7(), handle: `batch-alice-${suffix}` }); + createdUserIds.push(alice.id); const bob = await repo.create({ id: uuidv7(), handle: `batch-bob-${suffix}` }); + createdUserIds.push(bob.id); const both = await repo.findByIds([alice.id, bob.id]); assert.equal(both.length, 2); @@ -53,7 +59,18 @@ test('users pg repository: findByIds batches, drops unknown ids, and survives ma // The batched read agrees with the single-key read it replaces. const single = await repo.findById(alice.id); assert.deepEqual(withMissing[0], single); - } finally { - await pool.end(); - } + }); + + // Checked from outside, after cleanup has run and its pool is closed. + // + // These ids are freshly minted, so leaving them behind never made a later run *fail* — the table + // just grew every time anyone ran the suite against a database they reuse. Nothing inside the + // run can notice that, which is precisely why the check has to be an explicit one. + await withSharedDatabase({ cleanup: async () => undefined }, async (pool) => { + const remaining = await pool.query( + 'SELECT count(*)::text AS n FROM users WHERE id = ANY($1::uuid[])', + [createdUserIds], + ); + assert.equal(remaining.rows[0]?.n, '0', 'the suite removed the users it created'); + }); });