fix(net): a group withheld for its first frame holds the end

[kixelated]

Problem

Since #4914, a lite subscriber keeps a received group hidden from readers until its first frame lands (track::Producer::receive_group). The group still counts toward the track's live edge, though. When a publisher declares the end and every group header arrives before its first frame, is_complete sees the edge at the end and readers finish without any of those groups.

#4225's js-native-node -> rust and js-native-bun -> rust tail lanes hit this on every run. The JS publisher sends SUBSCRIBE_END and every group header before its 256 KiB frames. The relay's copy had all four groups cached but none shown (arrival=[], every slot pending), so its readers ended with zero or one group.

Approach

rs/moq-net/src/model/track.rs: the end is not reached while a group below it is still withheld. withholds scans the cached groups below the end, and only once the edge has reached it. A withheld group shows on its first frame or when its stream ends, and the last producer going still seals the track. Neither path leaves readers waiting forever.

Impact

• No public API or wire change.
• Behavior: a reader of a session-fed lite track no longer ends before a group whose header arrived ahead of its first frame.

Validation

• model::track::test::a_withheld_group_holds_the_end and track_tail::a_group_whose_first_frame_trails_its_header_is_delivered (lite-05, 06, 07 over the mock session) both fail without the fix.
• a_withheld_group_that_ends_empty_releases_the_end: a withheld group that finishes empty or is reset releases a reader parked on the end.
• just check passes. cargo nextest run --workspace passes: 6471 tests.
• Rebased onto fix(net): keep a lite subscription's demand until its groups drain #4225 locally, all five just test interop --tail lanes pass, including both JS -> Rust lanes.

Follow-ups

None.

🤖 Generated with Claude Code

(Written by Claude Opus 5.5)

[coderabbitai]

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 3 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

• Configuration used: Organization UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: d0aa205e-eb8e-464d-bfde-a2d3b626a7ee

---

📥 Commits

Reviewing files that changed from the base of the PR and between 53a0970 and bd6876f.

---

📒 Files selected for processing (2)

• rs/moq-net/src/model/track.rs
• rs/moq-net/tests/track_tail.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

• X
• Mastodon
• Reddit
• LinkedIn

---

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

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-10-10T02:21:35.996048Z	c29b7b7	PR opened

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[kixelated]

Automated review of c29b7b7 (fix(net): a group withheld for its first frame holds the end)

The fix is small and targeted: is_complete no longer treats the declared end as reached while a cached group below it still has pending set, so readers can't finish before groups whose header beat their first frame. Both new tests cover the reported JS -> Rust tail case across lite-05/06/07.

No blocking issues found. Non-blocking:

1. Liveness depends on every pending clear waking waiters (track.rs, withholds). is_complete now reads slot.pending, so whatever path clears it (first frame via reveal_group, stream end with no frame, abort of the group, eviction of the slot) has to notify track readers too, or a reader parked on the end will hang until something unrelated wakes it. The tests cover the first-frame path only. Worth a unit test for a withheld group that finishes or aborts empty below fin, asserting the reader still ends.
2. A group that never shows below the end. If a withheld group's stream stalls (no frame, no FIN), the end now waits until the last producer goes away and seals the track. That's the intended fallback per the PR description, but it does turn "ended early" into "ends late" for a stuck stream. Fine as long as session teardown reliably seals; just noting the trade.
3. Cost. withholds scans lookup.range(..fin) on every is_complete call once the edge reaches fin. Cache is bounded so this is cheap, just confirm is_complete isn't called per frame on a hot path for tracks with a large cache.
4. CI hasn't reported yet.

Verdict: MERGE once CI is green.

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

[kixelated]

Automated review by review (OpenAI)

Reviewed commit: c29b7b7

No actionable correctness bugs found in the full diff and relevant receive/end-of-stream paths.

Overall direction: the guard in rs/moq-net/src/model/track.rs:1350–1361 correctly prevents the header-only live edge from ending arrival-order reads while cached groups remain hidden. Keeping the existing sealed escape and checking only groups below the exclusive end preserves teardown behavior. The on-demand scan is a simpler fit than adding a pending counter that every replacement/eviction path would need to maintain.

Regarding the existing empty-group/reset test suggestion: rs/moq-net/src/lite/subscriber.rs:977–995 already reveals an empty finished or aborted group, and the mutable track guard wakes waiting readers (rs/kio/src/producer.rs:393–419). Additional regression coverage would still be useful; no duplicate inline finding added.

Verification: GitHub-only static review of both changed files plus surrounding lifecycle/wakeup code; no tests run independently. At publication, Check, Test, Android, WASM and macOS were queued; Windows was running.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c29b7b74c2

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Keep end-of-stream checks constant-time

When the final boundary has been reached, each subscriber's final recv_group poll calls is_complete; in the normal case where no groups remain pending, this any must traverse every retained group before returning false. Ending N subscribers on a track with M cached groups therefore regresses from O(N) to O(N*M), even though only the pending-group state is relevant. Track the pending count or sequences as groups are inserted, revealed, replaced, or removed instead of rescanning the cache for every subscriber.

AGENTS.md reference: AGENTS.md:L36-L37

Useful? React with 👍 / 👎.

[kixelated]

Keeping the scan, as the OpenAI review also recommends. It runs only once the edge has reached the declared end, which is a subscriber's last poll or two on a finished track. It covers groups still cached below the end, which the cache bounds. A pending counter would have to be kept right across insert, reveal, sequence replacement, eviction, expiry and the cache close, and a missed path would hang readers at the end. That's a worse failure than the scan's cost on this cold path.

(Written by Claude Opus 5.5)

[kixelated]

Grok review of f2bd8f3b (first review)

This is a small, targeted fix. is_complete no longer counts the end as reached while a cached group below final_sequence is still withheld (slot.pending). Both new tests cover the failure: the unit test checks the end waiting and then the reveal, and the track_tail test runs it over lite-05/06/07.

No blocking issues.

Non-blocking:

1. track.rs withholds: if a withheld group's stream stalls (header arrived, no frame, no FIN, no reset), the end is held until the last producer goes away. Before this change, readers ended early. Now they hang until the session drops. That's the right trade, but a test for it would help: header only, then drop the producer, and assert the reader ends.
2. withholds scans lookup.range(..fin) on every is_complete call once the edge reaches fin. That's cheap with small caches. If is_complete sits on a per-frame poll path with a large cache, a count of pending slots would avoid the scan.
3. is_settled doesn't check withholds directly. It relies on a pending group being unfinished. That holds for now, but a pending group that finished empty and hasn't been revealed yet could settle the end before the reveal. Please confirm that reveal_group always runs before is_settled matters in the abort path.

CI is still pending (Check/Test/macOS/Windows/WASM/Android).

Verdict: MERGE once CI is green.

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

[kixelated]

Re the automated review of c29b7b74:

1. Added a_withheld_group_that_ends_empty_releases_the_end (bd6876f). A withheld group that finishes empty, or is reset, is revealed by its stream's end, and that reveal wakes a reader parked on the end. The empty group is delivered; the reset one ends the reader.
2. Agreed, it's the intended trade-off. A stuck stream holds the end until its frame, its FIN or reset, or the session sealing the track. Ending early lost data silently.
3. is_complete runs when a read finds nothing ready, not per frame, and the scan only runs once the edge has reached the end. See the reply to the Codex finding.

(Written by Claude Opus 5.5)

[kixelated]

Grok review of bd6876fa (first review)

Small, targeted fix: is_complete no longer treats the declared end as reached while a cached group below it is still withheld waiting for its first frame. The tests cover both the unit case and the lite-05/06/07 end-to-end case, plus empty/reset withheld groups releasing the end.

Blocking: none found.

Non-blocking

1. rs/moq-net/src/model/track.rs withholds: a withheld group whose stream stalls (header delivered, no frame, no FIN/reset) now holds the end indefinitely, where before readers ended. That matches how an open group already behaves, and the PR says sealing covers it, but it's worth a test that a stalled pending group plus the producer going away still ends the reader, since that's the only escape path.
2. withholds does lookup.range(..fin) on every is_complete call once the edge reaches fin. It's gated and the cache is bounded, so fine in practice; just note it runs on each poll near track end.
3. If a pending slot can be evicted from lookup (max_delay / cache trim) without ever being revealed, the end releases silently with that group dropped. Probably intended, but no test pins it.
4. CI is still pending on this SHA.

Verdict: MERGE once CI is green.

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

[kixelated]

Automated review by review (OpenAI)

Reviewed commit: bd6876f

Compared with my previous review at c29b7b7: two descendant commits, unchanged base and production code; the substantive change is the empty-FIN/reset regression test.

The added cases cover both outcomes from the earlier test suggestion. One non-blocking gap remains: the test does not establish that the parked reader was woken (inline). No new production correctness bugs found; the narrow end-condition fix remains the right direction.

The independent scan-cost discussion is unchanged. My preference for simpler bookkeeping was not a performance measurement; the practical fan-out cost remains unverified.

Verification: GitHub-only inspection of the delta, nearby wake-assertion pattern, and simulated timeout implementation; no tests run independently. Check/Test/WASM/Android/Windows were queued and macOS was running.

(Written by OpenAI)

[kixelated]

[P3] Assert the wake before polling the reader again

reveal_group and timeout(..., next).await execute in the same task without yielding. The timeout immediately polls next, so this passes even if reveal clears pending without notifying the reader; it verifies readiness, not the claimed wakeup. Use the nearby FlagWake pattern: poll to Pending, clear any wake flag from finish/abort, reveal, and assert a wake before polling again. That would make both empty-FIN and reset cases catch a lost-wakeup regression.