Skip to content
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 41 additions & 10 deletions src/js/bun/sql.ts
Original file line number Diff line number Diff line change
Expand Up @@ -551,6 +551,20 @@
pool.attachConnectionCloseHandler(pooledConnection, onClose);
}

// Some databases (SQLite's ON CONFLICT ROLLBACK / OR ROLLBACK / RAISE(ROLLBACK)) can roll the
// transaction back on their own mid-callback. Once that happens the connection is in autocommit
// and any further COMMIT/ROLLBACK would fail with "no transaction is active".
let needs_rollback = false;
const isConnectionInTransaction = pool.isConnectionInTransaction?.bind(pool);
function transaction_still_open() {
return isConnectionInTransaction === undefined || isConnectionInTransaction(pooledConnection);
}
function transaction_aborted_error() {
return pool.invalidTransactionStateError(
"current transaction is aborted (the database already rolled it back), commands ignored until end of transaction",
);
}

function run_internal_transaction_sql(string) {
if (state.connectionState & ReservedConnectionState.closed) {
return Promise.$reject(pool.connectionClosedError());
Expand All @@ -567,6 +581,9 @@
) {
return Promise.$reject(pool.connectionClosedError());
}
if (needs_rollback && !transaction_still_open()) {
return Promise.$reject(transaction_aborted_error());
}

Check failure on line 586 in src/js/bun/sql.ts

View check run for this annotation

Claude / Claude Code Review

Auto-rollback guard runs at Query creation, not execution — array-return begin() still leaks writes

The `transaction_still_open()` guard runs when the lazy `Query` object is **constructed**, not when it executes — so the documented array-return form `sql.begin(tx => [tx`…`, tx`…`])` still leaks autocommit writes. Both queries are built synchronously while `db.inTransaction` is true; `Promise.all` then runs them via `#runAsync()`'s microtask hop, and if the first triggers `ON CONFLICT ROLLBACK` the second executes in autocommit and is durably committed even though `begin()` rejects. The check n
Comment thread
robobun marked this conversation as resolved.
Resolved
Comment thread
robobun marked this conversation as resolved.
Resolved
if ($isArray(strings)) {
// detect if is tagged template
if (!$isArray((strings as unknown as TemplateStringsArray).raw)) {
Expand All @@ -579,9 +596,15 @@
return queryFromTransaction(strings, values, pooledConnection, state.queries);
}
transaction_sql.unsafe = (string, args = []) => {
if (needs_rollback && !transaction_still_open()) {
return Promise.$reject(transaction_aborted_error());
}
return unsafeQueryFromTransaction(string, args, pooledConnection, state.queries);
};
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 => {
Expand Down Expand Up @@ -687,10 +710,13 @@
for (const query of transactionQueries) {
(query as Query<any, any>).cancel();
}
if (BEFORE_COMMIT_OR_ROLLBACK_COMMAND) {
await run_internal_transaction_sql(BEFORE_COMMIT_OR_ROLLBACK_COMMAND);
if (transaction_still_open()) {
if (BEFORE_COMMIT_OR_ROLLBACK_COMMAND) {
await run_internal_transaction_sql(BEFORE_COMMIT_OR_ROLLBACK_COMMAND);
}
Comment thread
claude[bot] marked this conversation as resolved.
Resolved
await run_internal_transaction_sql(ROLLBACK_COMMAND);
}
await run_internal_transaction_sql(ROLLBACK_COMMAND);
needs_rollback = false;

Check warning on line 719 in src/js/bun/sql.ts

View check run for this annotation

Claude / Claude Code Review

close({timeout}) setTimeout branch not guarded by transaction_still_open()

Nit: the `setTimeout` callback in `transaction_sql.close({timeout})` (~15 lines above) still issues `BEFORE_COMMIT_OR_ROLLBACK_COMMAND` / `ROLLBACK` unconditionally, while the non-timeout branch below now guards the identical sequence with `if (transaction_still_open())`. If the timer fires after a SQLite auto-rollback, `await run_internal_transaction_sql('ROLLBACK')` rejects with "cannot rollback - no transaction is active" inside an unobserved async callback (unhandled rejection, and the follo
state.connectionState |= ReservedConnectionState.closed;
};
transaction_sql[Symbol.asyncDispose] = () => transaction_sql.close();
Expand All @@ -707,6 +733,9 @@

try {
let result = await savepoint_callback(transaction_sql);
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}`);
Expand All @@ -716,8 +745,10 @@
}
return result;
} catch (err) {
if (!(state.connectionState & ReservedConnectionState.closed)) {
await run_internal_transaction_sql(`${ROLLBACK_TO_SAVEPOINT_COMMAND} ${save_point_name}`);
if (!(state.connectionState & ReservedConnectionState.closed) && transaction_still_open()) {
try {
await run_internal_transaction_sql(`${ROLLBACK_TO_SAVEPOINT_COMMAND} ${save_point_name}`);
} catch {}
}
throw err;
}
Expand Down Expand Up @@ -753,14 +784,16 @@
return await promise.finally(onSavepointFinished.bind(null, promise));
};
}
let needs_rollback = false;
try {
await run_internal_transaction_sql(BEGIN_COMMAND);
needs_rollback = true;
let transaction_result = await callback(transaction_sql);
if ($isArray(transaction_result)) {
transaction_result = await Promise.all(transaction_result);
}
if (needs_rollback && !transaction_still_open()) {
throw transaction_aborted_error();
}
// at this point we dont need to rollback anymore
needs_rollback = false;
if (BEFORE_COMMIT_OR_ROLLBACK_COMMAND) {
Expand All @@ -770,15 +803,13 @@
return resolve(transaction_result);
} catch (err) {
try {
if (!(state.connectionState & ReservedConnectionState.closed) && needs_rollback) {
if (!(state.connectionState & ReservedConnectionState.closed) && needs_rollback && transaction_still_open()) {
if (BEFORE_COMMIT_OR_ROLLBACK_COMMAND) {
await run_internal_transaction_sql(BEFORE_COMMIT_OR_ROLLBACK_COMMAND);
}
await run_internal_transaction_sql(ROLLBACK_COMMAND);
}
} catch (err) {
return reject(err);
}
} catch {}
return reject(err);
} finally {
state.connectionState |= ReservedConnectionState.closed;
Expand Down
1 change: 1 addition & 0 deletions src/js/internal/sql/shared.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2123,6 +2123,7 @@ export interface DatabaseAdapter<Connection, ConnectionHandle, QueryHandle> {

supportsReservedConnections?(): boolean;
getConnectionForQuery?(pooledConnection: Connection): ConnectionHandle | null;
isConnectionInTransaction?(connection: Connection): boolean;
attachConnectionCloseHandler?(connection: Connection, handler: () => void): void;
detachConnectionCloseHandler?(connection: Connection, handler: () => void): void;

Expand Down
3 changes: 3 additions & 0 deletions src/js/internal/sql/sqlite.ts
Original file line number Diff line number Diff line change
Expand Up @@ -487,6 +487,9 @@ class SQLiteAdapter implements DatabaseAdapter<BunSQLiteModule.Database, BunSQLi
getConnectionForQuery(connection: BunSQLiteModule.Database): BunSQLiteModule.Database {
return connection;
}
isConnectionInTransaction(connection: BunSQLiteModule.Database): boolean {
return connection.inTransaction;
}
array(_values: any[], _typeNameOrID?: number | ArrayType): SQLArrayParameter {
throw new Error("SQLite doesn't support arrays");
}
Expand Down
143 changes: 143 additions & 0 deletions test/js/sql/sqlite-sql.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1144,6 +1144,149 @@ describe("Transactions", () => {
const accounts = await sql`SELECT * FROM accounts WHERE id = 1`;
expect(accounts[0].balance).toBe(1002);
});

describe("SQLite automatic ROLLBACK (ON CONFLICT ROLLBACK / OR ROLLBACK / RAISE(ROLLBACK))", () => {
beforeEach(async () => {
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')`;
});

test("uncaught conflict: begin() rejects with the original statement error, not 'cannot rollback'", async () => {
let caught: any;
try {
await sql.begin(async tx => {
await tx`INSERT INTO t VALUES ('dup')`;
});
} catch (e) {
caught = e;
}
expect({ code: caught?.code, message: String(caught) }).toEqual({
code: "SQLITE_CONSTRAINT_PRIMARYKEY",
message: "SQLiteError: UNIQUE constraint failed: t.v",
});
});

test("caught conflict: later statements are refused and nothing is committed", async () => {
let beginErr: any;
let stmtErr: any;
try {
await sql.begin(async tx => {
await tx`INSERT INTO log VALUES ('first')`;
try {
await tx`INSERT INTO t VALUES ('dup')`;
} catch {}
try {
await tx`INSERT INTO log VALUES ('after-auto-rollback')`;
} catch (e) {
stmtErr = e;
throw e;
}
});
} catch (e) {
beginErr = e;
}
const durable = (await sql`SELECT v FROM log`).map((r: any) => r.v);
expect({
stmtErrCode: stmtErr?.code,
beginErrCode: beginErr?.code,
durable,
}).toEqual({
stmtErrCode: "ERR_SQLITE_INVALID_TRANSACTION_STATE",
beginErrCode: "ERR_SQLITE_INVALID_TRANSACTION_STATE",
durable: [],
});
// the connection must remain usable for a fresh transaction afterwards
await sql.begin(async tx => {
await tx`INSERT INTO log VALUES ('next-tx')`;
});
expect((await sql`SELECT v FROM log`).map((r: any) => r.v)).toEqual(["next-tx"]);
});

test("caught conflict where callback swallows everything: begin() still rejects, COMMIT is not attempted", async () => {
let beginErr: any;
try {
await sql.begin(async tx => {
try {
await tx`INSERT INTO t VALUES ('dup')`;
} catch {}
// every subsequent statement keeps failing; swallowing one must not let the next through
try {
await tx`INSERT INTO log VALUES ('a')`;
} catch {}
try {
await tx`INSERT INTO log VALUES ('b')`;
} catch {}
});
} catch (e) {
beginErr = e;
}
expect({
code: beginErr?.code,
message: String(beginErr),
durable: (await sql`SELECT v FROM log`).map((r: any) => r.v),
}).toEqual({
code: "ERR_SQLITE_INVALID_TRANSACTION_STATE",
message:
"SQLiteError: current transaction is aborted (the database already rolled it back), commands ignored until end of transaction",
durable: [],
});
});

test("INSERT OR ROLLBACK (statement-level conflict clause) behaves the same", async () => {
await sql`CREATE TABLE t2 (v TEXT PRIMARY KEY)`;
await sql`INSERT INTO t2 VALUES ('dup')`;
let caught: any;
try {
await sql.begin(async tx => {
await tx`INSERT OR ROLLBACK INTO t2 VALUES ('dup')`;
});
} catch (e) {
caught = e;
}
expect(caught?.code).toBe("SQLITE_CONSTRAINT_PRIMARYKEY");
});

test("tx.unsafe refuses to run after auto-rollback", async () => {
let stmtErr: any;
await sql
.begin(async tx => {
try {
await tx`INSERT INTO t VALUES ('dup')`;
} catch {}
await tx.unsafe("INSERT INTO log VALUES ('after-auto-rollback')");
})
.catch(e => (stmtErr = e));
expect({
code: stmtErr?.code,
durable: (await sql`SELECT v FROM log`).map((r: any) => r.v),
}).toEqual({
code: "ERR_SQLITE_INVALID_TRANSACTION_STATE",
durable: [],
});
});
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Resolved

test("auto-rollback inside a savepoint propagates the original error and aborts the outer transaction", async () => {
let caught: any;
try {
await sql.begin(async tx => {
await tx`INSERT INTO log VALUES ('outer')`;
await tx.savepoint(async sp => {
await sp`INSERT INTO t VALUES ('dup')`;
});
});
} catch (e) {
caught = e;
}
expect({
code: caught?.code,
durable: (await sql`SELECT v FROM log`).map((r: any) => r.v),
}).toEqual({
code: "SQLITE_CONSTRAINT_PRIMARYKEY",
durable: [],
});
});
});
});

describe("SQLite-specific features", () => {
Expand Down
Loading