Repository navigation
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 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 |
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. |
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: b5afa63.
The focused Rust/JS parity quest follows the independent #5301 finding. Keeping lite-07, reset-before-header handling, and reader errors in the parent is reasonable.
One actionable acceptance gap in quest/m1/subscribe-drop-early.md:28–33: the child explicitly says max-delay expiry also closes the parent's grace-aging hole, but dropping a buffered group does not cover a never-arrived gap. Rust marks a group served before its stream opens, and Tail::expire silently folds missing gaps. An absent group has no buffered object to expire and, correctly, is not an explicit upstream DROP. Keeping the existing end sweep alone can therefore still omit it.
Make preserving unresolved gaps for the final sweep an explicit acceptance condition, rather than treating them as delivered on the publisher's grace clock. Add a mirrored mock-time regression: serve group 0, leave group 1 absent, queue group 2 behind exhausted stream credit for longer than the publisher's grace, then release credit and finish. Group 1 must be named before FIN without waiting for the receiver's later-started grace. Also retain the live-reordering case: no inferred DROP while group 1 could still arrive.
This PR changes planning documents only; no current API or wire change. GitHub-only static review of all three changed files and relevant Rust/JS publisher, subscriber, model, and tail code; tests not run. Head and reviews rechecked before posting; no existing substantive review to duplicate.
(Written by OpenAI)
![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: b5afa63c85
ℹ️ 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".
| - Mirrored Rust and JS publisher unit tests for each trigger: the subscriber's | ||
| owed range settles at the skip, before the track ends and without the grace. |
There was a problem hiding this comment.
The requested pre-track-end assertion cannot pass within this quest's stated scope. JS invokes #settleTail only after #runResponses resolves on subscribe-stream FIN (js/net/src/lite/subscriber.ts:721-727), and Rust similarly enters ServeMode::Tail only after receiving FIN (rs/moq-net/src/lite/subscriber.rs:5179-5208); receiving a DROP merely accounts its range. Because lines 11-12 explicitly defer surfacing drops to readers, nothing externally settles when the early DROP arrives. Test the emitted/accounted DROP and immediate post-FIN settlement here, or pull reader disposition into this child.
Useful? React with 👍 / 👎.
| Update the lite draft's SUBSCRIBE_DROP text to say a publisher SHOULD send it | ||
| as soon as it gives a sequence up. Run `just drafts check`. |
There was a problem hiding this comment.
This child cannot make the requested normative draft change while keeping lite-07 as a non-goal. The current draft has no SUBSCRIBE_DROP message section and explicitly says lite-07 removed it (drafts/draft-lcurley-moq-lite.md:1542-1547), while the parent that depends on this child owns restoring it. Updating only the historical changelog would not establish the new SHOULD, but restoring the message here would prematurely change lite-07. Move this behavior wording to the parent, or move the restoration into this child so the implementation and draft can land consistently.
Useful? React with 👍 / 👎.
This branch has not been deployed
1 participant
Adds Early drops as a child of the SUBSCRIBE_DROP questline, following the Codex finding on #5301 that the JS publisher never sends SUBSCRIBE_DROP.
The new
[L]quest covers lite-05 and lite-06. The Rust and@moq/netpublishers send a DROP the moment they give a sequence up (cursor skips, max-delay expiry, and drops the upstream publisher sent), and both keep the end-of-track sweep. A per-sequence dropped mark in both track models lets a relay forward upstream drops. The parent keeps resets before the header, lite-07, and showing drops to readers.Decisions
How should the drop_unserved port be tracked?
Goal: mirror only Rust's end-of-track sweep in JS?
Early drops in Rust too?
What counts as giving a sequence up?
Tests?
just test interop --tailWhere does the per-sequence dropped mark go?
(Written by Claude Opus 5.5)
🤖 Generated with Claude Code