Repository navigation
fix(net): keep IETF subscriptions alive after a request FIN (backport #4658) - #5133
Conversation
…4658) On drafts 19-22 a requester may FIN its request stream right after SUBSCRIBE (draft-21 section 6.4.2.2). The publisher treated that FIN as a cancellation, so no media flowed. On draft 17+ any REQUEST_UPDATE byte also made Reader::poll_closed fail and silently ended the subscription. Port the subscription half of #4658 to release: - From draft 19, a clean FIN on a SUBSCRIBE or SUBSCRIBE_NAMESPACE stream stops reading but keeps serving; RESET_STREAM or STOP_SENDING cancels. Drafts 14-18 keep FIN cancellation. - From draft 17, a subscription REQUEST_UPDATE is parsed: a priority change is applied and acknowledged with REQUEST_OK, anything else is refused with NOT_SUPPORTED and ends the subscription with PUBLISH_DONE UPDATE_FAILED. - moq-tokio's wrapped send stream observes STOP_SENDING while idle, which the Rust subscriber's FIN plus STOP_SENDING unsubscribe needs once a FIN no longer cancels. 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. |
|
Automated review of The backport looks faithful: d19+ FIN no longer cancels SUBSCRIBE / SUBSCRIBE_NAMESPACE, d17+ REQUEST_UPDATE is read and answered, and the Non-blocking
Verdict: MERGE once CI is green. This is an automated review, not the maintainer's decision |
![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: 8e3b23261c
ℹ️ 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".
| self.finish = true; | ||
| if matches!(self.state.get_mut().unwrap(), Some(SendState::Closing(_))) | ||
| && let Some(tx) = self.interrupt.take() | ||
| { | ||
| let _ = tx.send(()); |
There was a problem hiding this comment.
When poll_closed has put an unfinished stream into SendState::Closing, finish() takes this branch and only records self.finish. If the caller then drops the stream before polling again, the Drop implementation's Closing branch salvages only resets, so it drops the owning future and the backend performs its default reset instead of the FIN for which finish() already returned success. The closing-state drop path needs to reclaim the interrupted future and apply a deferred finish too.
AGENTS.md reference: rs/AGENTS.md:L35-L35
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed this is a real gap, but not one to fix only on release. This hunk is a verbatim copy of #4658, and main has the same Drop Closing branch today, so the fix belongs on main first and then a backport.
It is also not reachable from moq-net on release. The only caller that starts an idle send watch is the publisher's run_subscription (stream.writer.poll_closed), and that stream always ends with a PUBLISH_DONE write, which reclaims it, followed by close().await. Every other writer.finish() call site never polls writer.poll_closed first. I'm tracking it as a follow-up: the Closing drop path should take the deferred finish and apply it in the salvage task, like the Writing branch already does.
(Written by Claude Opus 5.5)
There was a problem hiding this comment.
Confirmed, this is a real bug. Before #4658, Closing was reached only after finish/reset, so no FIN could be deferred there. Now an idle, unfinished stream can sit in Closing, and finish() then drop loses the FIN to the backend's drop reset. moq-net's Writer resets on drop unless closed, so its callers don't hit this today, but it is a contract bug in the adapter. main has it too.
The fix applies a deferred finish in the Closing drop branch, like the Writing branch, with the regression test a_deferred_finish_survives_a_drop_mid_watch, which fails without the fix. It is on main in #5156 and on release in #5164, since this PR already merged.
(Written by Claude Opus 5.5)
| if let Some(priority) = update.priority { | ||
| let mut subscription = serve.track.subscription(); | ||
| subscription.priority = super::priority::from_wire(priority); | ||
| if let Err(err) = serve.track.update(subscription) { | ||
| return Some((Err(err), filled)); |
There was a problem hiding this comment.
When a priority update arrives after a group stream has opened, this only updates the track subscription. Each existing GroupServe captured the old priority when it was constructed and never observes later subscription changes, so a long-running or flow-control-blocked stream remains scheduled at the old priority even after the update is acknowledged. Propagate the new priority to active group writers as well as using it for future groups.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not fixing this in the backport. main behaves the same way: GroupServe sets the priority once for each stream it opens, so an update reaches new groups but not the ones already in flight. This PR's goal is to stop a d19+ FIN from cancelling the subscription. Re-prioritizing in-flight group writers is new behavior that should land on main before it comes to release. I'm tracking it as a follow-up.
(Written by Claude Opus 5.5)
There was a problem hiding this comment.
Confirmed. Each GroupServe captured the priority once, so an acknowledged update reached only groups opened afterwards. main has the same gap. Lite already moves in-flight groups through a per-group kio consumer, so I did the same here instead of refusing priority updates, which would end a subscription over a benign request.
TrackServe now owns the priority in a kio channel. Every group stream follows it while open: at open, while serving, and while waiting for the FIN acknowledgement. The regression test subscription_update_reprioritizes_open_group_streams fails without the fix. It is on main in #5162 and on release in #5164.
(Written by Claude Opus 5.5)
|
Merge summary for
Merging with a merge commit, per the (Written by Claude Opus 5.5) |
fix(net): follow-ups to #5133 (deferred FIN on drop, reprioritize open group streams)
1 participant
Release-only backport of #4658 (
83ce47fe3onmain) for the 0.17.x line, ahead of IETF interop (imquic vs moq-relay on draft-22).The failure
In
rs/moq-net/src/ietf/publisher.rsthe subscription task cancelled as soon asstream.reader.poll_closedfired:Reader::poll_closedreturn an error, which also silently ended the subscription.What is ported
ietf/request_stream.rs(private):fin_cancels(version)(drafts 14-18),poll_cancel(a clean FIN on d19+ setsfinishedand stops reading; afterwards only the writer's STOP_SENDING or a reset cancels), and theUpdatedecoder for a draft-17+ REQUEST_UPDATE on a subscription.run_subscription, same shape as main at fix(net): keep IETF requests alive after FIN #4658. Drafts 14-16 keep today's behavior (anything on the virtual stream ends it). From draft 17 it reads REQUEST_UPDATE frames, and from draft 19 a FIN keeps serving. The END_OF_TRACK marker wait uses the same gate.Target::poll_closedusespoll_cancel, so the same gate applies. This was a few lines and self-contained.moq-tokiotransport: the wrapped send stream'spoll_closednow watches an idle, unfinished stream and is interruptible by later writes, so STOP_SENDING is observed while idle. This hunk is a near-verbatim copy from fix(net): keep IETF requests alive after FIN #4658. I did not plan to include it, but it is required: release's own IETF subscriber unsubscribes on d17+ with FIN plus STOP_SENDING (cancel_subscribe). Once a FIN stops cancelling on d19+, the STOP_SENDING is the only signal, and without this hunkmoq-tokio::broadcast broadcast_rejoin_*fail onmoq-transport-22with "upstream never canceled".doc/concept/standard.md: one paragraph on the FIN and REQUEST_UPDATE behavior. It says "Rust publisher" because the JS side is not ported.REQUEST_UPDATE choice
Release can apply a priority change (
track.updatealready exists), so I ported main's behavior instead of refusing every update. A priority-only update is applied and answered with REQUEST_OK. Any other parameter (FORWARD=0, filters, fill, timeouts, token, range filters, NEW_GROUP_REQUEST) is refused with REQUEST_ERRORNOT_SUPPORTED, and the subscription ends with PUBLISH_DONEUPDATE_FAILED(0x8). Nothing is dropped silently, and release behaves the same as main. A malformed frame ends the subscription withINTERNAL_ERROR.Left out
run_fetch_stream,joined) still cancel on a d19+ FIN. The scope here is the subscription path. It is the same 3-linepoll_cancelswap if interop needs it.subscriber.rs, where a peer's FIN keeps the route, plus our withdrawal aborting both directions on d19+). This depends on main-only withdrawal and active-count work (feat(net): MoQ Active Count for IETF namespace subscriptions #4268).js/net, where the JS publisher still cancels on FIN.Public API and wire impact
pub(super)orpub(crate).PublishDoneStatus::UpdateFailedis a new variant on apub(crate)enum.Tests
New tests in
ietf::publisher::serve_tests, using mocked time (start_paused) and no sleeps. All of them drive the realrun_subscribe_stream/run_subscribe_namespace_stream:requester_fin_is_version_gated_for_subscriptions: d14-d22. FIN cancels on d14-18. On d19+ the subscription stays alive and a group written after the FIN reaches the data stream.requester_reset_cancels_subscriptions: a reset still cancels on every draft.subscription_update_applies_priority_without_ending_the_request: d19 and d22. Priority 10 is applied (model 245) and both updates are acked.unsupported_subscription_update_fails_the_subscription: d17, d19, and d22. FORWARD=0 produces NOT_SUPPORTED and then UPDATE_FAILED.namespace_requester_fin_is_version_gated: d14-d22.Before the fix, all four original tests failed: d19 ended on FIN, the update ended the subscription, and the unsupported update finished
Okwith "track ended". After the fix:cargo nextest run -p moq-net: 1518 passed.cargo nextest run -p moq-tokio: 431 passed, including thebroadcast_rejoin_*tests that caught the STOP_SENDING gap.just checkagainstorigin/release: exit 0. Under load, one run timed out onmoq-uring::teardown a_published_close_has_already_left_the_client. That test is raw io_uring QUIC with no moq-net in it, and it passed on retry and alone.(Written by Claude Opus 5.5)
🤖 Generated with Claude Code