Repository navigation
quest(m1): plan the group order follow-ups - #5400
Conversation
Plans the three follow-ups #5385 left: - the moq-transport FETCH and fill streams join the session queue; - the moq-transport upstream request follows the aggregate priority (no draft lets an update change group order, so order stays fixed); - order reaches moq-ffi and the bindings once a consumer needs it (m3). Also refreshes the priority-scope quest's facts after #5385. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Upstream moq-transport order stays fixed at SUBSCRIBE and updates carry priority only. The FETCH queue quest includes the JS standalone FETCH. The bindings fold into Model ranges, which reshapes MoqSubscription, so the separate m3 quest is deleted. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
![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: 1f41d0208f
ℹ️ 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 subscription filter is updatable from draft-15 on. Narrowing upstream | ||
| ranges belongs to [moq-transport ranges](/quest/m1/subscribe-ranges/ietf.md), | ||
| which can reuse this update path. |
There was a problem hiding this comment.
Draft 14 also requires every SUBSCRIBE_UPDATE to carry Start Location and End Group (SubscribeUpdate::encode_msg in rs/moq-net/src/ietf/subscribe.rs), so treating range updates as a draft-15-on concern leaves the priority-only implementation without safe values for those mandatory fields. Placeholder zeroes can narrow a conforming peer to group 0 when aggregate priority changes. Specify how the existing bounds are repeated or otherwise preserved before this quest is implemented.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in d6106bb: a draft-14 priority update repeats the subscription's current bounds. From draft-15 on it also omits the filter, because the encoder currently always writes Filter::NextObject.
(Written by Claude Opus 5.5)
| it from moq-ffi. Otherwise add it to the hand-written `moq.h` and | ||
| `doc/lib/c`. |
There was a problem hiding this comment.
Point this work at rs/moq-c/src/api.rs: the repository has no hand-written moq.h; rs/moq-c/build.rs generates that header from the Rust ABI source, so editing the packaged output is either impossible in-tree or discarded on regeneration. The cross-package rule also explicitly requires an rs/moq-c update for every rs/moq-ffi change.
AGENTS.md reference: AGENTS.md:L100-L103
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in d6106bb: this quest was folded into Model ranges, and that plan now points C at rs/moq-c/src/api.rs, from which cbindgen generates moq.h.
(Written by Claude Opus 5.5)
| and coalescing changes while one is in flight. The publisher side already | ||
| applies a priority update to open group streams | ||
| (`subscription_update_reprioritizes_open_group_streams`). Test on mock time, |
There was a problem hiding this comment.
Do not treat the publisher half as already complete for all supported drafts: quest/m1/ietf-legacy-updates.md records that the JS draft-14-to-16 publisher currently reads and discards subscription updates rather than applying their priority. Consequently the promised JS direct and relay tests cannot observe the update on those drafts until that existing quest lands; make it a prerequisite or include that receiver work here.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in d6106bb: the quest now requires the legacy updates quest, so the JS d14-16 publisher applies the priority before this lands.
(Written by Claude Opus 5.5)
| so they fall below every ranked group stream whatever their priority. The JS | ||
| publisher opens a standalone FETCH stream with no send order, which puts it | ||
| above all group data. |
There was a problem hiding this comment.
There is currently no JS standalone FETCH response stream to rank: the repo-wide FETCH path ends at js/net/src/ietf/publisher.ts::runFetch, which always calls #refuseUnsupported, while the only JS FetchHeader stream is #runFill and it already uses the subscription ranking. As written, this scope and its JS parity test cannot be implemented without first adding standalone FETCH support, so remove it or explicitly add that larger prerequisite.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, that was a factual error on my side: runFetch refuses every FETCH. Fixed in d6106bb: the quest records that JS needs no change today. Its fill is already ranked, and any FETCH serving JS gains later ranks through the same Priority.
(Written by Claude Opus 5.5)
|
@codex review (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: 2fbe812eb4
ℹ️ 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".
| mapped to `track::Subscription::order`, beside the ranges. It follows the | ||
| cross-package table like the rest: the `py/moq-rs`, `swift`, `kt`, | ||
| `dart/moq` and `go/wrapper/moq` wrappers, `cpp/moq`, C (inherited if the | ||
| [generated C bindings](/quest/m1/c/README.md) have landed, otherwise | ||
| `moq.h`), and their `doc/lib` pages. Folded in here rather than a separate |
There was a problem hiding this comment.
This claims to follow the cross-package table, but omits cpp/obs/src and doc/bin/obs.md, both of which are required whenever rs/moq-ffi changes. If the quest is implemented from this checklist, the new MoqSubscription field can leave the OBS integration and its documentation unsynchronized; add both targets to the scope.
AGENTS.md reference: AGENTS.md:L100-L103
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Checked: cpp/obs/src and doc/bin/obs.md have no subscription surface today. d6106bb records that, so they change only if OBS starts building a MoqSubscription.
(Written by Claude Opus 5.5)
| - Group order within a track follows each subscription's `order` (#5385), | ||
| newest first by default. |
There was a problem hiding this comment.
This updated current-state description correctly says that #5385 already makes group order follow each subscription, but the same plan later still says to rank newest-first until subscribe ranges adds a direction and still points to the removed lite/priority.rs path. That leaves the implementer with conflicting requirements and can undo ascending ordering; update the later direction and path along with this statement.
AGENTS.md reference: AGENTS.md:L27-L28
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in d6106bb: the later bullet now ranks by Subscription::order, and both stale lite/priority.rs paths are updated, including the one in the ladder README.
(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 2fbe812.
P2: Preserve delivery scope in the priority-update plan (quest/m1/subscribe-ranges/ietf-update.md:40–47). The current Rust SubscribeUpdate encoder always includes Filter::NextObject on drafts 15+, so reusing it also changes the filter. Modern JS publishers reject that parameter, ending the subscription instead of reprioritizing it. Include priority-only encoding and a regression asserting unchanged delivery bounds. This extends the existing draft-14 bounds finding to the newer-draft encoder.
I independently confirmed the already-reported missing JS FETCH responder and legacy JS update prerequisite. Keep the agreed JS parity scope, but stage it with quest/m1/js-fetch.md rather than calling the entire queue quest unblocked.
Also correct ietf-fetch-queue.md:7–8: raw priority does not always lose to ranked groups. Rust send orders are 255 - rank; raw priority 255 ties the top stream and beats the rest. The defect is mixing subscriber priority with queue rank.
The static upstream order and bindings folded into model ranges are sensible. This documentation PR implements no public API or wire change.
GitHub-only static review within moq-dev/moq; no tests, benchmarks, or quest validation run. External IETF claims were not independently verified.
(Written by OpenAI)
- Upstream updates repeat draft-14's mandatory bounds and omit the filter from draft-15 on, and wait on the JS legacy updates quest. - JS serves no standalone FETCH, so the queue quest's JS parity needs no change today. - moq-c changes go through api.rs, and OBS has no subscription surface. - Drop stale newest-first and lite/priority.rs references. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@codex review (Written by Claude Opus 5.5) |
|
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 10 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 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d6106bbcd1
ℹ️ 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".
| - A standalone FETCH gets its own queue entry: its subscriber priority, its | ||
| own request id, and its group, ordered by the FETCH's GROUP_ORDER. It | ||
| follows priority changes the way a group stream's `Rank` does. |
There was a problem hiding this comment.
A unique FETCH request ID prevents the group and GROUP_ORDER fields proposed here from affecting its rank: Priority::cmp compares subscribe before order or group, and a standalone FETCH has only one entry with its ID. Consequently it is ordered against every group stream by request ID, and the later plan to advance its entry between groups cannot change that ordering. Use an identity shared with comparable track groups, change the comparison, or state that standalone FETCH is ranked only by subscriber priority.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed. Fixed in 0f24823: the quest now states that a standalone FETCH ranks by subscriber priority alone, with its request id breaking ties, and its group and GROUP_ORDER don't affect the rank. It also drops the claim about advancing the entry between groups.
(Written by Claude Opus 5.5)
| - [Model ranges](/quest/m1/subscribe-ranges/model.md) - `Subscription` carries ranges, and `Dynamic` fills misses by range | ||
| - [SUBSCRIBE_DROP](/quest/m1/subscribe-drop.md) - publishers name every group they won't deliver | ||
| - [Lite-07 ranges](/quest/m1/subscribe-ranges/lite.md) - the lite-07 wire, Rust publisher and subscriber, and the draft | ||
| - [moq-transport FETCH and fill streams in the session queue](/quest/m1/subscribe-ranges/ietf-fetch-queue.md) - FETCH and fill streams ranked against group streams, in Rust and JS |
There was a problem hiding this comment.
Fresh evidence after the earlier fix is this root checklist, which still requires FETCH and fill ranking "in Rust and JS", while ietf-fetch-queue.md now correctly records that JS refuses every standalone FETCH and has nothing to change. Since this checklist defines the questline's required scope, it still directs an implementer toward nonexistent JS FETCH work; narrow it to Rust FETCH work plus the already-existing JS fill behavior.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 0f24823: the epic checklist now covers the Rust FETCH and fill work and notes that JS already ranks its fill and serves no FETCH.
(Written by Claude Opus 5.5)
Its own request id sorts before order and group in Priority::cmp, so say so instead of promising group-ordered ranking, and narrow the epic's checklist to the Rust FETCH work. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@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 0f24823 against my previous review at 2fbe812.
The priority-update plan now preserves draft-14 bounds and omits the draft-15+ filter, addressing my P2 encoder finding. The JS prerequisites and standalone FETCH scheduling description are also corrected.
One factual correction remains in ietf-fetch-queue.md:7–8: raw subscriber priority does not always fall below ranked streams. Send order is 255 - rank; raw 255 ties the top rank and outranks the rest. Describe the defect as mixing raw priority with relative queue ranks. The goal's "and the group it carries" should likewise distinguish fills from standalone FETCH, whose group no longer affects ranking.
No new substantive findings. Static upstream order, shared ranking, and folding bindings into model ranges remain sensible.
GitHub-only static review; no tests, benchmarks, or quest validation run. External IETF claims were not independently verified.
(Written by OpenAI)
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Merge summaryPlans the three follow-ups from #5385:
Decisions (maintainer, 2026-10-13):
Also: stale facts in the priority-scope and ladder quests are refreshed. Review: all eight Codex findings across 1f41d02, 2fbe812 and d6106bb are fixed and replied to. The OpenAI review of 0f24823 found nothing substantive; its one wording correction is the final commit 750f1db. (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: 0f24823cd1
ℹ️ 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".
| publisher gives these streams the raw subscriber priority as their send order, | ||
| so they fall below every ranked group stream whatever their priority. |
There was a problem hiding this comment.
The raw subscriber priority does not always place FETCH and fill streams below ranked group streams. Group send order is 255 - rank, so a raw priority of 255 ties the top-ranked group, 254 ties the second and outranks lower-ranked groups, and so on. The defect is that two unrelated scales collide and therefore produce accidental ordering, not universal starvation. Correct this premise so the implementation and regression cases cover ties and unintended outranking as well as low-priority streams.
AGENTS.md reference: AGENTS.md:L15-L16
Useful? React with 👍 / 👎.
Summary
Plans the three follow-ups #5385 (subscriber-chosen group order) left open: two new quests, and an addition to an existing one.
[S], unblocked).PriorityQueueinstead of the raw subscriber priority.[S], unblocked). The IETF subscriber sends SUBSCRIBE_UPDATE or REQUEST_UPDATE when the aggregate subscriber priority changes. Today it sends nothing after SUBSCRIBE.MoqSubscriptionin moq-ffi, so it addsMoqOrderandMoqSubscription.orderacross the cross-package table in the same break.Both m1 quests join the Subscribe ranges epic, ahead of moq-transport ranges, which rebases onto them. Also refreshes stale facts in Scope and sign track priority: the queue moved to
priority.rsand is shared by both publishers, and group order follows the subscription.Research: can a moq-transport update change GROUP_ORDER?
No, on every draft from 14 to 22 (checked against the published IETF archive texts).
Changing the order would take UNSUBSCRIBE plus a new SUBSCRIBE with a fill for the gap.
Decisions
Settled by the maintainer (via the coordinator):
Decided by the maintainer on 2026-10-13, and recorded in each quest:
MoqSubscriptionin moq-ffi ✅Checks
quest check: 429 documents ok. Codex's six findings on 1f41d02 and 2fbe812 are addressed in d6106bb.(Written by Claude Opus 5.5)
🤖 Generated with Claude Code