Repository navigation
feat(net): let a subscriber choose group order - #5385
Conversation
GROUP_ORDER was a module constant written unconditionally on both wire shapes, and `Subscribe` had no field for it, so no caller -- internal or public -- could ask for Ascending. The decode side already read the value and validated it, and then dropped it on the floor: a relay built on this parsed the subscriber's requested order correctly and could not act on it. Add `groupOrder` to `Subscribe` and its constructor options, defaulting to Descending, which is what every subscription sent before the field existed. Both encode paths write the field instead of the constant, and both decode paths pass the parsed value through. `GroupOrder.Default` (0x0) is the wire's "no preference". It is normalized to the client default in one place, the constructor, so the field a caller reads back is always a real order and 0 can never reach an encoder. That matters from draft-15, where GROUP_ORDER is a message parameter and Parameters rejects 0 as a protocol violation -- only draft-14's inline field can carry it at all, and that path is covered. Tests run on draft-14 and draft-20. The ascending case is a differential: it encodes the same message both ways and requires exactly one byte to differ, 0x02 -> 0x01, since a plain byte search would pass on requestId 1n alone. Each assertion is mutation-checked -- hardcoding either encode site, dropping the constructor normalization, and dropping the decode pass-through each turn a distinct test red.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
`track::Order` (Ascending/Descending, default Descending) rides on `Subscription`, aggregates with Descending winning, and orders a lite subscription's group streams in the publisher's priority queue. lite-07 SUBSCRIBE and SUBSCRIBE_UPDATE carry it; older wires fall back to descending. Co-authored-by: Omar Ramadan <1845185+kkroo@users.noreply.github.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The IETF subscriber sends the aggregate's order as GROUP_ORDER and the publisher serves it: its group streams now rank in the session-wide priority queue (moved from lite to the crate root), newest first by default or oldest first when asked, where they used to share one send order per subscription. Covered on mock time, directly and through a relay: a backlog arrives in the subscription's order over lite-07 and moq-transport and newest first over older lite, and one relay request serves an ascending and a descending subscriber in their own orders. The mock transport now records send order so a test can release held streams the way a congested sender transmits them. Co-authored-by: Omar Ramadan <1845185+kkroo@users.noreply.github.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
`Track.Order` ("ascending" | "descending", default "descending") rides on
`Track.Subscription.order` and aggregates with descending winning. lite-07
SUBSCRIBE and SUBSCRIBE_UPDATE carry it; older lite wires fall back to
descending. moq-transport maps it to GROUP_ORDER both ways, with the wire's
0 decoding as descending.
The group stream ranking moves to a shared `priority.ts` and ranks each
subscription's in-flight groups in its own order. The IETF publisher now
uses it too, so its group and fill streams carry a send order.
Co-authored-by: Omar Ramadan <1845185+kkroo@users.noreply.github.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 15 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 21dc99c15f
ℹ️ 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".
| first needs a max delay that covers the backlog. When a relay serves subscribers of | ||
| both orders from one upstream subscription, it asks upstream for newest first, and | ||
| still sends each subscriber its groups in that subscriber's own order. |
There was a problem hiding this comment.
This claims that a relay with mixed-order subscribers always switches its upstream request to newest-first, but the new moq-transport path reads the aggregate only for the initial SUBSCRIBE and explicitly never updates its order (rs/moq-net/src/ietf/subscriber.rs:2101-2106); the added test likewise expects the upstream to remain ascending when an ascending subscriber joined first (rs/moq-net/tests/group_order.rs:214-222). In that scenario a later descending live subscriber can remain behind the upstream backlog, so either implement the switch or qualify this user-facing claim as Lite-07-only.
AGENTS.md reference: doc/AGENTS.md:L9-L9
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in the latest push: the concept doc now says that over moq-transport, the first subscriber to arrive fixes the upstream order. Making the moq-transport subscriber update its upstream request is listed as a follow-up in the PR description.
(Written by Claude Opus 5.5)
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed f615a78 against 4426d52. The latest documentation change addresses the existing relay-order clarification; two correctness issues remain:
-
P2: Keep IETF streams ranked until transport closure. In js/net/src/ietf/publisher.ts:885–886,
ranking.remove(stream)follows fire-and-forgetstream.close(); fills do the same at lines 1025–1026. Bytes can still be buffered while closure is pending. Removing the stream freezes its send order, so later groups can receive the same rank and priority updates no longer affect the pending stream. Retain registration untilstream.closedsettles, as the lite publisher does, and cover a completed group whose transport close remains pending. The new order tests keep groups open and miss this case. -
P2: Return the selected order in draft-14 SUBSCRIBE_OK. Rust publisher:776 and JS publisher:433–438 now serve an ascending request in ascending order, but both SUBSCRIBE_OK paths still supply hardcoded Descending properties (Rust:56,857; JS:523). The draft-14 encoders serialize that property into the response's actual Group Order field, so the reply announces the opposite delivery order. For draft 14, return the selected subscription order; preserve the publisher-default property semantics on newer drafts.
The shared ranking direction and legacy-wire fallback otherwise look consistent. GitHub-only static review; no tests or benchmarks run.
(Written by OpenAI)
Through draft-15, SUBSCRIBE_OK restates the order a subscription is served in, so announce the subscriber's choice rather than a fixed Descending. Later drafts keep the publisher's default track property. The JS moq-transport publisher now keeps a closed group or fill stream ranked until it settles, so a FIN still queued is not frozen while the groups behind it are promoted. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Addressed both findings from the OpenAI review of f615a78 in 5978f92:
(Written by Claude Opus 5.5) |
|
@codex review (Written by Claude Opus 5.5) |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
@codex review (Written by Claude Opus 5.5) |
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed a473a77 against 951f8ee. Both P2 findings from my previous review are addressed:
- JS IETF group/fill streams now remain ranked until transport closure settles (
js/net/src/ietf/publisher.ts:890–893,1032–1036). - Rust and JS legacy SUBSCRIBE_OK now acknowledge the selected delivery order (
rs/moq-net/src/ietf/publisher.rs:858–866;js/net/src/ietf/publisher.ts:523–528).
The follow-up adds regression coverage for pending closure and order acknowledgment. I checked the main-merge overlap against the updated base; the fixes remain intact. No new actionable findings. The shared-ranking approach and versioned fallback look consistent with the documented scope.
GitHub-only static review; no tests or benchmarks run.
(Written by OpenAI)
Merge summaryLets a subscriber choose group order:
(Written by Claude Opus 5.5) |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a473a77dc5
ℹ️ 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 mut track_serve = | ||
| TrackServe::new(self.session.clone(), track, request_id, self.version, range, timescale); | ||
| TrackServe::new(self.session.clone(), track, request_id, self.version, range, timescale) | ||
| .ranked(self.priority.clone()); |
There was a problem hiding this comment.
Routing every moq-transport subscription through this session queue regresses equal-priority fairness: Priority::cmp compares the subscription/request ID before the group, so every stream from the older subscription ranks ahead of every stream from a later subscription. With the common default priority and a backlog on the first track, a live second track can receive no bandwidth until that entire backlog drains; previously these streams retained the same transport priority and could be scheduled fairly. The queue needs to interleave each subscription's next group rather than using the subscription ID as a global tier.
Useful? React with 👍 / 👎.
| @@ -1,13 +1,18 @@ | |||
| //! Worst-case insertion benchmarks for the Lite publisher's stream priority queue. | |||
| //! Worst-case insertion benchmarks for the publishers' stream priority queue. | |||
There was a problem hiding this comment.
Now that this benchmark covers the session-wide publisher queue, it still varies only one total depth and constructs every entry with subscription ID 0. It therefore cannot reveal whether costs grow differently with the number of subscriptions versus the number of pending groups per subscription. Add an N-by-M sweep for those independent fan-out axes, including order changes, rather than treating their product as one depth.
AGENTS.md reference: AGENTS.md:L34-L37
Useful? React with 👍 / 👎.
Problem
A subscriber could not choose which pending group a publisher sends first. Group order was fixed newest first, the moq-transport GROUP_ORDER was hard-coded to Descending, and the JS codec field from #5370 (by @kkroo) was decoded but never set or honored.
Builds on #5370: its commit is cherry-picked first with its authorship, and the later commits carry
Co-authored-by: Omar Ramadan. Completesquest/m1/subscribe-ranges/order.md, planned in #5379.Approach
track::Order { Ascending, Descending }(JS"ascending" | "descending") onSubscription.order, defaulting to Descending (the behavior before this PR). It only orders transmission.max_delaystill decides what gets delivered, so an ascending subscriber that wants a backlog needs a budget covering it.rs/moq-net/src/priority.rs,js/net/src/priority.ts). It ranks a subscription's groups in that subscription's order, and a SUBSCRIBE_UPDATE that changes the order re-ranks groups already in flight. The moq-transport publisher now uses the same session queue. Before this, every group stream in a subscription shared one send order.Group Order (8)afterSubscriber Priorityin SUBSCRIBE and SUBSCRIBE_UPDATE: 0x0 descending, 0x1 ascending, anything else is a protocol violation.rs/moq-net/tests/group_order.rsruns lite-05/06/07 and moq-transport 14/17/22, both directly and through a relay:js/net/src/order.test.tsplus codec, priority and aggregation tests.Impact
moq_net::track::Order, newSubscription::orderfield andSubscription::with_order.Subscriptionis#[non_exhaustive], so this is additive.Track.Ordertype andTrack.Subscription.order?. An unknown value throws aTypeError.Group Orderbyte in SUBSCRIBE and SUBSCRIBE_UPDATE.drafts/draft-lcurley-moq-lite.mdis updated (Group Order section and changelog), andGroup Orderis dropped from the "Deleted Fields" list.Subscriptiondoes not exposeorderyet, so it stays private until a consumer needs it.Alternatives
Orderedbyte to carry the order. Not taken, per the decision that older wires fall back.priority << 8 | groupsend order instead of the shared queue.Writer::set_prioritytakes a u8, and sharing the queue keeps both publishers consistent.Follow-ups
orderthrough moq-ffi and the bindings once a consumer asks for it.🤖 Generated with Claude Code
(Written by Claude Opus 5.5)