sql(sqlite): detect SQLite's automatic ROLLBACK inside sql.begin()#33749
sql(sqlite): detect SQLite's automatic ROLLBACK inside sql.begin()#33749robobun wants to merge 7 commits into
Conversation
SQLite's ON CONFLICT ROLLBACK, INSERT OR ROLLBACK, and RAISE(ROLLBACK) roll the whole transaction back from inside a statement. The adapter-generic transaction runner in Bun.SQL never consulted the connection's actual transaction state, so after such an auto-rollback: - if the callback didn't catch the statement error, the runner's ROLLBACK failed with "cannot rollback - no transaction is active" and that bookkeeping error replaced the real SQLITE_CONSTRAINT the caller needed to see; and - if the callback did catch it, the rest of the callback ran in autocommit (each statement individually durable), then COMMIT failed, so a rejected sql.begin() left committed rows behind. Add an optional isConnectionInTransaction() to the adapter interface and implement it for SQLite via db.inTransaction (sqlite3_get_autocommit). The transaction runner now: - rejects every subsequent tx statement with ERR_SQLITE_INVALID_TRANSACTION_STATE once SQLite has dropped the transaction, preserving atomicity; - skips the impossible COMMIT/ROLLBACK/RELEASE SAVEPOINT/ROLLBACK TO SAVEPOINT in that state; and - always rejects with the callback's original error rather than a failure from its own cleanup SQL. The hook is optional and unimplemented for Postgres/MySQL, so their behaviour is unchanged.
|
Updated 5:54 AM PT - Jul 8th, 2026
⏳ @robobun, your commit bbcd708 is still building in
|
|
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 (2)
WalkthroughAdds SQLite transaction auto-rollback detection to the SQL runtime. Transaction execution now checks connection state after conflicts, skips or rejects work after rollback, and updates cleanup paths accordingly. The adapter interface exposes transaction-state checking, and tests cover rollback-on-conflict cases. ChangesAuto-rollback detection and handling
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![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 (3)
src/js/bun/sql.ts (3)
604-612: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRe-check transaction state after the async file read.
tx.file()checks beforeBun.file(path).text(), but another in-flight statement can auto-rollback while the file is being read; the.thenthen still enqueuesunsafeQueryFromTransaction, potentially running file SQL outside the aborted transaction.Proposed fix
transaction_sql.file = async (path: string, args = []) => { if (needs_rollback && !transaction_still_open()) { throw transaction_aborted_error(); } - return await Bun.file(path) - .text() - .then(text => { - return unsafeQueryFromTransaction(text, args, pooledConnection, state.queries); - }); + const text = await Bun.file(path).text(); + if (needs_rollback && !transaction_still_open()) { + throw transaction_aborted_error(); + } + return unsafeQueryFromTransaction(text, args, pooledConnection, state.queries); };🤖 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 `@src/js/bun/sql.ts` around lines 604 - 612, Re-check the transaction state in transaction_sql.file after Bun.file(path).text() resolves, because the initial needs_rollback/transaction_still_open() guard can become stale while the file is being read. Update the transaction_sql.file path so the async continuation verifies the transaction is still open before calling unsafeQueryFromTransaction, and abort with transaction_aborted_error() if the transaction was auto-rolled back during the file read.
731-738: 🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick winReject
savepoint()once the transaction has auto-rolled back.
A swallowedOR ROLLBACKcan leave SQLite in autocommit, and this path still issuesSAVEPOINTbefore checkingneeds_rollback. That can run the savepoint body and let later writes be committed by the outer transaction. Add a regression coveringbegin()→ swallowed auto-rollback →tx.savepoint(...); the body should not run and nothing should persist.🤖 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 `@src/js/bun/sql.ts` around lines 731 - 738, The savepoint flow in run_internal_savepoint currently creates the SAVEPOINT before checking whether the surrounding transaction has already auto-rolled back, which can let tx.savepoint(...) execute inside an autocommit state after a swallowed OR ROLLBACK. Move the rollback-state guard to the start of run_internal_savepoint (before run_internal_transaction_sql and before invoking savepoint_callback) so savepoints are rejected once needs_rollback indicates the transaction is no longer usable, and add a regression test around begin() → swallowed auto-rollback → tx.savepoint(...) to verify the body never runs and no writes persist.
794-802: 🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick winKeep
needs_rollbackarmed until afterCOMMIT. Clearing it beforeBEFORE_COMMIT_OR_ROLLBACK_COMMAND/COMMIT_COMMANDlets either failure skip the rollback path and return a connection that may still be inside a failed transaction. Move the reset to afterCOMMIT.src/js/bun/sql.ts:794-810🤖 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 `@src/js/bun/sql.ts` around lines 794 - 802, Keep needs_rollback set until the transaction has fully committed, since clearing it before BEFORE_COMMIT_OR_ROLLBACK_COMMAND and COMMIT_COMMAND can bypass rollback handling if either call fails. Update the commit flow in the transaction logic around transaction_aborted_error, run_internal_transaction_sql, and COMMIT_COMMAND so needs_rollback is only reset after COMMIT completes successfully, preserving rollback safety if the pre-commit hook or commit itself throws.
🤖 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 `@src/js/bun/sql.ts`:
- Around line 604-612: Re-check the transaction state in transaction_sql.file
after Bun.file(path).text() resolves, because the initial
needs_rollback/transaction_still_open() guard can become stale while the file is
being read. Update the transaction_sql.file path so the async continuation
verifies the transaction is still open before calling
unsafeQueryFromTransaction, and abort with transaction_aborted_error() if the
transaction was auto-rolled back during the file read.
- Around line 731-738: The savepoint flow in run_internal_savepoint currently
creates the SAVEPOINT before checking whether the surrounding transaction has
already auto-rolled back, which can let tx.savepoint(...) execute inside an
autocommit state after a swallowed OR ROLLBACK. Move the rollback-state guard to
the start of run_internal_savepoint (before run_internal_transaction_sql and
before invoking savepoint_callback) so savepoints are rejected once
needs_rollback indicates the transaction is no longer usable, and add a
regression test around begin() → swallowed auto-rollback → tx.savepoint(...) to
verify the body never runs and no writes persist.
- Around line 794-802: Keep needs_rollback set until the transaction has fully
committed, since clearing it before BEFORE_COMMIT_OR_ROLLBACK_COMMAND and
COMMIT_COMMAND can bypass rollback handling if either call fails. Update the
commit flow in the transaction logic around transaction_aborted_error,
run_internal_transaction_sql, and COMMIT_COMMAND so needs_rollback is only reset
after COMMIT completes successfully, preserving rollback safety if the
pre-commit hook or commit itself throws.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 802eff94-9c34-4911-9e3a-379ea4b4d326
📒 Files selected for processing (4)
src/js/bun/sql.tssrc/js/internal/sql/shared.tssrc/js/internal/sql/sqlite.tstest/js/sql/sqlite-sql.test.ts
…-rollback Follow-up to the auto-rollback detection: tx.savepoint() called after a swallowed ON CONFLICT ROLLBACK would issue SAVEPOINT in autocommit, which starts an implicit SQLite transaction, so the savepoint body ran and RELEASE SAVEPOINT committed it. Guard the savepoint entry point with the same needs_rollback/transaction_still_open() check used by the other tx paths, and re-check in tx.file() after the async file read resolves.
|
Addressed the review in 65de527:
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/js/bun/sql.ts (2)
713-719: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winApply the same rollback guard to timeout cleanup.
The immediate
close()path skips rollback when SQLite has already auto-rolled back, but the timeout callback above still unconditionally runsBEFORE_COMMIT_OR_ROLLBACK_COMMANDandROLLBACK_COMMAND. That parallel cleanup path can still hit “no transaction is active” and leave the timeout cleanup in a rejected async callback. As per coding guidelines, “Fix the whole class in the same PR” across parallel cleanup paths.🤖 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 `@src/js/bun/sql.ts` around lines 713 - 719, The timeout cleanup path in `run_internal_transaction_sql` still unconditionally issues the pre-rollback hook and `ROLLBACK_COMMAND`, unlike the immediate `close()` path that first checks `transaction_still_open()`. Add the same guard to the timeout callback so it only runs `BEFORE_COMMIT_OR_ROLLBACK_COMMAND` and `ROLLBACK_COMMAND` when the transaction is still open, preventing “no transaction is active” rejections in the parallel cleanup path.Source: Coding guidelines
735-745: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winValidate savepoint state after resolving array results.
Line 736 checks for SQLite auto-rollback before
Promise.all(result), so a savepoint callback that returns an array of transaction queries can auto-rollback while those queries settle and still proceed towardRELEASE SAVEPOINT. Mirror the outer transaction flow by awaiting array results before the aborted-transaction check and release.Proposed fix
let result = await savepoint_callback(transaction_sql); + if ($isArray(result)) { + result = await Promise.all(result); + } if (needs_rollback && !transaction_still_open()) { throw transaction_aborted_error(); } if (RELEASE_SAVEPOINT_COMMAND) { // mssql dont have release savepoint await run_internal_transaction_sql(`${RELEASE_SAVEPOINT_COMMAND} ${save_point_name}`); } - if ($isArray(result)) { - result = await Promise.all(result); - } return result;🤖 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 `@src/js/bun/sql.ts` around lines 735 - 745, The savepoint flow in the transaction handler is checking for SQLite auto-rollback before resolving array results, which can let a savepoint callback return transaction queries and still proceed toward releasing the savepoint after rollback. In the transaction/savepoint logic around savepoint_callback, transaction_still_open, and RELEASE_SAVEPOINT_COMMAND, await Promise.all(result) first when $isArray(result) is true, then perform the needs_rollback and transaction_still_open check, and only release the savepoint if the transaction is still valid.
🤖 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 `@test/js/sql/sqlite-sql.test.ts`:
- Around line 1269-1294: The test for tx.savepoint after auto-rollback should
also assert the inner INSERT failure, not just the outer begin error. Update the
anonymous async callback in this sqlite-sql test to capture the error from the
triggering tx`INSERT INTO t VALUES ('dup')` and assert its code before
continuing, so the case is verified to follow the intended ON CONFLICT ROLLBACK
path. Keep the existing beginErr and bodyRan checks, but add an inner error
variable in the same test block to validate the statement-level failure
alongside tx.savepoint.
---
Outside diff comments:
In `@src/js/bun/sql.ts`:
- Around line 713-719: The timeout cleanup path in
`run_internal_transaction_sql` still unconditionally issues the pre-rollback
hook and `ROLLBACK_COMMAND`, unlike the immediate `close()` path that first
checks `transaction_still_open()`. Add the same guard to the timeout callback so
it only runs `BEFORE_COMMIT_OR_ROLLBACK_COMMAND` and `ROLLBACK_COMMAND` when the
transaction is still open, preventing “no transaction is active” rejections in
the parallel cleanup path.
- Around line 735-745: The savepoint flow in the transaction handler is checking
for SQLite auto-rollback before resolving array results, which can let a
savepoint callback return transaction queries and still proceed toward releasing
the savepoint after rollback. In the transaction/savepoint logic around
savepoint_callback, transaction_still_open, and RELEASE_SAVEPOINT_COMMAND, await
Promise.all(result) first when $isArray(result) is true, then perform the
needs_rollback and transaction_still_open check, and only release the savepoint
if the transaction is still valid.
🪄 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: 2cfcfe25-46fa-483f-8ba9-eaeb545b1487
📒 Files selected for processing (2)
src/js/bun/sql.tstest/js/sql/sqlite-sql.test.ts
…fore RELEASE
Mirror the transaction_still_open() guard in the close({timeout}) timer
callback, and await an array result returned from a savepoint callback
before the aborted-transaction check and RELEASE SAVEPOINT so the check
sees the result of those queries. Also assert the triggering conflict's
error code in the savepoint test.
|
Applied the second-round review in 54180eb:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🔴
src/js/bun/sql.ts:733-738—tx.savepoint()is missing the sameneeds_rollback && !transaction_still_open()guard that was added totx\...`,tx.unsafe(), andtx.file(). After a caught auto-rollback,SAVEPOINT s0runs in autocommit and (per SQLite docs) acts likeBEGIN DEFERRED, sodb.inTransactionflips back totrue, every subsequent guard passes, andRELEASE SAVEPOINT s0acts likeCOMMIT— durably committing the savepoint callback's writes even thoughsql.begin()then rejects. Add the guard at the top ofrun_internal_savepoint(or intransaction_sql.savepoint) before issuingSAVEPOINT`.Extended reasoning...
What the bug is
The PR guards three of the four user-callable
tx.*entry points against SQLite's automatic rollback:transaction_sql(tagged-template),transaction_sql.unsafe, andtransaction_sql.fileall now checkneeds_rollback && !transaction_still_open()before running SQL. The fourth sibling —transaction_sql.savepoint/run_internal_savepoint— has no such guard before it issuesSAVEPOINT. It only checksstate.connectionState, andrun_internal_transaction_sql(which actually sendsSAVEPOINT s0) also has no guard. The new check insiderun_internal_savepointat line 736 runs only after the savepoint callback returns.The specific code path
await sql`CREATE TABLE t (v TEXT PRIMARY KEY ON CONFLICT ROLLBACK)`; await sql`CREATE TABLE log (v TEXT)`; await sql`INSERT INTO t VALUES ('dup')`; await sql.begin(async tx => { try { await tx`INSERT INTO t VALUES ('dup')`; } catch {} // SQLite auto-ROLLBACK -> autocommit await tx.savepoint(async sp => { await sp`INSERT INTO log VALUES ('leaked')`; }); });
Why existing code doesn't prevent it
Per the SQLite docs (lang_savepoint.html):
When a SAVEPOINT is the outer-most savepoint and it is not within a BEGIN...COMMIT then the behavior is the same as BEGIN DEFERRED TRANSACTION.
If the RELEASE command releases the outermost savepoint, so that the transaction savepoint stack is empty, then RELEASE is equivalent to COMMIT.
So once
SAVEPOINT s0runs in autocommit,sqlite3_get_autocommit()returns 0 again andconnection.inTransaction === true. Every guard the PR added keys on!transaction_still_open(), andtransaction_still_open()is nowtrue— so the guard onsp\...`` (line 584), the post-callback guard at line 736, and the outer post-callback guard at line 794 are all defeated for the duration of the savepoint.Step-by-step proof
BEGINruns;needs_rollback = true;db.inTransaction = true.INSERT INTO t VALUES ('dup')fails withSQLITE_CONSTRAINT_PRIMARYKEY; SQLite auto-rolls back the whole transaction;db.inTransaction = false. The user catches and continues.tx.savepoint(cb)→transaction_sql.savepointchecks onlyconnectionState(stillacceptQueries, notclosed) →run_internal_savepoint('s0', cb).- First line of
run_internal_savepoint:run_internal_transaction_sql('SAVEPOINT s0').run_internal_transaction_sqlonly checksclosed, so it executes. SQLite is in autocommit, soSAVEPOINT s0acts likeBEGIN DEFERRED— a new transaction opens;db.inTransaction = true. - Inside the callback,
spistransaction_sql. Its guard evaluatesneeds_rollback && !transaction_still_open()→true && !true→false.INSERT INTO log VALUES ('leaked')executes inside the fresh transaction. - Callback returns. Line 736 guard:
true && !true→false, no throw.RELEASE SAVEPOINT s0runs; releasing the outermost savepoint is equivalent toCOMMIT.'leaked'is now durable on disk;db.inTransaction = false. run_internal_savepointreturns; back in the outer runner, line 794's guard evaluatestrue && !false→true, so it throwstransaction_aborted_error()andsql.begin()rejects withERR_SQLITE_INVALID_TRANSACTION_STATE.
Net result:
sql.begin()rejects, yetlogdurably contains'leaked'— exactly case (A) from the PR description ("a rejectedsql.begin()left committed rows behind"), reached via the sibling entry point the PR didn't guard.Impact
This is the same data-integrity failure the PR sets out to eliminate. It's reachable via a documented public API (
tx.savepoint) using the same trigger (ON CONFLICT ROLLBACK/OR ROLLBACK/RAISE(ROLLBACK)) the PR already tests.How to fix
Add the same one-line guard before issuing
SAVEPOINT, e.g. at the top ofrun_internal_savepoint:async function run_internal_savepoint(save_point_name, savepoint_callback) { if (needs_rollback && !transaction_still_open()) { throw transaction_aborted_error(); } await run_internal_transaction_sql(`${SAVEPOINT_COMMAND} ${save_point_name}`); ... }
(or equivalently in
transaction_sql.savepointbefore callingrun_internal_savepoint). Worth adding a test alongside the existing "auto-rollback inside a savepoint" case that catches the auto-rollback before callingtx.savepoint()and assertslogstays empty.On the refutation
One verifier marked bug_004 REFUTED as a duplicate of bug_002 — that's a dedup note, not a disagreement on the substance. All seven substantive verifier passes confirmed the trace and the SQLite semantics it relies on.
…eation The earlier guard in transaction_sql fires when the lazy Query object is constructed. With the array-return form sql.begin(tx => [tx`a`, tx`b`]), both queries are constructed before either executes; if the first triggers ON CONFLICT ROLLBACK, the second was still executing in autocommit and being durably committed even though begin() rejected. Thread an optional preRunGuard callback through queryFromTransaction / unsafeQueryFromTransaction into queryFromTransactionHandler so the transaction-state check runs immediately before handle.run(). The reserved-connection path passes no guard, so its behaviour is unchanged. The guard is null for adapters that don't implement isConnectionInTransaction, so Postgres/MySQL see no extra work. Dropped the post-file-read re-check in tx.file() since the execution-time guard now covers it.
|
On the three claude[bot] findings:
|
…load for debug builds 10k sequential INSERTs exceed the 5s per-test budget under debug+ASAN (observed ~12s on main, unrelated to the auto-rollback change). Use 1k iterations in debug builds so the file runs clean under bun bd.
SAVEPOINT went through run_internal_transaction_sql without preRunGuard, so a concurrent unawaited conflict query could auto-rollback in the microtask between the call-time savepoint() guard and SAVEPOINT actually executing. SAVEPOINT in autocommit acts like BEGIN DEFERRED, which made db.inTransaction true again and let RELEASE SAVEPOINT durably commit the body's writes while begin() still rejected. Pass preRunGuard when issuing SAVEPOINT so the check runs at execution time like user queries already do.
CI statusThe diff is green for everything it touches.
This PR only edits |
1 participant
Reproduction
Cause
ON CONFLICT ROLLBACK,INSERT OR ROLLBACK, andRAISE(ROLLBACK)make SQLite roll the whole transaction back from inside a statement. The adapter-generic runner inonTransactionConnected(src/js/bun/sql.ts) never checked the connection's real transaction state, so after an auto-rollback:ROLLBACK, which failed with "cannot rollback - no transaction is active"; that error then shadowed the callback's real error because the inner catch rebounderr;COMMITthen failed, so a rejectedsql.begin()left committed rows behind.bun:sqlite's owndb.transaction()already guards ondb.inTransactionfor exactly this case; theBun.SQLrunner did not.Fix
Add an optional
isConnectionInTransaction(connection)hook to the adapter interface and implement it for SQLite viadb.inTransaction(sqlite3_get_autocommit). In the runner:needs_rollbackis set and the adapter reports the transaction is gone, every furthertx\...`/tx.unsafe()/tx.file()rejects withERR_SQLITE_INVALID_TRANSACTION_STATE` instead of executing in autocommit;COMMIT/ pre-RELEASE SAVEPOINTpaths make the same check so a callback that swallows everything still fails;ROLLBACKandROLLBACK TO SAVEPOINTare skipped when the adapter says there is nothing to roll back;The hook is optional; Postgres and MySQL don't implement it, so their behaviour is unchanged. Related but distinct from #33748 (COMMIT fails with
SQLITE_BUSYand leaves the transaction open); the two fixes are complementary.Verification