sql: close the socket at once when postgres or mysql fail a TLS connection whose peer stopped responding - #39015
sql: close the socket at once when postgres or mysql fail a TLS connection whose peer stopped responding#39015robobun wants to merge 1 commit into
Conversation
|
Warning Review limit reached
Next review available in: 1 minute Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (6)
Comment |
|
Status: reproduced and fixed; waiting on CI for abb16f0. Reproduced on main with a debug build using the fixture in this PR (test/js/sql/sql-tls-peer-stopped-responding.fixture.ts): postgres and mysql each report the failure against a TLS mock that stops responding, but none of the four mocks ever sees its connection close and the fixture never exits. With the fix all four connections close at once and the fixture exits on its own. CI: build 97996 (first head) ran 177 test jobs with no failures; it was marked failed only because the two macOS 14 aarch64 test lanes expired without an agent. The build for abb16f0 (comment trim, no code change) is the re-run. Fix: #39015 (this PR). Related open PRs touching the same functions for other reasons: #32573, #32861. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. The native change is small and mirrors the valkey fix in #37993, but it threads a close-code through the failure path of two SQL drivers' socket-lifecycle / ref-counting code, and the description flags overlaps with #32573 / #32861 that need coordination — a human look would still be worthwhile.
What was reviewed
- All callers of
ref_and_close/clean_queue_and_close/MySQLConnection::closeare updated;CloseKindis a type alias forCloseCode, so no signature mismatch. fail_with_js_value→Failure, user-facingdisconnect()/do_close→Normalin both drivers, matching the stated intent.- The fixture awaits real conditions (mock stopped responding, mock saw close, process self-exit) rather than sleeps, and the test asserts
signalCode: nullso a spawn-timeout kill fails the assertion.
Extended reasoning...
Overview
The PR threads a uws::CloseCode parameter through PostgresSQLConnection::ref_and_close and MySQLConnection::clean_queue_and_close/close, so that the failure path (fail_with_js_value) closes the socket with CloseCode::Failure while the user-facing close() keeps CloseCode::Normal. On a TLS socket, Normal defers the fd close (and the on_close dispatch that releases the poll ref / socket ref) until the peer answers close_notify; Failure closes immediately. The rest of the diff is test infrastructure: a startTlsServerSide helper in wire-frames.ts that runs a TLS engine over a hand-fed Duplex so the mock can stop answering entirely, a fixture that runs four driver×trigger scenarios concurrently, and a spawn test that asserts each client reports the failure, each mock sees its TCP connection close, and the fixture exits by itself.
Security risks
None identified. The change only affects how the client tears down a socket it has already decided to fail; no new inputs are parsed and no security checks are relaxed. The mock TLS server is test-only.
Level of scrutiny
Medium-high. The native diff is ~20 lines and mechanically mirrors the already-landed valkey change in #37993, but it sits in the connection-lifecycle / intrusive-refcount path of two drivers — the category REVIEW.md calls out as most-blocked. The observable wire-level change (RST instead of FIN on the failure path) is argued convincingly in the description and doc comments, but a maintainer should confirm they agree it is acceptable. The description also explicitly notes overlaps with two open PRs (#32573, #32861) touching the same functions; whichever lands second needs a rebase, which is a coordination decision a human should make.
Other factors
The test is well-constructed against the repo's testing rules: it awaits observable conditions (mockStoppedResponding, mockSawClose, process self-exit) rather than sleeping, sorts stdout lines so scenario interleaving does not flake, asserts signalCode: null so a timeout kill fails, and passes the harness TLS cert via env to avoid importing harness in the debug-build subprocess. I confirmed CloseKind is pub type CloseKind = CloseCode (src/uws/lib.rs:1371), so mixing the two names across call sites is fine, and grepped for any other callers of the changed functions — there are none. Given the lifecycle-sensitive area and the noted PR overlaps, I'm deferring rather than approving.
|
On the two points the review leaves to a human: the RST on TCP is intentional and is the same trade #37993 made for the valkey client (by the time this close runs, onclose has fired and every query on the connection has been rejected, so nothing is lost with the unsent data). On sequencing, this PR does not depend on #32573 or #32861; the overlap is limited to the |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| /// `fail_with_js_value` passes `CloseCode::Failure`, the one code usockets | ||
| /// never defers (see `CloseCode`): on a TLS socket `Normal` keeps the socket | ||
| /// open until the peer answers our close_notify, which a peer that stopped | ||
| /// responding never does, so `on_close` (and the ref it releases) would | ||
| /// never run. The cost is an RST instead of a FIN on TCP, which does not | ||
| /// matter once every request on the connection is being rejected. The JS | ||
| /// `close()` passes `Normal`. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // sockets the `Normal` close issued by `disconnect()` defers the | ||
| // actual close until the peer's close_notify arrives, so the struct | ||
| // must stay alive until then. The socket's onClose re-enters here | ||
| // (via failWithJSValue's defer) with isClosed() == true, at which | ||
| // point GC can proceed. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// `fail_with_js_value` passes `CloseCode::Failure`, the one code usockets | ||
| /// never defers (see `CloseCode`): on a TLS socket `Normal` keeps the | ||
| /// socket, and with it the poll ref taken below, until the peer answers our | ||
| /// close_notify, which a peer that stopped responding never does. The cost | ||
| /// is an RST instead of a FIN on TCP, which does not matter once every | ||
| /// request on the connection is being rejected. `disconnect()` passes | ||
| /// `Normal`. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
…fail a connection When the postgres or mysql client fails a connection itself (connection, idle or lifetime timeout, protocol error, forced close while connecting) it closed the socket with CloseCode::Normal. On a TLS socket past its handshake that sends close_notify and keeps the socket, and with it the poll ref (postgres) or the socket's ref on the connection (mysql), until the peer answers, which a peer that has stopped responding never does: the failure was reported but the socket stayed open and the process did not exit. Failure is the one code usockets never defers; the user-facing close() keeps Normal. The fixture runs both drivers against mocks that complete the TLS handshake and then stop responding, and has to exit by itself.
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it changes the close semantics on the failure path (synchronous on_close dispatch under TLS, RST instead of FIN over plain TCP) across two production drivers, and the comment-cop linter has three unaddressed inline notes on the new doc comments, a human look would still be worthwhile.
What was reviewed:
- Confirmed
CloseKindis a type alias forCloseCode(src/uws/lib.rs:1371), so the parameter threading type-checks againstAnySocket::close. - Checked re-entrancy:
Failurenow fireson_closesynchronously insideref_and_close/clean_queue_and_close, which re-entersfail_with_js_value; both drivers already guard onstatus == Failedand this path is already exercised over plain TCP. - Verified all callers of
ref_and_close/clean_queue_and_close/MySQLConnection::closeare updated in the diff (2 each), anddisconnect()/do_closestill passNormal. - Test drains stdout/stderr/exited concurrently, asserts
signalCode: null, and sorts stdout so the four concurrent scenarios don't order-flake.
Extended reasoning...
Overview
This PR threads a uws::CloseCode parameter through PostgresSQLConnection::ref_and_close and MySQLConnection::clean_queue_and_close/close, so that fail_with_js_value can pass CloseCode::Failure while the user-facing disconnect()/do_close keeps CloseCode::Normal. It adds a spawned-fixture test (sql-tls-peer-stopped-responding.fixture.ts) plus a startTlsServerSide helper in wire-frames.ts that runs a TLS engine over a hand-fed Duplex so the mock can stop answering close_notify. Native diff is ~15 lines across three files; the bulk is test infrastructure.
Security risks
None identified. The change only affects how a socket the client has already decided to fail is torn down. startTlsServerSide is test-only and swallows TLS errors on the mock side, which is intentional (the client sends an RST).
Level of scrutiny
Medium-high. The native change is mechanically small, but it alters observable behavior on every failure path in both SQL drivers: over TLS, on_close now dispatches synchronously inside fail_with_js_value (previously deferred), and over plain TCP the failed connection now ends with an RST (SO_LINGER{1,0}) instead of a FIN. The PR argues both are fine — the TCP re-entrancy already existed, and everything JS-visible has already been rejected by the time the close runs — and cites #37993 (valkey) as precedent. That reasoning looks correct to me, but it is a design trade-off touching production networking code that a maintainer should confirm.
Other factors
- The comment-cop bot has left three inline comments on the new paragraph-length doc comments (MySQLConnection.rs:305, PostgresSQLConnection.rs:617/1520) that have not been addressed or dismissed; per the repo's own guidance those may need shortening.
- The PR description flags overlap with #32573 and #32861 on
ref_and_close; whichever lands second needs a rebase, so a human should decide sequencing. - The new test is on the heavy side (30 s timeout, spawns a debug-build subprocess doing four TLS handshakes) but is well-justified: without the fix the fixture never exits, and the assertion on
signalCode: nullplus the sorted stdout lines makes the failure mode diagnosable rather than a bare timeout.
6ce8774 to
abb16f0
Compare
|
abb16f0 trims the three comments the linter flagged: the two doc comments are now one line each pointing at |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. The native change is small (thread a CloseCode through ref_and_close/clean_queue_and_close and pass Failure from fail_with_js_value), it mirrors the already-landed valkey change in #37993, and the fault-injection test is thorough. Because it changes teardown semantics in native connection-lifecycle code that two other in-flight PRs (#32573, #32861) are also hardening, a maintainer look would still be worthwhile.
What was reviewed:
- Verified every caller of
ref_and_close/clean_queue_and_close/MySQLConnection::closeis updated;CloseKindis a type alias ofCloseCode, so the parameter-type change is sound. - Checked that the synchronous
on_closedispatch this enables over TLS is not a new re-entrancy shape — plain-TCPclose()already dispatched it synchronously, so both drivers'fail()paths already had to tolerate it. - The three comment-cop flags are on one-line doc comments, not paragraph-long justifications — they read as false positives.
Extended reasoning...
Overview
The PR threads a uws::CloseCode parameter through three functions — PostgresSQLConnection::ref_and_close, MySQLConnection::close, and MySQLConnection::clean_queue_and_close — and has each driver's fail_with_js_value pass CloseCode::Failure instead of the previously hard-coded Normal. The user-facing close path (disconnect / the Connected arm of do_close) keeps Normal. A new spawned-fixture test in test/js/sql/sql-close-pending-connection.test.ts runs both drivers × two failure triggers against a TLS mock (startTlsServerSide in wire-frames.ts) that stops responding after taking the startup message, and asserts each mock sees the connection close and the fixture exits on its own.
Security risks
None identified. Failure on TCP sets SO_LINGER{1,0} so the failed connection ends with an RST rather than a FIN; the author addresses this (every JS-visible effect has already fired, and it matches the valkey precedent in #37993). No new user-controlled input reaches native code.
Level of scrutiny
Medium-high. The diff itself is mechanical, but it lives in native connection-lifecycle code with intrusive refcounts and poll refs, and the effect is that on_close now dispatches synchronously inside fail() over TLS. That is not a new shape (plain-TCP close() already did this, so the re-entrancy was already handled), but the PR description itself notes two open PRs touching ref_and_close/on_close for other lifecycle-safety reasons, which suggests a maintainer familiar with those invariants should sign off.
Other factors
- All call sites of the reshaped functions are updated (grep confirms two callers each;
on_end'ssocket.close(CloseKind::Normal)is a differentcloseonNewSocketHandler). - The test drains stdout/stderr/exited concurrently, asserts
signalCode: null(hang guard), sorts stdout lines before comparing, and passes the harness cert via env to avoid importingharnessin the debug-build subprocess. The 20 s spawn / 30 s test timeouts are commented as covering four debug-build TLS handshakes plus the without-fix hang. - The comment-cop bot left three inline flags on the added doc comments; each added comment is a single line ("
fail_with_js_valuepassesFailure, the one code a TLS socket never defers (seeCloseCode)."), so these look like false positives rather than something the author needs to address. - No CODEOWNERS entry covers
src/sql_jsc/.
|
Updated 12:13 PM PT - Aug 15th, 2026
🔄 @robobun, the build for your commit |
2 participants
Problem
close()while still connecting) over TLS against a peer that has stopped responding, the failure is reported but the socket stays open for good: on postgres it holds a poll ref, so the process never exits; on mysql it holds the ref the socket has on the native connection, so the connection (and the fd) is never released. Noted by @alii while reviewing valkey: close the socket on every fail() and mark the client disconnected before onclose runs #37993, where the valkey client had the same bug.fail()usedCloseCode::Normal.PostgresSQLConnection::fail_with_js_value->ref_and_close(src/sql_jsc/postgres/PostgresSQLConnection.rs) andJSMySQLConnection::fail_with_js_value->MySQLConnection::clean_queue_and_close->close(src/sql_jsc/mysql/MySQLConnection.rs). For a TLS socket past its handshake,Normalsends close_notify and defers the fd close, and with it the on_close dispatch, until the peer answers (us_internal_ssl_close, packages/bun-usockets/src/crypto/openssl.c). A peer that stopped responding never answers, so the poll refref_and_closetakes (postgres) and the ref the socket holds on the connection (mysql) are never released. Over plain TCP the same close dispatches on_close synchronously, which is why only TLS is affected.Fix
ref_and_closeandclean_queue_and_closetake the close code.fail_with_js_valuepassesCloseCode::Failurein both drivers; the user-facingclose()(disconnect()on postgres,do_closeon mysql) keepsNormal.Failureis the one code usockets never defers (Normalwaits for the peer's close_notify;FastShutdownis still held back while undelivered ciphertext is pending, i.e. exactly when the peer stopped reading), so on_close now runs inside the close call, the way it already does over TCP. Every JS-visible effect of the failure (onclose, rejected queries) happened before the close either way; the only visible difference is that a failed connection now ends with an RST instead of a FIN on TCP, which does not matter once everything on the connection has been rejected. Same shape and reasoning as the valkey change in valkey: close the socket on every fail() and mark the client disconnected before onclose runs #37993, which also updates theCloseCodedocs this relies on.fail()a timeout reaches.ref_and_close/on_closefor different reasons (re-entrancy guard, detaching the stored socket); whichever lands second needs a trivial rebase. The user-facingclose()of an established TLS connection whose peer stopped responding still waits for the peer (the pool'sclose()promise never resolves); that is a separate follow-up, as is the postgres poll ref leaking when a connection timeout fires while the TCP connect itself is still pending (no TLS involved).Background
CloseCodein src/uws_sys/us_socket_t.rs):Normal(0) is a graceful close: TCP FIN; on TLS, send close_notify and keep the socket until the peer's close_notify or FIN arrives, then close and dispatch on_close.Failure(1) closes immediately: on TLS a fast shutdown without waiting, on TCPSO_LINGER{1,0}so the close is an RST.FastShutdown(2) closes without waiting for the peer but is still deferred while the socket owns ciphertext the kernel has not accepted yet.KeepAlivea native object holds to keep the event loop (and so the process) alive while it expects further socket events.ref_and_closetakes one before closing and relies on on_close to release it.pause()does not stop that, so the fixture's mocks (startTlsServerSidein test/js/sql/wire-frames.ts) run the TLS engine over a Duplex they feed by hand and simply stop feeding it once they have the startup message; the TCP connection stays open, nothing is ever answered, which is what a hung server looks like to the client.