From c07f5dc81b7b8590a8641d55534e54b0f5122faf Mon Sep 17 00:00:00 2001 From: robobun Date: Wed, 8 Jul 2026 06:58:54 +0000 Subject: [PATCH 1/5] sql: hand the query to the pool synchronously in Query.then() Query.then()/catch()/finally() deferred the pool hand-off by one microtask via `await Promise.$resolve()`, so a close()/end() in the same synchronous block ran first, saw hasPendingQueries() == false, and rejected the already-awaited query with ERR_*_CONNECTION_CLOSED without ever sending it. q.execute() (which enqueues synchronously) did not have this problem. Query.then() now hands the query to the pool synchronously, the same way execute() does; the SQLQueryStatus.executed flag already guards re-entry from the then()/finally() the pool itself calls during bindQuery. This exposed a latent truthiness bug in BaseSQLAdapter.close(): the documented `{ timeout: 0 }` force-close went through the graceful-wait branch because `if (timeout)` is false for 0. It happened to reject same-tick queries only because nothing was enqueued yet. With queries now enqueued synchronously that no longer holds, so close() checks `timeout != null` to reach the existing `timeout === 0` fast path. --- src/js/internal/sql/query.ts | 4 ++-- src/js/internal/sql/shared.ts | 2 +- test/js/sql/sql-mysql.test.ts | 10 ++++++++++ test/js/sql/sql.test.ts | 17 +++++++++++++++++ test/js/sql/sqlite-sql.test.ts | 10 ++++++++++ 5 files changed, 40 insertions(+), 3 deletions(-) diff --git a/src/js/internal/sql/query.ts b/src/js/internal/sql/query.ts index 258f10f05922..596ccf4a4afa 100644 --- a/src/js/internal/sql/query.ts +++ b/src/js/internal/sql/query.ts @@ -149,8 +149,8 @@ class Query> extends PublicPromise { return this; } - await Promise.$resolve(); - + // Hand the query to the pool synchronously so a same-tick close() sees it + // as pending; the executed flag above guards re-entry from then()/finally(). try { return handler(this, handle); } catch (err) { diff --git a/src/js/internal/sql/shared.ts b/src/js/internal/sql/shared.ts index 21c781812aa6..f493dcb8ab86 100644 --- a/src/js/internal/sql/shared.ts +++ b/src/js/internal/sql/shared.ts @@ -1228,7 +1228,7 @@ abstract class BaseSQLAdapter 2 ** 31 || timeout < 0 || timeout !== timeout) { throw $ERR_INVALID_ARG_VALUE("options.timeout", timeout, "must be a non-negative integer less than 2^31"); diff --git a/test/js/sql/sql-mysql.test.ts b/test/js/sql/sql-mysql.test.ts index fd103f41f7ac..79eba4e8b217 100644 --- a/test/js/sql/sql-mysql.test.ts +++ b/test/js/sql/sql-mysql.test.ts @@ -971,6 +971,16 @@ if (isDockerEnabled()) { return expect(await promise).toEqual([{ x: 0 }]); }); + // Same contract as the .execute() case above, but via .then(): the query + // was handed to the pool one microtask late, so a same-tick end() ran + // first and rejected it with ERR_MYSQL_CONNECTION_CLOSED. + test("Connection end does not cancel a query awaited in the same tick", async () => { + const sql = new SQL({ ...getOptions(), max: 1 }); + await sql`select 1 as x`; + const [rows] = await Promise.all([sql`select 1 as x`.then(r => r), sql.end()]); + expect(rows).toEqual([{ x: 1 }]); + }); + test("Connection destroyed", async () => { const sql = new SQL(getOptions()); process.nextTick(() => sql.end({ timeout: 0 })); diff --git a/test/js/sql/sql.test.ts b/test/js/sql/sql.test.ts index 7818fecb5caf..01dad5bb2726 100644 --- a/test/js/sql/sql.test.ts +++ b/test/js/sql/sql.test.ts @@ -1790,6 +1790,23 @@ if (isDockerEnabled()) { return expect(await promise).toEqual([{ x: "" }]); }); + // Same contract as the .execute() case above, but via .then(): the query + // was handed to the pool one microtask late, so a same-tick end() ran + // first and rejected it with ERR_POSTGRES_CONNECTION_CLOSED. + test("Connection end does not cancel a query awaited in the same tick", async () => { + const sql = postgres({ ...options, max: 1 }); + await sql`select 1 as x`; + const [rows] = await Promise.all([sql`select 1 as x`.then(r => r), sql.end()]); + expect(rows).toEqual([{ x: 1 }]); + }); + + test("Connection end with a timeout does not cancel a query awaited in the same tick", async () => { + const sql = postgres({ ...options, max: 1 }); + await sql`select 1 as x`; + const [rows] = await Promise.all([sql`select 1 as x`.then(r => r), sql.end({ timeout: 5 })]); + expect(rows).toEqual([{ x: 1 }]); + }); + test("Connection destroyed", async () => { const sql = postgres(options); process.nextTick(() => sql.end({ timeout: 0 })); diff --git a/test/js/sql/sqlite-sql.test.ts b/test/js/sql/sqlite-sql.test.ts index f90ebfe3d191..82d8e3054981 100644 --- a/test/js/sql/sqlite-sql.test.ts +++ b/test/js/sql/sqlite-sql.test.ts @@ -1835,6 +1835,16 @@ describe("Connection management", () => { } }); + // Query.then() used to defer the pool hand-off by one microtask, so a + // close() in the same synchronous block ran first, saw zero pending + // queries, and rejected the already-awaited query with "Connection closed". + test("close() drains a query awaited in the same tick", async () => { + const sql = new SQL("sqlite://:memory:"); + await sql`SELECT 1 AS x`; + const [rows] = await Promise.all([sql`SELECT 42 AS x`.then(r => r), sql.close()]); + expect(rows).toEqual([{ x: 42 }]); + }); + test("reserve throws for SQLite", async () => { const sql = new SQL("sqlite://:memory:"); From ca27cede77acf5f05a5322a4c63eb92ae2aaa5ca Mon Sep 17 00:00:00 2001 From: robobun Date: Wed, 8 Jul 2026 07:21:12 +0000 Subject: [PATCH 2/5] test(sqlite-sql): shrink 10k-iteration statement-finalize loop on debug builds The 10000-iteration loop in "properly finalizes prepared statements" takes ~11s on debug+ASAN builds and times out under the default 5s budget (also on main). Use 1000 iterations under isDebug, matching the pattern in test/js/web/html/FormData.test.ts and elsewhere. --- 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 82d8e3054981..37f4794d9938 100644 --- a/test/js/sql/sqlite-sql.test.ts +++ b/test/js/sql/sqlite-sql.test.ts @@ -1,6 +1,6 @@ import { randomUUIDv7, SQL } from "bun"; 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"; @@ -2024,7 +2024,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 062e3cf71e748562f2b012519a853b4547f71326 Mon Sep 17 00:00:00 2001 From: robobun Date: Wed, 8 Jul 2026 07:29:57 +0000 Subject: [PATCH 3/5] sql: delegate #runAsync to #run instead of duplicating the body The body was byte-identical after dropping the microtask yield. --- src/js/internal/sql/query.ts | 32 +++----------------------------- 1 file changed, 3 insertions(+), 29 deletions(-) diff --git a/src/js/internal/sql/query.ts b/src/js/internal/sql/query.ts index 596ccf4a4afa..e18f93d696cd 100644 --- a/src/js/internal/sql/query.ts +++ b/src/js/internal/sql/query.ts @@ -128,35 +128,9 @@ class Query> extends PublicPromise { } async #runAsync() { - const { [_handler]: handler, [_queryStatus]: status } = this; - - if ( - status & - (SQLQueryStatus.executed | SQLQueryStatus.error | SQLQueryStatus.cancelled | SQLQueryStatus.invalidHandle) - ) { - return; - } - - if (this[_flags] & SQLQueryFlags.notTagged) { - this.reject(this[_adapter].notTaggedCallError()); - return; - } - - this[_queryStatus] |= SQLQueryStatus.executed; - const handle = this.#getQueryHandle(); - - if (!handle) { - return this; - } - - // Hand the query to the pool synchronously so a same-tick close() sees it - // as pending; the executed flag above guards re-entry from then()/finally(). - try { - return handler(this, handle); - } catch (err) { - this[_queryStatus] |= SQLQueryStatus.error; - this.reject(err as Error); - } + // Enqueue synchronously (same as execute()) so a same-tick close() sees + // the query as pending; #run()'s executed-status guard handles re-entry. + return this.#run(); } get active() { From ac4dd577b6cbd47e2c58083c1e80c4a63f8d0d8e Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Wed, 8 Jul 2026 09:09:34 +0000 Subject: [PATCH 4/5] ci: retrigger From d69317ae6343854e0f978ee8813b5f429de5ee41 Mon Sep 17 00:00:00 2001 From: robobun Date: Thu, 13 Aug 2026 04:59:29 +0000 Subject: [PATCH 5/5] sql: cover close({ timeout: 0 }) and close({ timeout: null }) with a query in flight (from #32039) --- src/js/internal/sql/shared.ts | 1 + .../sql/sql-close-pending-connection.test.ts | 138 +++++++++++++++++- 2 files changed, 138 insertions(+), 1 deletion(-) diff --git a/src/js/internal/sql/shared.ts b/src/js/internal/sql/shared.ts index 2b7a18857cb7..45ebce7ea968 100644 --- a/src/js/internal/sql/shared.ts +++ b/src/js/internal/sql/shared.ts @@ -1240,6 +1240,7 @@ abstract class BaseSQLAdapter 2 ** 31 || timeout < 0 || timeout !== timeout) { diff --git a/test/js/sql/sql-close-pending-connection.test.ts b/test/js/sql/sql-close-pending-connection.test.ts index df35ab760a25..7cdd89d3b19c 100644 --- a/test/js/sql/sql-close-pending-connection.test.ts +++ b/test/js/sql/sql-close-pending-connection.test.ts @@ -18,7 +18,21 @@ import { SQL } from "bun"; import { expect, test } from "bun:test"; -import { neverAnsweringServer } from "./wire-frames"; +import type { Server, Socket } from "node:net"; +import { + listeningServer, + mysqlHandshakeV10, + mysqlOkPacket, + mysqlReadPackets, + mysqlTextResultSet, + neverAnsweringServer, + pgAuthenticationOk, + pgCommandComplete, + pgDataRow, + pgReadFrontendMessages, + pgReadyForQuery, + pgRowDescription, +} from "./wire-frames"; const drivers = [ ["postgres", "postgres://postgres@", "ERR_POSTGRES_CONNECTION_CLOSED"], @@ -101,3 +115,125 @@ test("pool scans tolerate unassigned connection slots during pool start", async server.close(); } }); + +// https://github.com/oven-sh/bun/issues/32038 +// +// close({ timeout }) used to be gated on `if (timeout)`, so the documented +// `close({ timeout: 0 })` ("close now") fell into the graceful-drain branch and +// waited for in-flight queries forever. The tests above sidestep that with the +// truthy string "0"; these use the number. `timeout: null` must still mean +// "no timeout" (drain), not "timeout of 0". +// +// Each mock completes the handshake and hands the first command it receives to +// `onCommand`; by default it never answers, leaving the query in flight. + +type CommandMock = { port: number; server: Server; commandReceived: Promise }; + +async function pgReadyServer(onCommand?: (socket: Socket, type: number) => void): Promise { + const received = Promise.withResolvers(); + const { port, server } = await listeningServer(socket => { + let startup = true; + let buffered = Buffer.alloc(0); + socket.on("data", chunk => { + if (startup) { + startup = false; + socket.write(Buffer.concat([pgAuthenticationOk(), pgReadyForQuery()])); + return; + } + buffered = pgReadFrontendMessages(Buffer.concat([buffered, chunk]), type => { + onCommand?.(socket, type); + received.resolve(); + }); + }); + socket.on("error", () => {}); + }); + return { port, server, commandReceived: received.promise }; +} + +async function mysqlReadyServer( + onCommand?: (socket: Socket, seq: number, payload: Buffer) => void, +): Promise { + const received = Promise.withResolvers(); + const { port, server } = await listeningServer(socket => { + let buffered = Buffer.alloc(0); + let authed = false; + socket.write(mysqlHandshakeV10()); + socket.on("data", chunk => { + buffered = mysqlReadPackets(Buffer.concat([buffered, chunk]), (seq, payload) => { + if (!authed) { + authed = true; + socket.write(mysqlOkPacket(seq + 1)); + return; + } + onCommand?.(socket, seq, payload); + received.resolve(); + }); + }); + socket.on("error", () => {}); + }); + return { port, server, commandReceived: received.promise }; +} + +// Answers a simple-protocol `select 1 as x` with one text row once `respond()` is called. +const drainableMocks = { + async postgres() { + let respond!: () => void; + const mock = await pgReadyServer((socket, type) => { + if (type !== 0x51 /* Query */) return; + respond = () => + socket.write( + Buffer.concat([ + pgRowDescription([{ name: "x", typeOid: 25 }]), + pgDataRow([Buffer.from("1")]), + pgCommandComplete("SELECT 1"), + pgReadyForQuery(), + ]), + ); + }); + return { ...mock, respond: () => respond() }; + }, + async mysql() { + let respond!: () => void; + const mock = await mysqlReadyServer((socket, seq, payload) => { + if (payload[0] !== 0x03 /* COM_QUERY */) return; + respond = () => socket.write(mysqlTextResultSet(seq + 1, [{ name: "x", type: 0xfd }], [["1"]])); + }); + return { ...mock, respond: () => respond() }; + }, +} as const; + +const silentMocks = { + postgres: () => pgReadyServer(), + mysql: () => mysqlReadyServer(), +} as const; + +for (const [name, scheme, closedCode] of drivers) { + test(`${name}: close({ timeout: 0 }) force-closes with a query in flight`, async () => { + const { port, server, commandReceived } = await silentMocks[name](); + try { + const sql = new SQL({ url: `${scheme}127.0.0.1:${port}/db`, max: 1 }); + const queryError = sql`SELECT 1`.catch(e => e); + // the server has the query and will never answer it + await commandReceived; + await sql.close({ timeout: 0 }); + expect((await queryError).code).toBe(closedCode); + } finally { + server.close(); + } + }); + + test(`${name}: close({ timeout: null }) still waits for the query in flight`, async () => { + const { port, server, commandReceived, respond } = await drainableMocks[name](); + try { + const sql = new SQL({ url: `${scheme}127.0.0.1:${port}/db`, max: 1 }); + const rows = sql`select 1 as x`.simple().then(r => r); + await commandReceived; + const closing = sql.close({ timeout: null }); + respond(); + expect(await rows).toEqual([{ x: "1" }]); + await closing; + } finally { + server.close(); + } + }); +}