Conversation
|
Caution Review failedThe pull request is closed. WalkthroughAdds selectable QUIC backends (Quinn and Quiche) with new feature flags (quinn, quiche, websocket), refactors client and server to dispatch between backends, and introduces a WebSocket fallback/racing path. Introduces quiche and quinn backend modules, a websocket client module, and moves relay WebSocket handling into its own module. Updates workspace dependency declarations and crate Cargo features, adjusts build/CI/install justfile targets, adds RUST_BACKTRACE and RUST_LOG to environment, and adds tooling (cargo-sweep) to dev tooling. 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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: 9
🤖 Fix all issues with AI agents
In @.env:
- Line 2: Remove the surrounding double quotes from the RUST_LOG environment
variable so its value is plain debug instead of the literal "debug"; locate the
RUST_LOG entry in the .env file and change RUST_LOG="debug" to RUST_LOG=debug to
satisfy dotenv-linter and ensure parsers don’t include quote characters in the
value.
In `@Cargo.toml`:
- Line 20: The workspace-wide dependency declaration `moq-native = { version =
"0.12", path = "rs/moq-native", default-features = false }` disables all
backends for crates that consume `moq-native` via `workspace = true`; update
consuming crates (`rs/libmoq`, `rs/moq-relay`, `rs/moq-clock`, `rs/hang`) to
explicitly enable a QUIC backend (e.g., add `moq-native = { workspace = true,
features = ["iroh"] }` or choose `quinn`/`quiche`/`websocket` as appropriate) or
change the root declaration to include sensible default backend features,
ensuring `moq-cli` remains consistent with its `iroh` selection; target the
dependency spec for `moq-native` and the crates listed above when making the
change.
In `@rs/moq-cli/Cargo.toml`:
- Around line 19-23: The default feature list in Cargo.toml currently enables
only "iroh", so moq-cli will not include quinn/quiche transport by default;
update the [features] default array to include "quinn" (and "quiche" if desired)
alongside "iroh" (e.g., default = ["iroh", "quinn"]) or alternatively
remove/adjust the workspace-level default-features = false for the moq-native
dependency so moq-native's own default features (including "quinn") are
preserved; edit the Cargo.toml features block and/or the workspace dependency
settings involving moq-native accordingly (referencing the default = ["iroh"],
iroh = ["moq-native/iroh"], quinn = ["moq-native/quinn"], quiche =
["moq-native/quiche"] symbols).
In `@rs/moq-native/Cargo.toml`:
- Line 52: The rustls-webpki dependency currently forces the aws-lc-rs backend;
change the dependency declaration for rustls-webpki to be optional (remove the
unconditional features=["aws-lc-rs"]) and mirror the conditional pattern used
for rcgen/quinn: add feature flags in [features] that enable rustls-webpki with
either "aws-lc-rs" or "ring" depending on the selected top-level features (e.g.
make a quinn feature enable rustls-webpki + aws-lc-rs by default and make a ring
feature enable rustls-webpki + ring), and ensure the rustls-webpki optional
dependency is referenced from those feature entries so exactly one crypto
backend is selected.
In `@rs/moq-native/src/client.rs`:
- Around line 63-67: ClientConfig's websocket field is conditionally compiled
which causes serde to treat a websocket section in config as an unknown field
when the feature is off; change the field so it is always present in the struct
but ignored during deserialization when the feature is disabled. Concretely,
replace the #[cfg(feature = "websocket")] on the websocket field with a cfg_attr
that applies serde(skip_deserializing, default) when the feature is not enabled
(keep the normal deserialize behavior when feature = "websocket") so
ClientConfig (the struct deriving serde::Deserialize/deny_unknown_fields) can
accept configs containing a websocket section without error; update the
websocket field declaration (the symbol websocket and the ClientConfig struct)
accordingly.
In `@rs/moq-native/src/quiche.rs`:
- Around line 96-110: The quiche backend currently only loads the first cert/key
pair (load_quiche_cert called with config.tls.cert[0]/config.tls.key[0]); update
the logic in this block (which also calls generate_quiche_cert) to iterate over
all config.tls.cert / config.tls.key pairs and load each pair (calling
load_quiche_cert per pair) so SNI/multiple-certs work, or if the underlying
quiche API only accepts a single certificate, detect when multiple pairs are
provided and return an error or log a clear warning explaining the limitation
(refer to generate_quiche_cert and load_quiche_cert and the code path that later
calls into quiche/with_single_cert).
In `@rs/moq-native/src/quinn.rs`:
- Around line 82-87: Update the error text in the alpn match branch so it
correctly lists all supported schemes; in the match on url.scheme() used to set
alpn (the arms for "https", "moql", and "moqt"), change the bail message from
"url scheme must be 'http', 'https', or 'moql'" to include "moqt" as well (e.g.,
mention 'moqt' instead of omitting it) so the error accurately reflects
supported schemes when alpn assignment fails.
- Around line 449-454: The loop over self.info.read().expect("info read lock
poisoned").certs.iter() uses .end_entity_cert() and .try_into() which can panic
on malformed certs; change the logic in the code that builds
webpki::EndEntityCert (inside that loop) to catch failures instead of calling
.expect(): call end_entity_cert() and handle the Option/Result by skipping that
cert (returning None for that iteration) when it is missing or the try_into()
conversion fails, so malformed certs are ignored and the resolver falls back to
the first valid cert as implemented in ResolvesServerCert::resolve; ensure you
still log a debug/warn about the skipped cert for visibility.
In `@rs/moq-relay/src/websocket.rs`:
- Line 118: The match arm handling tungstenite::Message::Frame currently calls
unreachable!() which will abort in release builds; replace that panic with a
safe fallback: log the unexpected frame (use the existing logger e.g.,
process_logger or tracing::warn/error) and skip/continue processing so the
connection stays alive. Locate the match over tungstenite::Message in the
receive/read loop (the arm matching Message::Frame) and change it to emit a
warning with context (e.g., "unexpected Message::Frame received") and then
drop/continue rather than panic; alternatively, if you want strict debug-time
checks, use debug_unreachable (or debug_assert!) instead of unreachable!() so
only debug builds panic. Ensure the change references the
tungstenite::Message::Frame match arm and the surrounding receive loop/handler
function.
🧹 Nitpick comments (9)
rs/moq-relay/src/web.rs (1)
88-97: Consider warning whenconfig.wsis true but thewebsocketfeature is not compiled in.When the
websocketfeature is disabled at compile time, theconfig.ws = truesetting is silently ignored. Users who setMOQ_WEB_WS=truewithout enabling the feature will get no indication that WebSocket support is inactive. A compile-time warning or a runtime log message would help avoid confusion.Proposed approach
// If WebSocket is enabled, add the WebSocket route. #[cfg(feature = "websocket")] let app = match self.config.ws { true => app.route("/{*path}", axum::routing::any(crate::websocket::serve_ws)), false => app, }; + + #[cfg(not(feature = "websocket"))] + if self.config.ws { + tracing::warn!("WebSocket support requested but the 'websocket' feature is not enabled; ignoring"); + } let app = appjustfile (1)
363-366: Workspace-wide feature check may significantly increase CI time.Expanding
cargo hack checkfrom a single package to--workspace --each-featuremultiplies the number of compilations by the total number of features across all workspace crates. This is correct for validating the new feature matrix, but worth monitoring CI duration. If it becomes a bottleneck, consider--exclude-featuresfor expensive/rare combinations or limiting to crates that actually have feature flags.rs/moq-native/src/websocket.rs (3)
8-9:Mutex::lock().unwrap()will panic on a poisoned mutex.The
WEBSOCKET_WONglobal is accessed with.unwrap()in two places (Lines 82 and 133). If a thread panics while holding this lock (unlikely but possible), subsequent calls will panic. Since this is a best-effort optimization cache, consider using.lock().unwrap_or_else(|e| e.into_inner())to recover from poisoning gracefully.Proposed fix
- Some(delay) if !WEBSOCKET_WON.lock().unwrap().contains(&key) => { + Some(delay) if !WEBSOCKET_WON.lock().unwrap_or_else(|e| e.into_inner()).contains(&key) => {- WEBSOCKET_WON.lock().unwrap().insert(key); + WEBSOCKET_WON.lock().unwrap_or_else(|e| e.into_inner()).insert(key);Also applies to: 82-82, 133-133
106-111: Cloningrustls::ClientConfigon every WebSocket connection may be expensive.
Arc::new(tls.clone())performs a deep clone of the TLS config (including certificate stores) on each connection. Consider accepting or storing the config asArc<rustls::ClientConfig>upstream to avoid the repeated clone.
114-126: Large WebSocket message limits (64 MB message, 16 MB frame).These limits are generous. While media streaming may justify them, they also increase susceptibility to memory exhaustion from a malicious server. If clients typically connect to trusted servers this is acceptable; otherwise, consider making these configurable.
rs/moq-native/src/lib.rs (1)
43-49:QuicBackendis always compiled, even when neither backend feature is enabled.The enum variants
QuinnandQuicheare always present regardless of feature flags. This means a user can specify--server-backend quinneven when thequinnfeature is disabled — the error only surfaces at runtime ininit(). This is a reasonable design choice for CLI/serde ergonomics, but consider adding a doc comment on the enum noting that selecting an uncompiled backend will result in a runtime error.rs/moq-native/src/quiche.rs (1)
199-201: Prefer&Pathover&PathBufin function signatures.Idiomatic Rust prefers
&Pathover&PathBuffor function parameters, as&PathBufauto-derefs to&Pathat the call site and&Pathis more general.Proposed fix
-fn load_quiche_cert( - cert_path: &PathBuf, - key_path: &PathBuf, +fn load_quiche_cert( + cert_path: &Path, + key_path: &Path,(Add
use std::path::Path;to imports.)rs/moq-native/src/server.rs (1)
199-258: Significant code duplication between Quinn and Quicheselect!arms.The two
tokio::select!blocks (Lines 202–228 vs 231–257) are nearly identical — only the backend.accept()call and session constructor differ. This is understandable giventokio::select!'s macro constraints and#[cfg]limitations, but it doubles the maintenance surface for any future changes to the accept loop (e.g., adding metrics, graceful shutdown improvements, new backends).If this pattern grows (e.g., a third backend), consider extracting the common post-accept, iroh, futures, and ctrl_c arms into helper methods or a macro.
rs/moq-native/src/client.rs (1)
231-289: Racing logic is correct but theelsearm discards all error context.When both QUIC and WebSocket connections fail, the user sees only
"failed to connect to server"with no indication of why either path failed. The individual errors are logged atwarnlevel (Lines 238–241 for QUIC, and inwebsocket::race_handle), so the information isn't lost — but callers that catch the error programmatically get no useful detail.Consider capturing the last error for the bail message. Also, the Quinn and Quiche arms (Lines 231–260 vs 261–288) are nearly identical — same duplication pattern as in
server.rs'saccept().Optional: Capture error context in the bail
#[cfg(feature = "websocket")] { - let ws_handle = crate::websocket::race_handle(&self.websocket, &self.tls, url); + let ws_handle = crate::websocket::race_handle(&self.websocket, &self.tls, url.clone()); Ok(tokio::select! { Ok(quic) = quic_handle => self.moq.connect(quic).await?, Some(Ok(ws)) = ws_handle => self.moq.connect(ws).await?, - else => anyhow::bail!("failed to connect to server"), + else => anyhow::bail!("failed to connect to {url}: all transport attempts failed (check warn logs for details)"), }) }
| @@ -0,0 +1,2 @@ | |||
| RUST_BACKTRACE=1 | |||
| RUST_LOG="debug" | |||
There was a problem hiding this comment.
Remove unnecessary quotes around the value.
The dotenv-linter flags this: some .env parsers will include the literal " characters in the value, resulting in RUST_LOG being set to "debug" (with quotes) instead of debug. Remove the quotes for portability.
Proposed fix
-RUST_LOG="debug"
+RUST_LOG=debug📝 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.
| RUST_LOG="debug" | |
| RUST_LOG=debug |
🧰 Tools
🪛 dotenv-linter (4.0.0)
[warning] 2-2: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
🤖 Prompt for AI Agents
In @.env at line 2, Remove the surrounding double quotes from the RUST_LOG
environment variable so its value is plain debug instead of the literal "debug";
locate the RUST_LOG entry in the .env file and change RUST_LOG="debug" to
RUST_LOG=debug to satisfy dotenv-linter and ensure parsers don’t include quote
characters in the value.
| moq-lite = { version = "0.13", path = "rs/moq-lite" } | ||
| moq-mux = { version = "0.2", path = "rs/moq-mux" } | ||
| moq-native = { version = "0.12", path = "rs/moq-native" } | ||
| moq-native = { version = "0.12", path = "rs/moq-native", default-features = false } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how moq-native is referenced across workspace crates
echo "=== moq-native dependency references ==="
rg -n 'moq-native' --type toml
echo ""
echo "=== moq-native Cargo.toml ==="
fd -g 'Cargo.toml' -x sh -c 'if grep -q "name = \"moq-native\"" "$1"; then cat "$1"; fi' _ {}
echo ""
echo "=== Features enabled in consuming crates ==="
for file in $(find . -name 'Cargo.toml' -type f); do
if grep -q 'moq-native' "$file"; then
echo "--- $file ---"
sed -n '/\[dependencies\]/,/^\[/p' "$file" | grep -A 3 'moq-native'
fi
done
echo ""
echo "=== Default features sections ==="
for file in $(find . -name 'Cargo.toml' -type f); do
if grep -q '\[features\]' "$file"; then
echo "--- $file ---"
sed -n '/^\[features\]/,/^\[/p' "$file" | head -20
fi
doneRepository: moq-dev/moq
Length of output: 5924
Multiple consuming crates lack QUIC backend enablement and will fail at runtime.
With default-features = false in the root workspace definition, crates using moq-native = { workspace = true } without explicit features will have no QUIC backends available:
rs/libmoq: No backends enabledrs/moq-relay: Onlyaws-lc-rsenabled (crypto feature, not a backend)—needs at least one ofquinn,quiche,iroh, orwebsocketrs/moq-clock: No backends enabledrs/hang: No backends enabled
Only rs/moq-cli properly enables a backend (iroh) through its [features] section. All other crates must explicitly declare required backends in their dependency specification or via feature flags.
🤖 Prompt for AI Agents
In `@Cargo.toml` at line 20, The workspace-wide dependency declaration `moq-native
= { version = "0.12", path = "rs/moq-native", default-features = false }`
disables all backends for crates that consume `moq-native` via `workspace =
true`; update consuming crates (`rs/libmoq`, `rs/moq-relay`, `rs/moq-clock`,
`rs/hang`) to explicitly enable a QUIC backend (e.g., add `moq-native = {
workspace = true, features = ["iroh"] }` or choose `quinn`/`quiche`/`websocket`
as appropriate) or change the root declaration to include sensible default
backend features, ensuring `moq-cli` remains consistent with its `iroh`
selection; target the dependency spec for `moq-native` and the crates listed
above when making the change.
| [features] | ||
| default = ["iroh"] | ||
| iroh = ["moq-native/iroh"] | ||
| # See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html | ||
| quinn = ["moq-native/quinn"] | ||
| quiche = ["moq-native/quiche"] |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check moq-native's default features
echo "=== moq-native features ==="
rg -n -A 15 '\[features\]' rs/moq-native/Cargo.toml
echo -e "\n=== moq-cli Cargo.toml dependencies ==="
rg -n -A 5 'moq-native' rs/moq-cli/Cargo.toml
echo -e "\n=== Full moq-cli Cargo.toml ==="
cat -n rs/moq-cli/Cargo.tomlRepository: moq-dev/moq
Length of output: 2643
🏁 Script executed:
#!/bin/bash
# Check workspace Cargo.toml for moq-native dependency configuration
echo "=== Workspace Cargo.toml ==="
cat -n Cargo.toml | head -100
echo -e "\n=== Searching for moq-native in workspace ==="
rg -n 'moq-native' Cargo.tomlRepository: moq-dev/moq
Length of output: 1376
quinn and quiche are not in default features — moq-cli will have only iroh transport by default.
With default = ["iroh"] and moq-native's default-features = false set at the workspace level, a plain cargo install moq-cli will include only iroh support. The workspace-level setting disables moq-native's own defaults (["quinn", "aws-lc-rs", "websocket"]), so quinn and quiche are unavailable unless explicitly enabled. If standard QUIC connectivity via quinn is expected out of the box, consider adding "quinn" to the default features or revisiting the workspace-level default-features = false configuration for moq-native.
🤖 Prompt for AI Agents
In `@rs/moq-cli/Cargo.toml` around lines 19 - 23, The default feature list in
Cargo.toml currently enables only "iroh", so moq-cli will not include
quinn/quiche transport by default; update the [features] default array to
include "quinn" (and "quiche" if desired) alongside "iroh" (e.g., default =
["iroh", "quinn"]) or alternatively remove/adjust the workspace-level
default-features = false for the moq-native dependency so moq-native's own
default features (including "quinn") are preserved; edit the Cargo.toml features
block and/or the workspace dependency settings involving moq-native accordingly
(referencing the default = ["iroh"], iroh = ["moq-native/iroh"], quinn =
["moq-native/quinn"], quiche = ["moq-native/quiche"] symbols).
| rustls-native-certs = "0.8" | ||
| rustls-pemfile = "2" | ||
| rustls-webpki = { version = "0.103", features = ["aws-lc-rs"] } | ||
| rustls-webpki = { version = "0.103", features = ["aws-lc-rs"], optional = true } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd rs/moq-native && cat -n Cargo.toml | head -70Repository: moq-dev/moq
Length of output: 2847
🌐 Web query:
rustls-webpki 0.103 crate features aws-lc-rs ring
💡 Result:
In rustls-webpki 0.103.x, the relevant Cargo features are: (docs.rs)
ring: enables the ring backend (dep:ring). (docs.rs)aws-lc-rs: enables the aws-lc-rs backend (pulls inaws-lc-rs/aws-lc-sysandaws-lc-rs/prebuilt-nasm). (docs.rs)aws-lc-rs-unstable: enablesaws-lc-rsplusaws-lc-rs/unstable(for algorithms gated as “unstable” in aws-lc-rs). (docs.rs)aws-lc-rs-fips: enablesaws-lc-rsplusaws-lc-rs/fips(FIPS mode via aws-lc-rs). (docs.rs)std(default) andalloc(default viastd): general runtime/heap support, not a crypto provider by themselves. (docs.rs)
Example (pick exactly one crypto backend):
[dependencies]
rustls-webpki = { version = "0.103", default-features = false, features = ["std", "ring"] }
# or:
# rustls-webpki = { version = "0.103", default-features = false, features = ["std", "aws-lc-rs"] }Citations:
- 1: https://docs.rs/crate/rustls-webpki/latest/features
- 2: https://docs.rs/crate/rustls-webpki/latest/features
- 3: https://docs.rs/crate/rustls-webpki/latest/features
- 4: https://docs.rs/crate/rustls-webpki/latest/features
- 5: https://docs.rs/crate/rustls-webpki/latest/features
- 6: https://docs.rs/crate/rustls-webpki/latest/features
rustls-webpki hardcodes aws-lc-rs feature, preventing ring-only builds with quinn.
The rustls-webpki dependency unconditionally enables features = ["aws-lc-rs"] on line 52. Since rustls-webpki is pulled in by the quinn feature (line 21), a user building with quinn + ring (without aws-lc-rs) still gets aws-lc-rs linked through rustls-webpki, defeating the purpose of the ring feature. The rustls-webpki 0.103 crate supports both ring and aws-lc-rs as mutually exclusive crypto backends and requires selecting exactly one.
Align rustls-webpki with the conditional feature pattern already used for rcgen and quinn:
Proposed fix
-rustls-webpki = { version = "0.103", features = ["aws-lc-rs"], optional = true }
+rustls-webpki = { version = "0.103", default-features = false, optional = true }Then in the [features] section:
-aws-lc-rs = ["rustls/aws-lc-rs", "rcgen?/aws_lc_rs", "quinn?/rustls-aws-lc-rs"]
+aws-lc-rs = ["rustls/aws-lc-rs", "rcgen?/aws_lc_rs", "quinn?/rustls-aws-lc-rs", "rustls-webpki?/aws-lc-rs"]
-ring = ["rustls/ring", "rcgen?/ring", "quinn?/rustls-ring"]
+ring = ["rustls/ring", "rcgen?/ring", "quinn?/rustls-ring", "rustls-webpki?/ring"]🤖 Prompt for AI Agents
In `@rs/moq-native/Cargo.toml` at line 52, The rustls-webpki dependency currently
forces the aws-lc-rs backend; change the dependency declaration for
rustls-webpki to be optional (remove the unconditional features=["aws-lc-rs"])
and mirror the conditional pattern used for rcgen/quinn: add feature flags in
[features] that enable rustls-webpki with either "aws-lc-rs" or "ring" depending
on the selected top-level features (e.g. make a quinn feature enable
rustls-webpki + aws-lc-rs by default and make a ring feature enable
rustls-webpki + ring), and ensure the rustls-webpki optional dependency is
referenced from those feature entries so exactly one crypto backend is selected.
| #[cfg(feature = "websocket")] | ||
| #[command(flatten)] | ||
| #[serde(default)] | ||
| pub websocket: ClientWebSocket, | ||
| } |
There was a problem hiding this comment.
The websocket field in ClientConfig is behind #[cfg(feature = "websocket")] but the struct derives serde::Deserialize.
If a config file includes a websocket section when the feature is disabled, deserialization will fail due to #[serde(deny_unknown_fields)] on Line 42. This is technically correct behavior (rejecting unknown config), but could be confusing for users who switch between feature sets with the same config file.
🤖 Prompt for AI Agents
In `@rs/moq-native/src/client.rs` around lines 63 - 67, ClientConfig's websocket
field is conditionally compiled which causes serde to treat a websocket section
in config as an unknown field when the feature is off; change the field so it is
always present in the struct but ignored during deserialization when the feature
is disabled. Concretely, replace the #[cfg(feature = "websocket")] on the
websocket field with a cfg_attr that applies serde(skip_deserializing, default)
when the feature is not enabled (keep the normal deserialize behavior when
feature = "websocket") so ClientConfig (the struct deriving
serde::Deserialize/deny_unknown_fields) can accept configs containing a
websocket section without error; update the websocket field declaration (the
symbol websocket and the ClientConfig struct) accordingly.
| let (chain, key) = if !config.tls.generate.is_empty() { | ||
| generate_quiche_cert(&config.tls.generate)? | ||
| } else { | ||
| anyhow::ensure!( | ||
| !config.tls.cert.is_empty() && !config.tls.key.is_empty(), | ||
| "--tls-cert and --tls-key are required with the quiche backend" | ||
| ); | ||
| anyhow::ensure!( | ||
| config.tls.cert.len() == config.tls.key.len(), | ||
| "must provide matching --tls-cert and --tls-key pairs" | ||
| ); | ||
|
|
||
| // Load certs in PEM format and convert to DER for quiche | ||
| load_quiche_cert(&config.tls.cert[0], &config.tls.key[0])? | ||
| }; |
There was a problem hiding this comment.
Quiche server only loads the first cert/key pair, unlike Quinn which loads all pairs.
When multiple --tls-cert/--tls-key pairs are provided, load_quiche_cert at Line 109 only loads config.tls.cert[0] and config.tls.key[0]. The Quinn backend's ServeCerts::load_certs iterates over all pairs. This means SNI-based cert selection won't work with the quiche backend when multiple certs are configured.
Proposed fix — load all cert/key pairs (or at minimum document the limitation)
If the quiche backend's with_single_cert truly only supports one cert, this should be explicitly documented and the server should warn or error when multiple pairs are supplied:
- anyhow::ensure!(
- config.tls.cert.len() == config.tls.key.len(),
- "must provide matching --tls-cert and --tls-key pairs"
- );
-
- // Load certs in PEM format and convert to DER for quiche
- load_quiche_cert(&config.tls.cert[0], &config.tls.key[0])?
+ anyhow::ensure!(
+ config.tls.cert.len() == config.tls.key.len(),
+ "must provide matching --tls-cert and --tls-key pairs"
+ );
+
+ if config.tls.cert.len() > 1 {
+ tracing::warn!("quiche backend only supports a single TLS certificate; using the first pair");
+ }
+
+ // Load certs in PEM format and convert to DER for quiche
+ load_quiche_cert(&config.tls.cert[0], &config.tls.key[0])?📝 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.
| let (chain, key) = if !config.tls.generate.is_empty() { | |
| generate_quiche_cert(&config.tls.generate)? | |
| } else { | |
| anyhow::ensure!( | |
| !config.tls.cert.is_empty() && !config.tls.key.is_empty(), | |
| "--tls-cert and --tls-key are required with the quiche backend" | |
| ); | |
| anyhow::ensure!( | |
| config.tls.cert.len() == config.tls.key.len(), | |
| "must provide matching --tls-cert and --tls-key pairs" | |
| ); | |
| // Load certs in PEM format and convert to DER for quiche | |
| load_quiche_cert(&config.tls.cert[0], &config.tls.key[0])? | |
| }; | |
| let (chain, key) = if !config.tls.generate.is_empty() { | |
| generate_quiche_cert(&config.tls.generate)? | |
| } else { | |
| anyhow::ensure!( | |
| !config.tls.cert.is_empty() && !config.tls.key.is_empty(), | |
| "--tls-cert and --tls-key are required with the quiche backend" | |
| ); | |
| anyhow::ensure!( | |
| config.tls.cert.len() == config.tls.key.len(), | |
| "must provide matching --tls-cert and --tls-key pairs" | |
| ); | |
| if config.tls.cert.len() > 1 { | |
| tracing::warn!("quiche backend only supports a single TLS certificate; using the first pair"); | |
| } | |
| // Load certs in PEM format and convert to DER for quiche | |
| load_quiche_cert(&config.tls.cert[0], &config.tls.key[0])? | |
| }; |
🤖 Prompt for AI Agents
In `@rs/moq-native/src/quiche.rs` around lines 96 - 110, The quiche backend
currently only loads the first cert/key pair (load_quiche_cert called with
config.tls.cert[0]/config.tls.key[0]); update the logic in this block (which
also calls generate_quiche_cert) to iterate over all config.tls.cert /
config.tls.key pairs and load each pair (calling load_quiche_cert per pair) so
SNI/multiple-certs work, or if the underlying quiche API only accepts a single
certificate, detect when multiple pairs are provided and return an error or log
a clear warning explaining the limitation (refer to generate_quiche_cert and
load_quiche_cert and the code path that later calls into
quiche/with_single_cert).
| let alpn = match url.scheme() { | ||
| "https" => web_transport_quinn::ALPN, | ||
| "moql" => moq_lite::lite::ALPN, | ||
| "moqt" => moq_lite::ietf::ALPN, | ||
| _ => anyhow::bail!("url scheme must be 'http', 'https', or 'moql'"), | ||
| }; |
There was a problem hiding this comment.
Error message at Line 86 omits moqt scheme.
The match handles "moqt" at Line 85, but the error message only lists 'http', 'https', or 'moql'.
Proposed fix
- _ => anyhow::bail!("url scheme must be 'http', 'https', or 'moql'"),
+ _ => anyhow::bail!("url scheme must be 'http', 'https', 'moql', or 'moqt'"),📝 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.
| let alpn = match url.scheme() { | |
| "https" => web_transport_quinn::ALPN, | |
| "moql" => moq_lite::lite::ALPN, | |
| "moqt" => moq_lite::ietf::ALPN, | |
| _ => anyhow::bail!("url scheme must be 'http', 'https', or 'moql'"), | |
| }; | |
| let alpn = match url.scheme() { | |
| "https" => web_transport_quinn::ALPN, | |
| "moql" => moq_lite::lite::ALPN, | |
| "moqt" => moq_lite::ietf::ALPN, | |
| _ => anyhow::bail!("url scheme must be 'http', 'https', 'moql', or 'moqt'"), | |
| }; |
🤖 Prompt for AI Agents
In `@rs/moq-native/src/quinn.rs` around lines 82 - 87, Update the error text in
the alpn match branch so it correctly lists all supported schemes; in the match
on url.scheme() used to set alpn (the arms for "https", "moql", and "moqt"),
change the bail message from "url scheme must be 'http', 'https', or 'moql'" to
include "moqt" as well (e.g., mention 'moqt' instead of omitting it) so the
error accurately reflects supported schemes when alpn assignment fails.
| for ck in self.info.read().expect("info read lock poisoned").certs.iter() { | ||
| let leaf: webpki::EndEntityCert = ck | ||
| .end_entity_cert() | ||
| .expect("missing certificate") | ||
| .try_into() | ||
| .expect("failed to parse certificate"); |
There was a problem hiding this comment.
Multiple .expect() calls can panic on malformed certificates.
end_entity_cert() and the try_into() conversion to webpki::EndEntityCert will panic if a loaded certificate is malformed. Since certificates are loaded from user-provided files (and reloaded on SIGUSR1), a corrupted cert file would crash the server.
Consider returning None (skip the cert) instead of panicking, which aligns with the fallback-to-first-cert behavior already present in ResolvesServerCert::resolve.
Proposed fix
for ck in self.info.read().expect("info read lock poisoned").certs.iter() {
- let leaf: webpki::EndEntityCert = ck
+ let Ok(leaf_der) = ck.end_entity_cert() else {
+ continue;
+ };
+ let Ok(leaf): Result<webpki::EndEntityCert, _> = leaf_der
.end_entity_cert()
- .expect("missing certificate")
.try_into()
- .expect("failed to parse certificate");
+ else {
+ continue;
+ };
if leaf.verify_is_valid_for_subject_name(&dns_name).is_ok() {
return Some(ck.clone());🤖 Prompt for AI Agents
In `@rs/moq-native/src/quinn.rs` around lines 449 - 454, The loop over
self.info.read().expect("info read lock poisoned").certs.iter() uses
.end_entity_cert() and .try_into() which can panic on malformed certs; change
the logic in the code that builds webpki::EndEntityCert (inside that loop) to
catch failures instead of calling .expect(): call end_entity_cert() and handle
the Option/Result by skipping that cert (returning None for that iteration) when
it is missing or the try_into() conversion fails, so malformed certs are ignored
and the resolver falls back to the first valid cert as implemented in
ResolvesServerCert::resolve; ensure you still log a debug/warn about the skipped
cert for visibility.
| tungstenite::Message::Binary(bin) => axum::extract::ws::Message::Binary(bin.into()), | ||
| tungstenite::Message::Ping(ping) => axum::extract::ws::Message::Ping(ping.into()), | ||
| tungstenite::Message::Pong(pong) => axum::extract::ws::Message::Pong(pong.into()), | ||
| tungstenite::Message::Frame(_frame) => unreachable!(), |
There was a problem hiding this comment.
unreachable!() on Message::Frame will panic if ever reached.
While tungstenite documents that Message::Frame should not appear in received messages, a unreachable!() will abort the process in release builds (with panic = "abort" set in the workspace). Consider a safer fallback that logs and skips the message, or at minimum, use debug_unreachable to only panic in debug builds.
Proposed safer alternative
- tungstenite::Message::Frame(_frame) => unreachable!(),
+ tungstenite::Message::Frame(_) => {
+ tracing::warn!("unexpected tungstenite Frame message");
+ return Box::pin(async { Err(tungstenite::Error::ConnectionClosed) });
+ }🤖 Prompt for AI Agents
In `@rs/moq-relay/src/websocket.rs` at line 118, The match arm handling
tungstenite::Message::Frame currently calls unreachable!() which will abort in
release builds; replace that panic with a safe fallback: log the unexpected
frame (use the existing logger e.g., process_logger or tracing::warn/error) and
skip/continue processing so the connection stays alive. Locate the match over
tungstenite::Message in the receive/read loop (the arm matching Message::Frame)
and change it to emit a warning with context (e.g., "unexpected Message::Frame
received") and then drop/continue rather than panic; alternatively, if you want
strict debug-time checks, use debug_unreachable (or debug_assert!) instead of
unreachable!() so only debug builds panic. Ensure the change references the
tungstenite::Message::Frame match arm and the surrounding receive loop/handler
function.
1 participant
No description provided.