From 471e7f321c544e689a8ab76cdb78e9c0c71dc954 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Wed, 8 Jul 2026 03:24:58 +0000 Subject: [PATCH 01/10] sql: don't kill in-flight queries when idleTimeout/maxLifetime fires Postgres and MySQL rejected any in-flight query when idleTimeout or maxLifetime fired, because onConnectionTimeout/onMaxLifetimeTimeout failed the connection unconditionally. The query itself was healthy; only a client-side timer raced it. Postgres (src/sql_jsc/postgres/PostgresSQLConnection.rs): - get_timeout_interval returns 0 when requests are queued or IS_READY_FOR_QUERY is clear, so the idle timer never arms while a query is outstanding (mirrors MySQL's is_idle gate). - on_connection_timeout reschedules on the .connected branch if a request slipped in between arming and firing. - on_max_lifetime_timeout disconnect()s when idle, otherwise reschedules for 1s and retries until the connection returns to idle. MySQL (src/sql_jsc/mysql/JSMySQLConnection.rs): - on_max_lifetime_timeout close()s when idle, otherwise reschedules 1s. Tests: rewrite the idle/maxLifetime tests to the drained behavior (query completes, then the connection retires and the pool reconnects), add a Docker-free mock-server regression in sql-timer-drain.test.ts. Fixes #30646. Related: #25405 (MySQL idle). --- src/sql_jsc/mysql/JSMySQLConnection.rs | 27 +-- src/sql_jsc/postgres/PostgresSQLConnection.rs | 74 +++++--- test/js/sql/sql-mysql.test.ts | 58 +++++-- test/js/sql/sql-timer-drain.test.ts | 160 ++++++++++++++++++ test/js/sql/sql.test.ts | 126 ++++++++++---- 5 files changed, 369 insertions(+), 76 deletions(-) create mode 100644 test/js/sql/sql-timer-drain.test.ts diff --git a/src/sql_jsc/mysql/JSMySQLConnection.rs b/src/sql_jsc/mysql/JSMySQLConnection.rs index 26cbd93b1360..75f00ce8a02f 100644 --- a/src/sql_jsc/mysql/JSMySQLConnection.rs +++ b/src/sql_jsc/mysql/JSMySQLConnection.rs @@ -315,18 +315,21 @@ impl JSMySQLConnection { if self.connection.get().status == my_sql_connection::Status::Failed { return; } - use bun_core::fmt::{ConnTimeoutKind, fmt_conn_timeout}; - self.fail_fmt( - AnyMySQLErrorT::LifetimeTimeout, - format_args!( - "{}", - fmt_conn_timeout( - ConnTimeoutKind::MaxLifetime, - self.max_lifetime_interval_ms, - "" - ) - ), - ); + + // Only retire the connection once it's idle. If queries are queued or + // in-flight, reschedule the timer so we close between queries rather + // than killing healthy ones with ERR_MYSQL_LIFETIME_TIMEOUT (#30646). + if self.connection.get().status == my_sql_connection::Status::Connected + && self.connection.get().is_idle() + { + self.close(); + return; + } + + self.max_lifetime_timer.with_mut(|t| { + t.next = timespec::ms_from_now(TimespecMockMode::AllowMockedTime, 1000); + self.vm_mut().timer().insert(t); + }); } fn setup_max_lifetime_timer_if_necessary(&self) { diff --git a/src/sql_jsc/postgres/PostgresSQLConnection.rs b/src/sql_jsc/postgres/PostgresSQLConnection.rs index f575ab14d4f8..060147a52491 100644 --- a/src/sql_jsc/postgres/PostgresSQLConnection.rs +++ b/src/sql_jsc/postgres/PostgresSQLConnection.rs @@ -365,7 +365,21 @@ impl PostgresSQLConnection { fn get_timeout_interval(&self) -> u32 { match self.status.get() { - Status::Connected => self.idle_timeout_interval_ms, + Status::Connected => { + // The idle timer is only relevant when the connection is actually idle. + // If there are queued or in-flight requests, we must not arm the idle + // timer — otherwise we'd race against healthy queries and kill them + // when the timer fires (see #30646, #25405). + if self.requests.get().readable_length() > 0 + || !self + .flags + .get() + .contains(ConnectionFlags::IS_READY_FOR_QUERY) + { + return 0; + } + self.idle_timeout_interval_ms + } Status::Failed => 0, _ => self.connection_timeout_ms, } @@ -540,12 +554,26 @@ impl PostgresSQLConnection { use bun_core::fmt::{ConnTimeoutKind::*, fmt_conn_timeout}; let (code, kind, ms, sfx): (&[u8], _, _, _) = match self.status.get() { - Status::Connected => ( - b"ERR_POSTGRES_IDLE_TIMEOUT", - Idle, - self.idle_timeout_interval_ms, - "", - ), + Status::Connected => { + // Only fire the idle-timeout failure when the connection is genuinely + // idle. If a request slipped into the queue between the timer being + // armed and firing, reschedule rather than killing a healthy query. + if self.requests.get().readable_length() > 0 + || !self + .flags + .get() + .contains(ConnectionFlags::IS_READY_FOR_QUERY) + { + self.reset_connection_timeout(); + return; + } + ( + b"ERR_POSTGRES_IDLE_TIMEOUT", + Idle, + self.idle_timeout_interval_ms, + "", + ) + } Status::SentStartupMessage => ( b"ERR_POSTGRES_CONNECTION_TIMEOUT", Connection, @@ -569,18 +597,26 @@ impl PostgresSQLConnection { if self.status.get() == Status::Failed { return; } - use bun_core::fmt::{ConnTimeoutKind, fmt_conn_timeout}; - self.fail_fmt( - b"ERR_POSTGRES_LIFETIME_TIMEOUT", - format_args!( - "{}", - fmt_conn_timeout( - ConnTimeoutKind::MaxLifetime, - self.max_lifetime_interval_ms, - "" - ) - ), - ); + + // Only retire the connection once it's idle. If queries are queued or + // in-flight, reschedule the timer so we close between queries rather + // than killing healthy ones with ERR_POSTGRES_LIFETIME_TIMEOUT (#30646). + if self.status.get() == Status::Connected + && self.requests.get().readable_length() == 0 + && self + .flags + .get() + .contains(ConnectionFlags::IS_READY_FOR_QUERY) + { + self.disconnect(); + return; + } + + self.max_lifetime_timer.with_mut(|t| { + t.next = + bun_core::Timespec::ms_from_now(bun_core::TimespecMockMode::AllowMockedTime, 1000); + self.vm_mut().timer().insert(t); + }); } fn start(&self) { diff --git a/test/js/sql/sql-mysql.test.ts b/test/js/sql/sql-mysql.test.ts index ebbfa996a7a3..2b18036e9040 100644 --- a/test/js/sql/sql-mysql.test.ts +++ b/test/js/sql/sql-mysql.test.ts @@ -313,7 +313,7 @@ if (isDockerEnabled()) { expect((err as SQL.MySQLError).code).toBe(`ERR_MYSQL_IDLE_TIMEOUT`); }); - test("Max lifetime works", async () => { + test("Max lifetime closes an idle connection and the pool reconnects", async () => { const onClosePromise = Promise.withResolvers(); const onclose = mock(err => { onClosePromise.resolve(err); @@ -326,28 +326,54 @@ if (isDockerEnabled()) { onclose, max: 1, }); - let error: unknown; - try { - expect<[{ x: number }]>(await sql`select 1 as x`).toEqual([{ x: 1 }]); - - while (true) { - for (let i = 0; i < 100; i++) { - await sql`select SLEEP(1)`; - } - } - } catch (e) { - error = e; - } + // Get the server-side connection id before maxLifetime fires. + const [{ id: connBefore }] = await sql`select CONNECTION_ID() as id`; + // maxLifetime fires while the connection is idle → closed gracefully. + await onClosePromise.promise; expect(onclose).toHaveBeenCalledTimes(1); expect(onconnect).toHaveBeenCalledTimes(1); - expect(error).toBeInstanceOf(SQL.SQLError); - expect(error).toBeInstanceOf(SQL.MySQLError); - expect((error as SQL.MySQLError).code).toBe(`ERR_MYSQL_LIFETIME_TIMEOUT`); + // Pool reconnects on a different connection id. + const [{ id: connAfter }] = await sql`select CONNECTION_ID() as id`; + expect(connAfter).not.toBe(connBefore); }); + test( + "Max lifetime does not kill an in-flight query (#30646)", + async () => { + const onClosePromise = Promise.withResolvers(); + const onclose = mock(err => { + onClosePromise.resolve(err); + }); + const onconnect = mock(); + await using sql = new SQL({ + ...getOptions(), + max_lifetime: 1, + onconnect, + onclose, + max: 1, + }); + + const [{ id: connBefore }] = await sql`select CONNECTION_ID() as id`; + + // Query longer than max_lifetime must complete normally. + // Before the fix this rejected with ERR_MYSQL_LIFETIME_TIMEOUT. + const result = await sql`select SLEEP(3) as s, 42 as x`; + expect(result[0].x).toBe(42); + // The lifetime timer must not have killed the query mid-flight. + expect(onclose).not.toHaveBeenCalled(); + + await onClosePromise.promise; + expect(onclose).toHaveBeenCalledTimes(1); + + const [{ id: connAfter }] = await sql`select CONNECTION_ID() as id`; + expect(connAfter).not.toBe(connBefore); + }, + 30_000, + ); + // Last one wins. test("Handles duplicate string column names", async () => { const result = await sql`select 1 as x, 2 as x, 3 as x`; diff --git a/test/js/sql/sql-timer-drain.test.ts b/test/js/sql/sql-timer-drain.test.ts new file mode 100644 index 000000000000..43fd11f97e6c --- /dev/null +++ b/test/js/sql/sql-timer-drain.test.ts @@ -0,0 +1,160 @@ +import { SQL } from "bun"; +import { expect, test } from "bun:test"; +import { + listeningServer, + pgAuthenticationOk, + pgCommandComplete, + pgDataRow, + pgRaw, + pgReadyForQuery, + pgRowDescription, +} from "./wire-frames"; + +// Regression test for #30646. Before the fix, idle_timeout / max_lifetime fired +// `failFmt(...)` unconditionally and rejected any in-flight query with +// ERR_POSTGRES_IDLE_TIMEOUT / ERR_POSTGRES_LIFETIME_TIMEOUT — even though the +// query itself was healthy. After the fix: +// +// - The Postgres idle timer doesn't arm while a query is outstanding (a +// queued request or `is_ready_for_query == false` drops it). +// - When max_lifetime fires on a busy connection it reschedules for 1s and +// retries until the connection is idle, then disconnects gracefully. +// +// The mock server exposes deterministic timing: respond to SELECT only after a +// configurable delay, so we can prove queries complete even when the client- +// side timer interval is much shorter than the server response. + +// Startup-phase handshake (no SSL): AuthenticationOk + ReadyForQuery(idle). +const HANDSHAKE = Buffer.concat([pgAuthenticationOk(), pgReadyForQuery("I")]); + +// Full response to Bun's extended-query `Parse+Describe+Bind+Execute+Flush+Sync` +// batch for `SELECT 42 as x`: ParseComplete + ParameterDescription (0 params) + +// RowDescription + BindComplete + DataRow + CommandComplete + ReadyForQuery. +// Column type 23 = int4. +const QUERY_RESPONSE = Buffer.concat([ + pgRaw("1", Buffer.alloc(0)), // ParseComplete + pgRaw("t", Buffer.from([0, 0])), // ParameterDescription, 0 params + pgRowDescription([{ name: "x", typeOid: 23, typeSize: 4 }]), + pgRaw("2", Buffer.alloc(0)), // BindComplete + pgDataRow([Buffer.from("42")]), + pgCommandComplete("SELECT 1"), + pgReadyForQuery("I"), +]); + +/** + * Mock Postgres server: on the startup packet, reply with the handshake; on a + * client query batch, wait `queryDelayMs` then send the minimal result. Buffers + * inbound bytes and responds once per `Sync`/`Simple Query` so TCP chunking + * can't produce duplicate responses. `onClose` observes the server-side socket + * close. + */ +async function startMockServer( + queryDelayMs: number, + onClose?: () => void, +): Promise<{ port: number; stop: () => void }> { + const timers = new Set(); + const { port, server } = await listeningServer(socket => { + // 'startup' -> reply HANDSHAKE to any first packet; + // 'query' -> parse length-prefixed messages, respond on Sync/Simple Query. + let state: "startup" | "query" = "startup"; + let buf: Buffer = Buffer.alloc(0); + socket.on("data", chunk => { + if (state === "startup") { + state = "query"; + socket.write(HANDSHAKE); + return; + } + buf = buf.length === 0 ? chunk : Buffer.concat([buf, chunk]); + // Message format: type(1) + length(4 BE, includes the length field). + while (buf.length >= 5) { + const len = buf.readInt32BE(1); + const total = 1 + len; + if (buf.length < total) break; + const type = buf[0]; + buf = buf.subarray(total); + if (type === 0x53 /* 'S' Sync */ || type === 0x51 /* 'Q' Simple Query */) { + const t = setTimeout(() => { + timers.delete(t); + if (!socket.destroyed) socket.write(QUERY_RESPONSE); + }, queryDelayMs); + timers.add(t); + } + // Other message types (Parse/Bind/Describe/Execute/Flush) need no + // immediate response — the response batch goes out on Sync. + } + }); + socket.on("close", () => { + onClose?.(); + }); + socket.on("error", () => {}); + }); + return { + port, + stop: () => { + for (const t of timers) clearTimeout(t); + server.close(); + }, + }; +} + +test("idleTimeout does not kill an in-flight query (#30646)", async () => { + // Server takes 2s to respond to the query; client idleTimeout is 1s (in + // seconds — Bun multiplies by 1000 internally). Pre-fix: rejects with + // ERR_POSTGRES_IDLE_TIMEOUT. Post-fix: query completes. + const { port, stop } = await startMockServer(2000); + try { + await using sql = new SQL({ + url: `postgres://u@127.0.0.1:${port}/db?sslmode=disable`, + max: 1, + idleTimeout: 1, + }); + const result = await sql`SELECT 42 as x`; + expect(result[0].x).toBe(42); + } finally { + stop(); + } +}, 30_000); + +test("maxLifetime does not kill an in-flight query (#30646)", async () => { + // Server takes 2s to respond; client max_lifetime is 1s. Pre-fix: rejects + // with ERR_POSTGRES_LIFETIME_TIMEOUT. Post-fix: query completes, then the + // connection closes after it's idle. + const { port, stop } = await startMockServer(2000); + try { + await using sql = new SQL({ + url: `postgres://u@127.0.0.1:${port}/db?sslmode=disable`, + max: 1, + maxLifetime: 1, + }); + const result = await sql`SELECT 42 as x`; + expect(result[0].x).toBe(42); + } finally { + stop(); + } +}, 30_000); + +test("maxLifetime closes an idle connection so the pool can reconnect (#30646)", async () => { + // After the first query completes the connection is idle. The max_lifetime + // timer fires, `disconnect()` runs, and the server sees the socket close. + const { promise: closedOnServer, resolve: onServerClose } = Promise.withResolvers(); + const { port, stop } = await startMockServer(0, onServerClose); + try { + await using sql = new SQL({ + url: `postgres://u@127.0.0.1:${port}/db?sslmode=disable`, + max: 1, + maxLifetime: 1, + // Keep the idle timer out of the way — we want to prove maxLifetime + // alone retires the connection. + idleTimeout: 0, + }); + const result = await sql`SELECT 42 as x`; + expect(result[0].x).toBe(42); + + // Deterministic wait — the test's 30s budget bounds the flake risk. + await closedOnServer; + + await sql.close({ timeout: 0 }).catch(() => {}); + } finally { + stop(); + } +}, 30_000); diff --git a/test/js/sql/sql.test.ts b/test/js/sql/sql.test.ts index 75ce11d3b130..2ebc07654eac 100644 --- a/test/js/sql/sql.test.ts +++ b/test/js/sql/sql.test.ts @@ -780,25 +780,28 @@ if (isDockerEnabled()) { expect(onclose).toHaveBeenCalledTimes(1); }); - test("Idle timeout works at start", async () => { - const onclose = mock(); + test("Idle timeout fires only when the connection is truly idle", async () => { + const onClosePromise = Promise.withResolvers(); + const onclose = mock(err => { + onClosePromise.resolve(err); + }); const onconnect = mock(); await using sql = postgres({ ...options, - idle_timeout: 0.5, + idle_timeout: 1, onconnect, onclose, }); - let error: any; - try { - await sql`select pg_sleep(1)`; - } catch (e) { - error = e; - } - expect(error).toBeInstanceOf(SQL.SQLError); - expect(error).toBeInstanceOf(SQL.PostgresError); - expect(error.code).toBe(`ERR_POSTGRES_IDLE_TIMEOUT`); + // A query longer than idle_timeout must not be killed by the idle timer (#30646). + expect(await sql`select pg_sleep(2)`).toEqual([{ pg_sleep: "" }]); expect(onconnect).toHaveBeenCalled(); + // The timer must not have fired while the query was in flight. + expect(onclose).not.toHaveBeenCalled(); + // After the query returns, the connection is idle — the timer fires shortly after. + const err = await onClosePromise.promise; + expect(err).toBeInstanceOf(SQL.SQLError); + expect(err).toBeInstanceOf(SQL.PostgresError); + expect(err.code).toBe(`ERR_POSTGRES_IDLE_TIMEOUT`); expect(onclose).toHaveBeenCalledTimes(1); }); @@ -823,38 +826,103 @@ if (isDockerEnabled()) { expect(err.code).toBe(`ERR_POSTGRES_IDLE_TIMEOUT`); }); - test("Max lifetime works", async () => { + test("Max lifetime closes an idle connection and the pool reconnects", async () => { const onClosePromise = Promise.withResolvers(); const onclose = mock(err => { onClosePromise.resolve(err); }); const onconnect = mock(); - const sql = postgres({ + await using sql = postgres({ ...options, - max_lifetime: 0.5, + max_lifetime: 1, onconnect, onclose, }); - let error: any; - expect(await sql`select 1 as x`).toEqual([{ x: 1 }]); + + // Grab the server-side backend pid before maxLifetime fires. + const [{ pid: pidBefore }] = await sql`select pg_backend_pid() as pid`; expect(onconnect).toHaveBeenCalledTimes(1); - try { - while (true) { - for (let i = 0; i < 100; i++) { - await sql`select pg_sleep(1)`; - } - } - } catch (e) { - error = e; - } + // maxLifetime fires while the connection is idle and closes it gracefully. + await onClosePromise.promise; expect(onclose).toHaveBeenCalledTimes(1); - expect(error).toBeInstanceOf(SQL.SQLError); - expect(error).toBeInstanceOf(SQL.PostgresError); - expect(error.code).toBe(`ERR_POSTGRES_LIFETIME_TIMEOUT`); + // The pool transparently reconnects — new backend pid. + const [{ pid: pidAfter }] = await sql`select pg_backend_pid() as pid`; + expect(pidAfter).not.toBe(pidBefore); }); + test( + "Max lifetime does not kill an in-flight query (#30646)", + async () => { + const onClosePromise = Promise.withResolvers(); + const onclose = mock(err => { + onClosePromise.resolve(err); + }); + const onconnect = mock(); + await using sql = postgres({ + ...options, + max_lifetime: 1, + onconnect, + onclose, + max: 1, + }); + + // Record the backend pid, then launch a query that runs longer than max_lifetime. + // Before the fix this would reject with ERR_POSTGRES_LIFETIME_TIMEOUT. + const [{ pid: pidBefore }] = await sql`select pg_backend_pid() as pid`; + const result = await sql`select pg_sleep(3), 42 as x`; + expect(result[0].x).toBe(42); + // The lifetime timer must not have killed the query mid-flight. + expect(onclose).not.toHaveBeenCalled(); + + // Once the query returns, the connection is idle and the deferred lifetime + // timer closes it. The pool should then reconnect on a fresh backend. + await onClosePromise.promise; + expect(onclose).toHaveBeenCalledTimes(1); + + const [{ pid: pidAfter }] = await sql`select pg_backend_pid() as pid`; + expect(pidAfter).not.toBe(pidBefore); + }, + 30_000, + ); + + test( + "Idle timeout does not kill an in-flight query (#30646)", + async () => { + const onClosePromise = Promise.withResolvers(); + const onclose = mock(err => { + onClosePromise.resolve(err); + }); + const onconnect = mock(); + await using sql = postgres({ + ...options, + idle_timeout: 1, + onconnect, + onclose, + max: 1, + }); + + // A query slower than idle_timeout must complete, not be killed by the timer. + // Before the fix this rejected with ERR_POSTGRES_IDLE_TIMEOUT. + const [{ pid: pidBefore }] = await sql`select pg_backend_pid() as pid`; + const result = await sql`select pg_sleep(3), 42 as x`; + expect(result[0].x).toBe(42); + // The idle timer must not have killed the query mid-flight. + expect(onclose).not.toHaveBeenCalled(); + + // Once the query returns, the connection is idle and the timer fires, closing it. + const err = await onClosePromise.promise; + expect(err).toBeInstanceOf(SQL.PostgresError); + expect(err.code).toBe(`ERR_POSTGRES_IDLE_TIMEOUT`); + + // Pool reconnects on a fresh backend. + const [{ pid: pidAfter }] = await sql`select pg_backend_pid() as pid`; + expect(pidAfter).not.toBe(pidBefore); + }, + 30_000, + ); + // Last one wins. test("Handles duplicate string column names", async () => { const result = await sql`select 1 as x, 2 as x, 3 as x`; From 9f70cb572075e55ed51a06922454d3eb9409cea8 Mon Sep 17 00:00:00 2001 From: "autofix-ci[bot]" <114827586+autofix-ci[bot]@users.noreply.github.com> Date: Wed, 8 Jul 2026 03:26:58 +0000 Subject: [PATCH 02/10] [autofix.ci] apply automated fixes --- test/js/sql/sql-mysql.test.ts | 54 +++++++-------- test/js/sql/sql.test.ts | 126 ++++++++++++++++------------------ 2 files changed, 84 insertions(+), 96 deletions(-) diff --git a/test/js/sql/sql-mysql.test.ts b/test/js/sql/sql-mysql.test.ts index 2b18036e9040..2796356aa2fd 100644 --- a/test/js/sql/sql-mysql.test.ts +++ b/test/js/sql/sql-mysql.test.ts @@ -340,39 +340,35 @@ if (isDockerEnabled()) { expect(connAfter).not.toBe(connBefore); }); - test( - "Max lifetime does not kill an in-flight query (#30646)", - async () => { - const onClosePromise = Promise.withResolvers(); - const onclose = mock(err => { - onClosePromise.resolve(err); - }); - const onconnect = mock(); - await using sql = new SQL({ - ...getOptions(), - max_lifetime: 1, - onconnect, - onclose, - max: 1, - }); + test("Max lifetime does not kill an in-flight query (#30646)", async () => { + const onClosePromise = Promise.withResolvers(); + const onclose = mock(err => { + onClosePromise.resolve(err); + }); + const onconnect = mock(); + await using sql = new SQL({ + ...getOptions(), + max_lifetime: 1, + onconnect, + onclose, + max: 1, + }); - const [{ id: connBefore }] = await sql`select CONNECTION_ID() as id`; + const [{ id: connBefore }] = await sql`select CONNECTION_ID() as id`; - // Query longer than max_lifetime must complete normally. - // Before the fix this rejected with ERR_MYSQL_LIFETIME_TIMEOUT. - const result = await sql`select SLEEP(3) as s, 42 as x`; - expect(result[0].x).toBe(42); - // The lifetime timer must not have killed the query mid-flight. - expect(onclose).not.toHaveBeenCalled(); + // Query longer than max_lifetime must complete normally. + // Before the fix this rejected with ERR_MYSQL_LIFETIME_TIMEOUT. + const result = await sql`select SLEEP(3) as s, 42 as x`; + expect(result[0].x).toBe(42); + // The lifetime timer must not have killed the query mid-flight. + expect(onclose).not.toHaveBeenCalled(); - await onClosePromise.promise; - expect(onclose).toHaveBeenCalledTimes(1); + await onClosePromise.promise; + expect(onclose).toHaveBeenCalledTimes(1); - const [{ id: connAfter }] = await sql`select CONNECTION_ID() as id`; - expect(connAfter).not.toBe(connBefore); - }, - 30_000, - ); + const [{ id: connAfter }] = await sql`select CONNECTION_ID() as id`; + expect(connAfter).not.toBe(connBefore); + }, 30_000); // Last one wins. test("Handles duplicate string column names", async () => { diff --git a/test/js/sql/sql.test.ts b/test/js/sql/sql.test.ts index 2ebc07654eac..121c8b09b934 100644 --- a/test/js/sql/sql.test.ts +++ b/test/js/sql/sql.test.ts @@ -852,76 +852,68 @@ if (isDockerEnabled()) { expect(pidAfter).not.toBe(pidBefore); }); - test( - "Max lifetime does not kill an in-flight query (#30646)", - async () => { - const onClosePromise = Promise.withResolvers(); - const onclose = mock(err => { - onClosePromise.resolve(err); - }); - const onconnect = mock(); - await using sql = postgres({ - ...options, - max_lifetime: 1, - onconnect, - onclose, - max: 1, - }); + test("Max lifetime does not kill an in-flight query (#30646)", async () => { + const onClosePromise = Promise.withResolvers(); + const onclose = mock(err => { + onClosePromise.resolve(err); + }); + const onconnect = mock(); + await using sql = postgres({ + ...options, + max_lifetime: 1, + onconnect, + onclose, + max: 1, + }); - // Record the backend pid, then launch a query that runs longer than max_lifetime. - // Before the fix this would reject with ERR_POSTGRES_LIFETIME_TIMEOUT. - const [{ pid: pidBefore }] = await sql`select pg_backend_pid() as pid`; - const result = await sql`select pg_sleep(3), 42 as x`; - expect(result[0].x).toBe(42); - // The lifetime timer must not have killed the query mid-flight. - expect(onclose).not.toHaveBeenCalled(); - - // Once the query returns, the connection is idle and the deferred lifetime - // timer closes it. The pool should then reconnect on a fresh backend. - await onClosePromise.promise; - expect(onclose).toHaveBeenCalledTimes(1); - - const [{ pid: pidAfter }] = await sql`select pg_backend_pid() as pid`; - expect(pidAfter).not.toBe(pidBefore); - }, - 30_000, - ); + // Record the backend pid, then launch a query that runs longer than max_lifetime. + // Before the fix this would reject with ERR_POSTGRES_LIFETIME_TIMEOUT. + const [{ pid: pidBefore }] = await sql`select pg_backend_pid() as pid`; + const result = await sql`select pg_sleep(3), 42 as x`; + expect(result[0].x).toBe(42); + // The lifetime timer must not have killed the query mid-flight. + expect(onclose).not.toHaveBeenCalled(); - test( - "Idle timeout does not kill an in-flight query (#30646)", - async () => { - const onClosePromise = Promise.withResolvers(); - const onclose = mock(err => { - onClosePromise.resolve(err); - }); - const onconnect = mock(); - await using sql = postgres({ - ...options, - idle_timeout: 1, - onconnect, - onclose, - max: 1, - }); + // Once the query returns, the connection is idle and the deferred lifetime + // timer closes it. The pool should then reconnect on a fresh backend. + await onClosePromise.promise; + expect(onclose).toHaveBeenCalledTimes(1); - // A query slower than idle_timeout must complete, not be killed by the timer. - // Before the fix this rejected with ERR_POSTGRES_IDLE_TIMEOUT. - const [{ pid: pidBefore }] = await sql`select pg_backend_pid() as pid`; - const result = await sql`select pg_sleep(3), 42 as x`; - expect(result[0].x).toBe(42); - // The idle timer must not have killed the query mid-flight. - expect(onclose).not.toHaveBeenCalled(); - - // Once the query returns, the connection is idle and the timer fires, closing it. - const err = await onClosePromise.promise; - expect(err).toBeInstanceOf(SQL.PostgresError); - expect(err.code).toBe(`ERR_POSTGRES_IDLE_TIMEOUT`); - - // Pool reconnects on a fresh backend. - const [{ pid: pidAfter }] = await sql`select pg_backend_pid() as pid`; - expect(pidAfter).not.toBe(pidBefore); - }, - 30_000, - ); + const [{ pid: pidAfter }] = await sql`select pg_backend_pid() as pid`; + expect(pidAfter).not.toBe(pidBefore); + }, 30_000); + + test("Idle timeout does not kill an in-flight query (#30646)", async () => { + const onClosePromise = Promise.withResolvers(); + const onclose = mock(err => { + onClosePromise.resolve(err); + }); + const onconnect = mock(); + await using sql = postgres({ + ...options, + idle_timeout: 1, + onconnect, + onclose, + max: 1, + }); + + // A query slower than idle_timeout must complete, not be killed by the timer. + // Before the fix this rejected with ERR_POSTGRES_IDLE_TIMEOUT. + const [{ pid: pidBefore }] = await sql`select pg_backend_pid() as pid`; + const result = await sql`select pg_sleep(3), 42 as x`; + expect(result[0].x).toBe(42); + // The idle timer must not have killed the query mid-flight. + expect(onclose).not.toHaveBeenCalled(); + + // Once the query returns, the connection is idle and the timer fires, closing it. + const err = await onClosePromise.promise; + expect(err).toBeInstanceOf(SQL.PostgresError); + expect(err.code).toBe(`ERR_POSTGRES_IDLE_TIMEOUT`); + + // Pool reconnects on a fresh backend. + const [{ pid: pidAfter }] = await sql`select pg_backend_pid() as pid`; + expect(pidAfter).not.toBe(pidBefore); + }, 30_000); // Last one wins. test("Handles duplicate string column names", async () => { From ca8f1eef663faaed6e4e363e62fe0245bd58e7dc Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Wed, 8 Jul 2026 04:37:21 +0000 Subject: [PATCH 03/10] sql(postgres): guard disconnect() with a ref across teardown; use has_query_running() - on_max_lifetime_timeout: hold an intrinsic ref across disconnect(), whose socket.close() can synchronously run the JS onclose callback and make the wrapper GC-eligible before ref_and_close's clean_up_requests touches self. Mirrors fail_with_js_value's ref/deref discipline (the pre-rewrite path took this ref; the MySQL side already guards via ref_guard()). - Replace the inlined busy predicate with the existing has_query_running() helper in get_timeout_interval and on_max_lifetime_timeout. - Drop the dead idle guard in on_connection_timeout: get_timeout_interval() already returns 0 for a busy .connected connection, so the early return above covers it. --- src/sql_jsc/postgres/PostgresSQLConnection.rs | 64 +++++++------------ 1 file changed, 22 insertions(+), 42 deletions(-) diff --git a/src/sql_jsc/postgres/PostgresSQLConnection.rs b/src/sql_jsc/postgres/PostgresSQLConnection.rs index 060147a52491..21998997ce1e 100644 --- a/src/sql_jsc/postgres/PostgresSQLConnection.rs +++ b/src/sql_jsc/postgres/PostgresSQLConnection.rs @@ -365,21 +365,11 @@ impl PostgresSQLConnection { fn get_timeout_interval(&self) -> u32 { match self.status.get() { - Status::Connected => { - // The idle timer is only relevant when the connection is actually idle. - // If there are queued or in-flight requests, we must not arm the idle - // timer — otherwise we'd race against healthy queries and kill them - // when the timer fires (see #30646, #25405). - if self.requests.get().readable_length() > 0 - || !self - .flags - .get() - .contains(ConnectionFlags::IS_READY_FOR_QUERY) - { - return 0; - } - self.idle_timeout_interval_ms - } + // The idle timer is only relevant when the connection is actually idle. + // While a query is queued or in-flight we must not arm it — otherwise + // we'd race a healthy query and kill it when the timer fires (#30646, #25405). + Status::Connected if self.has_query_running() => 0, + Status::Connected => self.idle_timeout_interval_ms, Status::Failed => 0, _ => self.connection_timeout_ms, } @@ -552,28 +542,17 @@ impl PostgresSQLConnection { return; } + // A busy `.connected` connection never reaches here: `get_timeout_interval()` + // returns 0 for it (via `has_query_running()`), so the early return above + // fires. Reaching this match with `Connected` therefore implies idle. use bun_core::fmt::{ConnTimeoutKind::*, fmt_conn_timeout}; let (code, kind, ms, sfx): (&[u8], _, _, _) = match self.status.get() { - Status::Connected => { - // Only fire the idle-timeout failure when the connection is genuinely - // idle. If a request slipped into the queue between the timer being - // armed and firing, reschedule rather than killing a healthy query. - if self.requests.get().readable_length() > 0 - || !self - .flags - .get() - .contains(ConnectionFlags::IS_READY_FOR_QUERY) - { - self.reset_connection_timeout(); - return; - } - ( - b"ERR_POSTGRES_IDLE_TIMEOUT", - Idle, - self.idle_timeout_interval_ms, - "", - ) - } + Status::Connected => ( + b"ERR_POSTGRES_IDLE_TIMEOUT", + Idle, + self.idle_timeout_interval_ms, + "", + ), Status::SentStartupMessage => ( b"ERR_POSTGRES_CONNECTION_TIMEOUT", Connection, @@ -601,14 +580,15 @@ impl PostgresSQLConnection { // Only retire the connection once it's idle. If queries are queued or // in-flight, reschedule the timer so we close between queries rather // than killing healthy ones with ERR_POSTGRES_LIFETIME_TIMEOUT (#30646). - if self.status.get() == Status::Connected - && self.requests.get().readable_length() == 0 - && self - .flags - .get() - .contains(ConnectionFlags::IS_READY_FOR_QUERY) - { + if self.status.get() == Status::Connected && !self.has_query_running() { + // `disconnect()` → `ref_and_close()` → `socket.close()` can run the + // JS `onclose` callback synchronously, which lets the pool drop the + // wrapper and makes it GC-eligible before `clean_up_requests` runs. + // Hold a ref across the teardown (mirrors `fail_with_js_value`). + self.ref_(); self.disconnect(); + // SAFETY: `self` is a live Box-allocated connection; releases the ref above. + unsafe { Self::deref(self.as_ctx_ptr()) }; return; } From e33ba57924ac647646aec0c77673c30a4b77553d Mon Sep 17 00:00:00 2001 From: robobun Date: Thu, 13 Aug 2026 01:37:58 +0000 Subject: [PATCH 04/10] sql: keep ERR_*_LIFETIME_TIMEOUT; retire busy connections at the drain boundary Address review feedback: - Postgres: when max_lifetime fires with a query in flight, set a LIFETIME_EXCEEDED flag instead of polling; the ReadyForQuery arm acts on it before advance() dispatches more work, so max_lifetime stays a hard bound under steady traffic and onclose still reports ERR_POSTGRES_LIFETIME_TIMEOUT (no disconnect()/CONNECTION_CLOSED, no TLS close_notify window, no ref_/deref bracket needed). - MySQL: reschedule 1s when busy, otherwise fall through to the original fail_fmt(LifetimeTimeout), so ERR_MYSQL_LIFETIME_TIMEOUT is preserved. - Postgres do_run: move reset_connection_timeout after advance_and_flush so a synchronously-discarded request can't leave an idle connection with no timer armed. - Tests: drop the mock-server sql-timer-drain.test.ts (container tests cover the scenarios); restore ERR_*_LIFETIME_TIMEOUT assertions in the idle and in-flight container tests. - Drop the stale #25405 reference. --- src/sql/shared/ConnectionFlags.rs | 2 + src/sql_jsc/mysql/JSMySQLConnection.rs | 28 ++- src/sql_jsc/postgres/PostgresSQLConnection.rs | 54 ++++-- src/sql_jsc/postgres/PostgresSQLQuery.rs | 6 +- test/js/sql/sql-mysql.test.ts | 15 +- test/js/sql/sql-timer-drain.test.ts | 160 ------------------ test/js/sql/sql.test.ts | 22 ++- 7 files changed, 89 insertions(+), 198 deletions(-) delete mode 100644 test/js/sql/sql-timer-drain.test.ts diff --git a/src/sql/shared/ConnectionFlags.rs b/src/sql/shared/ConnectionFlags.rs index ec85975fffe3..83f050da5c36 100644 --- a/src/sql/shared/ConnectionFlags.rs +++ b/src/sql/shared/ConnectionFlags.rs @@ -10,6 +10,8 @@ bitflags! { const HAS_BACKPRESSURE = 1 << 4; /// `ref()` was called; `on_data` must not unref the idle connection. const KEEP_ALIVE_REQUESTED = 1 << 5; + /// maxLifetime expired mid-query; retire at the next drain boundary (#30646). + const LIFETIME_EXCEEDED = 1 << 6; } } diff --git a/src/sql_jsc/mysql/JSMySQLConnection.rs b/src/sql_jsc/mysql/JSMySQLConnection.rs index 75f00ce8a02f..47f38c4bd890 100644 --- a/src/sql_jsc/mysql/JSMySQLConnection.rs +++ b/src/sql_jsc/mysql/JSMySQLConnection.rs @@ -316,20 +316,30 @@ impl JSMySQLConnection { return; } - // Only retire the connection once it's idle. If queries are queued or - // in-flight, reschedule the timer so we close between queries rather - // than killing healthy ones with ERR_MYSQL_LIFETIME_TIMEOUT (#30646). + // Don't kill a healthy in-flight query (#30646): reschedule and retry + // once the connection is idle, then fail with the lifetime error as before. if self.connection.get().status == my_sql_connection::Status::Connected - && self.connection.get().is_idle() + && !self.connection.get().is_idle() { - self.close(); + self.max_lifetime_timer.with_mut(|t| { + t.next = timespec::ms_from_now(TimespecMockMode::AllowMockedTime, 1000); + self.vm_mut().timer().insert(t); + }); return; } - self.max_lifetime_timer.with_mut(|t| { - t.next = timespec::ms_from_now(TimespecMockMode::AllowMockedTime, 1000); - self.vm_mut().timer().insert(t); - }); + use bun_core::fmt::{ConnTimeoutKind, fmt_conn_timeout}; + self.fail_fmt( + AnyMySQLErrorT::LifetimeTimeout, + format_args!( + "{}", + fmt_conn_timeout( + ConnTimeoutKind::MaxLifetime, + self.max_lifetime_interval_ms, + "" + ) + ), + ); } fn setup_max_lifetime_timer_if_necessary(&self) { diff --git a/src/sql_jsc/postgres/PostgresSQLConnection.rs b/src/sql_jsc/postgres/PostgresSQLConnection.rs index 21998997ce1e..5ef4617b25c9 100644 --- a/src/sql_jsc/postgres/PostgresSQLConnection.rs +++ b/src/sql_jsc/postgres/PostgresSQLConnection.rs @@ -367,7 +367,7 @@ impl PostgresSQLConnection { match self.status.get() { // The idle timer is only relevant when the connection is actually idle. // While a query is queued or in-flight we must not arm it — otherwise - // we'd race a healthy query and kill it when the timer fires (#30646, #25405). + // we'd race a healthy query and kill it when the timer fires (#30646). Status::Connected if self.has_query_running() => 0, Status::Connected => self.idle_timeout_interval_ms, Status::Failed => 0, @@ -577,26 +577,30 @@ impl PostgresSQLConnection { return; } - // Only retire the connection once it's idle. If queries are queued or - // in-flight, reschedule the timer so we close between queries rather - // than killing healthy ones with ERR_POSTGRES_LIFETIME_TIMEOUT (#30646). - if self.status.get() == Status::Connected && !self.has_query_running() { - // `disconnect()` → `ref_and_close()` → `socket.close()` can run the - // JS `onclose` callback synchronously, which lets the pool drop the - // wrapper and makes it GC-eligible before `clean_up_requests` runs. - // Hold a ref across the teardown (mirrors `fail_with_js_value`). - self.ref_(); - self.disconnect(); - // SAFETY: `self` is a live Box-allocated connection; releases the ref above. - unsafe { Self::deref(self.as_ctx_ptr()) }; + // Don't kill a healthy in-flight query (#30646): mark the connection + // instead and retire it at the next queue-drain boundary (the + // ReadyForQuery arm, before advance() dispatches more work). + if self.status.get() == Status::Connected && self.has_query_running() { + self.update_flags(|f| f.insert(ConnectionFlags::LIFETIME_EXCEEDED)); return; } - self.max_lifetime_timer.with_mut(|t| { - t.next = - bun_core::Timespec::ms_from_now(bun_core::TimespecMockMode::AllowMockedTime, 1000); - self.vm_mut().timer().insert(t); - }); + self.fail_lifetime_timeout(); + } + + fn fail_lifetime_timeout(&self) { + use bun_core::fmt::{ConnTimeoutKind, fmt_conn_timeout}; + self.fail_fmt( + b"ERR_POSTGRES_LIFETIME_TIMEOUT", + format_args!( + "{}", + fmt_conn_timeout( + ConnTimeoutKind::MaxLifetime, + self.max_lifetime_interval_ms, + "" + ) + ), + ); } fn start(&self) { @@ -2546,6 +2550,20 @@ impl PostgresSQLConnection { ); } } + + // maxLifetime expired while a query was in flight; the query has + // now completed, so retire the connection before advance() + // dispatches more work. The on_data loop stops at Status::Failed. + if self + .flags + .get() + .contains(ConnectionFlags::LIFETIME_EXCEEDED) + { + self.fail_lifetime_timeout(); + self.update_ref(); + return Ok(()); + } + self.advance(); self.register_auto_flusher(); diff --git a/src/sql_jsc/postgres/PostgresSQLQuery.rs b/src/sql_jsc/postgres/PostgresSQLQuery.rs index 54e1e2d1954c..7ae81013623f 100644 --- a/src/sql_jsc/postgres/PostgresSQLQuery.rs +++ b/src/sql_jsc/postgres/PostgresSQLQuery.rs @@ -864,10 +864,14 @@ impl PostgresSQLQuery { if did_write { connection.flush_data_and_reset_timeout(); } else { - connection.reset_connection_timeout(); // For unnamed prepared statements with params, we skip writeQuery+Sync // in the enqueue path and let advance() handle it atomically. connection.advance_and_flush(); + // After advance(): it may have discarded the request synchronously + // (e.g. serialization failure), and with the idle timer gated on + // has_query_running() a reset before advance() would leave an idle + // connection with no timer armed. + connection.reset_connection_timeout(); } Ok(JSValue::UNDEFINED) } diff --git a/test/js/sql/sql-mysql.test.ts b/test/js/sql/sql-mysql.test.ts index 2796356aa2fd..274c177e78bb 100644 --- a/test/js/sql/sql-mysql.test.ts +++ b/test/js/sql/sql-mysql.test.ts @@ -330,8 +330,12 @@ if (isDockerEnabled()) { // Get the server-side connection id before maxLifetime fires. const [{ id: connBefore }] = await sql`select CONNECTION_ID() as id`; - // maxLifetime fires while the connection is idle → closed gracefully. - await onClosePromise.promise; + // maxLifetime fires while the connection is idle and retires it with + // the documented error code. + const err = await onClosePromise.promise; + expect(err).toBeInstanceOf(SQL.SQLError); + expect(err).toBeInstanceOf(SQL.MySQLError); + expect((err as SQL.MySQLError).code).toBe(`ERR_MYSQL_LIFETIME_TIMEOUT`); expect(onclose).toHaveBeenCalledTimes(1); expect(onconnect).toHaveBeenCalledTimes(1); @@ -363,7 +367,12 @@ if (isDockerEnabled()) { // The lifetime timer must not have killed the query mid-flight. expect(onclose).not.toHaveBeenCalled(); - await onClosePromise.promise; + // Once idle, the rescheduled timer retires the connection with the + // documented error code, and the pool reconnects. + const err = await onClosePromise.promise; + expect(err).toBeInstanceOf(SQL.SQLError); + expect(err).toBeInstanceOf(SQL.MySQLError); + expect((err as SQL.MySQLError).code).toBe(`ERR_MYSQL_LIFETIME_TIMEOUT`); expect(onclose).toHaveBeenCalledTimes(1); const [{ id: connAfter }] = await sql`select CONNECTION_ID() as id`; diff --git a/test/js/sql/sql-timer-drain.test.ts b/test/js/sql/sql-timer-drain.test.ts deleted file mode 100644 index 43fd11f97e6c..000000000000 --- a/test/js/sql/sql-timer-drain.test.ts +++ /dev/null @@ -1,160 +0,0 @@ -import { SQL } from "bun"; -import { expect, test } from "bun:test"; -import { - listeningServer, - pgAuthenticationOk, - pgCommandComplete, - pgDataRow, - pgRaw, - pgReadyForQuery, - pgRowDescription, -} from "./wire-frames"; - -// Regression test for #30646. Before the fix, idle_timeout / max_lifetime fired -// `failFmt(...)` unconditionally and rejected any in-flight query with -// ERR_POSTGRES_IDLE_TIMEOUT / ERR_POSTGRES_LIFETIME_TIMEOUT — even though the -// query itself was healthy. After the fix: -// -// - The Postgres idle timer doesn't arm while a query is outstanding (a -// queued request or `is_ready_for_query == false` drops it). -// - When max_lifetime fires on a busy connection it reschedules for 1s and -// retries until the connection is idle, then disconnects gracefully. -// -// The mock server exposes deterministic timing: respond to SELECT only after a -// configurable delay, so we can prove queries complete even when the client- -// side timer interval is much shorter than the server response. - -// Startup-phase handshake (no SSL): AuthenticationOk + ReadyForQuery(idle). -const HANDSHAKE = Buffer.concat([pgAuthenticationOk(), pgReadyForQuery("I")]); - -// Full response to Bun's extended-query `Parse+Describe+Bind+Execute+Flush+Sync` -// batch for `SELECT 42 as x`: ParseComplete + ParameterDescription (0 params) + -// RowDescription + BindComplete + DataRow + CommandComplete + ReadyForQuery. -// Column type 23 = int4. -const QUERY_RESPONSE = Buffer.concat([ - pgRaw("1", Buffer.alloc(0)), // ParseComplete - pgRaw("t", Buffer.from([0, 0])), // ParameterDescription, 0 params - pgRowDescription([{ name: "x", typeOid: 23, typeSize: 4 }]), - pgRaw("2", Buffer.alloc(0)), // BindComplete - pgDataRow([Buffer.from("42")]), - pgCommandComplete("SELECT 1"), - pgReadyForQuery("I"), -]); - -/** - * Mock Postgres server: on the startup packet, reply with the handshake; on a - * client query batch, wait `queryDelayMs` then send the minimal result. Buffers - * inbound bytes and responds once per `Sync`/`Simple Query` so TCP chunking - * can't produce duplicate responses. `onClose` observes the server-side socket - * close. - */ -async function startMockServer( - queryDelayMs: number, - onClose?: () => void, -): Promise<{ port: number; stop: () => void }> { - const timers = new Set(); - const { port, server } = await listeningServer(socket => { - // 'startup' -> reply HANDSHAKE to any first packet; - // 'query' -> parse length-prefixed messages, respond on Sync/Simple Query. - let state: "startup" | "query" = "startup"; - let buf: Buffer = Buffer.alloc(0); - socket.on("data", chunk => { - if (state === "startup") { - state = "query"; - socket.write(HANDSHAKE); - return; - } - buf = buf.length === 0 ? chunk : Buffer.concat([buf, chunk]); - // Message format: type(1) + length(4 BE, includes the length field). - while (buf.length >= 5) { - const len = buf.readInt32BE(1); - const total = 1 + len; - if (buf.length < total) break; - const type = buf[0]; - buf = buf.subarray(total); - if (type === 0x53 /* 'S' Sync */ || type === 0x51 /* 'Q' Simple Query */) { - const t = setTimeout(() => { - timers.delete(t); - if (!socket.destroyed) socket.write(QUERY_RESPONSE); - }, queryDelayMs); - timers.add(t); - } - // Other message types (Parse/Bind/Describe/Execute/Flush) need no - // immediate response — the response batch goes out on Sync. - } - }); - socket.on("close", () => { - onClose?.(); - }); - socket.on("error", () => {}); - }); - return { - port, - stop: () => { - for (const t of timers) clearTimeout(t); - server.close(); - }, - }; -} - -test("idleTimeout does not kill an in-flight query (#30646)", async () => { - // Server takes 2s to respond to the query; client idleTimeout is 1s (in - // seconds — Bun multiplies by 1000 internally). Pre-fix: rejects with - // ERR_POSTGRES_IDLE_TIMEOUT. Post-fix: query completes. - const { port, stop } = await startMockServer(2000); - try { - await using sql = new SQL({ - url: `postgres://u@127.0.0.1:${port}/db?sslmode=disable`, - max: 1, - idleTimeout: 1, - }); - const result = await sql`SELECT 42 as x`; - expect(result[0].x).toBe(42); - } finally { - stop(); - } -}, 30_000); - -test("maxLifetime does not kill an in-flight query (#30646)", async () => { - // Server takes 2s to respond; client max_lifetime is 1s. Pre-fix: rejects - // with ERR_POSTGRES_LIFETIME_TIMEOUT. Post-fix: query completes, then the - // connection closes after it's idle. - const { port, stop } = await startMockServer(2000); - try { - await using sql = new SQL({ - url: `postgres://u@127.0.0.1:${port}/db?sslmode=disable`, - max: 1, - maxLifetime: 1, - }); - const result = await sql`SELECT 42 as x`; - expect(result[0].x).toBe(42); - } finally { - stop(); - } -}, 30_000); - -test("maxLifetime closes an idle connection so the pool can reconnect (#30646)", async () => { - // After the first query completes the connection is idle. The max_lifetime - // timer fires, `disconnect()` runs, and the server sees the socket close. - const { promise: closedOnServer, resolve: onServerClose } = Promise.withResolvers(); - const { port, stop } = await startMockServer(0, onServerClose); - try { - await using sql = new SQL({ - url: `postgres://u@127.0.0.1:${port}/db?sslmode=disable`, - max: 1, - maxLifetime: 1, - // Keep the idle timer out of the way — we want to prove maxLifetime - // alone retires the connection. - idleTimeout: 0, - }); - const result = await sql`SELECT 42 as x`; - expect(result[0].x).toBe(42); - - // Deterministic wait — the test's 30s budget bounds the flake risk. - await closedOnServer; - - await sql.close({ timeout: 0 }).catch(() => {}); - } finally { - stop(); - } -}, 30_000); diff --git a/test/js/sql/sql.test.ts b/test/js/sql/sql.test.ts index 121c8b09b934..c235903fe1ac 100644 --- a/test/js/sql/sql.test.ts +++ b/test/js/sql/sql.test.ts @@ -843,8 +843,12 @@ if (isDockerEnabled()) { const [{ pid: pidBefore }] = await sql`select pg_backend_pid() as pid`; expect(onconnect).toHaveBeenCalledTimes(1); - // maxLifetime fires while the connection is idle and closes it gracefully. - await onClosePromise.promise; + // maxLifetime fires while the connection is idle and retires it with the + // documented error code. + const err = await onClosePromise.promise; + expect(err).toBeInstanceOf(SQL.SQLError); + expect(err).toBeInstanceOf(SQL.PostgresError); + expect(err.code).toBe(`ERR_POSTGRES_LIFETIME_TIMEOUT`); expect(onclose).toHaveBeenCalledTimes(1); // The pool transparently reconnects — new backend pid. @@ -870,13 +874,17 @@ if (isDockerEnabled()) { // Before the fix this would reject with ERR_POSTGRES_LIFETIME_TIMEOUT. const [{ pid: pidBefore }] = await sql`select pg_backend_pid() as pid`; const result = await sql`select pg_sleep(3), 42 as x`; + // The query completed — the lifetime timer did not kill it mid-flight. + // (Retirement happens at the completion boundary, so onclose may already + // have fired by the time this continuation runs.) expect(result[0].x).toBe(42); - // The lifetime timer must not have killed the query mid-flight. - expect(onclose).not.toHaveBeenCalled(); - // Once the query returns, the connection is idle and the deferred lifetime - // timer closes it. The pool should then reconnect on a fresh backend. - await onClosePromise.promise; + // Once the query returns, the deferred lifetime retirement closes the + // connection with the documented error code, and the pool reconnects. + const err = await onClosePromise.promise; + expect(err).toBeInstanceOf(SQL.SQLError); + expect(err).toBeInstanceOf(SQL.PostgresError); + expect(err.code).toBe(`ERR_POSTGRES_LIFETIME_TIMEOUT`); expect(onclose).toHaveBeenCalledTimes(1); const [{ pid: pidAfter }] = await sql`select pg_backend_pid() as pid`; From 390a6bce179ce0e2e87c62cea8b1a13ce624c187 Mon Sep 17 00:00:00 2001 From: robobun Date: Thu, 13 Aug 2026 01:44:50 +0000 Subject: [PATCH 05/10] sql: trim comments to repo limit --- src/sql_jsc/mysql/JSMySQLConnection.rs | 3 +-- src/sql_jsc/postgres/PostgresSQLConnection.rs | 19 +++++++------------ src/sql_jsc/postgres/PostgresSQLQuery.rs | 6 ++---- 3 files changed, 10 insertions(+), 18 deletions(-) diff --git a/src/sql_jsc/mysql/JSMySQLConnection.rs b/src/sql_jsc/mysql/JSMySQLConnection.rs index 47f38c4bd890..3c64124c3562 100644 --- a/src/sql_jsc/mysql/JSMySQLConnection.rs +++ b/src/sql_jsc/mysql/JSMySQLConnection.rs @@ -316,8 +316,7 @@ impl JSMySQLConnection { return; } - // Don't kill a healthy in-flight query (#30646): reschedule and retry - // once the connection is idle, then fail with the lifetime error as before. + // Don't kill a healthy in-flight query (#30646): retry once idle. if self.connection.get().status == my_sql_connection::Status::Connected && !self.connection.get().is_idle() { diff --git a/src/sql_jsc/postgres/PostgresSQLConnection.rs b/src/sql_jsc/postgres/PostgresSQLConnection.rs index 5ef4617b25c9..3c883273e757 100644 --- a/src/sql_jsc/postgres/PostgresSQLConnection.rs +++ b/src/sql_jsc/postgres/PostgresSQLConnection.rs @@ -365,9 +365,7 @@ impl PostgresSQLConnection { fn get_timeout_interval(&self) -> u32 { match self.status.get() { - // The idle timer is only relevant when the connection is actually idle. - // While a query is queued or in-flight we must not arm it — otherwise - // we'd race a healthy query and kill it when the timer fires (#30646). + // Never arm the idle timer while a query is outstanding (#30646). Status::Connected if self.has_query_running() => 0, Status::Connected => self.idle_timeout_interval_ms, Status::Failed => 0, @@ -542,9 +540,8 @@ impl PostgresSQLConnection { return; } - // A busy `.connected` connection never reaches here: `get_timeout_interval()` - // returns 0 for it (via `has_query_running()`), so the early return above - // fires. Reaching this match with `Connected` therefore implies idle. + // `Connected` here implies idle: `get_timeout_interval()` returns 0 + // for a busy connection, taking the early return above. use bun_core::fmt::{ConnTimeoutKind::*, fmt_conn_timeout}; let (code, kind, ms, sfx): (&[u8], _, _, _) = match self.status.get() { Status::Connected => ( @@ -577,9 +574,8 @@ impl PostgresSQLConnection { return; } - // Don't kill a healthy in-flight query (#30646): mark the connection - // instead and retire it at the next queue-drain boundary (the - // ReadyForQuery arm, before advance() dispatches more work). + // Don't kill a healthy in-flight query (#30646): retire at the next + // queue-drain boundary instead (the ReadyForQuery arm). if self.status.get() == Status::Connected && self.has_query_running() { self.update_flags(|f| f.insert(ConnectionFlags::LIFETIME_EXCEEDED)); return; @@ -2551,9 +2547,8 @@ impl PostgresSQLConnection { } } - // maxLifetime expired while a query was in flight; the query has - // now completed, so retire the connection before advance() - // dispatches more work. The on_data loop stops at Status::Failed. + // maxLifetime expired mid-query; the query is done, so retire + // before advance() dispatches more work. if self .flags .get() diff --git a/src/sql_jsc/postgres/PostgresSQLQuery.rs b/src/sql_jsc/postgres/PostgresSQLQuery.rs index 7ae81013623f..dcc682deaa25 100644 --- a/src/sql_jsc/postgres/PostgresSQLQuery.rs +++ b/src/sql_jsc/postgres/PostgresSQLQuery.rs @@ -867,10 +867,8 @@ impl PostgresSQLQuery { // For unnamed prepared statements with params, we skip writeQuery+Sync // in the enqueue path and let advance() handle it atomically. connection.advance_and_flush(); - // After advance(): it may have discarded the request synchronously - // (e.g. serialization failure), and with the idle timer gated on - // has_query_running() a reset before advance() would leave an idle - // connection with no timer armed. + // After advance(): it can discard the request synchronously, and the + // idle timer must re-arm for a connection that just became idle. connection.reset_connection_timeout(); } Ok(JSValue::UNDEFINED) From 8223f8132614990dfcbd47774fcedc384b918a8e Mon Sep 17 00:00:00 2001 From: robobun Date: Thu, 13 Aug 2026 03:22:28 +0000 Subject: [PATCH 06/10] sql: retire lifetime-expired MySQL connections at the queue drain boundary; gate Postgres retirement on the head request finishing --- src/sql_jsc/mysql/JSMySQLConnection.rs | 24 +++++++++++++++---- src/sql_jsc/mysql/MySQLConnection.rs | 10 ++++++++ src/sql_jsc/mysql/MySQLRequestQueue.rs | 10 ++++++++ src/sql_jsc/postgres/PostgresSQLConnection.rs | 11 +++++++-- test/js/sql/sql-mysql.test.ts | 4 +--- test/js/sql/sql.test.ts | 23 ++++++++++++++++++ 6 files changed, 72 insertions(+), 10 deletions(-) diff --git a/src/sql_jsc/mysql/JSMySQLConnection.rs b/src/sql_jsc/mysql/JSMySQLConnection.rs index 3c64124c3562..d595c173858b 100644 --- a/src/sql_jsc/mysql/JSMySQLConnection.rs +++ b/src/sql_jsc/mysql/JSMySQLConnection.rs @@ -316,17 +316,19 @@ impl JSMySQLConnection { return; } - // Don't kill a healthy in-flight query (#30646): retry once idle. + // Don't kill a healthy in-flight query (#30646): flag the connection + // and retire at the next queue-drain boundary instead. if self.connection.get().status == my_sql_connection::Status::Connected && !self.connection.get().is_idle() { - self.max_lifetime_timer.with_mut(|t| { - t.next = timespec::ms_from_now(TimespecMockMode::AllowMockedTime, 1000); - self.vm_mut().timer().insert(t); - }); + self.connection_mut().set_lifetime_exceeded(); return; } + self.fail_lifetime_timeout(); + } + + fn fail_lifetime_timeout(&self) { use bun_core::fmt::{ConnTimeoutKind, fmt_conn_timeout}; self.fail_fmt( AnyMySQLErrorT::LifetimeTimeout, @@ -341,6 +343,18 @@ impl JSMySQLConnection { ); } + /// Retires a connection whose maxLifetime expired mid-query, once the + /// queue has nothing in flight. Returns true when it failed the connection. + pub(crate) fn retire_if_lifetime_exceeded(&self) -> bool { + if self.connection.get().status != my_sql_connection::Status::Connected + || !self.connection.get().is_lifetime_exceeded() + { + return false; + } + self.fail_lifetime_timeout(); + true + } + fn setup_max_lifetime_timer_if_necessary(&self) { if self.max_lifetime_interval_ms == 0 { return; diff --git a/src/sql_jsc/mysql/MySQLConnection.rs b/src/sql_jsc/mysql/MySQLConnection.rs index b1e82173e9df..fd8aae51ebd8 100644 --- a/src/sql_jsc/mysql/MySQLConnection.rs +++ b/src/sql_jsc/mysql/MySQLConnection.rs @@ -215,6 +215,16 @@ impl MySQLConnection { self.queue.current().is_none() && self.write_buffer.len() == 0 } + #[inline] + pub(crate) fn set_lifetime_exceeded(&mut self) { + self.flags.insert(ConnectionFlags::LIFETIME_EXCEEDED); + } + + #[inline] + pub(crate) fn is_lifetime_exceeded(&self) -> bool { + self.flags.contains(ConnectionFlags::LIFETIME_EXCEEDED) + } + #[inline] pub(crate) fn enqueue_request(&mut self, request: *mut JSMySQLQuery) { self.queue.add(request); diff --git a/src/sql_jsc/mysql/MySQLRequestQueue.rs b/src/sql_jsc/mysql/MySQLRequestQueue.rs index a1852d5f214e..4b9c1a94fc83 100644 --- a/src/sql_jsc/mysql/MySQLRequestQueue.rs +++ b/src/sql_jsc/mysql/MySQLRequestQueue.rs @@ -125,6 +125,16 @@ impl MySQLRequestQueue { // momentary `Deref` lifetime. All queue mutation below goes through // `Cell`/`JsCell` interior mutability — `&Self` is sufficient. let queue_ref: ParentRef = ParentRef::new(&conn_ref.connection.get().queue); + + // maxLifetime expired mid-query (#30646): retire at the drain boundary, + // before dispatching more work, instead of killing the in-flight query. + if queue_ref.pipelined_requests.get() == 0 + && queue_ref.nonpipelinable_requests.get() == 0 + && conn_ref.retire_if_lifetime_exceeded() + { + return; + } + // reshaped for borrowck — the cleanup that must run at function exit // became a post-block pass; early returns become // `break 'advance` so cleanup always runs at function exit. diff --git a/src/sql_jsc/postgres/PostgresSQLConnection.rs b/src/sql_jsc/postgres/PostgresSQLConnection.rs index 3c883273e757..b9defe2f9c5c 100644 --- a/src/sql_jsc/postgres/PostgresSQLConnection.rs +++ b/src/sql_jsc/postgres/PostgresSQLConnection.rs @@ -2547,12 +2547,19 @@ impl PostgresSQLConnection { } } - // maxLifetime expired mid-query; the query is done, so retire - // before advance() dispatches more work. + // maxLifetime expired mid-query: retire before advance() dispatches + // more work, but only once the head request is finished. A named + // statement's Parse+Describe+Sync gets its own ReadyForQuery first. if self .flags .get() .contains(ConnectionFlags::LIFETIME_EXCEEDED) + && self.current().is_none_or(|request| { + matches!( + request.status.get(), + QueryStatus::Success | QueryStatus::Fail + ) + }) { self.fail_lifetime_timeout(); self.update_ref(); diff --git a/test/js/sql/sql-mysql.test.ts b/test/js/sql/sql-mysql.test.ts index 274c177e78bb..bd93179e5f18 100644 --- a/test/js/sql/sql-mysql.test.ts +++ b/test/js/sql/sql-mysql.test.ts @@ -364,10 +364,8 @@ if (isDockerEnabled()) { // Before the fix this rejected with ERR_MYSQL_LIFETIME_TIMEOUT. const result = await sql`select SLEEP(3) as s, 42 as x`; expect(result[0].x).toBe(42); - // The lifetime timer must not have killed the query mid-flight. - expect(onclose).not.toHaveBeenCalled(); - // Once idle, the rescheduled timer retires the connection with the + // The connection is then retired at the drain boundary with the // documented error code, and the pool reconnects. const err = await onClosePromise.promise; expect(err).toBeInstanceOf(SQL.SQLError); diff --git a/test/js/sql/sql.test.ts b/test/js/sql/sql.test.ts index c235903fe1ac..43ec57afc6b6 100644 --- a/test/js/sql/sql.test.ts +++ b/test/js/sql/sql.test.ts @@ -891,6 +891,29 @@ if (isDockerEnabled()) { expect(pidAfter).not.toBe(pidBefore); }, 30_000); + test("Max lifetime does not kill an in-flight parameterized query (#30646)", async () => { + const onClosePromise = Promise.withResolvers(); + const onclose = mock(err => { + onClosePromise.resolve(err); + }); + await using sql = postgres({ + ...options, + max_lifetime: 1, + onclose, + max: 1, + }); + + // A parameterized query uses a named statement (Parse+Describe+Sync gets + // its own ReadyForQuery before Bind+Execute). Retirement must wait for + // the query to finish, not fire on the prepare round-trip. + const result = await sql`select pg_sleep(2), ${42}::int as x`; + expect(result[0].x).toBe(42); + + const err = await onClosePromise.promise; + expect(err).toBeInstanceOf(SQL.PostgresError); + expect(err.code).toBe(`ERR_POSTGRES_LIFETIME_TIMEOUT`); + }, 30_000); + test("Idle timeout does not kill an in-flight query (#30646)", async () => { const onClosePromise = Promise.withResolvers(); const onclose = mock(err => { From 98bbb838e4a003efb64feb1c1d9be548107e5655 Mon Sep 17 00:00:00 2001 From: robobun Date: Thu, 13 Aug 2026 03:29:32 +0000 Subject: [PATCH 07/10] sql: tighten comments --- src/sql_jsc/mysql/JSMySQLConnection.rs | 6 ++---- src/sql_jsc/mysql/MySQLRequestQueue.rs | 3 +-- src/sql_jsc/postgres/PostgresSQLConnection.rs | 5 ++--- 3 files changed, 5 insertions(+), 9 deletions(-) diff --git a/src/sql_jsc/mysql/JSMySQLConnection.rs b/src/sql_jsc/mysql/JSMySQLConnection.rs index d595c173858b..d10edb552a07 100644 --- a/src/sql_jsc/mysql/JSMySQLConnection.rs +++ b/src/sql_jsc/mysql/JSMySQLConnection.rs @@ -316,8 +316,7 @@ impl JSMySQLConnection { return; } - // Don't kill a healthy in-flight query (#30646): flag the connection - // and retire at the next queue-drain boundary instead. + // Don't kill an in-flight query (#30646): retire at the next drain boundary. if self.connection.get().status == my_sql_connection::Status::Connected && !self.connection.get().is_idle() { @@ -343,8 +342,7 @@ impl JSMySQLConnection { ); } - /// Retires a connection whose maxLifetime expired mid-query, once the - /// queue has nothing in flight. Returns true when it failed the connection. + /// Fails the connection if maxLifetime expired mid-query; returns true when it did. pub(crate) fn retire_if_lifetime_exceeded(&self) -> bool { if self.connection.get().status != my_sql_connection::Status::Connected || !self.connection.get().is_lifetime_exceeded() diff --git a/src/sql_jsc/mysql/MySQLRequestQueue.rs b/src/sql_jsc/mysql/MySQLRequestQueue.rs index 4b9c1a94fc83..fdd7fdddcce4 100644 --- a/src/sql_jsc/mysql/MySQLRequestQueue.rs +++ b/src/sql_jsc/mysql/MySQLRequestQueue.rs @@ -126,8 +126,7 @@ impl MySQLRequestQueue { // `Cell`/`JsCell` interior mutability — `&Self` is sufficient. let queue_ref: ParentRef = ParentRef::new(&conn_ref.connection.get().queue); - // maxLifetime expired mid-query (#30646): retire at the drain boundary, - // before dispatching more work, instead of killing the in-flight query. + // maxLifetime expired mid-query (#30646): retire before dispatching more work. if queue_ref.pipelined_requests.get() == 0 && queue_ref.nonpipelinable_requests.get() == 0 && conn_ref.retire_if_lifetime_exceeded() diff --git a/src/sql_jsc/postgres/PostgresSQLConnection.rs b/src/sql_jsc/postgres/PostgresSQLConnection.rs index b9defe2f9c5c..c1971524e784 100644 --- a/src/sql_jsc/postgres/PostgresSQLConnection.rs +++ b/src/sql_jsc/postgres/PostgresSQLConnection.rs @@ -2547,9 +2547,8 @@ impl PostgresSQLConnection { } } - // maxLifetime expired mid-query: retire before advance() dispatches - // more work, but only once the head request is finished. A named - // statement's Parse+Describe+Sync gets its own ReadyForQuery first. + // maxLifetime expired mid-query (#30646): retire only once the head is + // finished; a named statement's Parse+Describe+Sync RFQs first. if self .flags .get() From 61052aa6fa7663bed983e6e3dc18c365fdf5d16a Mon Sep 17 00:00:00 2001 From: robobun Date: Thu, 13 Aug 2026 04:57:29 +0000 Subject: [PATCH 08/10] sql(mysql): don't retire a lifetime-expired connection while the head request is mid-prepare --- src/sql_jsc/mysql/MySQLRequestQueue.rs | 4 +++- test/js/sql/sql-mysql.test.ts | 22 ++++++++++++++++++++++ 2 files changed, 25 insertions(+), 1 deletion(-) diff --git a/src/sql_jsc/mysql/MySQLRequestQueue.rs b/src/sql_jsc/mysql/MySQLRequestQueue.rs index fdd7fdddcce4..ff2953df1123 100644 --- a/src/sql_jsc/mysql/MySQLRequestQueue.rs +++ b/src/sql_jsc/mysql/MySQLRequestQueue.rs @@ -126,9 +126,11 @@ impl MySQLRequestQueue { // `Cell`/`JsCell` interior mutability — `&Self` is sufficient. let queue_ref: ParentRef = ParentRef::new(&conn_ref.connection.get().queue); - // maxLifetime expired mid-query (#30646): retire before dispatching more work. + // maxLifetime expired mid-query (#30646): retire before dispatching more + // work, once the head finished (a prepare in flight bumps neither counter). if queue_ref.pipelined_requests.get() == 0 && queue_ref.nonpipelinable_requests.get() == 0 + && queue_ref.current_ref().is_none_or(|r| r.is_completed()) && conn_ref.retire_if_lifetime_exceeded() { return; diff --git a/test/js/sql/sql-mysql.test.ts b/test/js/sql/sql-mysql.test.ts index bd93179e5f18..6bc0fb8a791e 100644 --- a/test/js/sql/sql-mysql.test.ts +++ b/test/js/sql/sql-mysql.test.ts @@ -377,6 +377,28 @@ if (isDockerEnabled()) { expect(connAfter).not.toBe(connBefore); }, 30_000); + test("Max lifetime does not kill an in-flight parameterized query (#30646)", async () => { + const onClosePromise = Promise.withResolvers(); + const onclose = mock(err => { + onClosePromise.resolve(err); + }); + await using sql = new SQL({ + ...getOptions(), + max_lifetime: 1, + onclose, + max: 1, + }); + + // A parameterized query goes through COM_STMT_PREPARE before + // execute; retirement must wait for the query to finish. + const result = await sql`select SLEEP(2) as s, ${42} as x`; + expect(result[0].x).toBe(42); + + const err = await onClosePromise.promise; + expect(err).toBeInstanceOf(SQL.MySQLError); + expect((err as SQL.MySQLError).code).toBe(`ERR_MYSQL_LIFETIME_TIMEOUT`); + }, 30_000); + // Last one wins. test("Handles duplicate string column names", async () => { const result = await sql`select 1 as x, 2 as x, 3 as x`; From 8730e98a98d1e75da546eb22c3f1fa001f856ed9 Mon Sep 17 00:00:00 2001 From: robobun Date: Thu, 13 Aug 2026 05:50:29 +0000 Subject: [PATCH 09/10] sql: drop a redundant idle-timeout test; tighten test comments --- test/js/sql/sql-mysql.test.ts | 2 +- test/js/sql/sql.test.ts | 29 +++-------------------------- 2 files changed, 4 insertions(+), 27 deletions(-) diff --git a/test/js/sql/sql-mysql.test.ts b/test/js/sql/sql-mysql.test.ts index 6bc0fb8a791e..b036a71e1fb5 100644 --- a/test/js/sql/sql-mysql.test.ts +++ b/test/js/sql/sql-mysql.test.ts @@ -390,7 +390,7 @@ if (isDockerEnabled()) { }); // A parameterized query goes through COM_STMT_PREPARE before - // execute; retirement must wait for the query to finish. + // execute; cover that path end to end. const result = await sql`select SLEEP(2) as s, ${42} as x`; expect(result[0].x).toBe(42); diff --git a/test/js/sql/sql.test.ts b/test/js/sql/sql.test.ts index 43ec57afc6b6..1a624b12e33f 100644 --- a/test/js/sql/sql.test.ts +++ b/test/js/sql/sql.test.ts @@ -780,30 +780,8 @@ if (isDockerEnabled()) { expect(onclose).toHaveBeenCalledTimes(1); }); - test("Idle timeout fires only when the connection is truly idle", async () => { - const onClosePromise = Promise.withResolvers(); - const onclose = mock(err => { - onClosePromise.resolve(err); - }); - const onconnect = mock(); - await using sql = postgres({ - ...options, - idle_timeout: 1, - onconnect, - onclose, - }); - // A query longer than idle_timeout must not be killed by the idle timer (#30646). - expect(await sql`select pg_sleep(2)`).toEqual([{ pg_sleep: "" }]); - expect(onconnect).toHaveBeenCalled(); - // The timer must not have fired while the query was in flight. - expect(onclose).not.toHaveBeenCalled(); - // After the query returns, the connection is idle — the timer fires shortly after. - const err = await onClosePromise.promise; - expect(err).toBeInstanceOf(SQL.SQLError); - expect(err).toBeInstanceOf(SQL.PostgresError); - expect(err.code).toBe(`ERR_POSTGRES_IDLE_TIMEOUT`); - expect(onclose).toHaveBeenCalledTimes(1); - }); + // "Idle timeout does not kill an in-flight query (#30646)" below covers + // the in-flight case (the old "Idle timeout works at start" asserted the kill). test("Idle timeout is reset when a query is run", async () => { const onClosePromise = Promise.withResolvers(); @@ -904,8 +882,7 @@ if (isDockerEnabled()) { }); // A parameterized query uses a named statement (Parse+Describe+Sync gets - // its own ReadyForQuery before Bind+Execute). Retirement must wait for - // the query to finish, not fire on the prepare round-trip. + // its own ReadyForQuery before Bind+Execute); cover that path end to end. const result = await sql`select pg_sleep(2), ${42}::int as x`; expect(result[0].x).toBe(42); From d76bf1799a38a48ca36f40cdc4a78fd4b9177d2e Mon Sep 17 00:00:00 2001 From: robobun Date: Fri, 14 Aug 2026 23:13:04 +0000 Subject: [PATCH 10/10] sql: add serverless twins of the in-flight timer tests The container suites are skipped entirely where docker and the test services are unavailable, leaving #30646 unprovable there. The wire mock delays the query response past the client-side timer; on main both tests reject with ERR_POSTGRES_IDLE_TIMEOUT / ERR_POSTGRES_LIFETIME_TIMEOUT. --- test/js/sql/sql.test.ts | 115 +++++++++++++++++++++++++++++++++++++++- 1 file changed, 114 insertions(+), 1 deletion(-) diff --git a/test/js/sql/sql.test.ts b/test/js/sql/sql.test.ts index 1a624b12e33f..387d1904741c 100644 --- a/test/js/sql/sql.test.ts +++ b/test/js/sql/sql.test.ts @@ -15,7 +15,16 @@ function rel(filename: string) { // Use docker-compose infrastructure import * as dockerCompose from "../../docker/index.ts"; import { UnixDomainSocketProxy } from "../../unix-domain-socket-proxy.ts"; -import { neverAnsweringServer } from "./wire-frames"; +import { + listeningServer, + neverAnsweringServer, + pgAuthenticationOk, + pgCommandComplete, + pgDataRow, + pgRaw, + pgReadyForQuery, + pgRowDescription, +} from "./wire-frames"; if (isDockerEnabled()) { describe("PostgreSQL tests", async () => { @@ -12849,3 +12858,107 @@ test("data row that omits columns declared in the row description yields nulls f expect(filteredStderr).toBe(""); expect(exitCode).toBe(0); }, 30_000); + +// Serverless twins of the in-flight timer tests (#30646). The container suites +// above cover the same scenarios against a real server (which a mock must not +// replace for producible behavior like a slow SELECT), but they are skipped +// entirely where docker and the test services are unavailable, so these keep +// the regression executable everywhere: the mock delays the query response +// past the client-side timer, which on main kills the in-flight query. +describe.concurrent("timers do not kill in-flight queries (no server, #30646)", () => { + // Startup-phase handshake (no SSL): AuthenticationOk + ReadyForQuery(idle). + const HANDSHAKE = Buffer.concat([pgAuthenticationOk(), pgReadyForQuery("I")]); + + // Response to the extended-query batch for `SELECT 42 as x` (no params): + // ParseComplete + ParameterDescription + RowDescription + BindComplete + + // DataRow + CommandComplete + ReadyForQuery. Type oid 23 = int4. + const QUERY_RESPONSE = Buffer.concat([ + pgRaw("1", Buffer.alloc(0)), // ParseComplete + pgRaw("t", Buffer.from([0, 0])), // ParameterDescription, 0 params + pgRowDescription([{ name: "x", typeOid: 23, typeSize: 4 }]), + pgRaw("2", Buffer.alloc(0)), // BindComplete + pgDataRow([Buffer.from("42")]), + pgCommandComplete("SELECT 1"), + pgReadyForQuery("I"), + ]); + + // Replies to the startup packet immediately, then answers each query batch + // (one response per Sync / Simple Query) after `queryDelayMs`. + async function delayedServer(queryDelayMs: number): Promise<{ port: number; stop: () => void }> { + const timers = new Set(); + const { port, server } = await listeningServer(socket => { + let state: "startup" | "query" = "startup"; + let buf: Buffer = Buffer.alloc(0); + socket.on("data", chunk => { + if (state === "startup") { + state = "query"; + socket.write(HANDSHAKE); + return; + } + buf = buf.length === 0 ? chunk : Buffer.concat([buf, chunk]); + // Message format: type(1) + length(4 BE, length field included). + while (buf.length >= 5) { + const total = 1 + buf.readInt32BE(1); + if (buf.length < total) break; + const type = buf[0]; + buf = buf.subarray(total); + if (type === 0x53 /* Sync */ || type === 0x51 /* Simple Query */) { + const t = setTimeout(() => { + timers.delete(t); + if (!socket.destroyed) socket.write(QUERY_RESPONSE); + }, queryDelayMs); + timers.add(t); + } + } + }); + socket.on("error", () => {}); + }); + return { + port, + stop: () => { + for (const t of timers) clearTimeout(t); + server.close(); + }, + }; + } + + test("idleTimeout waits for the in-flight query", async () => { + // Server answers after 3s; idleTimeout is 1s. On main the idle timer kills + // the connection mid-query with ERR_POSTGRES_IDLE_TIMEOUT. + const { port, stop } = await delayedServer(3000); + try { + await using sql = new SQL({ + url: `postgres://u@127.0.0.1:${port}/db?sslmode=disable`, + max: 1, + idleTimeout: 1, + }); + const result = await sql`SELECT 42 as x`; + expect(result[0].x).toBe(42); + } finally { + stop(); + } + }, 30_000); + + test("maxLifetime waits for the in-flight query, then retires the connection", async () => { + // Server answers after 3s; maxLifetime is 1s. On main the lifetime timer + // kills the connection mid-query with ERR_POSTGRES_LIFETIME_TIMEOUT. + const onClosePromise = Promise.withResolvers(); + const { port, stop } = await delayedServer(3000); + try { + await using sql = new SQL({ + url: `postgres://u@127.0.0.1:${port}/db?sslmode=disable`, + max: 1, + maxLifetime: 1, + onclose: err => onClosePromise.resolve(err), + }); + const result = await sql`SELECT 42 as x`; + expect(result[0].x).toBe(42); + // The deferred retirement still enforces maxLifetime once the query is done. + const err = (await onClosePromise.promise) as SQL.PostgresError; + expect(err).toBeInstanceOf(SQL.PostgresError); + expect(err.code).toBe(`ERR_POSTGRES_LIFETIME_TIMEOUT`); + } finally { + stop(); + } + }, 30_000); +});