Skip to content

feat(net)!: bound peer-declared lengths and per-session requests - #4820

Merged
kixelated merged 25 commits into
mainfrom
quest/m0/request-caps
Oct 8, 2026
Merged

kixelated merged 25 commits into
mainfrom
quest/m0/request-caps

Conversation

@kixelated

@kixelated kixelated commented Oct 5, 2026 •

Copy link
Copy Markdown
Collaborator

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.json row carries announces_peak and subscriptions_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

  • Earlier: the non-exhaustive session::Limits struct with Client/Server with_limits builders, 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.
  • Going past a cap (maintainer 2026-10-07):
    • ✅ Close the session with TOO_MANY_REQUESTS, and add stats showing how close sessions get to the caps.
    • Refuse the one stream or request (the previous behavior: an over-cap announce dropped its whole announce stream).
  • Session code: ✅ reuse TOO_MANY_REQUESTS (0x7), shared with moq-transport, now registered in the lite draft, rather than a new code in lite's own 48-63 range.
  • Stats shape: ✅ announces_peak/subscriptions_peak per auth root (the most any one session held, the largest across nodes), rather than a near-cap counter, which would need an arbitrary threshold.
  • Cluster links under session-fatal default caps (maintainer 2026-10-08): ✅ merge as is, and 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

  • Public API: Rust adds session::Limits, Client::with_limits, Server::with_limits, Error::TooManyRequests, SessionError::TooManyRequests, and stats::Presence::{announces_peak, subscriptions_peak}. JS mirrors SessionCode.TooManyRequests. Existing non-exhaustive enums/structs permit these additions.
  • Wire: encodings are unchanged. The lite-07 draft registers session code TOO_MANY_REQUESTS (0x7) and recommends closing with it past an endpoint's bound on subscriptions or announcements. It also specifies the 65,535-byte boundary. Oversized lite messages are refused earlier, except for the legacy ANNOUNCE_INIT bulk limit. Drafts 14 to 16 advertise and replenish a finite request window and close with TOO_MANY_REQUESTS for violations.
  • Behavior: a peer past a session cap loses the session on lite and moq-transport alike.
  • Stats wire: sessions.json entries gain announces_peak and subscriptions_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 (current main merged): just check passes apart from the moq-uring tests, which need more RLIMIT_MEMLOCK than the local host allows. just test interop --all passes all cross-language pairs and the browser close code; python -> js timed out once at load average 40 and passed on rerun. request_caps.rs covers 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

  • JS has no per-session announce or subscription caps yet. fix(net): grant MAX_REQUEST_ID as draft 14-16 requests close #4966 covers only the request window, whose numbers this PR does not change. A JS mirror of the caps and their session close is a follow-up.
  • quest/m0/relay-session-limits.md: relay config and bindings for session::Limits; gates the next release.
  • A draft-16 SUBSCRIBE_NAMESPACE test for the request-window permit (Grok finding 2); it needs a scripted peer.
  • 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)

kixelated and others added 9 commits October 4, 2026 20:41
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>
@kixelated

Copy link
Copy Markdown
Collaborator Author

Quest outcome: quest/m0/request-caps is implemented and just check passes locally. Left as a draft: the four open decisions in the description (shape of Limits, the defaults, the lite refusal code, and whether to put the 64 KiB cap in the draft) need a maintainer call. Follow-ups are listed in the description.

(Written by Claude Opus 5.5)

@kixelated
kixelated marked this pull request as ready for review October 5, 2026 20:35
@coderabbitai

coderabbitai Bot commented Oct 5, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: ce37dc73-f282-402b-a883-b57c70596794
📥 Commits

Reviewing files that changed from the base of the PR and between 3afe557 and f714789.

📒 Files selected for processing (11)
  • doc/bin/relay/config.md
  • drafts/draft-lcurley-moq-lite.md
  • rs/moq-net/src/error.rs
  • rs/moq-net/src/ietf/adapter.rs
  • rs/moq-net/src/ietf/group.rs
  • rs/moq-net/src/ietf/publisher.rs
  • rs/moq-net/src/ietf/subscriber.rs
  • rs/moq-net/src/lite/publisher.rs
  • rs/moq-net/src/lite/subscriber.rs
  • rs/moq-net/src/session.rs
  • rs/moq-net/src/stats.rs

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


Walkthrough

The change adds 65,535-byte limits for Lite control messages and SETUP messages, with a 64 MiB exception for ANNOUNCE_INIT, and caps IETF object property blocks at 64 KiB. Client and server builders now pass configurable announce and subscription limits into sessions. Drafts 14–16 use those limits for request-ID windows. The change also adds TOO_MANY_REQUESTS, records per-session peak counts, and adds tests and session-churn benchmarks.

Priority: ➖ Normal

Merge Risk: 🔵 Low · up to f7147

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)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning 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… Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
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.
Title check ✅ Passed The title clearly summarizes the main changes: bounds on peer-declared lengths and per-session requests. It is concise and specific.
Description check ✅ Passed The description directly explains the limits, session behavior, request-window changes, statistics, validation, and follow-ups described by the changeset.
Full details: Docstring Coverage

Explanation

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.)

  • Fix all pre-merge checks with AI
✨ Finishing Touches
✨ Simplify code
  • Commit to this branch
  • Create a new PR
  • Autopilot · 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 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: 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.

kixelated and others added 2 commits October 5, 2026 22:44
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@kixelated

Copy link
Copy Markdown
Collaborator Author

Automated review of 4b759653 (head 4b75965)

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 live >= window check: the IDs it may use never exceed window + retired. The parity math matches on both roles (client.rs advertises initial_max_request_id(_, false), and the client Control uses with_window(_, true), which computes the same value). The >= comparison matches the draft's "max plus 1" semantics. Slot bookkeeping in lite Announced is balanced across reserve/withdraw/declined/restart. The 64 KiB lite cap doesn't touch FRAME or datagrams, which don't go through Message, and JS already honours incoming MAX_REQUEST_ID updates. No blocking issues.

Non-blocking

  1. Going one announce over the cap silently drops every announcement on that prefix for the rest of the session. On lite, AnnouncePrefix::poll turns TooManyRequests into a reset plus Ok(()) (lite/subscriber.rs:1276-1295). SubscriberDriver::poll then swap_removes the prefix (lite/subscriber.rs:573), and nothing ever reopens it. On draft-17+, the over-cap NAMESPACE returns Err from run_namespace_entries (ietf/subscriber.rs:1065). Its caller releases every live namespace on that stream and logs "subscribe_namespace failed, continuing without" (ietf/session.rs:281-288, 429-436). The usual interest prefix is the root, so a peer at announces + 1 ends up with a session that looks healthy but routes none of its broadcasts until it reconnects, with only a warn! to show for it. That's arguably worse than closing the session, since closing at least triggers reconnect logic and gets noticed. It barely matters at the 100,000 default, but it's exactly what the planned moq-relay follow-up with tighter client limits will hit. Options:

    • close the session with a clear error on this path;
    • decline only the over-cap announcements (track the id so ENDED and RESTART still resolve, but attach no route);
    • re-issue the prefix's announce request after a backoff.

    If you keep it as is, the "neither closes the session" sentence in doc/lib/rs/moq-net.md should say what the session loses.

  2. The draft-16 SUBSCRIBE_NAMESPACE permit path has no test (ietf/session.rs:1020-1030). It's the only place a permit is taken outside the adapter, and it's held by a task wrapper rather than the recv stream. tests/request_caps.rs only churns SUBSCRIBEs, so a regression that dropped _permit early (or never) wouldn't show up. A small churn of SUBSCRIBE_NAMESPACE through a window of a few on draft-16 would cover both the release and the grant waking the adapter.

  3. The window counts requests that Limits doesn't cap. FETCH, TRACK_STATUS, peer SUBSCRIBE_NAMESPACE, and every SUBSCRIBE_UPDATE all take permits, while requests() is sized as 2 * (announces + subscriptions). The 2x headroom covers this at the defaults. With small or zero limits, though (for example announces: 0, subscriptions: 0, which advertises MAX_REQUEST_ID 0 or 1), a peer can't even open its SUBSCRIBE_NAMESPACE, and one more request is session-fatal TOO_MANY_REQUESTS rather than a per-request refusal. Consider a floor on the window, or a note on Limits that it also sizes the request window on drafts 14 to 16.

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 moq-relay exposes lower limits.

This is an automated review, not the maintainer's decision
(Written by Grok)

@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: 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.

@kixelated

Copy link
Copy Markdown
Collaborator Author

Iterated on this PR:

  • Merged origin/main. Fixed the quest/m0/README.md conflict and removed a new stale link to the deleted quest from quest/m2/ietf-malformed-close.md.
  • Settled all four open decisions using the recommended option. They are recorded under Decisions in the description.
  • Decision 4 changes the lite draft. The Message Length section now says: except in FRAME, a Message Length over 65,536 bytes is unexpected. A sender MUST NOT exceed it, and a receiver MAY reject it from the length prefix alone. The lite-07 changelog bullet now covers this. just drafts check passes.
  • No review findings needed action. The automated review had no correctness findings.
  • just test interop --all: every pair passes except python -> js, a browser audio timeout. It fails the same way on unrelated branches.

(Written by Claude Opus 5.5)

@kixelated

Copy link
Copy Markdown
Collaborator Author

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 not-supported.md drops its own ceiling half and keeps only NOT_SUPPORTED. Please move the 64 KiB (65,536) cap and the lite draft text to 65,535.

The SETUP rule from lite07-finalize.md still applies: SETUP keeps its MUST unless the general rule is raised to the same MUST.

(Written by Claude Opus 5.5)

kixelated added a commit that referenced this pull request Oct 6, 2026
Per the 2026-10-06 audit: Lite NOT_SUPPORTED drops the 65,535-byte message
ceiling, which request caps (#4820) owns, and token-in-band and bindings
require the FFI shape line so the wrappers break once. Matches main (#4946).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
kixelated and others added 3 commits October 6, 2026 23:25
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>
@kixelated

Copy link
Copy Markdown
Collaborator Author

Automated follow-up review of 336405b4 (head 336405b), after the last review on 4b759653

This push lowers the lite control-message cap from 65,536 to 65,535 bytes and resolves the merge onto main's new Encoder/Decoder coding API. The 65,535 cap now applies everywhere it should: MAX_MESSAGE_SIZE (rs/moq-net/src/lite/message.rs:8), MAX_SETUP_SIZE (rs/moq-net/src/setup.rs:14), and both JS constants (js/net/src/lite/message.ts:7, setup.ts). The draft text and the boundary tests (65,535 waits for the body, 65,536 is refused) match it. The conflict resolution looks right to me. w.since(&prefix) measures only the body, which is the same quantity decode_size checks, so the encode-side refusal in AnnounceBroadcast (lite/announce.rs ~258 and ~289) and in the generic Message::encode lines up with the receiver. w.discard(prefix) drops the reserved byte, so a refused message leaves nothing on the wire. The FetchObject properties path still goes through ObjectExtensionsLength before buf.slice. The merge had dropped the 64 MiB ANNOUNCE_INIT exemption, and a069d93b puts it back. announce_init_waits_for_a_large_body and the JS counterpart now pin it on decode, and encode uses the same Self::MAX_SIZE, so that path is covered too. No new blocking issues.

Non-blocking

  1. New nit: doc/lib/rs/moq-net.md:95 still says "64 KiB for a moq-lite control message". The cap is now 65,535 bytes, one under 64 KiB, while the object property block (MAX_OBJECT_EXTENSIONS, ietf/group.rs) really is still 65,536. The sentence now gives one number for two different limits. Say "65,535 bytes for a moq-lite control message … and 64 KiB for an object's property block", or align the two constants.

Earlier findings

  • 1 (one announce over the cap silently drops the whole prefix for the session): still open. AnnouncePrefix still resets and retires the prefix on lite, and draft-17+ still "continues without" the namespace. The doc's "neither closes the session" sentence still doesn't say what the session loses.
  • 2 (no test for the draft-16 SUBSCRIBE_NAMESPACE permit path): still open. tests/request_caps.rs still only churns SUBSCRIBEs.
  • 3 (the request window counts requests that Limits doesn't cap, so small or zero limits make one extra request session-fatal): still open. There's no floor on the window and no note on Limits.

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 336405b4 when this was written.

Verdict: MERGE (once CI is green). As before, finding 1 should be settled before moq-relay exposes lower limits.

This is an automated review, not the maintainer's decision
(Written by Grok)

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@kixelated

Copy link
Copy Markdown
Collaborator Author

Automated follow-up review of a6e5d87e (head a6e5d87), after the last review on 3afe5570

This push fixes a stats gap: when a session changes tier (auth.rs ~520 calls set_tier on a refreshed token), the new tier's announces_peak / subscriptions_peak now start from what the session still holds, instead of zero until the next acquire. Slots::with_stats registers a Weak of its live counter on the stats::Session, and set_tier reads each live one into the new presence row. The logic looks right. Lock order is current then held, and nothing takes them the other way, so there's no deadlock. An acquire racing a tier change is fine: hold waits on current, so it either lands on the old row before the swap or on the new row after the seed. The old tier keeps its history, and the new test checks both rows (5 on the old tier, 3 on the new one).

Non-blocking

  1. held only shrinks in set_tier, so a reconnecting client keeps growing it. stats.rs ~1250 (track_held) pushes one entry per Slots::with_stats, and dead Weaks are dropped only inside set_tier's retain. set_tier is only called on accepted relay sessions (moq-relay/src/auth.rs ~520). A moq_net::Client keeps one stats::Session and clones it into every connect (client.rs ~154). So a reconnecting moq_tokio connection adds two entries on every reconnect or GOAWAY migration, and never drops them. Cluster dials are an example (cluster.rs ~1708 with with_reconnect(true)). Each dead Weak also keeps its ArcInner<AtomicUsize> alive, so that's about 40 bytes per Slots for the life of the link. It's small but has no bound on a flapping link. Fix: run the same retain(|(_, w)| w.strong_count() > 0) in track_held before the push, which keeps the list down to the counters still alive. A test that builds a few Slots on one session, drops them, and checks the length would pin it.

Still open from the last review

  1. Should fix: relay mesh links get the same session-fatal caps, and nothing can raise them. This push doesn't touch rs/moq-relay, rs/moq-native or rs/moq-tokio, so there's still no with_limits call. Cluster peer links keep the 100,000 / 10,000 defaults, and with close-on-cap a link carrying more than 10,000 tracks closes and flaps on reconnect. It still needs a cluster exemption (or much higher caps for cluster_peer sessions) and a relay flag for client sessions.
  2. No test for the draft-16 SUBSCRIBE_NAMESPACE permit path.
  3. ietf/subscriber.rs ~1536 still takes the slot before origin.dynamic checks scope, so at the cap an out-of-scope namespace closes as TOO_MANY_REQUESTS instead of being refused as unauthorized.

Cross-PR

#5010 and #4039 are still open and still add two links each to the quest/m0/request-caps.md file this PR deletes.

CI: no checks had started on a6e5d87e when this was written (only Auto-merge, skipped).

Verdict: ITERATE. This push's stats fix is correct, and the unbounded held list is a one-line fix. The verdict stays ITERATE only because item 2 (relay mesh caps) is still open.

This is an automated review, not the maintainer's decision
(Written by Grok)

# Conflicts:
#	rs/moq-net/src/lite/publisher.rs
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@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: 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.

Comment thread rs/moq-net/src/ietf/adapter.rs Outdated
Comment on lines +862 to +865
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)?);

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.

[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.

@kixelated kixelated Oct 8, 2026 •

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.

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)

kixelated and others added 2 commits October 7, 2026 19:01
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
@kixelated

Copy link
Copy Markdown
Collaborator Author

Automated follow-up review of 849e8e55 (head 849e8e5), after the last review on a6e5d87e

This push is one real commit (81cf8652) plus two merges of origin/main. Both merges are mechanical: the PR's own diff against the new main is identical apart from context lines.

81cf8652 fixes a window bypass that came in with the main merge. After #4961 routed draft 14–16 SUBSCRIBE_UPDATE to its target, the adapter charged the target's ID against MAX_REQUEST_ID. That ID was already accepted, so an update whose own Request ID was past the window was let through. classify now decodes both IDs (adapter.rs ~1013), run_read calls control.accept(own) and pushes to target (~862), and draft 17+ still fails as UnexpectedMessage in decode_update_target. I checked Control::accept (control.rs ~77): with window 2, the new test an_update_past_the_window_closes_the_session (ID 4, target 0) hits id >= max and returns TooManyRequests. The old code would have accepted target 0, so the test pins the regression. test_subscribe_update_reaches_its_target now also asserts own. This looks correct.

Fixed since the last review

  1. Unbounded held list on reconnecting clients. track_held (stats.rs ~1261) now runs retain(|(_, live)| live.strong_count() > 0) before the push, so a reused stats::Session keeps only live counters. Small gap: no test checks the list length after a few Slots are dropped and re-registered, so a later refactor could regress it silently.

Still open

  1. Should fix: relay mesh links get the same session-fatal caps, and nothing can raise them. This push doesn't touch rs/moq-relay, rs/moq-native or rs/moq-tokio. quest/m1/relay-session-limits.md defers the relay config, but until then cluster peer links keep the 10,000-subscription default, and with close-on-cap a link carrying more than that closes and flaps on reconnect. Landing a cluster exemption (or a much higher cap for cluster_peer sessions) in this PR would remove the risk. Otherwise, the relay quest should block a release.
  2. No test for the draft-16 SUBSCRIBE_NAMESPACE permit path.
  3. ietf/subscriber.rs still takes the slot before origin.dynamic checks scope, so at the cap an out-of-scope namespace closes as TOO_MANY_REQUESTS instead of being refused as unauthorized.

Cross-PR

#5010 and #4039 are still open and still link to quest/m0/request-caps.md, which this PR deletes.

CI: only Quest has passed on 849e8e55 so far. Check, Test, WASM, Windows, macOS, Android and Replay are pending.

Verdict: ITERATE. The window fix and the held fix are both correct. The verdict stays ITERATE only because item 2 (relay mesh caps) is still open.

This is an automated review, not the maintainer's decision
(Written by Grok)

@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: 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.

Comment thread drafts/draft-lcurley-moq-lite.md Outdated
- 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.

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.

[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.

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.

Removed in 34920ce1.

(Written by Claude Opus 5.5)

@kixelated

Copy link
Copy Markdown
Collaborator Author

Merge is held on one decision: item 2 of the Grok review of 849e8e55, about cluster links under session-fatal default caps.

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 quest/m1/relay-session-limits.md lands, the relay cannot raise the caps for cluster peers. Options:

  1. (Recommended) Merge as is, and mark the relay-session-limits quest as blocking the next release. main doesn't ship, and the defaults were already sized for meshes.
  2. Add a cluster exemption, or much higher caps for cluster_peer sessions, in this PR before merging.
  3. Hold this PR until relay-session-limits is done and land them together.

The other open items:

  • Item 3 (no draft-16 SUBSCRIBE_NAMESPACE permit test) is a coverage gap. It can follow up with the relay quest.
  • Item 4 (out-of-scope namespace at the cap) was answered on its thread.

Every finding from the OpenAI review of 5589b89e is fixed in 81cf8652.

(Written by Claude Opus 5.5)

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@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: 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 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: 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.

@kixelated

Copy link
Copy Markdown
Collaborator Author

Merge prep, final head d9ba19cb1:

  • Merged main (ec589de4c). Conflicts: doc/bin/relay/config.md now keeps both main's per-epoch stats sentence and this PR's announces_peak/subscriptions_peak sentence; quest/m0/README.md drops the request-caps.md entry main had re-added. Main also added links to the deleted quest in quest/m0/track-stream-demand.md and quest/m1/js-session-parity.md; both now cite feat(net)!: bound peer-declared lengths and per-session requests #4820 instead, and the TRACK boundary test expects a session close with TOO_MANY_REQUESTS, matching the settled policy.
  • Cluster-cap decision (maintainer, 2026-10-08): merge as is. relay-session-limits.md moves from m1 to m0 and gates the next release, so a large mesh link can't flap on the 10,000-subscription default once released.
  • Then merged main again (d9ba19cb1), cleanly, picking up fix(net): pass the epoch to request_broadcast in dial_split_horizon #5045's test fix.
  • Settled earlier: past a cap the session closes with TOO_MANY_REQUESTS (0x7); sessions.json rows carry announces_peak/subscriptions_peak, documented in doc/bin/relay/config.md and doc/concept/stats.md.
  • OpenAI review of ec589de4c: no findings. Open follow-ups: the draft-16 SUBSCRIBE_NAMESPACE permit test and the JS cap mirror (quest/m1/js-session-parity.md).

Local: just check passes apart from the moq-uring tests (host RLIMIT_MEMLOCK). just test interop --all passes; python -> js timed out once at load average 40 and passed on rerun.

(Written by Claude Opus 5.5)

@kixelated
kixelated enabled auto-merge (squash) October 8, 2026 05:02
@kixelated
kixelated merged commit 2717724 into main Oct 8, 2026
11 checks passed
@kixelated
kixelated deleted the quest/m0/request-caps branch October 8, 2026 05:42
kixelated added a commit that referenced this pull request Oct 8, 2026
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant