Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
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
37 changes: 29 additions & 8 deletions src/js/bun/sql.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,10 @@ enum ReservedConnectionState {
interface TransactionState {
connectionState: ReservedConnectionState;
reject: (err: Error) => void;
storedError?: Error | null | undefined;
/// 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`.
Comment on lines +29 to +31

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code

rollbacks: number;
queries: Set<Query<any, any>>;
}

Expand All @@ -39,6 +42,15 @@ interface ReserveAbortState {
onAbort: (() => void) | null;
}

/// Returns `error` with `cause` attached. A dropped connection rejects every
/// waiter with one shared error object, so the cause goes on a copy.
Comment on lines +45 to +46

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code

function withCause(error: unknown, cause: unknown): unknown {
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);
Comment on lines +48 to +51

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 (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…

}

function settleReservedTransaction(
reservedTransaction: Set<Promise<void>>,
finished: { promise: Promise<void>; resolve: () => void },
Expand Down Expand Up @@ -255,7 +267,7 @@ const SQL: typeof Bun.SQL = function SQL(
query.reject(err);
}

if (err) {
if (err && this.rollbacks === 0) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 (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…

return reject(err);
}
}
Expand Down Expand Up @@ -334,7 +346,7 @@ const SQL: typeof Bun.SQL = function SQL(
const state: TransactionState = {
connectionState: ReservedConnectionState.acceptQueries,
reject,
storedError: null,
rollbacks: 0,
queries: new Set(),
};

Expand Down Expand Up @@ -585,6 +597,7 @@ const SQL: typeof Bun.SQL = function SQL(
const state: TransactionState = {
connectionState: ReservedConnectionState.acceptQueries,
reject,
rollbacks: 0,
queries: new Set(),
};

Expand Down Expand Up @@ -819,7 +832,14 @@ 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}`);
state.rollbacks++;
try {
await run_internal_transaction_sql(`${ROLLBACK_TO_SAVEPOINT_COMMAND} ${save_point_name}`);
} catch (rollback_err) {
throw withCause(rollback_err, err);
} finally {
state.rollbacks--;
}
}
throw err;
}
Expand Down Expand Up @@ -871,15 +891,16 @@ const SQL: typeof Bun.SQL = function SQL(
await run_internal_transaction_sql(COMMIT_COMMAND);
return resolve(transaction_result);
} catch (err) {
try {
if (!(state.connectionState & ReservedConnectionState.closed) && needs_rollback) {
if (!(state.connectionState & ReservedConnectionState.closed) && needs_rollback) {
state.rollbacks++;
try {
if (BEFORE_COMMIT_OR_ROLLBACK_COMMAND) {
await run_internal_transaction_sql(BEFORE_COMMIT_OR_ROLLBACK_COMMAND);
}
await run_internal_transaction_sql(ROLLBACK_COMMAND);
} catch (rollback_err) {
return reject(withCause(rollback_err, err));
}
} catch (err) {
return reject(err);
}
return reject(err);
} finally {
Expand Down
257 changes: 257 additions & 0 deletions test/js/sql/sql-transaction-rollback-failure.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,257 @@
// When the callback of sql.begin() or sql.savepoint() throws, the driver sends
// ROLLBACK (or ROLLBACK TO SAVEPOINT). If that statement itself fails, or the
// connection drops under it, the rejection is the rollback failure. The error
// the callback threw has to stay reachable as its `cause`. The mock servers
// answer every statement with OK, except that a statement starting with
// ROLLBACK gets the configured fault. Wire bytes come from ./wire-frames.ts.
import { SQL } from "bun";
import { describe, expect, test } from "bun:test";
import type net from "node:net";
import {
listeningServer,
mysqlAckSessionSetup,
mysqlErrPacket,
mysqlHandshakeV10,
mysqlOkPacket,
mysqlReadPackets,
pgCommandComplete,
pgErrorResponse,
pgMockServer,
pgReadyForQuery,
} from "./wire-frames";

type RollbackFault = "error" | "drop" | "none";
type MockServer = (fault: RollbackFault, received: string[]) => Promise<{ port: number; server: net.Server }>;

const pgServer: MockServer = (fault, received) =>
pgMockServer((type, body, socket) => {
if (type !== "Q") return;
const sql = body.subarray(0, body.indexOf(0)).toString("utf8");
received.push(sql);
if (sql.startsWith("ROLLBACK")) {
if (fault === "drop") {
socket.destroy();
return;
}
if (fault === "error") {
return [
pgErrorResponse({ S: "ERROR", C: "25P01", M: "there is no transaction in progress" }),
pgReadyForQuery("I"),
];
}
}
return [pgCommandComplete(sql.split(" ", 1)[0]), pgReadyForQuery(sql.startsWith("BEGIN") ? "T" : "I")];
});

const mysqlServer: MockServer = (fault, received) => {
const COM_QUIT = 0x01;
const COM_QUERY = 0x03;
return listeningServer(socket => {
let buffered = Buffer.alloc(0);
let authed = false;
socket.write(mysqlHandshakeV10());
socket.on("data", (chunk: Buffer) => {
buffered = mysqlReadPackets(Buffer.concat([buffered, chunk]), (seq, payload) => {
if (!authed) {
authed = true;
socket.write(mysqlOkPacket(seq + 1));
return;
}
if (mysqlAckSessionSetup(socket, payload)) return;
if (payload[0] === COM_QUIT) {
socket.end();
return;
}
if (payload[0] !== COM_QUERY) return;
const sql = payload.subarray(1).toString("utf8");
received.push(sql);
if (sql.startsWith("ROLLBACK")) {
if (fault === "drop") {
socket.destroy();
return;
}
if (fault === "error") {
socket.write(mysqlErrPacket(1, 1305, "42000", "SAVEPOINT does not exist"));
return;
}
}
socket.write(mysqlOkPacket(1));
});
});
socket.on("error", () => {});
});
};

const adapters = [
{
adapter: "postgres" as const,
mockServer: pgServer,
begin: "BEGIN",
serverError: "ERR_POSTGRES_SERVER_ERROR",
connectionClosed: "ERR_POSTGRES_CONNECTION_CLOSED",
},
{
adapter: "mysql" as const,
mockServer: mysqlServer,
begin: "START TRANSACTION",
serverError: "ERR_MYSQL_SERVER_ERROR",
connectionClosed: "ERR_MYSQL_CONNECTION_CLOSED",
},
];

describe.each(adapters)("$adapter", ({ adapter, mockServer, begin, serverError, connectionClosed }) => {
async function withMock(fault: RollbackFault, fn: (sql: SQL, received: string[]) => Promise<void>) {
const received: string[] = [];
const { port, server } = await mockServer(fault, received);
const sql = new SQL({
adapter,
hostname: "127.0.0.1",
port,
username: "u",
password: "p",
database: "db",
max: 1,
tls: false,
idleTimeout: 5,
});
try {
await fn(sql, received);
} finally {
await sql.close({ timeout: 0 }).catch(() => {});
await new Promise<void>(r => server.close(() => r()));
}
}

const appError = () => new Error("APP-ERROR-MARKER");
const settle = (p: Promise<unknown>) =>
p.then(
v => ({ resolved: v }),
e => e,
);

test("begin(): a working ROLLBACK rejects with the callback error itself", async () => {
await withMock("none", async (sql, received) => {
const thrown = appError();
const err = await settle(
sql.begin(async tx => {
await tx.unsafe("SELECT 1");
throw thrown;
}),
);
expect(err).toBe(thrown);
expect(err.cause).toBeUndefined();
expect(received).toEqual([begin, "SELECT 1", "ROLLBACK"]);
});
});

test("begin(): a ROLLBACK the server rejects keeps the callback error as cause", async () => {
await withMock("error", async (sql, received) => {
const thrown = appError();
const err = await settle(
sql.begin(async tx => {
await tx.unsafe("SELECT 1");
throw thrown;
}),
);
expect(err).toBeInstanceOf(SQL.SQLError);
expect(err.code).toBe(serverError);
expect(err.cause).toBe(thrown);
expect(received).toEqual([begin, "SELECT 1", "ROLLBACK"]);
// the connection is still usable
await sql.unsafe("SELECT 2");
});
});

test("begin(): a connection that drops during ROLLBACK keeps the callback error as cause", async () => {
await withMock("drop", async (sql, received) => {
const thrown = appError();
const err = await settle(
sql.begin(async tx => {
await tx.unsafe("SELECT 1");
throw thrown;
}),
);
expect(err).toBeInstanceOf(SQL.SQLError);
expect(err.code).toBe(connectionClosed);
expect(err.cause).toBe(thrown);
expect(received).toEqual([begin, "SELECT 1", "ROLLBACK"]);
});
});

// The dropped connection rejects a query queued behind the transaction with
// the same connection error. That query has nothing to do with the callback
// error, so the cause must not show up on its rejection.
test("begin(): the cause does not leak to other queries the dropped connection rejects", async () => {
await withMock("drop", async sql => {
const thrown = appError();
const inTransaction = Promise.withResolvers<void>();
const queued = Promise.withResolvers<void>();
const transaction = settle(
sql.begin(async tx => {
await tx.unsafe("SELECT 1");
inTransaction.resolve();
await queued.promise;
throw thrown;
}),
);
await inTransaction.promise;
// max is 1, so this query waits in the pool queue for the transaction's connection
const waiter = settle(sql.unsafe("SELECT 'queued'"));
queued.resolve();
const [err, waiterErr] = await Promise.all([transaction, waiter]);
expect(err.code).toBe(connectionClosed);
expect(err.cause).toBe(thrown);
expect(waiterErr).toBeInstanceOf(SQL.SQLError);
expect(waiterErr.code).toBe(connectionClosed);
expect(waiterErr.cause).toBeUndefined();
});
});

test("savepoint(): a ROLLBACK TO SAVEPOINT the server rejects keeps the callback error as cause", async () => {
await withMock("error", async (sql, received) => {
const thrown = appError();
let savepointError: any;
const err = await settle(
sql.begin(async tx => {
await tx.unsafe("SELECT 1");
savepointError = await settle(
tx.savepoint(async sp => {
await sp.unsafe("SELECT 2");
throw thrown;
}),
);
throw savepointError;
}),
);
expect(savepointError).toBeInstanceOf(SQL.SQLError);
expect(savepointError.code).toBe(serverError);
expect(savepointError.cause).toBe(thrown);
// The outer ROLLBACK fails too. Its cause is the error the callback
// rethrew, so the whole chain stays reachable.
expect(err).toBeInstanceOf(SQL.SQLError);
expect(err.code).toBe(serverError);
expect(err).not.toBe(savepointError);
expect(err.cause).toBe(savepointError);
expect(received).toEqual([begin, "SELECT 1", "SAVEPOINT s0", "SELECT 2", "ROLLBACK TO SAVEPOINT s0", "ROLLBACK"]);
});
});

test("savepoint(): a connection that drops during ROLLBACK TO SAVEPOINT keeps the callback error as cause", async () => {
await withMock("drop", async (sql, received) => {
const thrown = appError();
const err = await settle(
sql.begin(async tx => {
await tx.unsafe("SELECT 1");
await tx.savepoint(async sp => {
await sp.unsafe("SELECT 2");
throw thrown;
});
}),
);
expect(err).toBeInstanceOf(SQL.SQLError);
expect(err.code).toBe(connectionClosed);
expect(err.cause).toBe(thrown);
expect(received).toEqual([begin, "SELECT 1", "SAVEPOINT s0", "SELECT 2", "ROLLBACK TO SAVEPOINT s0"]);
});
});
});
Loading