sql: make close({ timeout: 0 }) force-close immediately with queries in flight

[robobun]

Fixes #32038

Repro

const sql = new SQL({ url: "postgres://postgres@localhost:5432/postgres", max: 1 });
const pending = sql`SELECT pg_sleep(5)`.catch(e => e.code);
await Bun.sleep(500); // let the query get on the wire
await sql.close({ timeout: 0 }); // waits ~4.5s for pg_sleep to finish instead of closing

Cause

The pool close() (now BaseSQLAdapter.close in src/js/internal/sql/shared.ts, used by both the postgres and mysql adapters) gates the timeout option on truthiness:

let timeout = options?.timeout;
if (timeout) {                 // 0 is falsy, so timeout: 0 takes the else branch
  ...
  if (timeout === 0 || !this.hasPendingQueries()) {
    // close immediately       <- unreachable for 0
  }
  ...
} else {
  // graceful drain
}

The inner timeout === 0 branch shows the intent (immediate close), but if (timeout) makes it unreachable: close({ timeout: 0 }) with in-flight queries behaved exactly like close() with no options and waited for every pending query to drain. Long-standing behavior, not a regression.

The fix is the gate: if (timeout) becomes if (timeout != null), so 0 flows through validation and hits the immediate-close branch. undefined and null keep meaning graceful drain with no timer. A side effect of the presence gate is that timeout: NaN now throws ERR_INVALID_ARG_VALUE like other invalid values (e.g. timeout: "abc") already did, instead of being silently ignored.

Rebase note: the fix originally patched the two identical close() copies in postgres.ts and mysql.ts; after #32145 consolidated the pool plumbing into shared.ts, the same one-line gate change now lands in BaseSQLAdapter.close and the driver files are untouched.

The similar-looking truthiness gates in reserved_sql.close/transaction_sql.close (src/js/bun/sql.ts) are not affected: there, falling through for 0 lands on the immediate cancel-and-close path, which is already the correct semantics.

Tests

test/js/sql/sql-close-timeout-zero.test.ts uses mock servers (no Docker) that complete the handshake and never answer the query, so a query stays in flight forever:

• postgres and mysql: close({ timeout: 0 }) settles immediately and the in-flight query rejects with ERR_POSTGRES_CONNECTION_CLOSED / ERR_MYSQL_CONNECTION_CLOSED
• postgres and mysql: close({ timeout: null }) still drains gracefully (the pending query completes normally)

On the unfixed build, both timeout: 0 tests hang in close() and time out; with the fix all four pass. Existing suites that call close({ timeout: 0 }) (sql-connect-error-reporting, sql-mysql-query-string-leak, postgres-tls-ctx-leak) still pass.

[coderabbitai]

Walkthrough

The PR fixes a bug where sql.close({ timeout: 0 }) was treated as missing (falsy) instead of present (explicit zero), preventing immediate closure when queries are pending. Both Postgres and MySQL adapters now gate the timeout on presence rather than truthiness, and comprehensive wire-protocol tests validate immediate-close and graceful-drain semantics.

Changes

SQL Pool Close Timeout Fix

Layer / File(s)	Summary
Timeout condition fix in adapters src/js/internal/sql/postgres.ts, src/js/internal/sql/mysql.ts	Both adapters change the timeout branch condition from truthiness (if (timeout)) to explicit null/undefined check (if (timeout != null)), allowing timeout: 0 to flow through to the immediate-close path while undefined/null continue to trigger graceful drain.
Test imports and wire-protocol utilities test/js/sql/sql-close-timeout-zero.test.ts	Test suite imports and reusable TCP mocks that implement Postgres and MySQL handshake/protocol handling, including configurable query-swallowing behavior to simulate in-flight queries.
Postgres close-timeout tests test/js/sql/sql-close-timeout-zero.test.ts	Two Postgres tests validate that close({ timeout: 0 }) closes immediately (in-flight query fails with ERR_POSTGRES_CONNECTION_CLOSED) and close({ timeout: null }) drains gracefully (in-flight query completes before close resolves).
MySQL close-timeout test test/js/sql/sql-close-timeout-zero.test.ts	MySQL test validates that close({ timeout: 0 }) forces immediate closure with in-flight queries, causing the query to fail with ERR_MYSQL_CONNECTION_CLOSED.

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	Title clearly summarizes the main change: fixing close({ timeout: 0 }) to force immediate closure instead of graceful drain when queries are in flight.
Linked Issues check	✅ Passed	PR changes implement the fix described in #32038: replacing truthiness gate with presence check (timeout != null) in postgres and mysql adapters, and adds comprehensive tests verifying close({ timeout: 0 }) behavior.
Out of Scope Changes check	✅ Passed	All changes are directly scoped to #32038: two adapter files modified for the fix, one new test file added. No unrelated changes detected.
Description check	✅ Passed	The pull request description comprehensively addresses both template requirements with clear explanations of the problem, solution, and verification approach.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

---

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

[robobun]

^{Updated 11:07 AM PT - Jun 12th, 2026}

❌ @robobun, your commit 83ce87f has 3 failures in Build #62102 (All Failures):

• test/js/node/async_hooks/AsyncLocalStorage-tracking.test.ts - pid 14855 segmentation fault at address 0xc on 🍎 14 aarch64
• test/js/web/fetch/fetch-leak.test.ts - code 1 on 🍎 26 aarch64
• test/cli/init/init.test.ts - code 1 on 🍎 26 aarch64
• test/cli/init/init.test.ts - code 1 on 🍎 14 aarch64
• test/cli/init/init.test.ts - code 1 on 🍎 26 aarch64

---

🧪   To try this PR locally:

bunx bun-pr 32039

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

bun-32039 --bun

[coderabbitai]

Actionable comments posted: 2

🤖 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/sql-close-timeout-zero.test.ts`:
- Around line 76-95: The listen-await promise in postgresMock (the new
Promise<void>(r => server.listen(0, "127.0.0.1", () => r()))) does not reject on
server errors causing tests to hang; change it to use new Promise((resolve,
reject) => { server.once("error", reject); server.listen(..., () => {
server.removeListener("error", reject); resolve(); }) }) so bind/listen failures
reject immediately, and apply the same pattern to the other server.listen usage
in this file that follows the same pattern.
- Around line 201-246: Add a twin test for MySQL that covers the close({
timeout: null }) (graceful/drain) path: duplicate the existing "mysql: close({
timeout: 0 }) settles immediately with a query in flight" test, rename it to
something like "mysql: close({ timeout: null }) drains and waits for in-flight
query", and in the duplicate call sql.close({ timeout: null }) instead of
timeout: 0; keep the same mock server/queryReceived setup and assertions but
match the expected graceful behavior used in the Postgres null test (use the
same final expectation/assertion that the Postgres null test uses for
SQL/sql.close to verify the in-flight query is drained rather than immediately
closed). Ensure you reference the SQL instance (new SQL(...)) and sql.close({
timeout: null }) in the new test.

🪄 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: 664f8ba0-7528-49cc-8207-a93fea1c0254

📥 Commits

Reviewing files that changed from the base of the PR and between 88d48c2 and 2893233.

📒 Files selected for processing (3)

• src/js/internal/sql/mysql.ts
• src/js/internal/sql/postgres.ts
• test/js/sql/sql-close-timeout-zero.test.ts

[robobun]

CI status: four full runs of this diff (now rebased onto main with the fix in shared.ts), and every lane that runs this PR's files is green in all of them. Each run's only red was unrelated: bunx.test.ts live-registry breakage (repo-wide, skipped on main in #32042), a streams-leak.test.ts GC-counter flake (debian aarch64, build 61613), a duckdb.test.ts N-API segfault (debian x64, build 61702), and in build 62102 an AsyncLocalStorage-tracking.test.ts subprocess flake (darwin 14 aarch64) plus a darwin 26 shard that died building the vendor/elysia fixture (missing tsup, infra). None involve SQL code paths; the diff is two lines in BaseSQLAdapter.close plus its tests. Ready for review.

[robobun]

#32091 was filed for this same truthiness gate and is now closed as a duplicate of #32038. While reproducing it I ended up with an overlapping branch, farm/ea6a3a02/sql-close-timeout-zero, in case any of it is useful here. On top of this PR's fix it has:

• tests for the remaining spellings: close({ timeout: "0" }) and { timeout: undefined } on both adapters, and { timeout: null } on mysql as well
• the same timeout != null gate applied to reserved_sql.close and transaction_sql.close in src/js/bun/sql.ts. For 0 those paths already force-close, so behavior is unchanged there, but the truthiness gate skips validation: close({ timeout: NaN }) silently force-closes while "abc" throws ERR_INVALID_ARG_VALUE. Covered by reserved and transaction tests.

Two adjacent pre-existing bugs surfaced while testing, filed separately: #32095 (a forced close never resolves while a pool connection is mid-handshake, so force-close tests need an established connection with the query already on the wire) and #32096 (the 2 ** 31 timeout bound is checked in seconds but the timer is armed in milliseconds).

[claude]

LGTM — one-line truthiness→presence fix with thorough mock-server tests for both adapters.

Extended reasoning...

Overview

This PR fixes #32038: sql.close({ timeout: 0 }) was gated on if (timeout) (truthiness), so 0 fell into the graceful-drain branch instead of force-closing. The inner timeout === 0 immediate-close branch already existed but was unreachable. The fix is a single-line change in src/js/internal/sql/shared.ts (if (timeout) → if (timeout != null)), now applied once in the consolidated BaseSQLAdapter.close() after #32145 merged the duplicate postgres/mysql copies. The accompanying ~320-line test file adds mock TCP servers (no Docker) that complete the handshake and stall the query, then asserts force-close (timeout: 0) vs graceful-drain (timeout: null) for both postgres and mysql.

Security risks

None. This touches pool-close lifecycle timing only — no auth, crypto, permissions, or untrusted input handling. The only side effect noted is that timeout: NaN now throws ERR_INVALID_ARG_VALUE (consistent with other invalid values) instead of being silently ignored, which is a correctness improvement.

Level of scrutiny

Low. The source change is one effective line plus a clarifying comment, fixing a classic truthiness-vs-presence bug where the intended behavior was already coded (the timeout === 0 branch) but unreachable. The fix is the conventional != null presence check. undefined/null continue to mean graceful drain.

Other factors

• No bugs flagged by the bug-hunting system.
• Both CodeRabbit comments (reject on listen error, MySQL timeout: null drain twin) were addressed in commit 8807866 and confirmed resolved.
• robobun reports all relevant CI lanes green across three runs; remaining red is unrelated flake (bunx live-registry, streams-leak GC, duckdb N-API) outside this PR's code paths.
• No CODEOWNERS for these files.
• The follow-up note about applying the same gate to reserved_sql.close/transaction_sql.close is a validation-consistency nicety (NaN handling), not a blocker — the PR description confirms those paths already force-close on 0.