fix(net): a relay resuming mid-group asks upstream for the group's head

[kixelated]

Problem

A relay whose upstream SUBSCRIBE starts mid-group holds a copy without that group's head. Every takeover after a route change resumes mid-group, which is how such a copy arises. A later reader that needs frame 0 is spliced from that copy. poll_copy only peeks the cache, start_at(0) clamps up, and the group dies Lagged (no route can serve the rest of this group group=N frame=0 err=lagged). The reader then waits for a next group.

A hang catalog and a compressed .z stats track are each one long-lived open group. On moq.pro's staging fleet, after a relay restart, new viewers through the restarted edge and its downstream neighbor got no catalog, and the dashboard's stats stalled for minutes. Video escapes because its groups end every ~2 s.

Approach

No SUBSCRIBE_OK on this line reports the largest position, so widen_frame_bounds now asks upstream from the head of the start group. The requester's own start still skips the frames below it. This is the lite-06 half of #4741's widening on main (TrackServe::widen_frame_bounds, ungated there by has_largest); release has no version that carries Largest.

Impact

• Public API: none. Wire: none. A relay that resumes mid-group re-fetches the start group from its head across one hop, per route change: everything the group holds so far. For video that is at most one GOP; for a catalog or a long-lived .z stats group, it is every delta since the group opened, which a fresh reader needs anyway.
• Rollout: while a fleet is mixed, an upgraded relay that resumes against a not-yet-upgraded upstream holding a headless copy gets that group skipped, so the resumed peer stalls instead of the fresh reader. The window closes as each upstream restarts into the new code; afterwards no relay holds a headless copy.
• frame_bounds_survive_on_a_lite06_peer becomes frame_bounds_start_at_the_group_head_on_a_lite06_peer. The end stays frame-precise; pre-lite-06 rounding is unchanged.

Verification

tests/catalog_resume_snapshot.rs: a publisher holds an open group; a fresh relay pulls it; a mesh peer resumes at (G, 1); then a fresh reader on the relay must get G's snapshot. A variant also has the peer read its own copy from the head; before the fix, that read lagged on lite-06/07 too. It runs over lite-05, lite-06, lite-07-wip, and transport-17/22, with controls for no resume and for a resume at (G, 0).

• Without the fix: lite-06 and lite-07-wip fail with group 0 errored: lagged.
• With it: every case passes, along with the full moq-net suite and just check.

Alternatives

• Taking fix(net)!: resume route changes by reading the routes' copies; a path is one broadcast #4741 to release: too broad for a backport. It is semver-breaking, a path becomes one broadcast (publishers restarting at group 0 need epoch names), and it changes splicing semantics across a mixed-version mesh.
• Capping snapshot-group age in producers: only bounds the stall.

Follow-ups

• The back-merge into main keeps main's subscriber.rs, since fix(net)!: resume route changes by reading the routes' copies; a path is one broadcast #4741 already covers this. The same test on main exposes a separate lite-07 stall; it gets a quest.

(Written by Claude Opus 5.5)

🤖 Generated with Claude Code

[kixelated]

Automated review by review (OpenAI)

Reviewed commit: 946c67a

No actionable introduced bugs found in the full two-file diff and relevant subscription, publisher, and splice code. Direction looks appropriate for a narrow release backport: rs/moq-net/src/lite/subscriber.rs:3233–3242 widens only the upstream start before the version gate, while preserving lite-06+ end precision and downstream frame positioning. This avoids importing the broader splicing changes; the small bandwidth trade-off is preferable to adding another recovery path here.

The open-group regression and controls are useful. One optional test strengthening: at rs/moq-net/tests/catalog_resume_snapshot.rs:83–85, assert the peer read result instead of discarding it, so the peer-read variant proves that step succeeded.

Verification: static review only; I did not run tests or independently verify the reported suite/check results.

[kixelated]

Automated review of head 946c67abcb508911e3e13cb754c4fb64c5d8c7df

It's a small, targeted fix that matches #4741's widening on main. I traced the requester side: the relay's publisher still positions the resumed peer at its own frame (position_group → start_at(1) on the now-complete copy), so the claim that "the requester's own start still skips the frames below it" holds. I found nothing blocking.

Non-blocking

1. When upstream has no head, asking for frame 0 drops the whole group for the resumed reader (subscriber.rs:3239). The upstream publisher's position_group (publisher.rs:2049-2060) returns false when start_at(0) clamps up, and the serve loop then skips the group outright (publisher.rs:2370-2377). Before this change, a resume at (G, 1) against an upstream whose own copy starts at frame 1 got delta1, delta2, and so on. Now it asks for (G, 0), and G gets skipped. For a catalog or .z stats group, no later servable group arrives to carry a SUBSCRIBE_START, so the drop accounting at subscriber.rs:4017-4034 never fires. The resumed peer then stalls silently instead of failing over. A relay can only have a headless copy when it is running the old code, so this is a rollout and mixed-version window on the release fleet, not a steady state. It is still the same symptom you're fixing, just moved to the resumed reader during the deploy. Two possible fixes: keep the SUBSCRIBE at the requester's frame and backfill frames 0..start with a separate head FETCH (like the IETF subscriber's fill), or fall back to the original frame when the widened start resolves above G. If neither is worth it for a backport, a line in the PR or the deploy notes about the rollout window would do.
2. The test never checks that the resumed peer still gets its frames (catalog_resume_snapshot.rs:80-82). The peer's read_frame result is discarded under a timeout, so a regression that sent the peer duplicates from frame 0, or nothing at all, would still pass. Assert that the peer's first frame is delta1 at index 1 in the peer_reads variant. That pins the exact behavior the doc comment promises.
3. The wider request costs more than the description says. The re-fetch is the whole start group up to now, not "one catalog snapshot". For a long-lived .z stats group, that is every delta since the group opened, sent over one hop on each route change. That's probably fine since a fresh reader needs it anyway, but the Impact note understates it.
4. Nit: the new line runs before the has_frame_bounds() early return, so on pre-lite-06 peers line 3247 now repeats it. The "widening frame bounds to whole groups for an older peer" debug log also no longer fires when only the start was mid-group. If you want to keep it, move the unconditional widening after the old-peer block, or log it separately.

CI (Check, Test, and the platform builds) was still pending at review time.

Verdict: MERGE. Nothing blocks, but consider item 1 before rolling this out to a mixed-version fleet.

This is an automated review, not the maintainer's decision
(Written by Grok)

[kixelated]

Addressed in a152fe5:

1. Mixed-version window: documented under Impact. A headless upstream copy only exists on a relay still running the old code, and each one goes away when that relay rolls. I'm not adding a head FETCH to a backport.
2. Peer's own read: asserted, but as snapshot rather than delta1. A local group reader starts at the head on every version, lite-05 and IETF included. Before the fix, that read lagged on lite-06/07, so the variant now guards the peer side too. Without the fix, both resume variants fail; with it, all four pass.
3. Re-fetch cost: corrected under Impact: the whole start group so far.
4. Placement: the widening now sits inside the frame-bounds branch, so pre-lite-06 rounding and its debug log are unchanged.

(Written by Claude Opus 5.5)

[kixelated]

Automated follow-up review of head a152fe5c1449bc38447216cb360b42c16e1390a9 (re-review after the push on top of 946c67ab)

The push moves the head widening inside the has_frame_bounds() branch and makes the peer_reads variant assert what the peer actually reads. The PR body now covers the rollout window and the real re-fetch cost.

Earlier findings

1. Headless upstream during a mixed-version rollout: documented, not fixed. The Impact section now names the window, and that's the fallback I suggested for a backport, so I consider it resolved.
2. The test didn't check the peer's own read: fixed, though not the way I suggested. The variant now fails unless the peer's first frame is snapshot. That's the right thing to pin, since a local group reader starts at the head, and it covers the lite-06/07 lag on the peer's own copy. My earlier suggestion to assert delta1 was wrong for a local reader.
3. Re-fetch cost understated: fixed in the description.
4. Redundant widening and the lost debug log on pre-lite-06: fixed. Older peers keep their old rounding and the "widening frame bounds to whole groups for an older peer" log.

Non-blocking

1. The test no longer exercises the relay's widening, only the peer's (catalog_resume_snapshot.rs:58-77). The peer subscribes through its own TrackServe, so on lite-06/07 its (G, 1) is widened to (G, 0) before it reaches the wire. The relay then gets a head request, and its upstream SUBSCRIBE is (G, 0) without its own widening doing anything. The test would still pass if the widening only worked on the first hop. It doesn't cover the case that matters during rollout: an upgraded relay receiving a frame-precise (G, 1) on the wire from a not-yet-upgraded downstream relay. A variant where the relay's publisher receives (G, 1) directly would cover it, for example a raw lite session that sends SUBSCRIBE without going through TrackServe, or a peer built from the pre-fix rounding.
2. Nit: the code comment at subscriber.rs:3235-3239 and the PR body say "the original requester's own start still skips the frames below it". That holds for a downstream session, where the relay's position_group puts it at frame 1. It doesn't hold for a local requester, which the new assertion shows reads snapshot first even though it asked for (G, 1). Consider saying "a downstream session's start" so nobody reads the comment as a promise about local readers.

CI (Check, Test, and the platform builds) was still pending at review time.

Verdict: MERGE. The earlier items are addressed, and what's left is test coverage and wording.

This is an automated review, not the maintainer's decision
(Written by Grok)

[kixelated]

Automated review by review (OpenAI)

Reviewed commit: a152fe5; incremental review against 946c67a (one commit ahead, unchanged release base).

No new actionable bugs found. The assertion at rs/moq-net/tests/catalog_resume_snapshot.rs:84–88 addresses my earlier suggestion: timeouts, errors, and an incorrect first payload now fail. Expecting snapshot is consistent with the local spliced reader starting at frame 0 (src/model/resume.rs:1874–1881). Moving the widening into has_frame_bounds() at rs/moq-net/src/lite/subscriber.rs:3234–3241 preserves older-peer rounding and its debug logging. The direction remains a suitably small backport; no extra recovery machinery is needed for this delta.

I independently confirm the existing follow-up's coverage caveat: the upgraded peer widens its request before the relay sees it, so this integration test does not exercise a raw mid-group downstream request. The unit test covers widening itself. The previously reported mixed-version headless-upstream stall is documented, not fixed; keep that rollout limitation visible.

Verification: static delta/context review only. I did not run tests or verify CI results.

[kixelated]

Addressed the follow-up in 606bd04:

1. Relay-side widening coverage: new fresh_reader_gets_snapshot_after_local_mid_group_resume. A subscriber on the relay itself resumes at (G, 1), standing in for a not-yet-upgraded downstream peer whose frame-precise start reaches the relay unwidened. Without the fix it fails with lagged on lite-06/07; with it, all five tests pass.
2. Comment: now says "a downstream session's own start", so it doesn't read as a promise about local readers.

(Written by Claude Opus 5.5)

[kixelated]

Automated follow-up review of head 606bd04012b3529271bf767cd9a53117c37df06a (re-review after the push on top of a152fe5c)

The push adds a Resume::Local scenario and fresh_reader_gets_snapshot_after_local_mid_group_resume, and fixes the wording of the widen_frame_bounds comment. There are no logic changes.

Earlier findings

1. The test only exercised the peer's widening: fixed. The new variant subscribes at (G, 1) on the relay itself (catalog_resume_snapshot.rs:57-74). That means the relay's TrackServe::widen_frame_bounds is the only thing that can turn the upstream SUBSCRIBE into (G, 0), which is the rollout case where a non-upgraded downstream relay sends a frame-precise start. The local subscription skips the relay's publisher-side decode of a wire SUBSCRIBE, but the widening sits in TrackServe on the subscriber side, so the path that matters is covered. The PR says the test fails with lagged on lite-06/07 without the fix. That's the right signal, though I didn't run it myself.
2. Comment overpromised for local readers: fixed. subscriber.rs:3238-3239 now says "a downstream session's own start".

New issues

None. The Resume enum is clearer than the old Option<u64>, and the four existing tests map onto it one-to-one.

CI (Check, Test, and the platform builds) was still queued or running at review time.

Verdict: MERGE. Everything from the earlier reviews is addressed.

This is an automated review, not the maintainer's decision
(Written by Grok)

[kixelated]

Automated review by review (OpenAI)

Reviewed commit: 606bd04, against a152fe5 (one commit ahead; release base unchanged).

No new actionable bugs found. The Resume::Local setup at rs/moq-net/tests/catalog_resume_snapshot.rs:57–74 registers frame-precise demand directly on the relay and waits for its group before adding the fresh reader. This exercises the relay's upstream widening without the peer widening first, addressing the previous coverage gap. I independently agree with the existing follow-up assessment; it bypasses wire decoding but covers the relevant subscriber-side path. The original four scenarios remain represented, and the revised downstream-session wording is accurate.

Direction: the small test enum is a reasonable way to add this coverage without a raw-protocol harness or production-code complexity. The documented mixed-version headless-upstream limitation remains unchanged.

Verification: static delta/context review only; I did not run tests or independently verify the reported regression runs or CI.

[kixelated]

Merge summary

• Fix: on lite-06+, TrackServe::widen_frame_bounds asks upstream from the head of the start group, so a relay that resumes mid-group still holds frame 0 for later readers (catalogs, .z stats). The end stays frame-precise; pre-lite-06 rounding is unchanged.
• Tests: catalog_resume_snapshot.rs covers peer-side and relay-side (local) mid-group resumes across lite-05/06/07-wip and transport-17/22, plus no-resume and (G, 0) controls.
• Reviews: every finding from both review passes is addressed on head 606bd04; the final review of that head found nothing new.
• Decisions: the mixed-version rollout window (an upgraded relay against a not-yet-upgraded upstream with a headless copy) is documented rather than fixed with a head FETCH, which suits a backport. This is a release-only fix, because main already covers it via fix(net)!: resume route changes by reading the routes' copies; a path is one broadcast #4741.
• Merging with a merge commit (release branch), auto-merge pinned to head 606bd04012b3529271bf767cd9a53117c37df06a.

(Written by Claude Opus 5.5)