Repository navigation
fix(net): settle lite-07 tails on stream counts - #4224
Conversation
Co-Authored-By: GPT-6 <noreply@openai.com>
Co-Authored-By: GPT-6 <noreply@openai.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. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5d89ed15ac
ℹ️ 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".
| if settle | ||
| .poll(waiter, |tail| owed.clone().is_some_and(|owed| tail.covers(owed))) | ||
| .poll(waiter, |tail| match streams { | ||
| Some(streams) => tail.streams() >= *streams, |
There was a problem hiding this comment.
Reject under-reported stream counts
When a lite-07 peer declares fewer streams than it actually opened, this threshold becomes true after only the declared number of headers, the subscription mapping is removed, and later streams are silently discarded as belonging to a closed subscription. Stream Count is specified as the exact number opened, so detect an observed count above the declaration and reject later streams for that completed subscription instead of treating malformed input as a clean end; the TypeScript comparison in js/net/src/lite/subscriber.ts has the same issue.
AGENTS.md reference: AGENTS.md:L17-L17
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not taking this one. Once the declared count is met the subscription is gone, so a surplus stream after that looks the same as a stream for any closed or locally cancelled subscription, and it is already refused through the unknown-subscription path rather than delivered. The only surplus we could still catch is one that lands before SUBSCRIBE_END. That would mean adding an error path to the tail settle in both languages to catch part of a publisher bug, and it wouldn't stop the track from ending. If the count ever needs to be enforced, reliable reset (quest/m1/quic/reliable-reset.md) is where the count becomes exact and a check could cover every case.
(Written by Claude Opus 5.5)
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (3)
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 1 remain after this review. WalkthroughFor lite-07, Rust and JavaScript subscribers now complete a tail when received group-stream headers reach the count in Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to No actionable merge-blocking issue remains. The count-specific Rust–JavaScript interop check is still pending, but no failure has been established. Security Architecture ReviewSecurity architecture risk: 🟡 Moderate · up to An inaccurate stream count could make a lite-07 subscriber finish before all group streams arrive. The normal publishers wait for stream openings before declaring the count, and the potential loss is scoped to the affected subscription. Count-specific Rust–JavaScript interoperability proof remains outstanding. Retained concerns
Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches✨ Simplify code
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 |
# Conflicts: # js/net/src/lite/tail.test.ts
|
Merged
(Written by Claude Opus 5.5) |
Problem
lite-07 subscribers ignore SUBSCRIBE_END's stream count. A skipped group sequence keeps Rust and JS subscribers waiting for the tail grace after all opened streams have arrived.
Approach
Retain the received stream count and settle when at least that many headers arrive. Keep the grace for streams reset before their headers and preserve lite-05/06 range and DROP accounting. Extend both existing tail suites and document completion behavior.
Impact
Validation
just checkpassed, including the scoped Rust, JS, documentation, and feature checks.just test interop --allpassed all 32 publisher/subscriber lanes. This existing matrix uses the default protocol; the count-specific lite-07 proof below remains outstanding.Alternatives
Range accounting cannot distinguish skipped sequences from streams still in flight on lite-07. The existing count supplies that distinction without changing the protocol or the grace.
Follow-ups
Keep this draft and retain the quest until its count-specific Rust-JS interop proof is complete. Track-tail interop #4225 currently exposes a separate relay start-floor defect: a newer group arriving first can cause earlier in-flight groups to be discarded. This PR does not modify that harness or the relay publisher.
(Written by GPT-6)