sql: roll back when COMMIT itself fails (SQLite SQLITE_BUSY left tx open) by robobun · Pull Request #33748 · oven-sh/bun

robobun · 2026-07-08T10:30:50Z

Repro

import { Database } from "bun:sqlite";
const sql = new Bun.SQL({ adapter: "sqlite", filename: dbPath });
await sql`create table t (v text)`;
await sql`insert into t values ('seed')`;

// second connection holds a read lock so COMMIT on the writer returns SQLITE_BUSY
const reader = new Database(dbPath);
reader.run("BEGIN DEFERRED");
reader.prepare("select count(*) c from t").get();

await sql.begin(async tx => { await tx`insert into t values ('in-tx')`; })
  .catch(e => console.log(String(e)));  // SQLiteError: database is locked

reader.run("ROLLBACK"); reader.close();

await sql`insert into t values ('later')`;               // fulfills, but into the abandoned tx
new Database(dbPath).prepare("select * from t").all();   // SQLiteError: database is locked
await sql.begin(async tx => {});                         // SQLiteError: cannot start a transaction within a transaction
await sql.close();                                       // 'later' is gone

One SQLITE_BUSY collision at COMMIT time wedges the instance permanently: later writes on it fulfill but are never committed, every other connection to the file gets database is locked, and every later sql.begin() rejects. Reproduces on 1.4.0 and main with default options (journal_mode=delete, no busy_timeout).

Cause

onTransactionConnected in src/js/bun/sql.ts cleared needs_rollback = false before awaiting COMMIT:

needs_rollback = false;            // cleared too early
if (BEFORE_COMMIT_OR_ROLLBACK_COMMAND) { ... }
await run_internal_transaction_sql(COMMIT_COMMAND);  // throws SQLITE_BUSY

so the catch path issued no ROLLBACK. Postgres and MySQL destroy the transaction server-side when COMMIT fails, so the missing ROLLBACK was a no-op there; SQLite keeps the transaction open on a busy COMMIT by design (so the caller can retry), and the pooled connection was returned still inside it. bun:sqlite's own db.transaction() helper already handles this correctly (it checks db.inTransaction after a failure).

Fix

Clear needs_rollback only after COMMIT succeeds; null BEFORE_COMMIT_OR_ROLLBACK_COMMAND once it has run so the error path does not re-issue MySQL's XA END if XA PREPARE fails. ROLLBACK after a failed COMMIT is a no-op warning on Postgres/MySQL and the required cleanup on SQLite.
The cleanup ROLLBACK (and the sibling ROLLBACK TO SAVEPOINT in run_internal_savepoint) are now best-effort: their own failure is swallowed so sql.begin() / tx.savepoint() always reject with the original error. Previously the inner catch shadowed it, so a COMMIT failure where SQLite had already auto-rolled-back (SQLITE_FULL/IOERR/NOMEM/INTERRUPT) would surface cannot rollback - no transaction is active instead of the real error.

Verification

test/js/sql/sqlite-sql.test.ts gains a test that holds a read lock from a second bun:sqlite connection, forces sql.begin()'s COMMIT to fail with SQLITE_BUSY, releases the lock, then asserts:

later writes on the same instance are visible from a fresh connection (no stale RESERVED lock)
a subsequent sql.begin() succeeds
all post-failure writes survive sql.close()

Fails on the unfixed build with SQLiteError: database is locked at the fresh-connection read; passes with the fix. Also reduced the unrelated properly finalizes prepared statements test to 1,000 iterations under debug builds (it was timing out under ASAN on main).

The adapter-generic transaction runner cleared needs_rollback before awaiting COMMIT, so when COMMIT failed the error path issued no ROLLBACK. Postgres and MySQL destroy the transaction server-side on a failed COMMIT so the missing ROLLBACK was a no-op there, but SQLite leaves the transaction open (e.g. SQLITE_BUSY when another connection holds a read lock). The pooled connection was then returned still inside an open transaction: later writes on the instance fulfilled but were silently discarded at close, every other connection hit 'database is locked', and every later sql.begin() rejected with 'cannot start a transaction within a transaction'. Clear needs_rollback only after COMMIT succeeds, and null out BEFORE_COMMIT_OR_ROLLBACK once it has run so the error path does not re-issue it (MySQL XA END).

robobun · 2026-07-08T10:31:03Z

^{Updated 8:48 AM PT - Jul 8th, 2026}

❌ @robobun, your commit c830b33 has 1 failures in Build #70484 (All Failures):

test/regression/issue/26030.test.ts - code 1 on 🐧 3.23 x64-baseline

🧪 To try this PR locally:

bunx bun-pr 33748

That installs a local version of the PR into your bun-33748 executable, so you can run:

bun-33748 --bun

github-actions · 2026-07-08T10:34:47Z

Found 2 issues this PR may fix:

SQL: connection pool stalls under concurrent begin() + parameterized queries (idle connections never handed to pending queries) #32004 - PR fixes the root cause of connections left "idle in transaction" after sql.begin() by ensuring ROLLBACK is issued when COMMIT fails, preventing wedged pool connections
Bun.SQL: transaction.close({ timeout }) commits instead of rolling back when pending queries drain before the timeout #32148 - PR modifies the same onTransactionConnected function in sql.ts, fixing needs_rollback tracking and BEFORE_COMMIT_OR_ROLLBACK_COMMAND cleanup that contribute to incorrect commit-vs-rollback behavior

If this is helpful, copy the block below into the PR description to auto-close these issues on merge.

Fixes #32004
Fixes #32148

🤖 Generated with Claude Code

robobun · 2026-07-08T10:35:22Z

Neither suggested issue is fixed here:

SQL: connection pool stalls under concurrent begin() + parameterized queries (idle connections never handed to pending queries) #32004 is a pool scheduling stall (idle connections not handed to pending queries under concurrent begin()); this PR only changes where needs_rollback is cleared and doesn't touch connection selection.
Bun.SQL: transaction.close({ timeout }) commits instead of rolling back when pending queries drain before the timeout #32148 is about transaction.close({ timeout }) committing when queries drain before the timer; that code path is untouched here and is already addressed by sql: make transaction.close({ timeout }) roll back when pending queries drain before the timeout #32149.

This PR is scoped to the COMMIT-fails path (SQLite SQLITE_BUSY leaving the transaction open).

coderabbitai · 2026-07-08T10:35:31Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 7ceb8ab0-c19d-4533-b8c7-ac18d56bff1a

📥 Commits

Reviewing files that changed from the base of the PR and between a0e083b and 1d2aa83.

📒 Files selected for processing (1)

src/js/bun/sql.ts

Walkthrough

Transaction commit and rollback handling in src/js/bun/sql.ts now preserves the original error through cleanup, tolerates missing savepoints during rollback, and updates commit-state flags only after success. The SQLite test suite adds a locked-database regression test and adjusts one loop bound for debug mode.

Changes

Transaction cleanup behavior

Layer / File(s)	Summary
Commit and cleanup ordering `src/js/bun/sql.ts`	`BEFORE_COMMIT_OR_ROLLBACK_COMMAND` now runs before `COMMIT_COMMAND`, is cleared afterward, `needs_rollback` flips to `false` only after commit succeeds, and rollback cleanup failures no longer replace the original transaction error.
Savepoint rollback fallback `src/js/bun/sql.ts`	`run_internal_savepoint` now treats rollback-to-savepoint as best-effort and ignores failures when the savepoint is already gone.
SQLite regression test and loop bound `test/js/sql/sqlite-sql.test.ts`	A new file-backed SQLite locking regression test verifies rollback, follow-up writes, and durable visibility across connections, and the prepared-statement loop count now depends on `isDebug`.

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly summarizes the main change: fixing SQLite transaction rollback when COMMIT fails.
Description check	✅ Passed	The description covers the problem, fix, repro, and verification, satisfying the template's intent despite different headings.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{Comment @coderabbitai help to get the list of available commands.}

coderabbitai

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/js/bun/sql.ts (1)
764-784: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Don't retry BEFORE_COMMIT_OR_ROLLBACK before rollback src/js/bun/sql.ts:764-779

If run_internal_transaction_sql(BEFORE_COMMIT_OR_ROLLBACK_COMMAND) throws on the first attempt, the catch block runs it again and can return before ROLLBACK_COMMAND, leaving the MySQL distributed transaction open. Clear or guard that hook after the first attempt so rollback still runs.
🤖 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 764 - 784, The transaction cleanup path in
run_internal_transaction_sql is retrying BEFORE_COMMIT_OR_ROLLBACK_COMMAND
inside the catch block, which can short-circuit before ROLLBACK_COMMAND and
leave the distributed transaction open. Update the commit/rollback flow in
src/js/bun/sql.ts so the BEFORE_COMMIT_OR_ROLLBACK_COMMAND hook is only
attempted once (or is cleared/guarded immediately after the first attempt), and
ensure the catch path always proceeds to rollback when needs_rollback is still
true.

🤖 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 764-784: The transaction cleanup path in
run_internal_transaction_sql is retrying BEFORE_COMMIT_OR_ROLLBACK_COMMAND
inside the catch block, which can short-circuit before ROLLBACK_COMMAND and
leave the distributed transaction open. Update the commit/rollback flow in
src/js/bun/sql.ts so the BEFORE_COMMIT_OR_ROLLBACK_COMMAND hook is only
attempted once (or is cleared/guarded immediately after the first attempt), and
ensure the catch path always proceeds to rollback when needs_rollback is still
true.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 5e5d770f-e9b1-4e93-a907-dbe3ed7c9e5e

📥 Commits

Reviewing files that changed from the base of the PR and between 332f744 and e86706b.

📒 Files selected for processing (2)

src/js/bun/sql.ts
test/js/sql/sqlite-sql.test.ts

robobun · 2026-07-08T10:36:34Z

Re the CodeRabbit note on re-running BEFORE_COMMIT_OR_ROLLBACK_COMMAND when it throws:

That hook is only set for MySQL distributed transactions (XA END '<name>'); it is null for SQLite/Postgres/regular MySQL. On main, needs_rollback was already false before XA END ran, so if XA END threw the catch did nothing and the connection went back to the pool in ACTIVE XA state with no cleanup attempted. After this change the catch re-attempts XA END: if it succeeds the second time XA ROLLBACK runs (strictly better), and if it fails again the user sees the same XA END error they would have seen on main. No regression from main.

Nulling the hook before the first attempt would instead make the catch skip XA END and go straight to XA ROLLBACK, which fails with XAER_RMFAIL on an ACTIVE xid and masks the original error, so I've left it as a retry. Scoping this PR to the COMMIT-fail path.

The test inserts 10,000 rows one by one; under debug+ASAN builds this exceeds the default per-test timeout. Use 1,000 iterations there.

Now that a failed COMMIT routes through the cleanup ROLLBACK, a COMMIT failure where the engine already tore the transaction down (SQLite SQLITE_FULL/IOERR/NOMEM/INTERRUPT) would have the ROLLBACK fail with 'cannot rollback - no transaction is active' and that would mask the real error via the shadowing inner catch. The ROLLBACK is best-effort cleanup; always reject with the original error. Also use 'using' for the bun:sqlite handles in the new test so a failed assertion cannot leak the reader fd (and block rm on Windows).

Always reject with the original transaction error. The inner catch previously shadowed it, so a COMMIT failure where the engine already rolled back (SQLite SQLITE_FULL/IOERR/NOMEM/INTERRUPT) would surface 'cannot rollback - no transaction is active' instead of the real error.

coderabbitai

Actionable comments posted: 1

🤖 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`:
- Line 2074: The finalize-loop workload in the sqlite-sql test is being reduced
in debug builds instead of extending the test timeout, which weakens coverage of
the statement-finalize path. In the test around the iterations constant, keep
the 10000-iteration workload unchanged and adjust this test’s own isDebug-scaled
timeout to accommodate debug/ASAN slowdown, so the full finalize path remains
exercised in both modes.

🪄 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: e08a9d22-1fac-425a-b833-df1550bccf89

📥 Commits

Reviewing files that changed from the base of the PR and between e86706b and a0e083b.

📒 Files selected for processing (1)

test/js/sql/sqlite-sql.test.ts

…rror path Same treatment as the transaction-level catch: if the engine already tore the whole transaction down (SQLite SQLITE_FULL/IOERR/NOMEM/INTERRUPT) the savepoint no longer exists and ROLLBACK TO SAVEPOINT throws 'no such savepoint', masking the original error. Always rethrow the original.

claude

All prior feedback addressed and I don't see anything further — deferring to a human for the final sign-off since this changes shared transaction error-path semantics across all three SQL adapters.

Extended reasoning...

Overview

This PR fixes onTransactionConnected in src/js/bun/sql.ts so that when COMMIT itself fails (notably SQLite SQLITE_BUSY), the pooled connection is rolled back rather than returned to the pool still inside an open transaction. The core change is moving needs_rollback = false from before the await COMMIT to after it. Follow-up commits (all in response to review feedback) additionally:

Null BEFORE_COMMIT_OR_ROLLBACK_COMMAND after it runs so the error path doesn't re-issue MySQL's XA END when XA PREPARE fails.
Swallow best-effort ROLLBACK / ROLLBACK TO SAVEPOINT failures in the catch blocks so the caller always sees the original error rather than a secondary "cannot rollback - no transaction is active" / "no such savepoint" when SQLite has already auto-rolled-back.
Add a regression test in test/js/sql/sqlite-sql.test.ts that forces SQLITE_BUSY at COMMIT via a second bun:sqlite connection holding a read lock, then verifies later writes are durable, a fresh connection can read, and a subsequent begin() works.
Reduce an unrelated finalize-loop test's iteration count under isDebug.

Security risks

None. No auth, crypto, permissions, or untrusted-input parsing is touched. The change is confined to transaction lifecycle bookkeeping in the built-in SQL client.

Level of scrutiny

Moderate-to-high. The diff is small (~15 logic lines) and the mechanism is clearly correct — clearing needs_rollback before COMMIT was an ordering bug, and the SQLite docs explicitly document that a busy COMMIT leaves the transaction open. However, this is the shared commit/rollback error path for all three adapters (Postgres, MySQL, SQLite), and the follow-up changes alter error-reporting semantics: the cleanup ROLLBACK's own failure is now silently swallowed rather than rejected. The PR discussion reasons through each adapter's behavior (Postgres/MySQL treat ROLLBACK-outside-transaction as a warning/no-op; MySQL XA's BEFORE_COMMIT_OR_ROLLBACK_COMMAND interaction is no worse than main), and I agree with that analysis — but transaction error-path correctness has data-integrity implications, so a maintainer familiar with the SQL subsystem should confirm.

Other factors

All three of my prior inline nits (inner-catch shadowing, using for Database handles, the sibling savepoint catch) were addressed in follow-up commits and the threads are resolved.
The CodeRabbit suggestion on debug iteration count was reasonably declined per repo convention.
The new test is hermetic (temp dir, no network, using disposal for all bun:sqlite handles), asserts durable state from a fresh connection rather than the connection under test, and has no timing dependencies.
No CODEOWNERS entry covers these paths.
The bug-hunting system found no issues in the current revision.

robobun · 2026-07-08T12:34:33Z

CI status: the diff itself is green. test/js/sql/sqlite-sql.test.ts (the only file this PR touches in the test suite) passes on every lane across both builds 70474 and 70484.

The remaining red on 70484 is unrelated infra/flake:

test/regression/issue/26030.test.ts on alpine x64-baseline: the MySQL docker container failed its health check (application not healthy after 1m0s) before the test body ran.
Everything else is in the flaky annotation and passed on retry (install, napi, webview, http serve, spawn leak thresholds, no-orphans timing, etc.), none of it SQL-related.

Ready for review.

github-actions Bot added the claude label Jul 8, 2026

robobun mentioned this pull request Jul 8, 2026

sql(sqlite): detect SQLite's automatic ROLLBACK inside sql.begin() #33749

Open

coderabbitai Bot reviewed Jul 8, 2026

View reviewed changes

test(sql/sqlite): reduce stmt-finalize iterations under debug builds

4800afd

The test inserts 10,000 rows one by one; under debug+ASAN builds this exceeds the default per-test timeout. Use 1,000 iterations there.

claude Bot reviewed Jul 8, 2026

View reviewed changes

Comment thread src/js/bun/sql.ts

Comment thread test/js/sql/sqlite-sql.test.ts Outdated

robobun added 2 commits July 8, 2026 10:57

coderabbitai Bot reviewed Jul 8, 2026

View reviewed changes

Comment thread test/js/sql/sqlite-sql.test.ts

claude Bot reviewed Jul 8, 2026

View reviewed changes

Comment thread src/js/bun/sql.ts

claude Bot reviewed Jul 8, 2026

View reviewed changes

ci: retrigger

c830b33

claude Bot reviewed Jul 8, 2026

View reviewed changes

Comment thread src/js/bun/sql.ts

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

sql: roll back when COMMIT itself fails (SQLite SQLITE_BUSY left tx open)#33748

sql: roll back when COMMIT itself fails (SQLite SQLITE_BUSY left tx open)#33748
robobun wants to merge 6 commits into
mainfrom
claude/farm/31d98df8/sql-sqlite-commit-busy-rollback

robobun commented Jul 8, 2026 •

edited

Loading

Uh oh!

robobun commented Jul 8, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Jul 8, 2026

Uh oh!

robobun commented Jul 8, 2026

Uh oh!

coderabbitai Bot commented Jul 8, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot left a comment

Uh oh!

robobun commented Jul 8, 2026

Uh oh!

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

Uh oh!

claude Bot left a comment

Uh oh!

Uh oh!

robobun commented Jul 8, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

robobun commented Jul 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Repro

Cause

Fix

Verification

Uh oh!

robobun commented Jul 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Jul 8, 2026

Uh oh!

robobun commented Jul 8, 2026

Uh oh!

coderabbitai Bot commented Jul 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

robobun commented Jul 8, 2026

Uh oh!

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

claude Bot left a comment

Choose a reason for hiding this comment

Overview

Security risks

Level of scrutiny

Other factors

Uh oh!

Uh oh!

robobun commented Jul 8, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

robobun commented Jul 8, 2026 •

edited

Loading

robobun commented Jul 8, 2026 •

edited

Loading

coderabbitai Bot commented Jul 8, 2026 •

edited

Loading