Repository navigation
Conversation
…ngines lsquic's default adaptive mode picks BBRv1 whenever a connection's handshake RTT is over 1.5 ms, which any JS work on the loop that ticks the engine produces. BBRv1 can then only ever observe one cwnd per loop iteration as bandwidth, so cwnd never grows in startup and collapses to the 4-packet minimum once startup ends. Pin both engines to Cubic, which only reacts to loss, and add tests that measure how much a response body and a streamed request body move while the serving loop is busy.
|
Warning Review limit reached
Next review available in: 15 minutes 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 (4)
Comment |
|
Status: reproduced and fixed; CI is green on 47b966e (build 92179, all 190 jobs, including both Windows lanes). Reproduced with the two new tests in The first CI run failed the new tests on the Windows lanes; that was a second, pre-existing bug the Cubic switch would have exposed on every Windows connection (Cubic's pacing rate wraps in 32-bit arithmetic), fixed here by |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — the native change is a single settings field (es_cc_algo = 1, Cubic) applied via one helper to both engines, with a thorough root-cause writeup and verified regression tests. The inline nit about the tautological total >= secondHalf assertion is worth cleaning up but doesn't affect the test's ability to catch the regression.
Extended reasoning...
Overview
The PR sets es_cc_algo = 1 (Cubic) on the lsquic engine settings for both us_create_quic_socket_context (Bun.serve HTTP/3) and us_create_quic_client_context (fetch HTTP/3), via a shared us_quic_use_cubic helper placed right after lsquic_engine_init_settings. Two new tests in serve-http3.test.ts reproduce the BBRv1 window collapse on a busy event loop (one per direction) and assert Cubic-level throughput. node:quic is intentionally left alone since it exposes cc to the user.
Security risks
None. This is a congestion-control algorithm selection with no bearing on auth, TLS, or input validation.
Level of scrutiny
Moderate. It changes a production network default, but the change surface is one field on a settings struct, the value 1 = Cubic is confirmed against the in-tree lsquic reference (src/runtime/node/quic/endpoint.rs:1065), Cubic is what lsquic's adaptive mode already picks on a quiet loop and is the default in the other major QUIC stacks cited, and the PR's root-cause analysis (adaptive → BBRv1 above 1.5 ms handshake srtt → cwnd collapses to the 4-packet minimum on a busy loop) is backed by BUN_DEBUG_lsquic output and a measured before/after table.
Other factors
- Verification is thorough: both new tests fail 5/5 on unfixed debug main and 6/6 with
USE_SYSTEM_BUN=1, pass across 8 fixed-build runs with a >3× margin over the 1 MiB threshold, and the full h3 suite (plus the related fetch-http3 files) passes. - The tests pace via
Bun.sleepSyncinside asetImmediateloop and count iterations rather than wall-clock time, which is about as deterministic as a CC throughput test can be; the observed pass (3.5-7.2 MiB) vs fail (0-0.5 MiB) values leave a wide margin around the 1 MiB threshold. - No CODEOWNERS match these paths and there are no outstanding reviewer comments.
- The one finding is a nit (a
total >= secondHalfassertion that holds by construction of the accumulator loop). The load-bearingsecondHalf >= MIN_SECOND_HALF_BYTESon the next line is what actually distinguishes Cubic from a collapsed BBRv1 window, so the nit doesn't weaken the test.
|
Updated 4:16 AM PT - Aug 11th, 2026
✅ @robobun, your commit 47b966e646bd5d8643c46893d8e8d6bf48333ff8 passed in 🧪 To try this PR locally: bunx bun-pr 37455That installs a local version of the PR into your bun-37455 --bun |
Cubic keeps its windows in unsigned long, which is 32 bits on Windows, and its pacing rate is cwnd * 1000000 / srtt computed in that type, so the product wrapped for every window and Cubic connections on Windows were paced at an essentially random rate. The two loss-reduction products wrap the same way once the window passes ~4.6 MB. With both engines pinned to Cubic every HTTP/3 connection on Windows would have hit the pacing one.
Prime both engines with two idle round trips before the busy window so the controller under test has a real loopback min RTT (lsquic takes no sample for packets sent before the first acknowledgment), open the busy window when the transfer starts instead of after an idle phase that left Cubic loss-capped, and bound the server's count by what the client sent instead of an assertion that held by construction.
robobun
changed the title
|
Pushed d433735 and 47b966e. The first CI run failed the new tests on both Windows lanes with the Cubic switch in place. The lsquic log on a Windows build showed the cause: Cubic's pacing rate is computed as |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. The earlier nit about the upload test's vacuous total >= secondHalf assertion is resolved — the current diff checks total <= enqueued * CHUNK_SIZE, which is a cross-process comparison and can fail. Because this flips the congestion controller for every HTTP/3 connection, adds a vendored lsquic patch, and lands timing-based throughput tests, a human look is still worthwhile.
What was reviewed
es_cc_algo = 1applied via a shared helper at both engine init sites; confirmednode:quicsets its ownccand is unaffected.- The LLP64 patch:
(uint64_t)widening beforecu_cwnd * 1000000and the two* BETA / 1024products inlsquic_cubic_loss— arithmetic is correct, results still fitunsigned longafter the divide. - The two new tests: threshold (1 MiB) sits well between the reported unfixed (~0.4 MiB) and fixed (3.5–7 MiB) numbers; fixtures use
port: 0, spawn viawithCustomServer, and drain stderr.
Extended reasoning...
Overview
Four files: packages/bun-usockets/src/quic.c adds a one-line us_quic_use_cubic() helper setting es_cc_algo = 1 and calls it from both us_create_quic_socket_context and us_create_quic_client_context; patches/lsquic/cubic-llp64-overflow.patch widens three unsigned long multiplications in lsquic_cubic.c to uint64_t so they don't wrap on Windows; scripts/build/deps/lsquic.ts registers the patch; test/js/bun/http/serve-http3.test.ts gains a ~150-line describe block with two throughput tests (response body and streamed request body) that pace the server's event loop with Bun.sleepSync and assert bytes moved during a settled 60-iteration window.
Security risks
None identified. The change is a congestion-controller selection and an integer-widening patch to vendored code. No auth, crypto, parsing of untrusted input, or user-facing API surface is touched. The lsquic patch only inserts (uint64_t) casts before existing multiplications; the divisors (1024, srtt) and downstream stores are unchanged.
Level of scrutiny
Medium-high. The runtime change is tiny (one settings field), but it alters congestion-control behavior for every HTTP/3 connection in both Bun.serve({ http3: true }) and fetch({ protocol: "http3" }). The rationale is well-argued (adaptive→BBRv1 collapses when RTT samples include event-loop latency; Cubic is loss-driven and is the default in quiche/ngtcp2/msquic/quic-go), and the PR description traces the mechanism through lsquic source. Still, protocol-level performance changes and vendored-dep patches are the kind of thing REVIEW.md flags for maintainer sign-off ("Dependencies & vendoring", "Performance").
The tests are timing-based throughput measurements. The margin looks comfortable (threshold 1 MiB vs. ~0.4 MiB unfixed / 3.5–13 MiB fixed across Linux and Windows debug per the description), and the fixture measures only the second half of a 120-iteration window to let the controller settle. But throughput assertions across CI platforms (macOS, Windows, ASAN) carry inherent flake risk that a human should sanity-check against the CI run.
Other factors
- My earlier inline nit (vacuous
total >= secondHalfassertion) is not present in the current diff; the upload test now assertstotal <= enqueued * CHUNK_SIZE, a cross-process check analogous to the download test'sbody.length === chunks * CHUNK_SIZE. - Verified the helper is called after
lsquic_engine_init_settingsat both sites so the default isn't re-overwritten. - The LLP64 patch cites upstream litespeedtech/lsquic#391; the widened products (
cwnd * 819 / 1024,cwnd * 922 / 1024,cwnd * 1e6 / srtt) all fit back intounsigned longafter the divide on realistic window sizes, andbandwidth/pacing_rateare alreadyuint64_tin the surrounding function. node:quicis explicitly left alone (it exposesccto the user), so this only affects the two Bun-owned engines.
|
One correction to the summary above, for the record: node:quic only applies |
Problem
An HTTP/3 body served by
Bun.serve({ http3: true })from a process whose event loop is doing other work crawls, while the same body over H1/H2 is unaffected. The same happens to a streamed request body sent byfetch(url, { protocol: "http3" })to such a server.Repro shape (this is what the new tests do): an h3 server paces its own loop with
Bun.sleepSync(5 ms per iteration while the connection is set up, 12 ms per iteration while the body moves, with a couple of idle round trips in between) and a client in another process moves a stream of 512 KiB chunks through it. Bytes moved during the second 60 busy iterations:BUN_DEBUG_lsquic=1on the serving process on main:after which the window sits at BBR's 4-packet minimum (
5840) and the connection moves 5 or 6 packets per loop iteration.Cause
us_create_quic_socket_contextandus_create_quic_client_contextleavees_cc_algoat lsquic's default, 3 = adaptive. Adaptive mode makes a one-shot, permanent choice per connection (send_ctl_select_cc, run from the first RTT sample of the full connection, whose srtt is dominated by the handshake samples): at or belowes_cc_rtt_thresh(1.5 ms) it selects Cubic, above it BBRv1.Both of Bun's engines are only ticked from an event loop (
us_quic_loop_processfrom loop pre/post for the server, the HTTP thread's loop for the client), so every RTT lsquic measures includes whatever ran between two ticks. On a server doing any work during the handshake the srtt is several ms even on loopback, so BBRv1 is selected, and which controller a connection gets depends on what JS happened to be running when it arrived.BBRv1 then models loop latency as the path. Its window target is
gain * max_bandwidth * min_rtt. The only bandwidth it can observe is one window per loop iteration, andmin_rttis the quickest round trip the connection ever saw (any connection that caught the loop idle for a moment has a sub-millisecond one), so the target is a fraction of the current window: startup never grows it, and as soon as startup exits (three rounds without bandwidth growth) each round shrinks it by roughly2 * min_rtt / iterationuntil it sits at the minimum, as in the log above. A response made of small chunks is application-limited every tick and stays in startup at roughly the initial window instead; that is the "about one initial cwnd per loop iteration" shape of the original report.Fix
packages/bun-usockets/src/quic.c: setes_cc_algo = 1(Cubic) for both engines, in one helper so they stay in sync. Cubic only reacts to loss, which a slow loop does not produce, so its window grows until the path (here the receiver's UDP buffer) actually drops something, and every connection gets the same controller. It is also what adaptive mode already picks for every connection whose handshake lands on a quiet loop, so this makes the quiet-loop behavior the only behavior; Cubic is the default in the other QUIC stacks as well (quiche, ngtcp2, msquic, quic-go). The client engine is included because its RTT samples are inflated by the server's loop in exactly the same way and its send direction carries request bodies (second row of the table).patches/lsquic/cubic-llp64-overflow.patch: the first CI run failed the new tests on both Windows lanes with the fix applied, and the lsquic log on a Windows build showed why. Cubic keeps its windows inunsigned long, 32 bits on Windows, and computes its pacing rate ascu_cwnd * 1000000 / srttin that type, so the product wraps for every window (the initial one is 46720 bytes) and the pacer runs at(cwnd * 10^6 mod 2^32) / srtt: on the Windows build the log showed packets being scheduled 1186 us apart with a 52 KB window and a 4.8 ms srtt, i.e. about 1 MB/s, and the window growing to 7.8 MB without a loss because so little of it was ever in flight. Upstream still has this (lsquic_cubic.con master) and had it reported as a Windows throughput problem that went away with pacing disabled or BBR selected (litespeedtech/lsquic#391). On main it only affected the connections adaptive mode gave to Cubic; with every connection on Cubic it would have affected every HTTP/3 connection on Windows, so it has to land with this change. The patch widens the three products (the pacing rate, and the two loss reductions, which wrap once the window passes ~4.6 MB); a no-op on LP64 platforms. Measured on Windows with the final tests: 2 MiB in total for the response body and 1.6 to 1.9 MiB in the window for the request body without the patch, the numbers in the table with it.Not changed:
node:quicexposesccto the user and currently leaves the default to lsquic; that is tracked separately. Throughput of a busy h3 server is still bounded by the UDP socket buffers (#37452, independent) and by lsquic's pacer releasing about ten packets pluses_clock_granularityworth per engine tick, which is why the healthy numbers above are a few MiB rather than line rate; this change removes the collapse.Verification
Two tests in
test/js/bun/http/serve-http3.test.ts("congestion control"), one per direction. The fixture counts its loop iterations; after two idle round trips (lsquic takes no bandwidth/RTT sample for packets sent before the connection's first acknowledgment, so the first one only primes the sampler) the transfer runs for 120 iterations of 12 ms and the fixture reports how much the connection moved during the last 60; the tests require at least 1 MiB. Chunks are 512 KiB so every tick is window-limited rather than data-limited.bun bd test test/js/bun/http/serve-http3.test.ts -t "congestion control"passes 4/4 on Linux (4 MiB and 7.2 to 7.3 MiB) and 4/4 on a Windows debug build (8.5 to 13 MiB and 7.1 MiB). The whole file (50 tests) andfetch-http3-client.test.ts(53) pass on both;fetch-http3-adversarial,fetch-http3-cold-post,serve-protocolsandserve-direct-readable-streampass on Linux.quic.cchange: a Linux debug build fails both 8/8 (0.5 MiB and 0.42 MiB), the released Linux binary 4/4 and the released Windows binary 3/3 with the same numbers.fetch-backpressure.test.tsstill time out under that file's load on a debug build, as they do on main; that is what test: serve the h3 receive-backpressure bodies from an idle process #37445 fixes.