Skip to content

fix(net): a stalled subscriber holds one parked group per subscription - #6140

Merged
kixelated merged 6 commits into
mainfrom
quest/m1/egress-held-state
Oct 21, 2026
Merged

kixelated merged 6 commits into
mainfrom
quest/m1/egress-held-state

Conversation

@kixelated

@kixelated kixelated commented Oct 21, 2026 •

Copy link
Copy Markdown
Collaborator

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 Open for 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's TrackServe behaved 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_abandoned reports 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:

    • The Open state on both protocols. On lite-05+ the group is named with SUBSCRIBE_DROP, through the existing failed-group path.
    • A served stream parked on the transport. It now resets as soon as the cache evicts the part it has not read, instead of holding its buffered bytes until the peer grants flow control. A stream that has already read its whole group keeps draining.

    poll_abandoned waits 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 (default None, 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_credit in rs/moq-net/benches/session.rs: one viewer behind a relay gets one group stream at a time, swept over subscriptions (watch 1, 16, 64) by backlog per subscription (burst 1, 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-bench runs --bench '*').

  • The drill ietf_subscriber_max_age_skips_bufferbloat now also runs on moq-lite-06 as lite_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

  1. The peer never grants credit. The publisher holds one machine per subscription, however many groups are published. A parked group the pool evicts leaves at once.
  2. An inert pool. The same bound: one machine per subscription, plus whatever the pool itself keeps by policy.
  3. The peer grants stream credit but withholds byte credit. Streams are bounded by the stream credit granted. A stream stalled on flow control resets when its budget convicts it, or when the cache evicts the part it has not read (by default after the pool's 30 s idle window).
  4. The peer asks for an enormous max delay. The track cursor already clamps the budget with clamp_max_delay(.., max_age_bound()) to the track's max_age and the origin's cache_duration. Eviction now releases parked or stalled machines past that.
  5. A real viewer behind a congested link. It keeps the live edge: see the drill above and 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 (watch x burst groups, each one stream grant), on a shared dev machine, so treat single cells as noisy:

version watch x burst main this PR
moq-lite-06 16 x 64 294 ms 83 ms
moq-lite-06 64 x 16 642 ms 164 ms
moq-lite-06 64 x 64 8.16 s 538 ms
moq-lite-07-wip 64 x 64 10.8 s 495 ms
moq-transport-22 16 x 64 520 ms 54 ms
moq-transport-22 64 x 64 11.4 s 574 ms

Per grant at 64 subscriptions, main grows 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_subscription sweeps 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_once checks 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.
  • Integration 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-relay drills: lite_subscriber_max_age_skips_bufferbloat, next to the existing IETF one.

Public API and wire impact

  • Wire: none.
  • API: one additive field, moq_net::transport::memory::Config::max_concurrent_uni_streams: Option<usize>. Config is #[non_exhaustive], so this is not breaking and needs no MIGRATE.md entry. Everything else is crate-private (poll_abandoned is pub(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:

  • Both lite and IETF open each group's stream with waitUntilAvailable: false. A group that finds no stream slot is dropped at once, and lite-05+ names it with SUBSCRIBE_DROP.
  • Gap: a WebTransport implementation that parks an over-limit open instead of rejecting it leaves one pending open per group. Only the 10 s OPEN_TIMEOUT_MS or the subscriber leaving bounds it, and an expired or evicted group does not cancel it.

Decisions (settled by the maintainer, 2026-10-20)

  1. Relay default cache_duration.
    • ✅ Keep it unlimited (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, and max_age_bound already clamps the budget to max_age and cache_duration.
    • Clamp the serving budget to the pool's idle window, or lower the default.
  2. The per-grant benchmark.
    • ✅ Build it in this PR: a stream-credit limit in the in-memory transport, and a session benchmark swept over subscriptions x backlog that times each stream grant, run nightly.
    • Leave it as a follow-up.
  3. Who gets the next stream across subscriptions. The transport's wake order decides (moq-quic's stream_budget_available Notify), not priority, as before this PR.

Follow-ups

  • JS: have a parked open give up on group expiry or eviction (mirroring poll_abandoned), or confirm the fail-fast shape is the target and remove the timeout path.
  • A priority-ordered stream open in moq-quic, under Scope track priority; the benchmark's slope over subscriptions is its baseline.
  • Some existing 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

kixelated and others added 6 commits October 20, 2026 18:37
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>
@kixelated
kixelated marked this pull request as ready for review October 21, 2026 04:26
@coderabbitai

coderabbitai Bot commented Oct 21, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

You'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.

Check out review usage here.

View limit details

Limit details: You’ve used all 4 included reviews currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration
  • Configuration used: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 40bcd4e5-614c-49f4-9232-550e173bad0a
📥 Commits

Reviewing files that changed from the base of the PR and between 6f65f29 and 83dd8c3.

📒 Files selected for processing: 11
  • quest/m1/README.md
  • quest/m1/egress-held-state.md
  • rs/moq-net/benches/session.rs
  • rs/moq-net/src/ietf/publisher.rs
  • rs/moq-net/src/lite/publisher.rs
  • rs/moq-net/src/model/group.rs
  • rs/moq-net/src/transport/memory.rs
  • rs/moq-net/tests/integration/main.rs
  • rs/moq-net/tests/integration/stream_credit.rs
  • rs/moq-relay/tests/drills.rs
  • test/drill/README.md
  • 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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 21, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-21T04:29:54.502800Z 83dd8c3 Draft marked ready
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@kixelated

Copy link
Copy Markdown
Collaborator Author

Ready to merge at 83dd8c3b5b632d8e8b836a4c6d18d62950b6f009.

  • Each subscription parks at most one group on stream credit (lite and IETF); an abandoned parked group, or a stalled stream whose unread rest was evicted, leaves at once.
  • The in-memory transport gains max_concurrent_uni_streams, and session_delivery_credit times stream grants over subscriptions x backlog (nightly via --bench '*'). At 64 x 64 a round drops from about 8 to 11 s on main to about 0.5 s; per-grant cost is now flat over the backlog.
  • Settled: the relay keeps an unlimited cache_duration; the benchmark landed here; cross-subscription open order stays with moq-dev/moq#6142.
  • Local just check passed; redundant_importers_switch_through_bufferbloat_on_lite_07 flaked once, and also flakes on main (tracked by quest/m1/test-flakes-2/redundant-failover-drift.md). Codex reviewed this head with no findings; CodeRabbit was rate-limited.

(Written by Claude Opus 5.5)

@kixelated
kixelated added this pull request to the merge queue Oct 21, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Oct 21, 2026
@kixelated
kixelated added this pull request to the merge queue Oct 21, 2026
Merged via the queue into main with commit 4927167 Oct 21, 2026
10 checks passed
@kixelated
kixelated deleted the quest/m1/egress-held-state branch October 21, 2026 16:54
kixelated added a commit that referenced this pull request Oct 21, 2026
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>
kixelated added a commit that referenced this pull request Oct 21, 2026
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant