Repository navigation
node:quic: send a client's coded close before the server's first packet, ignore close options without code - #44846
Conversation
…acket lsquic's immediate_close() sent nothing for lsquic_conn_abort_error() on a client that had not heard from the server, never used the Initial packet number space, and turned code 0 into INTERNAL_ERROR. A close that the application asked for now goes out with its code in the highest space that has keys. lsquic's own aborts are not changed. Options without `code` follow Node's parser: close() is a plain NO_ERROR close and destroy() sends nothing. destroy() with a code always uses lsquic_conn_abort_error(), which does not wait for the streams. lsquic_conn_ack_now() no longer queues an ACK when no ack-eliciting application packet waits for one. With nothing received that ACK failed and reached the peer as an INTERNAL_ERROR close from a destroy() that must be silent.
|
Status Reproduced on a release build of main. The peer is a UDP socket that never answers. After PR: #44846 |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it patches lsquic's immediate_close() packet-space selection and changes what goes on the wire for every close()/destroy() option shape, a human look at the lsquic patch and the Node-parity claims would still be worthwhile.
What was reviewed:
- The
parse_close_optionsrefactor against the base:close()always reaches native with an object (quic.tsvalidateCloseOptions), so the explicitself_close = (false, 0, [])record in theNonearm keeps the base'sreport_closeoutcome;destroy(err, {})losing its NO_ERROR frame is the intentional Node-parity change stated in the description. - Helper wiring:
close_with_optionshas no remaining callers;apply_closeis still reached fromapply_graceful_closeandmaybe_finish_deferred_close; the twolsquic_conn_abort_errorsites inendpoint.rsare server-side, where the patch's new branches (version-negotiation early return, Handshake-keys fallback) are client-only and the non-zero CONNECTION_REFUSED code is unaffected by the code-0 change. ack_nowgating onifc_n_slack_akbl[PNS_APP]: an ACK-eliciting app packet recorded in rechist is exactly whatgenerate_ack_frame_for_pnsneeds, so the previously failing ACK build cannot be reached from the new path.- Candidates ruled out: lone Initial close for a session that never sent its ClientHello (gated by
IFC_FIRST_TICK, with a test covering the connect-inside-callback case); Initial-space close after Initial keys are discarded (thePNS_INITarm requires Handshake keys not ready, which precedes key discard).
Extended reasoning...
The change adds a new vendored lsquic patch (user-abort marker on cn_flags bit 5, Initial-space fallback and code-0 handling in immediate_close, lsquic_conn_ack_now returning whether an ACK was queued), updates the Rust FFI wrapper, and reworks node:quic session close()/destroy() option parsing so that options without code are a plain close or a silent destroy, plus roughly 500 lines of new tests that decrypt client Initial packets to assert the CONNECTION_CLOSE frames. It touches no auth, injection, or data-exposure surface, but it does alter protocol behavior on the wire for every close path and modifies vendored crypto-transport internals. The Node source lines cited for the parity claims could not be fetched from this environment, and the lsquic tick ordering around IFC_FIRST_TICK could only be checked from the patch context rather than the full vendored source, which is why a human look is recommended rather than an approval.
One part of this review's analysis that stopped early was run once more. It finished.
|
The review names two things that it could not check. Here is where to check them. Node parity. Both rules are in
The frame table in the Notes comes from runs of a Node v26.3.0 build with QUIC, not from a read of the source.
The test |
On main `cd test && bun check` had no type error after #44614. Merged with main, the tests of this branch had 8, all for the arguments of connect(). The edits are casts and `!`. The import line of node:quic is now the line that main has, so the two merge with no conflict. The transpiled output differs only in the line breaks of one object literal.
| fn apply_abort(&self, app: bool, code: u64, reason: &[u8]) { | ||
| let Some(c) = self.conn() else { return }; | ||
| let creason = core::ffi::CStr::from_bytes_until_nul(reason).unwrap_or(c"close"); | ||
| c.abort_error(app, code.min(u32::MAX as u64) as core::ffi::c_uint, creason); |
There was a problem hiding this comment.
🟣 pre-existing, not blocking: pre-existing: a peer receiving a node:quic close with a code above 0xFFFFFFFF gets code 0xFFFFFFFF on the wire, not the code the caller passed. apply_abort at session.rs:1966 clamps code.min(u32::MAX as u64) before lsquic_conn_abort_error, which takes unsigned. This PR now routes every destroy(undefined, { code }) and every coded close() through this helper, while self_close at session.rs:1906 keeps the full 64-bit code for the local report, so the two sides disagree. Fix: carry the full 62-bit QUIC error code to the CONNECTION_CLOSE frame (widen the lsquic_conn_abort_error/ci_abort_error code parameter in the vendored patch, or reject codes above the supported range at parse time as an error instead of silently clamping).
A small fix can ride a push you are already making; otherwise a short reply is enough.
Why this was flagged
A caller passes a close code that does not fit in 32 bits, for example a WebTransport-style application code, through session.close({ code: 0x52e4a40fa8dbn, type: "application" }) or session.destroy(undefined, { code: ... }). parse_close_options at src/runtime/node/quic/session.rs:1890 reads it as u64 and records the full value in self_close at session.rs:1906. apply_abort at session.rs:1966 then calls c.abort_error(app, code.min(u32::MAX as u64) as core::ffi::c_uint, creason), so the lsquic CONNECTION_CLOSE frame carries 0xFFFFFFFF. The peer's closed rejects with errorCode: 4294967295n while the local closed reports the original code. The base branch has the same clamp (the line is moved, not changed), so this is pre-existing; the PR does make destroy(undefined, { code }) for every code and the new Initial/Handshake-space closes go through it as well. No safeguard rejects the out-of-range code: validateCloseOptions in src/js/internal/quic/quic.ts:5195 only checks the type is bigint or number.
Verification: pre-existing. Trigger: a caller passes a close code above 0xFFFFFFFF. validateCloseOptions (src/js/internal/quic/quic.ts:5199-5203) only checks typeof code, no range. apply_abort (session.rs:1966) calls c.abort_error(app, code.min(u32::MAX as u64) as core::ffi::c_uint, creason), so the wire code is saturated to 0xFFFFFFFF. The identical clamp exists in the base at session.rs:1954.
There was a problem hiding this comment.
This is from before this PR, as the comment says. The description has it under "Left for later". It is not in this push. The full code needs a wider ci_abort_error in lsquic, which takes unsigned, or a second field for the code, and tests of its own. A throw for a code above 2^32-1 would reject a code that Node accepts.
close() records the close that report_close reports for every call, and destroy() records it when the options have a code. A plain close is no longer a case that the caller of the parser has to patch up. Each comment that this branch adds in src/ is now one line. The comment in destroy() about a parse failure is the one from before this branch. Test: on a session that has not sent its ClientHello, destroy() with a code must send nothing. The assertion ignored a connection that had sent a ClientHello, so it also passed when that state was not reached.
…ons cast The cast needed `type SessionOptions` on the import line of node:quic. Main changed that line in #44614, and the formatter removes a name that this branch alone does not use. The line is now the one of the base again, so it merges with main with no conflict.
There was a problem hiding this comment.
I re-reviewed the two new pushes and found no new bugs; because the fix lives in a hand-written patch to lsquic's immediate_close() whose base source is not in this checkout to diff against, a human look at patches/lsquic/user-abort-close.patch is still worthwhile.
What was reviewed:
- The
parse_close_optionssplit:close()always arrives with an object fromvalidateCloseOptions, so recordingself_closeon every call keeps the oldreport_closeoutput;destroy()withoutcodeno longer records one and takes the silent arm, which is the intended Node behavior. - The
destroy()silent arm now drives the engines only whenack_now()queued an ACK, and still callsabort_silent()on every path;apply_abortwithout a conn stays a no-op asapply_closedid. - The "has not sent its ClientHello" test now asserts the unfiltered
[], so it fails if the ClientHello went out before the destroy.
Extended reasoning...
The change adds a vendored lsquic patch marking application-requested aborts so a client can send a coded CONNECTION_CLOSE in an Initial packet before the server's first packet, changes lsquic_conn_ack_now to report whether it queued an ACK, reworks parse_close_options/record_self_close/apply_abort in src/runtime/node/quic/session.rs, and adds ~520 lines of tests including a hand-rolled QUIC Initial decoder. It touches no auth or credential surface, but it alters wire-level close behavior of the QUIC stack shared with HTTP/3. The two pushes since the prior review addressed the earlier test nit and moved close recording out of the parser; the Rust side checked out as behavior-preserving for close(). The lsquic C patch cannot be verified against the upstream source from this checkout, and the protocol-level change is non-trivial, which is why a human look is still worthwhile rather than approval.
One part of this review's analysis that stopped early was run once more. It finished.
Still open from earlier reviews (1):
- Unresolved: 1 minor or pre-existing.
|
Three commits since the first review.
Not changed: a close code above 2^32-1 is clamped, as before this PR. That thread is open, and the description lists it under "Left for later". Verified on a debug build of e37e601:
#44751, the base of this PR, now has a conflict with main in |
Merge #44751 first. This PR targets its branch.
Problem
session.close({ reason })rejects the peer'sclosedwith INTERNAL_ERROR "connection error".close({ code })sends no CONNECTION_CLOSE. Node sends it. A bun server keeps the session for 10 s.immediate_close()(lsquic_full_conn_ietf.c:4190) sends code 0 as INTERNAL_ERROR, and sends nothing before the first server packet. No user reported this.Fix
patches/lsquic/user-abort-close.patchmarks a close that comes fromlsquic_conn_abort_error, which only node:quic calls. A marked close keeps code 0 and can use an Initial packet.parse_close_optionsignorestypeandreasonwithoutcode, as Node does.lsquic_conn_ack_nowqueues an ACK only when one is owed, sodestroy()stays silent.test/js/node/quic/quic-endpoint.test.ts(47 new tests, 31 fail without the fix), the vendoredtest-quic-*.mjs, the HTTP/3 tests.Background
lsquic_conn_close, which sends only NO_ERROR: the code never reaches the peer.Downsides
destroy(err, {})sent NO_ERROR and now sends nothing, as Node does. The peer sees a stateless reset or its idle timer.session.closedgets an unhandled rejection from an early coded close..textgrows 320 bytes. Each abort runs 1 to 8 more instructions.Notes
Repro. The peer is a UDP socket that never answers. Run
bun probe.mjs plain|reason|code|app.plainreasoncodeapp--experimental-quicThe frames, before the server's first packet. The same peer decrypts every client Initial (RFC 9001 section 5.2). The table shows the CONNECTION_CLOSE as code and reason. Every frame is type 0x1c. Node is v26.3.0 built with
--experimental-quic(ngtcp2 1.22.1). "main" is the base of this PR, the head of #44751.close(),close({}),close({ code: 0n })close({ reason })close({ type: "application" })close({ code: 5n })close({ code: 5n, reason })close({ code: 5n, type: "application" }), also withreasonclose({ code: 0n, reason })close({ code: 0n, type: "application", reason })destroy()destroy(undefined, {})destroy(undefined, { reason }),destroy(undefined, { type: "application" })destroy(undefined, { code: 0n })destroy(undefined, { code: 5n })destroy(undefined, { code: 5n, reason })destroy(undefined, { code: 5n, type: "application" }), also withreasondestroy(undefined, { code: 0n, reason })destroy(undefined, { code: 0n, type: "application", reason })This PR equals Node in 22 of 22 cells. Main equals Node in 7 of 22. The reason equals Node's only up to 255 bytes and with no NUL byte (see "Left for later").
Node's rule for options without
code.MaybeSetCloseErrorreturns atcode_val->IsUndefined()before it readstypeorreason(session.cc#L926).Destroysends a CONNECTION_CLOSE only when a close error was set (session.cc#L992-L1000).validateCloseOptionsinquic.tsis a port of Node's and keepscodeundefined when the caller omits it.Why this is one change.
close({ type: "application" })before the handshake would then sendAPPLICATION_ERROR, anddestroy(err, { reason })would send a frame. Node sends NO_ERROR and nothing.destroy(err, {})takes the silent arm ofdestroy(). That arm callslsquic_conn_ack_nowfirst. With no application packet received, lsquic failed to build the ACK, raised an internal error, and sent CONNECTION_CLOSE INTERNAL_ERROR. So the silent arm was not silent between the server's first packet and its first 1-RTT packet. The last new test shows this (5 of 5 runs of its probe on main).destroy()drives the engine only to send that ACK. A drive with no ACK queued is a normal tick: it sent a ClientHello for a session that had not sent one, or retransmitted one that was due.destroy(err, { code })now always useslsquic_conn_abort_error.lsquic_conn_closefirst resets the streams and sends the frame only when they are gone. For{ code: 0n }on a server with an open client stream, the client got a stream reset and then the close after about 1,195 ms (3 of 3 runs on main). This PR sends the close in 3 to 5 ms and no reset, as Node does.Why this PR is on top of #44751. The two PRs change different stages of the same call. With the parser rule,
close({ type: "application" })is a plain close. On an HTTP/3 client with a pending stream, a plain close callslsquic_conn_going_awaybefore HTTP/3 starts, which is the crash that #44751 fixes. With a pending stream that has a body, main defers a coded close for 10 s, which #44751 also fixes. Two of the new tests cover these cells.What the lsquic patch changes, by caller.
QuicSession::apply_close(src/runtime/node/quic/session.rs) sends a close with a non-zero code, a reason or the application type throughlsquic_conn_abort_error. Every other close useslsquic_conn_close, which can only send NO_ERROR.lsquic_conn_abort_errorhas 5 callers, all in node:quic (session.rs3,endpoint.rs2). Bun.serve HTTP/3 and the fetch HTTP/3 client uselsquic_conn_closeandlsquic_conn_abort, which do not set the marker.ci_abort_errordirectly and not through that wrapper. Those aborts stay unmarked.APPLICATION_ERROR(0x0c) with no reason (RFC 9000 section 10.2.3). Upstream did that for Handshake packets only.litespeedtech/lsquic#693proposes the Initial fallback for every abort. It does not change the early return.Other states checked by hand on this PR (a peer that forges the server's packets, with the frames read from the decrypted Initials):
close({ code: 5n, reason })sends an Initial with the Retry token and code 5 "bye". Main sends nothing.close({ code: 5n })sends an Initial with code 5, an application close sends code 12. Main sends nothing. One new test covers this state with a server packet that the client cannot read.destroy()after the JS thread was blocked for 400 ms (the ClientHello retransmission is due): 1 datagram, as on main.destroy()after the handshake with no ACK owed and one open stream: the client sends 1 datagram, the reset of the stream. Main sends 2, the reset and an ACK that nothing needs. This state has no test (see "Not done from the self-review").BUN_DEBUG_lsquic=1on the debug build,generated CONNECTION_CLOSE frameappears 0 times fordestroy(),destroy(err), a handshake timeout at a silent peer, and a version negotiation failure.Measurements (release builds of the base 6546653 and of this PR, same toolchain and build directory):
immediate_close771 B to 838 B,parse_close_options1,156 B to 822 B plus a 157 B closure,QuicSessionPrototype__destroy624 B to 676 B,QuicSessionPrototype__gracefulClose455 B to 600 B. Total.text+320 B (llvm-nm -S,size). The stripped binary has the same file size, 89,142,856 B.struct ietf_full_conn(release): 10,232 B to 10,232 B (thecallocargument in both constructors). The marker is spare bit 5 ofcn_flags.ietf_full_conn_ci_tick: 2,396 B to 2,396 B, 555 instructions, identical when addresses are stripped.immediate_close: 187 to 206 instructions. The unmarked path runs 1 to 8 more instructions, once per aborting connection.close(), wholegracefulClosecall: property gets 3 to 1, string conversions 1 to 0,mi_malloc5 to 2,mi_free4 to 1 (gdb hit counters). A coded close is unchanged at 3, 1, 5, 4.destroy()with nothing to acknowledge: engine passes 2 to 1 per call.close({ code: 5n }): fulfilled after 10,015 to 10,021 ms on main, rejected with errorCode 5 after 11 to 19 ms on this PR (a plain close takes 12 to 89 ms on both). After the handshake,close({ reason }): rejected with errorCode 1 "connection error" on main, fulfilled on this PR. After the handshake,close({ code: 5n }): errorCode 5 on both.test-quic-*.mjs: 233 of 235 pass on both release builds. The 2 that fail importnode:stream/iter, which does not exist. On debug builds the same 8 fail with and without this PR.serve-http3,fetch-http3-client,fetch-http3-adversarial,fetch-http3-cold-post,fetch-http3-syscall-fault: 167 pass and 0 fail on both release builds.quic-endpoint.test.tswith the LeakSanitizer settings of the ASAN lane: 17 of 18 runs clean. The file without the new tests: 11 of 12. The abort is an lsquic leak that Update lsquic to 4.9.4 and free unsent stateless resets on destroy #41337 fixes. The new tests that use a server are not concurrent because of that leak: 5 of 5 runs abort when they are.Cells that change on purpose.
destroy(err, {})sent NO_ERROR and now sends nothing, likedestroy(err). Example on a server with an open client stream: the client'sclosedfulfilled after 4 ms on main. On this PR it rejects with transport error 1 after about 1.4 s, from a stateless reset.destroy()does the same on main.close({ reason })andclose({ type: "application" })reached the peer as INTERNAL_ERROR after the handshake and sent nothing before it. They are now a plain close. On a server with an open client stream a plain close reaches the client after about 1.4 s, so these two calls now take that long there.destroy()sends no ACK when none is owed.session.closedhas no handler: a client close with a code before the handshake now rejects that promise, which is an unhandled rejection. On main the promise fulfils after 10 s. A close with a code after the handshake rejects it on main too.Where this PR does not follow Node v26.3.0.
connect()inside the callback of another session on the same endpoint returns before the ClientHello leaves.destroy(undefined, { code: 5n })there sends nothing, as on main. Node sends the ClientHello insideconnect()and then the close. A close with no ClientHello before it is a close for a connection that the server never saw, so the patch stays silent there.close({ code: 5n })in the same state sends the ClientHello and then the close, as Node does.destroy(undefined, { code: 5n, reason })before the handshake ends still reaches the client as a stateless reset (transport error 1, no reason). Node delivers code 5 "bye". The server session has no connection yet, sodestroy()does not read the options. A serverclose({ code })in the session callback works.close()that the session callback makes synchronously: bun keeps it until the connection exists, Node drops it.Since the first push.
bun checkafter a merge with main (Fix every type error intest/#44614). The branch has no conflict with main in its own files.parse_close_optionsonly parses.close()anddestroy()record the close thatreport_closereports. Each comment that the branch adds insrc/is one line.destroy()with a code sends nothing at all.Not done from the self-review.
this.destroy()as soon as it has its 1-RTT keys and expects a CONNECTION_CLOSE. That frame is the INTERNAL_ERROR from the failed ACK. With this PR the call sends nothing, so those tests wait for a frame that does not come.this.destroy(undefined, { code: 1n })sends a close with and without this PR. The tests are not in this branch. The one of the two PRs that merges second must make that change.destroy()after the handshake sends no ACK when none is owed. To owe none, the client needs a peer that stops answering. The retransmission timer of the client then sends datagrams, and a count cannot tell them from the ACK without the 1-RTT keys. A probe that counts the datagrams of onedestroy()call: main sends the ACK in 40 of 40 runs and this PR in 0 of 40 (release builds). On the debug build under load, 2 of 24 runs of this PR count a retransmission.Left for later.
litespeedtech/lsquic#693).validateAddressis not read by the native code). Any Initial that no handshake follows holds a server session for 10 s. This PR removes one cause of that. The first server test depends on it.close()on a server with an open client stream reaches the client after about 1.4 s.ci_abort_errortakesunsignedand a 256-byte buffer.endpoint.destroy(err)gives the close options only to sessions whose handshake is complete, as Node does. A session that is still in its handshake stays silent there, and a bun server holds its side for 10 s.[policy-decision:dep] gate passed · iteration 0 · 5 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file