test(net): a mid-group rejoin keeps the relay's copy whole

[Dryvnt]

Problem

On moq-net 0.3.9, a client could leave a track and quickly rejoin while the newest group was still open. Its idle front had kept the frames already delivered, so the rejoin asked for only the next frame, (G, 1). The relay forwarded that narrowed SUBSCRIBE upstream, and its own copy of G then started at frame 1. Later readers at the relay couldn't get G until the publisher started a new group: an in-process reader got Lagged, and a subscriber through moq-relay got nothing. An external consumer (OneTooMany) hit this on a catalog track whose snapshot group stays open for deltas.

#4741 fixed it as a side effect: fronts no longer cache media, so the rejoin asks for the live edge again. No test covers the case.

Approach

Add rejoin_mid_group_keeps_the_head_for_later_readers to rs/moq-net/tests/rejoin.rs. It connects publisher, relay and client over mock sessions on paused time. The client reads the open group's only frame, leaves, and rejoins, and then a reader at the relay must get frame 0.

It fails on moq-net-v0.3.9 and on 0382d309f^ (moq-lite-06: the later reader lost the group's head: Err(Lagged)), and passes on main. lite-05 and the IETF drafts pass either way. They stay in the version list for coverage, like the neighbouring tests.

Impact

• Test only. No public API or wire changes.

Alternatives

• Run the same scenario through a real moq-relay with a second subscriber session, as in the original report. It reproduces there too, but it runs on the wall clock and needs a short sleep for the client to go idle. Over mock sessions, a second session's reader still got frame 0, so the in-process reader at the relay is the deterministic check.

Follow-ups

• The released moq-net 0.3.x still has the bug. Whether fix(net)!: resume route changes by reading the routes' copies; a path is one broadcast #4741, or a smaller fix, should be backported to release is the maintainer's call.

(Written by Claude Opus 5.5)

🤖 Generated with Claude Code

[coderabbitai]

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

🧰 Additional context used

📚 Code guidelines (1)

rs/AGENTS.md — auto-discovered

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

• Configuration used: Organization UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: 060c7270-e0bf-4d92-b838-dccaf02a8021

📥 Commits

Reviewing files that changed from the base of the PR and between 2704e10 and 4bd2a55.

📒 Files selected for processing (1)

• rs/moq-net/tests/rejoin.rs

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review.

---

Walkthrough

Adds an async test across five protocol versions. The test checks that a client can leave and rejoin an open group through a relay, then read a0. It also checks that a later relay reader receives sequence 0 with a0. Each run has a 10-second timeout.

Priority: ⬇️ Low

Merge Risk: ⚪ Minimal · up to 4bd2a

This change adds regression coverage for rejoining an open group through a relay and does not alter runtime behavior. No merge-blocking risk was identified.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly identifies the test and its purpose: preserving the relay’s complete group after a mid-group rejoin.
Description check	✅ Passed	The description explains the bug, the test approach, and the test-only impact. It is directly related to the changeset.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 1 files.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

✨ Simplify code

• Create a new PR

• Autopilot · 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

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[kixelated]

Automated review by review (OpenAI)

Reviewed commit: 4bd2a55

No actionable introduced bugs found in the full diff.

Direction: the focused regression test in rs/moq-net/tests/rejoin.rs:184–243 is appropriate. It waits for upstream demand to disappear before rejoining, retains the returning subscription, and checks that a later relay reader receives group 0 from its first frame. Two mock links are justified to exercise the client/relay distinction; using the existing paused-time harness and an in-process relay reader is simpler than adding a wall-clock relay integration test. No production, public API, or wire changes.

Verification: statically reviewed the complete change, neighboring tests, mock harness, and relevant group/track/front lifecycle code. I did not run tests or independently reproduce the reported failure on older revisions.

(Written by OpenAI)

[kixelated]

Automated review: moq#4828 at 4bd2a55e

Test-only PR, one file (rs/moq-net/tests/rejoin.rs +65), CI is green. It pins the 0.3.9 regression where a client rejoining an open group made the relay's fresh upstream copy start at frame 1, so later relay readers got Lagged. It's a useful guard against front-side media caching coming back. A few gaps, none of them blocking:

Non-blocking

1. It doesn't exercise the relay-side failure on main. rejoin.rs:224 uses subscribe(None). Since fix(net)!: resume route changes by reading the routes' copies; a path is one broadcast #4741 the client no longer narrows its rejoin, so on main the relay never sees a mid-group start, and the test passes without reaching the code that broke. Old 0.3.x clients and mesh peers resuming after a route change will still send an explicit (G, 1) start. fix(net): a relay resuming mid-group asks upstream for the group's head #4829's follow-up notes that the same scenario on main (an explicit with_start resume, in catalog_resume_snapshot.rs) still stalls on lite-07. Consider adding a variant here where the client subscribes with track::Subscription::default().with_start(Position { group: 0, frame: 1 }). Mark the lite-07 case as a known failure, or link it to the quest fix(net): a relay resuming mid-group asks upstream for the group's head #4829 promises, so that gap is tracked by a test and not just a PR note.
2. It overlaps fix(net): a relay resuming mid-group asks upstream for the group's head #4829's new test. catalog_resume_snapshot.rs on release covers the same open-group, later-reader shape, plus controls for no resume and for a resume at (G, 0). When release back-merges into main, you'll have two near-duplicate tests in different files. It's probably worth folding them together, or at least cross-referencing them in the doc comments.
3. Nothing checks that later frames still arrive. The publisher writes only a0 (rejoin.rs:203) and never appends a delta after the rejoin. A regression that widens to the head but drops later frames, or one that leaves the rejoined client parked, would still pass. Write an a1 after the later reader subscribes, and assert that both rejoined and later receive it. This is the same gap noted on fix(net): a relay resuming mid-group asks upstream for the group's head #4829, whose test doesn't assert that the resumed peer gets delta1.

Verdict: MERGE (head 4bd2a55e). It's a cheap, deterministic regression test. Item 1 is the main way to make it guard the relay, not just the client.

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

[kixelated]

Merging as is at 4bd2a55e662ae991a497aa95405f8fb8969b9192: test-only, CI green, no actionable review findings.

On the automated review's non-blocking notes:

1. Explicit mid-group with_start variant: left to the lite-07 head-fetch quest in quest(m1): retracted demand release and lite-07 fetched heads #4830, which owns that stall. This test guards the client side (no front-side media caching), which is what regressed in 0.3.9.
2. Overlap with fix(net): a relay resuming mid-group asks upstream for the group's head #4829's catalog_resume_snapshot.rs: that test is on release. Fold or cross-reference when release back-merges into main.
3. Later-frame assertion: tried it (write a1 after the later reader joins, assert both readers get it). All lite versions pass, but on moq-transport-19 and moq-transport-22 the rejoined client's open group ends with Ok(None) before a1. A first subscription on IETF does receive the delta, so this is a separate IETF rejoin bug, not this PR's scope. Left the assertion out and flagged it as a follow-up.

Thanks @Dryvnt!

(Written by Claude Opus 5.5)