feat(moq-native): add quic::Client/quic::Server transport config#2161
Conversation
![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
WalkthroughIntroduces shared client and server QUIC configuration types with resolution, defaults, CLI/TOML parsing, and validation tests. Native noq, quinn, quiche, and iroh backends now consume resolved QUIC settings for transport configuration. Client and server configuration fields are nested under QUIC-specific structures. CLI and relay startup paths pass resolved settings to iroh and initialize stream defaults through the new fields. Integration and configuration regression tests were updated for the new binding and TOML layouts. 🚥 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 |
7c3b147 to
3a642e8
Compare
kixelated
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
rs/moq-native/src/quic.rs (1)
27-106: 📐 Maintainability & Code Quality | 🔵 TrivialConsider reducing duplication of per-connection knobs between
ClientandServer.The five per-connection fields (
max_streams,gso,idle_timeout,keep_alive,mtu_discovery) and their#[serde]attributes are duplicated betweenClientandServer, and bothresolve()implementations are identical. The#[arg]ids/envs differ by prefix (client-vsserver-), so a shared embedded struct won't work, but a macro or a shared trait forresolve()could reduce the maintenance surface.Also applies to: 108-227
🤖 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/quic.rs` around lines 27 - 106, Reduce duplication between the `Client` and `Server` QUIC configuration types by introducing a shared abstraction for the five per-connection knobs and their serde behavior, while retaining distinct clap `#[arg]` identifiers and environment-variable prefixes. Use a suitable macro or shared helper to generate the repeated fields and attributes, and factor the identical `resolve()` logic into a shared trait or helper used by both implementations.
🤖 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 `@rs/moq-native/src/noq.rs`:
- Line 12: Add a concise rustdoc comment directly above the public re-export
`pub use web_transport_noq;`, describing what the re-export provides and why
consumers may use it.
In `@rs/moq-native/src/quinn.rs`:
- Line 10: Add a rustdoc comment directly above the public re-export `pub use
web_transport_quinn;`, briefly describing what the re-export provides, so every
public Rust item is documented.
---
Nitpick comments:
In `@rs/moq-native/src/quic.rs`:
- Around line 27-106: Reduce duplication between the `Client` and `Server` QUIC
configuration types by introducing a shared abstraction for the five
per-connection knobs and their serde behavior, while retaining distinct clap
`#[arg]` identifiers and environment-variable prefixes. Use a suitable macro or
shared helper to generate the repeated fields and attributes, and factor the
identical `resolve()` logic into a shared trait or helper used by both
implementations.
🪄 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
Run ID: cbf991df-a299-43d5-9e75-fc593f887aac
📒 Files selected for processing (12)
rs/moq-cli/src/main.rsrs/moq-native/src/client.rsrs/moq-native/src/iroh.rsrs/moq-native/src/lib.rsrs/moq-native/src/noq.rsrs/moq-native/src/quic.rsrs/moq-native/src/quiche.rsrs/moq-native/src/quinn.rsrs/moq-native/src/server.rsrs/moq-native/tests/backend.rsrs/moq-relay/src/config.rsrs/moq-relay/src/main.rs
|
|
||
| use web_transport_noq::noq; | ||
|
|
||
| pub use web_transport_noq; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Document this public re-export.
pub use web_transport_noq; is a public Rust item without rustdoc.
Proposed fix
+/// WebTransport implementation backed by NOQ.
pub use web_transport_noq;As per coding guidelines, “every pub Rust item” must have a doc comment.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pub use web_transport_noq; | |
| /// WebTransport implementation backed by NOQ. | |
| pub use web_transport_noq; |
🤖 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/noq.rs` at line 12, Add a concise rustdoc comment directly
above the public re-export `pub use web_transport_noq;`, describing what the
re-export provides and why consumers may use it.
Source: Coding guidelines
| use std::{net, time}; | ||
| use url::Url; | ||
|
|
||
| pub use web_transport_quinn; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Document this public re-export.
pub use web_transport_quinn; is a public Rust item without rustdoc.
Proposed fix
+/// WebTransport implementation backed by Quinn.
pub use web_transport_quinn;As per coding guidelines, “every pub Rust item” must have a doc comment.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pub use web_transport_quinn; | |
| /// WebTransport implementation backed by Quinn. | |
| pub use web_transport_quinn; |
🤖 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` at line 10, Add a rustdoc comment directly above
the public re-export `pub use web_transport_quinn;`, briefly describing what the
re-export provides, so every public Rust item is documented.
Source: Coding guidelines
Add role-split QUIC transport tuning sections: `quic::Client` (`--client-quic-*`) and `quic::Server` (`--server-quic-*`), each flattened directly onto ClientConfig / ServerConfig. Knobs: max streams, GSO, idle timeout, keep-alive, MTU discovery. The previously-hardcoded idle timeout / keep-alive / MTU discovery are now configurable. Because the args parse straight into the role config the endpoint is built from, there is no separate section to remember to pipe through: `--client-quic-*` lands in `ClientConfig::quic`, `--server-quic-*` in `ServerConfig::quic`, automatically. The split is meaningful (not just cosmetic) because the server owns accept-only knobs the client has no use for. The QUIC preferred address and QUIC-LB connection-ID fields move off ServerConfig's top level into `quic::Server`, so all of a server's QUIC tuning lives in one place. Their CLI flags and env vars are unchanged (`--server-preferred-v4`, `--server-quic-lb-id`, ...); only the TOML path moves under `[server.quic]`. Backend support for disabling GSO is uneven: quinn and noq honor every knob; quiche and iroh cannot force GSO off (it is socket-probed with no public setter), so `--*-quic-gso=false` errors at init on those backends. iroh previously ignored transport config entirely; it now applies max_streams (plus idle timeout and MTU) via its endpoint transport config, which matters because MoQ opens a stream per group and iroh's default of ~100 is far too low. The old `--client-max-streams` / `--server-max-streams` spellings are kept as hidden clap aliases. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
3a642e8 to
06dfe20
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@rs/moq-native/src/iroh.rs`:
- Around line 68-70: Update the GsoUnsupported error message to mention both
supported alternatives, the noq and quinn backends, and replace the inaccurate
--quic-gso=false reference with the actual client/server-specific flags
--client-quic-gso and --server-quic-gso.
🪄 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
Run ID: 614455ae-ac40-45fc-8cfa-9be5f13d5692
📒 Files selected for processing (12)
rs/moq-cli/src/main.rsrs/moq-native/src/client.rsrs/moq-native/src/iroh.rsrs/moq-native/src/lib.rsrs/moq-native/src/noq.rsrs/moq-native/src/quic.rsrs/moq-native/src/quiche.rsrs/moq-native/src/quinn.rsrs/moq-native/src/server.rsrs/moq-native/tests/backend.rsrs/moq-relay/src/config.rsrs/moq-relay/src/main.rs
🚧 Files skipped from review as they are similar to previous changes (10)
- rs/moq-native/tests/backend.rs
- rs/moq-relay/src/main.rs
- rs/moq-relay/src/config.rs
- rs/moq-native/src/client.rs
- rs/moq-native/src/quiche.rs
- rs/moq-native/src/lib.rs
- rs/moq-native/src/noq.rs
- rs/moq-native/src/quic.rs
- rs/moq-native/src/server.rs
- rs/moq-native/src/quinn.rs
|
|
||
| #[error("the iroh backend cannot disable GSO; drop --quic-gso=false or use the quinn backend")] | ||
| GsoUnsupported, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Error message only mentions quinn and uses an imprecise flag name.
The message says "use the quinn backend" but per the quic::Client docs, noq also supports disabling GSO. Additionally, --quic-gso=false doesn't match the actual CLI flags (--client-quic-gso / --server-quic-gso), which could confuse users troubleshooting the error.
📝 Suggested error message fix
- #[error("the iroh backend cannot disable GSO; drop --quic-gso=false or use the quinn backend")]
+ #[error("the iroh backend cannot disable GSO; drop --client-quic-gso=false / --server-quic-gso=false or use the quinn or noq backend")]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #[error("the iroh backend cannot disable GSO; drop --quic-gso=false or use the quinn backend")] | |
| GsoUnsupported, | |
| #[error("the iroh backend cannot disable GSO; drop --client-quic-gso=false / --server-quic-gso=false or use the quinn or noq backend")] | |
| GsoUnsupported, |
🤖 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/iroh.rs` around lines 68 - 70, Update the GsoUnsupported
error message to mention both supported alternatives, the noq and quinn
backends, and replace the inaccurate --quic-gso=false reference with the actual
client/server-specific flags --client-quic-gso and --server-quic-gso.
1 participant
Summary
Adds role-split QUIC transport tuning:
quic::Client(--client-quic-*) andquic::Server(--server-quic-*), each flattened directly ontoClientConfig/ServerConfig. Knobs:max_streams,gso,idle_timeout,keep_alive,mtu_discovery— the last three were previously hardcoded in the quinn/noq backends.Because the args parse straight into the role config the endpoint is built from, there's no separate section to remember to pipe through:
--client-quic-*lands inClientConfig::quicand--server-quic-*inServerConfig::quicautomatically.The split is meaningful, not cosmetic: the server owns accept-only knobs the client has no use for. The QUIC preferred address and QUIC-LB connection-ID fields move off
ServerConfig's top level intoquic::Server, so all of a server's QUIC tuning lives in one place. Their CLI flags/env vars are unchanged (--server-preferred-v4,--server-quic-lb-id, ...); only the TOML path moves under[server.quic].Backend support
TransportConfig::enable_segmentation_offload). quiche and iroh probe GSO from the socket with no public setter, so--*-quic-gso=falsereturns a backendGsoUnsupportederror at init rather than silently ignoring the flag.max_streams(plus idle timeout and MTU) via its endpoint transport config — MoQ opens a stream per group and iroh's default of ~100 is far too low. iroh uses the client-side quic knobs (single P2P endpoint, per-connection knobs are symmetric).Notes for reviewers
--client-max-streams/--server-max-streamsare folded into the new--client-quic-max-streams/--server-quic-max-streams, with the old spellings kept as hidden clap aliases (no breakage).ServerConfig::preferred_v4/v6andquic_lb_id/noncemove toServerConfig::quic(aquic::Server). CLI/env unchanged; TOML moves from[server]to[server.quic].main(moq-native is not in the dev breaking-change list).iroh::EndpointConfig::bindnow takes the resolved quic knobs; callers (relay, cli) pass the client-side section./docordemo/files referenced the old flags, so nothing to sync there.Test plan
cargo testfor moq-native, moq-relay, moq-bench, moq-cli (incl. new tests:--client-quic-*vs--server-quic-*distinctness, server-only knobs, deprecated aliases, TOML round-trip, GSO gating; relay[server.quic]clobber regression updated)clippy --all-targetsclean;RUSTDOCFLAGS=-D warnings cargo docclean; formatted via nixmoq-relay --helpshows--client-quic-*,--server-quic-*, and the server-only--server-preferred-*/--server-quic-lb-*🤖 Generated with Claude Code