Skip to content

moq-quic owns its waits - #7490

Open
kixelated wants to merge 18 commits into
mainfrom
quest/m1/quic-api/waits
Open

kixelated wants to merge 18 commits into
mainfrom
quest/m1/quic-api/waits

Conversation

@kixelated

@kixelated kixelated commented Nov 1, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

Finishes quest/m1/quic-api/waits.md. moq-dev/moq#7391 gave moq-quic its own waiter lists and poll_* forms, but moq-tokio's quic::driver, moq-uring's driver, and qmux still kept their own per-stream and per-connection waiter tables, fed by StreamEvent and the connection's Events. Every handle also kicked its driver after mutating the connection.

Approach

  • Consumers wait on moq-quic. moq-tokio's quic::driver, moq-uring's driver, and qmux now wait only through moq-quic's poll forms. They no longer have:

    • the readable/writable/finishing and blocked_readers/blocked_writers maps;
    • the accept/open/datagram/establish lists and the Notifys;
    • the stopped map and the event sweeps.

    moq-tokio's futures hold a kio::Park. Its stopped() futures keep the stream's ID until the last of the stream and its futures drops, so the end that moq-quic keeps until release() still answers them.

  • New waits in moq-quic, for the last two event-shaped waits. Connection::poll_handshake_data replaces HandshakeDataReady, used by moq-tokio's Connecting::handshake_data. recv::Stream::poll_received_reset replaces the readable wake behind received_reset. Also new: Connection::error(), which says why the connection was lost.

  • Events deleted. StreamEvent is gone, and so are the Event variants a handle waited on (HandshakeDataReady, Connected, HandshakeConfirmed, ConnectionLost, Stream, DatagramReceived). QMux's Event and poll are gone too. StreamsState::notify's event-to-list mapping became direct wakes. Event keeps PathUpdated, which no handle waits on.

  • poll_transmit is the driver's wait. Both QUIC and QMux take the driver's waiter and return Poll. With nothing to send, it registers the waiter. These wake it:

    • a write, or the DATA_BLOCKED a blocked write queues;
    • a finish, a reset, or a stop;
    • a refused open (STREAMS_BLOCKED);
    • returned credit, from a read, a lease, a received reset, or set_limits;
    • a datagram, or a close.

    Pacing and loss recovery stay on poll_timeout. Every handle-side kick is deleted. Each driver keeps only its ingress wake, so a received datagram or record still re-arms timers.

  • qmux close while output is blocked. qmux's stalled writer also registers on poll_closed, so a local close still starts the close bound while output is blocked. a_close_behind_a_stalled_peer_is_bounded covers this on paused time. It fails (3.9 s, over the 3 s bound) without the registration.

  • Tests count wakes. moq-quic's Event tests now count wakes with a counting waiter. New tests cover the handshake-data, received-reset, and transmit waits.

  • Acked prefix kept after a reset. A reset that frees its stream keeps the acknowledged prefix alongside its end. poll_acked still answers for that prefix after the Acked event's offset is gone.

Impact

  • Public API (moq-quic, breaking):
    • StreamEvent is gone, and so are Event::{HandshakeDataReady, Connected, HandshakeConfirmed, ConnectionLost, Stream, DatagramReceived}, mux::Event, and mux::Connection::poll.
    • New: Connection::poll_handshake_data, Connection::error, and recv::Stream::poll_received_reset.
    • Connection::poll_transmit(waiter, now, max_datagrams, buf) -> Poll<Transmit> and mux::Connection::poll_transmit(waiter, now) -> Poll<Bytes>.
    • Migration is in rs/moq-quic/MIGRATE.md (a table from each event to its wait), and the README's fork list is updated.
  • Public API (moq-tokio, moq-uring, qmux): unchanged. moq-tokio's open_uni/open_bi/accept_uni/accept_bi became async fn, with the same signatures for callers.
  • Behavior: after a local close, moq-uring's and qmux's stream, accept, and open waits fail at once with the close's error. Before, they failed when the close frame went out. moq-tokio already failed them at once. qmux's poll_open_request after a local close returns the close's error rather than Ok(None) (no credit).
  • Wire: none.

Decisions

  • ✅ Local-close publication in moq-uring and qmux: (a) keep the latch (maintainer, 2026-11-01). moq-uring and qmux each keep their one local-close latch, closed with one WaiterList. They publish a close this side asked for to poll_closed only once the CONNECTION_CLOSE is staged (uring), or delivered with the transport shut (qmux). That is I/O behavior moq-quic can't see, and keeping it changes no behavior. The reason is now documented next to each latch (rs/moq-uring/src/quic/driver/connection.rs State::closed, rs/qmux/src/session.rs State::closed). The alternatives were for moq-quic's poll_closed to wait for the close packet to leave poll_transmit (a regression for qmux), or to resolve at close() and drop the guarantee.

Choices made without asking

  • External failures keep a driver-side error. moq-uring's socket dying and moq-tokio's endpoint driver going away keep a driver-side error that wins when handles report the connection error. The driver also calls moq-quic's close() so every parked waiter wakes. moq-quic gained no "fail with a foreign error" API.
  • Ingress wakes stay. moq-uring's endpoint kick after demuxing, and qmux's reader waking the writer after a record, remain. They move timers and may queue replies that no application call made, which poll_transmit's wait does not cover. moq-tokio's ingress already arrives on its own channel, so its driver waker is deleted.
  • ShouldTransmit is still returned, but consumers ignore it. Chunks::finalize and the lease methods still return it. moq-quic now wakes the transmit waiter itself.
  • Fanout bench added to Nightly. The new bench runs in Nightly's rust-extra group, as sh/gh/nightly.test.py requires of every bench target. qmux drops rustc-hash, which only its waiter maps used.
  • The streams bench measures the waits now. The bench harness drives reads with real waiters and a run queue instead of events, so before/after numbers measure the whole change, not the lists alone.

Measurements

Base is origin/main at 406ab9a, head is this branch. Each side ran once, sequentially, from separate target directories. A single run is noisy: schedule/*, which this branch does not touch, moved 0.70x to 1.64x.

Driver fanout. rs/moq-tokio/benches/fanout.rs is new and uses only the unchanged public driver API, so the same file ran on both sides. It opens N uni streams at once on one connection, writes 1 KiB on each, and reads them all, so up to N waiters are parked.

N base, per stream head, per stream head/base
1 7.8 µs ±14% 6.4 µs ±4% 0.83
16 2.3 µs 2.0 µs 0.84
256 2.1 µs 2.2 µs 1.06
1024 5.0 µs ±12% 5.0 µs ±9% 0.99

No slope: the step from 256 to 1024 appears on both sides (1 MiB per iteration outgrows the window).

moq-quic/benches/streams.rs. The harness now drives reads with real waiters instead of events, so the end-to-end cases are not quite like for like. Nothing scales with stream count:

  • connection_blocked is flat or better from 1 to 1024 live writers (0.79x to 1.06x).
  • send_priority at 1024 streams is unchanged (1.00x to 1.02x, plus one base outlier).
  • set_priority showed 1.08x to 1.38x on the first run, but 0.96x to 0.97x on a rerun of that group alone.
  • The one consistent mild cost is send_write_size/window_16k, at 1.13x to 1.25x. Those are flow-control-blocked writes on a 16 KiB window, and the cost does not grow with streams.

Follow-ups

  • Already planned in quest/m1/quic-api/poll.md (moq-dev/moq#7568): remove the now-redundant ShouldTransmit from moq-quic's public API, and test moq-tokio's close while the socket is blocked.
  • A few unreachable!s on "released ID" paths in moq-tokio and moq-uring stopped run under the connection lock. They are invariants today, like the existing one in poll_write.
  • moq-tokio::integration backend::quic_client_close_drains_finished_track failed once under the full parallel run (subscribe failed: Unroutable, rs/moq-tokio/tests/integration/backend.rs:902), then passed 8 of 8 isolated runs and a full rerun. It looks like a load-dependent race between the auth grants and the subscribe, unrelated to the waits, but it was not reproduced on main.

Review

Head 4718586: local review skipped under the simple-review rule (moq-dev/quest#364). It is a one-line wake in Connection::local_address_changed, a regression test, and doc comments, and it touches no public API, wire format, security code, or unsafe. Online review findings on b423b27:

  • Pullfrog, local_address_changed queues frames without waking the parked driver: fixed. It now calls wake_transmit, the poll_transmit doc lists it, and waits::transmit_hears_a_local_address_change covers it. I tried a wake in set_probe_target as well and dropped it. Both drivers call that on every poll, so the wake made moq-tokio's sim::* tests spin into their timeout. The doc now says a new target waits for the next poll.
  • Pullfrog, rewording qmux's latch doc to "took it out of poll_transmit": refuted in part. writer_done publishes only after the close batch is flushed and the transport shut, so that wording would be wrong. The doc now names the stalled-close bound (CLOSE_TIMEOUT) it left out. The latch stays, per the maintainer's 2026-11-01 decision to keep it (option (a)).
  • The Codex online review (gpt-6.1-sol) on b423b27 had no findings.

Local checks on 4718586: cargo nextest run --all-targets --all-features -p moq-quic -p moq-tokio -p moq-uring -p qmux@0.7.0 (1543 passed), clippy -D warnings on moq-quic and qmux, and rustfmt on the changed files.

Previous local review, Codex, gpt-6.1-sol at medium effort, via quest review --base origin/main. Latest round: a fresh session (round 1) on head 803ded3, with no findings. It covered the origin/main merge, whose conflicts were resolved by hand. The current head b423b27 adds only a conflict-free merge of origin/main (a quest deletion, moq-dev/moq#7496).

What the merge resolved:

  • moq-tokio: handle drops take lock_for_drop (moq-dev/moq#7360), now in StreamRef too. The poisoned-driver terminate takes no shared argument here, and the test poison helper no longer wakes the driver waker this PR deleted (the driver reaches the poisoned lock on its first poll or its timer).
  • moq-uring: handles count toward the last-drop close (moq-dev/moq#7309). A dropped Driver now fails through Inner::fail, which also closes moq-quic, so handles parked in moq-quic's waits wake rather than hang.
  • moq-quic tests: the congestion renames (moq-dev/moq#7258) and main's keep-alive default test (moq-dev/moq#7449).

Earlier rounds, on the pre-merge branch (session 1): round 1 (head 03ccfb4) had one finding, fixed in e1a5ef1: moq-tokio's driver also parks on the non-consuming poll_closed, so a local close re-arms the closing timer while the socket holds output back. Rounds 2 (e1a5ef1) and 3 (4d71425) found nothing.

Local checks on 803ded3:

  • just rs check --workspace (clippy -D warnings, fmt, docs, shear, sort).
  • just rs test -p moq-quic -p moq-tokio -p moq-uring -p qmux@0.7.0: 1485 passed (see the flake under Follow-ups).
  • just rs uring-check.
  • cargo check --all-targets --all-features on those four crates, and quest check --base origin/main.

(Written by Claude Opus 5.5)

🤖 Generated with Claude Code

kixelated and others added 14 commits October 31, 2026 22:49
Quest-Claim: def91ad129e6e8bf
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
moq-tokio's quic::driver, moq-uring's driver, and qmux delete their
per-stream and per-connection waiter tables, Notifys, and stopped map,
and wait on moq-quic's poll_* forms instead. They keep their I/O, timers,
the driver kick, and the latch that publishes a local close once the
close frame is out.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Every wait is a poll_* on &kio::Waiter, so the events that reported them
go: StreamEvent, Event's HandshakeDataReady, Connected,
HandshakeConfirmed, ConnectionLost, Stream, and DatagramReceived, and
QMux's Event and poll. Event keeps PathUpdated. A reset that frees its
stream keeps the acknowledged prefix, so poll_acked still answers for it
once the Acked event's offset is gone.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
poll_transmit takes the driver's waiter and returns Poll<Transmit>: with
nothing to send it registers the waiter, which the application's writes,
finishes, resets, stops, refused opens, returned credit, datagrams, new
limits, and close wake. Pacing and loss recovery stay on poll_timeout.
QMux's poll_transmit takes the same shape.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Handles no longer wake their driver after mutating the connection:
moq-quic wakes the waiter the driver parked on poll_transmit. The drivers
keep only their ingress wake. qmux's stalled writer waits on poll_closed,
so a local close still starts its close bound while output is blocked.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Also tests the transmit wait, and formats.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Also drops qmux's rustc-hash, which only its waiter maps used.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…c55ddeffe12

# Conflicts:
#	rs/moq-tokio/src/quic/driver/connection.rs
…ming it

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
kixelated and others added 3 commits November 1, 2026 14:49
Conflicts resolved against this branch's moq-quic-owned waits:
- moq-tokio: handle drops take `lock_for_drop` (#7360), now in
  `StreamRef` too; the driver's poisoned-state `terminate` takes no shared
  argument, and the test poison helper no longer wakes a driver waker this
  branch deleted.
- moq-uring: handles count toward the last-drop close (#7309); a dropped
  driver fails through `Inner::fail`, which also closes moq-quic so handles
  parked in its waits wake.
- moq-quic tests: the congestion renames (#7258) and the keep-alive default
  (#7449).
- quest poll.md keeps the follow-ups planned from this PR, minus the link to
  the waits quest this PR deletes.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Settled in #7490: the latch is I/O behavior moq-quic can't see, publishing a local close only once the CONNECTION_CLOSE is staged (moq-uring) or delivered with the transport shut (qmux).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@kixelated
kixelated marked this pull request as ready for review November 1, 2026 23:00
@coderabbitai

coderabbitai Bot commented Nov 1, 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 50 minutes.

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
  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: ee8d0428-2af1-4725-bf72-9965c2aecd91
Commits

Reviewing files that changed from the base of the PR and between de6c6d7 and 4718586.

Files ignored due to path filters: 1
  • Cargo.lock is excluded by !**/*.lock
Files selected for processing: 45
  • quest/m1/quic-api/README.md
  • quest/m1/quic-api/poll.md
  • quest/m1/quic-api/waits.md
  • quest/m2/perf/quic-borrow-free.md
  • quest/m2/perf/quic-lock-contention.md
  • quest/m2/quic-ack-hook.md
  • quest/m2/quic-sansio-driver.md
  • rs/moq-quic/MIGRATE.md
  • rs/moq-quic/README.md
  • rs/moq-quic/benches/common/mod.rs
  • rs/moq-quic/benches/receive.rs
  • rs/moq-quic/src/connection/mod.rs
  • rs/moq-quic/src/connection/mux/mod.rs
  • rs/moq-quic/src/connection/mux/tests.rs
  • rs/moq-quic/src/connection/streams/lease.rs
  • rs/moq-quic/src/connection/streams/mod.rs
  • rs/moq-quic/src/connection/streams/recv.rs
  • rs/moq-quic/src/connection/streams/send.rs
  • rs/moq-quic/src/connection/streams/state.rs
  • rs/moq-quic/src/connection/waits.rs
  • rs/moq-quic/src/crypto.rs
  • rs/moq-quic/src/fuzz/connection.rs
  • rs/moq-quic/src/lib.rs
  • rs/moq-quic/src/tests/acked.rs
  • rs/moq-quic/src/tests/characterize/frames.rs
  • rs/moq-quic/src/tests/characterize/mod.rs
  • rs/moq-quic/src/tests/mod.rs
  • rs/moq-quic/src/tests/reset_stream_at.rs
  • rs/moq-quic/src/tests/sim.rs
  • rs/moq-quic/src/tests/token.rs
  • rs/moq-quic/src/tests/util.rs
  • rs/moq-quic/src/tests/waits.rs
  • rs/moq-tokio/Cargo.toml
  • rs/moq-tokio/benches/fanout.rs
  • rs/moq-tokio/src/quic/driver/connection.rs
  • rs/moq-tokio/src/quic/driver/recv.rs
  • rs/moq-tokio/src/quic/driver/send.rs
  • rs/moq-tokio/src/quic/driver/tests.rs
  • rs/moq-uring/src/quic/driver/connection.rs
  • rs/moq-uring/src/quic/driver/recv.rs
  • rs/moq-uring/src/quic/driver/send.rs
  • rs/qmux/Cargo.toml
  • rs/qmux/src/session.rs
  • rs/qmux/src/session/tests.rs
  • sh/gh/nightly.sh
  • 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

@kixelated kixelated left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review by review (OpenAI)

Reviewed commit: b423b27

No actionable bugs found in the changed areas reviewed. Centralizing waits in moq-quic removes duplicated waiter tables and event translation; retaining ingress wakes and transport-specific close-publication latches is justified by their distinct I/O responsibilities. I traced transmit wakeups, close/error propagation, receive draining, acknowledged-prefix retention after reset, and the lifetime of Tokio stopped() futures across the three adapters. The close-wait registration addresses the earlier local-review issue described in the PR.

Verification limits: static review of the diff and relevant surrounding code through GitHub; I did not run tests or benchmarks or independently reproduce the reported performance results.

(Written by gpt-6.1-sol)

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ Minor suggestions only.

Reviewed changes

  • Consumers wait through moq-quic's poll_* — moq-tokio's driver, moq-uring's driver, and qmux drop their own per-stream/per-connection waiter tables, accept/open/datagram lists, and event sweeps; moq-tokio holds a kio::Park, and its stopped() futures keep the stream's ID until the last of the stream and its futures drops.
  • poll_transmit is the driver's wait — both QUIC and QMux register the driver's waiter when they have nothing to send, and every application-side queue wakes them through Waits::wake_transmit; pacing and loss recovery stay on poll_timeout.
  • New waits, and deletions — Connection::poll_handshake_data, Connection::error, and recv::Stream::poll_received_reset replace the last event-shaped waits; StreamEvent, the waiting Event variants, and mux::Event/mux::Connection::poll are gone, leaving Event::PathUpdated.
  • ends keeps a reset's acknowledged prefix — poll_acked still answers for that prefix after the stream's state is freed.
  • Tests and benches — event-expectation tests became wake-counting tests, and a moq-tokio fanout bench swept over streams per connection is wired into Nightly.

Pullfrog  | Fix all ➔ | Fix 👍s ➔ | View workflow run | Using deepseek-v4.1-flash (free via Pullfrog for OSS) | 𝕏

Comment thread rs/moq-quic/src/connection/mod.rs Resolved
Comment thread rs/qmux/src/session.rs Resolved
`local_address_changed` queues a PING and any RETIRE_CONNECTION_ID, so it
wakes `poll_transmit` like every other application call; the doc lists it,
and says why a new probe target needs no wake. qmux's latch doc names the
stalled close bound.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes

Since the prior pullfrog review (b423b2740):

  • local_address_changed wakes the parked driver — it now calls self.streams.waits.wake_transmit() after queueing the PING and any RETIRE_CONNECTION_ID, so a sans-IO embedder that called it re-polls instead of leaving the frames queued.
  • poll_transmit doc names the new wake and the probe-target case — "a local address change" joins the wake list, and a new probe target is documented to wait for the next poll, since a wake in set_probe_target would spin both drivers, which call it every poll.
  • Regression test — waits::transmit_hears_a_local_address_change parks the driver, calls local_address_changed, and asserts the wake plus that the PING is what poll_transmit finds.
  • qmux close-latch docs — closed/local_close now name the stalled path ("gave up after [CLOSE_TIMEOUT]"), matching writer_done/run_writer/poll_stalled.

Pullfrog  | View workflow run | Using deepseek-v4.1-flash (free via Pullfrog for OSS) | 𝕏

@kixelated
kixelated enabled auto-merge (squash) November 1, 2026 23:49
@moq-bot

moq-bot Bot commented Nov 2, 2026

Copy link
Copy Markdown
Contributor

quest land: this pull request conflicts with a pull request queued ahead of it, in these files:

  • #7512: sh/gh/nightly.sh

Once one of them lands, this one conflicts with the base and is ejected when its turn comes. Resolve the overlap before then, such as by coordinating with that author, or by merging the base once it lands.

@moq-bot

moq-bot Bot commented Nov 2, 2026

Copy link
Copy Markdown
Contributor

codex / Codex land fixer note:

The supplied batch log reports a stack overflow in stack::debug_relay_load_and_run_fit_windows_stack at lines 22355–22364: "fatal runtime error: stack overflow, aborting". No mechanical source change was justified. The test passed in relay-only builds, with io_uring and WebRTC, and with the full workspace feature graph, both with this PR and on unchanged QUIC code. The full touched-package suite passed all 1,965 runnable tests, with 41 skipped. No PR could be identified as the cause, so blame is unset. Please follow up on this intermittent stack-budget failure.

The workspace rerun passed with cargo nextest run --locked --workspace --exclude moq-net-fuzz --exclude moq-quic-fuzz --features moq-video/capture,moq-audio/capture,moq-audio/playback,moq-cli/play,moq-cli/openh264,moq-tokio/webrtc,moq-relay/io-uring --test integration -E 'test(=stack::debug_relay_load_and_run_fit_windows_stack)'. The local build used installed pkg-config and libclang paths. The PR branch remains at its pinned head; base was merged only in a temporary detached checkout for validation.

(written by codex / Codex)

@moq-bot
moq-bot Bot disabled auto-merge November 2, 2026 03:24
@moq-bot moq-bot Bot removed the quest:merge label Nov 2, 2026
@moq-bot

moq-bot Bot commented Nov 2, 2026

Copy link
Copy Markdown
Contributor

quest land removed this pull request from the queue: this maintainer pick already used its mechanical fix attempt. Run /quest-iterate to diagnose the repeated failure, then pick it again.

This branch has not been deployed

No deployments
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