From 336d9fe1ecd85777d3aaa8a7f9650b1ad56791c1 Mon Sep 17 00:00:00 2001 From: Players Engineer Date: Sun, 11 Oct 2026 01:09:12 +0000 Subject: [PATCH 1/2] test(relay): close the arm-B harness's three drift hazards (BLO-41723) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Ally's three non-blocking suggestions on #5, plus one finding they surface. 1. `FETCH_BLOCK` is now a named const used at all three sites that were literal `1` (`fetch_group`, the SBN assertion, the groups tally) and at the report label, with `const _: () = assert!(FETCH_BLOCK < BLOCKS)` next to it. Editing `BLOCKS` to `1` used to fetch a group that was never published and surface as `expect("fetch timeout")` — drift reading as a delivery failure, which is what the two consts were introduced to remove. Verified: with `BLOCKS: u32 = 1` the tree now fails to compile, `error[E0080]: evaluation panicked: FETCH_BLOCK must name a published block` at the assert, instead of building and timing out. 2. The re-run recipe now ends by restoring `rs/moq-net`. Without it the reader is left holding a checkout with the Blockcast `moq-net` work deleted and nothing on screen saying so — this file's own subject one level down. Note the recipe uses `git checkout HEAD --`, not `git restore`: `git checkout -- ` stages into the index, so a plain index-sourced `git restore` rewrites the reverted tree rather than undoing it. Checked on a scratch repo before writing it down. 3. The attestation named `45cd0856`, which was `head~1` of #5 and is now neither upstream nor any commit a reader on `main` is looking at. It now names `a6bac516` — `main` as of this commit's parent — says it is the parent, and gives the `git diff a6bac516..HEAD -- …` that answers whether it still speaks for the reader's tree. Re-run today at that commit: 6/6 green both with `rs/moq-net` as-is and reverted to `0574f5b60`, same counts, `git diff 0574f5b60 --stat -- rs/moq-net` empty on the reverted tree. Arm B's zero-diff claim holds. Finding, from doing (3): the header's description of this branch was already stale on `main`. It named #2 alone (`publisher.rs +292 -34`), true of #5's head — Ally verified exactly that at `bfb7262f` — but #3 merged between #4 and #5, so `main` carries two files, +414 -41. Header corrected to name both #2 and #3 with their real counts. Neither is an arm-B carried patch, so the argument is unchanged; only the arithmetic was wrong. This is the merge-skew form of the drift #5 existed to fix, which is why (3) now records a diff command rather than only a SHA. Comments, three constants and a report label. No assertion changes an outcome; every `[arm-b/…]` count is byte-identical to #4's and #5's. Co-Authored-By: Claude Opus 5 (1M context) --- rs/moq-relay/tests/mmtp_arm_b.rs | 79 ++++++++++++++++++++++---------- 1 file changed, 54 insertions(+), 25 deletions(-) diff --git a/rs/moq-relay/tests/mmtp_arm_b.rs b/rs/moq-relay/tests/mmtp_arm_b.rs index d3751ace186..ad17465d951 100644 --- a/rs/moq-relay/tests/mmtp_arm_b.rs +++ b/rs/moq-relay/tests/mmtp_arm_b.rs @@ -26,21 +26,47 @@ //! ## This branch is NOT a stock checkout, so that claim has to be re-run //! //! The file lives on `Blockcast/moq`'s `main`, which is upstream `0574f5b60` -//! plus Blockcast/moq#2 -- `rs/moq-net/src/ietf/publisher.rs +292 -34`, the -//! draft-20 FETCH work from BLO-41181. So "zero diff against upstream" is a -//! claim about `0574f5b60`, not about the tree this file is read in, and a -//! green run here does not on its own establish it. Re-establish it with: +//! plus two Blockcast changes under `rs/moq-net`, together `priority.rs +38` +//! and `publisher.rs +417 -41` (two files, +414 -41): +//! +//! * Blockcast/moq#2 -- `publisher.rs +292 -34`, the draft-20 FETCH work from +//! BLO-41181; +//! * Blockcast/moq#3 -- `priority.rs +38`, `publisher.rs +91 -7`, the +//! repair-band ordering pins from BLO-41724. +//! +//! Neither is an arm-B carried patch -- both are Blockcast's own `moq-net` +//! work that this harness merely sits beside -- so "zero diff against +//! upstream" is a claim about `0574f5b60`, not about the tree this file is +//! read in, and a green run here does not on its own establish it. +//! Re-establish it with: //! //! ```text //! git checkout 0574f5b60 -- rs/moq-net //! RUSTUP_TOOLCHAIN=stable cargo test -p moq-relay --features test-support \ //! --test mmtp_arm_b -- --nocapture +//! git checkout HEAD -- rs/moq-net # put #2 and #3 back; see below //! ``` //! -//! Verified green that way on 2026-10-09 at `45cd0856`: 6/6, same counts as -//! the unreverted tree. Anything cut from this branch inherits #2 -- see the -//! scope note on `arm_b_measurements_are_taken_on_a_moq_lite_session`, where -//! #2 changes what the IETF half of the FETCH result would mean. +//! The last line is not optional, and plain `git restore rs/moq-net` is not a +//! substitute: `git checkout -- ` stages into the index as +//! well, so an index-sourced restore just rewrites the reverted tree. Skip it +//! either way and the checkout is left with both changes removed while nothing +//! on screen says so: anything run next there -- #2's own `a_draft20_fetch_*` +//! tests and #3's priority pins most of all -- would silently measure a tree +//! with the draft-20 FETCH deleted. +//! That is this file's own subject one level down, a reader holding a tree +//! that is not the tree they think they hold. +//! +//! Verified green that way on 2026-10-11 at `a6bac516` -- `main` at the time +//! this note was written, i.e. the parent of the commit carrying it: 6/6 both +//! reverted and unreverted, same counts. The SHA is recorded so a reader can +//! diff forward from it. A later commit does not invalidate the run; it only +//! means the run is older than the tree, and `git diff a6bac516..HEAD -- +//! rs/moq-net rs/moq-relay/tests/mmtp_arm_b.rs` is the whole question of +//! whether it still speaks for this tree. Anything cut from this branch +//! inherits both -- see the scope note on +//! `arm_b_measurements_are_taken_on_a_moq_lite_session`, where #2 changes what +//! the IETF half of the FETCH result would mean. //! //! ## Ordering matters, and getting it wrong measures the cache instead //! @@ -748,11 +774,17 @@ async fn arm_b_fetch_group_under_contiguous_object_ids() { let live_drain = spawn_drain("fetch/live-subscription", live); tokio::time::sleep(SETTLE).await; - // Every count below is derived from these two, not restated: a hand-written - // literal that drifts from the layout shows up as a delivery failure, after - // the `[arm-b/fetch/...]` line has already printed the wrong `dropped=`. + // Every count and group selector below is derived from these three, not + // restated: a hand-written literal that drifts from the layout shows up as a + // delivery failure, after the `[arm-b/fetch/...]` line has already printed + // the wrong `dropped=`. `FETCH_BLOCK` is the one with no count to check it, + // so its coupling to `BLOCKS` is asserted at compile time instead -- out of + // range, it would fetch a group that was never published and surface as the + // `fetch timeout` below rather than as the edit that caused it. const BLOCKS: u32 = 3; const SYMBOLS_PER_BLOCK: u32 = 4; + const FETCH_BLOCK: u32 = 1; + const _: () = assert!(FETCH_BLOCK < BLOCKS, "FETCH_BLOCK must name a published block"); let mut sent = [Flow::default()]; publish_blocks(BLOCKS, &[(&producers[0], 0, SYMBOLS_PER_BLOCK, b"src")], &mut sent).await; @@ -767,37 +799,34 @@ async fn arm_b_fetch_group_under_contiguous_object_ids() { "every object reached the relay cache" ); - // FETCH block 1 by group sequence, not by subscribing and waiting. + // FETCH one block by group sequence, not by subscribing and waiting. let track = h.consumer.track("video").expect("track announced"); let mut fetched = Flow { sent: SYMBOLS_PER_BLOCK as usize, ..Default::default() }; - let mut group = tokio::time::timeout(TIMEOUT, track.fetch_group(1, None)) + let mut group = tokio::time::timeout(TIMEOUT, track.fetch_group(FETCH_BLOCK as u64, None)) .await .expect("fetch timeout") - .expect("fetch group 1"); + .expect("fetch group"); + // The report label names the block too, so it cannot mislabel the + // `[arm-b/fetch/...]` evidence line if `FETCH_BLOCK` is edited. + let label = format!("fetch/group{FETCH_BLOCK}"); loop { let frame = match tokio::time::timeout(TIMEOUT, group.read_frame()).await { Ok(Ok(Some(frame))) => frame, Ok(Ok(None)) => break, - Ok(Err(err)) => panic!( - "fetch/group1: transport error after {} objects: {err}", - fetched.received - ), - Err(_) => panic!( - "fetch/group1: timed out after {TIMEOUT:?}, {} objects in", - fetched.received - ), + Ok(Err(err)) => panic!("{label}: transport error after {} objects: {err}", fetched.received), + Err(_) => panic!("{label}: timed out after {TIMEOUT:?}, {} objects in", fetched.received), }; let decoded = decode(&frame.payload); - assert_eq!(decoded.sbn, 1, "fetched block carries its own SBN"); + assert_eq!(decoded.sbn, FETCH_BLOCK, "fetched block carries its own SBN"); fetched.received += 1; fetched.objects.push(decoded); - *fetched.groups.entry(1).or_default() += 1; + *fetched.groups.entry(FETCH_BLOCK as u64).or_default() += 1; } - println!("{}", fetched.report("fetch/group1")); + println!("{}", fetched.report(&label)); assert_eq!( fetched.received, SYMBOLS_PER_BLOCK as usize, "FETCH served the whole block" From 88cd16a43c8899f78c624ef2fe4c7d2277650cb4 Mon Sep 17 00:00:00 2001 From: Players Engineer Date: Mon, 12 Oct 2026 10:26:34 +0000 Subject: [PATCH 2/2] test(relay): correct arm-B header per-file diffstat (BLO-41723) publisher.rs counts were the --stat bar total (insertions+deletions) read as insertions: +376 -41 overall, +84 -7 for #3, per git diff --numstat. Also widen the forward-diff pathspec to rs/ Cargo.lock, drop a parenthetical that goes false once main moves, and say 'test output' rather than 'on screen' (git status does show the staged revert). Co-Authored-By: Claude Opus 5.5 (1M context) Signed-off-by: Players Engineer --- rs/moq-relay/tests/mmtp_arm_b.rs | 25 +++++++++++++------------ 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/rs/moq-relay/tests/mmtp_arm_b.rs b/rs/moq-relay/tests/mmtp_arm_b.rs index ad17465d951..20e5740d9d4 100644 --- a/rs/moq-relay/tests/mmtp_arm_b.rs +++ b/rs/moq-relay/tests/mmtp_arm_b.rs @@ -27,11 +27,11 @@ //! //! The file lives on `Blockcast/moq`'s `main`, which is upstream `0574f5b60` //! plus two Blockcast changes under `rs/moq-net`, together `priority.rs +38` -//! and `publisher.rs +417 -41` (two files, +414 -41): +//! and `publisher.rs +376 -41` (two files, +414 -41): //! //! * Blockcast/moq#2 -- `publisher.rs +292 -34`, the draft-20 FETCH work from //! BLO-41181; -//! * Blockcast/moq#3 -- `priority.rs +38`, `publisher.rs +91 -7`, the +//! * Blockcast/moq#3 -- `priority.rs +38`, `publisher.rs +84 -7`, the //! repair-band ordering pins from BLO-41724. //! //! Neither is an arm-B carried patch -- both are Blockcast's own `moq-net` @@ -51,20 +51,21 @@ //! substitute: `git checkout -- ` stages into the index as //! well, so an index-sourced restore just rewrites the reverted tree. Skip it //! either way and the checkout is left with both changes removed while nothing -//! on screen says so: anything run next there -- #2's own `a_draft20_fetch_*` -//! tests and #3's priority pins most of all -- would silently measure a tree -//! with the draft-20 FETCH deleted. +//! in the test output says so: anything run next there -- #2's own +//! `a_draft20_fetch_*` tests and #3's priority pins most of all -- would +//! silently measure a tree with the draft-20 FETCH deleted. //! That is this file's own subject one level down, a reader holding a tree //! that is not the tree they think they hold. //! //! Verified green that way on 2026-10-11 at `a6bac516` -- `main` at the time -//! this note was written, i.e. the parent of the commit carrying it: 6/6 both -//! reverted and unreverted, same counts. The SHA is recorded so a reader can -//! diff forward from it. A later commit does not invalidate the run; it only -//! means the run is older than the tree, and `git diff a6bac516..HEAD -- -//! rs/moq-net rs/moq-relay/tests/mmtp_arm_b.rs` is the whole question of -//! whether it still speaks for this tree. Anything cut from this branch -//! inherits both -- see the scope note on +//! this note was written: 6/6 both reverted and unreverted, same counts. The +//! SHA is recorded so a reader can diff forward from it. A later commit does +//! not invalidate the run; it only means the run is older than the tree, and +//! `git diff a6bac516..HEAD -- rs/ Cargo.lock` is the first question of +//! whether it still speaks for this tree (the run also depends on +//! `moq-relay`'s own source and `moq-lite`, so narrowing the pathspec to +//! `rs/moq-net` would hide a change that matters). Anything cut from this +//! branch inherits both -- see the scope note on //! `arm_b_measurements_are_taken_on_a_moq_lite_session`, where #2 changes what //! the IETF half of the FETCH result would mean. //!