Skip to content

fix(net): preserve current groups across IETF range updates - #7551

Merged
kixelated merged 4 commits into
mainfrom
quest/m0/test-flakes-2/draft22-interop
Nov 3, 2026
Merged

kixelated merged 4 commits into
mainfrom
quest/m0/test-flakes-2/draft22-interop

Conversation

@kixelated

@kixelated kixelated commented Nov 1, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

A relay widening an existing IETF subscription to an open start reused the initial join's NextObject selector. 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 with invalid 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

  • Build range updates from absolute group bounds on every draft, reusing the existing older-draft path. An open start uses Unfiltered, or absolute zero with a bounded end. Initial joins keep their Next Object selector and current-group fill.
  • Add a simulated request-stream test covering widening to an open start with bounded and unbounded ends. It fails before the fix because the update sends Next Object.
  • Remove the completed quest and its epic link. No obsolete multi-group FETCH/fill paths return.

Impact

  • Public API: none.
  • Wire behavior: draft-20/22 REQUEST_UPDATE with an open start carries Unfiltered, or absolute zero with the requested end, instead of Next Object. This uses existing selectors; framing, fields, negotiation, and one-group history are unchanged.
  • JS has no corresponding range-update sender: its IETF #runUpdates follows 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.
  • The focused simulated update test passed for bounded and unbounded ends.
  • Full default 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/main and git 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) with quest review --base origin/main: Codex, gpt-6.1-sol, medium effort, round 1, base 0ef46ad072. Findings: none.

Still needed on today's main: the draft-20+ update path in rs/moq-net/src/ietf/subscriber.rs still called subscribe_join, so an open-start widening still sent Next Object. The publisher's update_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_join infallible, which conflicted with the removed match. 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-net passed (3310 tests, 13 skipped), including widening_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, and git diff --check all 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)

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>
@kixelated kixelated changed the title Draft-22 interop with a native publisher holds fix(net): preserve current groups across IETF range updates Nov 1, 2026
@kixelated
kixelated marked this pull request as ready for review November 1, 2026 18:19
@coderabbitai

coderabbitai Bot commented Nov 1, 2026 •

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

You'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.

Check out review usage here.

View limit details

Limit details: You’ve used all 4 included reviews currently available.

Learn how review limits work.

Review configuration:

Run configuration
  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: ae3c115c-cbac-41ee-a2ab-9113c1c9aa51
Commits

Reviewing files that changed from the base of the PR and between 0ef46ad and 7d45fa8.

Files selected for processing: 3
  • quest/m0/test-flakes-2/README.md
  • quest/m0/test-flakes-2/draft22-interop.md
  • rs/moq-net/src/ietf/subscriber.rs
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

Share

CodeRabbit Verify

  • Run CodeRabbit Verify to test the changes in this pull request.

@kixelated kixelated left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ 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 reused subscribe_join is gone; an open-start update now sends Filter::Unfiltered (or Absolute { start: 0, end } when bounded), matching the publisher's update_range at publisher.rs:4457 that already reads an unfiltered update as a widening back to zero. The initial join at line 2943 keeps subscribe_join, so live joins still use NextObject.
  • Bounded open-start end no longer dropped: the old subscribe_join(None, Some(end)) path ignored end on draft 20+; the new Absolute { start: 0, end } carries it. The ..Default::default() keeps fill: None / fetch: false, equivalent to the removed else { join.fill = None }.
  • Test + quest cleanup: widening_to_an_open_start_does_not_rejoin_at_the_next_object covers both open ends; the completed draft22-interop quest 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.

Pullfrog  | View workflow run | Using deepseek-v4.1-flash (free via Pullfrog for OSS) | 𝕏

@kixelated
kixelated enabled auto-merge (squash) November 1, 2026 18:32
@kixelated
kixelated disabled auto-merge November 3, 2026 06:37
# Conflicts:
#	rs/moq-net/src/ietf/subscriber.rs
@kixelated
kixelated added this pull request to the merge queue Nov 3, 2026
Merged via the queue into main with commit 7ed14bb Nov 3, 2026
9 of 10 checks passed
@kixelated
kixelated deleted the quest/m0/test-flakes-2/draft22-interop branch November 3, 2026 07:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant