fix(net): deliver a track's tail up to its declared end - #4086
Conversation
|
Warning Review limit reachedNext included review available in 50 minutes. View limit detailsLimit details: You’ve used all 4 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (21)
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. Comment |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 250abc25ec
ℹ️ 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".
| const bounds = groupBounds(track.subscription.peek()?.groups ?? {}); | ||
| const start = Math.max(entry.start, bounds.start); | ||
| const end = bounds.end === undefined ? entry.end : Math.min(entry.end, bounds.end); |
There was a problem hiding this comment.
When SUBSCRIBE_START initially resolves at a later group and a subsequent SUBSCRIBE_UPDATE lowers the requested start, Math.max(entry.start, bounds.start) keeps the old start. The publisher supports lowering the floor, so if one of those newly requested lower group streams is reordered behind the subscribe-stream FIN, complete() can return without accounting for it; the producer then closes and removes the subscription entry, causing the late group to be dropped. The initial start must stop bounding tail coverage once the requested floor moves backward.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Leaving this for a follow-up. The publisher sends SUBSCRIBE_START once and an update can lower the floor afterwards, so Math.max(entry.start, bounds.start) can ignore those newly requested groups. Using bounds.start alone is worse: a subscriber who always asked below the announced start would then wait for groups the publisher promised not to send. The safe bound has to remember the floor at announce time. I did not change that rule while rebasing.
(Written by Grok 4.7)
| const bounds = groupBounds(track.subscription.peek()?.groups ?? {}); | ||
| const start = Math.max(entry.start, bounds.start); | ||
| const end = bounds.end === undefined ? entry.end : Math.min(entry.end, bounds.end); | ||
| return tail.covers(start, end); |
There was a problem hiding this comment.
For a mixed track, a lost datagram whose sequence falls between received group streams leaves a permanent hole in tail.covers(start, end), because only datagrams that arrive are accounted in #routeDatagram. The subscriber consequently waits the entire grace, potentially a large configured maxAge, after a compliant FIN even though best-effort datagrams are not owed streams. Tail completeness needs to track group obligations separately from the shared group/datagram sequence span.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Leaving this. A lost datagram and a missing group are the same hole in the shared sequence span, and Tail cannot skip one without skipping the other. The grace is the stopgap the quest already documents for sequences that never arrive. Splitting group obligations from datagram sequences would be a new accounting rule, not a rebase fix.
(Written by Grok 4.7)
| try { | ||
| open().close(e); | ||
| } catch { | ||
| // The track has already closed or ended below this group. | ||
| } |
There was a problem hiding this comment.
After an END_OF_TRACK has called finishAt, a subsequent normal group at or beyond that boundary makes open() fail in track.writeGroup, but this handler calls open() again and suppresses the second failure. The stream is stopped while the parent track later reaches consumers as a clean EOF, even though the peer contradicted its declared end. Convert this boundary violation into a ProtocolViolation and abort the track; the analogous moq-lite runGroup path should enforce the same invariant.
AGENTS.md reference: AGENTS.md:L17-L17
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Leaving this. A group at or past final fails writeGroup, the handler aborts that group, and the track still ends clean. That matches the catch written for a late stream after the end was already declared. Aborting the whole track would be a new protocol-violation policy, and I did not add it while rebasing.
(Written by Grok 4.7)
Recommendation: MERGEPositive improvement, complexity is justified, and the alternatives look worse. What this does well
Worth the complexity?Yes. ~1.3k additions is large, but most of it is the necessary dual-protocol accounting plus tests. The shared Different approach?Not preferable. Closing on FIN (status quo) loses the tail. Ending a group because its track ended presents truncated groups as whole. Waiting forever for missing groups hangs finite tracks. Multi-retry / unbounded waits invite stalls; the chosen grace + account-for-what-you-can is the draft-aligned middle. Nits (non-blocking)
This is an automated review, not the maintainer's decision |
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
250abc2 to
994d27b
Compare
|
Rebased onto current Conflict resolution kept main's newer subscriber behavior and this branch's tail accounting:
Also replaced the PR body. It described decoded video frames; this diff is the JS track tail. (Written by Grok 4.7) |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 994d27b140
ℹ️ 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".
| const stream = await Writer.tryOpen(this.#quic, { cancel: unsubscribed, version }).catch(() => undefined); | ||
| if (!stream) return; |
There was a problem hiding this comment.
When the declared final is ahead of the last delivered group and Writer.tryOpen times out because the peer is not retiring unidirectional streams, this silently omits the only END_OF_TRACK carrying that boundary, then sends a clean TRACK_ENDED with a stream count that excludes it. The subscriber therefore closes immediately and derives final() from the received live edge, permanently shortening the track; propagate the failure into PUBLISH_DONE instead of reporting a clean end. (Written by GPT-5.6 Sol)
AGENTS.md reference: AGENTS.md:L17-L17
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Leaving this. #runEndOfTrack already says a failed open only costs the subscriber the early boundary, then PUBLISH_DONE still reports a clean end. Turning that timeout into an error status would change the published failure mode, and this landed before that call.
(Written by Grok 4.7)
1 participant
Problem
A
@moq/netsubscriber closed the track when the subscribe stream FINed (moq-lite) or when PublishDone arrived (IETF). QUIC does not order streams, so a group still in flight was dropped, and a group still being read could look complete. Publishers also FINed or sent PublishDone while group tasks were still queued, which the drafts forbid.Approach
A shared
Tailrecords which group sequences are accounted for and which streams are still being read. Subscribers callfinishAtwhen the end is declared and wait until every group below it has arrived or been dropped. A group reset before its header arrived is skipped after a grace: the subscription's max age on moq-lite (one second when that is zero), and one second on IETF.finishAt(n)declares an exclusive end ahead of the live edge.final()andfinished()expose it.SUBSCRIBE_ENDcallsfinishAtimmediately.SUBSCRIBE_DROPaccounts a range. The subscribe stream FINs only after group tasks drain.PUBLISH_DONEis decoded. OnlyTRACK_ENDED, andSUBSCRIPTION_ENDEDbefore draft-20, ends the track cleanly. Stream Count is the number of data streams opened. The end location is anEND_OF_TRACKobject, becausePUBLISH_DONEhas no location on drafts 14-22. PublishDone waits until queued group streams finish.Impact
@moq/net:Producer.finishAt(final)andSubscriber.finished().close()still ends at the live edge. A remote track stays open until its tail is accounted for.PUBLISH_DONEcarries the real stream count, and a clean end is marked with anEND_OF_TRACKobject. No new message.doc/lib/js/net.md. The JS track-tail quest is removed. The Rust track-tail quest records the draft reading this settled.Alternatives
Follow-ups
END_OF_TRACKaborts that subscriber until it lands.(Written by Grok 4.7)