Repository navigation
Conversation
lsquic's default windows are for HTTP/3: 0 for a bidirectional stream that the server opens, and 12 KiB (server) or 32 KiB (client) for a unidirectional stream. lsquic accepts 16 KiB where it advertises 0, and it picks the first send limit of a stream by the role of the connection. So a stream that a node:quic server opened ended with a flow control violation above 16 KiB, in both directions, and a unidirectional stream opened in 0-RTT ended above 12 KiB. us_nq_settings_init now uses the window of the client-opened bidirectional streams for all three: 1 MiB on a server, 6 MiB on a client. With equal windows such a sender reads the right number whichever window it takes.
|
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 6 minutes. 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 |
|
Status How the failure reproduces, on a release build of main (71bc818) and on bun 1.4.2. Both ends are in one process and use default
With this PR both bodies arrive. The tests are the |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| } as SessionOptions); | ||
| client.closed.catch(() => {}); | ||
| const [down, up] = await Promise.all([clientRead.promise, serverRead.promise]); | ||
| client.close(); |
There was a problem hiding this comment.
🟡 (optional) When one of the new exchange tests fails, the runner reports a timeout instead of the real error, and each failing variant stalls about 10 s. The dismissal assumed the endpoint's asyncDispose tears sessions down, but close() is graceful: quic.ts:4655-4661 only sets isPendingClose and endpoint.rs:2568-2571 finishes only once sessions is empty. If Promise.all at :680 rejects while the client session is still alive, client.close() at :681 is skipped and the await using endpoint at :664 waits for the session's default 10 s idle timeout (endpoint.rs:113) before the rejection propagates. Fix: close the client before any await can reject (register it in a try/finally or using before :680) or set transportParams: { maxIdleTimeout: 1 } as the file's earlier tests at :43 and :87 do, in both helpers. [also at: test/js/node/quic/quic-stream.test.ts:726 - nit: when a connection in the new helpers dies before its stream delivers, the test hangs until the runner timeout instead of failing with the error.]
Why this was flagged
Trigger: serverRead.reject fires at :659 (a server-side read error, a session that errors without dying) while the client session is open, in serverOpenedExchange, or received.reject at :699 in resumedEarlyStream. The helper throws before reaching client.close() at :681 (second.close() at :734). Async disposal of endpoint runs before the error leaves the function: QuicEndpoint[SymbolAsyncDispose] at quic.ts:4894 awaits close(), which does not close sessions; close_gracefully at endpoint.rs:2568 only schedules processing and the gate at endpoint.rs:1533 needs sessions empty. With no maxIdleTimeout set, the session lives for DEFAULT_MAX_IDLE_TIMEOUT_SECS (10 s, endpoint.rs:113), so the real rejection arrives after the harness timeout and the test fails as a timeout with no diagnostic. On the base branch these tests do not exist; the existing tests at :43 and :87 bound this with maxIdleTimeout: 1.
Verification: serverOpenedExchange does const [down, up] = await Promise.all([clientRead.promise, serverRead.promise]); client.close(); (test/js/node/quic/quic-stream.test.ts:680-681), so a rejection skips the manual close. close_gracefully finishes only if self.sessions.get().is_empty() (src/runtime/node/quic/endpoint.rs:2568-2572), so disposal waits for the orphaned client session to die on its own, about 10 s.
| ["to the client", 256 * 1024, 1024], | ||
| ["from the client", 1024, 256 * 1024], | ||
| ])("let a bidirectional stream that the server opens carry 256 KiB %s", async (_, downSize, upSize) => { | ||
| const down = pattern(downSize, 1); | ||
| const up = pattern(upSize, 2); | ||
| expect(await serverOpenedExchange({ down, up })).toEqual({ down: summary(down), up: summary(up), errors: [] }); | ||
| }); |
There was a problem hiding this comment.
🟡 (optional) The "256 KiB from the client" variant at quic-stream.test.ts:785-791 passes on the unfixed build whenever it runs after the other tests, so it does not prove the client-to-server direction of the fix and a later regression in that direction can go green. The PR description itself reports it fails only when run alone. Only the up size (upSize) differs between the two rows, and the server reads as fast as data arrives, so its window moves before the violation. Fix: make the variant discriminating on base, for example by having the server delay reading until the full body is sent (await a signal before readAll at :657) so the advertised window is the only thing that lets 256 KiB through, or drop the row and state why one direction suffices.
Why this was flagged
Trigger: run the file with the fix for the server-side bidi_local window absent (node_quic_shim.c:449 reverted) on a build where earlier tests have warmed the engine; the PR author reports the "from the client" row then passes in every run. REVIEW.md asks that deleting each load-bearing clause of the fix breaks at least one test and that a test which passes both ways is worse than none; this row's only purpose is the client-to-server direction and it cannot fail for that reason reliably. The "are one value per endpoint" test at :745 does catch the clause by asserting transport parameters, so the fix is covered, but the behavioural test for that direction is vacuous in practice and adds 256 KiB of transfer (slow under ASAN per CLAUDE.md) without discriminating power. Remedy: gate the server's read on a signal so the window, not read speed, governs, or remove the row with a stated reason.
Verification: The two rows at test/js/node/quic/quic-stream.test.ts:785-786 differ only in sizes, and the only assertion (line 790) is that the exchange completes. In serverOpenedExchange (lines 644-679) the server consumes the stream immediately, so on the unfixed build the row fails only if the client's burst outruns the server's MAX_STREAM_DATA grants, a timing race, not a deterministic failure.
| /* lsquic's default windows are for HTTP/3: 0 for a bidirectional stream | ||
| * that the server opens (a server's bidi_local, a client's bidi_remote), | ||
| * and 12 KiB or 32 KiB for a unidirectional stream. Use the window of the | ||
| * client-opened bidirectional streams for all three. Upstream lsquic, and | ||
| * so every bun up to 1.4.x, picks the first send limit of a bidirectional | ||
| * stream by the role of the connection, not by who opened the stream, and | ||
| * after 0-RTT it gives a unidirectional stream the limit of a | ||
| * bidirectional one. With equal windows such a sender reads the right | ||
| * number in both places. Node's default is 256 KiB for every stream | ||
| * window (node/src/quic/transportparams.h). */ |
There was a problem hiding this comment.
🟡 nit (optional): maintainers get a ten-line comment that argues why copying one window into the other two is safe, which CLAUDE.md rule 13 names as a sign the code is wrong. The real defect is lsquic choosing a stream's send limit by connection role in apply_trans_params, and the paragraph exists only to justify working around it at packages/bun-usockets/src/node_quic_shim.c:438-447. Fix: keep one line stating the intent (one default stream window per endpoint, lsquic's H3 defaults are 0 and 12/32 KiB) and move the lsquic send-limit rationale to the PR or the lsquic patch that corrects it, so the comment says only what the code cannot.
Why this was flagged
The diff adds a 10-line block comment at packages/bun-usockets/src/node_quic_shim.c:438-447 ahead of the two assignment branches at 448-454. The root CLAUDE.md at /home/claude/bun/CLAUDE.md:206 says a paragraph-long comment justifying why a workaround is OK means the code is wrong, and /home/claude/bun/CLAUDE.md:207 says a comment that is not clearly needed is noise. The comment's second half (lsquic picks the send limit by role, gives a uni stream a bidi limit after 0-RTT) describes an lsquic defect the PR text says a later lsquic change fixes, so it will go stale and nobody reading the shim can act on it. The same explanation is repeated in the test file at test/js/node/quic/quic-stream.test.ts:722-726. No runtime behaviour differs from the base branch; this is a convention finding only.
Verification: packages/bun-usockets/src/node_quic_shim.c:438-447 is a 10-line block comment preceding the two assignment branches at :448-454. Lines 441-446 exist only to argue why copying one window into the other two is safe given an lsquic defect in the send-limit selection, which CLAUDE.md:206 rule 13 names. Triggering condition: unconditional, the comment and workaround ship as written.
| if (is_server) { | ||
| s->es_init_max_stream_data_bidi_local = s->es_init_max_stream_data_bidi_remote; | ||
| s->es_init_max_stream_data_uni = s->es_init_max_stream_data_bidi_remote; | ||
| } else { | ||
| s->es_init_max_stream_data_bidi_remote = s->es_init_max_stream_data_bidi_local; | ||
| s->es_init_max_stream_data_uni = s->es_init_max_stream_data_bidi_local; | ||
| } |
There was a problem hiding this comment.
🟣 pre-existing, not blocking: node:quic users who pass only one of the three stream windows in transportParams still get server-opened streams killed with "flow control violation", though the PR promises one window per endpoint. The copy at node_quic_shim.c:448-454 runs before the NQ_SET setters at :466-468 that apply_transport_params calls (endpoint.rs:1002-1010), so a server with only initialMaxStreamDataBidiLocal: 262144 advertises 262144 for its own streams but 1048576 for bidi_remote, and the peer sends up to 1048576. Fix: derive every window the user left unset from the one they set, i.e. run the equalising step after the setters (or in apply_transport_params), so equal windows hold for every partial transportParams, not only for the no-options default.
A small fix can ride a push you are already making; otherwise a short reply is enough.
Why this was flagged
A server calls listen with transportParams: { initialMaxStreamDataBidiLocal: 262144 } (Node's default, a natural port) and opens a bidirectional stream. us_nq_settings_init copies bidi_remote into bidi_local at node_quic_shim.c:449, then the setter at :466 overwrites bidi_local with 262144 while bidi_remote stays 1048576. lsquic picks the client's send limit by role (the server's bidi_remote, 1048576), so the client sends past 262144 and the server kills the connection with QUIC transport error 1. The base branch fails the same way, but this PR's stated invariant (equal windows make the role-based pick correct) is broken by the simplest user configuration, and the dismissal relied on the setters still applying rather than on them applying after the copy. The remedy lives in this checkout: fill unset windows from the user-set one after apply_transport_params so the invariant holds for partial transportParams.
Verification: pre-existing; acknowledged in diff: the PR description says "Explicit windows that differ still fail." The copy at packages/bun-usockets/src/node_quic_shim.c:448-454 runs before the setters at :466, so a server given only initialMaxStreamDataBidiLocal: 262144 advertises bidi_local=262144, bidi_remote=1048576 → FLOW_CONTROL_ERROR, connection killed. The base fails by the same route.
Problem
transportParams, a node:quic server opens a bidirectional stream and sends more than 16 KiB. The connection dies:QUIC transport error 1: flow control violation on stream 1.us_nq_settings_init(packages/bun-usockets/src/node_quic_shim.c:424) keeps lsquic's HTTP/3 windows: 0 for a server-opened bidirectional stream, 12 or 32 KiB for a unidirectional one. lsquic also reads the wrong window as the send limit.Fix
transportParamsstill win.test/js/node/quic/quic-stream.test.ts, 4 new tests, main fails 3 in every run. Also the 235test-quic-*.mjsfiles.Background
bidi_local), bidirectional that its peer opens (bidi_remote), unidirectional.Bun.serveandfetchdo not change.Downsides
Notes
How it was found. The failure is in the "Found, not in this PR" list of #46054. No user reported it.
Why equal windows are enough for the default case. lsquic keeps one send limit for bidirectional streams and picks it by the role of the connection (
apply_trans_params,lsquic_full_conn_ietf.c). A server takes the client'sbidi_local, a client takes the server'sbidi_remote. That is right only for a stream the client opens. After 0-RTT the same function gives a unidirectional stream the server'sbidi_remote. When an endpoint's three windows are equal, every one of these reads returns the right number. The same values set by hand throughtransportParamscarry 4 MB each way on main and on bun 1.4.2.Measurements. Release builds of main 71bc818 and of this PR. A server-opened bidirectional stream, two processes on loopback, 6 runs each. "1.4.2" is the released bun.
No pair is worse than before. A new server that sends to a 1.4.x client still overruns that client's window, because the client advertises 0 and accepts 16 KiB.
Bun.serveandfetch: 72 and 88 bytes before and after.quic-test: 1533 to 1545 before, 1541 to 1553 after. 80 tickets each. The range is the connection ID length.size). One symbol changed,QuicEndpoint::build_engine, +219 bytes.Tests. On a release build of main, "are one value per endpoint", "256 KiB to the client" and the 0-RTT test fail in 6 of 6 runs. "256 KiB from the client" depends on timing there: it fails in 12 of 12 runs as the only test of a process and passes after the other tests. No body size makes that direction fail in every run, because the server reads as fast as the data arrives and its window moves.
Also run on the debug build: all 235
test/js/node/test/parallel/test-quic-*.mjsfiles. 229 pass. The 6 others fail on main as well: 2 importnode:stream/iter, 1 asserts an RTT below 10 ms, 3test-quic-callback-error-*are timing dependent (1 to 2 of 6 runs on both builds).Not in this PR. lsquic still picks the send limit by role. With explicit windows that differ from each other, a server-opened stream still fails. #46322 is based on this PR and changes lsquic.