feat(js/net): enforce the subscriber latency budget - #2926
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: cdde14f60e
ℹ️ 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".
| get closed(): GetPromise<Error | null> { | ||
| return this.#state.closed; | ||
| this.#watchClosed(); | ||
| return this.#closed; |
There was a problem hiding this comment.
If consumer.closed is accessed while the group is open and the producer then closes it in the same turn, this wrapper remains pending until #runClosed() resumes from Signal.race in a later microtask. Consequently, the previously obtained handle reports closed.peek() === undefined even though the group is already closed, breaking the documented synchronous terminal-state check and allowing callers to proceed as if it were open. (Written by GPT-5.6 Sol)
Useful? React with 👍 / 👎.
0a4b614 to
a73c5d7
Compare
cdde14f to
3b4eee0
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3b4eee0f9d
ℹ️ 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".
| presentation: this.#state.frames.peek()[0]?.timestamp ?? this.#state.latest, | ||
| activity: this.#state.activity, |
There was a problem hiding this comment.
With a multi-frame group, readFrame*() removes the current frame before both lite/publisher.ts#runGroup and ietf/publisher.ts#runGroup guard its potentially blocked encode/write. This position therefore uses the next buffered frame's timestamp, or the group's latest timestamp/activity, rather than the frame still being transmitted. Under flow control, an old payload can consequently appear inside the latency budget and remain blocked for up to the rest of the group's timeline, violating the subscriber's latency ceiling; preserve the removed frame's position for the guarded operation. (Written by GPT-5.6 Sol)
Useful? React with 👍 / 👎.
The JS mirror of the moq-net work in the parent PR: js/net applies the same drift budget as it reads, and js/watch requests the playback latency ceiling from its first subscription. Drift is measured against both a presentation-time and a wall-clock live edge, from the reader's own position rather than the group's first frame, so a reader keeping pace is not convicted by how long ago its group opened. A group with frames in hand always drains; only a read that would wait is judged, which is also what keeps the policy off the read path. A drained group the budget gives up on ends rather than fails, since nothing was lost; the latency error is reserved for a caller still holding content. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Matches the moq-mux rename in the parent PR: `subscribeAll` said nothing about what "all" meant and read like a subscribe-to-every-track operation. It is a replay window. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Mirrors the moq-net fix in the parent PR. Drafts 01 and 02 have no latency field in SUBSCRIBE, so it decodes as `0`, which this branch would otherwise act on: the serving side discards every group but the live edge for a subscriber that never declined the backlog. Those two are the most preferred negotiated versions, so it is the ordinary wire. `carriesLatency` is the one place that answers whether a decoded zero means "real time" or "not stated". Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
3b4eee0 to
4bc781c
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4bc781c3f4
ℹ️ 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".
| changed: [ | ||
| this.#state.groups, | ||
| this.#state.timelineChanged, | ||
| this.#state.update, | ||
| this.#cursor, | ||
| this.#state.closed, |
There was a problem hiding this comment.
If a subscriber receives and begins waiting on a group before Producer.accept() commits track info, #drift() initially uses the requested latency without the publisher retention clamp. A later accept({ latencyMax: ... }) updates state.info, but this dependency list does not observe that signal, so a pending read or guarded write can remain blocked indefinitely even when the newly known retention window makes it stale. Include this.#state.info among the expiry change sources.
Useful? React with 👍 / 👎.
| const requested = this.#state.update.peek()?.latencyMax ?? 0; | ||
| const retained = this.#state.info.peek()?.latencyMax; | ||
| return { | ||
| budget: this.#enforceLatency | ||
| ? retained === undefined | ||
| ? requested | ||
| : Math.min(requested, retained) |
There was a problem hiding this comment.
When a track retains 2 seconds but a subscriber requests 10 seconds, this clamps only that subscriber's local reads; Producer.#updateSubscription() still exposes the unbounded result of combineSubscriptions() as 10 seconds. A relay or wire publisher therefore asks its upstream to serve content that this hop will immediately discard, and applications observing Request.subscription receive a budget the producer cannot honor. Clamp the aggregate against Info.latencyMax while preserving each subscriber's own requested value, matching the Rust implementation this JS counterpart is intended to track.
AGENTS.md reference: AGENTS.md:L176-L181
Useful? React with 👍 / 👎.
1 participant
Follow-up to #2890, which did the same work in
rs/moq-netand has merged. Rebased ontodev, so this now carries only thejs/half.Replaces #2919, which GitHub auto-closed as merged when a mistaken force-push briefly put these commits onto the base branch. Nothing was merged into
dev; the base branch has since been corrected and carries nojs/changes.Summary
js/netcarriedlatencyMaxas metadata and never acted on it. It now applies the same drift budget as it reads, andjs/watchrequests the playback latency ceiling from its first subscription instead of only after acceptance.performance.now()so it does not react to system-clock changes.Lagged).fetchGroupstays exempt: a fetch names historical content explicitly, so there is no live edge to be late against.Public API changes
None. Enforcement and the watch subscription plumbing use package-internal helpers (
hooksininternal.ts), so no exported surface moved.Wire behavior changes
None on the encoding. What changes is what a local subscriber is handed: a group that has drifted past its own
latencyMaxis skipped rather than delivered, and one that stalls after handoff ends (or fails, if content was unread) instead of waiting indefinitely. The publisher side resets a group stream whose encode or write is blocked as the group expires, which it previously would not have done. The default budget is zero, so a delayed live subscriber deliberately advances to the live edge; callers that need history must request a replay window.Cross-package sync
Mirrors
rs/moq-netin #2890. No draft change: the wire format and parameters are unchanged, and the existing draft already specifies the two age backstops.Test plan
nix develop --command just checknix develop --command just test: 3258 Rust tests pass, 2 skipped; all JS suites green, including 517@moq/netand 147@moq/watch.real time reads a live stream without truncating it— 2s GOPs read as they arrive at the default budget, with the group's close landing after its successor's first frame.a budget is measured from the reader's position— a straggler frame arriving after the next group opened still reaches a reader that is inside its budget.(Written by Opus 5)