fix(net): bound reach by the immediate successor, and collapse snapshots mid-roll - #3167
Conversation
…ots mid-roll Follow-ups from the post-merge review of #3099. Reach resolved to the next *stamped* group, so an unstamped immediate successor let a later group's timestamp shrink the bound in the unsafe direction: a rewind could expire content that was still inside the budget. Reach is now unbounded until the immediate servable successor presents its first frame, in Rust and JS, and a held group's expiry re-evaluates when that successor gains its timestamp. The JS snapshot consumer only collapsed to the newest group when acquiring one, so a held group's buffered deltas replayed superseded state across a roll; it now abandons the held group whenever a newer one exists, mirroring the Rust consumer. The draft's Expiration summary said "exceeds" where the rule is an exclusive bound, and SUBSCRIBE still resolved starts from a group's first frame; both now defer to the reach rule so start selection and expiry cannot disagree. Ordered.readFrame's doc no longer claims it discards older groups. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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: d8dcea0e71
ℹ️ 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".
| ## Expiration | ||
| Expiration governs when an older group is dropped. | ||
| The publisher SHOULD reset Group Streams for non-latest groups whose age relative to the latest group exceeds `Subscriber Max Age` (see [SUBSCRIBE](#subscribe)); the subscriber MAY also locally drop such groups. | ||
| The publisher SHOULD reset Group Streams for non-latest groups whose age relative to the latest group reaches `Subscriber Max Age` (see [SUBSCRIBE](#subscribe), and the age definition below); the subscriber MAY also locally drop such groups. |
There was a problem hiding this comment.
This now specifies expiration when age reaches Subscriber Max Age, but the Subscriber Max Age field definition still says a publisher resets groups only when their age exceeds the duration (draft-lcurley-moq-lite.md:969-972). At exact equality, including the important zero-budget case, implementations following the field definition can retain a group that the normative Expiration section and both implementations consider expired. Update the field definition to use the same inclusive threshold.
Useful? React with 👍 / 👎.
1 participant
Follow-ups from the post-merge review round of #3099 (these fixes were verified locally but not yet pushed when that PR merged).
Fixes
TrackState::reach, replacing the per-poll suffix table) and JS (Subscriber.#reach). A held group's expiry also registers on every unstamped servable group above it, so the verdict re-evaluates when the immediate successor gains its first timestamp.next()now abandons the held group whenever a newer one exists, mirroring the Rust consumer's drain-to-newest poll.draft-lcurley-moq-lite): the Expiration summary said age must exceed Max Age where the reach rule is an exclusive bound (>=), and the SUBSCRIBE section still told publishers to resolveGroup Startfrom a group's first frame. Both now defer to the Expiration rule, keeping the promise that start selection and expiry cannot disagree.Ordered.readFramedoc: no longer claims it discards older groups; it rides the sequence cursor and drains the backlog like every other ordered read.Regression tests
an_unstamped_immediate_successor_leaves_reach_unbounded(rs) /an unstamped immediate successor leaves reach unbounded(js)a held group is abandoned when a newer snapshot group rolls(js/json)Test plan
cargo nextest run -p moq-net: 1014 passed.bun testin js/net and js/json: green.just drafts checkpasses. The fulljust check/just testsweep is running; CI on this PR is the gate either way.(Written by Claude Fable 5)