test(db): failing-first tests for the RAII fixes - #61
Conversation
…once - `reserve()`'s handle runs direct only while the pin is held; a statement issued after `release()` takes its own connection out of the pool instead of landing inside whichever unit of work holds that connection now - `release()` is idempotent on both drivers — `withTransaction`'s `finally` and disposal are two owners on one exit path - `DbConnection extends Disposable`; `[Symbol.dispose]` is `release()` itself, so `using connection = await client.reserve()` is the shape Co-Authored-By: Claude <noreply@anthropic.com>
- `PostgresClient.close()` dropped its driver handle after the await, so a teardown that threw left the dead pool cached; the next `connect()` handed it back and no second `close()` could clear it - read-then-clear, matching `pglite.ts`: the rejection still reaches the caller, the client is empty either way, and a `connect()` racing the teardown opens a fresh pool - 4 tests, two of which fail without the fix; CHANGELOG + db/CLAUDE.md Co-Authored-By: Claude <noreply@anthropic.com>
- withTransaction reserved a connection, ran BEGIN above its `try`, and released in that block's `finally` — so a rejecting BEGIN leaked the reservation forever. On PGlite that is the single session's turn, and every later statement in the process waits on it. - The pin is now held by a `using` declaration and BEGIN sits inside the guarded scope. readOnlyQuery, which already had the right shape, is converted to the same declaration so both sites read alike. - ROLLBACK TO SAVEPOINT is best-effort, matching the root's ROLLBACK: a dead connection no longer replaces the error that caused the rollback. SAVEPOINT and RELEASE stay uncaught, and the reason is written down. - 8 tests; 2 of them fail without this change. Co-Authored-By: Claude <noreply@anthropic.com>
- `withAdvisoryLock` reserves a connection (`using`), locks on it, and hands that session to its callee: `pg_advisory_lock` is session-scoped, so a lock taken on the pool was unlocked on a different connection (answers false, held until the backend dies) and its idle holder could be closed by the pool's idle timeout mid-migration. `ROLE=migrate` masked it with `max: 1` - ledger, audit and every migration transaction now run on the locked session — also the only thing that works on a `max: 1` pool - `rollback()` took no lock at all; it takes the same one, with the same `lock: false` escape hatch for a private branch database - 6 tests over a pin-observable pool; 4 fail without the fix Co-Authored-By: Claude <noreply@anthropic.com>
- Turn (pglite-turns.ts) gains release()/[Symbol.dispose], matching DbConnection's shape; TurnQueue.run() holds its turn with using instead of a hand-rolled try/finally. - pglite.ts's reserve() calls turn.release() where it used to call turn() directly, since its turn outlives the function and can't be scoped with using. - CHANGELOG + packages/db/CLAUDE.md updated in lockstep. Co-Authored-By: Claude <noreply@anthropic.com>
- New packages/db/src/type-pins.ts asserts DbConnection extends Disposable and Turn extends Disposable at compile time - Regression on either interface is now a typecheck failure, not a silently-degraded `using` guard Co-Authored-By: Claude <noreply@anthropic.com>
- transaction.test.ts: nesting three deep still takes and releases exactly one pin — a second reserve() per SAVEPOINT level would leak a connection the root's `using` never sees. - pglite.test.ts: a failed BEGIN gives the turn back, so the next statement runs instead of queuing behind a reservation nobody can reach. - migrate.live.test.ts (new): against a real Postgres — two concurrent migrate() calls serialize (one applies, the other skips, never a unique-violation race), and the lock releases after a failed migration so the next migrate() finishes instead of hanging on a lock left stuck. Verified both new live cases fail against the pre-fix migrate.ts (advisory lock taken on a pooled, not pinned, handle). Co-Authored-By: Claude <noreply@anthropic.com>
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 47 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThe database package now uses disposable connection and turn lifecycles, guarded transaction cleanup, and session-pinned advisory locks for migrations and rollbacks. Tests and documentation cover cleanup, failure handling, concurrent migrations, and lock configuration. ChangesDatabase resource lifecycle
Session-pinned migration locking
Estimated code review effort: 4 (Complex) | ~45 minutes Mergeability Score: 🟡 Moderate · up to The PR adds regression coverage and serializes concurrent migrations, but a held migration lock can still leave a deployment waiting indefinitely without a bounded failure or recovery path. Merge should wait for a lock timeout/error solution or explicit owner acceptance; the remaining findings are limited to test, documentation, and maintainability cleanup. Sequence Diagram(s)sequenceDiagram
participant Deployment
participant migrate
participant DbConnection
participant PostgreSQL
Deployment->>migrate: start migration
migrate->>DbConnection: reserve pinned session
DbConnection->>PostgreSQL: acquire advisory lock
migrate->>PostgreSQL: run migration transaction
migrate->>PostgreSQL: release advisory lock
migrate->>DbConnection: dispose reservation
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/db/src/migrate.ts (2)
137-178: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
migrate.tsnow needs "and" to describe it.The file applies migrations, keeps the ledger honest, and owns session-pinned advisory locking.
packages/db/CLAUDE.mdstates the rule: "Files | < 200 LOC, one responsibility,kebab-case.ts, test beside source". This file is roughly 265 lines after the change.Move
withAdvisoryLock,MIGRATION_LOCK_KEY, and the lock's doc comment intopackages/db/src/migration-lock.tswith a test beside it.migrate.tsthen imports one named function, and the lock gets its own failing-first test file rather than sharingmigrate.test.ts.As per coding guidelines: "Files | < 200 LOC, one responsibility,
kebab-case.ts, test beside source".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/db/src/migrate.ts` around lines 137 - 178, Extract MIGRATION_LOCK_KEY, withAdvisoryLock, and its documentation into a new migration-lock.ts module, preserving the existing session-pinning and unlock behavior. Update migrate.ts to import and call the named lock function rather than defining it locally. Add a colocated migration-lock.test.ts covering the lock behavior, including failure cleanup, instead of extending migrate.test.ts.Source: Coding guidelines
154-178: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
pg_advisory_lockblocks forever, and a hang is the one failure an agent cannot read.Line 166 takes the lock with no bound. If another session holds it — a wedged migrator, a paused deploy, a psql window someone left open — this call waits with no output, no code, and no fix. The live test at
packages/db/src/migrate.live.test.tslines 80-83 documents that exact shape: the caller "blocks until that connection's idle timeout fires". Axiom 4 wants a stableX_*code and a runnable fix. An indefinite wait produces neither.Set
lock_timeouton the pinned session before taking the lock, then convert the timeout into a codedDbErrorwith a fix command.🔒️ Proposed bound on the lock wait
const session: DbClient = pinned ?? client; - await session.execute(sql`select pg_advisory_lock(${MIGRATION_LOCK_KEY})`); + // Bounded on purpose: an unbounded `pg_advisory_lock` turns "another migrator is running" into a + // deploy that hangs with no code and no fix — unreadable to the agent driving `x db migrate`. + await session.execute(raw(`SET lock_timeout = ${MIGRATION_LOCK_TIMEOUT_MS}`)); + try { + await session.execute(sql`select pg_advisory_lock(${MIGRATION_LOCK_KEY})`); + } catch (error) { + throw migrationLockBusy( + `another session has held the migration lock for more than ${MIGRATION_LOCK_TIMEOUT_MS}ms`, + 'x db status --json # find the running migrator, then retry once it finishes', + error, + ); + } try {
migrationLockBusybelongs inpackages/db/src/errors.tsbesidemigrationConflict, with its ownX_*code.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/db/src/migrate.ts` around lines 154 - 178, Update withAdvisoryLock to set a bounded lock_timeout on the selected session before pg_advisory_lock, using the existing migration error patterns. Catch a lock-timeout failure and throw the new migrationLockBusy DbError defined beside migrationConflict in errors.ts, including its unique X_* code and runnable fix command; preserve normal lock acquisition and cleanup behavior for other outcomes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/db/src/pglite-turns.test.ts`:
- Line 88: Replace the bare errors thrown in
packages/db/src/pglite-turns.test.ts:88,
packages/db/src/transaction.test.ts:277, and
packages/db/src/transaction.test.ts:311 with dbUnavailable() or another
UltimateError subclass carrying a stable X_* code, cause, and exact fix command.
Preserve the existing assertions that verify cleanup retains the original typed
failure.
In `@packages/db/src/transaction.test.ts`:
- Around line 15-52: Extract the shared PinCounts and reservableOver reservation
fixture into one reusable db test fixture module, preserving the existing
reservation counting and idempotent release behavior. In
packages/db/src/transaction.test.ts lines 15-52 and
packages/db/src/readonly-query.test.ts lines 12-48, remove the local fixture
definitions and import the shared reservableOver fixture instead.
In `@wiki/Entities-And-Migrations.md`:
- Line 167: Update the Errors table in wiki/Entities-And-Migrations.md to remove
the active migration-failure entry for X_MIGRATE_CONCURRENT or explicitly mark
it as reserved, matching the status described in the pre-deploy row and
wiki/Error-Codes.md. Keep the error documentation consistent and avoid
presenting this code as thrown.
---
Outside diff comments:
In `@packages/db/src/migrate.ts`:
- Around line 137-178: Extract MIGRATION_LOCK_KEY, withAdvisoryLock, and its
documentation into a new migration-lock.ts module, preserving the existing
session-pinning and unlock behavior. Update migrate.ts to import and call the
named lock function rather than defining it locally. Add a colocated
migration-lock.test.ts covering the lock behavior, including failure cleanup,
instead of extending migrate.test.ts.
- Around line 154-178: Update withAdvisoryLock to set a bounded lock_timeout on
the selected session before pg_advisory_lock, using the existing migration error
patterns. Catch a lock-timeout failure and throw the new migrationLockBusy
DbError defined beside migrationConflict in errors.ts, including its unique X_*
code and runnable fix command; preserve normal lock acquisition and cleanup
behavior for other outcomes.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: ASSERTIVE
Plan: Pro
Run ID: be057d59-464c-4aaa-9e65-65b102c836ec
📒 Files selected for processing (21)
CHANGELOG.mdpackages/db/CLAUDE.mdpackages/db/src/client.test.tspackages/db/src/client.tspackages/db/src/migrate.live.test.tspackages/db/src/migrate.test.tspackages/db/src/migrate.tspackages/db/src/pglite-turns.test.tspackages/db/src/pglite-turns.tspackages/db/src/pglite.test.tspackages/db/src/pglite.tspackages/db/src/readonly-query.test.tspackages/db/src/readonly-query.tspackages/db/src/transaction.test.tspackages/db/src/transaction.tspackages/db/src/type-pins.tswiki/CLI-Reference.mdwiki/Deployment.mdwiki/Entities-And-Migrations.mdwiki/Error-Codes.mdwiki/Known-Gaps.md
- extract PinCounts/reservableOver into fake-reservable.ts; transaction and readonly-query tests import it instead of each keeping a copy - drop the X_MIGRATE_CONCURRENT row from Entities-And-Migrations' Errors table and rewrite Troubleshooting's: the code is reserved and never thrown, so a table of thrown codes must not list it and the symptom a reader actually sees is a migrate that waits - packages/db/CLAUDE.md: name which throws are typed and which are the caller's arbitrary failure, and point tests at the shared fixture Co-Authored-By: Claude <noreply@anthropic.com>
|
Looks ready — CI is green, CodeRabbit approved, and this is a routine patch with regression tests. Ready to merge when you're ready. 🤖 Posted by developerz.ai — the maintainer agent, not a human. |
1 participant
Summary
transaction.test.ts: nesting three deep still takes and releases exactly one pin (a secondreserve()per SAVEPOINT level would leak).pglite.test.ts: a failed BEGIN gives the turn back — the next statement runs instead of queuing behind an unreachable reservation.migrate.live.test.tsagainst a real Postgres: two concurrentmigrate()calls serialize (one applies, the other skips, never a unique-violation race), and the advisory lock releases after a failed migration so the nextmigrate()finishes instead of hanging.migrate.ts(lock taken on a pooled, not pinned, handle) before restoring the fix — genuinely failing-first.Test plan
bun test packages/db(149 pass incl. live, against a local Postgres)bun run typecheckbun run verify(12/17, 5 honest skips — repo baseline unchanged)🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by CodeRabbit
New Features
Bug Fixes
Documentation