From 53a46a255d835a3df8a593693e9f114251dcf0a8 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Sun, 14 Jun 2026 20:40:24 +0000 Subject: [PATCH 1/2] sql: grow connection pool lazily instead of opening max on first use MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rebuilt on main (pool plumbing consolidated into BaseSQLAdapter / BasePooledConnection in shared.ts). Previously connect() instantiated every pool slot eagerly on the first query, and each pooled-connection constructor dials TCP immediately, so a single SELECT 1 on a max: 500 pool burst 500 connections. - Pool starts empty; #tryGrowPool() appends one connection at a time (push, so the array is dense — no unassigned holes), capped at maxPoolSize (= options.max). - First connect() opens exactly one connection. - flushConcurrentQueries() grows the pool when queued queries can't be served by ready or pending slots, discounting pending slots already earmarked for reservedQueue. - maxDistribution() divides by the pool ceiling, not the current size. - connect()'s retry scan revives one closed slot per waiter (break after the first), so a query arriving after the pool idled out doesn't redial every slot. - Non-retryable auth failures fail fast with the cached error, unless password is a function (dynamic credential), in which case a closed slot is force-retried so a rotated token can take effect at max: 1. Because the array is dense, the unassigned-hole guards added in #32201 are unnecessary here — a slot being created is pushed only after createPooledConnection returns, so re-entrant scans during a synchronous password() see an empty (not holey) array. Tests: test/js/sql/sql-pool-lazy-growth.test.ts (single query opens 1 socket for both adapters; static-password auth failure fast-fails; function password retries per query at max: 1; synchronous password throw rejects without hanging). Updated sql-close-pending-connection and sql-onconnect-onclose-throw, which asserted the old per-slot eager password/onclose counts at max: 2 — one slot opens per query now. --- src/js/internal/sql/shared.ts | 154 ++++++++++++--- .../sql/sql-close-pending-connection.test.ts | 8 +- .../sql/sql-onconnect-onclose-throw.test.ts | 5 +- test/js/sql/sql-pool-lazy-growth.test.ts | 175 ++++++++++++++++++ 4 files changed, 308 insertions(+), 34 deletions(-) create mode 100644 test/js/sql/sql-pool-lazy-growth.test.ts diff --git a/src/js/internal/sql/shared.ts b/src/js/internal/sql/shared.ts index 192c6ed892ed..78b8537ddf4c 100644 --- a/src/js/internal/sql/shared.ts +++ b/src/js/internal/sql/shared.ts @@ -815,6 +815,16 @@ abstract class BasePooledConnection void, err: Error | null) { @@ -895,7 +905,16 @@ abstract class BaseSQLAdapter = new Set(); public waitingQueue: Array<(err: Error | null, result: any) => void> = []; @@ -908,11 +927,8 @@ abstract class BaseSQLAdapter= this.maxPoolSize) return null; + const connection = this.createPooledConnection(); + this.connections.push(connection); + return connection; + } + + /// Count connections that are still completing their handshake. A pending + /// connection will soon join `readyConnections`, so we don't need to grow + /// the pool further just because no connection is ready *right now*. + #pendingConnectionsCount(): number { + let count = 0; + const len = this.connections.length; + for (let i = 0; i < len; i++) { + if (this.connections[i].state === PooledConnectionState.pending) count++; + } + return count; + } + flushConcurrentQueries() { const maxDistribution = this.maxDistribution(); if (maxDistribution === 0) { @@ -1022,6 +1065,21 @@ abstract class BaseSQLAdapter !(c.flags & PooledConnectionFlags.preReserved) && c.queryCount < maxDistribution, ); if (nonReservedConnections.length === 0) { + // No idle connection can take another query. Grow the pool only if + // the number of still-handshaking connections is less than the + // backlog — otherwise those pending connections will drain the + // queue on their own once they become ready. + // + // `release()` hands freshly-connected slots to `reservedQueue` + // first (and returns early, never feeding `waitingQueue`), so up + // to `reservedQueue.length` pending sockets are already spoken for + // and don't count as capacity for `waitingQueue`. + const pending = this.#pendingConnectionsCount(); + const pendingForWaiting = Math.max(0, pending - this.reservedQueue.length); + const unservedWaiters = this.waitingQueue.length - pendingForWaiting; + if (unservedWaiters > 0 && this.connections.length < this.maxPoolSize) { + this.#tryGrowPool(); + } return; } const orderedConnections = nonReservedConnections.sort((a, b) => a.queryCount - b.queryCount); @@ -1276,21 +1334,24 @@ abstract class BaseSQLAdapter {}); - // the pool-start fill loop runs synchronously inside connect(), invoking - // password() once per pool slot - expect(passwordCalls).toBe(2); + // The pool grows lazily (#30632): the first connect() opens a single + // slot, invoking password() once. The re-entrant connect() inside + // password() runs while that slot is still being created (not yet in + // connections[]), so it enqueues without opening another slot. + expect(passwordCalls).toBe(1); expect(errors).toEqual([]); } finally { // force an immediate close even with waiters queued diff --git a/test/js/sql/sql-onconnect-onclose-throw.test.ts b/test/js/sql/sql-onconnect-onclose-throw.test.ts index 175b32e6dc89..a37a5d13a70b 100644 --- a/test/js/sql/sql-onconnect-onclose-throw.test.ts +++ b/test/js/sql/sql-onconnect-onclose-throw.test.ts @@ -162,6 +162,9 @@ test.concurrent( // close) threw a TypeError on the holes when called from inside the callback. // The callback is now deferred until the pool is fully constructed. Nothing // is dialed: password() throws before the connection is created. +// +// With lazy pool growth (#30632) a single query opens one slot even at +// max: 2, so exactly one onclose fires. test.concurrent("postgres: pool calls from onclose are safe when connecting fails synchronously", async () => { const fixture = /* ts */ ` import { SQL } from "bun"; @@ -194,7 +197,7 @@ try { process.exit(0); `; const { stdout, exitCode } = await runFixture(fixture); - expect(stdout).toBe("reentry ok\nreentry ok\nquery rejected: password error\n"); + expect(stdout).toBe("reentry ok\nquery rejected: password error\n"); expect(exitCode).toBe(0); }); diff --git a/test/js/sql/sql-pool-lazy-growth.test.ts b/test/js/sql/sql-pool-lazy-growth.test.ts new file mode 100644 index 000000000000..28043e27d939 --- /dev/null +++ b/test/js/sql/sql-pool-lazy-growth.test.ts @@ -0,0 +1,175 @@ +// Issue #30632: `new Bun.SQL({ max: N })` must grow the pool lazily on demand, +// not open all N connections up-front. Uses a bare TCP listener as a drop-in +// sink so we can count the opened sockets without needing Docker or a real +// Postgres / MySQL server. +import { SQL } from "bun"; +import { describe, expect, test } from "bun:test"; + +type Adapter = "postgres" | "mysql"; + +function makeSink() { + let opened = 0; + const server = Bun.listen({ + hostname: "127.0.0.1", + port: 0, + socket: { + open() { + opened++; + }, + data() {}, + close() {}, + error() {}, + }, + }); + return { + port: server.port, + [Symbol.dispose]() { + server.stop(); + }, + get opened() { + return opened; + }, + }; +} + +describe.each(["postgres", "mysql"] as Adapter[])("%s connection pool grows lazily (#30632)", adapter => { + test("a single query only opens one TCP connection, not `max`", async () => { + using sink = makeSink(); + await using sql = new SQL({ + adapter, + host: "127.0.0.1", + port: sink.port, + username: "x", + database: "x", + max: 50, + connectionTimeout: 1, + }); + + // Query fails (nothing is speaking the DB protocol on the other end); + // we only care about how many sockets Bun opened. + await sql`SELECT 1`.catch(() => {}); + expect(sink.opened).toBe(1); + }); +}); + +// Followup from #30632 review (@claude-bot / @Lillious): when a connection +// fails with a non-retryable auth error (unsupported auth method, bad +// password, TLS refused, etc.), subsequent queries must fail fast with the +// cached error — not keep opening new sockets to hit the same auth wall. +// Uses a minimal fake server that answers the startup message with an +// AuthenticationRequest carrying an unsupported auth code, which Bun rejects +// as `ERR_POSTGRES_UNSUPPORTED_AUTHENTICATION_METHOD`. +// +// Returns the listener + a counter of opened sockets. Every client write +// (the StartupMessage) gets an AuthenticationRequest with auth code 9 +// (SSPI), which Bun treats as an unsupported method. +// Wire: 'R' (1 byte) + int32 length (4) + int32 auth code (4). +function makeUnsupportedAuthPgServer() { + let opened = 0; + const server = Bun.listen({ + hostname: "127.0.0.1", + port: 0, + socket: { + open() { + opened++; + }, + data(socket) { + const buf = Buffer.alloc(9); + buf.write("R", 0); + buf.writeInt32BE(8, 1); + buf.writeInt32BE(9, 5); + socket.write(buf); + }, + close() {}, + error() {}, + }, + }); + return { + port: server.port, + [Symbol.dispose]() { + server.stop(); + }, + get opened() { + return opened; + }, + }; +} + +describe("postgres pool fast-fails on non-retryable auth errors (#30632)", () => { + test("repeated queries with a static password do not open more sockets after an auth failure", async () => { + using server = makeUnsupportedAuthPgServer(); + await using sql = new SQL({ + adapter: "postgres", + host: "127.0.0.1", + port: server.port, + username: "x", + database: "x", + max: 20, + connectionTimeout: 1, + }); + + // Fire 5 sequential queries. The first one opens a connection, the + // auth handshake fails, and the remaining 4 should reject immediately + // with the cached auth error — no extra sockets. + for (let i = 0; i < 5; i++) { + await sql`SELECT ${i}`.catch(() => {}); + } + expect(server.opened).toBe(1); + }); + + test("function password retries auth on each new query (rotatable credentials)", async () => { + // When `password` is a function, Bun re-invokes it every time it opens + // a new TCP connection, so a rotated IAM token / Vault lease can take + // effect. Verify that after an initial auth failure, subsequent + // queries actually try again — even at `max: 1` where there's no room + // to grow the pool, which forces reuse of the existing closed slot. + using server = makeUnsupportedAuthPgServer(); + await using sql = new SQL({ + adapter: "postgres", + host: "127.0.0.1", + port: server.port, + username: "x", + database: "x", + max: 1, + connectionTimeout: 1, + password: () => "rotating-token", + }); + + for (let i = 0; i < 3; i++) { + await sql`SELECT ${i}`.catch(() => {}); + } + // 3 attempts, each dialing fresh TCP on the same slot. + expect(server.opened).toBe(3); + }); + + test("synchronous `password()` throw does not hang subsequent queries", async () => { + // `createPooledConnectionHandle` in shared.ts defers a thrown + // `password()` to `onClose` via `process.nextTick`. This guards the + // no-hang contract: even if a future change made that path + // synchronous (so `release()` could drain the queue before + // `connect()` enqueues the waiter), both queries must still reject + // with the thrown error instead of hanging. The runner's default + // per-test timeout fails the test if anything hangs. + await using sql = new SQL({ + adapter: "postgres", + host: "127.0.0.1", + port: 1, + username: "x", + database: "x", + max: 1, + password: () => { + throw new Error("boom"); + }, + }); + + for (let i = 0; i < 2; i++) { + let err: any; + try { + await sql`SELECT ${i}`; + } catch (e) { + err = e; + } + expect(err?.message).toBe("boom"); + } + }); +}); From 80d204ae12a08f0047e90964b3d696b0a3415883 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Sun, 14 Jun 2026 21:35:19 +0000 Subject: [PATCH 2/2] sql: bound lazy pool growth under re-entrant function password() createPooledConnection() runs a function-valued password() synchronously before the new slot is appended to connections[], so a password() that re-enters the pool while an earlier slot is still mid-handshake read a stale connections.length in #tryGrowPool()'s cap check and kept opening slots past max (unbounded recursion in the worst case). Track slots that are mid-creation in a #growing counter and include it in the cap check so the limit holds under synchronous re-entrancy. --- src/js/internal/sql/shared.ts | 23 ++++++++++--- test/js/sql/sql-pool-lazy-growth.test.ts | 41 ++++++++++++++++++++++++ 2 files changed, 60 insertions(+), 4 deletions(-) diff --git a/src/js/internal/sql/shared.ts b/src/js/internal/sql/shared.ts index 78b8537ddf4c..b416e92d0e8d 100644 --- a/src/js/internal/sql/shared.ts +++ b/src/js/internal/sql/shared.ts @@ -925,6 +925,14 @@ abstract class BaseSQLAdapter void) | null = null; + /// Count of in-flight `#tryGrowPool()` calls whose slot has not been pushed + /// onto `connections` yet. `createPooledConnection()` runs a function-valued + /// `password()` synchronously before the push, and that user code can + /// re-enter the pool (e.g. issue a query); counting the slot being created + /// keeps a re-entrant grow from recursing past `maxPoolSize` off a stale + /// `connections.length`. + #growing: number = 0; + constructor(connectionInfo: Bun.SQL.__internal.DefinedPostgresOrMySQLOptions) { this.connectionInfo = connectionInfo; this.connections = []; @@ -1036,10 +1044,17 @@ abstract class BaseSQLAdapter= this.maxPoolSize) return null; - const connection = this.createPooledConnection(); - this.connections.push(connection); - return connection; + // Count slots that are mid-creation (see `#growing`) so a `password()` + // re-entering here while another slot is being created can't grow past max. + if (this.connections.length + this.#growing >= this.maxPoolSize) return null; + this.#growing++; + try { + const connection = this.createPooledConnection(); + this.connections.push(connection); + return connection; + } finally { + this.#growing--; + } } /// Count connections that are still completing their handshake. A pending diff --git a/test/js/sql/sql-pool-lazy-growth.test.ts b/test/js/sql/sql-pool-lazy-growth.test.ts index 28043e27d939..ec0ec7c00616 100644 --- a/test/js/sql/sql-pool-lazy-growth.test.ts +++ b/test/js/sql/sql-pool-lazy-growth.test.ts @@ -142,6 +142,47 @@ describe("postgres pool fast-fails on non-retryable auth errors (#30632)", () => expect(server.opened).toBe(3); }); + test("re-entrant function `password()` does not grow the pool past `max`", async () => { + // Lazy growth (#30632) + re-entrant password (#32198): a function-valued + // `password()` runs synchronously while a new slot is being created, + // before that slot is appended to `connections`. If such a password + // re-enters the pool while an earlier slot is still mid-handshake, the + // grow path must not keep opening slots off a stale `connections.length` + // and recurse past `max`. Re-entry is capped here so the pre-fix recursion + // shows up as an inflated password-call count instead of a stack overflow. + using sink = makeSink(); + let passwordCalls = 0; + const sql = new SQL({ + adapter: "postgres", + host: "127.0.0.1", + port: sink.port, + username: "x", + database: "x", + max: 2, + connectionTimeout: 0, // disable the connect timer so slots stay pending + password: () => { + passwordCalls++; + if (passwordCalls < 100) { + try { + sql.connect().catch(() => {}); + } catch {} + } + return "pw"; + }, + }); + try { + // Two back-to-back connects: the first opens slot 0 (still handshaking), + // the second sees it pending and grows exactly one more slot. + // `password()` runs once per real slot — `max` (2) times — not once per + // recursion level. + sql.connect().catch(() => {}); + sql.connect().catch(() => {}); + expect(passwordCalls).toBe(2); + } finally { + await sql.close({ timeout: "0" }); + } + }); + test("synchronous `password()` throw does not hang subsequent queries", async () => { // `createPooledConnectionHandle` in shared.ts defers a thrown // `password()` to `onClose` via `process.nextTick`. This guards the