Repository navigation
sql: reject a transaction or reserved query that first runs after the handle closed - #43261
Conversation
… handle closed
A Query is lazy. It runs when it is first awaited or when execute() is
called. The transaction and reserved handles read their closed state
only when the query is created. A query created while the handle was
open and first awaited after COMMIT, ROLLBACK or release() still ran on
the connection bound at creation. On SQLite the statement ran outside
the rolled back transaction. On PostgreSQL and MySQL it ran on the
pooled connection, inside the transaction of the next caller.
queryFromTransactionHandler now receives the TransactionState instead
of its query set and rejects with the adapter's connection closed error
when the closed bit is set.
Behaviour change: a query returned without await from a block that
releases the handle, such as `using reserved = await sql.reserve();
return reserved`...`` or `try { return reserved`...` } finally {
reserved.release() }`, first runs after release() and now rejects with
ERR_*_CONNECTION_CLOSED. The docs note that each query must be awaited
before the release.
Fixes #43245
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟣
src/js/bun/sql.ts— Users who callawait tx.close({ timeout })while a query is in flight get their transaction COMMITTED instead of rolled back, on this branch as on the base. The drain branch at src/js/bun/sql.ts:800-804 resolves close() when the pending queries settle but never sends ROLLBACK and never setsclosed. The callback then returns,needs_rollbackis cleared, and COMMIT at src/js/bun/sql.ts:886 passes the check at :687 becauseclosedis unset. Fix: the drain path must do what the no-timeout path at :810-814 does: send BEFORE_COMMIT/ROLLBACK and setclosedbefore resolving; same for the reserved sibling at :528-532 which never closes the connection. [also at: src/js/bun/sql.ts:532 - Users who callawait reserved.close({ timeout })with a query in flight leak the pooled connection: it is never closed and never released, so withmax: 1every later query orreserve()waits forever.]Extended reasoning...
The PR body justifies checking
closedonly with: close({ timeout }) drains pending queries in that window. The dismissing finders read the lines as unchanged but did not follow the drain path to its end. Trace: user runs sql.begin(async tx => { const p = txUPDATE ...; p.then(()=>{}); await tx.close({ timeout: 5 }); }). close() at :771 passes acceptsQueries, clears acceptQueries at :774. transactionQueries.size > 0 so :783 enters the timeout branch. Promise.all(pending_queries) at :800 waits for the UPDATE; when it settles, .finally clears the timer and resolves close(). No ROLLBACK is sent. state.connectionState still lacksclosed. close() returns; the callback returns undefined. At :877 the await completes, :882 sets needs_rollback = false, :886 creates COMMIT through run_internal_transaction_sql; :687 seesclosedunset so COMMIT is sent. The handler's new guard at :196 also seesclosedunset, so it does not help. The UPDATE the user asked close() to discard is committed.…Verification: pre-existing (the base file at cb285a4 has byte-identical
transaction_sql.close; this PR only reasons about the path in its description — "close({ timeout }) drains pending queries in that window" — and routes COMMIT through the handler it changes, it does not fix it). Trigger:await tx.close({ timeout: N })inside asql.begincallback while at least one transaction query is still in… -
🟣
src/js/bun/sql.ts— Users who startreserved.begin(cb)and let the reserved handle be released before it finishes still get that transaction's BEGIN, statements and COMMIT sent on a connection the pool has already handed to another caller. This is the exact cross-caller leak the PR guards against, unguarded through the nested-transaction path.release()at src/js/bun/sql.ts:544-557 marks only the reservedstateclosed and ignoresreservedTransaction; the inner transaction at :617 has its own state, so the handler guard at :196 never fires for its queries. Fix: release() must refuse or defer whilereservedTransactionis non-empty, or mark the inner states closed, as close({timeout}) already waits on them at :516.Extended reasoning...
The dismissing finder marked release() untouched and stopped. Trace: { using reserved = await sql.reserve(); reserved.begin(async tx => { await tx
INSERT ...; await slowWork(); await txUPDATE ...; }); } The block exits synchronously, Symbol.dispose at :560 calls release(): :549 sets closed on the reserved state, :555 calls pool.release(pooledConnection). Meanwhile runReservedTransaction created a separate TransactionState at :617 with acceptQueries set; BEGIN at :875 was sent through run_internal_transaction_sql, whose :687 check reads the inner state. With max: 1 the next sql.begin() or plain sql...from another caller gets the same pooledConnection and runs inside the still-open BEGIN. When slowWork resolves the UPDATE goes through transaction_sql at :696 (inner state still accepts) and the handler guard at :196 sees the inner state not closed, so it runs on the other caller's connection; COMMIT at :886 then commits the other caller's statements too. close({timeout}) at :512-516 does wait on reservedTransaction, proving the author knows the inner transactions must finish before the handle…Verification: pre-existing — the base fails the same way by the same route; the PR's new guard does not reach this path. Trigger:
reserved.begin(cb)is not awaited (e.g.return reserved.begin(async tx => {...})from ausing reservedblock, or release() infinally) and the reserved handle is released before the transaction settles. Mechanism verified in /home/claude/bun/src/js/bun/sql.ts: -… -
🟣
src/js/bun/sql.ts— A process that callstx.close({ timeout })insidesql.beginwithout awaiting it and then returns from the callback crashes with an unhandled ERR_*_CONNECTION_CLOSED rejection once the timer fires, after the transaction already committed. The timer callback at src/js/bun/sql.ts:788-797 is an async function whoseawait run_internal_transaction_sql(ROLLBACK_COMMAND)at :795 rejects at :687 because the finally at :901 setclosedfirst; nothing catches that rejection. Fix: the timer callback must skip the ROLLBACK whenclosedis already set and catch rejections from run_internal_transaction_sql, and the same guard belongs in the reserved timer at :517.Extended reasoning...
Steps.
sql.begin(async tx => { const q = tx\SELECT pg_sleep(10)`; q.then(...); tx.close({ timeout: 1 }); }). close() at :769 clears acceptQueries, size > 0, arms the timer at :788, returns the promise, which the user does not await. The callback returns, :877 resumes, :886 sends COMMIT (closed unset at :687 and at :196). COMMIT resolves, :901 sets closed, pool.release hands the connection back. One second later the timer fires. :789 cancels the pending queries (they never settle, the :809 finding). :795 calls run_internal_transaction_sql(ROLLBACK); :687 returns Promise.$reject(connectionClosedError()). Theawait` throws inside the async setTimeout callback; the returned promise has no handler. Bun's default unhandled-rejection behavior reports the error and exits the process with code 1. Base cb285a4 has the same lines, so the crash is pre-existing, but the dismissing finders only noted the diff does not change the lines; neither followed the rejection to the process exit. Population: any server that uses close({ timeout }) as a fire-and-forget abort inside a transaction.…Verification: pre-existing — the base commit has byte-identical code on this path (git show cb285a4:src/js/bun/sql.ts: the same
setTimeout(async () => { ... await run_internal_transaction_sql(ROLLBACK_COMMAND); ... })and the sameif (closed) return Promise.$reject(pool.connectionClosedError())in run_internal_transaction_sql); the diff only swaps the argument at :690 (state.queries->state) and adds…
|
On the review findings. All of them are on the base, and none of them is in the path this PR changes. Where they stand:
This PR stays on the one rule it adds: a handle query that first runs after |
This branch has not been deployed
1 participant
Stacked on #43249. The base is that PR's branch. Only the last commit is new.
Problem
Querycreated on a transaction or reserved handle still runs when it is first awaited after the handle closed. On SQLite the insert from the issue runs afterROLLBACKand the row exists. On PostgreSQL and MySQL withmax: 1the statement lands on the pooled connection, inside the next caller's transaction. A mock server recordsBEGIN, SELECT 'T3a', SELECT 'lazy from T1', SELECT 'T3b', COMMIT.queryFromTransactionHandler(src/js/bun/sql.ts:179). The handle readsclosedonly when the query is created. The handler runs on the connection bound at creation and never reads the state again. sql: reject unsafe() and file() on a closed transaction or reserved handle #43249 adds the creation time check forunsafe()andfile()and defers this case to Bun.SQL: a query created on a transaction or reserved handle still runs when it is first awaited after the handle closed #43245.Fix
queryFromTransactionandunsafeQueryFromTransactionpass theTransactionStateto the handler instead of its query set. The handler rejects with the adapter's connection closed error when theclosedbit is set. A query that was already sent is not affected.closedonly, notacceptQueries.close()clearsacceptQueriesbefore it sendsROLLBACKthrough the same handler, andclose({ timeout })drains pending queries in that window.return reserved\...`withoutawaitfrom ausing reservedblock or afinally { reserved.release() }block now rejects withERR_*_CONNECTION_CLOSED. The query first runs afterrelease(), on a connection that can already belong to another caller. The docs always await inside the block. One new test covers both forms, anddocs/runtime/sql.mdx` gets one paragraph.test/js/sql/sql-pool-transaction-isolation.test.ts(three new tests per adapter on the postgres and mysql mock servers) andtest/js/sql/sqlite-sql.test.ts(two new cases). All ten fail on the base without thesrc/change. Alsosql.test.ts,postgres-listen-notify.test.ts,sql-reserve-abort.test.ts,sql-close-pending-connection.test.ts.Background
Query(src/js/internal/sql/query.ts) is a promise subclass. It does nothing untilthen(),catch(),finally()orexecute()runs its handler.awaitcallsthen().sql.begin(tx => ...)) and a reserved handle (sql.reserve()) share oneTransactionState. ItsconnectionStatebit field hasacceptQueries,closedandreleased. Commit, rollback,close()andrelease()setclosed.pool.release()hands the connection back to the pool. Withmax: 1the nextsql.begin()gets the same connection, so a late statement runs inside that transaction.Notes
expect(query).rejectshangs on aQuery: the matcher resolves the value without the overriddenthen(), so the lazy query never runs. The tests usequery.then(() => null, e => e.code)like the rest of the file.unsafe()without parameters because the mock servers answer simple protocol queries only. The SQLite test covers the tagged template form.sql.begin(async tx => tx\...`)andsql.begin(async tx => [tx`a`, tx`b`])still work:onTransactionConnectedawaits the callback result beforeCOMMIT`, so those queries run while the handle is open.test/js/sql/sql-mysql.transactions.test.tsfails in this container withAccess denied for user 'root'@'localhost'on main as well. It is a local service setup issue, not this change.lazy resolved: [],rows: [{ v: "lazy" }]. After:lazy rejected: ERR_SQLITE_CONNECTION_CLOSED,rows: []. Same on a real PostgreSQL for the transaction and the reserved handle.usingandfinallyforms, add the docs line).[human-review] gate passed · iteration 0 · 4 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file
root cause · written by the author bot
A
Queryis lazy, but the handle'sclosedstate was only read when the query was created, soqueryFromTransactionHandlerran a query first awaited after COMMIT, ROLLBACK, orrelease()on the captured connection, which on SQLite autocommitted rolled-back work and on PostgreSQL and MySQL sent the statement to a pooled connection another caller might already own. The fix passes theTransactionStateinto the handler so it re-checks theclosedbit when the query actually runs and rejects with the existingconnectionClosedError()if the handle has since closed. This intentionally mak…