moq-net: negotiate Lite03+ via legacy SETUP when ALPN is unavailable#1307
Draft
kixelated wants to merge 1 commit into
Draft
moq-net: negotiate Lite03+ via legacy SETUP when ALPN is unavailable#1307kixelated wants to merge 1 commit into
kixelated wants to merge 1 commit into
Conversation
Contributor
WalkthroughThe pull request extends WebTransport protocol negotiation across JavaScript and Rust implementations to support Lite versions 03 and 04, in addition to existing versions 01 and 02. A key change introduces conditional handling of the SETUP bootstrap control stream based on version legacy status: versions 01 and 02 receive the control stream, while versions 03 and higher have it explicitly closed before the connection is established. The versions list advertised during negotiation is expanded to include the new Lite drafts alongside IETF Draft 14. Both client and server implementations are updated with corresponding logic, and integration tests are added to verify negotiation without ALPN. 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![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: 2
🧹 Nitpick comments (1)
js/lite/src/connection/connect.ts (1)
182-195: Extract the Draft14 fallback versions list into a shared constant.This list now exists in both
connect()andconnectTransport(). Pulling it into one named constant would make the fallback negotiation policy harder to accidentally drift the next time a Lite draft is added or reordered.♻️ Suggested refactor
+const NEGOTIATED_FALLBACK_VERSIONS = [ + Lite.Version.DRAFT_04, + Lite.Version.DRAFT_03, + Lite.Version.DRAFT_02, + Lite.Version.DRAFT_01, + Ietf.Version.DRAFT_14, +] as const; + const client = new Ietf.ClientSetup({ @@ - : [ - Lite.Version.DRAFT_04, - Lite.Version.DRAFT_03, - Lite.Version.DRAFT_02, - Lite.Version.DRAFT_01, - Ietf.Version.DRAFT_14, - ], + : [...NEGOTIATED_FALLBACK_VERSIONS],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/lite/src/connection/connect.ts` around lines 182 - 195, Extract the Draft14 fallback array into a shared constant (e.g., DRAFT14_FALLBACK_VERSIONS) and use it from both connect() and connectTransport() instead of duplicating the literal list; locate the current inline array used when setupVersion !== DRAFT_16 && !== DRAFT_15 in connect() (the array containing Lite.Version.DRAFT_04..DRAFT_01 and Ietf.Version.DRAFT_14) and the identical array in connectTransport(), move that array to a single exported/locally-scoped constant, and replace both inline occurrences with a reference to the new constant so future draft additions only need to be made in one place.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@js/lite/src/integration.test.ts`:
- Around line 67-76: Add test cases that mirror the existing "no ALPN" tests but
set the protocol argument to Lite.ALPN so the SETUP-based negotiation path that
falls back to "moql" is exercised; specifically, add tests calling
runPublishSubscribeFlow(Lite.ALPN, Lite.Version.DRAFT_03) and
runPublishSubscribeFlow(Lite.ALPN, Lite.Version.DRAFT_04) (with test names
analogous to the existing no-ALPN titles) so the behavior when protocol ===
Lite.ALPN is covered.
In `@rs/moq-lite/src/client.rs`:
- Around line 430-486: Add a parallel test that covers the Lite04 fallback path:
create a new async helper (e.g., run_alpn_lite_fallback_lite04_case) modeled on
run_alpn_lite_fallback_lite03_case but set server.version to
Version::Lite(lite::Version::Lite04), assert the resulting session.version() is
Lite04, and assert the client setup advertisement still lists the NEGOTIATED set
starting with Lite04; then add two tokio::test entries that call this helper
with Some(ALPN_LITE) and None to cover both ALPN and no-ALPN code paths so
Lite04 selection/bootstrapping is exercised.
---
Nitpick comments:
In `@js/lite/src/connection/connect.ts`:
- Around line 182-195: Extract the Draft14 fallback array into a shared constant
(e.g., DRAFT14_FALLBACK_VERSIONS) and use it from both connect() and
connectTransport() instead of duplicating the literal list; locate the current
inline array used when setupVersion !== DRAFT_16 && !== DRAFT_15 in connect()
(the array containing Lite.Version.DRAFT_04..DRAFT_01 and Ietf.Version.DRAFT_14)
and the identical array in connectTransport(), move that array to a single
exported/locally-scoped constant, and replace both inline occurrences with a
reference to the new constant so future draft additions only need to be made in
one place.
🪄 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: 18d17b50-1ebb-4d1c-a29a-a2c087d4b33e
📒 Files selected for processing (7)
js/lite/src/connection/accept.tsjs/lite/src/connection/connect.tsjs/lite/src/integration.test.tsjs/lite/src/lite/version.tsrs/moq-lite/src/client.rsrs/moq-lite/src/server.rsrs/moq-lite/src/version.rs
Comment on lines
+67
to
+76
| // No-ALPN fallback (e.g. Firefox WebTransport, which can't select an ALPN). | ||
| // The SETUP exchange advertises every moq-lite version, so the server can | ||
| // pick the preferred version — including Lite03+ — even without ALPN selection. | ||
| test("integration: lite draft-03 via SETUP fallback (no ALPN)", async () => { | ||
| await runPublishSubscribeFlow("", Lite.Version.DRAFT_03); | ||
| }); | ||
|
|
||
| test("integration: lite draft-04 via SETUP fallback (no ALPN)", async () => { | ||
| await runPublishSubscribeFlow("", Lite.Version.DRAFT_04); | ||
| }); |
Contributor
There was a problem hiding this comment.
Add an explicit moql fallback case here too.
These tests only exercise the empty-protocol fallback. The same negotiation path is also entered when protocol === Lite.ALPN, and that is part of the behavior this PR changes, so a regression there would still go unnoticed.
🧪 Suggested coverage
test("integration: lite draft-03 via SETUP fallback (no ALPN)", async () => {
await runPublishSubscribeFlow("", Lite.Version.DRAFT_03);
});
test("integration: lite draft-04 via SETUP fallback (no ALPN)", async () => {
await runPublishSubscribeFlow("", Lite.Version.DRAFT_04);
});
+
+test("integration: lite draft-03 via SETUP fallback (`moql` ALPN)", async () => {
+ await runPublishSubscribeFlow(Lite.ALPN, Lite.Version.DRAFT_03);
+});
+
+test("integration: lite draft-04 via SETUP fallback (`moql` ALPN)", async () => {
+ await runPublishSubscribeFlow(Lite.ALPN, Lite.Version.DRAFT_04);
+});📝 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.
Suggested change
| // No-ALPN fallback (e.g. Firefox WebTransport, which can't select an ALPN). | |
| // The SETUP exchange advertises every moq-lite version, so the server can | |
| // pick the preferred version — including Lite03+ — even without ALPN selection. | |
| test("integration: lite draft-03 via SETUP fallback (no ALPN)", async () => { | |
| await runPublishSubscribeFlow("", Lite.Version.DRAFT_03); | |
| }); | |
| test("integration: lite draft-04 via SETUP fallback (no ALPN)", async () => { | |
| await runPublishSubscribeFlow("", Lite.Version.DRAFT_04); | |
| }); | |
| // No-ALPN fallback (e.g. Firefox WebTransport, which can't select an ALPN). | |
| // The SETUP exchange advertises every moq-lite version, so the server can | |
| // pick the preferred version — including Lite03+ — even without ALPN selection. | |
| test("integration: lite draft-03 via SETUP fallback (no ALPN)", async () => { | |
| await runPublishSubscribeFlow("", Lite.Version.DRAFT_03); | |
| }); | |
| test("integration: lite draft-04 via SETUP fallback (no ALPN)", async () => { | |
| await runPublishSubscribeFlow("", Lite.Version.DRAFT_04); | |
| }); | |
| test("integration: lite draft-03 via SETUP fallback (`moql` ALPN)", async () => { | |
| await runPublishSubscribeFlow(Lite.ALPN, Lite.Version.DRAFT_03); | |
| }); | |
| test("integration: lite draft-04 via SETUP fallback (`moql` ALPN)", async () => { | |
| await runPublishSubscribeFlow(Lite.ALPN, Lite.Version.DRAFT_04); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@js/lite/src/integration.test.ts` around lines 67 - 76, Add test cases that
mirror the existing "no ALPN" tests but set the protocol argument to Lite.ALPN
so the SETUP-based negotiation path that falls back to "moql" is exercised;
specifically, add tests calling runPublishSubscribeFlow(Lite.ALPN,
Lite.Version.DRAFT_03) and runPublishSubscribeFlow(Lite.ALPN,
Lite.Version.DRAFT_04) (with test names analogous to the existing no-ALPN
titles) so the behavior when protocol === Lite.ALPN is covered.
Comment on lines
+430
to
+486
| async fn run_alpn_lite_fallback_lite03_case(protocol: Option<&'static str>) { | ||
| // Server negotiates Lite03 via the legacy SETUP versions list. | ||
| // No SessionInfo frame is appended — Lite03+ has no session stream protocol. | ||
| let mut server_bytes = Vec::new(); | ||
| let server = setup::Server { | ||
| version: Version::Lite(lite::Version::Lite03).into(), | ||
| parameters: Bytes::new(), | ||
| }; | ||
| server | ||
| .encode(&mut server_bytes, Version::Ietf(ietf::Version::Draft14)) | ||
| .unwrap(); | ||
|
|
||
| let fake = FakeSession::new(protocol, server_bytes); | ||
| let client = Client::new().with_versions( | ||
| [ | ||
| Version::Lite(lite::Version::Lite04), | ||
| Version::Lite(lite::Version::Lite03), | ||
| Version::Lite(lite::Version::Lite02), | ||
| Version::Lite(lite::Version::Lite01), | ||
| Version::Ietf(ietf::Version::Draft14), | ||
| ] | ||
| .into(), | ||
| ); | ||
|
|
||
| let session = client.connect(fake.clone()).await.unwrap(); | ||
| assert_eq!(session.version(), Version::Lite(lite::Version::Lite03)); | ||
|
|
||
| // Client must advertise Lite04, Lite03 (the whole NEGOTIATED set intersection). | ||
| let mut setup_bytes = Bytes::from(fake.control_writes()); | ||
| let setup = setup::Client::decode(&mut setup_bytes, Version::Ietf(ietf::Version::Draft14)).unwrap(); | ||
| let advertised: Vec<Version> = setup.versions.iter().map(|v| Version::try_from(*v).unwrap()).collect(); | ||
| assert_eq!( | ||
| advertised, | ||
| vec![ | ||
| Version::Lite(lite::Version::Lite04), | ||
| Version::Lite(lite::Version::Lite03), | ||
| Version::Lite(lite::Version::Lite02), | ||
| Version::Lite(lite::Version::Lite01), | ||
| Version::Ietf(ietf::Version::Draft14), | ||
| ] | ||
| ); | ||
| } | ||
|
|
||
| #[tokio::test(start_paused = true)] | ||
| async fn alpn_lite_falls_back_to_draft14_and_switches_version_post_setup() { | ||
| run_alpn_lite_fallback_case(Some(ALPN_LITE)).await; | ||
| } | ||
|
|
||
| #[tokio::test(start_paused = true)] | ||
| async fn alpn_lite_fallback_negotiates_lite03() { | ||
| run_alpn_lite_fallback_lite03_case(Some(ALPN_LITE)).await; | ||
| } | ||
|
|
||
| #[tokio::test(start_paused = true)] | ||
| async fn no_alpn_fallback_negotiates_lite03() { | ||
| run_alpn_lite_fallback_lite03_case(None).await; | ||
| } |
Contributor
There was a problem hiding this comment.
Cover the Lite04 fallback selection as well.
NEGOTIATED now prefers Lite04 first, but these new fallback tests only assert the Lite03 path. A regression in selecting or bootstrapping Lite04 via ALPN_LITE or no ALPN would still pass here.
🧪 Suggested extension
- async fn run_alpn_lite_fallback_lite03_case(protocol: Option<&'static str>) {
+ async fn run_alpn_lite_fallback_case(protocol: Option<&'static str>, negotiated: lite::Version) {
// Server negotiates Lite03 via the legacy SETUP versions list.
// No SessionInfo frame is appended — Lite03+ has no session stream protocol.
let mut server_bytes = Vec::new();
let server = setup::Server {
- version: Version::Lite(lite::Version::Lite03).into(),
+ version: Version::Lite(negotiated).into(),
parameters: Bytes::new(),
};
@@
- assert_eq!(session.version(), Version::Lite(lite::Version::Lite03));
+ assert_eq!(session.version(), Version::Lite(negotiated));
@@
#[tokio::test(start_paused = true)]
async fn alpn_lite_fallback_negotiates_lite03() {
- run_alpn_lite_fallback_lite03_case(Some(ALPN_LITE)).await;
+ run_alpn_lite_fallback_case(Some(ALPN_LITE), lite::Version::Lite03).await;
}
#[tokio::test(start_paused = true)]
async fn no_alpn_fallback_negotiates_lite03() {
- run_alpn_lite_fallback_lite03_case(None).await;
+ run_alpn_lite_fallback_case(None, lite::Version::Lite03).await;
+ }
+
+ #[tokio::test(start_paused = true)]
+ async fn alpn_lite_fallback_negotiates_lite04() {
+ run_alpn_lite_fallback_case(Some(ALPN_LITE), lite::Version::Lite04).await;
+ }
+
+ #[tokio::test(start_paused = true)]
+ async fn no_alpn_fallback_negotiates_lite04() {
+ run_alpn_lite_fallback_case(None, lite::Version::Lite04).await;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rs/moq-lite/src/client.rs` around lines 430 - 486, Add a parallel test that
covers the Lite04 fallback path: create a new async helper (e.g.,
run_alpn_lite_fallback_lite04_case) modeled on
run_alpn_lite_fallback_lite03_case but set server.version to
Version::Lite(lite::Version::Lite04), assert the resulting session.version() is
Lite04, and assert the client setup advertisement still lists the NEGOTIATED set
starting with Lite04; then add two tokio::test entries that call this helper
with Some(ALPN_LITE) and None to cover both ALPN and no-ALPN code paths so
Lite04 selection/bootstrapping is exercised.
Firefox's WebTransport doesn't expose an ALPN selection API, so clients there can never pick a dedicated moq-lite ALPN. Previously the fallback SETUP path (bare `moql` ALPN or no ALPN at all) only advertised [Lite02, Lite01, Draft14], stranding those peers on Lite02. Extend the fallback NEGOTIATED set to advertise every shipped moq-lite version (Lite04, Lite03, Lite02, Lite01, Draft14) in the draft-14 SETUP versions list. When the peer selects Lite03+, gracefully close the bootstrap SETUP stream after the exchange and run the session as if it had been ALPN-negotiated (no SessionInfo control messages). Lite05Wip stays out of the fallback set, matching ALPNS: it's work-in-progress and not advertised by default. Mirror the change across the Rust client/server and the TypeScript client/server paths, and add unit + integration tests covering the no-ALPN / `moql` negotiation of Lite03 and Lite04. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
kixelated
force-pushed
the
claude/moq-lite-03-fallback-Ed5FJ
branch
from
1693b80 to
0fc8db6
Compare
kixelated
changed the title
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.
Firefox's WebTransport doesn't expose an ALPN selection API, so clients there can never pick a dedicated moq-lite ALPN (
moq-lite-03/moq-lite-04). Previously the fallback SETUP path (baremoqlALPN, or no ALPN at all) only advertised[Lite02, Lite01, Draft14], stranding those peers on Lite02.Summary
NEGOTIATEDset inrs/moq-net/src/version.rsto advertise every shipped moq-lite version (Lite04, Lite03, Lite02, Lite01, Draft14) in the draft-14 SETUP versions list, so Lite03+ can be negotiated without a dedicated ALPN.Lite05Wipis intentionally left out of the fallback set, matchingALPNS: it's work-in-progress and must not be advertised by default.client.rs) and server (server.rs), and the TypeScript client (js/net/src/connection/connect.ts) and server (js/net/src/connection/accept.ts).Note: this revives #1307 against the restructured tree (
moq-lite→moq-net,js/lite→js/net); the branch was reset onto currentmain.Public API
No public API changes.
NEGOTIATEDispub(crate); this is a behavioral change to SETUP negotiation only, backward/forward compatible with peers running the old fallback set.Test plan
cargo test -p moq-net(newalpn_lite_fallback_negotiates_lite03/no_alpn_fallback_negotiates_lite03unit tests + updated advertised-versions assertion)bun test js/net/src/integration.test.ts(newlite draft-03/04 via SETUP fallback (no ALPN)integration tests, plus alite draft-04ALPN case)cargo clippy -p moq-net --all-targets+cargo fmtbiome check+tsc --noEmit(Written by Claude)