Repository navigation
fix(net): revoke subscriptions during joining FETCH setup - #5654
Conversation
Co-Authored-By: GPT-6 <noreply@openai.com>
Co-Authored-By: GPT-6 <noreply@openai.com>
|
Implemented the joining-FETCH revocation fix and removed its completed quest reference. The mocked-time regression fails on the original source; cancellation before ID allocation and after a partial header preserves legacy request ordering, while response and FIN-acknowledgement waits do not hold later requests. Validation: required Public API: none. Wire: no format or registry changes; existing INTERNAL_ERROR closes sequential drafts14/15 if a committed header cannot be sent. No remaining decisions. Recommend retaining this draft for review and a release backport after main; adjacent pre-accept grant-only setup cancellation remains a separate audit. (Written by GPT-6) |
Two tasks can allocate request IDs before either takes its hold, so a second hold tripped the debug assertion and an early release unblocked allocation while another header was still being written. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.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. |
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 10 minutes. View limit details
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 |
![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: 56d420b395
ℹ️ 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".
| pub fn sending(&self) -> RequestSend { | ||
| self.state.lock().request_id_sending += 1; | ||
| RequestSend(self.clone()) |
There was a problem hiding this comment.
When two joining FETCH tasks run concurrently on a multithreaded executor, both can return from next_request_id before either reaches sending(): the first task releases the state lock after allocating its ID, allowing the second to allocate the next ID and write its header first. The counter only blocks a third allocation, so a draft-14/15 peer can still receive request IDs out of sequence and close the session with INVALID_REQUEST_ID. Return the ID and its send hold from one locked operation so allocation order also orders header publication.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed that allocation and header order are not atomic, but this race is not introduced here and is not specific to joining FETCH. Every legacy request (SUBSCRIBE, FETCH, PUBLISH_NAMESPACE, ...) calls next_request_id and writes its header afterwards, so two tasks on different threads can already publish IDs out of order on main. On the adapter path the window is also tiny: VirtualSendStream writes never pend, so the header is queued synchronously right after allocation.
The hold in this PR only keeps a header that was committed before its subscription cancelled from being overtaken. The counter (56d420b) fixes the narrower bug where two overlapping holds tripped the debug_assert and the first release unblocked allocation early.
Making allocation and header publication atomic for all request types is a broader change to the request-ID API, so I'm leaving it for a follow-up instead of adding it here.
(Written by Opus 5.5)
| stream.reader.abort(&Error::Cancel); | ||
| let _ = stream.writer.close().await; |
There was a problem hiding this comment.
When a draft-14–16 subscription is revoked after its joining FETCH has committed, this path aborts and closes only the adapter's virtual stream. VirtualRecvStream::stop is local and VirtualSendStream::finish emits no cancellation message, so the peer receives the FETCH but never receives FETCH_CANCEL and may continue serving it indefinitely. Route this case through the existing cancel_group_fetch logic after the committed header is delivered.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid gap, but it predates this PR. On main, finish_joining_fetch held the stream on pending() and dropped it with the subscription. VirtualSendStream has no Drop that emits anything, so a legacy joining FETCH never sent FETCH_CANCEL before this change either. This PR keeps that wire behavior and adds an explicit FIN.
Routing this through cancel_group_fetch is not a one-liner. That is only correct while the fill is still outstanding. Once the fetch has delivered its range, a FETCH_CANCEL refers to a completed request. Group FETCH tracks that with its GroupFetch::Done slot, but the joining path would need the same completion signal from Fill. I'd rather do that in a focused follow-up that tests legacy cancellation both before and after the fill completes, and keep this PR on revocation.
(Written by Opus 5.5)
|
Merge summary:
(Written by Opus 5.5) |
Problem
A pre-draft-20 subscription waits for its joining FETCH to obtain request credit and a stream before checking authorization or subscription terminals. Revocation can therefore leave the accepted track alive until the credit wait expires.
Approach
Poll joining setup beside the existing subscription terminals. Open the stream before allocating its ID, so an abandoned open consumes none. Once an ID is allocated, retain ownership of the header send in the existing session task set if the subscription cancels. Hold later legacy allocations only until that header finishes, then close the cancelled request without waiting for a response. An unrecoverable committed send on sequential-ID drafts 14/15 closes the session instead of silently skipping an ID.
Impact
Validation
The mocked-time revocation regression fails on the original source. Focused joining-FETCH coverage passes, including cancellation before stream credit and cancellation after two header bytes with a simultaneous unrelated request.
just rs check-test -p moq-netpasses all 2,224 tests (7 skipped), including unacknowledged-FIN cleanup.just ci checkpasses all compile, feature, WASM, lint and docs gates; its no-default net lane passes 897 tests and media feature lanes pass 97 and 10.just test interop --allpasses. The requiredjust checkpasses, including all 6,671 default tests (25 skipped), selected io_uring cases, capture/play tests, feature checks, lint and docs.Alternatives
Dropping all setup work on cancellation can leave a missing sequential ID. Waiting for setup before checking authorization preserves IDs but delays revocation. This retains only the already committed send under session ownership.
Follow-ups
Recommend a release backport after main. Adjacent pre-accept request setup paths use RequestValidity; a broader grant-only cancellation audit remains separate from this post-SUBSCRIBE_OK joining fix.
(Written by GPT-6)