feat(native): default quinn and quiche to delay-based congestion control - #2468
Conversation
Live media wants a send rate an encoder can track, so BBR beats CUBIC's
sawtooth as the out-of-the-box behavior. quinn now defaults to BBRv1 and
quiche to BBRv2 gcongestion when --{client,server}-quic-congestion-control
is unset.
noq and iroh move the other way, from BBRv3 to CUBIC. Their shared BBRv3
subtracts without a floor when computing the inflight bytes at the loss
event (noq-proto inflight_at_loss), so a single loss can underflow and
panic, aborting the process. That is not acceptable on a default path, so
delay-based stays reachable there only when asked for by name.
Each backend now picks its own default locally, since they no longer
agree. quic::Resolved keeps congestion_control as an Option so "unset" is
distinguishable from an explicit request.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @kixelated, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
|
Warning Review limit reached
Next review available in: 13 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: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
WalkthroughQUIC congestion control documentation now specifies backend-specific defaults and warns about the noq/iroh BBRv3 implementation. Quinn and quiche explicitly configure Delay when unset, while noq and iroh use Loss through centralized helpers. Tests verify unset and explicit settings for iroh, noq, and quiche. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches✨ Simplify code
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
rs/moq-native/src/quinn.rs (1)
70-74: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a regression test for Quinn's unset default.
This changes Quinn's effective default from CUBIC to BBR, but the supplied diff does not test
Client::default()throughapply_transport. Cover the unset case and explicitLossmapping so a future change cannot silently restore Quinn's CUBIC default.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rs/moq-native/src/quinn.rs` around lines 70 - 74, Extend the regression tests around apply_transport to exercise Client::default() with no congestion_control configured, asserting it selects the BBR congestion controller instead of Quinn’s CUBIC default. Also add coverage for an explicit CongestionControl::Loss value and assert it maps to the expected loss-based controller, using the existing transport/test helpers.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@doc/bin/relay/config.md`:
- Around line 197-198: Remove the generic "delay" value from congestion_control
in doc/bin/relay/config.md, leaving it unset or making it backend-specific.
Update the noq/iroh client option near quic.rs lines 139-140 and the noq server
option near lines 254-255 to explicitly warn that selecting delay (BBRv3) can
panic on packet loss and is intended only for deliberate testing.
In `@rs/moq-native/src/quic.rs`:
- Around line 139-140: Update the congestion-control documentation comment in
quic.rs to explicitly warn that selecting delay on noq or iroh may panic when
packets are lost, and state that it should only be used for testing. Retain the
existing default-family information and platform distinctions.
- Around line 254-255: Update the congestion-control option help text in the
QUIC configuration documentation to state that explicitly selecting delay for
the noq server may panic on packet loss, while retaining the existing
explanation that loss remains the default because noq’s BBRv3 is not suitable by
default.
---
Nitpick comments:
In `@rs/moq-native/src/quinn.rs`:
- Around line 70-74: Extend the regression tests around apply_transport to
exercise Client::default() with no congestion_control configured, asserting it
selects the BBR congestion controller instead of Quinn’s CUBIC default. Also add
coverage for an explicit CongestionControl::Loss value and assert it maps to the
expected loss-based controller, using the existing transport/test helpers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 36bf7d7c-cbae-4f0c-ada4-d38acc91b04a
📒 Files selected for processing (6)
doc/bin/relay/config.mdrs/moq-native/src/iroh.rsrs/moq-native/src/noq.rsrs/moq-native/src/quic.rsrs/moq-native/src/quiche.rsrs/moq-native/src/quinn.rs
Address review feedback on #2468. The relay config example set congestion_control = "delay" with no caveat, which is right for the default quinn backend but is the panicking path on noq/iroh. Say so in the example, in both clap doc comments (which become --help), and in the prose warning. Also extract quinn's default into a named congestion_control() fn so it can be unit-tested, matching noq/iroh. Nothing else could pin quinn's unset default: TransportConfig doesn't expose the factory it was given. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Thanks, that's a concrete profile I can build to, and the "removes signal" point on random loss is well taken. I've re-specced our outstanding CC test to exactly your numbers: 10 Mb/s source → 5 Mb/s cap, 100 ms base RTT, 500 ms queue depth, reporting delivered goodput (→5) and standing RTT together — expecting BBR near ~100 ms and CUBIC bloating toward ~500 ms, with an fq_codel/cake counterfactual to see AQM tame CUBIC. I'll also do a 2–3-flow fairness run at the same bottleneck. Dropping reordering and the random-loss sweep from the "CC" framing entirely — agreed they conflate loss-response with congestion-response and reward zero-CC for the wrong reason. I've relabelled our earlier CUBIC-collapse-under-random-loss table accordingly (loss-signal interpretation, not CC quality). Also saw #2468 land — makes sense, and it clears up something on our side: our earlier "noq-BBRv3 is unstable" datapoint (one run 71%, next 8%) was almost certainly the #768 inflight_at_loss subtract-overflow panic aborting the relay mid-capture, not a CC effect. So I'm parking BBRv3 until that fix is upstream, and pinning --*-quic-congestion-control explicitly on every run now that the defaults have moved. When I get to the bufferbloat run I'll post whether the |
2 participants
Summary
--{client,server}-quic-congestion-controllands on BBR for quinn (BBRv1) and quiche (BBRv2 gcongestion) instead of each backend's own CUBIC. Live media wants a send rate an encoder can track, not CUBIC's sawtooth.inflight_at_losscomputes(inflight_prev_threshold.round() as u64) - lost_prevwith a plain-while every neighboring term usessaturating_sub, so oncelost_prevexceedsLOSS_THRESH * inflight_previt underflows and panics, aborting the process. Since feat(moq-native): expose a congestion control knob for the quinn backend #2432 that has been the default on those backends. A panic is not an acceptable default path, so they sit on CUBIC until the fix lands upstream.delayis still selectable there by name.quic::Resolved::congestion_controlstaysOption<CongestionControl>so "unset" remains distinguishable from an explicit request.Public API changes
None.
Resolvedispub(crate); thepubquic::Client/quic::Servercongestion_controlfields keep theirOption<CongestionControl>type and only their doc comments changed. This is a default-behavior change, not a contract break, hencemainrather thandev.Cross-package sync
doc/bin/relay/config.mdupdated: the per-backend table's "Default when unset" column now matches, plus a note on why noq/iroh are the exception. No other row in the table applies (no wire, catalog, FFI, or CLI-surface change; the flag names and values are untouched).Test plan
cargo test -p moq-native --all-features— 79/19/11/75 pass, including two new tests,noq::tests::congestion_control_defaults_to_lossandiroh::tests::congestion_control_defaults_to_loss, which pin the CUBIC default so a later edit can't quietly put the panicking controller back on the default path.quiche::tests::apply_settings_writes_cc_algorithmnow asserts the unset case reachesbbr2_gcongestionand that an explicitlossstill reachescubic.cargo clippy -p moq-native --all-features --all-targets -- -D warnings,cargo fmt --all --check,RUSTDOCFLAGS=-D warnings cargo doc -p moq-native --all-features.bun remark(nonode_modulesin this worktree) and the smoke matrix.Reviewer note
quinn's BBRv1 and quiche's BBRv2 now ship on by default without the soak time CUBIC has here. The measurements behind this are quinn-only (CUBIC p50 RTT ~558ms vs BBRv1 ~90ms at equal goodput under bufferbloat, via
just demo cc compare bloat); quiche's BBRv2 default is going out without a comparable local measurement.(Written by Opus 4.8)