Repository navigation
sql: make transaction.close({ timeout }) roll back when pending queries drain before the timeout - #32149
sql: make transaction.close({ timeout }) roll back when pending queries drain before the timeout#32149robobun wants to merge 5 commits into
Conversation
…es drain before the timeout
transaction_sql.close({ timeout }) resolved without issuing ROLLBACK and
without setting the closed bit when the pending queries settled before
the timer fired, so the COMMIT issued after the begin() callback
returned persisted writes the user explicitly closed. The internal
Promise.all chain was also dropped, turning a rejecting pending query
into an unhandledRejection even when the user handled it.
Converge every close path (drain, timer, no-timeout) on one memoized
helper that cancels pending queries, runs
BEFORE_COMMIT_OR_ROLLBACK/ROLLBACK, and sets the closed bit in a finally
so a failed rollback still blocks a later COMMIT. Wait out the grace
period with Promise.allSettled so one rejecting query neither cuts the
grace period short nor leaks an unhandled rejection, and propagate
rollback failures to the close() caller instead of hanging it.
|
Updated 11:26 PM PT - Jun 11th, 2026
✅ @robobun, your commit 7f10658adcd086e3fe07ddc36a5ce5d4f401ccd7 passed in 🧪 To try this PR locally: bunx bun-pr 32149That installs a local version of the PR into your bun-32149 --bun |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughCentralizes reserved transaction shutdown into a memoized closeTransaction helper and changes transaction.close({ timeout }) to wait for pending queries/savepoints (grace period) before performing rollback. Adds in-process Postgres and MySQL protocol-mock tests validating drain, savepoint, rejection, and timeout ordering scenarios. ChangesTransaction close grace-period behavior
Possibly related issues
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
The explicit close stays only in the unhandledRejection test, where the pool must tear down while the listener is still attached.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/js/sql/sql-transaction-close.test.ts (1)
320-335: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAssert the protocol-level negative and ordering invariants.
These tests currently prove the client eventually rejects and that
ROLLBACKeventually appears, but they do not prove the two load-bearing properties this PR is fixing: a post-close query never reaches the wire, and the timeout path does not emitROLLBACKbefore the held query settles. A regression in either direction could still pass here.🔧 Tighten the assertions
await expect(begin).rejects.toMatchObject({ code: "ERR_POSTGRES_CONNECTION_CLOSED" }); expect(pendingResult).toEqual([{ x: "hold" }]); expect(queryAfterClose).toBe("ERR_POSTGRES_CONNECTION_CLOSED"); + expect(pg.commands).not.toContain("select 1 as x"); expect(pg.commands).toContain("ROLLBACK"); expect(pg.commands).not.toContain("COMMIT");while (!pending.cancelled) { await Bun.sleep(5); } + expect(pg.commands).not.toContain("ROLLBACK"); // ROLLBACK can only go out on the wire after the in-flight query // completes, so answer it now (settlement of a cancelled in-flight // query is not the subject here) socket.write(selectResponse("hold"));As per coding guidelines, "Every assertion must be able to fail, and must assert the strongest invariant."
Also applies to: 430-445
🤖 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 `@test/js/sql/sql-transaction-close.test.ts` around lines 320 - 335, The test must assert protocol-level ordering: ensure a post-close query never reached the wire and that ROLLBACK is emitted only after the held query settles. After running the operations, capture pg.commands and assert (1) there is no entry corresponding to the post-close query (use the same query string used for queryAfterClose and the tx template) and (2) that the index of "ROLLBACK" in pg.commands is greater than the index of the held query's command (the select used to produce pendingResult) so ROLLBACK occurs after that query finished; apply the same strengthened assertions in the corresponding block around lines 430-445 using the same variables (tx, pendingResult, queryAfterClose, begin, pg.commands).Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@test/js/sql/sql-transaction-close.test.ts`:
- Around line 320-335: The test must assert protocol-level ordering: ensure a
post-close query never reached the wire and that ROLLBACK is emitted only after
the held query settles. After running the operations, capture pg.commands and
assert (1) there is no entry corresponding to the post-close query (use the same
query string used for queryAfterClose and the tx template) and (2) that the
index of "ROLLBACK" in pg.commands is greater than the index of the held query's
command (the select used to produce pendingResult) so ROLLBACK occurs after that
query finished; apply the same strengthened assertions in the corresponding
block around lines 430-445 using the same variables (tx, pendingResult,
queryAfterClose, begin, pg.commands).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 30c9dbf3-8432-43d1-a441-8ddcf38df434
📒 Files selected for processing (1)
test/js/sql/sql-transaction-close.test.ts
…for the in-flight query
|
Applied the assertion strengthening from the CodeRabbit review in 3c10069: the drain test now asserts the rejected post-close query never reached the wire, and the timer test asserts ROLLBACK is not on the wire before the held query is answered (combined with the existing toContain("ROLLBACK") this pins the ordering, so a separate indexOf check would be redundant). |
There was a problem hiding this comment.
No issues found and the earlier nits are addressed — deferring only because this reworks the transaction close/rollback state machine (data-integrity path), which is worth a quick look from a SQL maintainer.
Extended reasoning...
Overview
This PR fixes #32148 by converging all three transaction_sql.close() paths (drain-before-timeout, timer-fires-first, no-timeout) onto a single memoized closeTransaction() helper in src/js/bun/sql.ts. Previously, the drain branch resolved without issuing ROLLBACK or setting the closed bit, so a transaction the user explicitly closed could still be committed by onTransactionConnected. The fix is ~30 net lines in production code plus ~480 lines of new tests using minimal in-process postgres/mysql protocol mocks.
Security risks
None identified. No auth, crypto, or permission surfaces are touched. The change is internal control-flow in the SQL transaction lifecycle.
Level of scrutiny
This is squarely in data-integrity-critical territory: the bug being fixed is "a transaction the user closed gets committed instead of rolled back," and the fix changes when/whether ROLLBACK reaches the wire and when the closed bit gates a subsequent COMMIT. The refactor is careful (memoized promise for idempotence, allSettled to avoid short-circuiting the grace period and to observe rejections, finally to set the closed bit even on rollback failure) and mirrors the already-landed #32101 treatment of reserved_sql.close. But subtle ordering in async transaction state machines is exactly where a domain owner's review pays off — e.g., interaction with onTransactionDisconnected setting the closed bit mid-grace-period, or the pooled connection's reuse after a failed ROLLBACK.
Other factors
- Both of my earlier inline nits (redundant manual
sql.close()alongsideawait using; 1s wall-clock sleep in the timer test) have been addressed in cb1784c and 3b54722, and the threads are resolved. - The bug-hunting pass on the current revision found nothing.
- Test coverage is thorough: 5 tests covering drain, rejection-during-grace (no unhandledRejection), savepoint drain, timer-fires-first, and the mysql adapter path, plus a real-postgres manual verification per the description.
- CodeRabbit suggested
alii/cirospaciarias reviewers, which matches who typically owns this area.
Given the category of change (transaction commit/rollback semantics) rather than any specific concern with the implementation, I'm deferring rather than approving.
There was a problem hiding this comment.
No issues found and the earlier nits are addressed — the fix and tests look solid, but since this changes transaction commit/rollback semantics in the SQL driver I'd like a maintainer to sign off rather than auto-approving.
Extended reasoning...
Overview
This PR fixes #32148 by refactoring transaction_sql.close({ timeout }) in src/js/bun/sql.ts. The drain-before-timer branch previously resolved without issuing ROLLBACK or setting the closed bit, allowing a closed transaction to be committed. The fix converges all three close paths (drain, timer, no-timeout) on a single memoized closeTransaction() helper that cancels pending queries, runs ROLLBACK, and sets the closed bit in a finally. It also swaps Promise.all for Promise.allSettled to avoid leaking unhandled rejections and cutting the grace period short. A new test file (test/js/sql/sql-transaction-close.test.ts, ~480 lines) adds minimal mock postgres/mysql wire-protocol servers and 5 tests covering drain, rejection-during-grace, savepoint drain, timer-fires-first, and the mysql adapter path.
Security risks
None identified. The change is internal transaction-lifecycle bookkeeping; no auth, crypto, input parsing, or external surface is touched. The mock servers in the test file bind to 127.0.0.1:0 and are torn down via await using.
Level of scrutiny
High. This is production-critical data-integrity code: the bug being fixed is that an explicitly closed transaction was silently committed instead of rolled back. The fix changes the ordering and error-propagation of ROLLBACK relative to the closed bit and the outer onTransactionConnected commit path. While the diff is modest (~30 net lines in sql.ts) and mirrors the already-landed shape from #32101 for reserved_sql.close, transaction commit/rollback semantics are exactly where a subtle regression would cause user data loss or corruption. A maintainer familiar with the SQL adapter (cirospaciari/alii per the CodeRabbit suggestion) should confirm the interaction with onTransactionConnected's own rollback path and the BEFORE_COMMIT_OR_ROLLBACK_COMMAND (mysql XA) ordering.
Other factors
- Both of my earlier inline nits (redundant
await sql.close()alongsideawait using, and the 1s timer-test wall-clock) were addressed in cb1784c and 3b54722; the only commit since is a CI retrigger. - The bug-hunting pass on the current revision found nothing.
- Test coverage is thorough and asserts wire-level command ordering against mock servers, which is strong evidence the fix behaves as described.
- No CODEOWNERS entry for
src/js/bun/sql.ts.
Given the criticality of the code path, I'm deferring rather than approving even though I have no specific concerns with the implementation.
|
Independently reproduced and arrived at the same root cause via a sqlite repro. Pushed a simpler variant on That branch also has sqlite-backed tests in |
|
Related: #39617 fixes the same drained arm for reserved.close({ timeout }). As of 25edb8d it adds a module-level waitForPendingWork(pending, timeout) helper in src/js/bun/sql.ts (timer plus allSettled) that close() awaits before it runs its one close body. transaction.close({ timeout }) can await the same helper before its ROLLBACK, which would remove the need for a memoized close promise here. Its tests live next to the reserved.close cases in test/js/sql/sql-pool-transaction-isolation.test.ts, whose mocks now have FAIL and HOLD markers. |
|
Context from #41492 (a query cancelled before it runs now settles). That change depends on this fix for one sequence: I wrote the same change before I found this PR, so I did not open a second one. For reference only: main...robobun/455fc2ef/sql-close-timeout-settle covers the reserved scope (#39617) and this scope with one helper. This PR handles a failed ROLLBACK and a repeated |
…timeout Every arm of transaction.close() runs one memoized helper that cancels the pending queries, sends ROLLBACK, and sets the closed bit. Squash of #32149.
1 participant
Fixes #32148. Split out of the review of #32101, which fixed the identical shape in
reserved_sql.close({ timeout }).Repro
Cause
transaction_sql.close({ timeout })insrc/js/bun/sql.tsraces the pending queries/savepoints against a timer. The drain branch (pending work settles before the timer) only ranclearTimeout(timer); resolve();:ROLLBACKand never setReservedConnectionState.closed, so when thebegin()callback returned,onTransactionConnectedproceeded toCOMMIT(the closed gate inrun_internal_transaction_sqlwas not set) and a transaction the user explicitly closed got committed. The no-timeout path correctly rolls back and blocks the later COMMIT withERR_POSTGRES_CONNECTION_CLOSED.Promise.all([...]).finally(...)chain was dropped, so a pending query rejecting during the grace period surfaced as an unhandledRejection (nonzero exit) even when the user handled their own query promise.The timer branch additionally ran its rollback in a bare async callback: a rollback failure there was another unhandled rejection and left
close()pending forever.Fix
Converge all three paths (drain, timer, no-timeout) on one memoized helper, mirroring the #32101 treatment of
reserved_sql.close:BEFORE_COMMIT_OR_ROLLBACK_COMMAND(mysql XA) thenROLLBACK, and sets the closed bit in afinallyso even a failed rollback blocks a later COMMITPromise.allSettled, so one rejecting query neither cuts the grace period short for the rest nor leaks an unhandled rejectionclose()caller instead of hanging itreserved_sql.close, the pooled connection intentionally stays open; it is released back to the pool byonTransactionConnectedonce the begin callback settles (unchanged)Verification
test/js/sql/sql-transaction-close.test.tsuses minimal mock postgres/mysql servers (same approach as #32101) to assert which commands reach the wire. All 5 tests fail on the unfixed build withExpected promise that rejects, Received promise that resolved(the COMMIT went through) and pass with the fix:RELEASE SAVEPOINT) before the rollbackAlso verified against a real postgres 17: the issue's repro now rolls back (
begin()rejects withERR_POSTGRES_CONNECTION_CLOSED, the INSERT is not persisted, matching the no-timeout path), and commit/rollback/savepoint/timer-path/pool-reuse flows behave as before.