Repository navigation
Conversation
lsquic read "error code != 0" as "an error is recorded". A close with code 0 that came in through lsquic_conn_abort_error (an application close, or a transport close with a reason) lost its code and type in immediate_close() and went out as a transport CONNECTION_CLOSE with INTERNAL_ERROR. Record the error with a flag of its own in struct conn_err. The same line in immediate_close() is also changed by the lsquic patch of #44846, which has to rebase onto this.
… the test timeout
When the local lsquic connection closed with an error, the session read only lsquic_conn_status(). That enum has no error code, and it reports every error that comes with a failed handshake as a version negotiation failure. So the side that found the error reported transport error 1 for each one, and reported an error that also failed the handshake as a clean close: `closed` resolved and `onerror` did not run. A new lsquic patch adds lsquic_conn_close_info(). It returns the close cause and the error from conn_close_error(), the selection that immediate_close() builds CONNECTION_CLOSE from. on_conn_closed reads it in place of lsquic_conn_status(). lsquic's record is now the only local record of a close. The session's own copy, self_close, is gone: with the `recorded` flag of the previous commit lsquic keeps a close with code 0 too. So close() with no code can no longer hide an error that ends the connection afterwards.
|
Status
|
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 1 minute. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Updated 4:05 AM PT - Oct 22nd, 2026
❌ @robobun, your commit 112f3bc has 1 failures in
🧪 To try this PR locally: bunx bun-pr 46487That installs a local version of the PR into your bun-46487 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline findings, I also checked the FFI boundary for the new close-info path: Conn::close_info() copies lsquic's reason into an owned Vec inside on_conn_closed before the conn is freed, RawCloseInfo matches struct lsquic_close_info field-for-field (c_int, c_int, u64, pointer) with a size check in debug_assert_layout, and map_close_info is an exhaustive match over CloseCause with no wildcard arm, so a future variant cannot silently fall through.
Extended reasoning...
The change adds two lsquic vendor patches (a recorded flag on struct conn_err and a new lsquic_conn_close_info() API), a Rust FFI wrapper in src/lsquic_sys/lib.rs, and rewires report_close in src/runtime/node/quic/session.rs to use the captured close info instead of lsquic_conn_status and the deleted self_close field. It touches the TLS certificate-rejection paths only insofar as those now rely on lsquic recording the abort code rather than a session-side copy. Inline findings are posted; this note only records the lifetime and ABI checks ruled out on the new FFI surface.
Findings marked 🟡 are optional suggestions and need no follow-up push.
| // The error lsquic selects for its CONNECTION_CLOSE frame. For an | ||
| // HTTP/3 violation that is an RFC 9114 application code, where node | ||
| // sends and reports transport INTERNAL_ERROR. | ||
| CloseCause::Error => (i32::from(info.is_app), info.code, Some(info.reason)), |
There was a problem hiding this comment.
🔴 Server operators whose graceful close() waits on a stalled request now get closed rejected with transport error 12 after 60 s, where main resolved and Node resolves. The PR notes this follows from its purpose, but it is lsquic's own no-progress abort (es_noprogress_timeout, default 60 s on servers, never set in us_nq_settings_init), not a peer error, and an uncaught await session.close() now ends the process with an unhandled rejection. Fix: keep reporting peer- and lsquic-detected protocol errors through map_close_info at session.rs:2456, but do not surface the self-inflicted no-progress abort as an error after a user close(): set es_noprogress_timeout to 0 for node:quic engines in node_quic_shim.c (Node has no such timer), or map that abort to a clean close. [also at: src/runtime/node/quic/session.rs:1761 - Server operators who call session.close() on an HTTP/3 session with an idle open request now get closed rejected with transport error 12 after 60 s, where the base branch resolved it. report_close at session.rs:1754-1761 no longer records the session's own code-0 close, so lsquic's server-only no-progress abort (es_noprogress_timeout, default 60 s, not set by Bun) becomes the reported error via map_close_info.; src/runtime/node/quic/session.rs:2454 - Server apps whose graceful close() waits on a request the client leaves open now get closed rejected with transport error 12 after 60 s, where the base resolved it.]
Why this was flagged
A server session accepts a request stream, the client keeps the connection alive (keepalive pings) but makes no stream progress, and the server calls session.close() with no code; src/runtime/node/quic/session.rs:1876 defers the close on the open stream.
Verification: normal — acknowledged in diff: the PR description's table lists "a server's close() that waits 60 s on an open request (lsquic's no-progress timer) | closed resolves | transport 12 | [Node] has no such timer" and defers it ("Turning the timer off for node:quic is a separate change"); the note's claims are accurate, but it is a declared cost put up for sign-off, not a resolved one. Trigger: a server session calls session.close() with no code while a request stream stays open without progress until lsquic's server-default es_noprogress_timeout (60 s) fires.
| Buffer.from([0x0a, 0x0a, 0x0a, 0x0a]), | ||
| ]); | ||
| }); | ||
| expect(closed).not.toEqual(expect.objectContaining({ code: "ERR_QUIC_TRANSPORT_ERROR" })); |
There was a problem hiding this comment.
🟡 nit (optional): the version-negotiation test cannot catch a wrong close report other than a transport error. At test/js/node/quic/quic-endpoint.test.ts:369 it only asserts closed is not ERR_QUIC_TRANSPORT_ERROR, so an application error, an idle-type close or any other wrong shape still passes. Fix: assert the exact expected value, expect(closed).toBe("resolved"), since map_close_info returns (2, 0, None) for VersionNegotiationFailed and kFinishClose resolves on code === 0n. [also at: test/js/node/quic/quic-endpoint.test.ts:365 - nit: the version-negotiation test cannot catch a wrong report, so a regression to an application error or a different transport code still passes.]
Why this was flagged
The test at test/js/node/quic/quic-endpoint.test.ts:355-370 drives a client into a failed version negotiation and reads closed through settled, which yields either the string "resolved" or a { code, type, errorCode } object. The assertion at line 369 is not.toEqual(expect.objectContaining({ code: "ERR_QUIC_TRANSPORT_ERROR" })). The code under test maps CloseCause::VersionNegotiationFailed to (2, 0, None) at src/runtime/node/quic/session.rs:2466, and src/js/internal/quic/quic.ts:3640 resolves when code === 0n, so the real expected value is "resolved". If a future change made that path report an application error or any non-transport shape, this assertion would still pass; the base branch has no test for this path at all, so this is a weak new assertion rather than a regression.
Verification: nit. Triggering condition: any future regression that makes a failed version negotiation report an application error, idle close, or other non-transport shape. test/js/node/quic/quic-endpoint.test.ts:369 asserts only expect(closed).not.toEqual(expect.objectContaining({ code: "ERR_QUIC_TRANSPORT_ERROR" })). The base branch already returned (2, 0, None), so nothing in production breaks.
Fixes #45499
Problem
verifyPeer: "strict"and an untrusted certificate,closedresolves andonerrornever runs. Node v26.3.0 rejects with transport 304.map_conn_status(src/runtime/node/quic/session.rs:2487) read onlylsquic_conn_status(), which has no error code. A local error became transport 1, or a clean close when the handshake also failed.Fix
close-cause.patchaddslsquic_conn_close_info(): the close cause, and the error fromconn_close_error(), whichimmediate_close()builds CONNECTION_CLOSE from.on_conn_closedreads it once.self_close(the session's own copy of a close) is deleted: it letclose()with no code hide a later error. This needs therecordedflag of node:quic: send a close with code 0 as code 0, not INTERNAL_ERROR #45702, so its patch and 8 tests are included.test/js/node/quic/quic-endpoint.test.ts(32 new tests, 25 fail on main). Alsotest/js/node/quic/, the 235 portedtest-quic-*.mjs, and the HTTP/3 suites.Background
patches/lsquic/*.patch. lsquic keeps a local error (type, code) inconn->ifc_error, read only byimmediate_close().ifc_errorbesidelsquic_conn_status(). It keeps two reads, and an abort with no code stays a clean close.Downsides
.textis 336 bytes smaller. The teardown callback runs 106 instructions (main 122).Notes
Repro (public API only). A server with a certificate that has no trusted issuer, and a client with
verifyPeer: "strict":openedclosedonerrorcallsERR_INVALID_STATEERR_QUIC_TRANSPORT_ERROR304ERR_QUIC_TRANSPORT_ERROR304--experimental-quicERR_QUIC_TRANSPORT_ERROR304ERR_QUIC_TRANSPORT_ERROR304The server gets transport 304 from the client's CONNECTION_CLOSE on all three. With the default
verifyPeer: "auto"all three print the same:openedrejects,closedresolves. Thestrictblock of Node'stest/parallel/test-quic-reject-unauthorized.mjsfails on main and passes here. The whole file still fails at its line 38 on both builds (the server'sopened), so it is not vendored.Other inputs, side that finds the error. Release builds, main then this PR.
closedresolvesclosedresolvesinitialMaxDataclosedresolvesclose(),close({ reason })orclose({ type: "application" }), then an HTTP/3 violation by the peerclosedresolvesclose())close()that waits 60 s on an open request (lsquic's no-progress timer)closedresolvesCloses that resolved and now reject.
verifyPeer: "strict"check, and any TLS alert that fails the client handshake.initialMaxStreamsUnibelow 3. The client reports application 257. Its frame goes out in a Handshake packet, so the peer gets transportAPPLICATION_ERROR(RFC 9000 section 10.2.3).close()with no code, on a client or a server. On a server this includes lsquic's 60 s no-progress timer (es_noprogress_timeout). A gracefulclose()that waits 60 s on an open request now rejects with transport 12, the code its peer already got on main. Node has no such timer. Turning the timer off for node:quic is a separate change.Codes that change on a close that already rejected. Every error lsquic records: 1 becomes the recorded code (3, 7, 10, 12 and so on, or an HTTP/3 application code). The server's no-progress timer goes from 1 to 12, the code in its frame.
What does not change. A stateless reset stays transport 1 (Node: 10). A handshake timeout stays 0x128 (Node: 1). A failed version negotiation stays a clean close (Node rejects with
ERR_QUIC_VERSION_NEGOTIATION_ERRORafteronversionnegotiation). lsquic has no code for the first two, and the third needs the peer's version list. They aretest.todoentries. Streams that lsquic had opened still resolveclosedwhen the session rejects: that is #45741. Server handshakes that fail in lsquic's mini connection keep their own path (on_mini_conn_failed). The fetch HTTP/3 client still readslsquic_conn_status()for a debug log.The error is the one lsquic selects, not always one it sends. Before a client has Handshake keys, lsquic at this pin sends no CONNECTION_CLOSE. The first four new tests run there: the peer sees nothing and the client reports 10, 7, 266 or 1.
Rejected self-review concerns.
ietf_full_conn_ci_status(). That status is shared with the fetch HTTP/3 client, and open http(h3): use TCP when the QUIC connection to an Alt-Svc alternative cannot be established #45539 and http(h3): bounded retry budget for the fetch client's pre-headers failures #41564 readLSCONN_ST_VERNEG_FAILUREas "the handshake failed". A change there changes their input with no compile error. The test stays inlsquic_conn_close_info(). For the same reason theLSCONN_ST_*constants stay inlsquic_sys.onversionnegotiation. That is a separate change.Other open PRs.
conn_close_error()testsifc_error.recorded. If node:quic: send a close with code 0 as code 0, not INTERNAL_ERROR #45702 merges first, this PR drops those two commits in a rebase. If this PR merges first, node:quic: send a close with code 0 as code 0, not INTERNAL_ERROR #45702 has nothing left to add.code#44846 changes the same line of lsquic (now insideconn_close_error()) andparse_close_options. It has to rebase. It must not bringself_closeback.session.rs. node:quic: give a stream still open at connection close the connection's error #45741 and node:quic: make the session the server connection's context from its first packet #46226 touch other code.strictmessage inquic-sni.test.ts. It now expectsQUIC transport error 300: TLS alert 44.Tests. 24 new tests and 3
test.todoin onedescribe, plus the 8 of #45702. Anode:dgramsocket plays the server for four of them: it answers the client's first packet with one Initial packet built withnode:crypto(the keys of an Initial packet come from the client's first connection ID, RFC 9001 section 5.2). 7 of the 32 pass on main and pin what must not change. 1,280 runs of the 32 tests passed on the debug build with four runners in parallel.Measurements. Release builds of b1d4686 and of this diff on it.
stepi, 20 of 20 teardowns identical.lsquic_conn_close_infocall, 0 separateietf_full_conn_ci_statuscalls (main 1), 0 mallocs inon_conn_closedon a clean close (main 0), 1 when lsquic recorded a message (main 1)..text81,997,829 to 81,997,493 B,.rodata22,627,276 B on both (llvm-size -A).immediate_close771 to 776 B,on_conn_closed402 to 373 B,report_close1723 to 1398 B,lsquic_conn_close_info223 B.ietf_full_conn_ci_packet_in,lsquic_engine_packet_in,lsquic_engine_process_connsandietf_full_conn_ci_tickhave identical instruction lists (llvm-objdump, data addresses normalized).process_regular_packetgains 4 instructions, one store in each of its abort branches (therecordedflag).sizeof(struct ietf_full_conn): 10,232 B on both (themi_callocsize).sizeof(QuicSession): 1120 to 1088 B, same 1280 B bin.test-quic-session-initial-rtt.mjsfails on both debug builds, and threetest-quic-callback-error-*files fail on both when four run in parallel.lsquic_close_info: 24 B in C and Rust, offsets equal (gdbptype /o).bun run rust:check-all: 12 of 12 targets pass.