From 7bb1831064a8f998214d9e3edffd574fd1ac51a6 Mon Sep 17 00:00:00 2001
From: robobun <bot@oven.sh>
Date: Fri, 12 Jun 2026 18:36:51 +0000
Subject: [PATCH 1/2] sql: ignore stale on_connect firing after close in pooled
 connections

When a pooled connection's socket is closed in the same I/O tick as the
handshake completing (e.g. Postgres ReadyForQuery followed by the server's
FIN in one read buffer), handleClose runs synchronously before the queued
on_connect microtask. handleConnected then blindly set state = connected
and re-added the dead entry to readyConnections with connection === null.
Subsequent queries dispatched null to the native query's run() and failed
with 'connection must be a PostgresSQLConnection' forever; the pool never
retried a slot it thought was live.

Bail out of BasePooledConnection.handleConnected when state !== pending.
Covers both the Postgres and MySQL adapters.

Fixes #30947
---
 src/js/internal/sql/shared.ts                 |  13 ++
 .../postgres-close-during-handshake.test.ts   | 151 ++++++++++++++++++
 2 files changed, 164 insertions(+)
 create mode 100644 test/js/sql/postgres-close-during-handshake.test.ts

diff --git a/src/js/internal/sql/shared.ts b/src/js/internal/sql/shared.ts
index 94e82acef85..bfdcae7f0bc 100644
--- a/src/js/internal/sql/shared.ts
+++ b/src/js/internal/sql/shared.ts
@@ -631,6 +631,19 @@ abstract class BasePooledConnection<ConnectionHandle extends { close(): void; fl
   }
 
   protected handleConnected(err: any) {
+    // The native connection queues this callback as a microtask when the
+    // handshake completes (e.g. Postgres ReadyForQuery). If the socket is
+    // closed in the same I/O tick (the server's FIN rides the same read
+    // buffer, or the pool closes mid-handshake), handleClose runs
+    // synchronously first and transitions us to `closed`. When this
+    // microtask then fires, honoring it would resurrect a dead connection
+    // into readyConnections with `this.connection === null`, which
+    // dispatches null to the native query's run() and wedges the pool
+    // permanently (it never retries a slot it thinks is live). Only a slot
+    // still `pending` is legitimately waiting for this callback.
+    if (this.state !== PooledConnectionState.pending) {
+      return;
+    }
     if (err) {
       err = this.wrapError(err);
     }
diff --git a/test/js/sql/postgres-close-during-handshake.test.ts b/test/js/sql/postgres-close-during-handshake.test.ts
new file mode 100644
index 00000000000..09c510799f1
--- /dev/null
+++ b/test/js/sql/postgres-close-during-handshake.test.ts
@@ -0,0 +1,151 @@
+// Regression test for https://github.com/oven-sh/bun/issues/30947
+//
+// Bun.SQL's pool would be permanently corrupted when all pool connections were
+// closed server-side (e.g. by a connection pooler's idle reaper or
+// `pg_terminate_backend`) while the event loop was blocked. The underlying
+// PostgresSQLConnection queues its on_connect callback as a microtask when the
+// server's ReadyForQuery arrives; if the socket is closed in the same I/O tick,
+// `#onClose` runs synchronously first and transitions the pooled connection to
+// `closed`. The pending `#onConnected` microtask then fires and, without the
+// fix in `src/js/internal/sql/postgres.ts`, unconditionally overwrites state
+// to `connected` and re-adds the dead connection (with `this.connection ===
+// null`) to `readyConnections`. Subsequent queries dispatch `null` to Rust's
+// PostgresSQLQuery.run, which throws "connection must be a
+// PostgresSQLConnection" — and the pool never recovers because it thinks the
+// ghost entry is still live.
+
+import { expect, test } from "bun:test";
+import { bunEnv, bunExe, tempDir } from "harness";
+
+// A fake postgres server that sends the full trust-mode handshake response
+// (AuthenticationOk + many ParameterStatus messages + BackendKeyData +
+// ReadyForQuery + an admin-shutdown ErrorResponse) and immediately closes the
+// socket. The ParameterStatus stack matches what a real server sends on
+// startup so uSockets' recv delivers the whole handshake plus the FIN in one
+// poll dispatch; that is what makes on_close land in the same I/O tick as
+// ReadyForQuery and fire `#onClose` before the queued `#onConnected` microtask
+// drains.
+const FIXTURE = /* js */ `
+const net = require("node:net");
+const { SQL } = require("bun");
+
+function pkt(type, body) {
+  const h = Buffer.alloc(5);
+  h.write(type, 0);
+  h.writeInt32BE(body.length + 4, 1);
+  return Buffer.concat([h, body]);
+}
+function int32(n) {
+  const b = Buffer.alloc(4);
+  b.writeInt32BE(n, 0);
+  return b;
+}
+function cstr(s) {
+  return Buffer.concat([Buffer.from(s), Buffer.from([0])]);
+}
+function paramStatus(k, v) {
+  return pkt("S", Buffer.concat([cstr(k), cstr(v)]));
+}
+
+const handshakeResponse = Buffer.concat([
+  pkt("R", int32(0)), // AuthenticationOk
+  paramStatus("application_name", ""),
+  paramStatus("client_encoding", "UTF8"),
+  paramStatus("server_encoding", "UTF8"),
+  paramStatus("server_version", "17.0"),
+  paramStatus("session_authorization", "test"),
+  paramStatus("standard_conforming_strings", "on"),
+  paramStatus("TimeZone", "UTC"),
+  paramStatus("integer_datetimes", "on"),
+  paramStatus("IntervalStyle", "postgres"),
+  paramStatus("is_superuser", "off"),
+  pkt("K", Buffer.concat([int32(12345), int32(67890)])), // BackendKeyData
+  pkt("Z", Buffer.from("I")), // ReadyForQuery
+]);
+const adminShutdown = pkt("E", Buffer.concat([
+  cstr("SERROR"),
+  cstr("C57P01"),
+  cstr("Mterminating connection due to administrator command"),
+  Buffer.from([0]),
+]));
+
+const server = net.createServer(socket => {
+  let handshook = false;
+  socket.setNoDelay(true);
+  socket.on("data", () => {
+    if (handshook) return;
+    handshook = true;
+    // Full handshake + admin-shutdown error + FIN — the same close pattern
+    // pg_terminate_backend produces.
+    socket.write(handshakeResponse);
+    socket.write(adminShutdown);
+    socket.end();
+  });
+  socket.on("error", () => {});
+});
+
+await new Promise(r => server.listen(0, "127.0.0.1", r));
+const port = server.address().port;
+
+const sql = new SQL({
+  url: \`postgres://u@127.0.0.1:\${port}/db\`,
+  max: 10,
+  connectionTimeout: 2,
+});
+
+// A broken pool throws "connection must be a PostgresSQLConnection" on every
+// subsequent query once a ghost entry has leaked into readyConnections. A
+// healthy pool just keeps seeing "Connection closed" (or the admin-shutdown
+// error) because our fake server kicks every connection. A handful of
+// iterations is enough to trigger the race reliably; bail out on the first
+// occurrence so we don't hang against a corrupted pool.
+let corrupted = false;
+let iterations = 0;
+for (let i = 0; i < 20; i++) {
+  iterations = i + 1;
+  try {
+    await sql\`SELECT 1\`;
+  } catch (err) {
+    if (/connection must be a PostgresSQLConnection/i.test(err && err.message)) {
+      corrupted = true;
+      break;
+    }
+  }
+}
+
+console.log(JSON.stringify({ corrupted, iterations }));
+// A corrupted pool can refuse to close cleanly (sql.close() spins trying to
+// flush ghost entries), so just exit immediately — the subprocess dying is
+// the tear-down signal for the fake server.
+process.exit(0);
+`;
+
+test("pool recovers after every connection is closed mid-handshake", async () => {
+  using dir = tempDir("pg-close-mid-handshake", {
+    "fixture.js": FIXTURE,
+  });
+
+  await using proc = Bun.spawn({
+    cmd: [bunExe(), "fixture.js"],
+    env: bunEnv,
+    cwd: String(dir),
+    stdout: "pipe",
+    stderr: "pipe",
+  });
+
+  const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
+
+  // Fixture prints a single JSON line on stdout. `corrupted: true` means the
+  // pool produced the internal "connection must be a PostgresSQLConnection"
+  // error at least once — always a bug, never expected fallout from the test
+  // scenario (the fake server just closes every connection).
+  // Fold stderr into the assertion so a fixture crash surfaces in the diff
+  // instead of leaving CI with an opaque `corrupted: undefined`.
+  const line = stdout.trim().split("\n").at(-1) ?? "";
+  const parsed = line ? JSON.parse(line) : {};
+  expect({ stderr, corrupted: parsed.corrupted, exitCode }).toEqual({
+    stderr: "",
+    corrupted: false,
+    exitCode: 0,
+  });
+});

From e6c829bf81e5f387b09492a5855cfb75d6da4168 Mon Sep 17 00:00:00 2001
From: robobun <117481402+robobun@users.noreply.github.com>
Date: Fri, 12 Jun 2026 19:06:08 +0000
Subject: [PATCH 2/2] test: point header comment at
 BasePooledConnection.handleConnected

---
 test/js/sql/postgres-close-during-handshake.test.ts | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/test/js/sql/postgres-close-during-handshake.test.ts b/test/js/sql/postgres-close-during-handshake.test.ts
index 09c510799f1..6c8b9fc404e 100644
--- a/test/js/sql/postgres-close-during-handshake.test.ts
+++ b/test/js/sql/postgres-close-during-handshake.test.ts
@@ -5,10 +5,11 @@
 // `pg_terminate_backend`) while the event loop was blocked. The underlying
 // PostgresSQLConnection queues its on_connect callback as a microtask when the
 // server's ReadyForQuery arrives; if the socket is closed in the same I/O tick,
-// `#onClose` runs synchronously first and transitions the pooled connection to
-// `closed`. The pending `#onConnected` microtask then fires and, without the
-// fix in `src/js/internal/sql/postgres.ts`, unconditionally overwrites state
-// to `connected` and re-adds the dead connection (with `this.connection ===
+// `handleClose` runs synchronously first and transitions the pooled connection
+// to `closed`. The pending `handleConnected` microtask then fires and, without
+// the guard in `BasePooledConnection.handleConnected`
+// (src/js/internal/sql/shared.ts), unconditionally overwrites state to
+// `connected` and re-adds the dead connection (with `this.connection ===
 // null`) to `readyConnections`. Subsequent queries dispatch `null` to Rust's
 // PostgresSQLQuery.run, which throws "connection must be a
 // PostgresSQLConnection" — and the pool never recovers because it thinks the
@@ -23,8 +24,8 @@ import { bunEnv, bunExe, tempDir } from "harness";
 // socket. The ParameterStatus stack matches what a real server sends on
 // startup so uSockets' recv delivers the whole handshake plus the FIN in one
 // poll dispatch; that is what makes on_close land in the same I/O tick as
-// ReadyForQuery and fire `#onClose` before the queued `#onConnected` microtask
-// drains.
+// ReadyForQuery and fire `handleClose` before the queued `handleConnected`
+// microtask drains.
 const FIXTURE = /* js */ `
 const net = require("node:net");
 const { SQL } = require("bun");