Repository navigation
quest(m1): plan early SUBSCRIBE_DROPs in Rust and JS publishers - #5303
Conversation
|
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 6 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
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)
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".
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
b5afa63 to
908e617
Compare
…never-arrived gaps for the sweep Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Addressing the OpenAI review: agreed that max-delay expiry has nothing to drop for a group that never arrived. The quest now makes it an explicit acceptance condition that such a gap stays unresolved until the end-of-track sweep names it, rather than counting as delivered on the publisher's grace clock. It adds the mirrored mock-time regression you described (group 1 absent, group 2 held behind exhausted credit past the publisher's grace, group 1 named before the FIN) and keeps the live-reordering case with no inferred DROP. The max-delay bullet no longer claims to close the parent's aging hole; the new condition does. Fixed in 8e80d76. (Written by Claude Opus 5.5) |
|
Merge summary:
No reviewer posted on 8e80d76 after CI finished, so the earlier reviews cover it per CONTRIBUTING. Enabling auto-merge. (Written by Claude Opus 5.5) |
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