feat(moq-net): enforce the subscriber latency budget - #2890
Conversation
![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: b80213e295
ℹ️ 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".
| // Drop a group the drift budget has given up on and keep scanning, so one | ||
| // poll walks a whole backlog off rather than handing it out group by group. | ||
| if ready!(self.poll_stale(&consumer, drift, waiter))? { | ||
| self.stale += 1; | ||
| continue; |
There was a problem hiding this comment.
The new subscriber-side enforcement exists only in Rust. The corresponding js/net/src/track.ts methods Subscriber.recvGroup, nextGroup, and readFrameSequence still return buffered groups without consulting Subscription.latencyMax or comparing timestamps. Browser subscribers therefore continue replaying a backlog whenever another subscriber widens the publisher's aggregate budget, contrary to the newly documented both-end semantics. The repository explicitly requires rs/moq-net API changes to be synchronized with js/net. (Written by GPT-5.6 Sol)
AGENTS.md reference: AGENTS.md:L170-L177
Useful? React with 👍 / 👎.
| let (latency, end) = { | ||
| let prefs = self.subscription.read(); | ||
| (prefs.latency, prefs.end) | ||
| }; | ||
| let cap = servable_cap(self.end_sequence, end, self.stale_cap); |
There was a problem hiding this comment.
When this subscriber has a Subscription::end, poll_drift treats that wire preference as a local delivery cap. However, subscription bounds are aggregated and do not filter this handle, so another unbounded subscriber can cause groups beyond this end to be received here. In that case the live edge remains pinned at the requested end, every later candidate has a sequence above the edge and is therefore never stale, allowing an arbitrarily old backlog through even with the default real-time budget. Only end_sequence and the outer reader cap should constrain the local edge. (Written by GPT-5.6 Sol)
Useful? React with 👍 / 👎.
| if self.is_stale(*sequence, drift.edge, drift.budget) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
When callers use Subscriber::read_frame, stale groups are discarded inside TrackState::poll_read_frame by this branch, but the skipped count is never returned to PlainSubscriber::stale. Consequently the subsequent count_stale call records zero, so the published Traffic::stale metric under-reports precisely for consumers using the frame-level helper. This occurs whenever one or more stale groups precede the frame that is returned. (Written by GPT-5.6 Sol)
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d32e6e27ef
ℹ️ 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".
| let subscription = self.track.subscription().with_latency(latency); | ||
| let _ = self.track.update(subscription); |
There was a problem hiding this comment.
When a caller requests nonzero latency through an exporter or decoder, the track has already been subscribed with the default preferences before this post-accept update runs (for example, container/source.rs subscribes at lines 198-218). On a remote Lite publisher, that initial REAL_TIME subscription can walk off the cached backlog and advance its cursor while the subscription is being established; this later update cannot re-offer those skipped groups. As a result, recorders configured with a larger latency budget can still start at the live edge, so carry the configured latency in the initial Subscription instead.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f43edc839e
ℹ️ 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 self.is_stale(*sequence, drift.edge, drift.budget) { | ||
| stale += 1; | ||
| continue; |
There was a problem hiding this comment.
When a completed track's remaining groups are all stale, this scan increments its local stale count and then returns Ok(None) without producing a FrameHit. PlainSubscriber::poll_read_frame only transfers hit.stale from the Some path, so the public wrapper records zero skips. The new FrameHit propagation fixes the previously reported case where a newer frame is returned, but the terminal path still causes Traffic::stale to under-report whenever read_frame skips the tail of a finished track. (Written by GPT-5.6 Sol)
AGENTS.md reference: AGENTS.md:L100-L100
Useful? React with 👍 / 👎.
| when the datagram enters or leaves the model, so an egress datagram dropped by | ||
| congestion or an oversized body still counts. | ||
|
|
||
| - `stale`: cumulative groups skipped because they drifted further behind the live |
There was a problem hiding this comment.
I think stale should be split into { bytes, frames, groups, datagrams } as well. The size of a group is quite arbitrary.
We should probably move the existing counters into a transmitted section or something IDK. But keep backwards compatibility with the current scheme for now.
| /// These tests publish a whole feed before exporting it, which the default | ||
| /// [`Latency::REAL_TIME`](moq_mux::Latency::REAL_TIME) collapses to the live edge: | ||
| /// completeness has to be asked for, exactly as a real recorder does. | ||
| const BATCH: std::time::Duration = std::time::Duration::from_secs(3600); |
There was a problem hiding this comment.
3600 seconds seems reaaally high? I don't understand what we're doing here. At least use something reasonable, like 30s IDK.
| /// These tests write or import a whole broadcast and only then export it, which the | ||
| /// exporter's default [`Latency::REAL_TIME`](crate::Latency::REAL_TIME) collapses to the | ||
| /// live edge: completeness has to be asked for, exactly as a real recorder does. | ||
| const BATCH: std::time::Duration = std::time::Duration::from_secs(3600); |
There was a problem hiding this comment.
I don't understand this name. why BATCH?
| // The whole track is written before the exporter runs, so it needs a budget | ||
| // wide enough to read it: the default skips everything but the live edge. | ||
| let mut export = Export::new(crate::source::announced(&consumer), Once(Some(catalog))) | ||
| .with_latency(crate::Latency::max(std::time::Duration::from_secs(3600))); |
There was a problem hiding this comment.
3600 seconds seems reaaally high? I don't understand what we're doing here.
| /// Both layers enforce the same budget, and normally should: the transport skipping | ||
| /// a group the consumer was going to skip anyway just saves the bandwidth. These | ||
| /// tests are the exception, since they are about the consumer's half of it. | ||
| fn consumer_only(track: moq_net::track::Subscriber, latency: Latency) -> Consumer<Container> { |
There was a problem hiding this comment.
confusing name.
The eventual plan is to move the Container ordered consumer into moq-net. So moq-net would be responsible for skipping late groups/frames based on the transport timestamp, not the container timestamp. But that requires moq-lite-05 so we're waiting for a liiiitle bit first.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 459328a34d
ℹ️ 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".
| let Some(timestamp) = slot.group.timestamp() else { | ||
| return false; |
There was a problem hiding this comment.
When an older group opens but its first frame stalls while a newer stamped group arrives, this branch permanently exempts the older group from latency filtering. A raw subscriber can therefore receive that group and block reading it despite requesting REAL_TIME; it may remain blocked until retention eviction, or indefinitely if no later write drives another eviction scan. The same exemption exists in js/net/src/track.ts. The normative expiration rule in drafts/draft-lcurley-moq-lite.md:511-518 requires wall-clock age to backstop missing timestamps and explicitly covers empty groups, so track arrival time needs to participate in the stale decision. (Written by GPT-5.6 Sol)
AGENTS.md reference: AGENTS.md:L189-L189
Useful? React with 👍 / 👎.
| let subscription = self.track.subscription().with_latency(latency); | ||
| let _ = self.track.update(subscription); |
There was a problem hiding this comment.
When a decoder requests nonzero latency, updating the subscription here happens only after SUBSCRIBE_OK, so a remote publisher can already have served the initial default REAL_TIME request and skipped backlog that this update cannot recover. Fresh evidence after the earlier exporter fix is that rs/moq-video/src/decode/consumer.rs:41-49, rs/moq-audio/src/decode/consumer.rs:51-59, rs/libmoq/src/consume.rs:363-367, and rs/moq-gst/src/source/imp.rs:445-447 still subscribe without their configured latency and only apply it through this post-accept call. Include the latency in those initial subscriptions, or reshape construction so the container consumer's budget is available before subscribing. (Written by GPT-5.6 Sol)
AGENTS.md reference: AGENTS.md:L119-L123
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0396224d67
ℹ️ 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 (hooks.groupTimestamp(group) !== undefined && (!edge || group.sequence > edge.sequence)) edge = group; | ||
| } | ||
|
|
||
| const requested = this.#state.update.peek()?.latencyMax ?? 0; |
There was a problem hiding this comment.
When a browser viewer selects any nonzero latency, this still resolves to zero because the watch audio and video paths subscribe with only a priority (js/watch/src/audio/decoder.ts:275 and js/watch/src/video/decoder.ts:299) and pass their latency only to the later container consumer. The new transport filter can therefore discard every buffered group except the live edge before that consumer gets to apply its configured tolerance, breaking buffered playback and recovery from jitter. Fresh evidence after JS enforcement was added is that these production subscriptions still omit latencyMax; include the configured budget in the initial subscription and keep it synchronized when the setting changes. (Written by GPT-5.6 Sol)
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cbf92f27d6
ℹ️ 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 ready!(self.poll_stale(&consumer, drift, waiter))? { | ||
| self.stale.add(consumer.content()); | ||
| continue; | ||
| } | ||
| return Poll::Ready(Ok(Some(consumer))); |
There was a problem hiding this comment.
When a subscriber polls while an empty group is still the live edge, this path returns that group immediately; if its first frame then stalls and a newer group arrives, the latency check never runs on it again. In the Rust wire path, TrackRun moves the returned group into an independent GroupServe, and the JS publisher similarly starts #runGroup, neither of which monitors drift, so a zero-budget raw reader can block indefinitely and a publisher can leave the stale stream consuming flow control instead of resetting it. Fresh evidence beyond the earlier report is that the new wall-clock check still runs only before this handoff, while both current group-serving paths continue independently afterward. Apply the budget to already-delivered groups or explicitly cancel their serving tasks when they become stale, in both implementations.
AGENTS.md reference: AGENTS.md:L172-L177
Useful? React with 👍 / 👎.
| /// A group that has yet to present a frame is never skipped, and neither is one on a track | ||
| /// where nothing newer has: with no timestamp there is nothing to measure, and the frames | ||
| /// may simply not have arrived yet. Such a group is bounded by the publisher's retention | ||
| /// window instead, which aborts it once it ages out. |
There was a problem hiding this comment.
This new public Latency documentation says a group without a first-frame timestamp is never skipped and is bounded only by retention, but the implementation now deliberately marks it stale from the wall-clock arrival delta in TrackState::is_stale, as covered by wall_clock_expires_an_unstamped_group. Consumers reading the API docs will therefore get the opposite behavior for empty or stalled groups; describe the wall-clock backstop here as the concept documentation does.
AGENTS.md reference: AGENTS.md:L97-L99
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 95376e02d0
ℹ️ 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 drift = this.#drift(); | ||
| for (let i = 0; i < groups.length; ) { | ||
| const group = groups[i]; | ||
| if (end !== undefined && group.sequence > end) break; |
There was a problem hiding this comment.
When a caller sets endAt() but consumes through readFrame() or readFrameSequence(), this new break excludes every group above the cap and, after a clean track close, leaves the read waiting indefinitely for the cursor to change. The public endAt contract only caps nextGroup and recvGroup, and the Rust Subscriber::poll_read_frame still does not apply end_sequence, so this introduces a JS-only behavior change. Use the cap to bound the drift anchor without removing those groups from the frame reader's readable set. (Written by GPT-5.6 Sol)
AGENTS.md reference: AGENTS.md:L170-L177
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 716564a52e
ℹ️ 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".
| // Each segment keeps its own route-specific expiry policy. Applying the | ||
| // head segment's policy to the assembled group would use the wrong edge | ||
| // after a takeover. | ||
| expiry: None, |
There was a problem hiding this comment.
When a handed-out spliced group crosses a route takeover, leaving the outer expiry unset allows the latency budget to be bypassed. resume::Group::poll_current resolves subsequent segment copies directly through track.poll_peek_group, which returns an unguarded consumer, so after the original latched copy expires or fails, a replacement can continue serving the stale group without any further expiry checks. Fresh evidence beyond the earlier handed-out-group report is this replacement-copy path, which was not covered by attaching expiry only to the initially returned segment group.
Useful? React with 👍 / 👎.
| /// [`Poll::Ready`]; the track ending surfaces as the error the caller was going to | ||
| /// get anyway. | ||
| fn poll_drift(&self, waiter: &kio::Waiter) -> Poll<Result<Drift>> { | ||
| let latency = self.subscription.read().latency; |
There was a problem hiding this comment.
When poll_read_frame is pending because all completed buffered candidates exceed the old budget while the newest group has no complete frame, widening the latency through SubscriberControl::update should make those candidates readable. This direct read() does not register the supplied waiter on the subscription channel, however, so the pending read remains asleep until unrelated track activity occurs even though the next poll would accept the frame. Poll the subscription with the waiter, or otherwise propagate its updates to the read waiter.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f46193e71f
ℹ️ 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".
| /** True once the group has been closed, regardless of whether buffered frames remain unread. Synchronous complement to the {@link closed} promise. */ | ||
| get isClosed(): boolean { | ||
| return this.#state.closed.peek() !== undefined; | ||
| return this.#expiration() !== undefined || this.#state.closed.peek() !== undefined; |
There was a problem hiding this comment.
closed when a subscriber group expires
When a handed-out group becomes stale while its producer remains open, #expiration() makes isClosed true and causes frame reads to fail, but the public closed promise still points only at #state.closed and never settles. A consumer that uses await group.closed for lifecycle cleanup can therefore wait indefinitely after this handle has become terminal, contradicting the documented promise that isClosed complements. Represent expiry in the per-consumer closed signal as well. (Written by GPT-5.6 Sol)
AGENTS.md reference: AGENTS.md:L97-L99
Useful? React with 👍 / 👎.
| fn poll_expired(&mut self, waiter: &kio::Waiter) -> bool { | ||
| if !self.expired && self.expiry.as_ref().is_some_and(|expiry| expiry.is_expired(waiter)) { | ||
| self.expired = true; | ||
| self.stats.stale(self.content()); | ||
| } |
There was a problem hiding this comment.
When a caller reads part of a group and that group later exceeds its latency budget, self.content() snapshots the entire shared cache, including frames and bytes already returned by this cursor. Those payloads have already incremented the delivered counters, so adding the full snapshot to stale over-reports skipped traffic and violates the documented guarantee that stale and delivered content are disjoint. Compute the stale amount from this consumer's remaining cursor, and ensure cloned consumers cannot count the same delivery repeatedly. (Written by GPT-5.6 Sol)
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5f36cf71e5
ℹ️ 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 candidate = this.#state.timeline.get(group.sequence); | ||
| if (candidate?.group !== group) return false; |
There was a problem hiding this comment.
When publisher retention evicts a cleanly closed group that a subscriber already holds, #evict deletes this timeline entry and closes the mirror without an error, while its buffered frames remain readable. This check then treats the missing candidate as not stale, so the reader can continue draining content beyond both the publisher retention window and its clamped latency budget. Fresh evidence beyond the earlier handed-out-group report is this eviction path, which removes the data needed by the expiry guard before the next read; expire the mirror before removal or preserve a terminal expiry verdict. (Written by GPT-5.6 Sol)
AGENTS.md reference: AGENTS.md:L174-L177
Useful? React with 👍 / 👎.
| await group; | ||
| return; | ||
| } | ||
| await Promise.race([group, this.#expiry.changed()]); |
There was a problem hiding this comment.
When a reader consumes a long open group without newer groups arriving, each frame resolves group, but the losing this.#expiry.changed() promise remains subscribed because an outer Promise.race cannot cancel it. The next frame read creates another waiter, so streaming an indefinite catalog or data group accumulates track-signal callbacks linearly until some unrelated group, option, or track change finally releases them. Use one cancellable registration spanning both readiness sources, as Signal.race does internally. (Written by GPT-5.6 Sol)
AGENTS.md reference: AGENTS.md:L100-L100
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4870c58110
ℹ️ 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 wallStale = | ||
| drift.wall !== undefined && | ||
| drift.wall.sequence > group.sequence && | ||
| drift.wall.time - candidate.time > drift.budget; |
There was a problem hiding this comment.
When multiple groups are queued within the same millisecond and have equal timestamps, as commonly happens with synchronous writeString or writeJson calls, both calculated ages are zero, so these strict > drift.budget comparisons let every older group through under the default latencyMax: 0. A zero budget is documented to return only the latest group once any newer sequence exists; special-case zero rather than changing the positive-budget boundary, and make the corresponding change to the identical comparisons in rs/moq-net/src/model/track.rs:590-608. (Written by GPT-5.6 Sol)
AGENTS.md reference: AGENTS.md:L174-L177
Useful? React with 👍 / 👎.
| hooks.evaluateGroupExpiry(mirror); | ||
| sink.groups.mutate((groups) => { | ||
| const i = groups.indexOf(mirror); | ||
| if (i >= 0) groups.splice(i, 1); | ||
| }); | ||
| if (sink.timeline.get(mirror.sequence)?.group === mirror) sink.timeline.delete(mirror.sequence); | ||
| mirror.close(); |
There was a problem hiding this comment.
When #prune is triggered by a new subscriber rather than a new publication, this evaluation can remain false even though the entry exceeded retention: for example, groups queued 10 ms apart with a 100 ms budget, followed by 200 ms of inactivity before another subscriber joins. Fresh evidence after the prior fix is that #addSink invokes #prune without adding a newer edge; the code then deletes the timeline entry and cleanly closes the mirror, whose buffered frames remain readable and whose future expiry checks return false because the candidate is gone. Thus a handed-out mirror can still pin and deliver data beyond retention, so eviction itself needs to abort or clear it independently of the drift verdict. (Written by GPT-5.6 Sol)
AGENTS.md reference: AGENTS.md:L174-L177
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7bdf908e35
ℹ️ 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".
| ): boolean { | ||
| const candidate = this.#state.timeline.get(group.sequence); | ||
| if (candidate?.group !== group) return false; | ||
|
|
There was a problem hiding this comment.
When the browser's system clock changes between publishing the candidate and live-edge groups, the timeline values captured with Date.now() make this subtraction either jump forward and discard fresh buffered content or go negative and let an unstamped stalled group bypass a positive latency budget. Rust uses a monotonic Instant for the matching behavior, so record these arrival times with performance.now()/Time.Milli.now() as well. (Written by GPT-5.6 Sol)
AGENTS.md reference: AGENTS.md:L172-L177
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5bea9b2934
ℹ️ 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 self.poll_expired(waiter) { | ||
| return Poll::Ready(Err(Error::Old)); |
There was a problem hiding this comment.
When a partial frame header is available before its payload finishes, poll_next_frame passes its frame::Consumer out after this single expiry check. If a newer group then advances the live edge, frame::Consumer::{poll_read_chunk,poll_read_all} watches only the group abort state, so a raw reader and the Lite/IETF GroupServe implementations, which store the frame and poll it directly, can remain blocked indefinitely and keep serving a stale stream despite REAL_TIME. Fresh evidence after the earlier handed-out-group fix is that expiry is now attached to group::Consumer but is lost at this frame handoff; carry it into the frame handle and mirror the cancellation in JS's current-frame send path.
AGENTS.md reference: AGENTS.md:L177-L177
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1b6ff09c9e
ℹ️ 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 ready!(self.poll_stale(&group, drift, waiter))? { | ||
| self.stale.add(group.content()); | ||
| continue; |
There was a problem hiding this comment.
When a connection negotiates IETF moq-transport and the serving side has multiple groups available, this filtering consumes every group except the live edge even if the client requested a nonzero latency budget. The IETF publishers construct the model subscription with only priority (rs/moq-net/src/ietf/publisher.rs:541-546 and js/net/src/ietf/publisher.ts:143), because this protocol path does not transmit Subscription::latency; consequently the serving subscriber always uses the zero default and irreversibly discards backlog before the receiving subscriber can apply its actual budget. Use a non-dropping publisher-side fallback for protocols without this parameter, leaving the receiver to enforce its requested ceiling.
Useful? React with 👍 / 👎.
| end: None, | ||
| current: None, | ||
| dead: None, | ||
| stale_stats: Default::default(), |
There was a problem hiding this comment.
When a tagged spliced subscriber uses Subscriber::read_frame and its in-progress group expires after handoff, this meter remains untagged: resume::Subscriber::poll_read_frame keeps the group internally, so the outer track wrapper never calls set_stale_meter, then the resume reader swallows Error::Old and advances. The expiry therefore writes its unread tail into a no-op meter, while take_stale has nothing to drain, causing Traffic::stale to under-report this route-takeover/frame-level scenario. Pass the logical subscriber's meter into internally drained groups or return their expired content through the cursor accounting path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 16c7cea4b9
ℹ️ 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".
| fn serving_subscription(subscriber_priority: u8) -> Subscription { | ||
| Subscription { | ||
| priority: super::priority::from_wire(subscriber_priority), | ||
| latency: Latency::max(Duration::MAX), |
There was a problem hiding this comment.
Fresh evidence after the earlier IETF finding is that the chosen non-dropping sentinel cannot be encoded when this IETF subscription is forwarded to a Lite upstream. Before the upstream track info is known, Duration::MAX reaches Duration::encode, whose millisecond conversion exceeds the 62-bit varint and fails the subscription; the matching Number.POSITIVE_INFINITY in js/net/src/ietf/publisher.ts fails at BigInt(v) for the same gateway flow. Use a finite wire-representable ceiling in both implementations so an IETF client served through a Lite route can subscribe successfully.
AGENTS.md reference: AGENTS.md:L174-L177
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4f06a6a13b
ℹ️ 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 self.expired || self.read_idx >= self.size() { | ||
| return self.expired; |
There was a problem hiding this comment.
When a complete frame is handed to the Lite or IETF publisher, poll_read_chunk advances read_idx to the frame size, so this early return permanently disables its expiry policy. Both current serving loops then detach the returned Bytes into GroupState::Serve::chunk and can remain blocked in writer.poll_write (lite/publisher.rs:2970-2979, ietf/publisher.rs:1566-1575) without polling either the frame or group again. Under congestion, a newer group can therefore make this stream stale while the already-read payload continues consuming flow control indefinitely, so the latency budget still needs to guard the transport-owned chunk until it drains. This is fresh evidence beyond the earlier in-flight-frame finding because the new frame guard is present, but expiry is lost at the subsequent frame-to-chunk handoff. (Written by GPT-5.6 Sol)
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a8738599e2
ℹ️ 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".
| // Keep the group guard live while the transport owns a detached | ||
| // frame chunk and may be blocked on flow control. | ||
| if self.group.poll_expired(waiter) { |
There was a problem hiding this comment.
When unidirectional stream credit is exhausted, a group remains in GroupState::Open at lines 2908-2916 and registers only the transport's open-stream waker, so it never reaches this expiry check and newer groups cannot wake it. Both the Lite and IETF Rust publishers can therefore accumulate expired group machines indefinitely and spend later stream credit opening and resetting stale groups ahead of live ones. Poll expiry before poll_open_uni as well as while serving. (Written by GPT-5.6 Sol)
Useful? React with 👍 / 👎.
|
|
||
| const obj = new Frame({ payload: frame.payload, timestamp: frame.timestamp }); | ||
| await obj.encode(stream, header.flags, timescale, this.#session.version); | ||
| await hooks.guardGroup(group, obj.encode(stream, header.flags, timescale, this.#session.version)); |
There was a problem hiding this comment.
When flow control blocks header.encode at line 263, this guard has not been installed yet, so a newer group can make this one stale while its stream remains blocked indefinitely instead of being reset. The Lite publisher has the same pre-guard gap around its stream type and GroupMessage writes. Guard the entire per-group serialization operation, including its header, rather than only object/frame encoding. (Written by GPT-5.6 Sol)
Useful? React with 👍 / 👎.
| let subscription = self.track.subscription().with_latency(latency); | ||
| let _ = self.track.update(subscription); |
There was a problem hiding this comment.
When a caller follows rs/hang/examples/subscribe.rs:76-82 against a remote Lite publisher, the initial subscription still carries only priority and therefore uses the zero latency budget; this update happens only after SUBSCRIBE_OK, when retained groups may already have been skipped irreversibly. Fresh evidence after the earlier initial-subscription fixes is that this shipped example still uses the late-only path despite advertising a 500 ms tolerance. Include that latency in the example's initial Subscription. (Written by GPT-5.6 Sol)
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: af78893073
ℹ️ 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".
| fn live_edge(&self, cap: Option<u64>) -> Option<Edge> { | ||
| let mut wall: Option<WallEdge> = None; | ||
| let mut presentation: Option<PresentationEdge> = None; | ||
| for slot in self.lookup.values() { |
There was a problem hiding this comment.
When a concurrent FETCH inserts a higher sequence through insert_group_request, that group is deliberately absent from arrival, but this scan still selects it from lookup. With the default zero budget, it immediately makes every lower visible group stale, so recv_group and read_frame can irreversibly discard their live backlog even though those cursors can never receive the fetched group as a replacement. Build their drift edge only from visible arrival entries, or otherwise track visibility on each slot. (Written by GPT-5.6 Sol)
AGENTS.md reference: AGENTS.md:L100-L100
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c773fe92f1
ℹ️ 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".
| }); | ||
|
|
||
| await header.encode(stream, this.#session.version); | ||
| await hooks.guardGroup(group, header.encode(stream, this.#session.version)); |
There was a problem hiding this comment.
When flow control stalls header.encode and a newer group causes this group to expire, guardGroup rejects here, but this await sits outside the try that resets stream. Fresh evidence after the earlier header finding is that the added guard only races the passed encoding promise and cannot cancel it, so control reaches finally and closes only the model group while the underlying unidirectional stream may later finish its partial header without ever being FINed or reset, retaining stream credit under congestion. Put the guarded header write inside the reset-catching scope. (Written by GPT-5.6 Sol)
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d820fc1fba
ℹ️ 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".
| /// Content skipped because it drifted past a subscriber's | ||
| /// [`Latency`](crate::Latency) budget. Disjoint from the top-level payload | ||
| /// counters: skipped content is never handed over. A steady rate here means | ||
| /// subscribers are consistently behind the live edge. | ||
| pub stale: Content, |
There was a problem hiding this comment.
When operators monitor a relay through /metrics, the new skipped-content totals are invisible: rs/moq-relay/src/internal.rs::render_metrics still exports only the original Traffic payload fields and never emits stale.bytes, stale.frames, stale.groups, or stale.datagrams. This prevents Prometheus users from detecting the sustained behind-live-edge condition that this field is intended to reveal, even though the JSON stats tracks expose it. Add corresponding moq_relay_stale_*_total counters to the renderer. (Written by GPT-5.6 Sol)
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fd934a30ba
ℹ️ 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 self.poll_expired(waiter) { | ||
| return Poll::Ready(Err(Error::Old)); |
There was a problem hiding this comment.
When a returned group has already delivered its final frame, a newer group can advance the live edge before the caller polls once more for EOF, so this unconditional check returns Error::Old instead of Ok(None). The Lite and IETF serving loops can hit the same race after draining the last payload and reset an otherwise complete stream, while the matching JS #terminalError path rejects normal group-drain loops. Apply expiry only while unread or unfinished content remains, and mirror that terminal-state check in js/net. (Written by GPT-5.6 Sol)
AGENTS.md reference: AGENTS.md:L174-L178
Useful? React with 👍 / 👎.
| let subscription = self.track.subscription().with_latency(latency); | ||
| let _ = self.track.update(subscription); |
There was a problem hiding this comment.
When an external caller follows the public Consumer::new(track, format).with_latency(...) API after awaiting a remote Lite subscription, this update is too late: the publisher has already served the initial default REAL_TIME request and may have irreversibly skipped the retained backlog. Fresh evidence after the internal decoder and exporter fixes is that the public consumer documentation still directs callers to this builder for startup latency, so consumers outside this repository remain broken. Reshape the high-level entry point so its latency is available when the subscription is opened rather than relying on this post-accept update. (Written by GPT-5.6 Sol)
AGENTS.md reference: AGENTS.md:L133-L137
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d97558dac8
ℹ️ 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(edge) = edge.and_then(|edge| edge.presentation) | ||
| && let Some(slot) = state.lookup.get(&edge.sequence) | ||
| { | ||
| let _ = slot.group.poll_timestamp(waiter); | ||
| } |
There was a problem hiding this comment.
When a handed-out group is stalled between frames and a higher-sequence group already exists without a timestamp, this registers only the stalled candidate and the current presentation edge. If the higher group later receives its first timestamp and that timestamp pushes the candidate beyond a positive latency budget, only the higher group's channel is notified, so the stalled reader never rechecks expiry and can remain blocked until unrelated track activity occurs. Propagate first-timestamp changes through the track state or register on every unstamped group that could become the presentation edge. (Written by GPT-5.6 Sol)
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c5b2e2f311
ℹ️ 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".
| let outcome = 'serve: { | ||
| // Keep the group guard live while the transport owns a detached | ||
| // frame chunk and may be blocked on flow control. | ||
| if self.group.poll_expired(waiter) { |
There was a problem hiding this comment.
When a completed group's final frame has been handed to the writer and flow control blocks its detached chunk, this guard cannot expire it: advancing the group cursor made Plain::expiry_pending false because index == fin, and the frame consumer is not polled again while chunk is present. A newer group can therefore make this stream stale while the final payload remains blocked and consumes flow control indefinitely; the identical guard in ietf/publisher.rs has the same gap. Fresh evidence after the earlier transport-drain report is that the newly added group-level guard explicitly disables itself for this common final-frame state, so the transport-owned chunk needs an expiry check independent of the group's unread-frame cursor.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 69eeafc98c
ℹ️ 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".
| this.#terminal = new Lagged(); | ||
| if (this.#closed.peek() === undefined) this.#closed.set(this.#terminal); |
There was a problem hiding this comment.
When retention pruning reaches a cleanly closed mirror after its caller has already drained every frame, this unconditionally replaces the group's clean terminal state with Lagged. A later readFrame() therefore throws after it previously returned EOF, and a caller that first accesses closed after eviction observes an error instead of null. Fresh evidence beyond the earlier latency-expiry report is that #terminalError now preserves drained completion, but this separate retention-eviction path still does not; only mark the mirror lagged when buffered unread frames are actually discarded. (Written by GPT-5.6 Sol)
AGENTS.md reference: AGENTS.md:L97-L99
Useful? React with 👍 / 👎.
e2e9913 to
0673f87
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0673f87f0e
ℹ️ 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 self.expired { | ||
| return Poll::Ready(Err(Error::Old)); |
There was a problem hiding this comment.
When an open group has yielded every currently available frame but has not received FIN, a newer group can make its pending read expire; the Some(false) branch returns Ok(None), but poll_expired_while_pending has already set self.expired, so the next poll takes this early return and produces Error::Old. Callers that probe again after EOF or alternate between read_frame, next_frame, and finished therefore see a clean completion turn into an error. Fresh evidence after the earlier final-frame fix is that the clean branch was added without recording a sticky clean-terminal state. (Written by GPT-5.6 Sol)
AGENTS.md reference: AGENTS.md:L100-L100
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5340a26d29
ℹ️ 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".
| .lookup | ||
| .get(&edge.wall.sequence) | ||
| .is_some_and(|live| live.stamp == edge.wall.stamp && !live.group.is_aborted()) | ||
| && (budget.is_zero() |
There was a problem hiding this comment.
When a connection negotiates Lite01 or Lite02, lite::Subscribe::decode_msg substitutes Duration::ZERO because those versions cannot carry latency_max, and lite/publisher.rs passes that value into the model subscription. This new zero-budget branch then discards every buffered group except the live edge, even when the receiving subscriber requested a nonzero budget locally, so legacy peers irreversibly lose backlog before receiver-side enforcement. Preserve the pre-enforcement, non-dropping behavior for protocol versions without this field, as is already done for IETF subscriptions. (Written by GPT-5.6 Sol)
Useful? React with 👍 / 👎.
Post-handoff expiry judged every read, which cost two things. Semantics: under the default real-time budget, the arrival of the next group is exactly what makes the current one no longer newest, so a reader draining a buffered group lost its tail every time. That is not what the budget is for. It bounds a group that has *stalled* while the live edge moved on, so judge only a read that found nothing and is about to park. A group with frames in hand now always drains to its end. Cost: evaluating the policy walks the track's group cache under the track lock, which every subscriber of that track shares, and it also disabled the prefetch fast path in `read_frame`. Reading 20k frames with 50 groups cached went from 13ns/frame to 998ns/frame. Off the ready path it is back to 14ns. The wire publishers keep the check where the group cursor cannot see the stall: parking on transport flow control, so a blocked stream cannot pin credit for content that has gone stale. Everything else they park on (the next frame, the current frame's tail) now reports expiry itself, so the per-pass check and `Writer::has_pending` are gone. Also stop pinning track demand. `GroupExpiry` held a `kio::Consumer` of the track state, which joins the consumer count that `broadcast::Demand` reads, so a handed-out group kept an upstream subscription open past the last real subscriber. A relay serves one for the life of its stream. It holds a `ConsumerWeak` now, which needed a `poll` on that type. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two things a group boundary got wrong. A group was measured against the live edge from its *first* frame, so a 2s GOP read live always looked 2s behind, however current its reader was. Any budget shorter than one GOP therefore dropped the tail of every GOP, which is why the fixtures here wanted 30-second windows. A group is not late because it opened long ago; what can be late is the content its reader has yet to take. Both measures now start from the cursor: the next frame it would read (or the group's newest, once it has taken them all) for presentation time, and the group's last write for wall-clock time. A group nobody has started reading still sits at its own start, so group selection is unchanged. And expiry always reported `Error::Old`. A group-level read only parks once the cursor has taken every frame, so expiring there loses nothing: what it was waiting for was the producer's FIN, and a group abandoned at its own end is indistinguishable from one that ended there. It now ends cleanly, and `Old` is reserved for a cursor that still holds unread content (a half-read payload, a publisher with buffered frames). That matters most at a relay, which was resetting the downstream stream for a group it had delivered in full: the FIN and the next group's first frame are separate streams, so the reader is routinely parked at the group's end when the verdict is taken. Both regressions are covered, and each new test fails with only its own half reverted. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`TrackRun` outgrew the other variants once it started carrying the group expiry guards, and the enum is moved on every state transition. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
An API diff shows nothing when the change is which content gets delivered, or whether a stream is reset rather than FIN'd. That is still something another implementation has to match, so it belongs in the description and in a review's checklist alongside the public API list. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`with_test_latency` was a `#[cfg(test)]` method on the public Consumer that only called `set_latency` and returned self. It existed to keep the old `with_latency` chaining style at the call sites after that method was removed, which meant this PR deleted a public builder and then quietly re-added it for its own use. Worse, it made the tests skip the API the removal was in aid of. The point of `Consumer::new` inheriting the subscription's budget is that a caller sets latency on the `Subscription` before subscribing; every test was instead subscribing with the default and patching the consumer afterwards, so nothing but `new_inherits_the_initial_subscription_latency` exercised the real path. They now all do. `container_latency_only` still gives the two layers different budgets on purpose, so it keeps the explicit `set_latency` plus a `control.update`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`subscribe_all` said nothing about what "all" meant, read like a subscribe-to-every-track operation, and covered two different signatures in two files. It is a replay window: these tests write every group up front and only then read, so they have to ask for history rather than the live edge. Renamed to `replay()`, returning a `Subscription`, which is the shape moq-net and moq-bench already use for the same thing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The SI subscription added by #2909 uses the default budget, which this branch now enforces: real-time takes only the newest group, so a track carrying table snapshots at a low rate would lose the revisions between them. SI needs the same window as the media it describes, which the export already threads to every other source. The test that repoints an SI entry writes both GOPs up front and only then reads, so it asks for that window too. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
0a4b614 to
a73c5d7
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a73c5d75f4
ℹ️ 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".
| loop { | ||
| match &mut self.state { | ||
| GroupState::Open => { | ||
| if self.group.poll_expired(waiter) { |
There was a problem hiding this comment.
When a finished empty group, such as the discontinuity markers produced by moq-mux, is handed out before stream credit is available, this call never evaluates its expiry policy because Plain::expiry_pending is false at index == fin == 0. The state machine therefore registers only on poll_open_uni; newer groups cannot wake or discard it, and later credit still opens the obsolete marker's stream. Fresh evidence after the earlier stream-credit report is that the added guard calls the non-forcing variant; use poll_expired_while_pending(waiter, true) here and in the IETF equivalent. (Written by GPT-5.6 Sol)
AGENTS.md reference: AGENTS.md:L100-L100
Useful? React with 👍 / 👎.
The clean-end branch reported `Ok(None)` but left only `expired` set, so the next poll took the failure guard and turned the same group into `Error::Old`. A caller is allowed to probe past the end of a group, and `moq_mux::container::Consumer` does exactly that when it follows a read with `poll_finished`. The outcome is now sticky in its own right, as it already was in the JS mirror. Found by the Codex reviewer on the PR. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Lite01/02 have no latency field in SUBSCRIBE, so `decode_msg` substitutes `Duration::ZERO`. That is indistinguishable from a peer asking for real time, and this branch now acts on it: the serving side discards every group but the live edge for a subscriber that never declined the backlog. Those two are `NEGOTIATED[0]` and `[1]`, so this is the ordinary wire for current moq-lite peers rather than a legacy corner. They now get a window wide enough not to drop, leaving enforcement to the receiving subscriber, which is what the IETF path already does for the same reason. `Version::carries_latency` is the one place that answers whether a decoded zero means "real time" or "not stated". Found by the Codex reviewer on the PR. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Swept the open review threads before merging. @kixelated's five are all addressed by later commits; GitHub still shows them unresolved but outdated:
Noted on the last one that the eventual plan is to move the ordered container consumer into moq-net once lite-05 lands, so moq-net skips on the transport timestamp rather than the container's. Nothing in this PR forecloses that. Two Codex findings were real and are now fixed, each with a regression test verified to fail without it:
One more found while rebasing: #2909 landed The remaining Codex threads are stale against later commits (initial-subscription latency, resume replacement copies, spliced stale metering, Prometheus counters, JS enforcement). JS enforcement lives in #2926. (Written by Opus 5) |
1 participant
Summary
Subscription::latency/latencyMaxwas propagated as metadata but the track model never acted on it. This enforces it inrs/moq-netat both ends of a subscription, which is the ceiling that #2784 needs now that #2785 is reverted ondev.The JS mirror is stacked on top as #2919 (originally filed as #2892), so each language reviews on its own.
track::Subscriber'spoll_recv_group/poll_next_group/poll_read_frameskip a group that has drifted past the subscription's budget, so one poll walks a whole backlog off. Both ends go through the same code: a relay serves downstream viapoll_recv_group, and a local consumer reads the same way.fetch_groupis exempt, since a fetch names historical content explicitly.Error::Oldis reserved for a cursor still holding unread content (a half-read payload, a publisher with buffered frames). Without this a relay reset the downstream stream for a group it had delivered in full, since the FIN and the next group's first frame are separate streams and the reader is routinely parked at the group's end when the verdict is taken.moq_mux::container::Consumer::newinherits that already-negotiated budget.stale: { bytes, frames, groups, datagrams }in JSON stats and as matchingmoq_relay_stale_*_totalPrometheus counters. Expiry after handoff counts only the unread tail, and cloned readers share the one attribution.Public API changes
Targets
devfor one breaking cleanup:moq_mux::container::Consumer::with_latency.Consumer::newnow inherits the initialmoq_net::track::Subscriptionlatency;set_latencyremains for mid-stream changes.moq_net::stats::Contentcounter shape, and astats::Traffic::stale: Contentfield on the existing#[non_exhaustive], serde-defaulted struct.kio::ConsumerWeak::poll, mirroringConsumer::poll. Additive: it lets a watcher register for changes without joining the consumer count.Wire behavior changes
The encoding is unchanged; no draft update is needed, and the existing draft already specifies the two age backstops. What a peer observes differently:
Oldwhen the group expires while its transport write is flow-control blocked, or while waiting on stream credit. It was previously held open.Test plan
nix develop --command just check/just check-allnix develop --command just test all: 3258 Rust tests pass, 2 skipped; all JS suites green.real_time_reads_a_live_stream_without_truncating_it(2s GOPs read as they arrive at the default budget, with the FIN landing after the successor's first frame) anda_budget_is_measured_from_the_readers_position(a straggler frame arriving after the next group opened still reaches a reader inside its budget).dev, 9-14ns here.(Written by Opus 5)