fix(api-test): enforce shared database ownership in pg-security integration tests

[edwardnewgate710]

M15 Increment 48 — the API PostgreSQL security integration suite left every row it created in the shared database. This gives it ownership of that state and proves the ownership holds.

Original starting origin/main: 90211916fbdaca290e994fc7d556e7f320e4ddd4.
main merged during this branch's continuation: df8e5105bae97ecc752cabf96b3e5d26af8c0359 (PR #35 — Signature B correlator hardening). Brought in by a normal merge, no rebase and no force; zero conflicts, and no implementation-file overlap.
Final HEAD after documentation synchronization: ba88113af4ac90c54003efba0bf1b7c4f04d87dd.

Changed files against current main (5): packages/api/test/pg-security.integration.test.ts, packages/api/test/pg-security-ownership.integration.test.ts, packages/persistence/package.json, docs/PROJECT_STATE.md, docs/ROADMAP.md.

---

The defect, reproduced before anything was edited

PostgreSQL 16.14 (pgvector/pgvector:pg16), Node v24.15.0, pg 8.22.0, dedicated database, DATABASE_URL set (no run was accepted without it).

	users	credentials	roles	sessions	rate_limit_buckets	result
before	3 (0021 seeds)	0	0	0	0	—
after run 1	7 (+4)	4 (+4)	4 (+4)	4 (+4)	9 (+9)	11/11 pass
after run 2	11 (+4)	8 (+4)	8 (+4)	8 (+4)	18 (+9)	11/11 pass

25 rows per run, accumulating linearly, with the suite green forever. That is the whole defect: not a failure, a leak. Every identifier the file mints is a fresh uuidv7(), so a second run never collides and no assertion in the file can see the residue. Sentinel rows belonging to a notional other suite were planted before run 1 and were still present after run 2 — the file destroys nothing, it only accumulates.

An independent read-only audit (delegation 1) derived the same 4/4/4/4/9 = 25 from the source without seeing these numbers.

Ownership ledger

test	creates	table	identifier	FK behaviour	cleanup before	residue	boundary now
registration race	1 user (of 2 attempted) + password + role	users, credentials, roles	uuidv7(), handle Race<suffix>	children ON DELETE CASCADE (0001_init.sql:91,127)	none	3 rows	delete both minted ids
refresh rotation	user + 2 sessions (original + rotation child)	users, credentials, roles, sessions	uuidv7()	sessions.user_id cascade (0001_init.sql:111); rotated_from is not an FK (0001_init.sql:115)	none	5 rows	delete user id
session revocation	user + 1 session	same	uuidv7()	cascade	none	4 rows	delete user id
session metadata	user + 1 session	same	uuidv7()	cascade	none	4 rows	delete user id
shared/atomic limiter	1 bucket	rate_limit_buckets	integration:<uuid>	no FK, no cascade reaches it	none	1 row	delete exact key
multi-bucket all-or-nothing	2 buckets	rate_limit_buckets	integration:full|roomy:<run>	none	none	2 rows	exact keys
longest wait	2 buckets	rate_limit_buckets	integration:a-short|z-long:<run>	none	none	2 rows	exact keys
refusal counter	1 bucket	rate_limit_buckets	integration:refusal:<uuid>	none	none	1 row	exact key
bucket-creation race	1 bucket, inserted by the test's own statement, not the limiter	rate_limit_buckets	integration:create-race:<uuid>	none	none	1 row	exact key
duplicate key	none (refusal writes nothing — asserted)	—	integration:dup:<uuid>	—	none	0 rows	key still declared
deadlock-free combined	2 buckets	rate_limit_buckets	integration:auser|zip:<run>	none	none	2 rows	exact keys

Residue classes found: users; password credentials; roles; sessions (including a rotation child); rate-limit buckets. Non-row resources were audited too and were already sound — every pool closed and every transaction settled — with one exception noted below.

Root cause

The file violates the shared-database ownership contract: it creates uniquely identified rows through real repositories and closes its connection pools without deleting any of them. UUID uniqueness hides the defect by preventing collisions while residue accumulates. There is one mechanism, not several — every leaked row traces to the same missing cleanup, and nothing in the file ever issued a DELETE.

rate_limit_buckets deserves its own sentence because it has no owner in the graph: no foreign key references it, so no cascade can ever reach it, and PgRateLimiter.sweep only evicts buckets that expired over an hour ago (and only once per thousand admissions). The only thing that removes such a row is naming it.

Chosen boundary, and what was rejected

Scoped shared-database cleanup through Increment 46's withSharedDatabase, not disposable databases. Evidence: no test here mutates schema, touches the append-only game_events, tampers with schema_migrations, or asserts on an unscoped count — the conditions that forced withTestDatabase elsewhere. Disposable databases would mean 31 migrations per test, 11 times over, for rows a keyed DELETE removes in milliseconds.

Reusing the existing helper rather than writing a second one keeps a single failure-precedence contract in the monorepo. Reaching it required exporting ./test-support/fixtures from the persistence package; withSharedDatabase was otherwise unreachable outside that package, while ./test-support (for withTestDatabase) is already consumed by packages/api.

Rejected, and why:

• TRUNCATE, DELETE FROM users, DELETE FROM rate_limit_buckets — claims ownership of a database this suite does not own. Increment 46 closed exactly this bug; repeating it would destroy the 0021 bot seeds permanently, since migrate has already recorded 0021 as applied.
• LIKE 'integration:%' — the prefix is a naming convention, not an ownership claim. A sentinel bucket using that prefix is planted in the regression precisely to fail this.
• Randomising identifiers / new handle prefixes / ON CONFLICT DO NOTHING — these prevent collisions, which was never the problem. Silent growth is.
• Serialising the suite — not a cleanup strategy, and the concurrency is the thing under test.

No production semantics changed. No migration, constraint, FK, unique index or repository behaviour was touched.

A second defect found by the adversarial pass

The bucket-creation race test read two backend pids between admin.connect() and the try whose finally releases the client. A failure in either read leaked the lease — and pool.end() never settles while a client is checked out (verified directly: it had not resolved after 3s). The file would hang instead of reporting the error that caused it. The two reads now sit inside the try.

Concurrency preserved

packages/api already runs node --test --test-concurrency=1; that is a pre-existing repository invariant, not something this change adds or relies on. The cleanup is safe independently of it: every predicate names identifiers minted in that test, so it cannot reach another file's rows at any concurrency. Nothing was serialised, no assertion moved out of a concurrent path, no race was pre-empted, and no real-server behaviour was replaced by a fake. Pool sizes were deliberately left at the driver default so the 10- and 8-way concurrent admissions still overlap genuinely.

Regression coverage

packages/api/test/pg-security-ownership.integration.test.ts (3 tests). The first runs the real compiled suite as a child process against a disposable database and compares row identity — not counts — before and after, because the defect cannot be seen from inside a suite whose every assertion passes on a polluted database. Sentinels standing in for another suite (including a bucket sharing the integration: prefix) must survive untouched. The remaining two prove cleanup runs when the body throws with the body's own error still surfacing, and that cleanup takes the keys it owns and not the ones that merely look like them.

Proven to fail before the fix: against the pre-fix suite it reported the exact expected/actual diff — 8 users vs 4, 5/5/5 credentials/roles/sessions vs 1, 10 buckets vs 1. After the fix, 3/3 pass.

One harness detail worth recording: the child must not inherit NODE_TEST_CONTEXT. A child that does believes it is a nested runner, declines to execute the file, and exits 0 having produced nothing — which would have made this test pass for the worst possible reason.

Repeated-database acceptance

One migrated PostgreSQL 16 database, sentinels planted, no reset between runs:

run	result	users	credentials	roles	sessions	buckets
pre	—	4	1	1	1	1
A	11/11 pass	4	1	1	1	1
B	11/11 pass	4	1	1	1	1
C	11/11 pass	4	1	1	1	1

After C the survivors are exactly the three migration-seeded bots plus the four sentinel rows — including integration:sentinel-other-suite, the bucket a prefix match would have taken. 0 leaked test_db_* databases, 0 lingering backends.

Falsification

9 mutations, applied to real source, each rebuilt and run, each restored and verified byte-identical by SHA-256.

#	mutation	verdict
M1	skip user cleanup	KILLED
M2	skip bucket cleanup	KILLED
M3	broaden bucket cleanup to the integration: prefix	KILLED
M4	broaden user cleanup to every user	KILLED
M5	clean child rows instead of the parent user	KILLED
M6	run cleanup only when the body succeeded	KILLED
M7	let the teardown failure replace the body failure	KILLED by packages/persistence (see note)
M8	never record the identifier, so a throw after create orphans the row	KILLED
M9	acquire backend pids before the try that releases the client	SURVIVED

8 of 9 killed. Three notes, because the raw numbers would otherwise mislead:

• A first run reported all nine "COMPILE REJECTED". That was a harness bug — npx cannot be spawned through execFileSync on Windows, so every build failed. Reporting it as nine kills would have been a fabricated score. The harness now invokes tsc directly.
• M7 initially survived because it was judged only by the API suite, while the precedence contract it breaks belongs to packages/persistence. Re-run against that package's own suite it kills 6 tests. Not a coverage gap — a harness scoping artefact.
• M8 as first written did not model the hazard at all: it moved the recording later but still ahead of the throw, so nothing was ever unrecorded. Rewritten as the real hazard (never recorded) it is killed.
• M9 is an honest survivor. Killing it needs backendPid to fail, which requires fault injection no passing suite performs. It is hardening for an error path, and the mechanism behind it was proven separately rather than asserted.

Same-pattern audit of sibling API tests

Surveyed every API test touching PostgreSQL. pg-observer.integration.test.ts creates probe tables but already drops them in finally; analysis-cache-composition and bootstrap-analysis-cache never connect. auth-signin-schema and semantic-pipeline already carry cleanup or use disposable databases. No sibling file leaks rows — pg-security was the only one, and nothing outside it was modified.

One adjacent defect found and recorded, not fixed. packages/api/test/analysis-cache-durable.integration.test.ts never calls migrate(): it opens pools and assumes the schema is already there. Run inside the full suite it passes, because packages/persistence migrates the database first; run as npm test --workspace @chess-platform/api against a fresh database it fails 6 of 10, expecting a cached row and getting a search. Verified by running that one file against three databases — fresh: 6 fail; two previously-migrated: 0 fail. It is the mirror image of the defect this PR fixes — a suite depending on state it does not establish, rather than leaking state it does not remove — and it belongs to the same ownership family. Last touched by #19; out of scope here, recorded for a follow-up.

Validation

All re-run at the final HEAD 22ee0d7, against a fresh PostgreSQL 16 database:

npm run build ✅ · npm run lint ✅ (exit 0) · full repository npm test ✅ exit 0, 19 packages fail 0 — and zero occurrences of DATABASE_URL not set, so no database suite self-skipped. packages/api alone: 978 tests, 968 pass, 0 fail, 10 skipped (Fairy-Stockfish binary gates).

check:ci-parity ✅ · check:variant-parity ✅ · check:adr-claims ✅ · check:engine-pin-parity ✅ · check:observability ✅ · test:scripts ✅ · git diff --check clean.

test:counts reported 3286 tests (28 skipped) at the implementation HEAD, up from 3283 — exactly the +3 this branch adds, measured on both sides under identical env and with stale build output removed. After merging main it reports 3301 (28 skipped), the extra 15 being Increment 47's expanded correlator suite. It exits 1 in both readings, and that needs stating plainly rather than as a tick: services/gateway is not an npm workspace (workspaces is ["packages/*"]), so ioredis is never installed locally and its tsc step fails. Pre-existing, environmental, untouched by this diff, and CI skips that job for this PR. I did not npm install there, because that mutates services/gateway/package-lock.json outside this PR's scope.

An earlier pass of these checks reported all seven as exit 0. That reading was wrong: the exit code was taken after a pipe, so it measured tail rather than the command. The figures above are measured without a pipe.

Guards: clean-code-guard, test-guard, security-and-hardening. No credentials or connection strings committed. docs-guard N/A — see below. Impeccable N/A.

Signature B

UNRESOLVED, and untouched by this work. No Signature B occurrence during this task — every suite that ran reported its own tests, and no bare 'test failed' file-level failure appeared in the baseline, the A/B/C runs, the mutation rounds, or the full-repository validation.

Remaining known defects — all OPEN, none fixed here

• Signature B — UNRESOLVED. An open tracked defect after Increment 47's diagnostic correlator hardening. PR test(api-diagnostics): harden Signature B correlator against cross-directory false attribution and signed NTSTATUS codes #35 is now merged, so it is not "owned" by an open PR; it is simply still open. Increment 48 observed 0 occurrences, which bounds the rate and resolves nothing.
• analysis-cache-durable.integration.test.ts depends on schema it does not establish — OPEN. See the section above. A bounded follow-up for a separate PR after fix(api-test): enforce shared database ownership in pg-security integration tests #41.
• npm run test:counts exits 1 — OPEN. services/gateway sits outside the root npm workspaces (packages/*), so its ioredis resolution fails. A separate bounded tooling/workspace investigation; the remedy is not assumed to be "make gateway a workspace". Not fixed here, and no gateway dependency was installed or mutated.
• M9's error path is defensive and unproven by any passing test (above).

---

Canonical documentation — SYNCHRONIZED

PR #35 is merged, so the deferral is over and the payload is now recorded in the canonical docs rather than only here:

• docs/PROJECT_STATE.md — header moved to Increment 48; a new section inserted before Increment 47 covering the measured residue, the single root cause, the ownership boundary and what was rejected, the ./test-support/fixtures export, the leased-client fix, the red-then-green regression, A/B/C, and 8 of 9 mutations with the survivor named. Increment 47 is preserved verbatim — its 26 bounded runs with 0 captures, the four correlator defects, the three Increment 46 persistence observations, and Signature B UNRESOLVED.
• docs/ROADMAP.md — an Increment 48 entry beside Increment 46's, two new OPEN follow-up entries, and the existing Signature B entry extended with Increment 48's 0 occurrences.

Test counts are written as readings taken during Increment 48, not standing invariants, and are given both at the implementation HEAD and after the merge.

Final gate at ba88113

Validation	build ✅ · lint ✅ · full npm test exit 0, 19 packages fail 0, 0 database self-skips · API package 993 / 983 pass / 10 skipped / 0 fail on a fresh migrated database · six parity gates ✅ · git diff --check clean
test:counts	3301 tests (28 skipped), exit 1 — measured without a pipe. Same services/gateway / ioredis cause as before the merge; only the total moved.
A/B/C	11/11 · 11/11 · 11/11 on one migrated database, no reset, state identical after each
Residue	only the 3 migration seeds + sentinels survive; 0 leaked test_db_*; 0 lingering backends

NOT MERGED. The repository owner merges manually.

🤖 Generated with Claude Code

https://claude.ai/code/session_01CUPqvu66J5ZVyiv4nJ797r

Summary by CodeRabbit

• Tests

  • Added PostgreSQL security integration coverage for exact cleanup of test data, including failure scenarios and rate-limit bucket ownership.
  • Improved shared-database isolation and cleanup across concurrency, duplicate-key, and rate-limiter scenarios.
  • Added safeguards and diagnostics for database connection and locking scenarios.

• Documentation

  • Updated project status and roadmap documentation with resolved database cleanup issues and remaining test follow-ups.

• Chores

  • Exposed persistence test fixtures for supported test integrations.

[edwardnewgate710]

/review

[edwardnewgate710]

@coderabbitai full review

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Team

Run ID: 8530ed9c-9956-4021-bc44-9362b375c971

📥 Commits

Reviewing files that changed from the base of the PR and between df8e510 and ba88113.

📒 Files selected for processing (5)

• docs/PROJECT_STATE.md
• docs/ROADMAP.md
• packages/api/test/pg-security-ownership.integration.test.ts
• packages/api/test/pg-security.integration.test.ts
• packages/persistence/package.json

Included review availability: Your plan provides up to 8 included reviews per hour; 6 remain after this review.

---

📝 Walkthrough

Walkthrough

The pull request adds PostgreSQL cleanup-ownership integration tests, migrates security tests to shared fixture cleanup, improves concurrency-test resource handling, and exports persistence test fixtures.

Changes

PostgreSQL security test isolation

Layer / File(s)	Summary
Shared fixture lifecycle packages/persistence/package.json, packages/api/test/pg-security.integration.test.ts	The persistence package exports shared fixtures. Security tests record user and bucket ownership before database operations and apply exact cleanup across scenarios.
Concurrency and pool cleanup packages/api/test/pg-security.integration.test.ts	The bucket-creation race test protects holder-client cleanup, rolls back failures, awaits pending admission cleanup, closes auxiliary pools, and verifies PostgreSQL blocking.
Ownership verification suite packages/api/test/pg-security-ownership.integration.test.ts	The new suite snapshots security tables, runs the compiled suite in an isolated child process, and verifies cleanup after successful and failing bodies. It preserves unrelated buckets with the same prefix.
Project state and follow-ups docs/PROJECT_STATE.md, docs/ROADMAP.md	Project records document the resolved cleanup defect, validation results, and remaining analysis-cache, workspace, and Signature B findings.

Estimated code review effort: 4 (Complex) | ~45 minutes

Merge Risk: ⚪ Minimal · up to ba881

The updated PostgreSQL test fixtures and cleanup coverage do not introduce an actionable production or test reliability risk.

Sequence Diagram(s)

sequenceDiagram
  participant OwnershipTest
  participant ChildProcess
  participant PostgreSQL
  OwnershipTest->>PostgreSQL: Snapshot security rows and insert sentinels
  OwnershipTest->>ChildProcess: Run compiled PostgreSQL security suite
  ChildProcess->>PostgreSQL: Create and clean owned rows
  ChildProcess-->>OwnershipTest: Return test result
  OwnershipTest->>PostgreSQL: Compare snapshots and verify unrelated rows

Loading

Suggested reviewers: hessiun710, gemy07101999

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main change: enforcing shared database ownership in API PostgreSQL security integration tests.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 2 files. (3 skipped: 3 …
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch claude/api-pg-security-db-ownership

---

_{Comment @coderabbitai help to get the list of available commands.}

[qodo-code-review]

PR Summary by Qodo

Enforce fixture ownership in PostgreSQL security integration tests

🐞 Bug fix 🧪 Tests ⚙️ Configuration changes 🕐 40+ Minutes

AI Description

• Clean user and rate-limit fixtures by exact identifiers after every PostgreSQL security test.
• Preserve primary test failures while running shared-database cleanup on all exit paths.
• Add regression coverage proving owned rows disappear without deleting sentinel fixtures.

Diagram

sequenceDiagram
  actor R as Test Runner
  participant O as Ownership Test
  participant D as Disposable PostgreSQL
  participant S as Security Suite
  participant F as Fixture Helpers
  R->>O: Start ownership check
  O->>D: Migrate seed snapshot
  O->>S: Run isolated child
  S->>F: Begin owned test
  F->>D: Open shared pool
  S->>D: Create fixture rows
  F->>D: Delete exact owners
  F->>D: Close shared pool
  S-->>O: Report passing suite
  O->>D: Read final state
  D-->>O: State unchanged

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Disposable database per suite

• ➕ Provides strong isolation through whole-database teardown.
• ➕ Avoids maintaining per-table ownership ledgers.
• ➖ Requires database-creation privileges and adds setup overhead.
• ➖ Does not prove the suite can safely coexist in the intended shared database.

2. Transaction rollback per test

• ➕ Offers compact, automatic cleanup for ordinary fixture writes.
• ➕ Avoids explicit identifier tracking for transaction-local rows.
• ➖ Cross-pool concurrency and blocking tests cannot share one rollback boundary.
• ➖ Uncommitted fixtures would alter the PostgreSQL race behavior being tested.

Recommendation: Keep the PR's explicit ownership model. Exact user IDs leverage safe foreign-key cascades, exact bucket keys protect other suites, and withSharedDatabase preserves cleanup and error precedence without weakening real concurrency semantics. Disposable databases remain appropriate for the outer ownership regression, while transaction rollback is unsuitable for these multi-connection tests.

Files changed (3) +393 / -124

Bug fix (1) +174 / -124

pg-security.integration.test.tsClean explicitly owned fixtures after every security test +174/-124

Clean explicitly owned fixtures after every security test

• Wraps all eleven integration tests with 'withSharedDatabase', recording user IDs and rate-limit keys before creation so cleanup also works after partial failures. Users are removed through cascading fixture cleanup, buckets by exact key, and the bucket-race test now safely rolls back transactions, releases leased clients, and closes auxiliary pools.

packages/api/test/pg-security.integration.test.ts

Tests (1) +215 / -0

pg-security-ownership.integration.test.tsAdd end-to-end fixture ownership regression coverage +215/-0

Add end-to-end fixture ownership regression coverage

• Adds an external harness that runs the compiled PostgreSQL security suite in a child process and compares row identities before and after execution. Sentinel fixtures verify cleanup preserves foreign rows, while focused cases cover failing bodies, original-error propagation, and exact-key bucket deletion.

packages/api/test/pg-security-ownership.integration.test.ts

Other (1) +4 / -0

package.jsonExport shared fixture helpers through a test-support subpath +4/-0

Export shared fixture helpers through a test-support subpath

• Adds the '@chess-platform/persistence/test-support/fixtures' package export so API tests can consume the shared-database and fixture cleanup helpers without exposing them through a production entry point.

packages/persistence/package.json

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)

Great, no issues found!

Qodo reviewed your code and found no material issues that require review

Tip of the day

💡 Did you know, you can keep summaries lean with Finding overflow, which tucks the rest behind 'View more'

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Previous reviews

Review updated until commit ba88113 ⚖️ Balanced

Results up to commit 04dbd51 🧠 Deep

No changes from previous review

Results up to commit 22ee0d7 🚀 Fast

No changes from previous review

[coderabbitai]

⚠️ Action not completed

Head commit changed.

[edwardnewgate710]

/review

[edwardnewgate710]

@coderabbitai full review

[qodo-code-review]

Code review by qodo was updated up to the latest commit 22ee0d7

[coderabbitai]

✅ Action performed

Full review finished.

[edwardnewgate710]

/review

[edwardnewgate710]

@coderabbitai full review

[qodo-code-review]

Code review by qodo was updated up to the latest commit ba88113

[coderabbitai]

✅ Action performed

Full review finished.