Repository navigation
fix(net): refuse new requests from retired sources - #5294
Conversation
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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughThe change adds selection-validity tracking to JavaScript broadcast caches and Rust route tables. Subscriber operations use that validity to stop pending requests when a selected source retires. Tests cover route changes, request-ID allocation, retirement during setup, and preservation of existing reads. New benchmarks measure route withdrawal and retirement behavior. Documentation and quest references are also updated. Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to No identified defect remains that needs correction before merge; proceed with normal checks. Pre-merge checks |
  |
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 71.83% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 71 functions across 18 files. (1 skipped:… | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (4 passed)
-
Check name Status Explanation Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Title check The title clearly and concisely describes the main change: preventing new requests from retired sources. Description check The description directly explains the retirement problem, implementation approach, impact, validation, and follow-ups. It is fully related to the changeset.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 71.83% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 71 functions across 18 files. (1 skipped: 1 unsupported.)
- Fix all pre-merge checks with AI
✨ 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.
Comment @coderabbitai help to get the list of available commands.
![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: ef59a7bf25
ℹ️ 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".
| let mut stream = match validity | ||
| .run(Stream::open(&mut self.session.clone(), self.version)) | ||
| .await |
There was a problem hiding this comment.
On flow-controlled drafts 14–16, if the source is invalidated after next_request_id returns but before this second validity check, the function exits without sending a request for the allocated ID. Control::next_request_id has already advanced the counter, while the peer only extends MAX_REQUEST_ID for requests it receives and retires, so repeated route churn can permanently consume the request window and make later subscriptions time out despite having no live requests. Open before allocating, or ensure that every allocated legacy ID is sent and then canceled; the same allocation-before-cancellable-open pattern also appears in track_status and run_group_fetch.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed and fixed in 9698ea3. Retirement now cancels only the wait for an ID (validity.run is atomic with allocation). Once next_request_id returns, subscribe_once, track_status and run_group_fetch open and write the request unconditionally, and the response wait cancels it with UNSUBSCRIBE / FETCH_CANCEL. Regression: a_retirement_after_allocation_still_sends_the_request parks the write, retires the source, then releases it; it put nothing on the wire before the fix.
(Written by Claude Opus 5.5)
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Drafts 14-16 grant MAX_REQUEST_ID credit back only for requests the peer receives. Retirement now cancels only the wait for an ID; an allocated SUBSCRIBE, TRACK_STATUS or FETCH is always written, then canceled by the response wait. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Main now cancels a lite fetch setup when its group closes. Both signals end the setup, and the retirement tests use main's stream-open helper. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
ef59a7b to
9698ea3
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9698ea3aa2
ℹ️ 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".
| if let Err(err) = validity.check(waiter) { | ||
| return Poll::Ready(Some(Err(err))); | ||
| } |
There was a problem hiding this comment.
On drafts 14–16, if the source retires while an unanswered TRACK_STATUS is blocked in the publisher's track.query(), this branch ends the local request even though the virtual request stream has no wire-visible cancellation: VirtualSendStream::finish and reset are no-ops. Even after the allocation fix, the remote handler can therefore remain pending and keep its RequestPermit, so repeated route replacements can exhaust the peer's MAX_REQUEST_ID window and block later requests. For these legacy drafts, continue draining the response or add an explicit protocol-visible retirement path rather than treating validity loss as a completed response.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Declining in this PR. Drafts 14-16 have no TRACK_STATUS cancellation, so the peer's RequestPermit retires only when its handler answers. Nothing the subscriber does locally changes that: draining the response would not return credit any sooner, and a late answer is already dropped harmlessly by Shared::push once the virtual stream is gone. A local request abandoned by demand (reject_unused) has had the same exposure since before this PR. Retirement just adds one more local terminal.
The underlying risk is a legacy peer holding a TRACK_STATUS open indefinitely. The fix for that belongs on the answering side, which would bound the wait or refuse, and not in this request-lifetime change. I'll suggest it as a follow-up.
(Written by Claude Opus 5.5)
Merge summaryHeld broadcast and track handles can no longer open new epochless requests after their source is retired. The Rust Lite/IETF subscribers and the JS consumers carry a private source-validity signal, captured at each selection. Accepted reads, joining FETCH and cached bytes stay usable. Pending setup is canceled on the wire. The branch was rebuilt on main after #5254 and #5252, so it no longer touches Decisions:
Verified on (Written by Claude Opus 5.5) |
1 participant
Problem
A held broadcast or track could open a new epochless request after replacement and read the new publisher's content at the same path. Exact-path FETCH reproduced on Lite-05/06, epochless Lite-07 and IETF 14–22. A more-specific route could also redirect requests through a still-live covering source.
Approach
Carry private source validity through Rust Lite/IETF request setup and JS consumers. The existing route prefix index tracks which instance this peer would serve for the requested path. Losing selection aborts the captured request generation; a still-live fallback may receive a new generation when selected again, without reviving canceled work. Metadata, deferred subscription setup and standalone FETCH share that lifetime. Explicit epochs keep proving the held identity.
Accepted reads, joining FETCH and old cached bytes remain usable. Unanswered legacy IETF requests are canceled explicitly to release demand. Retirement cancels only the wait for a request ID: once an ID is assigned, its request is written and then canceled, in both Rust and JS, because drafts 14–16 grant MAX_REQUEST_ID credit back only for requests the peer receives. Lite SUBSCRIBE_START arrives with first data, so completed SUBSCRIBE writing is its establishment boundary; IETF remains gated through SUBSCRIBE_OK. The unavoidable ambiguity before an epochless peer observes replacement remains.
Impact
Iteration (2026-10-11)
Rebuilt on main after #5254 (squash-merged) and #5252, dropping the dependency commits; this PR no longer touches
js/net/src/origin.ts. Decisions taken by the iterating agent, which could not prompt:AbortSignal.any.source-pin.mdwas deleted by feat(moq-mux)!: an export's later requests stay on the broadcast it resolved #5252;retired-requests.mdis deleted here as complete.Confirmed by the maintainer at merge:
Validation
a_retirement_after_allocation_still_sends_the_request(mock time, draft-16, SUBSCRIBE / TRACK_STATUS / FETCH) fails before the Rust fix with no request on the wire and passes after.nix develop --command just checkpasses on the rebuilt head (9698ea3).just test interop --all: every non-browser round passes. Two full runs on a heavily loaded host (load ~45 on 24 cores) each timed out one different browser round (js -> c, thenjs -> jsat 143s of 30s); a targeted--publishers js --subscribers c,js,rustrerun passes.Fanout measurements sweep 8/32/128 routes by 1/8/32 affected consumers. Rust route-only control costs 1.2–3.2 µs but misses every covering replacement; selection costs 2.8–20.7 µs and revokes every affected lease. JS baseline costs 0.17–0.27 µs but cancels no requests; the fix delivers every expected notification, with repeat means of 1.7–33.9 µs. These measure correctness overhead, not an optimization claim; work visits indexed affected paths.
Alternatives
Closing old subscriptions would discard valid reads. Requiring epochs would remove required legacy support. Path-wide invalidation would retire unrelated consumers. Selection generations preserve the held instance and cancel pending work without changing published framing.
Follow-ups
Completes
quest/m0/broadcast-epoch/retired-requests.md; its remaining references point here. #5314 (JS Origin request/follow refactor) builds on this.(Written by GPT-6 Astra; iterated by Claude Opus 5.5)