Repository navigation
Conversation
When the callback of sql.begin() or sql.savepoint() throws, the driver sends ROLLBACK (or ROLLBACK TO SAVEPOINT). If that statement failed, or the connection dropped under it, the transaction rejected with the rollback failure alone and the callback error was lost. The rollback failure stays the rejection and now carries the callback error as its cause.
|
Warning Review limit reached
On-demand reviews are free for the next 13 days. After that, they cost $0.25 per reviewed file. Or wait 57 minutes for your next included review. View limit detailsLimit details: You’ve used all 10 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (2)
Comment |
|
Updated 6:55 PM PT - Sep 7th, 2026
❌ @Jarred-Sumner, your commit aa9035d has 7 failures in
🧪 To try this PR locally: bunx bun-pr 41817That installs a local version of the PR into your bun-41817 --bun |
|
Status: ready for review. Reproduced on bun 1.4.3 with the mock peers in |
![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.
Beyond the inline findings, I also checked the outer transaction rollback path (src/js/bun/sql.ts:891-903) for a missing finally { state.rollbacks-- } — it is safe: that path terminates in reject() and the per-call state object is never reused, so the counter staying at 1 cannot affect a later disconnect.
Extended reasoning...
The change restructures two catch blocks and adds a counter-based guard in the disconnect handler. The savepoint path decrements rollbacks in a finally; the outer begin() path does not. I traced the outer path: on both branches (rollback succeeds → reject(err); rollback throws → reject(withCause(...))) the begin() promise settles and the enclosing finally releases the connection, after which state is unreachable and onTransactionDisconnected will not fire again for it — so the un-decremented counter is dead. The two inline findings (unsettled-promise hazard when a user callback swallows a savepoint disconnect error, and non-primordial Object.* / in usage in withCause) stand on their own and warrant human review; I had nothing further to add there.
| } | ||
|
|
||
| if (err) { | ||
| if (err && this.rollbacks === 0) { |
There was a problem hiding this comment.
🟡 (optional) Suppressing reject(err) while a savepoint rollback is in flight can leave sql.begin() unsettled: when the connection drops during ROLLBACK TO SAVEPOINT, the error is now thrown into the user's transaction callback instead of rejecting begin() directly, so a callback that catches its tx.savepoint(...) rejection and then awaits non-SQL work never lets the outer catch run — on the base branch begin() rejected immediately here. The PR notes this as accepted; it still violates "settle every pending promise slot". Fix: after state.rollbacks returns to 0 in the savepoint finally, if connectionState is closed reject the outer promise with the composed error so disconnect always settles begin().
Extended reasoning...
Path: user calls sql.begin(async tx => { try { await tx.savepoint(cb) } catch (e) { log(e); await someExternalEvent; } }). cb throws → run_internal_savepoint enters its catch, sets state.rollbacks = 1, awaits ROLLBACK TO SAVEPOINT. The server drops the socket → onTransactionDisconnected runs, marks connectionState |= closed, rejects every queued query, then hits line 270: err is truthy but this.rollbacks === 1, so reject(err) is skipped. The awaited rollback rejects, withCause wraps it, the savepoint finally decrements rollbacks to 0, and the wrapped error is thrown to the user's catch (e). The user swallows it and awaits something unrelated to tx (any further tx query would reject immediately because connectionState is closed, but non-SQL awaits do not). await callback(transaction_sql) at line 882 never resolves, so neither resolve nor reject is called and the finally at 906 never releases the pooled connection. On the base branch onTransactionDisconnected unconditionally called reject(err), so begin() settled immediately even if the…
Verification: nit — acknowledged in diff: the PR "Notes" section describes exactly this scenario ("A callback that swallows that error and then waits forever on non-SQL work keeps begin() pending"); the note's claims hold. The path is real and reachable: - src/js/bun/sql.ts:835-842 — when the savepoint callback throws, state.rollbacks++ is set before awaiting ROLLBACK TO SAVEPOINT. If the socket…
| if (!Error.isError(error) || "cause" in error) return error; | ||
| const copy = new Error(error.message, { cause }); | ||
| Object.setPrototypeOf(copy, Object.getPrototypeOf(error)); | ||
| return Object.assign(copy, error); |
There was a problem hiding this comment.
🟡 (optional) withCause calls Object.setPrototypeOf/getPrototypeOf/assign (and checks "cause" in error) at call time; a user that monkeypatches Object.assign or sets Error.prototype.cause before their transaction callback throws will make the ROLLBACK error path throw or silently drop the callback error — base branch had no such dependency. Fix: follow the src/js/ primordial convention — capture Object.getPrototypeOf/setPrototypeOf/assign into module-level consts at load time (as node/vm.ts, _http_client.ts, etc. do) and test own-property presence via $getOwnPropertyDescriptor/hasOwn rather than the prototype-walking in operator.
Extended reasoning...
REVIEW.md requires builtins in src/js/ to be tamper-resistant with "globals captured at module load". withCause runs on the ROLLBACK-failed path with user code already on the stack. If user code has done Object.assign = () => { throw new Error() }, line 51 throws inside the outer catch at sql.ts:902 (return reject(withCause(...))): the exception escapes the catch, the finally releases the connection, and the thrown value propagates out of onTransactionConnected — the sql.begin() promise is never settled (base branch called reject(err) unconditionally). Separately, "cause" in error walks the prototype chain, so Error.prototype.cause = undefined makes every rollback error short-circuit at line 48 and return the shared connection-closed error unchanged, both dropping the callback error and mutating-by-omission defeating the whole purpose of the copy. Other src/js/ files that need these methods capture them at module scope (const ObjectSetPrototypeOf = Object.setPrototypeOf in node/vm.ts, internal/serialization_buffers.ts, const ObjectAssign = Object.assign…
Verification: nit — the tamper-resistance concern is real per REVIEW.md ("Built-in JS modules (src/js/) are hot-path code in a hostile environment... globals captured at module load") and widespread repo convention (src/js/internal/util/inspect.js:83-95, src/js/node/vm.ts:24-25, src/js/internal/assert/assertion_error.ts:10-12, etc. all do const ObjectAssign = Object.assign;), but the concrete…
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| /// ROLLBACK statements in flight after a callback threw. While non-zero, a | ||
| /// disconnect leaves the rejection to the code awaiting the ROLLBACK, which | ||
| /// attaches the callback error as `cause`. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// Returns `error` with `cause` attached. A dropped connection rejects every | ||
| /// waiter with one shared error object, so the cause goes on a copy. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
This branch has not been deployed
2 participants
Problem
sql.begin()throws and theROLLBACKthen fails too, the rejection is the rollback failure alone (PostgresError: Connection closed,code: "ERR_POSTGRES_CONNECTION_CLOSED",cause: undefined, or the server's25P01). The callback error is lost. Same forsavepoint(), on PostgreSQL and MySQL.onTransactionConnected(src/js/bun/sql.ts:893). It rejects with the rollback error and drops the callback error. When the connection drops duringROLLBACK,onTransactionDisconnectedrejects with the connection error even earlier.Fix
cause. A workingROLLBACKstill rejects with the callback error itself.withCauseputscauseon a copy of the error. A dropped connection rejects every query queued on it with one shared error object, so a mutation would leakcauseinto unrelated rejections.rollbackscounter makesonTransactionDisconnectedleave the rejection to the code that awaits theROLLBACK, which knows the callback error. A counter, because savepoints can run concurrently.test/js/sql/sql-transaction-rollback-failure.test.ts(mock PostgreSQL and MySQL peers, 10 of 12 tests fail on bun 1.4.3). Also ransql-pool-transaction-isolation,sqlite-sql,sql-reserve-abort. Self-reviewed: 1 concern raised (see Notes), 0 defects.Background
sql.begin(fn)reserves a connection, sendsBEGIN, runsfn, thenCOMMIT, orROLLBACKwhenfnthrows.onTransactionConnectedis that runner for every adapter.onTransactionDisconnectedis the close handler the runner attaches to the connection. It rejects the in-flight queries and thebegin()promise.Notes
cause, so code that matches one.codesees no change. The two PRs conflict on a few lines. Either merge order is easy to resolve.withCauseisnew Error(message, { cause })with the original's prototype and own enumerable properties (name,code,errno, ...), soinstanceof SQL.PostgresErrorand.codekeep working.ROLLBACK TO SAVEPOINTis in flight,begin()now rejects when the callback finishes instead of at once, because the savepoint error (with itscause) is rethrown into the callback first. A callback that swallows that error and then waits forever on non-SQL work keepsbegin()pending. Any furthertxquery rejects at once with the connection error.OR ROLLBACK), theROLLBACKfails with "cannot rollback - no transaction is active". That error now carries the constraint error ascause. sql(sqlite): detect SQLite's automatic ROLLBACK inside sql.begin() #33749 avoids the failingROLLBACKentirely for that case, which is still the better fix there.--drop-on ROLLBACK,--err-word ROLLBACK:25P01).