sql: keep pool slot queryCount balanced across connection death - #33743
Conversation
When the server closes a socket with N queries bound to the pool slot, #finishClose() zeroed queryCount and rejected every bound query, but each query's paired release (bindQuery's query.finally / sql.begin's finally) still ran one microtask later and decremented the already-zeroed counter to -N. Nothing repaired it on reconnect. A negative queryCount then let connect(reserved=true) hand the slot out as idle while other queries were already distributed to it, and each of those queries' release() crossed zero and handed the still-open transaction's socket to the next reservedQueue waiter. Concurrent sql.begin() callers interleaved BEGIN/COMMIT/ROLLBACK on one socket and all resolved successfully: a sibling's ROLLBACK discarded your writes, later statements escaped to autocommit, and a COMMIT applied to the wrong transaction, with no error surfaced. Stop zeroing queryCount in #finishClose(); the paired releases bring it to 0 naturally (release() already returns early for a closed slot). Also treat a nonzero count as busy in the reserved-connection selector and skip reserved connections in flushConcurrentQueries, so any future count desync cannot cascade into cross-transaction interleaving.
WalkthroughThis PR fixes pool bookkeeping in ChangesSQL Pool Release and Transaction Isolation
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 3:41 PM PT - Aug 18th, 2026
✅ @Jarred-Sumner, your commit 883d17c1890c9b57eee1c8aa313e89132d22053a passed in 🧪 To try this PR locally: bunx bun-pr 33743That installs a local version of the PR into your bun-33743 --bun |
|
Found 4 issues this PR may fix:
🤖 Generated with Claude Code |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
![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
🤖 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 `@src/js/internal/sql/shared.ts`:
- Around line 765-768: The inline comment in shared.ts is too long for the
repo’s 3-line limit. Trim the explanatory block near queryCount so it stays at
three lines or fewer, keeping only the essential rationale, and move any extra
detail into docs if needed; update the comment text in the same location around
queryCount to remain concise.
In `@test/js/sql/postgres-pool-transaction-isolation.test.ts`:
- Around line 85-192: These two transaction-isolation tests are independent and
should run concurrently to reduce test time. Update the two `test(...)` cases in
`postgres-pool-transaction-isolation.test.ts` to use `test.concurrent(...)`
since each one creates and cleans up its own `pgMockServer` and `SQL` instance
in `finally`, and there is no shared state between them.
- Around line 22-26: Trim the extra commentary in the SQL fault-injection test
file so only the shared top banner remains, and remove the additional multi-line
mechanism/root-cause and bug-history notes in the affected blocks. Update the
inline comments near the test cases referenced by the shared banner, keeping
them within the repo’s 3-line guideline and focusing the remaining comments only
on the test intent.
🪄 Autofix (Beta)
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.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a5ca6bc0-e9ac-4447-893b-645ecc79e5e8
📒 Files selected for processing (2)
src/js/internal/sql/shared.tstest/js/sql/postgres-pool-transaction-isolation.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
reserved_sql.release() early-returned without calling pool.release() once the connection was marked closed, so the reservation's single queryCount increment had no paired decrement on the server-drop and close() paths. #finishClose() zeroing queryCount used to mask this; with that removed, the slot stayed at queryCount=1 forever and the next reserved connect() waited on reservedQueue indefinitely. Route both the onClose handler and reserved_sql.release() through a releaseReservation() helper that fires pool.release() exactly once per reservation, gated on a released flag.
|
Added MySQL coverage: the test file is now Fail-before ( Earlier CI runs on this branch were green on the changed files; the only red lanes were Windows loopback-connection flake in unrelated wire-mock tests that run alphabetically earlier, plus an Alpine |
…arios The pool slot accounting lives in BaseSQLAdapter/BasePooledConnection, which MySQLAdapter inherits, so the same negative-queryCount interleaving reproduces with adapter: "mysql" and a COM_QUERY-only mock server. The test file is renamed to drop the postgres- prefix and runs the four scenarios against both adapters via describe.each.
There was a problem hiding this comment.
Thanks for addressing the sql.reserve() release path — that and the extra tests look right. I'm still deferring to a human here: this reworks a load-bearing pool-lifecycle invariant (paired increment/decrement of queryCount across close/reconnect), changes the user-visible contract of reserved.release() (reject → resolve when already released), and overlaps with #32006/#32101, so it's worth a maintainer's eyes on the state machine as a whole.
Extended reasoning...
Overview
The PR fixes cross-transaction interleaving in Bun.SQL's connection pool after a server-side disconnect. It touches:
src/js/internal/sql/shared.ts: stops#finishClose()from zeroingqueryCount(relying instead on each bound query's already-scheduled pairedrelease()), tightensconnect(reserved=true)to treatqueryCount !== 0as busy, and excludesreservedconnections fromflushConcurrentQueries().src/js/bun/sql.ts: adds aReservedConnectionState.releasedflag and a sharedreleaseReservation()helper sosql.reserve()'s slot is returned exactly once whether the socket dies or the user callsrelease()/close().reserved.release()now resolves idempotently instead of rejecting when the connection is already closed.- New
test/js/sql/sql-pool-transaction-isolation.test.tswith 8 fault-injection scenarios (postgres + mysql wire mocks) that fail on main and pass with the fix.
My earlier inline review found a real regression on the sql.reserve() path (queryCount stuck at +1 → deadlock); the author fixed it in 41ef6dc and added dedicated tests. A second pre-existing sibling issue (onTransactionConnected early-return releases ignoring dontRelease) was acknowledged and reasonably deferred to a follow-up since testing it requires fixing an unrelated unhandled-rejection bug first.
Security risks
None identified. This is internal pool bookkeeping; no new input parsing, no auth/crypto/permissions, no external data handling. The validateTransactionOptions regex path is unchanged.
Level of scrutiny
High. This is a concurrency-sensitive state machine in a production database driver where the failure mode is silent transaction interleaving (data corruption). The core change — removing queryCount = 0 from #finishClose — depends on a global invariant ("every increment has exactly one paired release that always runs") that already had one counterexample I found during review and one acknowledged pre-existing counterexample being deferred. The fix is well-reasoned and well-tested, but the invariant spans several files and multiple lifecycle paths (bindQuery, begin, reserve, close, disconnect, retry), and getting it wrong trades one corruption mode for another (deadlock/slot leak).
Other factors
- API behavior change:
reserved.release()after a server-side close previously rejected withconnectionClosedError(); it now resolvesundefined. This is arguably better (idempotent, matchesSymbol.disposeexpectations) but is user-observable and worth a maintainer sign-off. - PR overlap: the description notes overlap with #32006 (same
reservedfilter line) and #32101 (samesql.reserve()release path, different approach). A human should confirm the merge order / composition story. - Test coverage: strong — 8 deterministic fail-before/pass-after tests across both adapters, plus the existing lifecycle suites reported green. One unrelated Windows-baseline flake in CI (
postgres-binary-array-bounds.test.ts). - CODEOWNERS:
src/js/internal/sql/andsrc/js/bun/sql.tsare core SQL driver code that historically gets maintainer review.
Given the subtlety, the iteration it took to get right, the API-visible change, and the overlap with two other open PRs, this is not a change I'd approve without a human maintainer looking at the pool state machine holistically.
|
This also fixes #39562: release() on a reserved connection whose backend was closed server-side returns a rejected promise that no caller can catch, and sql.end() then never resolves because the reservation's pool slot is never returned. I verified it on this PR's src/js diff applied to main 4c68990: the issue's repro script passes (no unhandled rejection, end() resolves, exit 0), and it fails on main without the diff. Branch farm/9e2c3b38/sql-reserved-release-dead-backend has a regression test for that exact scenario against a real postgres backend (reserve, pg_terminate_backend from a second pool, release, end), plus a reserved.close() then sql.end() test. Both fail on main and pass with this PR's fix. Feel free to lift test/js/sql/sql-reserve-dead-backend.test.ts and its fixture into this PR, and to add "Fixes #39562" to the description. |
…efore BEGIN (#39598) ### Problem - `reserved.begin()` and `reserved.beginDistributed()` return the reservation's pool slot to the pool when they reject before `BEGIN` is sent. The caller still holds the reservation. - The three early returns in `onTransactionConnected()` call `pool.release(pooledConnection)` without a `dontRelease` check (`src/js/bun/sql.ts:583`, `:599`, `:614` on main). The `finally` block at `:862` has the check. The reserved callers pass `dontRelease = true` and never called `pool.connect()`, so there is no increment for these releases to pair with. - After the leaked release, a concurrent `sql.begin()` starts on the connection the reservation still uses. The later `reserved.release()` takes `queryCount` to -1. Since #33743, `connect(reserved = true)` treats a slot with `queryCount !== 0` as busy, so with `max: 1` every later `sql.begin()` or `sql.reserve()` waits forever. - `reserved.begin()` also chained `promise.finally(...)` for the `close({ timeout })` bookkeeping and dropped the chained promise (`:402`, `:428`). When the transaction rejects, that chained promise rejects too, and nobody handles it. Every rejected reserved transaction, for example a callback that throws inside `try/catch`, printed an unhandled rejection and set the exit code to 1. - `reserved.close({ timeout })` has the same defect one level up (`:473`). It waits with `Promise.all(...).finally(...)` on the promises the caller holds, so a transaction that fails while `close()` waits is reported as unhandled as well. Repro against a real postgres with `max: 1` (release build): a pool `sql.begin()` starts on the reserved connection while `r` is held, the caught error is printed as unhandled, and on main the final `sql.begin()` never resolves. ```ts const r = await sql.reserve(); await r.begin("read-only", async () => {}).catch(() => {}); // rejects: invalid options const t = sql.begin(async () => {}); // must wait, r holds the only slot r.release(); // queryCount: 0 -> -1 await t; await sql.begin(async () => {}); // hangs on main ``` ### Fix - The three early returns release the connection only when `dontRelease` is false. This is the same rule the `finally` block applies. A reserved connection belongs to the reservation until `reserved.release()`. - `reserved.begin()` and `beginDistributed()` share a `runReservedTransaction()` helper. For each transaction it creates a second promise that only fulfills, stores that one in `reservedTransaction`, and passes two bound `settleReservedTransaction()` callbacks to `onTransactionConnected()`. Each callback removes the entry, fulfills it, then settles the caller's promise. Nothing is attached to the promise the caller gets, and `close({ timeout })` now waits on promises that cannot reject. Both helpers are defined outside `onReserveConnected()` and get the per-reservation state as arguments, so no function is created per reservation. - This is correct because any handler attached to the caller's promise changes what the caller observes. `.then()` or `.finally()` marks the promise as handled, so a rejection the caller ignores is never reported. A `.finally()` chain adds a second rejection that the caller cannot handle. With no handler, a rejected reserved transaction is reported exactly when the caller ignores it, which is what `sql.begin()` does today. - The third early return (`getTransactionCommands()` throws) gets the same check for consistency. No adapter with reserved connections throws there, so it has no test. - Verified with `test/js/sql/sql-pool-transaction-isolation.test.ts`. It runs against in-process postgres and mysql wire mocks that record each statement per connection. New cases, per adapter: - `begin()` with invalid options and `beginDistributed()` with an invalid name: the recorded statements show that a concurrent `sql.begin()` waits for `release()`, and that the slot can be reserved again afterwards. - `reserved.close({ timeout })` waits for a reserved transaction that commits. This pins the success path that now resolves through the helper. It passes on main too. - `reserved.close({ timeout })` waits for a reserved transaction that fails and the caller catches. The `ROLLBACK` is recorded, and the test fails if `close()` reports the caught error as unhandled. It fails on main and on the first revision of this PR. - A child process handles one rejected reserved transaction and ignores another: exactly the ignored one reaches `unhandledRejection`. On main both are reported. A fix that attaches `.then()` to the promise reports neither. I checked both variants against this test. - On main's `src/`, the eight new rejection tests fail at once (no timeouts) and the eight existing tests pass. With the fix all 18 pass under the debug (ASAN) build. - The repro above and a script that exercises `reserved.begin()` success, array results, options, savepoints, a throwing callback, and `close({ timeout })` behave correctly against a real postgres with the fixed build. ### Background - `sql.reserve()` takes one pooled connection out of the pool. The pool marks the slot with `queryCount = 1` and the `reserved` flag. `reserved.release()` calls `pool.release()` once, which brings the count back to 0 and makes the slot available again. - `onTransactionConnected()` runs a transaction on a pooled connection. `sql.begin()` obtains the connection with `pool.connect()` and passes `dontRelease = false`, so the function releases it when the transaction ends. `reserved.begin()` passes the reservation's own connection and `dontRelease = true`, because the reservation still owns it afterwards. - `reservedTransaction` is the set of in-flight transactions on a reservation. `reserved.close({ timeout })` waits for them before it closes the connection. On main the set holds the promises returned to the caller. With this change it holds one internal promise per transaction. - A promise rejection is reported as unhandled when the promise has no reaction handler at the time it rejects. Attaching any handler, including `.finally()` or `Promise.all()`, marks the promise as handled. `.finally()` also returns a new promise that rejects with the same reason, and that one has no handler unless the code keeps it. ### Related - #33743 fixed `sql.reserve()`'s own release path and left these early returns for a follow-up. - `reserved.close({ timeout })` and `transaction.close({ timeout })` still wait on the caller's query objects with the same `.finally()` shape (`pending_queries` at `:473` and `:753` on main). A direct query that fails while `close()` waits is still reported as unhandled. The queries are tracked as the caller's objects in `state.queries`, so that needs the same kind of internal signal in the query tracking, which is shared with plain transactions. It is not touched here. <details> <summary>Revision history</summary> - 09a06ca kept the caller's promise in `reservedTransaction` and only moved the removal into the settle wrappers. That fixed `reserved.begin()` itself, but `reserved.close({ timeout })` still produced an unhandled rejection when a caught transaction failed while it waited. - e63835c switches the set to internal promises and adds the failing-transaction `close({ timeout })` test. - f55cad2 moves the two helpers out of `onReserveConnected()` after review. The per-reservation state is passed as arguments and bound instead of captured. </details>
3 participants
Repro
What the server received on the reconnected socket, in order:
All three
sql.begin()calls resolve successfully. The triggering event is just the server closing a socket with work in flight (server restart,pg_terminate_backend/KILL <id>,idle_in_transaction_session_timeout/wait_timeout, LB idle kill). The pool machinery is shared between adapters, soadapter: "mysql"reproduces the identical interleaving withSTART TRANSACTIONin place ofBEGIN.Cause
BasePooledConnection.#finishClose()setqueryCount = 0and rejected every bound query, but each of those queries still has a pairedrelease()scheduled (bindQuery'squery.finally(onQueryFinish)for plain queries,sql.begin()'sfinally { pool.release(pooledConnection) }for reservations). Those releases run one microtask later and decrement the already-zeroed counter to-N, and nothing repairs it on reconnect.The negative count then cascades through two selection predicates:
connect(cb, reserved=true)skipped busy connections withqueryCount > 0, so a negative count read as idle and the slot was handed tosql.begin()whileflushConcurrentQueries()had already distributed other queries to it.release()broughtqueryCountback through 0, which clearedreservedand shifted the nextreservedQueuewaiter onto the same socket while the first transaction was still open.Fix
src/js/internal/sql/shared.ts:#finishClose()no longer zeroesqueryCount. The paired releases bring it to 0 naturally;release()already returns early for a slot whosestate !== connected, so the decrements have no other side effect.totalQuerieswas already decremented by those releases and stays correct.connect(cb, reserved=true)treatsqueryCount !== 0as busy, andflushConcurrentQueries()also excludesreservedconnections, so any future count desync cannot cascade into cross-transaction interleaving.src/js/bun/sql.ts:sql.reserve()was the one place whose paired release was conditional:reserved_sql.release()early-returned without callingpool.release()once the connection was marked closed, so the old zeroing masked a missing decrement. The reserve wrapper'sonClosehandler andreserved_sql.release()now both route through areleaseReservation()helper that firespool.release()exactly once per reservation (gated on areleasedflag), so a server-side disconnect duringsql.reserve()and an explicitreserved.close()both return the slot.Tests
test/js/sql/sql-pool-transaction-isolation.test.tsruns four scenarios against both the postgres and mysql adapters (describe.each), using minimal in-process wire servers (no Docker) that record every simple-protocol / COM_QUERY statement per connection and destroy the socket on a marked query:sql.begin()(the server seesBEGIN,BEGIN,BEGIN/START TRANSACTIONx3 before the fix);sql.begin(), then two concurrentsql.begin();sql.reserve(), then the slot is still reservable;reserved.close()on a healthy connection, then the slot is still reservable.On main's src all eight fail; with the fix all eight pass. Deterministic 3/3 both ways. The existing connection-lifecycle suites (
sql-connect-error-reporting,sql-close-pending-connection,postgres-failed-connection-resurrection,sql-onconnect-onclose-throw,wire-frames,postgres-binary-array-bounds,postgres-invalid-message-length) stay green.Related
reservedfilter toflushConcurrentQueries()for a different symptom (pool stall); whichever merges second becomes a no-op on that line.sql.reserve()release path from a different angle (its symptom issql.close()hanging afterreserved.close()); the two approaches compose.[review] gate passed · iteration 9 · 3 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 1 rejected · iteration 9
evidence per changed file