You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
fix(quic): a paused stream holds only its stream window - #8794
A bulk transfer through a relay over QUIC or WebTransport could end Evicted partway, even with a writer awaiting group::Producer::ready and a single reader that keeps reading (found validating moq tcp in #8741, since removed).
moq-net's ingest paces a group by its fastest reader: when the reader falls behind, ingest stops reading the stream, so flow control pushes back to the publisher. It charges a paused stream its stream window (1 MiB by default) against half the connection window (4 MiB). moq-quic broke that assumption. It granted MAX_STREAM_DATA from the bytes a lease moved out of the receive buffer, not the bytes the reader took, and every lease moves everything that has arrived. So a stream that ingest stopped reading kept granting credit until it held the whole connection window. Then the stuck-window pass evicted it. The trace: the paced stream held 3.9 MB of lease plus 0.3 MB unread, against a 4 MiB connection window.
Approach
In moq-quic, stream credit now starts from what the reader took: assembler.bytes_read() minus the leased bytes it has not taken yet (Leased::untaken, which includes bytes out in a Lease). Handing a lease back queues MAX_STREAM_DATA for what was taken. A reader that stops taking now holds its stream window and no more, which is what ReceiveWindow and moq-tokio's DEFAULT_STREAM_RECEIVE_WINDOW docs already promised.
A reader waiting for a unit larger than the stream window (lease(want)) gets credit for the whole unit, which the connection window still bounds. The credit is sent even when the increase is below the usual eighth-of-a-window threshold, because nothing the reader takes while it waits would add to it. Without that, a reader waiting on a unit a few bytes past its credit deadlocks.
moq-net's charge also misses one more hold. poll_read_frame keeps the credit of the bytes it took into a frame's unsealed segment (up to 1 MiB) until the segment seals, and a stream paused mid-segment keeps that credit beside its stream window. coding::Reader now counts those bytes, and Pause::pause(taken) reserves stream window + taken (Pullfrog's finding).
The repro is a moq-relay sim test over simulated QUIC. A moq-lite-07-wip socket session sends 40 MB in 8 KiB frames, with the writer awaiting ready, and the reader drains at about 64 MB/s. Before the fix it ends Evicted after 320 KiB.
Impact
No public API or wire change. Behavior change in moq-quic (and so in moq-tokio, moq-uring, WebTransport and qmux on top of it): a receive stream's credit tracks what its reader takes, not what it leases.
Behavior change in moq-net: a stream paused partway through a segment reserves that segment's taken bytes too, so fewer such streams pause before the cap and the rest read on.
Validation
quic_sim::a_socket_session_carries_a_bulk_transfer_losslessly: fails on main with after 327680 bytes: evicted, and passes with the fix (2.5 s). I also ran it locally with 5% loss, duplication and reordering on both client paths, and it passed (not committed, since it takes 18 s).
I instrumented StreamsState::evict on main to find the cause: wmd=5555014 recvd=5555014 held=3895495 rw=4194304 unread=298809, all on the paced stream.
moq-quic tests: leases_hold_one_stream_window replaces lease_spends_connection_window_only, which asserted the old behavior. It checks that leasing without taking stops at one stream window, and that taking returns stream credit but keeps connection credit until release. a_waiting_reader_gets_room_for_its_unit fails without the below-threshold rule.
Segment hold: a sim case with 4 MiB frames (2 to 4 sessions on one connection) paused streams holding up to 1048575 unreleased bytes while charged only the stream window. None of those runs evicted at the default windows, so the regression test is coding::reader::a_pause_mid_segment_reserves_what_it_took, plus a transport::boxed case for the larger charge.
just check passes: 12849 tests on the current head (8061 before the segment fix). One earlier run hit an unrelated flake: moq-rtmp server::tests::play_multitrack_to_capable_client fired a host-clock timer under load. It passes alone.
Not run on the real binary: no CLI that opens socket sessions is left since moq tcp was removed. The sim test runs moq-relay's real Connection over real moq-quic.
Alternatives
Charge a paused stream the whole connection credit it might hold, in moq-net. That would allow at most one paused stream, and moq-net can't see what the transport buffered.
Have ingest keep reading while paused, buffering outside the group. That moves the unbounded buffer instead of bounding it.
Follow-ups
Every stream partway through a large frame holds up to a segment of connection credit, paused or not. N concurrent large-frame streams therefore need N segments of connection window: a 1.5 MiB window evicted two of them in the sweep. That concurrency budget is kixel.ai's paused-credit quest, alongside the paused-credit cap.
More concurrent paused streams than the paused-credit cap (half the connection window, so two at moq-tokio's defaults) still read on and shed, per the decision in feat(net): pace relay ingest by the group's fastest reader #7388. kixel.ai plans this as a quest for its byte streams, since live media expects a slow reader to fall off.
Size: M
Implementer: Claude Code, claude-opus-5-5, unknown effort (arm A)
Implementer: Claude Code, claude-opus-5-5, unknown effort (iterate, M, arm A)
mechanical fix by codex, gpt-6.1-sol, medium effort: Merged base and mechanically migrated new tests to its group/start and timestamp APIs; ran nix develop --command just ci base (12832 tests passed) and git diff --check base. (head 2d68e88)
Score: 9/10, Receive-credit accounting appears consistent across leasing and pacing, with no actionable defects found; builds and tests were not run, as instructed.
Rounds: 1; first round 0; findings 0: fixed 0, refuted 0, open 0
Findings: none
Reviewer: Codex, gpt-6.1-sol, medium effort, shadow
Stream credit ran from the bytes a lease moved out of the assembler, not the
bytes its reader took, so a reader that leased and then stopped taking kept
granting the peer more, up to the connection window. moq-net's ingest pauses
a stream exactly that way to pace a slow reader, assuming it holds at most its
stream window, so a paced stream filled the connection window and the stuck
window evicted it: a bulk transfer through a relay ended Evicted.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The reason will be displayed to describe this comment to others. Learn more.
ℹ️ No critical issues — one gap worth a follow-up inline.
Reviewed changes
I reviewed the whole PR: moq-quic now grants stream credit from the bytes a reader takes, not the bytes it leases, and adds tests at the moq-quic and relay-sim levels.
Stream credit from taken bytes: Recv::max_stream_data now starts from bytes_read - Leased::untaken(), and the new lent field counts bytes that are out in a Lease, so a stream that ingest stops reading holds back its stream credit.
Room for a waiting unit: a reader blocked on lease(want) gets max(stream_window, want), and the short rule sends that credit even below the eighth-of-a-window threshold, which fixes the deadlock a few bytes past the limit.
Credit on hand-back: put_back (restore/release) and the Blocked branch of lease_inner now queue MAX_STREAM_DATA through the new queue_max_stream_data.
Tests: leases_hold_one_stream_window replaces the test that asserted the old behavior, a_waiting_reader_gets_room_for_its_unit covers the below-threshold rule, and a_socket_session_carries_a_bulk_transfer_losslessly reproduces the eviction through a relay over simulated QUIC.
I checked these and found no problem:
assembler.clear() keeps bytes_read, so the subtraction can't underflow after a stop or eviction.
An arrived() that clears want before the queued frame is written fixes itself when the reader leases again.
Plain Chunks reads have untaken() == wanted() == 0, so their behavior is unchanged.
No actionable comments were generated in the recent review.
Walkthrough
Receive-stream lease accounting now tracks bytes held in outstanding leases. Stream-credit calculations use bytes taken and the waiting reader’s requested lease size. Lease operations report when updated credit requires transmission. Unit tests cover lease consumption and requests larger than the stream window. A QUIC simulator test transfers framed data through a relay and checks for complete, uncorrupted frames.
Merge Risk: Minimal · up to 92946
This change makes a paused QUIC stream hold only its own stream window instead of drawing credit up to the connection window. It also gives a reader waiting on a large message enough credit for it. The review found no concrete defect, and the new tests cover the changed behavior. The only leftover item is some doc comments that now understate the credit a stream may hold. The change looks safe to merge.
Pre-merge checks: 4
Check name
Status
Explanation
Title check
The title names the main change: a paused stream's receive credit is limited to its stream window in moq-quic. The changeset (lease.rs, recv.rs, moq-quic tests, and docs) implements stream credit trac…
Linked Issues check
Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check
Check skipped because no linked issues were found for this pull request.
Description check
The description is specific and relevant to the changeset. It explains the problem (a receive stream's credit follows leased bytes instead of bytes the reader takes, so a paused stream can hold up to …
Full details: Title check
Explanation
The title names the main change: a paused stream's receive credit is limited to its stream window in moq-quic. The changeset (lease.rs, recv.rs, moq-quic tests, and docs) implements stream credit tracking bytes taken rather than bytes leased, which matches the title's subject and intent. The title uses the fix(quic) convention and is specific, under 60 characters, and free of noise.
Full details: Description check
Explanation
The description is specific and relevant to the changeset. It explains the problem (a receive stream's credit follows leased bytes instead of bytes the reader takes, so a paused stream can hold up to the connection window and get evicted). It describes the approach in moq-quic (credit from bytes taken, untaken accounting, queuing MAX_STREAM_DATA on lease handback, the below-threshold credit rule for waiting readers) and the moq-net pause charge for mid-segment bytes. It also lists the tests that cover these changes, the impact on moq-tokio, moq-uring, WebTransport and qmux, and the alternatives considered. The raw summary's file changes (lease.rs, recv.rs, state.rs, tests, README, and the moq-tokio docs) match these claims.
Finishing Touches
Simplify code
Commit to this branch
Create a new PR
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.
File: No configuration file from this repository applied
Commit: Unavailable
Layers: Organization UI
Review profile: CHILL
Plan: Advanced
Run ID: 38e8ce68-0d23-4111-8ebc-06fed93f94a4
Commits
Reviewing files that changed from the base of the PR and between 98ed0f8 and 9294662.
Files selected for processing: 8
rs/moq-quic/README.md
rs/moq-quic/src/connection/streams/lease.rs
rs/moq-quic/src/connection/streams/recv.rs
rs/moq-quic/src/connection/streams/state.rs
rs/moq-quic/src/tests/mod.rs
rs/moq-relay/tests/integration/quic_sim.rs
rs/moq-tokio/src/quic/driver/recv.rs
rs/moq-tokio/src/quic/driver/tests.rs
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 2 remain after this review.
The reason will be displayed to describe this comment to others. Learn more.
✅ No new issues found. The undercount I flagged last round is fixed in moq-net, where ingest knows what it holds.
Reviewed changes
Since the prior review, the new commits charge a paused stream for the part of an unsealed segment it has taken, and the docs now describe what a stopped reader holds.
Unreleased segment bytes tracked: coding::Reader::unreleased counts the bytes that poll_read_frame restores rather than releases, and resets to 0 when the segment seals. Inlining the lease borrows only self.stream, so the counter can update while the lease is live.
Larger pause charge: Pause::pause(taken) flows through boxed::RecvStream::pause to Shared::reserve. That reserves stream window + taken (capped at the connection window) against half the connection window. The amount is fixed at the first pause, which is correct because unreleased only changes after resume().
Tests: a_pause_mid_segment_reserves_what_it_took checks that the reservation equals the held credit. paused_streams_hold_at_most_half_the_connection_window now also covers a 25-byte segment hold that refuses a second pause.
Doc corrections: the Leased, ReceiveWindow, moq-quic README, recv.rs, moq-tokio poll_lease and sim-test docs now say that a stopped reader holds at most its stream window of untaken bytes, plus what it took and has not released.
quest land watch: the merge queue removed this pull request at 3e5aff9 because its merge_group build failed (failed_checks).
The land fixer works on it in the background, and the watcher re-enqueues it at the back of the queue once it is fixed or judged flaky, or hands it back here with what it needs.
codex, gpt-6.1-sol, medium effort land fixer note:
The new tests used group and timestamp APIs replaced on base. Merged base and adapted the test initializers and bulk-transfer test using the same migration as base. Production behavior is unchanged. Validation: nix develop --command just ci base passed, including lint, formatting, documentation, dependency checks, and all 12,832 selected Rust tests. Protected paths match base.
quest land watch: armed auto-merge on this pull request at 2d68e88, so it joins the back of the merge queue once its required checks pass, with the land fixer's fix (codex, gpt-6.1-sol, medium effort): Merged base and mechanically migrated new tests to its group/start and timestamp APIs; ran nix develop --command just ci base (12832 tests passed) and git diff --check base..
The reason will be displayed to describe this comment to others. Learn more.
✅ No new issues found. The new commits only move the tests to main's updated APIs. The fix itself is unchanged.
Reviewed changes
Since the prior review, the branch merged main and moved two tests to its current model APIs.
Receive-credit test migrated: a_pause_mid_segment_reserves_what_it_took now uses group::Info { start: 0 } and the non-optional frame::Info::timestamp.
Bulk-transfer sim test migrated: a_socket_session_carries_a_bulk_transfer_losslessly now calls append_group(0) and writes with Timestamp::unscaled(0). It ends with group.close(), which ends the group without a known end and still delivers every frame written. These calls match main's definitions in model/track.rs, model/time.rs and model/group.rs.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
Problem
A bulk transfer through a relay over QUIC or WebTransport could end
Evictedpartway, even with a writer awaitinggroup::Producer::readyand a single reader that keeps reading (found validatingmoq tcpin #8741, since removed).moq-net's ingest paces a group by its fastest reader: when the reader falls behind, ingest stops reading the stream, so flow control pushes back to the publisher. It charges a paused stream its stream window (1 MiB by default) against half the connection window (4 MiB). moq-quic broke that assumption. It granted
MAX_STREAM_DATAfrom the bytes a lease moved out of the receive buffer, not the bytes the reader took, and every lease moves everything that has arrived. So a stream that ingest stopped reading kept granting credit until it held the whole connection window. Then the stuck-window pass evicted it. The trace: the paced stream held 3.9 MB of lease plus 0.3 MB unread, against a 4 MiB connection window.Approach
In moq-quic, stream credit now starts from what the reader took:
assembler.bytes_read()minus the leased bytes it has not taken yet (Leased::untaken, which includes bytes out in aLease). Handing a lease back queuesMAX_STREAM_DATAfor what was taken. A reader that stops taking now holds its stream window and no more, which is whatReceiveWindowand moq-tokio'sDEFAULT_STREAM_RECEIVE_WINDOWdocs already promised.A reader waiting for a unit larger than the stream window (
lease(want)) gets credit for the whole unit, which the connection window still bounds. The credit is sent even when the increase is below the usual eighth-of-a-window threshold, because nothing the reader takes while it waits would add to it. Without that, a reader waiting on a unit a few bytes past its credit deadlocks.moq-net's charge also misses one more hold.
poll_read_framekeeps the credit of the bytes it took into a frame's unsealed segment (up to 1 MiB) until the segment seals, and a stream paused mid-segment keeps that credit beside its stream window.coding::Readernow counts those bytes, andPause::pause(taken)reservesstream window + taken(Pullfrog's finding).The repro is a moq-relay sim test over simulated QUIC. A
moq-lite-07-wipsocket session sends 40 MB in 8 KiB frames, with the writer awaitingready, and the reader drains at about 64 MB/s. Before the fix it endsEvictedafter 320 KiB.Impact
Validation
quic_sim::a_socket_session_carries_a_bulk_transfer_losslessly: fails onmainwithafter 327680 bytes: evicted, and passes with the fix (2.5 s). I also ran it locally with 5% loss, duplication and reordering on both client paths, and it passed (not committed, since it takes 18 s).StreamsState::evictonmainto find the cause:wmd=5555014 recvd=5555014 held=3895495 rw=4194304 unread=298809, all on the paced stream.moq-quictests:leases_hold_one_stream_windowreplaceslease_spends_connection_window_only, which asserted the old behavior. It checks that leasing without taking stops at one stream window, and that taking returns stream credit but keeps connection credit until release.a_waiting_reader_gets_room_for_its_unitfails without the below-threshold rule.coding::reader::a_pause_mid_segment_reserves_what_it_took, plus atransport::boxedcase for the larger charge.just checkpasses: 12849 tests on the current head (8061 before the segment fix). One earlier run hit an unrelated flake:moq-rtmp server::tests::play_multitrack_to_capable_clientfired a host-clock timer under load. It passes alone.moq tcpwas removed. The sim test runs moq-relay's realConnectionover real moq-quic.Alternatives
Follow-ups
paused-creditquest, alongside the paused-credit cap.moq-rtmphost-timer flake above.Caused by
Models
Size: M
Implementer: Claude Code, claude-opus-5-5, unknown effort (arm A)
Implementer: Claude Code, claude-opus-5-5, unknown effort (iterate, M, arm A)
(Written by Opus 5.5)
🤖 Generated with Claude Code
Review
mechanical fix by codex, gpt-6.1-sol, medium effort:
Merged base and mechanically migrated new tests to its group/start and timestamp APIs; ran nix develop --command just ci base (12832 tests passed) and git diff --check base.(head 2d68e88)Rewritten by
quest reviewafter each round.Head: 3e5aff9
Base: origin/main at 77b8ec7
Risk: medium,
the base has no RISK.md to name what is always high or customer-facing, so every change is mediumClassified: medium by default
Reviewer: Codex, gpt-6.1-sol, high effort
Session:
01a1de3b-898d-71c2-b3b9-c2921f26ae89, round 1Score: 9/10,
Receive-credit accounting appears consistent across leasing and pacing, with no actionable defects found; builds and tests were not run, as instructed.Rounds: 1; first round 0; findings 0: fixed 0, refuted 0, open 0
Findings: none
Reviewer: Codex, gpt-6.1-sol, medium effort, shadow
Session:
01a1de3b-8988-7b01-ba53-d43247d8d25a, round 1Score: 9/10,
Source review found no actionable defects and supports landing, but builds and tests were not run as instructed.Rounds: 1; first round 0; findings 0: fixed 0, refuted 0, open 0
Findings: none