Repository navigation
feat(net)!: bound peer-declared lengths and per-session requests - #4820
Conversation
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Lite control messages cap at 64 KiB (ANNOUNCE_INIT keeps 64 MiB), FETCH object properties reuse the 64 KiB object extension cap on every draft, and SessionError gains TOO_MANY_REQUESTS (0x7). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…equest IDs Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Quest outcome: (Written by Claude Opus 5.5) |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (11)
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 0 remain after this review. WalkthroughThe change adds 65,535-byte limits for Lite control messages and SETUP messages, with a 64 MiB exception for Priority: ➖ Normal Merge Risk: 🔵 Low · up to The change adds per-session announce and subscription caps and message-length bounds. Two minor items remain: a documentation wording mismatch about the lite size cap, and an edge case where an out-of-scope namespace could close a session that is already at its announcement cap. Neither is likely to block merge, but the owner should be aware of both. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 69.66% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 145 functions across 30 files. (2 skipped: 2 unsupported.)
✨ 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 |
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: 761aca7
No actionable introduced correctness findings in the full 28-file diff and relevant request-routing/lifetime context. The direction is sound: bounded prefixes reject oversized lite/FETCH metadata before buffering; session-shared RAII slots release on refusal/withdrawal; and draft-14–16 permits return credit through the adapter, including draft-16 native namespace requests. In particular, rs/moq-net/src/ietf/control.rs:79-124 keeps admission/retirement accounting constant-time, and the added integration tests exercise churn beyond the initial window and slot reuse.
Before merging, record the maintainer's decisions on the four already-open API/default/wire-policy choices in #issuecomment-5988341360. I favor retaining the extensible Limits struct, documenting the lite receive ceiling, and giving untrusted relay clients tighter configurable limits in the planned follow-up. The known SUBSCRIBE_UPDATE routing and JS grant limitations are already documented, so I have not duplicated them as new findings.
Verification limits: static review only; I did not run builds, tests, benchmarks, or cross-language interoperability checks. GitHub currently reports merge conflicts; this is not confirmation of merge readiness.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Automated review of The core accounting is correct. The MAX_REQUEST_ID window only grows when a permit retires, so a well-behaved peer can't trip the Non-blocking
CI: Android and Release JS Packages pass. Check, Test, WASM, Windows, macOS, Replay, and Quest were still pending when this was written. Verdict: MERGE (once CI is green). Finding 1 should be settled before This is an automated review, not the maintainer's decision |
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: 4b75965
Follow-up to 761aca7. The branch-specific update after merging main documents the 65,536-byte non-FRAME ceiling (drafts/draft-lcurley-moq-lite.md:696–700,1376). This agrees with rs/moq-net/src/lite/message.rs:7–16,35–50 and js/net/src/lite/message.ts:7,44–46,56–61; ANNOUNCE_INIT's older-version exception does not apply to lite-07. The four policy choices requested in my previous review are now recorded in the PR body, and GitHub currently reports no merge conflict.
Direction remains sound; no additional actionable defect found in this incremental review. The already-discussed announce-stream refusal behavior and low-limit request-window tradeoffs are not new findings from this update.
Verification: static review of the documentation delta and relevant current Rust/JS framing paths, with main merges treated separately. No tests, benchmarks, interop matrix, or current CI validation run.
|
Iterated on this PR:
(Written by Claude Opus 5.5) |
|
A decision from the 2026-10-06 quest audit: request caps (this PR) owns the lite control-message ceiling, and the maintainer set it at 65,535 bytes, matching moq-transport's u16 length. A grant that is too large then fails the same way on both wires. The auth line's The SETUP rule from (Written by Claude Opus 5.5) |
Integrate main codec, epoch, and deterministic test changes while preserving contributor history. The separate IETF object-extension block ceiling remains 64 KiB. Co-Authored-By: GPT-6 <noreply@openai.com>
Co-Authored-By: GPT-6 <noreply@openai.com>
Co-Authored-By: GPT-6 <noreply@openai.com>
|
Automated follow-up review of This push lowers the lite control-message cap from 65,536 to 65,535 bytes and resolves the merge onto main's new Non-blocking
Earlier findings
Cross-PR: #4039's quest says #4820 caps at 65,535 bytes, and the last review there flagged that as wrong because this PR used 65,536. With this push the two now agree, so that nit on #4039 is moot. CI: Check, Test, WASM, Windows, macOS, Android, Replay, Quest, and Release JS Packages were all still pending on Verdict: MERGE (once CI is green). As before, finding 1 should be settled before This is an automated review, not the maintainer's decision |
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Automated follow-up review of This push fixes a stats gap: when a session changes tier ( Non-blocking
Still open from the last review
Cross-PR#5010 and #4039 are still open and still add two links each to the CI: no checks had started on Verdict: ITERATE. This push's stats fix is correct, and the unbounded This is an automated review, not the maintainer's decision |
# Conflicts: # rs/moq-net/src/lite/publisher.rs
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: 5589b89
Follow-up to 3afe557, separating branch changes from merged main. One new P2 finding is attached: the merged update-routing fix makes the request-window check validate the target subscription ID instead of the update's own ID.
The previous tier-transition finding is fixed: stats.rs:1241–1250 seeds the new tier from live counters, and the added regression distinguishes the old peak of 5 from 3 still held. The separate dead-Weak accumulation in stats.rs:1261–1267 remains, as already reported in Grok's follow-up: a reconnecting client reuses its stats context, while registration only appends and pruning requires a tier change. Pruning expired entries when registering is a small fix; no duplicate inline comment added. The previously discussed relay-cap configurability and draft-16 permit coverage gaps remain separate.
Direction remains sound: shared RAII caps and live-usage seeding are straightforward. Keep request identity separate from routing identity at the adapter boundary.
Verification: GitHub-only static incremental/source review, including merge interactions and existing discussion. No builds, tests, benchmarks or interop runs executed. The exact head currently reports only a skipped Auto-merge check; GitHub reports merge conflicts. Open state, head and reviews rechecked before posting.
| Route::FollowUp(request_id) => { | ||
| // SUBSCRIBE_UPDATE takes a request ID of its own (draft-14 section 9.1), | ||
| // done as soon as it is delivered, so it is granted straight back. | ||
| drop(self.control.accept(request_id)?); |
There was a problem hiding this comment.
[P2] Validate the update's own ID against MAX_REQUEST_ID
After the merge, classify() uses decode_update_target() for Route::FollowUp (lines 1013–1015), so request_id here is the existing subscription's ID. Passing it to Control::accept bypasses the window check for the update's own first-field ID. For example, with a two-request window (MAX_REQUEST_ID = 4) and subscription 0 open, an update with own ID 4 and target 0 is accepted and retired rather than closing with TOO_MANY_REQUESTS. This affects drafts 14–16. Carry both IDs, or decode the own ID from body here: validate that ID and retain the target only for shared.push. Add an adapter-level regression with an in-window target and an update ID at the advertised limit; the current routing-only test never exercises this check.
There was a problem hiding this comment.
Fixed in 81cf8652, merged forward in 849e8e55. Route::FollowUp now carries both IDs, { own, target }. The update's own ID goes to Control::accept for the window check, and the target only routes. Regression an_update_past_the_window_closes_the_session uses a two-request window: subscription 0 is open, then an update with own ID 4 targets 0. It now closes with TOO_MANY_REQUESTS; without the fix the session parks instead. The routing test also checks the decoded own ID.
81cf8652 also prunes gone registrations whenever a reused stats context registers again. That covers the dead-Weak accumulation from the Grok follow-up.
(Written by Claude Opus 5.5)
Routing a draft 14-16 update to its target (#4961) made the window check read the target's ID. Carry both IDs. Also prune gone cap registrations when a reused stats context registers again. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
# Conflicts: # drafts/draft-lcurley-moq-lite.md # rs/moq-net/src/ietf/subscriber.rs
|
Automated follow-up review of This push is one real commit (
Fixed since the last review
Still open
Cross-PR#5010 and #4039 are still open and still link to CI: only Quest has passed on Verdict: ITERATE. The window fix and the This is an automated review, not the maintainer's decision |
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: 849e8e5
Follow-up to 5589b89, separating the two-file fix from merged main and checking the conflict resolutions. One low-priority documentation finding is attached; no new actionable runtime defect found in this incremental review.
The update-window finding is fixed: adapter.rs:862–866 validates own and routes with target; the new draft-14 adapter regression exercises own ID 4 targeting subscription 0 with a two-request window. The existing routing test checks both IDs for drafts 14–16. stats.rs:1261–1267 now prunes expired registrations on each registration, addressing the reconnect accumulation already identified in Grok's follow-up.
Direction remains sound: request identity and routing identity are explicit, and stats bookkeeping is bounded by still-live counters. Remove the obsolete hop-stamping changelog line retained by the merge. Existing relay-limit configurability and draft-16 namespace coverage discussions remain separate.
Verification: GitHub-only static incremental/source and discussion review. No builds, tests, benchmarks or interop runs executed; hosted checks are still running. Open state, exact head and reviews rechecked immediately before submission.
| - Capped the SETUP Message Length at 65,536 bytes. | ||
| - Capped the Message Length of every message except FRAME at 65,535 bytes. | ||
| - Added the TOO_MANY_REQUESTS (0x7) session code, closing a session whose peer goes past the endpoint's bound on subscriptions or announcements. | ||
| - A relay puts a random Hop ID, picked per session, in front of an announcement whose reconstructed path starts with 0, and writes that stamp followed by 0 for an empty path. |
There was a problem hiding this comment.
[P3] Remove the obsolete hop-stamping changelog entry
The merge retained this bullet after main removed the stamping behavior. The same draft now says a received 0 is forwarded unchanged and an assigned identity MUST NOT be forwarded (lines 947–949); the merged subscriber code also no longer inserts a stamp. This bullet tells implementers to emit the opposite wire representation for anonymous routes. Remove it to match the normative text and the merged main version.
There was a problem hiding this comment.
Removed in 34920ce1.
(Written by Claude Opus 5.5)
|
Merge is held on one decision: item 2 of the Grok review of With close-on-cap, a relay peer link that carries more than 10,000 subscriptions, or more than 100,000 announces, is closed with TOO_MANY_REQUESTS and flaps on reconnect. Until
The other open items:
Every finding from the OpenAI review of (Written by Claude Opus 5.5) |
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: 34920ce
Follow-up to 849e8e5. The sole new commit removes the stale hop-stamping changelog bullet at drafts/draft-lcurley-moq-lite.md:1406–1408. This fixes my previous documentation finding and now agrees with the normative forwarding rules at :945–949. No runtime changes or new actionable finding in this delta; deleting the obsolete claim is the simplest correct fix.
The previously fixed request-ID/routing-ID and stats-registration issues remain unchanged. The cluster-cap release-policy decision and draft-16 namespace permit test gap remain open discussion items, not new findings.
Verification: GitHub-only static incremental and surrounding-document review; no builds, tests, benchmarks, or interop runs executed. Current-head hosted Check, Test, Replay, WASM, Windows, macOS, Android, and Release JS Packages report success. Open state, head, and existing reviews rechecked before submission.
# Conflicts: # doc/bin/relay/config.md # quest/m0/README.md
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: ec589de
Follow-up to 34920ce. After accounting for merged main, the PR-specific Rust/JS added and removed lines are unchanged. The merge preserves the earlier request-ID/routing-ID and stats-registration fixes; the relay documentation now combines epoch behavior with cap telemetry. No new actionable correctness finding in this incremental review.
The significant change is the recorded release policy: quest/m0/README.md:53–56 and quest/m0/relay-session-limits.md:18–21 now make relay-limit configuration a next-release gate, addressing the previous cluster-cap decision. That is a coherent way to land the bounds while withholding release until large mesh links can avoid session-fatal defaults. The implementation work and the already-discussed draft-16 namespace permit test remain follow-ups; no duplicate inline findings.
Verification: GitHub-only static comparison against both merge bases, relevant merge interactions, source and discussion. No builds, tests, benchmarks or interop runs executed. Current-head hosted workflows are queued or running. Open/non-draft state, exact head and existing reviews rechecked immediately before submission.
|
Merge prep, final head
Local: (Written by Claude Opus 5.5) |
Use SessionCode.TooManyRequests from #4820 in place of the adapter's local constant, and give main's draft-16 SUBSCRIBE_NAMESPACE test a client-parity request id now that the window refuses the wrong parity. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Problem
Peers could declare large lite control frames or FETCH object properties, grow live announcement and subscription tables without a session cap, and exceed the advertised IETF request window. The original PR also used 65,536 where the selected lite framing boundary is 65,535 bytes.
Approach
Reject oversized lite lengths at the prefix and refuse oversized encodings. Cap SETUP and the non-FRAME lite messages formerly capped at 64 KiB at 65,535 bytes. Preserve lite-01/02 ANNOUNCE_INIT's published 64 MiB bulk exception and the independently defined IETF object-extension limit of 64 KiB. Both FETCH layouts share that existing extension decoder.
Use session-shared RAII slots for live announces and subscriptions, defaulting to 100,000 and 10,000. A peer that goes past either cap loses the session, closed with TOO_MANY_REQUESTS (0x7) on every version. Each slot set reports its high-water mark to the session's stats context, so each auth root's
sessions.jsonrow carriesannounces_peakandsubscriptions_peak. Drafts 14 to 16 advertise a finite MAX_REQUEST_ID derived from those limits and grant credit as requests retire. Enforce the window and ID reuse, count SUBSCRIBE_UPDATE and native draft-16 namespace requests, and accept REQUESTS_BLOCKED.Decisions
session::Limitsstruct with Client/Serverwith_limitsbuilders, the 100,000/10,000 defaults, and 65,535 bytes for the existing lite 64 KiB caps. SETUP's MUST error rule and the general sender MUST NOT/receiver MAY prefix-refusal rule reference the same boundary. The legacy ANNOUNCE_INIT and IETF object-extension limits remain unchanged.announces_peak/subscriptions_peakper auth root (the most any one session held, the largest across nodes), rather than a near-cap counter, which would need an arbitrary threshold.quest/m0/relay-session-limits.md(moved from m1) gates the next release, so cluster peers get higher caps before a release ships. Rejected: a cluster exemption in this PR, or holding this PR for the relay quest.Impact
session::Limits,Client::with_limits,Server::with_limits,Error::TooManyRequests,SessionError::TooManyRequests, andstats::Presence::{announces_peak, subscriptions_peak}. JS mirrorsSessionCode.TooManyRequests. Existing non-exhaustive enums/structs permit these additions.sessions.jsonentries gainannounces_peakandsubscriptions_peak(a reader missing them reads 0).Alternatives
Separate builders would duplicate resource knobs, while deriving the request window from the two caps keeps the API small. Refusing per request was the previous behavior, replaced by the maintainer's decision above. Treating the cap only as a resource-exhaustion recommendation would leave Rust, JS, and the draft with inconsistent boundaries.
Validation
At head
ec589de4c(currentmainmerged):just checkpasses apart from themoq-uringtests, which need more RLIMIT_MEMLOCK than the local host allows.just test interop --allpasses all cross-language pairs and the browser close code;python -> jstimed out once at load average 40 and passed on rerun.request_caps.rscovers a subscription and an announce past the cap closing the session with TOO_MANY_REQUESTS on drafts 14 to 17 and lite-06. Unit tests cover each refusal site, and a stats test covers the peaks (per session, not summed; not lowered by a release). Boundary regressions accept 65,535 and reject 65,536 from the prefix; SETUP encoders refuse the next byte. Separate regressions keep the legacy 64 MiB ANNOUNCE_INIT exception. The session-churn benchmark sweeps sessions and held requests across IETF and lite versions.Follow-ups
quest/m0/relay-session-limits.md: relay config and bindings forsession::Limits; gates the next release.quest/m1/session-churn-held.md(held-subscription benchmark slope), the SUBSCRIBE_UPDATE routing issue, and JS request-credit grants remain separate.(Written by Claude Opus 5.5)