sql: reject tx.unsafe()/tx.file() on a settled transaction or released reserved handle

[robobun]

Every other entry point on a transaction/reserved handle (the tagged template, savepoint(), connect(), flush(), begin(), and the runner's own internal SQL) checks state.connectionState and rejects with Connection closed once sql.begin() has settled or the reserved connection has been released. unsafe() and file() on the same handle did not, so a tx reference retained past the callback silently executes on a pooled connection the handle no longer owns.

Reproduction

import { SQL } from "bun";
import { Database } from "bun:sqlite";
import { mkdtempSync } from "node:fs"; import { tmpdir } from "node:os";

const path = mkdtempSync(tmpdir() + "/esc-") + "/a.db";
const sql = new SQL(`sqlite://${path}`);
await sql`create table w (v text)`;
let leaked;
await sql.begin(async tx => { leaked = tx; await tx.unsafe("insert into w (v) values ('inside')"); });
console.log(await leaked`select 1`.then(() => "ok", e => "rejected: " + e));
// rejected: SQLiteError: Connection closed
console.log(await leaked.unsafe("insert into w (v) values ('escaped')").then(() => "ok", e => "rejected: " + e));
// ok   <-- should reject
await sql.close();
console.log(new Database(path, { readonly: true }).query("select v from w").all());
// [{ v: "inside" }, { v: "escaped" }]   <-- escaped write is durable, ran in autocommit

On a Postgres/MySQL pool the leaked statement reaches whatever the released connection is doing next, which may be another caller's open transaction.

Fix

Add the same closed/acceptQueries guard that transaction_sql() already has to transaction_sql.unsafe, transaction_sql.file, reserved_sql.unsafe, and reserved_sql.file. file() re-checks after the async read so a handle closed while the file is being read still rejects.

Verification

test/js/sql/sqlite-sql.test.ts gains a test that retains tx past begin(), asserts all four entry points now reject Connection closed, and asserts no escaped rows landed. test/js/sql/sql.test.ts gains the same for a released reserved handle and a settled tx on Postgres. Before the fix the sqlite test shows unsafe: "fulfilled" / file: "fulfilled"; after, all reject.

[coderabbitai]

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: 53f62ead-e5e2-4bfd-b41e-e475dc4b98dd

📥 Commits

Reviewing files that changed from the base of the PR and between a271a5e and baa774c.

📒 Files selected for processing (2)

• src/js/bun/sql.ts
• test/js/sql/sql-transaction-closed-handle.test.ts

---

Walkthrough

This PR introduces a shared cannotAcceptQueries(state) helper in sql.ts that consolidates closed/acceptQueries state checks. It replaces inline preconditions across reserved and transaction SQL entry points (unsafe, file, begin, close, release, savepoint) and adds regression tests for Postgres and SQLite verifying rejection after connection/transaction closure.

Changes

Connection/transaction closed-state guards

Layer / File(s)	Summary
Shared closed-state helper and reserved connection guards src/js/bun/sql.ts	Adds cannotAcceptQueries(state) helper checking closed/acceptQueries bits, and applies it in reserved_sql, .unsafe, .file (including async continuation), .begin, .close, and .release.
Transaction guards src/js/bun/sql.ts	Applies cannotAcceptQueries(state) in transaction_sql, .unsafe, .file (including async continuation), .close, and .savepoint, rejecting/throwing pool.connectionClosedError() when appropriate.
Postgres regression tests test/js/sql/sql.test.ts	Adds tests confirming released reserved connections and settled transaction handles reject template, .unsafe(), and .file() calls with ERR_POSTGRES_CONNECTION_CLOSED.
SQLite regression test test/js/sql/sql-transaction-closed-handle.test.ts	Adds a new test verifying that a leaked transaction handle after sql.begin() settles rejects template, .savepoint(), .unsafe(), and .file() calls with "Connection closed", and that only the intended write persists.

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly describes the main behavioral change: rejecting tx.unsafe()/tx.file() after settlement or release.
Description check	✅ Passed	The description covers the bug, fix, reproduction, and verification steps, which satisfies the template despite using 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.}

[robobun]

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

❌ @robobun, your commit 65e0e01 has some failures in Build #70485 (All Failures)

---

🧪   To try this PR locally:

bunx bun-pr 33751

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

bun-33751 --bun

[coderabbitai]

Actionable comments posted: 3

🤖 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 `@src/js/bun/sql.ts`:
- Around line 280-308: The closed/acceptQueries check is duplicated across
reserved_sql.unsafe, reserved_sql.file, transaction_sql.unsafe,
transaction_sql.file, and the related reserved_sql/transaction_sql close paths,
so extract it into a small helper and reuse it everywhere. Add a named guard
helper near the existing pool/transaction state logic in src/js/bun/sql.ts, then
replace each inline state.connectionState check with that helper while
preserving the same Promise.$reject(pool.connectionClosedError()) behavior.

In `@test/js/sql/sql.test.ts`:
- Around line 4000-4042: The existing reserved/tx rejection tests only verify
`.file()` after the handle is already closed, so they miss the TOCTOU case the
fix in `reserved.file()` and `tx.file()` is meant to protect. Add a test that
starts `reserved.file(rel("select.sql"))` or `tx.file(rel("select.sql"))`, then
releases/settles the handle while `Bun.file(...).text()` is still pending, and
assert it rejects with `ERR_POSTGRES_CONNECTION_CLOSED`. Use the existing
`reserved`, `tx`, `outcome`, and `.file()` paths so the race in the async
read-and-recheck flow is actually exercised.

In `@test/js/sql/sqlite-sql.test.ts`:
- Around line 1148-1175: The sqlite transaction close test only covers the
pre-read guard for tx.file(), not the post-read re-check added in the .then()
path. Update the test around tx.file() in tx.unsafe() and tx.file() reject after
begin() settles so the leaked transaction remains open while file reading is in
flight, then gets closed before the promise resolves. Use the existing leaked
handle from sql.begin and the tx.file(join(dir, "q.sql")) call to assert the
second “Connection closed” rejection path is exercised, not just the initial
guard.

🪄 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: 29a2b4a5-e8da-4065-84f0-fabf520d6fae

📥 Commits

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

📒 Files selected for processing (3)

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

[claude]

LGTM — brings .unsafe()/.file() in line with the existing closed-handle guard on sibling entry points.

Extended reasoning...

Overview

This PR closes a consistency gap in src/js/bun/sql.ts: the tagged-template call, .savepoint(), .begin(), .close(), and .release() on transaction/reserved handles all check state.connectionState and reject once the handle is settled/released, but .unsafe() and .file() did not. A retained tx reference could therefore execute writes on a pooled connection it no longer owns. The fix adds the same guard to those four entry points (plus a defensive post-read re-check in .file()), and extracts the repeated predicate into a cannotAcceptQueries(state) helper applied at all 13 sites. Tests are added for both SQLite (new dedicated file per the test/js/sql/ subject-per-file convention) and Postgres (in sql.test.ts), covering both reserved and tx handles.

Security risks

None introduced — the change is strictly fail-closed where it was previously fail-open. The bug being fixed is itself a mild isolation concern (a stale handle could inject SQL into whatever the released pooled connection is doing next), so this is a net safety improvement. No new inputs, no auth/crypto/permissions surface.

Level of scrutiny

Low-to-moderate. The runtime change is a targeted bug fix that copies an existing, well-established guard to four sibling entry points that were missing it. I verified the cannotAcceptQueries helper is byte-for-byte identical to the inline predicate it replaces at the seven pre-existing sites, so the refactor portion is behavior-preserving. The new guards use the same rejection (pool.connectionClosedError()) as the tagged-template path.

Other factors

All three CodeRabbit review threads are resolved: the helper-extraction request was addressed in a99ad43, and the two TOCTOU-coverage nits were withdrawn after the author explained why deterministic mid-read race coverage would be inherently flaky. No CODEOWNERS match this path. The bug hunter found no issues. Test coverage is solid — it asserts all four entry points reject with the expected error and confirms no escaped rows land in the SQLite case.

[robobun]

CI on 65e0e01 is red on unrelated lanes; the diff itself is green.

• sql-transaction-closed-handle.test.ts and the two new cases in sql.test.ts pass on every lane.
• The SQL-adjacent failures (postgres-binary-array-bounds.test.ts, postgres-invalid-message-length.test.ts on Windows 11 aarch64) are ERR_POSTGRES_CONNECTION_REFUSED from the mock server before any query runs; neither test touches transaction/reserved handles, and both also fail on main build 70431.
• The remaining failures (bun-install.test.ts, dev-server-ssr-100.test.ts, inspect.test.ts, no-orphans.test.ts, res.send.test.ts, napi.test.ts, bun-jsc.test.ts, webview-chrome.test.ts) do not touch Bun.SQL; most are also failing on main builds 70430/70431.

Ready for review.