Repository navigation
fix(net): a stalled subscriber holds one parked group per subscription - #6140
Conversation
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>
The in-memory transport gains `max_concurrent_uni_streams`, QUIC's uni stream credit: an open pends until the peer reads one of its streams to the end, stops, or drops it. `session_delivery_credit` grants a viewer one group stream at a time, swept over its subscriptions and backlog, so a publisher that parks state per backlogged group shows a slope. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
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 56 seconds. 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. |
|
Ready to merge at
(Written by Claude Opus 5.5) |
GroupServe::settle_failed (from #6140) now takes the failure, so a group that never got a stream is dropped with its own error, as a failed serve is. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Group.Consumer.expired settles with the Old error once the subscription's budget gives up on a group, even after every frame was read, mirroring the Rust poll_expired. It watches the budget only while raced, so a race per write leaks nothing. The lite and IETF publishers race their writes against it, and hooks.guardGroup, hooks.readGroupFrame, and ReadGroupFrame go away. A read no longer holds the consumer's closed open while the frame is written: expired already judges a group read whole, and holding closed on every read would hang a reader that stops at the last frame without a trailing read. serve.md keeps the one serving item left, the crate-private Rust poll_abandoned that #6140 added. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Quest:
quest/m1/egress-held-state.md(deleted here, since this PR completes it).Problem
A moq-net publisher (a relay, or anything serving a subscriber) built a serve machine for every group a subscription reached, as soon as the group arrived. Each machine then waited in
Openfor stream credit, with no limit on how many could wait. A subscriber that withheld uni-stream credit therefore made the publisher hold one machine per group published, for as long as it kept withholding. The only exit was group expiry, and an aborted group (evicted, cut, or failed upstream) never expires. So once the cache's idle window evicted a parked group's bytes, its machine stayed parked forever. When credit finally arrived, it was spent on a group that could only be reset. moq-transport'sTrackServebehaved the same way.Every grant also woke every parked machine, so each stream credit cost the whole backlog: see the benchmark below.
Change
The credit decides what the publisher holds, not the arrival rate. On both lite (
TrackRun) and IETF (TrackServe), each subscription has at most one group waiting for stream credit (opening). The cursor hands out no further group until that one gets a stream or gives up. The backlog stays in the cache, which already bills, ages and evicts it, and the cursor's drift budget skips any group that went stale meanwhile, reporting it through its existing drop path. Groups that have a stream run concurrently as before, so they are bounded by the stream credit the peer grants.A parked group that can no longer be served leaves at once. The new
group::Consumer::poll_abandonedreports two cases: the drift budget convicted the group (Old), or the group was aborted before the cursor finished reading it and no front route will continue it. It is used in two places:Openstate on both protocols. On lite-05+ the group is named with SUBSCRIBE_DROP, through the existing failed-group path.poll_abandonedwaits only on the group's close (an abort closes it), so a stalled subscriber does not wake on every frame written to its parked group.The in-memory transport has stream credit.
transport::memory::Config::max_concurrent_uni_streams(defaultNone, unlimited) bounds the uni streams each side has open toward its peer, as QUIC's MAX_STREAMS does. A stream holds its credit until its reader reaches the end, stops, or drops it, accepted or not; an open past the limit pends and fails once the session closes.New benchmark
session_delivery_creditinrs/moq-net/benches/session.rs: one viewer behind a relay gets one group stream at a time, swept over subscriptions (watch1, 16, 64) by backlog per subscription (burst1, 16, 64), on lite-06, lite-07 and moq-transport-22. Every group waits for a stream grant, so a cost that grows with what the publisher parks shows as a slope over the backlog. It runs nightly with the other moq-net bench targets (just gh nightly rust-benchruns--bench '*').The drill
ietf_subscriber_max_age_skips_bufferbloatnow also runs on moq-lite-06 aslite_subscriber_max_age_skips_bufferbloat. This is the benign case: a viewer on a 64 kb/s link behind a 500 ms finite queue still receives the newest group.The quest's scenarios after this change
clamp_max_delay(.., max_age_bound())to the track'smax_ageand the origin'scache_duration. Eviction now releases parked or stalled machines past that.credit_after_a_stall_serves_the_live_edge.Benchmark
cargo bench -p moq-net --bench session -- session_delivery_credit --warm-up-time 1 --measurement-time 2 --sample-size 10, median time per round (watchxburstgroups, each one stream grant), on a shared dev machine, so treat single cells as noisy:mainPer grant at 64 subscriptions,
maingrows from about 0.14 ms (backlog 1) to 2 to 2.8 ms (backlog 64): every grant wakes every parked group. With this PR it stays at about 0.13 to 0.16 ms across the backlog. What remains is a slope over subscriptions (about 0.03 to 0.06 ms per grant at 1 subscription): each grant still wakes every subscription's one parked opener, because the transport's open wait wakes all waiters. That is the cross-subscription open queue left to Scope track priority.Tests
New tests, all on mock time. The publisher ones fail when the old behavior is put back (a machine per group, and no abort check):
lite::publisher::serve_group_test:withheld_credit_holds_one_group_machine_per_subscriptionsweeps tracks {1, 16} by groups per track {1, 64}. It asserts that held machines equal the number of tracks, then that every group is served once credit is granted.an_evicted_group_waiting_for_credit_leaves_at_oncechecks the drop is named, the next group takes the slot, and the credit goes to that group.credit_after_a_stall_serves_the_live_edge.blocked_transport_write_resets_once_the_group_is_evicted.ietf::publisher::group_priority_test:withheld_credit_holds_one_group_machine_per_subscription, the same sweep.an_evicted_group_waiting_for_credit_leaves_at_once.transport::memory::tests::uni_credit_pends_opens_until_a_stream_is_done: credit is per direction, held by unaccepted streams, returned once on read-to-end or drop, and a parked open fails on session close.stream_credit::one_stream_credit_serves_every_backlogged_group: a real session pair at one group stream of credit delivers every backlogged group on lite-06, lite-07 and moq-transport-22.moq-relaydrills:lite_subscriber_max_age_skips_bufferbloat, next to the existing IETF one.Public API and wire impact
moq_net::transport::memory::Config::max_concurrent_uni_streams: Option<usize>.Configis#[non_exhaustive], so this is not breaking and needs noMIGRATE.mdentry. Everything else is crate-private (poll_abandonedispub(crate)). The relay's defaults are unchanged.JS (
js/net)The JS publishers have a different shape and are bounded already, so this PR does not change them:
waitUntilAvailable: false. A group that finds no stream slot is dropped at once, and lite-05+ names it with SUBSCRIBE_DROP.OPEN_TIMEOUT_MSor the subscriber leaving bounds it, and an expired or evicted group does not cancel it.Decisions (settled by the maintainer, 2026-10-20)
cache_duration.Duration::MAX), with no new clamp from the pool's idle window. Eviction now releases parked and stalled machines, so the idle window already bounds them, andmax_age_boundalready clamps the budget tomax_ageandcache_duration.stream_budget_availableNotify), not priority, as before this PR.Follow-ups
poll_abandoned), or confirm the fail-fast shape is the target and remove the timeout path.session_delivery_*ids (e.g.burst) pass Criterion's 100-character title limit, which truncates and renumbers them (#2,#3) in reports. The new sweep uses a short id; the others would need a rename that resets their history.(Written by Claude Opus 5.5)
🤖 Generated with Claude Code