Skip to content

fix(net): bound IETF advertisements and close peer violations - #6899

Merged
kixelated merged 2 commits into
mainfrom
quest/m1/cut-audit/js-ietf-violations
Oct 27, 2026
Merged

kixelated merged 2 commits into
mainfrom
quest/m1/cut-audit/js-ietf-violations

Conversation

@kixelated

@kixelated kixelated commented Oct 27, 2026 •

Copy link
Copy Markdown
Collaborator

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 Slots pool 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 Duplicate and UnexpectedStream errors both map to the existing shared ProtocolViolation session 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

  • Public API: none. No migration.
  • Wire format: none. Peers causing alias collisions, unsolicited announcements after MoQ Solicit, or an initial REQUEST_UPDATE now receive ProtocolViolation instead of a clean close or only a stream reset, matching Rust.
  • The existing announcement budget now covers all live wire advertisements, including reflected routes, and returns capacity on DONE or request end. Exceeding it keeps the existing TooManyRequests close code.

Validation

  • All ten modern-draft adversarial cases failed before source changes. The early reset case holds the existing readiness gate so a later transport rejection exercises the previously unobserved promise.
  • 18 focused tests / 50 assertions pass, including two running Connection drivers 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.
  • Package TypeScript check and just fix pass.
  • Clean-head just ci passed in 107 seconds at 8b69342e75f5693bf3a5a9eb981734c2a0c606fa, including 2,794 net unit tests and 624 quest documents. Full just test interop --all passed 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))

kixelated and others added 2 commits October 26, 2026 18:03
@kixelated kixelated changed the title JS moq-transport sessions close on peer violations and bound reflected namespaces fix(net): bound IETF advertisements and close peer violations Oct 27, 2026
@kixelated
kixelated marked this pull request as ready for review October 27, 2026 01:29
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 27, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-27T01:30:41.307433Z 8b69342 Draft marked ready
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@kixelated kixelated left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@coderabbitai

coderabbitai Bot commented Oct 27, 2026

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review.

Recent review info
Run configuration
  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 086648fa-bcd5-4a8e-89f1-5ab324d74d65
Commits

Reviewing files that changed from the base of the PR and between b8561a3 and 8b69342.

Files selected for processing: 6
  • js/net/src/ietf/connection.ts
  • js/net/src/ietf/peer-violations.test.ts
  • js/net/src/ietf/publisher.ts
  • js/net/src/ietf/subscriber.ts
  • quest/m1/cut-audit/README.md
  • quest/m1/cut-audit/js-ietf-violations.md
Files with no reviewable changes: 2
  • quest/m1/cut-audit/README.md
  • quest/m1/cut-audit/js-ietf-violations.md

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 1 remain after this review.

Walkthrough

The changes update protocol-violation handling for invalid updates, unsolicited namespace publications, and duplicate track aliases. Subscriber announcement state now accounts for live advertisements and incoming namespace requests with slots. Track-subscription handling maps fulfillment and rejection of the QUIC closed promise to the closed result. Tests cover these behaviors across draft 18 and draft 22. The cut-audit list and audit document were also edited or removed.

Priority: Normal

Merge Risk: Minimal · up to 8b693

No actionable merge-blocking issue remains; the change is ready to merge after normal checks.

Pre-merge checks: Passed 5
Check name Status Explanation
Title check Passed The title clearly summarizes the primary changes: bounding IETF advertisements and closing peer violations.
Description check Passed The description directly explains the advertisement-slot changes, protocol-violation handling, transport rejection handling, tests, and validation results.
Docstring Coverage Passed No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Documentation-only edits and deletions within survi…
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage

Explanation

No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 0 functions across 4 files.

Finishing Touches
✨ Simplify code
  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@kixelated

Copy link
Copy Markdown
Collaborator Author

Merge summary for head 8b69342e75f5693bf3a5a9eb981734c2a0c606fa:

  • Reviews: Codex completed with no findings on this head; CodeRabbit no findings; no unresolved threads.
  • Verified locally on this head merged with current main (through 6d55fb4427, which merges cleanly): just check passed (js check/test including 2,826 @moq/net tests, markdown, gh, quest, shell), quest check passed, and just test interop --all passed (173 cells, no failures or skips).
  • No changes pushed; the head lands as reviewed and the merge queue tests it against main.
  • Follow-up from the PR body: a release backport of the three inherited close-code fixes (alias collision, initial REQUEST_UPDATE, unsolicited announcement) is left for the maintainer to plan.

(Written by Claude Opus 5.5)

@kixelated
kixelated added this pull request to the merge queue Oct 27, 2026
Merged via the queue into main with commit 06c58c7 Oct 27, 2026
12 checks passed
@kixelated
kixelated deleted the quest/m1/cut-audit/js-ietf-violations branch October 27, 2026 04:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant