Repository navigation
feat(tokio): accept_with and connect_with hand an established session to moq-net's policy - #8085
Conversation
Quest-Claim: 6d29d5f662a39e19
…ux session to moq-net's policy 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>
|
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
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit 2785505.
No actionable findings in the changed code. I traced the new accept/connect handoffs, the shared Request constructor and limit application, and the WebRTC establishment guard against the surrounding moq-net activity/deadline logic, qmux session lifetime, and runtime clock implementation. The two-phase WebRTC timeout behavior is documented, and the establishing guard remains held until the handoff has acquired its own activity guard.
Direction: no code changes requested from this static review; await CI before treating validation as complete.
Verification limits: GitHub-only source review of all seven changed files and relevant surrounding code. I did not run builds, tests, quest commands, or browser checks. The Check workflow for this SHA was still in progress when inspected.
There was a problem hiding this comment.
ℹ️ The handoffs themselves check out; one gap worth recording for the WebRTC rename.
Reviewed changes
Listener::accept_with/Client::connect_with— new handoffs that read SETUP, or run the connect policy, over an established qmux session, each bounding only its own half.- WebRTC through them —
accept_webrtcnow holds anActivityguard across ICE and channel setup before handing off, andconnect_webrtcestablishes then delegates. Each phase keeps a full timeout, so the worst case doubles (documented). - One
Requestconstructor — five hand-built literals collapse intoRequest::accept/accept_qmuxoverIncoming, with the cfg-gatedquic/qmuxfields replaced byUnder. - Quest bookkeeping —
tokio-handoff.mddeleted with its epic## Requiredline, and the WebRTC rename gains a note about owning its establishment bound.
The refactor is faithful to the old paths (guard ordering, url/authority/identity plumbing, Under dispatch in quic()/qmux_window()/ok()), and the new drain test genuinely pins the invariant: _establishing is bound from accept_webrtc's first poll and the handoff into accept_with has no suspension point where the count could reach zero.
deepseek-v4.1-flash-1017 (free via Pullfrog for OSS) | 𝕏
|
|
||
| - Move `rs/moq-rtc` to `rs/moq-webrtc` and merge `rs/moq-ice` in as a module. Keep the gateway's `server` feature; add a feature for the data-channel transport so WHIP/WHEP-only builds skip SCTP. | ||
| - Move moq-tokio's `webrtc` module (`rs/moq-tokio/src/webrtc.rs`, host gathering, `.local` resolution) into moq-webrtc, built on moq-tokio's public session types. Its connect and accept go through moq-tokio's `connect_with`/`accept_with` handoffs, which run moq-net's session policy over the established data channel. | ||
| - Move moq-tokio's `webrtc` module (`rs/moq-tokio/src/webrtc.rs`, host gathering, `.local` resolution) into moq-webrtc, built on moq-tokio's public session types. Its connect and accept go through moq-tokio's `connect_with`/`accept_with` handoffs, which run moq-net's session policy over the established data channel. The handoffs take the qmux session and bound only the MoQ handshake from the call; bounding ICE and the channel coming up (moq-tokio's wrappers use the listen and connect timeouts today) moves with the transport, so moq-webrtc needs its own establishment bound. |
There was a problem hiding this comment.
The note records the establishment bound that moves out with ICE, but accept_webrtc also does two things before the handoff that a Listener handle cannot express: it holds a moq_net::server::Active guard across establishment so a drain waits for a peer still coming up (the round-1 fix, pinned by a_drain_waits_for_an_establishing_peer), and it refuses a data-channel protocol the listener's Versions does not speak. Listener exposes neither its Activity nor its Versions, and accept_with counts the connection only from the handoff and never checks link.alpn, so the rename either plumbs both in from the app's server::Config/listen::Config or silently loses those guarantees. Recording that here (or folding the checks into accept_with) keeps the follow-up from regressing them.
Technical details
# WebRTC handoff leaves the listener-identity checks behind
## Affected sites
- `quest/m2/webrtc-rename.md:14` — the new note covers only the establishment deadline.
- `rs/moq-tokio/src/server.rs:861` — `Listener::accept_with` enters `Activity` only at the handoff, and copies `link.alpn` through unchanged.
- `rs/moq-tokio/src/server.rs:887` — `_establishing` guard, on the private `Server::activity`.
- `rs/moq-tokio/src/server.rs:888` — `versions.alpns()` gate, on the private `Server::versions`.
## Required outcome
- Either the rename quest records that moq-webrtc also needs the listener's `Activity`
(to hold `Active` across ICE/channel setup) and its `Versions` (to refuse a non-MoQ
data-channel protocol), or `accept_with` owns both checks so a consumer cannot drop them.
## Suggested approach (optional)
- Add `Listener::activity()` / `Listener::versions()` accessors, or have `accept_with`
validate `link.alpn` against the server's versions and accept a pre-acquired `Active`
for the establishment window.
- The same gap applies to the `connect_with` side: `Client::versions` and `Client::policy`
are `pub(crate)`, so a crate handed a `Client` cannot re-run `connect_webrtc`'s ALPN gate.
Summary
Completes moq-tokio becomes a thin shell, part of the session policy epic. Its prerequisites moq-dev/moq#8003 (accept policy) and moq-dev/moq#7904 (connect policy as
moq_net::Reconnect) already moved the policy itself into moq-net and deletedDeadlineand friends. This PR adds the handoffs and removes the per-transport duplication that was left:Listener::accept_with(transport, session, link)reads SETUP on a qmux session established somewhere else and returns the usualRequest. moq-net's accept deadline covers SETUP, admission and the reply, counted from the call.Client::connect_with(session)runsmoq_net::Client::connectover an established qmux session, bounded by the connect timeout. A handed-over transport can't be redialed, so there's noReconnect.accept_webrtcandconnect_webrtcnow only bring up ICE, the channel and qmux (bounded by the listen or connect timeout), then hand the session off. That lets the WebRTC rename move them into moq-webrtc without touching moq-tokio's internals.Requestconstructor. Five hand-builtRequest { .. }literals (QUIC/WebTransport, WebSocket, the stream listeners, WebRTC, and a test) are now one privateRequest::acceptover a transport'sIncomingfacts. The two cfg-gatedquic/qmuxfields became oneUnderenum naming the connectionok()widens.tokio-handoff.md. It updates the epic's decision line, and adds a note to the WebRTC rename that moq-webrtc needs its own establishment bound once it owns ICE.Impact
Public API
Listener::accept_withandClient::connect_with, available withwebsocket,tcp, orwebrtc. No public item was removed or changed: the internals this would have dropped (Deadline,qmux_limits,accept_request) were already removed by feat(net)!: accept policy lives in moq-net #8003, andBackoffis now only the CLI knobs that resolve intomoq_net::reconnect::Backoff.webrtcis#[doc(hidden)]): the MoQ handshake's deadline now starts at the handoff, not at theaccept_webrtc/connect_webrtccall. Each phase is still bounded by the same timeout, so the worst case goes from one timeout to two.Migration
Nothing to migrate: the change is additive, and the WebRTC entry points keep their signatures. For the release notes: "moq-tokio:
Listener::accept_withandClient::connect_withrun a MoQ session over a qmux session established outside moq-tokio, such as a WebRTC data channel."Choices made without asking
qmux::Session, not anymoq_nettransport. The one consumer, WebRTC, is qmux. Taking qmux keepsRequest::with_quic_limitsworking in full: it applies stream counts through qmux's ownset_limits, and moq-net'stransport::Limitsonly carries the receive window. Making it generic would mean either silently ignoring those limits or widening moq-net'sLimits, and the coordinator asked to leave moq-net's API alone.qmux::Sessionis already part of moq-tokio's public API (tcp::Listener::accept,websocket::Listener::accept).accept_withtakes theTransportkind as well as theLink, soRequest::transport()reports it. That's three arguments instead of the quest's sketch of(transport, link).accept_webrtcholds a drain guard while establishing and drops it only afteraccept_withhas taken its own. A drain therefore waits for a peer that is still coming up and still admits it (Codex round 1).Tests
webrtc::tests::a_drain_waits_for_an_establishing_peer. It fails with the establishing guard removed and passes with it.webrtc::tests::transferalso checks that the handed-off request reports the channel's ALPN.Validation
quest gate nix develop --command just check origin/mainpassed on 2785505a06, with every module passing. The earlier run on 85d7543 ran 5,890 Rust tests, all passing.cargo clippy -p moq-tokio --no-default-features --features {quic,websocket,tcp,webrtc,uds} --lib -D warningsis clean. Under--profile test, the single-feature builds still warn about dead test helpers inconnection.rsandworker/group.rs, which this PR doesn't touch. Those warnings are also on main, and the integration tests already neededwebsocketthere.dispatch.sh check --queryselected only the Rust lanes, so no Playwright or browser lane ran locally, and none was started.Review
Codex, gpt-6.1-sol, medium effort (
quest review --base origin/main), session01a1b305-640b-73b0-95f5-b41cde8904b6:accept_webrtcnow holds a guard untilaccept_withtakes over, and there's a regression test.Suggested follow-ups
🤖 Generated with Claude Code
(Written by Claude Opus 5.5)