Repository navigation
fix(js): never hold overlapping SUBSCRIBE_NAMESPACEs at a foreign peer - #6714
Conversation
Quest-Claim: 27600c2eaa4bf564
A peer that declared no MoQ Solicit may enforce the drafts' PREFIX_OVERLAP rule, as moxygen does, and refuse a broader watch held beside a narrower one, which turned the JS session blind. Mirror Rust (#6530): - forward.ts runs exclusive discovery at such a peer: a broader request closes the narrower ones beneath it, watched or lingering. - The ietf subscriber holds a request until every overlapping request already closed is off the wire: UNSUBSCRIBE_NAMESPACE written on drafts 14/15, the peer's half ended from draft 16 (FIN or CANCELLED reset, then drain). - A closed reader stays registered while its request drains, so no announcement is appended to it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
kixelated
changed the title
|
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 21 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: c1da613
No actionable bugs found. Separating local watcher closure from confirmed wire teardown is the right direction: broader discovery closes narrower interests, and overlapping replacements wait for ordered UNSUBSCRIBE_NAMESPACE on drafts 14/15 or the peer's stream end from draft 16 onward. Keeping closed readers registered only for that barrier, while excluding them from announcement delivery, avoids reviving closed consumers. Explicit route withdrawal during replacement is consistent with not assuming publisher identity.
Verification: static inspection of forwarding, subscriber teardown and RequestStream ownership, plus new exclusive-forwarding and draft-15/19 tests. I did not run tests or cross-implementation interop; the draft-16–18 FIN-cancellation path was inspected, not exercised.
![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: c1da6139fc
ℹ️ 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>
# Conflicts: # doc/concept/standard.md
Drafts 19-21 were dropped on main, and VERSION is now draft 18, whose FIN cancels, so the test pins draft 22 and the helpers take its version. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: 96d9a58
Compared with c1da613, accounting for main integration. The independent wall-clock assertion finding #6714 (comment) is addressed: the absence assertion now observes a pending stream read after confirmed session progress, then awaits that same read after peer closure. Moving this reset-cancellation case to supported draft22 preserves its purpose after drafts19–21 were removed.
No new actionable bugs found. The focused test changes strengthen verification without changing the reviewed overlap-barrier design.
Verification: static inspection of both test-fix commits and surrounding subscriber, forwarding and cancellation paths. No tests or interop runs performed.
|
Merged
(Written by Claude Opus 5.5) |
The adapter's lost-AUTH change (#6734) goes with the adapter; its session tests move to draft 18. The JS overlapping SUBSCRIBE_NAMESPACE wait (#6714) keeps only its stream-cancel path, and the moql SETUP tests from Client::with_url (#6749) drop their draft-14 case. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Finishes quest/m1/ietf-lazy-discovery.md: a JS moq-transport session never holds two overlapping SUBSCRIBE_NAMESPACE requests at a peer that declared no MoQ Solicit. A peer that enforces PREFIX_OVERLAP (such as moxygen) no longer refuses a broader watch and turns the session blind. This mirrors the Rust side from moq-dev/moq#6530.
Changes
connection/forward.ts: at such a peer, discovery is exclusive. A broader request closes every narrower request beneath it, watched or lingering. Our own peers keep the overlapping hold semantics.ietf/subscriber.ts: a request goes out only after every overlapping request already closed has left the wire. On drafts 14/15 that means the UNSUBSCRIBE_NAMESPACE is written, since requests share the ordered control stream. From draft 16 it means the peer has ended its half: we FIN where FIN cancels (drafts 16-18) or reset with CANCELLED (draft 19+), drop whatever the peer still sends, and wait for its end. This replaces the immediate FIN plus STOP_SENDING, which gave no signal. A request's routes go as soon as it starts ending.ietf/request_stream.ts:RequestStream.cancel()cancels a request but keeps reading.wire.Establishedview gainsexclusive: ietf sets it tosolicit === undefined, lite sets it tofalse.Choices made
Impact
Checks
just checkpasses (JS check and tests, markdown, quest, shell).forward.test.ts; a draft-19 overlap that waits on the peer's half, and draft-15 ordering behind UNSUBSCRIBE_NAMESPACE, insubscriber.test.ts. The draft-19 test fails without the change.just test interop --all, since no wire format changed.🤖 Generated with Claude Code
(Written by Claude Opus 5.5)