moq-relay: stop downgrading WebSocket clients to moq-lite-02#1523
Merged
Conversation
Two integration tests guard the ALPN/version path we just audited after a report of Android clients negotiating moq-lite-02 over the WebSocket fallback: - broadcast_websocket_uses_newest_version asserts a direct ws:// connect ends up on moq-lite-04 (the newest both sides advertise by default). A Lite02 outcome here would mean qmux's subprotocol negotiation lost the modern per-version ALPNs and only matched bare `moql`, falling through to the legacy SETUP path. - broadcast_race_quic_wins binds the WebSocket TCP listener and the QUIC UDP listener on the same port, disables the WebSocket head start, and asserts the server saw `transport() == "quic"` with the newest version. Catches accidental WebSocket wins when QUIC is reachable. Both assertions run on client and server. A shared NEWEST_LITE constant makes the bump obvious when Versions::all() advances. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
|
Actionable comments posted: 0 |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThis PR updates WebSocket handling to advertise a full qmux × moq-net subprotocol matrix, captures the negotiated ALPN during upgrade and passes it into qmux::ws::Bare via with_alpn, adds the workspace dependency web-transport-trait, and adds end-to-end tests: native broadcast tests asserting NEWEST_LITE negotiation (including a QUIC-vs-WebSocket race) and a relay smoke test that round-trips a frame while verifying moq-lite negotiation. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
`serve_ws` told axum that the only supported subprotocol was `["webtransport"]`. qmux clients offer a richer list (`qmux-00.moq-lite-04`, `qmux-00.moq-lite-03`, `qmux-00.moql`, ..., `webtransport`, `webtransport.moq-lite-04`, ...). axum performs an exact-string match and selected the bare `webtransport`, which carries no version. The `Bare::new(socket).accept()` call below then ignored the negotiated header anyway. Both gaps fed into `moq-net/src/server.rs`'s `Some(ALPN_LITE) | None` arm, which falls back to SETUP-based negotiation with `[Lite02, Lite01, Draft14]` and picks Lite02. That's what the Android reporter was seeing. Fix both halves: - New `supported_subprotocols()` builds the cross product of `qmux::PREFIXES` × `moq_net::ALPNS`, with the bare qmux fallbacks appended last so versioned subprotocols always win. - The upgrade callback captures `socket.protocol()` before the Stream/Sink adapters erase the WebSocket type, then threads it into `qmux::ws::Bare::with_alpn(...)` so qmux can resolve the moq version from the header axum negotiated. Two regression tests guard against this coming back: - `supported_subprotocols_lists_full_matrix` pins the list shape: newest moq ALPN first, every ALPN under every prefix, bare qmux fallbacks last. - `axum_ws_negotiates_newest_moq_alpn` spins up a minimal axum router that mirrors `serve_ws`'s subprotocol wiring, connects with a qmux client offering `moq_net::ALPNS`, and asserts both the client side and the server side observe `moq-lite-04` on the resulting qmux session. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
kixelated
changed the title
Adds rs/moq-relay/tests/smoke.rs, modelled on rs/moq-native/tests/broadcast.rs. It stands up the relay's actual axum + Auth + Cluster stack on a free 127.0.0.1 port with fully public auth, connects a publisher and a subscriber via `ws://`, pushes one frame end-to-end, and asserts both sides observe `moq-lite-04` on the resulting session. This complements the in-`websocket.rs` unit and integration tests: the unit tests cover the protocol matrix and the qmux `Bare` wiring in isolation, while this smoke test exercises the full request path (auth verify → cluster scoping → subprotocol negotiation → qmux SETUP → broadcast announce → group/frame delivery). A regression that downgraded modern clients to Lite02, or broke the relay's publish/subscribe pipeline, would fail this test immediately. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
rs/moq-relay/src/websocket.rs (1)
194-205: ⚡ Quick winDerive the expected ALPN/prefixes from the source-of-truth constants.
These assertions duplicate the current
"moq-lite-04","qmux-00", and"webtransport"literals, so the next ALPN/prefix bump will break the regression test independently of the production logic. Iteratingqmux::PREFIXESandmoq_net::ALPNShere keeps the test aligned withsupported_subprotocols().♻️ Suggested cleanup
#[cfg(test)] mod tests { use super::*; use axum::{Router, extract::WebSocketUpgrade, routing::any}; use std::sync::Mutex; use tokio::sync::oneshot; // Brings `qmux::Session::protocol` and `::closed` into scope. use web_transport_trait::Session as _; + + fn newest_moq_alpn() -> &'static str { + *moq_net::ALPNS.first().expect("missing moq ALPNs") + } #[test] fn supported_subprotocols_lists_full_matrix() { let list = supported_subprotocols(); // Newest moq ALPN under the preferred prefix must come first so axum // picks it whenever the client offers it. - assert_eq!(list.first().map(String::as_str), Some("qmux-00.moq-lite-04")); + let expected_first = format!("{}{}", qmux::PREFIXES[0], newest_moq_alpn()); + assert_eq!(list.first().map(String::as_str), Some(expected_first.as_str())); // Every moq ALPN must appear under every qmux prefix. - for &alpn in moq_net::ALPNS { - assert!(list.contains(&format!("qmux-00.{alpn}")), "missing qmux-00.{alpn}"); - assert!( - list.contains(&format!("webtransport.{alpn}")), - "missing webtransport.{alpn}" - ); + for &prefix in qmux::PREFIXES { + for &alpn in moq_net::ALPNS { + assert!(list.contains(&format!("{prefix}{alpn}")), "missing {prefix}{alpn}"); + } } } ... assert_eq!( session.protocol(), - Some("moq-lite-04"), + Some(newest_moq_alpn()), "client side should see the newest moq ALPN, got {:?}", session.protocol(), ); ... assert_eq!( server_alpn.as_deref(), - Some("moq-lite-04"), + Some(newest_moq_alpn()), "server side should see the newest moq ALPN after Bare::with_alpn", );Also applies to: 282-297
🤖 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-relay/src/websocket.rs` around lines 194 - 205, Replace hard-coded ALPN/prefix literals with values derived from the source-of-truth constants: build the expected first ALPN string using qmux::PREFIXES.first() combined with the newest moq ALPN from moq_net::ALPNS (so the assertion comparing list.first() uses format!("{}.{}", qmux::PREFIXES.first().unwrap(), moq_net::ALPNS.<use newest index>)), and replace the manual "qmux-00" / "webtransport" checks by iterating qmux::PREFIXES and moq_net::ALPNS to assert list.contains(&format!("{}.{alpn}")) for each prefix and alpn (apply the same change for the second occurrence around lines 282–297); use the qmux::PREFIXES and moq_net::ALPNS symbols so the test follows supported_subprotocols() automatically.
🤖 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.
Nitpick comments:
In `@rs/moq-relay/src/websocket.rs`:
- Around line 194-205: Replace hard-coded ALPN/prefix literals with values
derived from the source-of-truth constants: build the expected first ALPN string
using qmux::PREFIXES.first() combined with the newest moq ALPN from
moq_net::ALPNS (so the assertion comparing list.first() uses format!("{}.{}",
qmux::PREFIXES.first().unwrap(), moq_net::ALPNS.<use newest index>)), and
replace the manual "qmux-00" / "webtransport" checks by iterating qmux::PREFIXES
and moq_net::ALPNS to assert list.contains(&format!("{}.{alpn}")) for each
prefix and alpn (apply the same change for the second occurrence around lines
282–297); use the qmux::PREFIXES and moq_net::ALPNS symbols so the test follows
supported_subprotocols() automatically.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 26f4123b-5b5d-413a-92ba-5633e74454b9
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
rs/moq-relay/Cargo.tomlrs/moq-relay/src/websocket.rs
Replace hard-coded "moq-lite-04", "qmux-00", "webtransport" literals in the websocket regression tests with iteration over qmux::PREFIXES, qmux::ALPNS, and moq_net::ALPNS. Adding a newer ALPN or renaming a prefix no longer breaks these tests independently of the production code; they keep tracking whatever supported_subprotocols() advertises. Per CodeRabbit nitpick on #1523. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
Addressed CodeRabbit's nitpick in fad7813: the websocket tests now derive expected ALPNs by iterating (Written by Claude) |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
rs/moq-relay/tests/smoke.rs (1)
15-17: ⚡ Quick winDerive the expected lite version from
moq_net::ALPNSinstead of pinningmoq-lite-04.This is now the one place in the new coverage that will go stale on the next lite-version bump, even if negotiation is still correct.
♻️ Proposed change
-const NEWEST_LITE: &str = "moq-lite-04"; +fn newest_lite_version() -> moq_net::Version { + moq_net::ALPNS + .iter() + .copied() + .find(|alpn| alpn.starts_with("moq-lite")) + .expect("missing moq-lite ALPN") + .parse() + .expect("parse newest lite version") +} @@ - let expected_version: moq_net::Version = NEWEST_LITE.parse().expect("parse version"); + let expected_version = newest_lite_version();Also applies to: 103-103
🤖 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-relay/tests/smoke.rs` around lines 15 - 17, Replace the hardcoded NEWEST_LITE ("moq-lite-04") with a value derived from moq_net::ALPNS so the test picks up the current lite version automatically; update the constant/usage of NEWEST_LITE in tests/smoke.rs to compute the expected lite ALPN from moq_net::ALPNS (e.g., select the appropriate element such as the last/ newest entry) and keep TIMEOUT as-is, ensuring all assertions that referenced NEWEST_LITE now use the derived value.
🤖 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.
Nitpick comments:
In `@rs/moq-relay/tests/smoke.rs`:
- Around line 15-17: Replace the hardcoded NEWEST_LITE ("moq-lite-04") with a
value derived from moq_net::ALPNS so the test picks up the current lite version
automatically; update the constant/usage of NEWEST_LITE in tests/smoke.rs to
compute the expected lite ALPN from moq_net::ALPNS (e.g., select the appropriate
element such as the last/ newest entry) and keep TIMEOUT as-is, ensuring all
assertions that referenced NEWEST_LITE now use the derived value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d57113c8-50cd-4f0c-8d73-de3bc094c68b
📒 Files selected for processing (2)
rs/moq-relay/src/websocket.rsrs/moq-relay/tests/smoke.rs
Replaces the hard-coded NEWEST_LITE = "moq-lite-04" with a helper that picks the first `moq-lite-*` entry out of moq_net::ALPNS. A future lite bump (e.g. lite-05 leaving WIP) won't break this test independently of the production negotiation. Per CodeRabbit nitpick on #1523. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Collaborator
Author
Contributor
|
Actionable comments posted: 0 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes the Android-side report where WebSocket clients were silently negotiated down to
moq-lite-02. Two compounding bugs inrs/moq-relay/src/websocket.rs::serve_ws:ws.protocols(["webtransport"])advertised only the barewebtransportsubprotocol to axum. qmux clients offer the full matrix (qmux-00.moq-lite-04,qmux-00.moq-lite-03,qmux-00.moql, ...,webtransport,webtransport.moq-lite-04, ...). axum exact-matches and picks the barewebtransport, which carries no version.qmux::ws::Bare::new(socket).accept()never forwarded the negotiated header to qmux, so even if axum had picked a versioned subprotocol,qmux::Session::protocol()would still come backNone.Both gaps feed into
rs/moq-net/src/server.rs'sSome(ALPN_LITE) | Nonearm, which falls back to SETUP-based negotiation with[Lite02, Lite01, Draft14]and picks Lite02.Changes
supported_subprotocols()builds the cross product ofqmux::PREFIXES×moq_net::ALPNS, with bare qmux fallbacks appended last so versioned subprotocols always win.socket.protocol()before the Stream/Sink adapters erase the WebSocket type, then threads it intoqmux::ws::Bare::with_alpn(...).rs/moq-relay/src/websocket.rs:supported_subprotocols_lists_full_matrixpins the list shape.axum_ws_negotiates_newest_moq_alpnruns a minimal axum router that mirrorsserve_ws's subprotocol wiring, connects with a qmux client offeringmoq_net::ALPNS, and asserts both sides seemoq-lite-04.rs/moq-relay/tests/smoke.rs(modelled onrs/moq-native/tests/broadcast.rs): stands up the relay's full axum + Auth + Cluster stack on a free port with public auth, connects a publisher and subscriber viaws://, pushes a frame end-to-end, and asserts both sides seemoq-lite-04. Covers the full request path (auth verify → cluster scoping → subprotocol negotiation → broadcast announce → frame delivery).rs/moq-native/tests/broadcast.rs(audit work that confirmed the lower layers were already correct):broadcast_websocket_uses_newest_version— direct ws:// connect must negotiatemoq-lite-04.broadcast_race_quic_wins— with QUIC and WebSocket on the same port and zero head start, QUIC must still win.Test plan
cargo test -p moq-relaycargo test -p moq-relay --test smokecargo test -p moq-native --test broadcastcargo clippy -p moq-relay --all-targets -- -D warningscargo clippy --tests -p moq-native -- -D warningscargo fmt --check(Written by Claude)