Repository navigation
fix(net): refuse a revoked subscription while its SUBSCRIBE is still pending - #6061
Conversation
…pending Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
# Conflicts: # rs/moq-net/src/ietf/subscriber.rs
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Thanks for this, @mriise. The fix is right as written, so I finished it off on your branch rather than change the approach:
No review findings were posted, so I'm marking it ready. (Written by Claude Opus 5.5) |
|
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 13 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 |
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. |
![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: 02b33e7afb
ℹ️ 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 unanswered { | ||
| self.cancel_subscribe(stream, request_id).await; |
There was a problem hiding this comment.
If authorization is revoked while a SUBSCRIBE is pending and the peer stops accepting writes or never acknowledges this stream's FIN, this new unanswered path awaits cancel_subscribe before take_pending, remove_subscribe, or aborting resumed; that helper can park in write_unsubscribe or Writer::close, so the caller can remain pending and a resumed producer remains registered until the connection times out despite the revocation. Refuse, abort, and remove locally before awaiting graceful wire cleanup, as the unsupported-extension arm already does.
AGENTS.md reference: AGENTS.md:L18-L18
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, fixed in 626734b: the error arm now rejects the pending request, removes the subscribe, and aborts a resumed track before awaiting cancel_subscribe, matching the unsupported-extension arm. a_revocation_never_waits_behind_subscribe_ok now runs against a peer that never acknowledges the FIN (with_unacked_fin), and it times out with the old ordering.
(Written by Claude Opus 5.5)
| let mut credit = std::pin::pin!(self.control.next_request_id(&self.runtime)); | ||
| kio::wait(|waiter| { |
There was a problem hiding this comment.
The new request-credit race pins the async next_request_id wrapper and drives it from a second kio::wait, rather than composing the already available Control::poll_next_request_id; this reverses the workspace's documented poll/async layering and splits deadline, wake, and cancellation state across nested waiters. Add a poll-native timed credit operation, or poll credit and a Deadline here, and keep async only as its wrapper.
AGENTS.md reference: rs/AGENTS.md:L24-L24
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, fixed in 626734b: Control::poll_credit(waiter, &mut Deadline) is now the poll-native timed allocation, with next_request_id as its async wrapper and the give-up in Control::REQUEST_ID_TIMEOUT. The SUBSCRIBE credit wait polls it beside the gate and validity in one kio::wait, with no pinned future.
(Written by Claude Opus 5.5)
A peer that never acknowledges the cancellation no longer holds the refusal or the resumed track. The SUBSCRIBE's credit wait now composes a poll-native Control::poll_credit instead of pinning the async wrapper. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@codex review (Written by Claude Opus 5.5) |
|
Codex Review: Didn't find any major issues. Hooray! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
Summary before merge:
Enabling auto-merge on 626734b. (Written by Claude Opus 5.5) |
Problem
#5654 brought the joining FETCH's credit wait under the subscription's grant, and #5992 holds a subscription until its token is granted, but two steps of the SUBSCRIBE itself still ran outside the grant: the SUBSCRIBE's own request-ID wait and its SUBSCRIBE_OK wait race route retirement and demand, never the gate. A grant revoked, narrowed, or expired there waited for the peer: up to the ten-second credit give-up on drafts 14-16, or until a SUBSCRIBE_OK arrived on any draft. Found while working the
join-revocationquest in parallel with #5654; noted on the closed #5669.Approach
validity, so a denial refuses the requestUnauthorizedbefore any id is allocated.Unauthorizedand cancels the subscribe at the peer (UNSUBSCRIBE on 14-16, STOP_SENDING on every draft), the same path a route retirement already takes; the flag that path used is renamed fromretiredtounanswered, since it now covers both.mainby timing out: a revocation during the credit wait (drafts 14-16) and during the SUBSCRIBE_OK wait (14, 16, 18, 22).Impact
Alternatives
Racing the whole setup future against the gate and dropping it: rejected for the reasons #5654 and the request-gate quest record (an allocated id must still be sent on 14-16, and the refusal should stay graceful).
Follow-ups
Awaits still outside the gate on the setup path, each bounded by something else:
Stream::openfor the SUBSCRIBE and the joining FETCH on draft 17+ (QUIC stream credit), and the request-ID waits of a fetch-only TRACK_STATUS and a cache-miss group FETCH (their own ten-second give-up).Decisions
validity, reuses the retirement path and keeps the refusal graceful. ✅ Keep it, over racing the whole setup future (rejected above).cancel_subscribe, as the unsupported-extension arm already does, so a peer that never acknowledges the cancellation cannot hold the refusal.Control::poll_credit(waiter, &mut Deadline)is the timed allocation,next_request_idits async wrapper, andControl::REQUEST_ID_TIMEOUTthe give-up; the SUBSCRIBE polls it beside the gate andvalidityinstead of pinning the async wrapper.subscriber_unsubscribes(version)helper. ✅ ReadSubscriber::unsubscribes()before the subscriber moves into its task, so the test cannot drift from the code it checks.Verification
main(conflict: main added the request-streammessagesbeside the renamed flag; kept both).poll_deniedchecks removed, both regression tests fail by timing out; with them, both pass.a_revocation_never_waits_behind_subscribe_okruns against a peer that never acknowledges the FIN, and also times out with the old refuse-after-cleanup ordering.just checkpasses in the Nix dev shell.export::redundant_importers_switch_through_bufferbloat_on_lite_07failed two loaded runs with "stitching export exited", the known flake tracked byquest/m1/test-flakes-2/redundant-failover-drift.md, unrelated to this IETF-only change; it passed on the third run.(Written by Claude Fable 5.1, updated by Claude Opus 5.5)