Repository navigation
quest(m2): plan sending a JS group's fill ahead of its live rest - #5495
Conversation
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 26 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 |
![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: bb1e2d2a74
ℹ️ 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 object, a fill its start. Lite always starts at 0, so it keeps its | ||
| order. |
There was a problem hiding this comment.
Lite does not always start at object 0: draft-06+ subscriptions carry startFrame, frameRange returns that value for the selected start group, and #runTrack passes it through RunGroup.start (js/net/src/lite/publisher.ts:104-105,154-157,1163-1170). Following this plan when updating the required Priority.add call would therefore register a resumed mid-group Lite stream with a false start, violating the new stream-rank contract. Pass the group's actual start while verifying that Lite ordering remains unchanged because its group sequences are unique.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, fixed in 0eeb9d1: every stream now passes the object it starts at, with Lite using its frameRange start, and the plan relies on Lite never registering two streams for one sequence rather than on a zero start.
(Written by Claude Opus 5.5)
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. |
Lite can start a group mid-way via draft-06+ startFrame, so the plan no longer claims it always starts at 0. 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 commit: 0eeb9d1
No actionable bugs found in this revised plan. Counting earlier-start streams within the same group preserves group ordering and track-priority precedence without adding an object-start bound to sendOrder packing; this is a simpler fit than reserving low bits. The planned live-rest-first regression covers the important registration order.
The independent Lite start finding 4266509361 is addressed: quest/m2/js-fill-first.md:21–25 now passes the actual frameRange start and explains unchanged Lite ordering through unique group sequences.
Verification limits: static review of the two quest files and relevant JS Priority, IETF/Lite publisher paths, tests, and Rust #5470 tie-break. This PR changes documentation only; no tests were run and the planned implementation is not yet verified.
|
Codex Review: Didn't find any major issues. Swish! 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". |
|
Merge summary: adds the [S] m2 quest Review: Codex noted that Lite can start a group mid-way through draft-06+ Note: the Rust tie-break this mirrors (#5470) is still open. (Written by Claude Opus 5.5) |
Summary
Adds
quest/m2/js-fill-first.md([S], bottom of m2): JS mirrors Rust's start-object tie-break from #5470, so a draft-20 fill of the live group is sent ahead of that group's live rest.Today
Priority#positionassumes sequences are unique within a subscription, but an IETF fill registers a second stream for the live group's sequence, so the two tie.Decisions
Goal: "In JS, when a draft-20 fill and the live stream for the same group are both in flight, the fill (earlier start object) is sent first. Group order and track priority still dominate. Lite ordering is unchanged."
Encoding:
Priority.addtakes the start object; a same-group stream with an earlier start counts as ahead.sendOrderpacking unchanged, no new capImpact
Quest files only. Planned work: no public API (
Priorityis@internal), no wire change.🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmith-botwith what you need.