fix(adapter-pg): release the pool client at most once per transaction - #29954
fix(adapter-pg): release the pool client at most once per transaction#29954ianduvall wants to merge 3 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthrough
ChangesPostgreSQL transaction settlement
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Thanks for this — the mechanism write-up and the linked issue made it easy to follow. One process ask before we review: could you retarget this at It should be a clean move — One thing you've probably noticed: CI hasn't run here, and that's on us rather than on your change. Workflow runs on this branch currently fail to start before they reach the approval step. The fix for that landed on Your DCO sign-off and CLA are both already in order, so the retarget is the only thing outstanding. |
46d2b35 to
3d22d7a
Compare
|
@wmadden-electric done, thanks! |
When an interactive transaction timeout expires while COMMIT is in flight, the query engine settles the transaction twice: the abandoned commit chain and the compensating rollback both reach the adapter. The second client.release() lands on a client pg-pool has already re-lent, silently corrupting the pool accounting until the new owner releases and pg throws an uncatchable double-release error inside its socket-data handler, killing the process. PgTransaction now settles at most once: the second commit/rollback is a no-op, queries after settlement reject with TransactionAlreadyClosed without dispatching SQL, and a client with statements still in flight at settlement is released with an error so the pool destroys the connection instead of re-lending a busy one. Fixes prisma#29952 Claude-Session: https://claude.ai/code/session_01U3QGpkQTcYEAGGn5PVy6WG Signed-off-by: Ian Duvall <ian@omni.co>
3d22d7a to
f0714a3
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/adapter-pg/src/__tests__/pg.test.ts`:
- Line 197: Update the five new test descriptions in the pg test suite to remove
the leading “should” wording, while preserving the rest of each description and
its intended behavior.
- Around line 173-195: Update the test setup around setup to ensure every newly
created pg.Pool is closed after each test, either by returning a teardown
function that invokes pool.end() and calling it from each test or by registering
pools for centralized afterEach cleanup. Preserve the existing adapter and
release-tracking behavior while preventing pools from remaining active after
tests complete.
In `@packages/adapter-pg/src/pg.ts`:
- Around line 172-185: Update settle() so the client release logic always
executes even when this.cleanup?.() throws: wrap cleanup invocation in a
try/finally and keep both in-flight and normal release paths inside the finally
block.
- Around line 147-161: Update the performIO override return type from
pg.QueryArrayResult<any> to pg.QueryArrayResult<unknown[]> while preserving the
existing transaction and in-flight query behavior.
🪄 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 Plus
Run ID: e033b821-4f19-4f90-80be-86e5ab268c93
📒 Files selected for processing (2)
packages/adapter-pg/src/__tests__/pg.test.tspackages/adapter-pg/src/pg.ts
Each PgTransaction settlement test creates its own pg.Pool through setup() but never closed it, leaking live pools past the point where afterEach restores the mocked pg.Client methods. Track the pools setup() creates and end them in afterEach before restoring mocks. Claude-Session: https://claude.ai/code/session_013ZRubEFkLdS4givbjSKXBJ Signed-off-by: Ian Duvall <ian@omni.co>
settle() exists to guarantee an at-most-once release of the pooled client, but it invoked the cleanup callback before the release, so a throwing callback could skip the release entirely. Wrap the callback in try/finally so the release is unconditional. Claude-Session: https://claude.ai/code/session_013ZRubEFkLdS4givbjSKXBJ Signed-off-by: Ian Duvall <ian@omni.co>
Fixes the adapter side of #29952:
@prisma/adapter-pgdouble-releases the pg pool client when the query engine settles an interactive transaction twice, which eventually kills the Node process with an uncatchable error.Mechanism
When an interactive transaction timeout expires while
COMMITis awaiting the driver adapter, the engine'stx_timeout!select drops the commit future, but the napi-backed JS chain cannot be cancelled and eventually callsPgTransaction.commit()→client.release(). The timeout arm then compensates with a rollback, producing a second chain:executeRaw("ROLLBACK")on the still-busy client, thenPgTransaction.rollback()→client.release()again. pg-pool's double-release guard is per-checkout, so when the pool has re-lent the client between the two releases, the second release lands silently on the new owner's fresh closure and pushes a busy client back into the idle set. The new owner's own release then throwsRelease called on client which has already been released to the poolinside pg's socket-data handler, rethrown onprocess.nextTick— uncatchable, killing the process.The root cause (the engine dropping an uncancellable JS commit future and compensating while the first chain is still running) is in prisma-engines and is out of scope here; this PR hardens the adapter so a double settlement can no longer crash the process. App-visible behavior is unchanged: the racing transaction still fails with the engine's expired-transaction error.
Fix
PgTransactionsettles at most once:commit()/rollback()is a no-op — never a secondclient.release().queryRaw/executeRawafter settlement reject withTransactionAlreadyClosedwithout dispatching SQL, so the compensation'sROLLBACKcannot land on a client the pool already re-lent. The normalusePhantomQuery: falseflow (the engine sendsCOMMIT/ROLLBACKasexecuteRawon the open transaction before settling) is unaffected and pinned by a test.performIO) rather than reaching intopgclient internals.Tests
The new
PgTransactiontests stubpg.Client'sconnect/query/end(no database needed) and script the engine's settlement sequences directly, including the exact re-lend interleaving from the issue. All four regression tests fail on the unpatched adapter — the re-lend test reproduces the exactRelease called on client which has already been released to the poolerror — and pass with the fix. The pre-existing suite stays green.Sibling adapter audit
Kept out of this PR to keep the fix reviewable:
adapter-neonhas the verbatim samecommit()/rollback()→client.release()shape and the same hole; this fix applies to it directly.adapter-mariadbhas the same class of hole: unguarded double settlement double-end()s the pooled connection and can dispatch late SQL on a re-lent connection.adapter-planetscalesettles idempotently via a deferred, but still dispatches SQL after settlement.adapter-libsqlandadapter-better-sqlite3double-invokeclient.commit()/unlockParent()on double settlement — different, milder failure modes.adapter-mssql(mutex plus the tedious transaction state machine),adapter-d1(no-op settlement), andadapter-ppg(already guarded with a#finishedflag) are not exposed to the pool-corruption scenario.Summary by CodeRabbit
Bug Fixes
Tests