Repository navigation
Conversation
Quest-Claim: f8d38490ac20e8a0
Delete transport::StreamCredit, transport::Dir, and the poll::Session::stream_credit hook, with every implementation and moq-quic's Streams::credit/blocked accessors that only fed it. The open line now treats any pending head as a wait for the peer's credit. To keep that true, moq-tokio's and moq-uring's WebTransport opens return once QUIC grants the stream; the send stream writes the session header ahead of its first data instead of the open waiting on it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…the session Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Resetting it and reporting a clean close hid that the peer never saw the stream. finish, poll_closed, and shutdown now fail with Error::Unsent. 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 1 minute. 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. |
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed d16adc7. No actionable findings. Transport opens now return after stream credit is granted, with WebTransport headers written ahead of payload across both runtime adapters and Tokio AsyncWrite. Pending headers report unacknowledged, and finishing before header credit is available retains an Unsent failure rather than reporting a clean unseen stream. The removed credit hook and open-line inference match the chosen transport contract.
Verification: GitHub source/diff review of hook removal, open/preemption logic, header/write/reset/finish paths, and credit-exhaustion regressions. No builds, tests, browser interoperability, or benchmarks run.
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
ℹ️ Minor suggestions only — the hook removal, the open-line inference, and the lazy WebTransport header all hold up under tracing.
Reviewed changes
Reviewed the full 19-file diff at d16adc7, including both runtime adapters' new header state machine and every in-tree poll::Session implementor.
- Stream-credit hook removed —
moq_net::transport::{StreamCredit, Dir}andpoll::Session::stream_creditare gone, with the boxed forwarding, memory, moq-tokio, moq-uring, and qmux implementations;moq-quic::Streams::{credit, blocked}go too, whileopen_refusedstill drives STREAMS_BLOCKED. - Open line infers the wait —
make_roomno longer consults the transport andLinesdrops itsWeak, so a pending head of the line is a credit wait and every pending uni open may preempt a lower-ranked group stream. - WebTransport header written lazily — both
web::send::Streamtypes holdunwritten/header/unsent, write the header at open when it fits, and otherwise ahead of the first data; theOpeningbridges and thei64::MAXreprioritize step are gone. Error::Unsent— finishing a stream whose header never fit resets it and failsfinish,poll_closed, and tokio'sshutdown, instead of reporting a clean close the peer never saw.- Tests —
an_open_waits_only_for_stream_creditin both runtimes, and the uring reset-before-header test rewritten for the new contract. - Quest — the child quest is deleted and the epic's decision line records the replacement contract, with
MIGRATE.md:151left to the release cut asAGENTS.mdrequires.
ℹ️ The "an open pends only for credit" contract is missing from the trait it constrains
The open line now resets a lower-ranked group stream on nothing but a pending open, so a transport that pends for any other reason — the WebTransport header was the one in-tree case — makes the session preempt healthy streams. poll::Session::poll_open_uni and poll_open_bi (rs/moq-net/src/transport.rs:286, :294) still say only that they block "while there are too many concurrent streams"; the new requirement is stated in the PR's Impact but not where an implementer will read it.
Technical details
# State the pending-open invariant on the trait
## Affected sites
- `rs/moq-net/src/transport.rs:286` — `poll_open_uni` doc: "blocks while there are too
many concurrent streams"; says nothing about other reasons to pend.
- `rs/moq-net/src/transport.rs:294` — the same for `poll_open_bi`.
- `rs/moq-net/src/transport/open.rs:214` — `make_room`, which now trusts the invariant
unconditionally (no `StreamCredit::blocked` guard remains).
## Required outcome
- A transport implementer reading only the trait docs learns that a `Pending` from these
two methods means the peer's stream credit, and that any per-stream preamble (a
WebTransport header, framing, a handshake) must be written as the stream's first data
instead of blocking the open.
- This is the only thing standing between a third-party adapter and spurious resets of
healthy group streams, since `make_room` can no longer ask.
(Written by DeepSeek V4.1 Flash 1017)
deepseek-v4.1-flash-1017 (free via Pullfrog for OSS) ｜ 𝕏
| if self.poll_header(&kio::Waiter::noop()).is_pending() { | ||
| self.reset_mut(0); | ||
| self.unsent = true; | ||
| return Err(Error::Unsent); | ||
| } |
There was a problem hiding this comment.
poll_header can return Ready(Err(..)) — a peer STOP_SENDING, a session close, or ClosedStream — and is_pending() treats that as a written header, so finish still reports the clean close this PR set out to stop reporting. Match the result and return the header's error; the same shape is at rs/moq-uring/src/quic/web/send.rs:205.
| if self.poll_header(&kio::Waiter::noop()).is_pending() { | |
| self.reset_mut(0); | |
| self.unsent = true; | |
| return Err(Error::Unsent); | |
| } | |
| match self.poll_header(&kio::Waiter::noop()) { | |
| Poll::Ready(Ok(())) => {} | |
| Poll::Pending => { | |
| self.reset_mut(0); | |
| self.unsent = true; | |
| return Err(Error::Unsent); | |
| } | |
| Poll::Ready(Err(err)) => return Err(err), | |
| } |
Finishes quest/m1/stream-credit/no-credit-hook.md and deletes it.
Summary
moq_net::transport::StreamCredit,transport::Dir, andpoll::Session::stream_creditare gone, along with every implementation: moq-tokio, moq-uring (QUIC andtransport::local), qmux, and the memory transport.Dirlives on privately intransport/open.rs, where it picks a line.Streams::creditandStreams::blockedwent too. moq-dev/moq#7894 added them only to feed the hook. Their README entry and test assertions are removed.rs/moq-net/src/transport/open.rs) treats a pending head as a wait for the peer's credit, the same wayjs/netdoes.make_roomno longer asks the transport first, andLinesno longer holds a weak reference to the transport. The memory transport'sblockedflag, which only fed the hook, is removed.web::Session::poll_open_uni/poll_open_bireturn as soon as QUIC grants the stream. The send stream holds the header it still has to write and writes it before any data:write,write_all,poll_write_buf, and tokio'sAsyncWrite. When the header fits,newwrites it right away, so the peer can accept the stream before its first data, same as before. Both web sessions drop theirOpeningbridge, which existed only to hold a half-written header across polls.Choices made without asking
reset_atas before (a_reset_stream_keeps_its_headeris unchanged and passes).finishon a stream whose header still cannot be queued resets it and fails loudly with the newquic::web::send::Error::Unsent, fromfinish,poll_closed, and tokio'sshutdown. It no longer reports a clean close the peer never saw. Holding the FIN until the header fits was rejected because it needs the same owed-header driver state. The olda_reset_stream_keeps_its_header_without_creditnow asserts the new contract: the reset stream never reaches the session, and the next stream arrives intact. Rejected alternative: keep the header-on-reset guarantee by holding the reset until the header fits. That needs owed-header state polled by a driver task in both runtimes, which is the state feat(quic)!: reliable reset from a send-stream checkpoint #5690 removed, and it buys nothing for a stream that carried no data.acknowledged()reportsSome(false), so takeover never picks a stream that still owes bytes.send::Optionsstraight to QUIC. Before, it opened at priorityi64::MAXand reprioritized after the header. The header now goes out at the stream's own priority, ahead of its data.Tests
an_open_waits_only_for_stream_creditin moq-tokio (raw QUIC and WebTransport) and moq-uring (uring server to tokio client). Once the peer's connection window is spent, a secondpoll_open_unireturnsReadyright away, and both streams arrive intact once the peer reads. Against the old open path, the tokio test fails at thepoll_open_uniassertion.just check: 12788 tests passed, 149 skipped.Performance
The open line's hot path is unchanged: an open that gets credit right away never reaches
make_roomorLine::start. A waiting open now skips aWeak::upgradeand a transport call. A WebTransport open now takes no bridge lock, and in the common case it writes the header once at open. No benchmark was run because no fan-out path changed.Impact
Breaking for transport implementers (
feat(net)!). Nothing on the wire changes.moq_net::transport::{StreamCredit, Dir}andpoll::Session::stream_creditare removed. Delete yourstream_creditimpl. Nothing replaces it: the session treats any pending open as waiting for the peer's credit, sopoll_open_uni/poll_open_bimust pend only for stream credit. If you write a stream header, write it as the stream's first data instead of waiting on it inside the open.moq_quic::Streams::{credit, blocked}are removed. They had no consumer besides the hook.quic::web::Session::poll_open_*no longer wait to write the WebTransport header. A stream reset before its header fits arrives at the peer as a plain reset of a stream it cannot place, which it drops. Finishing one fails with the newquic::web::send::Error::Unsentvariant.origin/releasehas nostream_credit). The release cut should drop thepoll::Session::stream_creditnote atrs/moq-net/MIGRATE.mdline 151, which this PR leaves alone per the MIGRATE rule.Suggested follow-ups
None.
Models
🤖 Generated with Claude Code
(Written by Claude Opus 5.5 with medium)
Review
Rewritten by
quest reviewafter each round.01a1ba33-2a6a-7b30-9ece-356cae838ee8, round 2The incremental changes fix silent completion failures in both runtimes, with no additional findings from source review.rs/moq-tokio/src/quic/web/send.rs:207:Finish pending WebTransport headers instead of resetting them;Both adapters now return and retain Error::Unsent when finishing with a blocked header (rs/moq-tokio/src/quic/web/send.rs:161,215 and rs/moq-uring/src/quic/web/send.rs:124,204), and Tokio shutdown propagates that error at rs/moq-tokio/src/quic/web/send.rs:286.(Written by Claude Opus 5.5 with medium)