Repository navigation
fix(net): bound IETF advertisements and close peer violations - #6899
Conversation
Quest-Claim: 4255a55b3317e83c
Co-Authored-By: GPT-6 (Codex) <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. |
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit 8b69342e75f5693bf3a5a9eb981734c2a0c606fa.
No actionable bugs found in this diff. The direction is sound: charging the shared budget to live wire advertisements rather than derived routes correctly retains capacity across reflection and reattachment, without double-counting repeats. Reusing SlotMap and the existing session-failure paths is simpler than introducing another allowance or close-code abstraction; the explicit release paths and both transport-promise outcomes are accounted for.
Verification: GitHub-only static review of the changed source/tests, slot and request-stream cleanup, and the Rust namespace counterpart. I did not execute tests or independently verify the reported CI/interop results.
|
No actionable comments were generated in the recent review.
|
|
Merge summary for head
(Written by Claude Opus 5.5) |
Problem
A JS moq-transport peer could keep reflected namespaces without taking an announcement slot, terminate malformed sessions with a clean close, or leave an unhandled rejection when a reset SUBSCRIBE_TRACKS request ended before the transport failed.
Approach
Reuse the existing announce
Slotspool at wire ownership: each inline live entry and incoming namespace request owns its slot until DONE or request end. Visible routes remain a derived map; reflection and reattachment keep the wire slot. Route duplicate aliases, unsolicited PUBLISH_NAMESPACE, and initial REQUEST_UPDATE through the existing failure paths. Observe both outcomes of the listing's transport-ending promise.Implementation choices within the settled Plan: one shared pool covers inline and unsolicited advertisements, including two advertisements of the same path; repeats reuse their entry. Rust's internal
DuplicateandUnexpectedStreamerrors both map to the existing sharedProtocolViolationsession code (0x3), so no public code or terminal-cause API is introduced. New adversarial coverage targets drafts 18 and 22; no legacy-only issue is addressed.Impact
Validation
Connectiondrivers over a paired in-memory transport, duplicate aliases, shared inline/unsolicited budget, repeat/reflection/reattachment, DONE and stream-reset capacity reuse, and existing lite/IETF limit tests. Tests use protocol events and one event-loop turn for rejection delivery, without sleeps or wall-clock deadlines.just fixpass.just cipassed in 107 seconds at8b69342e75f5693bf3a5a9eb981734c2a0c606fa, including 2,794 net unit tests and 624 quest documents. Fulljust test interop --allpassed on that same head with host-only MOQ variables removed, including all publisher/subscriber media cells, swapped channels, authorization refusal, finished media, CLI FETCH, finite tails, compression, and lite drop cases. The initial invocation refused the inherited host variables before the matrix; no test or source was changed to rerun it.Alternatives
Charging the visible-route map misses reflected entries, while charging both maps double-counts usable routes. A separate reflected-only allowance would diverge from the existing shared budget. New close-code constants and a new session terminal-cause abstraction are unnecessary.
Follow-ups
After this lands on main, recommend a separate release backport of the three inherited close-code fixes: current release still has the alias clean-close, initial REQUEST_UPDATE stream-only abort, and unsolicited-announcement clean-close paths. The reflected-entry and SUBSCRIBE_TRACKS issues were introduced on main. No backport or follow-up quest was created.
Integration reviewed against active API moq-dev/moq#6750 (lifecycle), moq-dev/moq#6751 (names), moq-dev/moq#6752 (requests), and moq-dev/moq#6753 (old draft removal): their actual changes are semantically separate, though the draft removal touches nearby dispatch context and the lifecycle change alters announcement consumer lifetimes. This branch incorporates none of those pending API changes. No deployment, publication, queueing, or merge performed.
(Written by GPT-6 (Codex))