feat(net)!: resolve the subscribe start from max age, with Group Start as an absolute floor - #3158
Conversation
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: fbaed9a8ea
ℹ️ 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".
| // The ring places samples by timestamp, so a group that arrives below the delivery | ||
| // cursor still lands in its own slot. That is how the head a publisher serves ahead | ||
| // of the live edge survives being sent after it (groups go newest-first). | ||
| outOfOrder: true, |
There was a problem hiding this comment.
When a nonzero maxAge causes the publisher to send the live edge before older cached groups, this option forwards the encoded chunks to the same AudioDecoder in arrival order, such as group 3 followed by 0, 1, and 2. The timestamp-indexed ring only orders the already-decoded PCM; it cannot restore the predictive/overlap state of codecs such as Opus or AAC, and the decoder is then left at an older codec state before subsequent live chunks arrive. Reorder the encoded groups before decoding, or fix the serving order, rather than enabling out-of-order input at this call site.
AGENTS.md reference: AGENTS.md:L133-L137
Useful? React with 👍 / 👎.
| // the default zero budget: a subscriber that tolerates some age gets the head of what | ||
| // it can still use, rather than joining at the live edge and discarding a track's | ||
| // opening groups that are sitting right here in the cache. | ||
| let resolved = resolved_start(&track, ctx.version); |
There was a problem hiding this comment.
When a SUBSCRIBE omits Group Start but supplies an end_group below the current live edge, resolved_start runs before track.end_at, so fresh_start measures freshness against groups the subscription cannot receive. For example, with live group 4, end group 2, and a short budget, this resolves to 3 or 4 and then creates an empty range instead of resolving against group 2 as the edge. Apply the cap first; the JS mirror in js/net/src/lite/publisher.ts has the same ordering and needs the matching fix and regression coverage.
AGENTS.md reference: AGENTS.md:L191-L191
Useful? React with 👍 / 👎.
| endGroup: msg.endGroup, | ||
| }); | ||
| const startGroup = msg.startGroup ?? track.latest(); | ||
| const startGroup = msg.startGroup ?? resolvedStart(track, this.version); |
There was a problem hiding this comment.
This changes the meaning of an omitted start when maxAge is nonzero, but the exported Subscription.startGroup member in js/net/src/track.ts still says omission starts at the latest group. That stale comment is emitted into the package's public API documentation and will mislead callers about the newly introduced backfill behavior; describe publisher selection based on maxAge there as well.
AGENTS.md reference: AGENTS.md:L107-L109
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0ef1346731
ℹ️ 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".
| if let Some(start_group) = bounds.start_group.or_else(|| track.latest()) { | ||
| track.start_at(start_group); | ||
| } | ||
| position_cursor(&mut track, ctx.version, bounds.start_group); |
There was a problem hiding this comment.
When a relay cached groups out of sequence, such as 3 followed by 0, 1, and 2 from independently delivered upstream streams, resolve_start correctly chooses 0 for a sufficient max-age budget, but this cursor still reads in arrival order. TrackRun::poll therefore receives 3 first, emits SUBSCRIBE_START(3), and calls start_at(3), permanently discarding the cached groups that the new max-age resolution selected. Preserve the model-resolved floor until the advertised start is determined from the full eligible range, rather than replacing it with the first arrival.
AGENTS.md reference: AGENTS.md:L45-L49
Useful? React with 👍 / 👎.
…x age A SUBSCRIBE that names no Group Start meant the latest group, so a subscriber joining a track already in flight skipped everything the publisher was still holding for it. That is invisible on a continuous broadcast and obvious on a publisher that opens a track per utterance: the head of every track is discarded while it sits in the cache. Resolve the start from the budget the subscriber already sent instead. Max Age says how far behind the live edge a group may be before it is skipped, so the oldest group worth starting at is the oldest one that budget does not already convict. A zero budget (the default) still resolves to the latest group, since every older group is stale the moment a newer one exists, so an existing subscriber sees no change. The two bounds cannot disagree: the same predicate that expires a group on the way out picks where to begin, so a publisher never sends history the subscriber would discard on arrival, nor withholds a group it would have taken. A wire that cannot carry Max Age is served with an unbounded budget so backlog is never dropped under a legacy subscriber, and that must not read as a request to replay the cache, so those sessions keep resolving to the latest group. Groups go out newest-first, so the resolved head arrives after the live edge that was served alongside it. The container consumer dropped anything below its delivery cursor, which threw the head away again one layer down. Audio can take those: the ring is timestamp indexed, so a late group lands in its own slot however it arrives. Video cannot, so the strict cursor stays the default and audio opts in. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…helper The start a subscription resolves from its own max age was a method on track::Subscriber that every serving path had to remember to call, and a publisher that forgot got the old live-edge behaviour silently. It is also nothing a caller outside the model has any use for. Resolve it where a subscription becomes a cursor instead. A new subscriber starts at the group its subscription named, or the oldest cached one its max age still considers fresh, so the model answers the question once for every serving path (and honours an explicit start without the publisher translating it). The public surface goes back to what it was: no new API on either side. The lite publisher keeps one line of its own, for a wire that cannot carry Max Age: those sessions are served an unbounded budget so a legacy subscriber never has backlog dropped under it, and the cursor that budget would resolve to is the whole cache. A FETCH scan on the JS side walks a subscriber from the cache head, so it needs the exemption the Rust fetch path gets structurally: renamed the internal hook to say what it now covers, since a fetch names one old group explicitly and is neither late against the live edge nor bound by a live subscription's start. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Groups are sent newest-first, so the head of a subscription arrives after the live edge it was served alongside. The container consumer dropped anything below its cursor, which threw that head away one layer below where the publisher had just served it. Both consumers can place a late group. Audio writes into a timestamp-indexed ring, which ignores anything behind the playhead and fills its own slot otherwise. Video drops a late frame at render, by timestamp, twice on the way. So there is no caller the strict cursor was protecting, and no reason for the opt-in this started as. How far back a group may be is the subscription's own max age, applied before it reaches here. A group the reset boundary proves reneged is still dropped, since that is a timeline the publisher withdrew rather than one that arrived late. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1bd666e to
49b5027
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 49b5027dac
ℹ️ 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".
| **Group Start**: | ||
| The first group to deliver. | ||
| A value of 0 means the latest group (default). | ||
| A value of 0 means the publisher chooses (default). |
There was a problem hiding this comment.
When Subscriber Max Age is nonzero, this new rule can start before the latest group, but a repo-wide search still finds the old guarantee in this draft's Track overview (line 129) and Appendix B (line 1438), plus the published Rust and binding surfaces in rs/moq-net/src/model/subscription.rs, rs/moq-ffi/src/consumer.rs, and rs/libmoq/src/api.rs. Beyond the already-noted JS member, these are additional stale surfaces that give implementers and Rust/C/binding consumers a contract contradicting this normative text, so update them together.
AGENTS.md reference: AGENTS.md:L194-L200
Useful? React with 👍 / 👎.
|
Reviewed the behavior change end to end (model resolution, both wire publishers, the js container consumer, the draft text) and reproduced the failure modes locally. The direction is right: deriving the join point from the budget that already expires groups is the correct core idea, and it is strictly better than IETF 1. CI is red, and the failing test is a real second constituency, not a stale assertion
It is not fixable by updating the test, because the new wire cannot express the old intent at all: Your own "Follow-ups" section documents the same conflation from the other side: Recommendation: split the knob. Add a separate join bound (e.g. 2. The js container consumer truncates a below-cursor group that is still downloading
Repro (local test): live group 3 delivering, backlog group 1 arrives and delivers its first frame; the very next 3. The js container consumer splits the open live group around the backlog, which video cannot decode
Recommendation for both: rather than admitting out-of-order groups into a decoder-bound path and patching the cursor rules, anchor the consumer's start deterministically. The publisher already announces the resolved start as SUBSCRIBE_START (the js subscriber currently drains it as "informational" in 4. Every Rust consumer downloads the backlog and throws it away
5. The new semantics apply to published draft versions
6. Smaller notes
On the IETF questionAgreed it should be an extension, and combining it with the timestamp draft is right: "age" only exists once objects carry timestamps at a declared timescale, so the dependency is structural, not incidental. Shape it as SUBSCRIBE parameters in draft-lcurley-moq-timestamp ( (Written by Claude Fable 5) |
Reworks the resolution design per review: the model no longer scans the cache to resolve a start. A subscription's named start is a floor (min group), the budget is the only thing that asks for data, and delivery's existing staleness gate decides what above the floor is worth sending. A zero budget still delivers only the latest group; a larger one reaches back over the head of what the subscriber can still use. A late lower group within the budget is now delivered rather than suppressed, which is what the resume layer's pruned-cursor semantics always required. On the lite-06 wire, Group Start becomes the raw absolute floor (default 0) instead of sequence + 1 with 0 meaning the latest group. A vacuous floor (0 with no frame offset) canonicalizes to absent on decode, and folds back to absent when re-encoded toward a pre-06 wire, whose explicit group 0 means "replay from the beginning". Group 0 stays nameable when a Frame Start qualifies it, so a mid-group-0 resume (a catalog never leaves group 0) survives. Pre-06 wires are pinned to the latest group when they name no start, matching their published drafts; only lite-06 resolves from the budget. The spliced (origin-backed) serving path now honors floors at all: poll_activate assigned the inner cursor's start, discarding the floor the inner subscription carried, so an explicit start never survived an origin. It raises the floor instead (track::Subscriber::raise_start_to). The broadcast_route_migration test names its floor explicitly, which is the migration semantics the old default start was standing in for. The hang container consumer no longer truncates a below-cursor group whose buffer momentarily drains mid-download: removal now waits for the group to finish, with the latency skip still bounding a stalled head. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 42f98a79c4
ℹ️ 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".
| /// partway through group 0, e.g. a catalog that never leaves it). | ||
| fn canonical_start_group(version: Version, start_group: Option<u64>, start_frame: u64) -> Option<u64> { | ||
| match (start_group, start_frame) { | ||
| (Some(0), 0) if version.resolves_start() => None, |
There was a problem hiding this comment.
On lite-06, an update that widens Group Start from a positive value to the unfloored value 0 is canonicalized to None, but both publisher update handlers only call start_at/startAt when the decoded value is present. If the update is processed before the next group pop, the cursor therefore retains its previous positive floor and groups below it remain unavailable despite the widened request. Apply 0 explicitly for an absent lite-06 update in both this Rust path and the JS mirror in js/net/src/lite/publisher.ts.
AGENTS.md reference: AGENTS.md:L196-L200
Useful? React with 👍 / 👎.
kixelated
changed the title
|
Reworked per direction (single Two genuine bugs fell out of chasing the CI failure, both fixed here:
Wire details worth a look: on lite-06 a vacuous floor ( Left out on purpose: (Written by Claude Fable 5) |
…-ordered-handle Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 participant
Summary
A subscriber with a 3s jitter buffer used to start empty and spend 3s filling it: an absent
Group Startmeant "the latest group", so a join always began at the live edge no matter how much older content the subscriber could still play. NowSubscriber Max Ageis the only thing that decides where delivery begins, and a join is handed the head of what it can still use.Group Startis an absolute floor on lite-06. The wire carries the raw minimum group sequence (default 0), replacing thesequence + 1encoding where 0 meant the latest group. A floor is not a request: it only bounds how far back the budget may reach, so migrations and range requests still pin exactly where they need to without dragging backlog in.Group Start = 0withFrame Start = 0decodes as no floor; group 0 stays nameable when aFrame Startqualifies it, so a mid-group-0 resume survives (a catalog never leaves group 0). Aggregation folds floors accordingly: the loosest floor wins, and any subscriber without one clears it.Bug fixes surfaced by the review
resume::poll_activateassigned each segment's inner cursor from the segment boundary, silently discarding the start the inner subscription carried, so an explicitGroup Startevaporated on any track served through an origin. It now raises the floor instead (track::Subscriber::raise_start_to). This was the actual mechanism behind thebroadcast_route_migrationCI failure; that test now names its floor explicitly, which is what its 10s tolerance was standing in for.next()shifted it out and its remaining frames were silently lost (withcontinuousstill reporting true). Removal now waits for the group to finish, with the latency skip still bounding a stalled head. Regression-tested with an incremental mid-download arrival.Out-of-order delivery
Groups go out newest-first, so the head of a join arrives after the live edge served alongside it.
Container.Consumerdropped anything below its delivery cursor, which threw that head away one layer down; it now admits it, since the subscription's own max age already bounds how far back one can be, and the rendering consumers place content by timestamp. A group the reset boundary proves reneged is still dropped. Consumers that need strict decode order are #3099'sOrderedcursor's job, not this consumer's.Public API changes
Subscription::start/startGroupsemantics change from "start exactly here" to a floor (which is why this targetsdev), and aggregation folds it as one (loosest wins, absent clears). No items added or removed;moq-ffi/libmoqdoc comments updated to match (no signature change, so the generated bindings are untouched).Test plan
cargo nextest run -p moq-net -p moq-tokio -p moq-relay -p moq-mux -p hang -p moq-gst(2429 tests),cargo clippyover the changed crates plusmoq-ffi/libmoq/moq-cli,cargo fmt --check,bun test js/{net,hang,watch}/src(875 tests),bunx tsc -b,biome check, andjust drafts check.New coverage: the budget reaching back over the cache, a named start as a floor (cut off, vacuous, and above-the-edge cases), a late lower group within the budget delivering, the pre-06 pin per version (unbounded lite-01 budget, declared lite-05 tolerance, lite-06 resolution), the raw-vs-
+1wire encodings with the vacuous-floor canonicalization and pre-06 fold in both languages,Frame Startqualifying group 0, floor aggregation clearing on an unfloored subscriber, and the container consumer's mid-download truncation regression.Follow-ups, deliberately not here
rs/moq-mux's container consumer still drops below-cursor groups, so a native consumer subscribing with a budget over lite-06 transfers the join backlog and discards it. The right shape for consumers that need decode order is refactor(net)!: make group delivery order a handle, and retire the Ordered wire field #3099'sOrderedcursor rather than porting the admission rule, so this waits for that to land.devtoday.subscribeMedia'smaxAge(the buffer ceiling) still decides the join depth; whether a buffered viewer should joinmaxBufferbehind a real-time publisher is a pre-existing choice with its own trade-off.Supersedes #3114, which asked for
startGroup: 0from the client with a bound nothing enforced.(Written by Claude Fable 5)