Repository navigation
fix(net): preserve current groups across IETF range updates - #7551
Conversation
Quest-Claim: c148f8816d946158
Build range updates from their absolute group bounds instead of reusing the initial join's Next Object filter. Rejoining at a newer object resets a stream that readers are still draining, and a duplicate head fill cannot replace it. Keep the existing one-group fill and initial join behavior. Reproduced on main e02e26b with: just test interop --all --publishers rust,cpp --versions ietf Draft 22 failed six Rust publisher media cells with audio gaps after an invalid-range recovery FETCH. Drafts 18 and 20 passed. The simulated open-start update test also fails before this change, for bounded and unbounded ends. Co-Authored-By: GPT-6.1 Sol <noreply@openai.com>
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 14 minutes. View limit details
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. CodeRabbit Verify
|
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: 8b585ad.
No actionable bugs found in the changed areas and relevant surrounding code. The direction is sound: range updates now express absolute bounds instead of resolving a fresh live edge, while initial joins retain Next Object and their current-group fill. Reusing the older-draft construction is simpler than adding recovery retries or redesigning fill ownership. The regression test checks both unbounded and bounded open-start updates; the publisher's existing update handling preserves the live stream under the widened range.
Verification: static inspection of the update sender, bounds resolution, filter encoding, publisher range/fill handling, and new test. I did not run tests or independently reproduce the reported interop results.
(Written by OpenAI)
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
- Range updates from absolute bounds on every draft (
rs/moq-net/src/ietf/subscriber.rs:3640): the draft-20+ branch that reusedsubscribe_joinis gone; an open-start update now sendsFilter::Unfiltered(orAbsolute { start: 0, end }when bounded), matching the publisher'supdate_rangeatpublisher.rs:4457that already reads an unfiltered update as a widening back to zero. The initial join at line 2943 keepssubscribe_join, so live joins still useNextObject. - Bounded open-start end no longer dropped: the old
subscribe_join(None, Some(end))path ignoredendon draft 20+; the newAbsolute { start: 0, end }carries it. The..Default::default()keepsfill: None/fetch: false, equivalent to the removedelse { join.fill = None }. - Test + quest cleanup:
widening_to_an_open_start_does_not_rejoin_at_the_next_objectcovers both open ends; the completeddraft22-interopquest and its index bullet are removed with no dangling references.
Verified independently this run: the new test passes, cargo clippy --locked -p moq-net --all-targets -- -D warnings passes, and JS #runUpdates carries only priority and max age (no filter/fill), so no js/net parity work is needed.
deepseek-v4.1-flash (free via Pullfrog for OSS) | 𝕏
# Conflicts: # rs/moq-net/src/ietf/subscriber.rs

Problem
A relay widening an existing IETF subscription to an open start reused the initial join's
NextObjectselector. The publisher resolved a newer live edge and reset the current group's stream. Its replacement head fill duplicated already-cached objects, so recovery FETCH started past Largest Object, was refused withinvalid fetch range, and several downstream audio readers saw a gap.Reproduced on main e02e26b4c27663208fcc8e8cf9271d968a81cffb, after moq-dev/moq#7204, with
just test interop --all --publishers rust,cpp --versions ietf: drafts 18 and 20 passed; draft 22 failed six Rust-publisher media cells (Rust, Python, Go, Bun, C, GStreamer). One Opus timestamp trace lost the tail after 360 ms and resumed at 1000 ms. A draft-22-only run had passed earlier, so a single green run was insufficient evidence to delete the quest.Approach
Impact
#runUpdatesfollows only priority and max age. No user workflow or format documentation changes.Alternatives
Changing the publisher's valid start-past-Largest refusal, retrying FETCH, or redesigning fill ownership would hide or expand this problem. Correct the selector at its source instead. The quest's evidence-only completion option was ruled out by the reproduction.
Validation
quest gate just ci: passed, including 9,301 Rust tests (106 skipped), lint/build/doc checks, and repository checks.quest gate just test interop --all --publishers rust,cpp --versions ietf: passed after the fix, 46 passing records per draft and all 54 media cells. No invalid-FETCH-range, head-stitching, audio-gap, or timestamp-gap diagnostic in the retained logs.quest gate just test interop --all: passed, 178 passing records, including all 45 positive media cells, negative controls, finished media, and protocol control lanes.quest check --base origin/mainandgit diff --check: passed.All commands used this checkout's Nix dev shell with BASH_ENV disabled. Reproduction and fixed logs remain in this checkout's ignored
.tmp/.Review
Reviewed head
7d45fa8aa5(afaec4c plus conflict-resolving merges of origin/main through bf5e2b0) withquest review --base origin/main: Codex, gpt-6.1-sol, medium effort, round 1, base0ef46ad072. Findings: none.Still needed on today's main: the draft-20+ update path in
rs/moq-net/src/ietf/subscriber.rsstill calledsubscribe_join, so an open-start widening still sent Next Object. The publisher'supdate_range(rs/moq-net/src/ietf/publisher.rs:4615) still reads an update's Unfiltered as a widening back to zero. This keeps to the standing "simplest IETF FETCH/SUBSCRIBE path" rule: one absolute-bounds spelling for every draft, with no draft-specific fallback.Merge resolution: moq-dev/moq#7625 made
subscribe_joininfallible, which conflicted with the removedmatch. I kept this PR's side. The diff against main matches the earlier reviewed diff.Automated reviews on
8b585ad981: OpenAI and Pullfrog reported no issues, and CodeRabbit was rate-limited. No review threads are open.Post-merge checks:
cargo nextest run -p moq-netpassed (3310 tests, 13 skipped), includingwidening_to_an_open_start_does_not_rejoin_at_the_next_object.cargo clippy --locked -p moq-net --all-targets -- -D warnings,cargo fmt --check -p moq-net,quest check --base origin/main, andgit diff --checkall passed.Follow-ups
None. Independent test-CPU and relay auth-trust work is untouched. A release backport should be considered after this lands because this fixes media loss; preserve the release API.
(Written by GPT-6.1 Sol; Review section by Claude Opus 5.5)