From 54034fe66bce3ddba0802695a74c29b92f6ba43d Mon Sep 17 00:00:00 2001 From: Players Engineer Date: Thu, 8 Oct 2026 18:29:13 +0000 Subject: [PATCH 1/5] test(net): pin repair-band ordering across the IETF priority boundary (BLO-41724) MoQT SUBSCRIBER_PRIORITY is lower-first; the moq-net model is higher-first. Both bindings already convert at every crossing through one named function (ietf/priority.rs from_wire/to_wire, ietf/priority.ts fromWire/toWire, both from upstream 72703bb0d), so no conversion is added here -- a second one would restore the very inversion it looks like it fixes. What was missing is an assertion on the ordering the FEC scheme depends on. The existing tests pin the endpoints (0 <-> 255) and the round trip, but nothing pinned that draft-ramadan-moq-fec section 10's bands survive the boundary: Source Media 64..=191 and AL-FEC repair 192..=255 ascending by repair layer must come out with source preempting repair and layer 0 preempting layer 1. Includes the adjacent-boundary case (wire 191 vs 192), which is the tightest: it proves the two bands do not overlap after conversion. Verified by mutation -- replacing from_wire with the identity (the copy-instead-of-map bug) turns the new test red, and makes source=128, repairL0=240, repairL1=241, inverting both orderings at once. Co-Authored-By: Claude --- js/net/src/ietf/priority.test.ts | 22 ++++++++++++++++++++++ rs/moq-net/src/ietf/priority.rs | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 54 insertions(+) diff --git a/js/net/src/ietf/priority.test.ts b/js/net/src/ietf/priority.test.ts index b04da83354e..9fcb370eaf2 100644 --- a/js/net/src/ietf/priority.test.ts +++ b/js/net/src/ietf/priority.test.ts @@ -22,3 +22,25 @@ test("subscriber priority round trips", () => { test("an unset track priority is the draft's usual publisher priority", () => { expect(toWire(infoDefaults().priority)).toBe(128); }); + +// draft-ramadan-moq-fec section 10 bands, on the IETF wire's lower-first scale: +// Source Media is 64..=191 and AL-FEC repair is 192..=255, ascending by repair +// layer. The model ranks higher-first, so a repair priority that is COPIED here +// rather than converted inverts both orderings at once: repair preempts the +// source it repairs, and the highest repair layer outranks layer 0. +test("converted repair bands yield to source and order by layer", () => { + // Both endpoints of the repair band. + expect(fromWire(192)).toBe(63); + expect(fromWire(255)).toBe(0); + // Both endpoints of the source media band. + expect(fromWire(64)).toBe(191); + expect(fromWire(191)).toBe(64); + + // The adjacent boundary is the tightest case: the LEAST urgent source must + // still preempt the MOST urgent repair, or the bands overlap. + expect(fromWire(191)).toBeGreaterThan(fromWire(192)); + + // Repair layers ascend on the wire, so they must descend in the model. + expect(fromWire(240)).toBeGreaterThan(fromWire(241)); + expect(fromWire(128)).toBeGreaterThan(fromWire(240)); +}); diff --git a/rs/moq-net/src/ietf/priority.rs b/rs/moq-net/src/ietf/priority.rs index abc97c9ac00..5f73605100e 100644 --- a/rs/moq-net/src/ietf/priority.rs +++ b/rs/moq-net/src/ietf/priority.rs @@ -27,4 +27,36 @@ mod tests { fn priority_round_trips() { assert!((0u8..=u8::MAX).all(|priority| from_wire(to_wire(priority)) == priority)); } + + /// draft-ramadan-moq-fec section 10 bands, on the IETF wire's lower-first + /// scale: Source Media is 64..=191 and AL-FEC repair is 192..=255, + /// ascending by repair layer. The model ranks higher-first, so a repair + /// priority that is COPIED here rather than converted inverts both + /// orderings at once: repair preempts the source it repairs, and the + /// highest repair layer outranks layer 0. + #[test] + fn converted_repair_bands_yield_to_source_and_order_by_layer() { + // Both endpoints of the repair band. + assert_eq!(from_wire(192), 63); + assert_eq!(from_wire(255), 0); + // Both endpoints of the source media band. + assert_eq!(from_wire(64), 191); + assert_eq!(from_wire(191), 64); + + // The adjacent boundary is the tightest case: the LEAST urgent source + // must still preempt the MOST urgent repair, or the bands overlap. + assert!( + from_wire(191) > from_wire(192), + "the least urgent source media must preempt the most urgent repair" + ); + + // Repair layers ascend on the wire, so they must descend in the model. + let layer_0 = from_wire(240); + let layer_1 = from_wire(241); + assert!(layer_0 > layer_1, "repair layer 0 must preempt layer 1"); + assert!( + from_wire(128) > layer_0, + "a source track must preempt the repair that protects it" + ); + } } From 933cff19776c223ccd5e9e237ebc82a732b43312 Mon Sep 17 00:00:00 2001 From: Players Engineer Date: Fri, 9 Oct 2026 04:34:34 +0000 Subject: [PATCH 2/5] test(net): assert the repair bands' send order on the transport, not just the arithmetic (BLO-41724) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses Ally's two Important findings and Suggestion 1 on #3. Important 1 — the premise "nothing pinned the ordering" was false. `ietf_priority_is_lower_first` / "IETF subscriber priority is lower first" already assert strict monotonicity across the whole domain, so every ordering claim in the new tests is a corollary of it and `from_wire(255) == 0` was an exact duplicate of `from_wire(u8::MAX) == 0` in the same file. Dropped that duplicate in both bindings and corrected the doc comments to say what the new tests actually add: which concrete model values section 10's band endpoints land on, and that the two bands do not overlap after conversion. Suggestion 1 — reuse the existing `group_priority_test` harness instead of asserting the conversion arithmetic alone. `repair_group_streams_yield_to_ source_on_the_transport` serves three groups over `SinkSession` at `from_wire(191)`, `from_wire(240)` and `from_wire(241)` — the least urgent source against the two most urgent repair layers — and asserts the send orders the transport was handed come back `[64, 15, 14]`. Copying the wire values instead of converting them yields `[191, 240, 241]`, inverting both orderings; that is the mutation proof, run and reverted. `group_stream_preserves_model_priority` now shares the extracted `serve_group_at` helper; its assertion is unchanged. Co-Authored-By: Claude Opus 5 (1M context) --- js/net/src/ietf/priority.test.ts | 10 ++++++-- rs/moq-net/src/ietf/priority.rs | 10 ++++++-- rs/moq-net/src/ietf/publisher.rs | 44 +++++++++++++++++++++++++++----- 3 files changed, 53 insertions(+), 11 deletions(-) diff --git a/js/net/src/ietf/priority.test.ts b/js/net/src/ietf/priority.test.ts index 9fcb370eaf2..5a89854d60f 100644 --- a/js/net/src/ietf/priority.test.ts +++ b/js/net/src/ietf/priority.test.ts @@ -28,10 +28,16 @@ test("an unset track priority is the draft's usual publisher priority", () => { // layer. The model ranks higher-first, so a repair priority that is COPIED here // rather than converted inverts both orderings at once: repair preempts the // source it repairs, and the highest repair layer outranks layer 0. +// +// "IETF subscriber priority is lower first" already pins the orderings +// themselves, over the whole domain. What it cannot carry is which concrete +// model values section 10's band endpoints land on, or that those two bands do +// not overlap after conversion -- so this test goes red with a named reason +// where the generic one goes red with an arithmetic one. test("converted repair bands yield to source and order by layer", () => { - // Both endpoints of the repair band. + // The repair band's most urgent value. Its least urgent, fromWire(255), is + // the same fact as fromWire(0xff) above and is not restated. expect(fromWire(192)).toBe(63); - expect(fromWire(255)).toBe(0); // Both endpoints of the source media band. expect(fromWire(64)).toBe(191); expect(fromWire(191)).toBe(64); diff --git a/rs/moq-net/src/ietf/priority.rs b/rs/moq-net/src/ietf/priority.rs index 5f73605100e..dce90ed7244 100644 --- a/rs/moq-net/src/ietf/priority.rs +++ b/rs/moq-net/src/ietf/priority.rs @@ -34,11 +34,17 @@ mod tests { /// priority that is COPIED here rather than converted inverts both /// orderings at once: repair preempts the source it repairs, and the /// highest repair layer outranks layer 0. + /// + /// `ietf_priority_is_lower_first` already pins the orderings themselves, + /// over the whole domain. What it cannot carry is which concrete model + /// values section 10's band endpoints land on, or that those two bands do + /// not overlap after conversion -- so this test goes red with a named + /// reason where the generic one goes red with an arithmetic one. #[test] fn converted_repair_bands_yield_to_source_and_order_by_layer() { - // Both endpoints of the repair band. + // The repair band's most urgent value. Its least urgent, from_wire(255), + // is the same fact as from_wire(u8::MAX) above and is not restated. assert_eq!(from_wire(192), 63); - assert_eq!(from_wire(255), 0); // Both endpoints of the source media band. assert_eq!(from_wire(64), 191); assert_eq!(from_wire(191), 64); diff --git a/rs/moq-net/src/ietf/publisher.rs b/rs/moq-net/src/ietf/publisher.rs index 694ea9f9235..b2b450a5407 100644 --- a/rs/moq-net/src/ietf/publisher.rs +++ b/rs/moq-net/src/ietf/publisher.rs @@ -3087,6 +3087,42 @@ mod group_priority_test { #[moq_net_sim::test] async fn group_stream_preserves_model_priority() { let log = crate::lite::test_transport::Log::default(); + serve_group_at(&log, 200).await; + + assert_eq!( + log.priorities(), + vec![200], + "model priority must pass through unchanged" + ); + } + + /// draft-ramadan-moq-fec section 10 puts Source Media at wire 64..=191 and AL-FEC + /// repair at 192..=255, ascending by repair layer, on the IETF wire's lower-first + /// scale. The transport sends the HIGHEST value first, so those bands only keep + /// their meaning if `from_wire` converts them. This serves the three groups that + /// compete -- the LEAST urgent source against the two MOST urgent repair layers, + /// the tightest case the bands allow -- and reads back the send orders the + /// transport was actually handed, rather than asserting the arithmetic alone. + #[moq_net_sim::test] + async fn repair_group_streams_yield_to_source_on_the_transport() { + let log = crate::lite::test_transport::Log::default(); + serve_group_at(&log, priority::from_wire(191)).await; + serve_group_at(&log, priority::from_wire(240)).await; + serve_group_at(&log, priority::from_wire(241)).await; + + // Higher is transmitted first, so this must descend: source, then repair + // layer 0, then layer 1. Copying the wire values instead of converting + // them yields [191, 240, 241] and inverts the whole order. + assert_eq!( + log.priorities(), + vec![64, 15, 14], + "source must outrank repair, and repair layer 0 must outrank layer 1" + ); + } + + /// Serve one group at model `priority` onto `log`, so a caller can serve several and + /// read back the send orders the transport was given, in call order. + async fn serve_group_at(log: &crate::lite::test_transport::Log, priority: u8) { let session = SinkSession::new(log.clone()); let track = track::Producer::new(std::sync::Arc::new(crate::broadcast::Info::default()), "test", None); @@ -3108,19 +3144,13 @@ mod group_priority_test { let mut serve = GroupServe::new( session, msg, - 200, + priority, consumer, Some(Timescale::default()), Version::Draft14, GroupSlice::default(), ); kio::wait(|waiter| serve.poll_serve(waiter)).await.unwrap(); - - assert_eq!( - log.priorities(), - vec![200], - "model priority must pass through unchanged" - ); } /// The publisher's own ranking of its tracks (`track::Info::priority`) is what a relay From 9ddfc0f46d9aef240fd05019d7a86d71e1427c64 Mon Sep 17 00:00:00 2001 From: Players Engineer Date: Fri, 9 Oct 2026 07:10:54 +0000 Subject: [PATCH 3/5] test(net): pin the production call site that converts a subscriber priority (BLO-41724) repair_group_streams_yield_to_source_on_the_transport converts in its own test body and hands the result to GroupServe::new, so it enters the chain one hop after the only link that can regress. Replacing serving_subscription's `from_wire(subscriber_priority)` with a bare `subscriber_priority` -- the copy the from_wire doc names as the hazard -- leaves it green; only mutating from_wire's own body turns it red. Add the assertion at the production entry instead. Verified by applying that exact mutation: the new test fails (left: 191, right: 64) while the transport test still passes, which is the gap it closes. Nothing in the file asserted a converted subscription priority before this: all 11 test SUBSCRIBEs use subscriber_priority 128 and its converted value 127 appears nowhere. Two crossings stay unpinned and are now named in the doc rather than implied by a broader claim: the SUBSCRIBE_UPDATE priority change and run_fetch_stream's own conversion. Both need an accepted request on a scripted peer; the refused-fetch harness returns before set_priority. Also narrow the transport test's doc to what it proves, and record that group_stream_preserves_model_priority's vec![200] is what holds set_priority to one call per serve -- which is what makes vec![64, 15, 14] a three-element assertion rather than one passing by coincidence. Co-Authored-By: Claude Opus 5 (1M context) --- rs/moq-net/src/ietf/publisher.rs | 40 +++++++++++++++++++++++++++++--- 1 file changed, 37 insertions(+), 3 deletions(-) diff --git a/rs/moq-net/src/ietf/publisher.rs b/rs/moq-net/src/ietf/publisher.rs index b2b450a5407..dde9c4e0968 100644 --- a/rs/moq-net/src/ietf/publisher.rs +++ b/rs/moq-net/src/ietf/publisher.rs @@ -3096,13 +3096,40 @@ mod group_priority_test { ); } + /// The production entry for a SUBSCRIBE's priority, and the one link + /// `repair_group_streams_yield_to_source_on_the_transport` cannot see: that test + /// converts in its own body, so `priority: subscriber_priority` here -- the copy the + /// `from_wire` doc names as the hazard -- leaves it and every other test in this + /// module green while inverting repair against source on every subscribe we serve. + /// + /// Two exact values rather than an ordering: the copy still orders 191 against 192 + /// correctly in the wrong direction's own terms, so only the magnitudes discriminate. + /// They are the adjacent band boundary, the tightest case section 10 allows. + /// + /// This covers the SUBSCRIBE path (`:639`) and the joining FETCH's subscribe + /// (`:1419`). The two crossings that do not route through here -- the + /// SUBSCRIBE_UPDATE priority change and `run_fetch_stream`'s own conversion -- stay + /// unpinned; reaching either needs an accepted request on a scripted peer, which is + /// more harness than a one-call conversion earns. + #[test] + fn serving_subscription_converts_the_subscriber_priority() { + // Least urgent source media, which must stay above ... + assert_eq!(serving_subscription(191).priority, 64); + // ... the most urgent repair. + assert_eq!(serving_subscription(192).priority, 63); + } + /// draft-ramadan-moq-fec section 10 puts Source Media at wire 64..=191 and AL-FEC /// repair at 192..=255, ascending by repair layer, on the IETF wire's lower-first /// scale. The transport sends the HIGHEST value first, so those bands only keep - /// their meaning if `from_wire` converts them. This serves the three groups that - /// compete -- the LEAST urgent source against the two MOST urgent repair layers, - /// the tightest case the bands allow -- and reads back the send orders the + /// their meaning if `from_wire` converts them. This serves the three groups whose + /// bands adjoin -- the LEAST urgent source against the two MOST urgent repair + /// layers, the tightest case the bands allow -- and reads back the send orders the /// transport was actually handed, rather than asserting the arithmetic alone. + /// + /// It enters the chain already converted, so it pins the bands against `set_priority` + /// and nothing upstream of `GroupServe`; + /// `serving_subscription_converts_the_subscriber_priority` pins the call site. #[moq_net_sim::test] async fn repair_group_streams_yield_to_source_on_the_transport() { let log = crate::lite::test_transport::Log::default(); @@ -3122,6 +3149,13 @@ mod group_priority_test { /// Serve one group at model `priority` onto `log`, so a caller can serve several and /// read back the send orders the transport was given, in call order. + /// + /// Each call must contribute exactly one entry, or a multi-call assertion reads a + /// vec whose length it did not choose: `test_transport`'s `set_priority` pushes + /// unconditionally, so a second call per serve would lengthen it silently. + /// `group_stream_preserves_model_priority`'s `vec![200]` is what holds that, which + /// makes it load-bearing for its siblings -- relaxing it to a `contains` or a + /// first-element check would quietly weaken them too. async fn serve_group_at(log: &crate::lite::test_transport::Log, priority: u8) { let session = SinkSession::new(log.clone()); From 590eba28267e18d31b68c456e379be02410392dd Mon Sep 17 00:00:00 2001 From: Players Engineer Date: Fri, 9 Oct 2026 13:19:36 +0000 Subject: [PATCH 4/5] docs(net): correct the false reason for preferring exact values (BLO-41724) The doc comment on serving_subscription_converts_the_subscriber_priority claimed the band-pair ordering form could not discriminate a copied priority: "the copy still orders 191 against 192 correctly in the wrong direction's own terms, so only the magnitudes discriminate." That is false. Under `priority: subscriber_priority` the field holds 191 and 192 on the wire's lower-first scale while being consumed higher-first, so `assert!(serving_subscription(191).priority > serving_subscription(192).priority)` evaluates `191 > 192` -> false -> red. Measured, not reasoned: a temporary probe carrying exactly that assertion fails under the copy mutation and passes on the correct code. The clause is load-bearing downstream -- BLO-42129 writes assertions for the remaining crossings, and a maintainer taking it at face value would rule out the ordering form there on a premise that does not hold. Replaced with the reason that is actually true (the literals are not computed from `from_wire`, so the assertion cannot go tautological), keeping the exact-value form as the stronger of two working choices rather than the only one. Also drops the two bare line-number references, the only such references in this 7702-line file, in favour of the semantic naming already in the same sentence; and records that the unpinned crossings -- `subscriber.rs` and both JS sites -- do route through `from_wire`, so what is missing is assertions and not the conversion. Doc comments only; no test or production logic changes. moq-net lib suite 1443 passed, 0 failed, 4 ignored. cargo fmt clean. Co-Authored-By: Claude Opus 5 (1M context) --- rs/moq-net/src/ietf/publisher.rs | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/rs/moq-net/src/ietf/publisher.rs b/rs/moq-net/src/ietf/publisher.rs index dde9c4e0968..0988cae87d0 100644 --- a/rs/moq-net/src/ietf/publisher.rs +++ b/rs/moq-net/src/ietf/publisher.rs @@ -3102,15 +3102,20 @@ mod group_priority_test { /// `from_wire` doc names as the hazard -- leaves it and every other test in this /// module green while inverting repair against source on every subscribe we serve. /// - /// Two exact values rather than an ordering: the copy still orders 191 against 192 - /// correctly in the wrong direction's own terms, so only the magnitudes discriminate. - /// They are the adjacent band boundary, the tightest case section 10 allows. + /// Exact values rather than an ordering: the literals are not computed from + /// `from_wire`, so the assertion cannot go tautological the way + /// `assert_eq!(serving_subscription(191).priority, from_wire(191))` would. They are + /// the adjacent band boundary, the tightest case section 10 allows. (The ordering + /// form would discriminate too -- under the copy it reads `191 > 192`, which is + /// false -- so this is a choice of the stronger assertion, not the only one.) /// - /// This covers the SUBSCRIBE path (`:639`) and the joining FETCH's subscribe - /// (`:1419`). The two crossings that do not route through here -- the - /// SUBSCRIBE_UPDATE priority change and `run_fetch_stream`'s own conversion -- stay - /// unpinned; reaching either needs an accepted request on a scripted peer, which is - /// more harness than a one-call conversion earns. + /// This covers the SUBSCRIBE path and the joining FETCH's subscribe. The other + /// crossings in this file -- the SUBSCRIBE_UPDATE priority change and + /// `run_fetch_stream`'s own conversion -- stay unpinned; reaching either needs an + /// accepted request on a scripted peer, which is more harness than a one-call + /// conversion earns. `subscriber.rs` and both JS crossings are unpinned too. Every + /// one of them does route through `from_wire`; it is the assertions that are + /// missing, not the conversion. BLO-42129 carries the census. #[test] fn serving_subscription_converts_the_subscriber_priority() { // Least urgent source media, which must stay above ... From 79f0dc3b5e296ba25fac57c73038fa29c93d1350 Mon Sep 17 00:00:00 2001 From: Players Engineer Date: Fri, 9 Oct 2026 21:27:36 +0000 Subject: [PATCH 5/5] docs(net): count all seven crossings and name all three JS ones (BLO-41724) The census in the test's doc comment said "`subscriber.rs` and both JS crossings", which drops `js/net/src/ietf/publisher.ts:484` -- the SUBSCRIBE_UPDATE conversion, and the exact JS analogue of the Rust `publisher.rs:893` the comment defers in the sentence above it. BLO-42129 is scoped off this sentence, so the omission turned a deliberate deferral into an invisible one. Verified by census at this head: inbound crossings are `publisher.rs:32`/`:893`/`:1293` and `subscriber.rs:2034` in Rust, `publisher.ts:355`/`:484` and `subscriber.ts:810` in JS -- seven, one pinned. The comment now states both totals and names `fromWire` beside `from_wire`, since the sentence spans both bindings. Also records why the bindings are four against three rather than paired: `js/net/src/ietf/publisher.ts:1386` refuses FETCH outright, so no JS path reads a FETCH's subscriber priority. The tautology clause gains the qualifier it needed. "Cannot go tautological" is true only against a mutation of `from_wire`'s own body; against the call-site copy the test exists for, the `from_wire`-computed form reads `191 == 64` and goes red like every other form. The paragraph discussed both mutation classes without distinguishing them, which is its whole job. Comment-only. `cargo fmt -p moq-net --check` clean; `ietf::` 511 passed, 0 failed, 1 ignored. Co-Authored-By: Claude Opus 5 (1M context) --- rs/moq-net/src/ietf/publisher.rs | 22 +++++++++++++++------- 1 file changed, 15 insertions(+), 7 deletions(-) diff --git a/rs/moq-net/src/ietf/publisher.rs b/rs/moq-net/src/ietf/publisher.rs index 0988cae87d0..4c938891e84 100644 --- a/rs/moq-net/src/ietf/publisher.rs +++ b/rs/moq-net/src/ietf/publisher.rs @@ -3103,19 +3103,27 @@ mod group_priority_test { /// module green while inverting repair against source on every subscribe we serve. /// /// Exact values rather than an ordering: the literals are not computed from - /// `from_wire`, so the assertion cannot go tautological the way + /// `from_wire`, so the assertion cannot go tautological under a mutation of + /// `from_wire` itself, the way /// `assert_eq!(serving_subscription(191).priority, from_wire(191))` would. They are - /// the adjacent band boundary, the tightest case section 10 allows. (The ordering - /// form would discriminate too -- under the copy it reads `191 > 192`, which is - /// false -- so this is a choice of the stronger assertion, not the only one.) + /// the adjacent band boundary, the tightest case section 10 allows. Against the + /// other mutation class -- the call-site copy this test exists for -- every form + /// discriminates: the ordering reads `191 > 192`, and even the `from_wire`-computed + /// form reads `191 == 64`. So the literals are the choice that is strong against + /// both classes, not the only choice that works against either. /// /// This covers the SUBSCRIBE path and the joining FETCH's subscribe. The other /// crossings in this file -- the SUBSCRIBE_UPDATE priority change and /// `run_fetch_stream`'s own conversion -- stay unpinned; reaching either needs an /// accepted request on a scripted peer, which is more harness than a one-call - /// conversion earns. `subscriber.rs` and both JS crossings are unpinned too. Every - /// one of them does route through `from_wire`; it is the assertions that are - /// missing, not the conversion. BLO-42129 carries the census. + /// conversion earns. `subscriber.rs`'s SUBSCRIBE_OK conversion and all three JS + /// crossings -- `publisher.ts`'s SUBSCRIBE and its SUBSCRIBE_UPDATE, the mirror of + /// the Rust one just named, and `subscriber.ts`'s SUBSCRIBE_OK -- are unpinned too. + /// That is seven crossings, six of them unpinned. The bindings are four against + /// three rather than paired because JS `runFetch` refuses FETCH outright, so no JS + /// path ever reads a FETCH's subscriber priority. Every one of the seven does route + /// through `from_wire` / `fromWire`; it is the assertions that are missing, not the + /// conversion. BLO-42129 carries the census. #[test] fn serving_subscription_converts_the_subscriber_priority() { // Least urgent source media, which must stay above ...