moq-net: map MoQ versions to required qmux versions#1513
Open
kixelated wants to merge 10 commits into
Open
Conversation
The MoQ WG decided qmux's draft version is tied to the moq-transport version. moq-transport-18 requires qmux-01; moq-transport-14..17 stay on qmux-00. moq-lite is unconstrained: existing Lite01..Lite04 advertise both for back-compat, and future moq-lite versions should pin to a single qmux version like moq-transport does. Add a local QmuxVersion enum, Version::qmux_versions (the spec mapping), Version::accepts_qmux (server-side validation), and Versions::qmux_alpns (ordered, dedup'd (qmux_version, app_alpn) pairs for building the Sec-WebSocket-Protocol list). Lite arms are listed by variant rather than wildcard so adding a new Lite variant fails to compile until someone makes a deliberate choice. Wiring through moq-native (and the qmux 0.0.8 bump) follows in a separate change once upstream PR moq-dev/web-transport#226 lands and exposes an API for explicit (version, protocol) pairs instead of the default cross-product. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR introduces qmux version support to the MoQ protocol library. A new QmuxVersion enum represents qmux draft versions (00 and 01). Version gains qmux_versions() and accepts_qmux() to enumerate and validate allowed qmux drafts per MoQ version. QMUX_ALPNS and QMUX_ALPN_STRINGS provide ordered advertisement pairs/strings. The native client and listener now advertise and negotiate qmux subprotocols via Sec-WebSocket-Protocol and map the negotiated value to a qmux::Version; the relay pins qmux sessions to the negotiated version. 🚥 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 |
CI's rustfmt collapses single-expr match arms; local fmt didn't flag it. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
qmux 0.1 pins a single Version per Client/Server. To keep advertising
qmux-01.* and qmux-00.* on the same WebSocket connection (which moq-lite
needs for back-compat), moq-native now owns the multi-version
Sec-WebSocket-Protocol composition:
- moq-net exposes Versions::qmux_alpn_strings() that formats the
ordered (QmuxVersion, app_alpn) pairs as "qmux-XX.app".
- moq-native::websocket::connect builds the request itself and hands
the upgraded socket to qmux::ws::Bare with the version inferred from
the negotiated header.
- WebSocketListener does the accept handshake the same way: pick the
first server-supported pair, then wrap with Bare pinned to that
version.
- moq-relay::serve_ws lets axum filter on the same string list, reads
socket.protocol() before adapter conversion, and passes the resolved
qmux::Version through to handle_socket.
Cargo.toml temporarily points qmux at the local qmux-01 worktree and
adds a [patch.crates-io] entry for web-transport-{trait,proto} so the
whole web-transport family shares one crate instance. Drop both once
qmux 0.1.0 ships on crates.io.
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Cargo.toml (1)
55-71:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winRemove machine-local absolute
pathdependencies fromCargo.toml.
Cargo.tomlcurrently depends on unpublished crates via absolute/Users/kixelated/...worktree paths (qmuxat line 55;[patch.crates-io]forweb-transport-traitandweb-transport-protoat lines 70–71). This will fail for anyone outside that filesystem (CI included). Replace with a shareable source: published crates.io versions,git+rev, or a path relative to the repo (or vendored sources) so the build is reproducible.🤖 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 `@Cargo.toml` around lines 55 - 71, The Cargo.toml contains machine-local absolute path dependencies (qmux and the [patch.crates-io] entries for web-transport-trait and web-transport-proto) which are not portable; update the qmux dependency and the web-transport-trait/web-transport-proto patch entries to use a shareable source (replace the absolute path for the qmux dependency with either the published crates.io version, a git = "<repo-url>" + rev = "<commit>" reference, or a relative path inside the repo), and similarly change the [patch.crates-io] entries for web-transport-trait and web-transport-proto to use crates.io versions, git+rev, or relative paths so CI and other developers can build reproducibly.
🤖 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/websocket.rs`:
- Around line 181-183: bind_with_alpns currently only checks for empty alpns but
later accept_socket expects every negotiated ALPN to map to a known
qmux::Version; validate the provided alpns at bind time by iterating over the
alpns Vec<String> in bind_with_alpns and ensuring each entry maps to a supported
qmux::Version (use the same mapping logic as accept_socket or
qmux::Version::from_str/parse equivalent), and return an error (anyhow::Error)
if any unknown/unsupported subprotocol is present so the API fails
deterministically rather than allowing an unsupported ALPN through the
handshake.
---
Outside diff comments:
In `@Cargo.toml`:
- Around line 55-71: The Cargo.toml contains machine-local absolute path
dependencies (qmux and the [patch.crates-io] entries for web-transport-trait and
web-transport-proto) which are not portable; update the qmux dependency and the
web-transport-trait/web-transport-proto patch entries to use a shareable source
(replace the absolute path for the qmux dependency with either the published
crates.io version, a git = "<repo-url>" + rev = "<commit>" reference, or a
relative path inside the repo), and similarly change the [patch.crates-io]
entries for web-transport-trait and web-transport-proto to use crates.io
versions, git+rev, or relative paths so CI and other developers can build
reproducibly.
🪄 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: 091c7ed2-b8bc-4bf2-97b6-108dbf348c1d
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
Cargo.tomlrs/moq-native/src/client.rsrs/moq-native/src/websocket.rsrs/moq-net/src/version.rsrs/moq-relay/src/websocket.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- rs/moq-net/src/version.rs
Collaborator
Author
|
Converting to draft — CI is intentionally red while this is blocked on qmux 0.1.0 publishing.
(Written by Claude) |
A bad subprotocol string would otherwise pass the WebSocket handshake (server happily echoes it back) and only fail later when accept_socket tried to map it to a qmux::Version. Reject up front so the listener errors at startup instead of every connection. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The qmux-01 branch is now pushed to moq-dev/web-transport, so CI can
fetch it. Replaces the absolute /Users/... paths with git deps on the
same branch (qmux 0.1.0 + web-transport-{trait,proto} 0.3.5 / 0.6.0).
Still temporary. Drop the git source and the [patch.crates-io] block
once qmux 0.1.0 publishes to crates.io.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
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 `@Cargo.toml`:
- Line 55: Replace the floating git branch pins with the exact commit rev so the
manifest is immutable: change the qmux dependency entry that currently has
branch = "claude/update-qmux-draft-01-sTTN0" to use rev =
"fd1289acf7e2c5599745059c8ec37c1dba204d18", and do the same for the patch
entries for web-transport-trait and web-transport-proto (replace their branch
fields with the same rev value); after updating Cargo.toml, update the lockfile
by running cargo update -p qmux --precise
fd1289acf7e2c5599745059c8ec37c1dba204d18 (or run cargo generate-lockfile) to
ensure Cargo.lock and the manifest are consistent.
🪄 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: ab0609e6-261b-48f1-8ed9-4c71c8d493dd
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
Cargo.tomlrs/moq-native/src/websocket.rsrs/moq-relay/src/websocket.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- rs/moq-relay/src/websocket.rs
- rs/moq-native/src/websocket.rs
kixelated
commented
May 27, 2026
Per the review on PR #1513: - moq-net: replace `Versions::qmux_alpns()` / `qmux_alpn_strings()` (the per-instance Vec-returning methods) with module-level consts `QMUX_ALPNS: &[(QmuxVersion, &str)]` and `QMUX_ALPN_STRINGS: &[&str]`. The permutation table is small and baked in; no need to recompute. A unit test pins it against the typed list to catch drift. - moq-net: add `Lite05Wip` arm to `Version::qmux_versions` (pinned to qmux-01, opt-in only — excluded from QMUX_ALPNS just like main keeps it out of ALPNS / Versions::all). This is the merge-from-main fix for the CI break introduced by #1518. - moq-native (client): drop the unused `versions` field and switch the websocket helper to take `&[(QmuxVersion, &str)]` (typed pair list). Recover the qmux version after the handshake by index lookup in the pair list rather than re-parsing the prefix. - moq-relay: bundle handle_socket's seven arguments into a `Handler` struct with a `run()` method. axum filters on `QMUX_ALPN_STRINGS.iter().copied()`; the negotiated subprotocol is also resolved by index lookup so the relay no longer has its own prefix-parsing helper. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/client.rs`:
- Line 280: The WebSocket client construction is advertising the full QMUX_ALPNS
instead of honoring the configured Versions subset; change the call sites that
construct moq_net::Client (currently using
moq_net::Client::new().with_versions(config.versions()) and where QMUX_ALPNS is
used) to first derive the offered ALPN/QUIC-version pairs from the configured
Versions (ClientConfig::versions or Versions::... helper) and pass that filtered
list into the WebSocket-path client setup so the same filtered pair list is used
for both normal and WebSocket paths (reuse the filtered list rather than
unconditionally using moq_net::QMUX_ALPNS).
In `@rs/moq-native/src/websocket.rs`:
- Around line 199-205: bind_with_alpns currently accepts arbitrary app strings
and defers validation until handshake, which can panic when
HeaderValue::from_str is later called; validate the formatted ALPN strings at
bind time by iterating over the results of pair_to_alpn (the same vector
assigned to formatted) and attempting HeaderValue::from_str(picked) for each
entry, returning an anyhow::Error if any fail; apply the same pre-validation for
the other bind path that builds ALPNs (the similar block around the 257-263
region) so invalid Sec-WebSocket-Protocol values are rejected during
configuration rather than causing a runtime panic during handshake.
In `@rs/moq-net/src/version.rs`:
- Around line 205-213: The qmux_versions() match currently treats only
Ietf::Draft18 as the "new" QMux01, which will break forward compatibility;
update the match in qmux_versions so that all future IETF drafts default to
QmuxVersion::QMux01 and explicitly list older drafts (Draft14–Draft17) as the
exception mapping to Qmux00. Concretely, change the Ietf arm so older variants
(I::Draft14 | I::Draft15 | I::Draft16 | I::Draft17) return
&[QMuxVersion::QMux00] and use a catch-all Ietf pattern (e.g., I::Draft18 or
I::_ ) that returns &[QMuxVersion::QMux01]; keep the Lite arms unchanged. Ensure
patterns reference qmux_versions and ietf::Version variants (Draft14..Draft18)
so the match remains exhaustive and future drafts inherit QMux01.
🪄 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: 08726d88-b713-46fb-9025-73d1ce243473
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
rs/moq-native/src/client.rsrs/moq-native/src/websocket.rsrs/moq-net/src/version.rsrs/moq-relay/src/websocket.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- rs/moq-relay/src/websocket.rs
| Ok(Self { | ||
| moq: moq_net::Client::new().with_versions(versions.clone()), | ||
| versions, | ||
| moq: moq_net::Client::new().with_versions(config.versions()), |
Contributor
There was a problem hiding this comment.
Don't bypass ClientConfig.version on WebSocket paths.
These call sites now advertise moq_net::QMUX_ALPNS unconditionally. That ignores the configured versions() subset for WebSocket, so --client-version moq-lite-02 still offers qmux-01.moq-lite-04, qmux-01.moqt-18, etc., and the server can negotiate one of them before moq_net::Client gets a chance to enforce the subset. Please derive the offered pair list from the configured Versions and reuse that filtered list here.
Also applies to: 392-393, 423-424, 453-454, 472-473
🤖 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/client.rs` at line 280, The WebSocket client construction
is advertising the full QMUX_ALPNS instead of honoring the configured Versions
subset; change the call sites that construct moq_net::Client (currently using
moq_net::Client::new().with_versions(config.versions()) and where QMUX_ALPNS is
used) to first derive the offered ALPN/QUIC-version pairs from the configured
Versions (ClientConfig::versions or Versions::... helper) and pass that filtered
list into the WebSocket-path client setup so the same filtered pair list is used
for both normal and WebSocket paths (reuse the filtered list rather than
unconditionally using moq_net::QMUX_ALPNS).
Comment on lines
+199
to
+205
| pub async fn bind_with_alpns( | ||
| addr: net::SocketAddr, | ||
| alpns: &'static [(QmuxVersion, &'static str)], | ||
| ) -> anyhow::Result<Self> { | ||
| anyhow::ensure!(!alpns.is_empty(), "no WebSocket subprotocols to accept"); | ||
| let listener = tokio::net::TcpListener::bind(addr).await?; | ||
| let server = qmux::Server::new().with_protocols(alpns); | ||
| Ok(Self { listener, server }) | ||
| let formatted = alpns.iter().map(|(qv, app)| pair_to_alpn(*qv, app)).collect(); |
Contributor
There was a problem hiding this comment.
Reject invalid custom subprotocol strings at bind time.
bind_with_alpns accepts arbitrary app strings, but the handshake path later does HeaderValue::from_str(picked).expect(...). A custom pair that formats to an invalid Sec-WebSocket-Protocol value will panic the listener during handshake instead of failing configuration early.
🛡️ Proposed fix
pub async fn bind_with_alpns(
addr: net::SocketAddr,
alpns: &'static [(QmuxVersion, &'static str)],
) -> anyhow::Result<Self> {
anyhow::ensure!(!alpns.is_empty(), "no WebSocket subprotocols to accept");
- let listener = tokio::net::TcpListener::bind(addr).await?;
- let formatted = alpns.iter().map(|(qv, app)| pair_to_alpn(*qv, app)).collect();
+ let formatted: Vec<String> = alpns.iter().map(|(qv, app)| pair_to_alpn(*qv, app)).collect();
+ for protocol in &formatted {
+ tungstenite::http::HeaderValue::from_str(protocol)
+ .with_context(|| format!("invalid WebSocket subprotocol: {protocol}"))?;
+ }
+ let listener = tokio::net::TcpListener::bind(addr).await?;
Ok(Self {
listener,
pairs: alpns,
formatted: Arc::new(formatted),
})Also applies to: 257-263
🤖 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/websocket.rs` around lines 199 - 205, bind_with_alpns
currently accepts arbitrary app strings and defers validation until handshake,
which can panic when HeaderValue::from_str is later called; validate the
formatted ALPN strings at bind time by iterating over the results of
pair_to_alpn (the same vector assigned to formatted) and attempting
HeaderValue::from_str(picked) for each entry, returning an anyhow::Error if any
fail; apply the same pre-validation for the other bind path that builds ALPNs
(the similar block around the 257-263 region) so invalid Sec-WebSocket-Protocol
values are rejected during configuration rather than causing a runtime panic
during handshake.
Comment on lines
+205
to
+213
| pub fn qmux_versions(&self) -> &'static [QmuxVersion] { | ||
| use ietf::Version as I; | ||
| use lite::Version as L; | ||
| match self { | ||
| Self::Ietf(I::Draft18) => &[QmuxVersion::QMux01], | ||
| Self::Ietf(I::Draft14 | I::Draft15 | I::Draft16 | I::Draft17) => &[QmuxVersion::QMux00], | ||
| Self::Lite(L::Lite01 | L::Lite02 | L::Lite03 | L::Lite04) => &[QmuxVersion::QMux01, QmuxVersion::QMux00], | ||
| Self::Lite(L::Lite05Wip) => &[QmuxVersion::QMux01], | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Default new IETF drafts to qmux-01 here.
This hard-codes Draft18 as the only "new" case, so the next IETF draft will not inherit the newest qmux behavior automatically.
♻️ Proposed fix
match self {
- Self::Ietf(I::Draft18) => &[QmuxVersion::QMux01],
Self::Ietf(I::Draft14 | I::Draft15 | I::Draft16 | I::Draft17) => &[QmuxVersion::QMux00],
+ Self::Ietf(_) => &[QmuxVersion::QMux01],
Self::Lite(L::Lite01 | L::Lite02 | L::Lite03 | L::Lite04) => &[QmuxVersion::QMux01, QmuxVersion::QMux00],
Self::Lite(L::Lite05Wip) => &[QmuxVersion::QMux01],
}As per coding guidelines, "When matching on Version enums in Rust, default to the newest draft behavior so future versions default forward. Explicitly list older versions as the exception case."
🤖 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-net/src/version.rs` around lines 205 - 213, The qmux_versions() match
currently treats only Ietf::Draft18 as the "new" QMux01, which will break
forward compatibility; update the match in qmux_versions so that all future IETF
drafts default to QmuxVersion::QMux01 and explicitly list older drafts
(Draft14–Draft17) as the exception mapping to Qmux00. Concretely, change the
Ietf arm so older variants (I::Draft14 | I::Draft15 | I::Draft16 | I::Draft17)
return &[QMuxVersion::QMux00] and use a catch-all Ietf pattern (e.g., I::Draft18
or I::_ ) that returns &[QMuxVersion::QMux01]; keep the Lite arms unchanged.
Ensure patterns reference qmux_versions and ietf::Version variants
(Draft14..Draft18) so the match remains exhaustive and future drafts inherit
QMux01.
Now that qmux 0.1.0 has shipped, drop the temporary git/path deps:
- Cargo.toml: qmux switches from the local-path/git dep on the qmux-01
branch back to a normal crates.io version pin. The [patch.crates-io]
block for web-transport-{trait,proto} is gone too; ^0.3.4 and ^0.6
now resolve cleanly to the published 0.3.5 / 0.6.0.
- js/net: bump @moq/qmux to ^0.1.0 and update the WebSocket fallback in
connect.ts to the new API. qmux 0.1.0 pins a single QMux version per
Session, so the polyfill now constructs `new Session(url, { version:
"qmux-01", protocols: [...] })` and offers the QMux01-compatible app
protocols (moq-lite-04/03/01-02, moqt-18). Older moq-transport drafts
that require QMux00 reach the relay via native WebTransport instead.
bun.lock will refresh on the next install once npm has fully propagated
@moq/qmux 0.1.0; not committed here because frozen-lockfile would fail.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@moq/qmux@0.1.0 is now live on npm; this picks it up so `bun install --frozen-lockfile` passes again. js/net typechecks (`bun run check`) against the new SessionOptions surface. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
moq-net:QmuxVersionenum,Version::qmux_versions,Version::accepts_qmux, andVersions::qmux_alpns(ordered dedup'd(qmux_version, app_alpn)pairs for building theSec-WebSocket-Protocollist).Why this lives in moq-net
The composition of
(qmux_version, app_alpn)pairs is policy that depends on the moq-transport spec, not on qmux. qmux today (0.0.7) and after moq-dev/web-transport#226 cross-products qmux versions × app protocols internally, which would advertise illegal pairs likeqmux-00.moqt-18orqmux-01.moqt-17. Keeping the mapping here meansmoq-nativecan build exactly the right pairs and feed them through whatever opaque-protocol path qmux exposes (or, if needed, prompt a smallwith_raw_protocolsaddition upstream).What's not in this PR
moq-native's websocket client/server andmoq-relay's sub-protocol list. That's blocked on the qmux 0.0.8 release that ships PR 226 with an API for explicit pairs.qmuxworkspace dep from0.0.7.Test plan
cargo test -p moq-net --lib version::— 7 new tests pinning the table and per-version mappings, plus the unchanged 327 existing tests, all pass.cargo clippy -p moq-net --all-targets -- -D warningsclean.cargo fmt -p moq-net -- --checkclean.cargo build -p moq-native -p moq-relay -p moq-cli— downstream crates still compile against the new surface (it's purely additive).(Written by Claude)