From e86706b77dca0658ca5cac2d6c6016f947f42ee8 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Wed, 8 Jul 2026 10:30:15 +0000 Subject: [PATCH 1/6] sql: roll back when COMMIT itself fails 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). --- src/js/bun/sql.ts | 6 ++-- test/js/sql/sqlite-sql.test.ts | 57 ++++++++++++++++++++++++++++++++++ 2 files changed, 61 insertions(+), 2 deletions(-) diff --git a/src/js/bun/sql.ts b/src/js/bun/sql.ts index c3a74582a259..6e89a8393b9c 100644 --- a/src/js/bun/sql.ts +++ b/src/js/bun/sql.ts @@ -761,12 +761,14 @@ const SQL: typeof Bun.SQL = function SQL( if ($isArray(transaction_result)) { transaction_result = await Promise.all(transaction_result); } - // at this point we dont need to rollback anymore - needs_rollback = false; if (BEFORE_COMMIT_OR_ROLLBACK_COMMAND) { await run_internal_transaction_sql(BEFORE_COMMIT_OR_ROLLBACK_COMMAND); + BEFORE_COMMIT_OR_ROLLBACK_COMMAND = null; } await run_internal_transaction_sql(COMMIT_COMMAND); + // COMMIT succeeded; only now is it safe to skip ROLLBACK on the error path. + // SQLite leaves the transaction OPEN when COMMIT fails (e.g. SQLITE_BUSY). + needs_rollback = false; return resolve(transaction_result); } catch (err) { try { diff --git a/test/js/sql/sqlite-sql.test.ts b/test/js/sql/sqlite-sql.test.ts index f90ebfe3d191..7e66e5a24bfd 100644 --- a/test/js/sql/sqlite-sql.test.ts +++ b/test/js/sql/sqlite-sql.test.ts @@ -1,4 +1,5 @@ import { randomUUIDv7, SQL } from "bun"; +import { Database } from "bun:sqlite"; import { afterAll, afterEach, beforeAll, beforeEach, describe, expect, mock, test } from "bun:test"; import { tempDirWithFiles } from "harness"; import { existsSync } from "node:fs"; @@ -1144,6 +1145,62 @@ describe("Transactions", () => { const accounts = await sql`SELECT * FROM accounts WHERE id = 1`; expect(accounts[0].balance).toBe(1002); }); + + test("failed COMMIT (SQLITE_BUSY) rolls back instead of leaking an open transaction", async () => { + const dir = tempDirWithFiles("sqlite-commit-busy", {}); + const dbPath = path.join(dir, "busy.db"); + + const fileSql = new SQL({ adapter: "sqlite", filename: dbPath }); + try { + await fileSql`CREATE TABLE t (v TEXT)`; + await fileSql`INSERT INTO t VALUES ('seed')`; + + // Hold a read lock from a second connection 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(); + + const beginError = await fileSql + .begin(async tx => { + await tx`INSERT INTO t VALUES ('in-tx')`; + }) + .then( + () => null, + e => e, + ); + expect(String(beginError)).toContain("database is locked"); + + reader.run("ROLLBACK"); + reader.close(); + + // The connection must no longer be inside the aborted transaction. + await fileSql`INSERT INTO t VALUES ('later-1')`; + await fileSql`INSERT INTO t VALUES ('later-2')`; + + // A fresh connection must be able to read (no stale RESERVED lock). + const other = new Database(dbPath); + const rowsFromOther = other.prepare("SELECT v FROM t ORDER BY v").all() as { v: string }[]; + other.close(); + expect(rowsFromOther.map(r => r.v)).toEqual(["later-1", "later-2", "seed"]); + + // A subsequent begin() on the same instance must work. + const second = await fileSql.begin(async tx => { + await tx`INSERT INTO t VALUES ('later-3')`; + return "ok"; + }); + expect(second).toBe("ok"); + + // All post-failure writes must be durable after close. + await fileSql.close(); + const final = new Database(dbPath); + const finalRows = (final.prepare("SELECT v FROM t ORDER BY v").all() as { v: string }[]).map(r => r.v); + final.close(); + expect(finalRows).toEqual(["later-1", "later-2", "later-3", "seed"]); + } finally { + await fileSql.close().catch(() => {}); + await rm(dir, { recursive: true, force: true }); + } + }); }); describe("SQLite-specific features", () => { From 4800afd69a8e1d169601062501a627f1908c0467 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Wed, 8 Jul 2026 10:41:17 +0000 Subject: [PATCH 2/6] test(sql/sqlite): reduce stmt-finalize iterations under debug builds 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. --- test/js/sql/sqlite-sql.test.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/js/sql/sqlite-sql.test.ts b/test/js/sql/sqlite-sql.test.ts index 7e66e5a24bfd..434d3b571dbf 100644 --- a/test/js/sql/sqlite-sql.test.ts +++ b/test/js/sql/sqlite-sql.test.ts @@ -1,7 +1,7 @@ import { randomUUIDv7, SQL } from "bun"; import { Database } from "bun:sqlite"; import { afterAll, afterEach, beforeAll, beforeEach, describe, expect, mock, test } from "bun:test"; -import { tempDirWithFiles } from "harness"; +import { isDebug, tempDirWithFiles } from "harness"; import { existsSync } from "node:fs"; import { rm, stat } from "node:fs/promises"; import { join } from "node:path"; @@ -2071,7 +2071,7 @@ describe("Memory and resource management", () => { await sql`CREATE TABLE stmt_test (id INTEGER PRIMARY KEY, value TEXT)`; - const iterations = 10000; + const iterations = isDebug ? 1000 : 10000; for (let i = 0; i < iterations; i++) { await sql`INSERT INTO stmt_test (id, value) VALUES (${i}, ${"test" + i})`; From a0e083b451a09666a56f9c3fdb53b541cce5343f Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Wed, 8 Jul 2026 10:57:36 +0000 Subject: [PATCH 3/6] sql: surface the original transaction error when cleanup ROLLBACK fails 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). --- test/js/sql/sqlite-sql.test.ts | 48 +++++++++++++++++----------------- 1 file changed, 24 insertions(+), 24 deletions(-) diff --git a/test/js/sql/sqlite-sql.test.ts b/test/js/sql/sqlite-sql.test.ts index 434d3b571dbf..1ee15deb79f8 100644 --- a/test/js/sql/sqlite-sql.test.ts +++ b/test/js/sql/sqlite-sql.test.ts @@ -1155,33 +1155,34 @@ describe("Transactions", () => { await fileSql`CREATE TABLE t (v TEXT)`; await fileSql`INSERT INTO t VALUES ('seed')`; - // Hold a read lock from a second connection 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(); - - const beginError = await fileSql - .begin(async tx => { - await tx`INSERT INTO t VALUES ('in-tx')`; - }) - .then( - () => null, - e => e, - ); - expect(String(beginError)).toContain("database is locked"); - - reader.run("ROLLBACK"); - reader.close(); + { + // Hold a read lock from a second connection so COMMIT on the writer returns SQLITE_BUSY. + using reader = new Database(dbPath); + reader.run("BEGIN DEFERRED"); + reader.query("SELECT COUNT(*) c FROM t").get(); + + const beginError = await fileSql + .begin(async tx => { + await tx`INSERT INTO t VALUES ('in-tx')`; + }) + .then( + () => null, + e => e, + ); + expect(String(beginError)).toContain("database is locked"); + reader.run("ROLLBACK"); + } // The connection must no longer be inside the aborted transaction. await fileSql`INSERT INTO t VALUES ('later-1')`; await fileSql`INSERT INTO t VALUES ('later-2')`; // A fresh connection must be able to read (no stale RESERVED lock). - const other = new Database(dbPath); - const rowsFromOther = other.prepare("SELECT v FROM t ORDER BY v").all() as { v: string }[]; - other.close(); - expect(rowsFromOther.map(r => r.v)).toEqual(["later-1", "later-2", "seed"]); + { + using other = new Database(dbPath); + const rowsFromOther = other.query("SELECT v FROM t ORDER BY v").all() as { v: string }[]; + expect(rowsFromOther.map(r => r.v)).toEqual(["later-1", "later-2", "seed"]); + } // A subsequent begin() on the same instance must work. const second = await fileSql.begin(async tx => { @@ -1192,9 +1193,8 @@ describe("Transactions", () => { // All post-failure writes must be durable after close. await fileSql.close(); - const final = new Database(dbPath); - const finalRows = (final.prepare("SELECT v FROM t ORDER BY v").all() as { v: string }[]).map(r => r.v); - final.close(); + using final = new Database(dbPath); + const finalRows = (final.query("SELECT v FROM t ORDER BY v").all() as { v: string }[]).map(r => r.v); expect(finalRows).toEqual(["later-1", "later-2", "later-3", "seed"]); } finally { await fileSql.close().catch(() => {}); From 1cde25528f2502966ffcaeba4b923375e866a645 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Wed, 8 Jul 2026 10:58:55 +0000 Subject: [PATCH 4/6] sql: swallow best-effort ROLLBACK failure in transaction error path 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. --- src/js/bun/sql.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/js/bun/sql.ts b/src/js/bun/sql.ts index 6e89a8393b9c..e36faabd9df9 100644 --- a/src/js/bun/sql.ts +++ b/src/js/bun/sql.ts @@ -778,8 +778,9 @@ const SQL: typeof Bun.SQL = function SQL( } await run_internal_transaction_sql(ROLLBACK_COMMAND); } - } catch (err) { - return reject(err); + } catch { + // Best-effort cleanup; the adapter may have already torn the transaction + // down (SQLite SQLITE_FULL/IOERR, Postgres/MySQL on COMMIT failure). } return reject(err); } finally { From 1d2aa83ee50c51e53eb717ed8c4e44ce9cda7614 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Wed, 8 Jul 2026 11:24:17 +0000 Subject: [PATCH 5/6] sql: swallow best-effort ROLLBACK TO SAVEPOINT failure in savepoint error 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. --- src/js/bun/sql.ts | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/js/bun/sql.ts b/src/js/bun/sql.ts index e36faabd9df9..58c545497b1a 100644 --- a/src/js/bun/sql.ts +++ b/src/js/bun/sql.ts @@ -717,7 +717,12 @@ const SQL: typeof Bun.SQL = function SQL( return result; } catch (err) { if (!(state.connectionState & ReservedConnectionState.closed)) { - await run_internal_transaction_sql(`${ROLLBACK_TO_SAVEPOINT_COMMAND} ${save_point_name}`); + try { + await run_internal_transaction_sql(`${ROLLBACK_TO_SAVEPOINT_COMMAND} ${save_point_name}`); + } catch { + // Best-effort; the savepoint may no longer exist if the engine + // already rolled the whole transaction back (SQLite SQLITE_FULL/IOERR). + } } throw err; } From c830b33c310b03967c5530fac1f74646acc1a851 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Wed, 8 Jul 2026 11:57:23 +0000 Subject: [PATCH 6/6] ci: retrigger