sql: make forced close() resolve while a pool connection is mid-handshake - #32097
Conversation
…hake
A forced pool close (close({ timeout: "0" })) never resolved when a
connection had been accepted at the TCP level but the database handshake
had not completed yet. uws skips the on_close dispatch for sockets whose
connect never finished (POLL_TYPE_SEMI_SOCKET), so the socket-close ->
on_close -> fail chain that normally fires the JS onclose callback never
ran, and the onFinish promise in the pool's #close() stayed pending
forever.
- postgres: disconnect() only tore down Status::Connected sockets, so a
close during Connecting/SentStartupMessage left the in-flight socket
alive (it even sent the startup message after close). close() now fails
the connection directly for those states, which consumes and fires the
JS onclose callback, rejects pending queries, closes whatever socket is
live, and releases the poll ref that no socket event will release.
- mysql: do_close closed the socket but got no event back for in-flight
connects and left the status Connecting, keeping the event loop
referenced forever. It now fails the connection directly for the
pre-connected states.
- mysql (js): the pool never stored the native handle until onConnected,
so close() on a pending connection was a no-op; store it at creation
like the postgres adapter does.
- both (js): if the pool is force-closed before the native handle is
assigned (createConnection resolves one microtask later), close the
handle as soon as it materializes so onFinish settles.
The same uws quirk was already worked around in the valkey client
(src/runtime/valkey_jsc/valkey.rs close()).
Fixes #32095
|
Updated 12:56 AM PT - Jun 11th, 2026
❌ @robobun, your commit 8fc74b1 has 5 failures in
🧪 To try this PR locally: bunx bun-pr 32097That installs a local version of the PR into your bun-32097 --bun |
WalkthroughThis PR fixes a bug where ChangesSQL pool forced close during handshake
Possibly related issues
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/sql_jsc/mysql/JSMySQLConnection.rs`:
- Around line 709-711: The match arm that groups S::Connecting | S::Handshaking
| S::Authenticating | S::AuthenticationAwaitingPk should be split so only
S::Connecting calls fail() directly; for the post-on_open states (Handshaking,
Authenticating, AuthenticationAwaitingPk) explicitly drop the socket ref taken
by on_open before invoking fail() — e.g., invoke the same DerefOnDrop release
logic used in on_close (or call the code path that releases this.ref_()) so the
refcount goes down prior to clean_queue_and_close/ fail_with_js_value; update
the match to call fail() after releasing the ref for those three states to avoid
the semi-socket refcount leak.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 60c2d765-7ea0-4e0e-90c3-8e8c6f32d694
📒 Files selected for processing (5)
src/js/internal/sql/mysql.tssrc/js/internal/sql/postgres.tssrc/sql_jsc/mysql/JSMySQLConnection.rssrc/sql_jsc/postgres/PostgresSQLConnection.rstest/js/sql/sql-close-pending-connection.test.ts
The re-integration commit hosts the #32041 try/finally and the #32097 forced-close settlement in one BasePooledConnection close handler, so pin their interaction per driver: onclose throws while the pool is force-closed mid-handshake, and close() must still resolve and the pending query must still reject.
…n in shared.ts (#32145) ## What this does Final slice from #31994 (closed as too big), on top of #32128, #32135, #32141. Consolidates the JS adapters' duplicated pool/connection/query plumbing from `src/js/internal/sql/{postgres,mysql,sqlite}.ts` into `src/js/internal/sql/shared.ts` as a `BasePooledConnection` class hierarchy. Net −995 lines across 4 files. Two commits: - **Cherry-pick from #31994** (1aa3dbb) — checks out `src/js/internal/sql/` from `origin/claude/split/sql` as-is. This intentionally clobbers two pool-lifecycle fixes that landed on `main` since #31994's merge-base: #32041 (throwing onconnect/onclose corrupting the pool) and #32097 (forced `close()` resolving mid-handshake). - **Re-integrate #32041 + #32097 into the new structure** (6e6fd4f) — re-hosts both fixes in `BasePooledConnection`: - #32041: try/finally around the user `onconnect`/`onclose` calls in `handleConnected` and `#finishClose` so a throwing callback never skips pool bookkeeping; the `createPooledConnectionHandle` catch always defers via `process.nextTick` (drops the per-driver `deferSyncCloseError` flag — both drivers now match). - #32097: `startConnection()` is `abstract Promise<void>` and both subclasses await + assign `this.connection` at creation; `#beginConnecting` awaits it and closes the handle if `onFinish` was set (pool force-closed) in the microtask window before it materialized. ## Behavioral equivalence The original `BasePooledConnection` extraction in #31994 carried a per-file behavioral-equivalence audit against its merge-base (placeholders, escaping, helper commands, error message text, pool/transaction semantics all preserved) and was green on full CI build 61383. This PR re-applies that diff and adds the two missing fixes; the load-bearing verification is below. ## Verification - `bun bd`: builds clean. - The three lifecycle test files (`sql-onconnect-onclose-throw.test.ts`, `sql-close-pending-connection.test.ts`, `sql-connect-error-reporting.test.ts`): 26 pass / 0 fail. - **New tests in this PR**: `sql-onconnect-onclose-throw.test.ts` gains a forced-close variant per driver (a throwing `onclose` while the pool is force-closed mid-handshake, against a fake `net` server). The two re-hosted fixes meet in `BasePooledConnection`'s close handler, and that interaction had no coverage. Note these pass against `origin/main`'s `src/js/internal/sql` too (verified): both underlying fixes already live on main per-driver, so versus main this PR is equivalence-preserving by design and no test can fail on one side only. The HEAD~1 stash test below is the proof that the re-integration commit is load-bearing. - **Load-bearing stash test**: built and ran the same three files at HEAD~1 (the cherry-pick before re-integration) — 6 fail (`mysql: forced close() resolves when called before the native handle is stored` timeout; `postgres: pool calls from onclose are safe when connecting fails synchronously` → `reentry threw: TypeError`; both drivers' `throwing onclose still rejects pending queries on connect refused` timeouts). With the re-integration: 26 pass. The diff is required for both fixes. - Full `test/js/sql/` against live `postgres_plain`/`mysql_plain`/`*_tls` containers: 1526 pass / 7 fail / 6 errors locally. Every failure reproduces on `origin/main` in the same environment — see Notes below. ## Notes on local-only test failures (none introduced by this PR) - `sql-mysql-query-string-leak.test.ts` fails locally on a debug+ASAN build for both `origin/main` (314.6 MiB) and this branch (317.2 MiB), within noise of each other; both exceed the 256 MiB ASAN threshold. A `heapStats()` probe of the same workload shows `MySQLQuery 2→1` after GC — wrappers are finalized correctly. The test passes on `main`'s CI (release build); the threshold is tuned for release RSS overhead, not local debug. - `sql.test.ts > query string memory leak test` (postgres): same category. - `sqlite-sql.test.ts > Query Normalization Fuzzing Tests > handles exotic but valid SQL patterns`: 5.5s timeout on debug builds — pre-existing per #31994's original verification notes. - `describeWithContainer` cold-start races: `docker compose up -d --wait` returns non-zero on an already-healthy container (`test/docker/index.ts:157`), causing beforeEach hook timeouts in `sql-mysql.test.ts` (TLS), `sql-mysql-bind-oob`, `sql-mysql.helpers`, `sql-mysql.auth`. Pre-existing harness flake; clears on warm rerun. The Rust-side changes from #32097 (`src/sql_jsc/`) were already on `main` before this PR — only the JS hooks needed re-integrating.
What does this PR do?
Fixes #32095. A forced pool close (
sql.close({ timeout: "0" })) never resolved when a pool connection had been accepted at the TCP level but the database handshake had not completed yet (a database that is still starting up). The pending queries were rejected withERR_POSTGRES_CONNECTION_CLOSED/ERR_MYSQL_CONNECTION_CLOSED, but the promise returned byclose()stayed pending forever. With a nonzeroconnectionTimeoutthe close blocked until the connect timer fired (up to 30s by default); withconnectionTimeout: 0it hung forever.Root cause
The pool's
#close()resolves its per-connection promise from theonFinishhook, which only fires from the JSonClose/onConnectedcallbacks. For a mid-handshake connection those callbacks never came:on_closewhen an application closes a socket whose TCP connect has not resolved yet (POLL_TYPE_SEMI_SOCKET, seeus_internal_socket_close_rawinpackages/bun-usockets/src/socket.c), so closing an in-flight connect produces no event at all.disconnect()gated onStatus::Connected, so a close duringConnecting/SentStartupMessagedid not touch the socket. The connect kept going and even sent the startup message afterclose()returned.do_closeclosed the socket unconditionally but got no event back for in-flight connects and never changed the status, sois_active()stayed true and kept the event loop referenced forever.#onConnected, soconnection.connection?.close()was a no-op for every pending connection.The fix
src/sql_jsc/postgres/PostgresSQLConnection.rs:close()now fails the connection directly when the status isConnecting/SentStartupMessage(outside VM shutdown).fail()consumes and fires the JS onclose callback, rejects pending queries, and closes whatever socket is live; since no socket event will do it, the poll ref is released explicitly. Shutdown paths and connected/terminal states keep going throughdisconnect()unchanged.src/sql_jsc/mysql/JSMySQLConnection.rs:do_closedoes the same forConnecting/Handshaking/Authenticating/AuthenticationAwaitingPk; connected/terminal states keep the existingclean_queue_and_closepath.src/js/internal/sql/mysql.ts:#startConnectionstores the native handle at creation (like the postgres adapter already did) so a forced close can reach it.src/js/internal/sql/postgres.ts+mysql.ts: if the pool is force-closed in the microtask window before the native handle is assigned, close the handle as soon as it materializes soonFinishsettles.fail()is idempotent (early-returns once the status isFailed), so the established-socket case, where closing does dispatchon_closesynchronously, does not double-fire. The valkey client already works around the same uws behavior (src/runtime/valkey_jsc/valkey.rsclose()), so this brings the SQL drivers in line; no other NsHandler consumer is affected.How did you verify your code works?
test/js/sql/sql-close-pending-connection.test.ts(no docker needed): for each driver, a TCP server accepts and never answers, and the test assertsclose({ timeout: "0" })resolves, both while a connection is mid-handshake and when close is called before the native handle is stored. All four tests hang (5s timeout) on the unfixed build and pass with the fix.Also verified:
closedand the process exits on its own (no lingering poll ref), for both driversconnectionTimeout: 2, forced close resolves in ~40ms instead of ~2stest/js/sql/sql-connect-error-reporting.test.tsandtest/js/sql/sql-onconnect-onclose-throw.test.tsstill pass