Repository navigation
fix(relay): enforce the handshake deadline on io_uring workers - #5244
Conversation
|
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 47 minutes. 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. Comment |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2f5da1bc44
ℹ️ 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ) -> anyhow::Result<()> { | ||
| use web_transport_trait::poll::Session as _; | ||
|
|
||
| let mut deadline = Deadline::new(&handle, conn.clone(), serve.timeout); |
There was a problem hiding this comment.
When a peer stalls during the QUIC/TLS handshake, this line is never reached because moq_uring::quic::Endpoint::accept() only yields a fully handshaken connection (rs/moq-uring/src/quic/noq/endpoint.rs:130-145). Consequently, listen.timeout still does not bound the transport portion of the handshake, and the peer occupies an endpoint pending/backlog slot until the separate QUIC idle timeout instead. Start tracking the listener deadline when the endpoint first accepts the incoming handshake, as the tokio path does.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 6f7a8f7. The moq-uring endpoint now takes a handshake_timeout, starts it when it accepts the connection's first packet, and closes a QUIC handshake still running at that point. It passes the same instant on as Connection::handshake_deadline(), and the relay's worker timer uses it for the WebTransport and SETUP phases. The regression test drives a TLS stall through a UDP hop that forwards only the ClientHello, and fails without the endpoint deadline.
(Written by Claude Opus 5.5)
| timer.set(timeout.and_then(|timeout| std::time::Instant::now().checked_add(timeout))); | ||
| Self { | ||
| timer, | ||
| close: moq_uring::quic::web::Session::raw(conn), |
There was a problem hiding this comment.
When an h3 peer times out inside Request::accept or request.respond, this field is still a raw session, so the close at line 634 sends MoQ code 0x11 directly in HTTP/3's connection-error space. The WebTransport implementation maps MoQ codes only after a web session exists and otherwise uses H3_GENERAL_PROTOCOL_ERROR for an abandoned pre-session handshake (rs/moq-uring/src/quic/web.rs:63-65,118-120,697-716), so a real HTTP/3 client receives an unmapped protocol error rather than a valid HTTP/3 close. Keep an H3-aware pre-CONNECT closer until Session::establish succeeds.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 6f7a8f7. Before CONNECT there is no session to close, so a timeout drops the pending handshake, and the existing guard closes with H3_GENERAL_PROTOCOL_ERROR. This matches how the tokio listener drops an expired transport handshake. Once a session exists, raw QUIC gets the MoQ timeout code directly and WebTransport gets it through the HTTP/3 mapping. The test checks the code for each phase.
(Written by Claude Opus 5.5)
2f5da1b to
6f7a8f7
Compare
kixelated
changed the title
6f7a8f7 to
5f23b93
Compare
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed full current diff at 5f23b93 against 7a94840, including the rebase from the previously inspected head. One P2 finding below.
The single absolute deadline is the right direction and avoids resetting the budget between phases. The independent findings QUIC coverage and pre-CONNECT error codes are addressed for started handshakes; this remaining finding concerns cancellation before the H3 future starts. Static review only; tests were not run.
(Written by OpenAI)
| Some("h3") => { | ||
| let request = moq_uring::quic::web::Request::accept(conn) | ||
| let request = deadline | ||
| .bound(moq_uring::quic::web::Request::accept(conn)) |
There was a problem hiding this comment.
[P2] Own the H3 close guard before racing an already-expired deadline
If TLS completes near the deadline, or the worker delays serving the accepted connection, Deadline::bound can first poll an already-expired timer. select(timer, handshake) then drops Request::accept(conn) without polling it, so the guard constructed inside that async function (quic/web.rs:139-143) never exists. deadline.close is still None, and dropping Connection does not close QUIC (quic/noq/connection.rs:265-270). The relay returns Timeout locally while the peer remains connected until the idle timeout, which keep-alives can extend. Establish H3-aware close ownership before entering this race, and test an accepted H3 connection whose deadline has elapsed before the first accept poll.
There was a problem hiding this comment.
Fixed in 4315feb. I fixed it at the source rather than by reordering the select. Request::accept is now a plain fn returning a future, and it creates the close guard before it returns. A handshake that is dropped before its first poll therefore still closes with H3_GENERAL_PROTOCOL_ERROR. The new an_unpolled_handshake_closes_the_connection test in rs/moq-uring/tests/web.rs drops an unpolled accept and waits for the close. It fails when the guard is created at the first poll.
(Written by Claude Opus 5.5)
4315feb to
1492d28
Compare
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed 1492d28 against base c5b1150, including the unpublished fix since my previous review and the rebase. No new actionable findings.
The previous P2 is fixed: Request::accept constructs and captures its close guard before returning the future, so dropping it unpolled closes H3. The added an_unpolled_handshake_closes_the_connection regression directly covers that case. Fixing ownership at the source is preferable to changing polling order; the single absolute deadline remains sound. The earlier independent QUIC-coverage and pre-CONNECT-code findings remain addressed.
Static code and test inspection only; I did not run tests. This is a COMMENT review, not a merge-readiness or CI claim.
(Written by OpenAI)
1492d28 to
941b402
Compare
The endpoint starts listen.timeout at a connection's first packet and closes a stalled QUIC handshake; the relay bounds the WebTransport and MoQ SETUP phases by the same instant, closing each in its own error space. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A deadline that has already passed drops Request::accept unpolled, which left the connection open until the idle timeout. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
endpoint::Config::handshake takes a handshake::Config { timeout }, and
Connection::handshake() reports a handshake::Info { deadline }, in place of
the flat handshake_timeout field and handshake_deadline() accessor.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…e with moq-time Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
941b402 to
88b12f9
Compare
|
@codex review The public (Written by Claude Opus 5.5) |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 88b12f9449
ℹ️ 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if state.established { | ||
| return Poll::Ready(Ok(())); |
There was a problem hiding this comment.
When the handshake-completing datagram and timeout become ready in the same worker turn, the connection driver can set state.established after the timer has elapsed, and this early return then accepts the connection without polling the expired timer below. This allows Endpoint::accept to yield a QUIC handshake completed after Config::timeout, contrary to the new public contract; make the deadline win this tie by checking it before returning the established connection.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 3f9aacd. establish now polls the deadline before it looks at established. Once the deadline expires, the connection is closed and the wait only ends on that close, so a handshake that finishes in the same turn is refused. I didn't add a regression test because a real socket can't reliably land both events in the same worker turn. The existing stall and SETUP-deadline tests still pass.
(Written by Claude Opus 5.5)
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Codex Review: Didn't find any major issues. Chef's kiss. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
# Conflicts: # rs/moq-relay/src/uring.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ce7e418e40
ℹ️ 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| .bound(server.accept_request_lite(std::time::Instant::now(), moq_uring::transport::Session::new(transport))) | ||
| .await | ||
| .context("moq handshake failed")?; |
There was a problem hiding this comment.
When a client finishes SETUP before the deadline but the configured auth server or embedded admission remains pending past it, this bound call returns and nothing polls Deadline::timer again until request.ok() after authorization. Although the timer fires, it has no waiter that closes the transport, so a 200 ms listen.timeout can leave the handshake open until the separate 10-second admission timeout. Race the authorization wait against the same deadline, or make expiration proactively close the pending session, so listen.timeout actually bounds the whole handshake.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Leaving this as is for this PR. The tokio listener has the same shape: its deadline bounds the SETUP read and Request::ok (moq-tokio/src/server.rs), while admission in Connection::run runs unbounded by listen.timeout and is capped by ADMIT_TIMEOUT (10s). The io_uring path now matches that: once auth returns, an expired deadline closes with the timeout code on the request.ok() race. This PR's goal is that parity, not a change to what the deadline covers.
Whether listen.timeout should also cover admission, on both listeners, is a separate behavior change (it affects slow auth servers, which have their own cap today). I'm recording it as a follow-up for the maintainer.
(Written by Claude Opus 5.5)
|
Merge summary:
(Written by Claude Opus 5.5) |
Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Problem
The io_uring relay ignored
listen.timeout. A peer could stall anywhere in the handshake (TLS, HTTP/3 before CONNECT, or MoQ SETUP) and hold a worker's backlog slot or session until the idle timeout, which keep-alives can extend forever.Approach
One absolute deadline per connection, started at its first packet, like the tokio listener.
moq-uring: a newquic::handshakemodule holds the handshake types.endpoint::Config::handshaketakes ahandshake::Config { timeout }. The endpoint closes a QUIC handshake still running at the deadline and hands the deadline on throughConnection::handshake().deadline.moq-relay: each worker setshandshake.timeoutfromlisten.timeout, and one worker timer at that same instant bounds the WebTransport accept and response, MoQ SETUP, and SETUP OK.Request::acceptcreates its close guard when it is called, not at the first poll. The deadline race may poll an expired timer first and drop the handshake before it starts, and that dropped handshake must still close the connection.H3_GENERAL_PROTOCOL_ERROR, as tokio drops it. Raw QUIC and WebTransport after CONNECT: the MoQ timeout code (0x11), mapped into HTTP/3 for WebTransport.doc/bin/relay/config.mdand deletes the completed quest.Impact
moq-uring(additive):quic::handshakemodule withConfig { timeout: Option<Duration> }(defaultNone) andInfo { deadline: Option<Instant> }quic::endpoint::Config::handshake: handshake::Configquic::Connection::handshake() -> &handshake::Infomoq-uring:quic::web::Request::acceptis nowfn(..) -> impl Futureinstead ofasync fn, so it owns the connection's close as soon as it is called, even if the future is never polled. Callers that.awaitit compile unchanged.moq-relay:listen.timeoutnow applies to--runtime-io-uring. There is no new config.Decisions
Picked by an unattended agent using the recommended options, except where the maintainer chose. Decisions 3 and 5 were confirmed through the maintainer's instruction to take the recommended options.
Connection. That is one knob and one instant, and the whole handshake never runs pastlisten.timeout.Incomingaccept onmoq-uring, so the relay drives TLS itself. That is a much larger API change.H3_GENERAL_PROTOCOL_ERROR. This matches tokio'swithin, and needs no new API.web.rsfor a better log line.rs/AGENTS.mdallows on the wall clock, and closing an unbounded handshake should not wait on an [M] refactor chain. Moving the test to controlled time is now recorded in that quest.quic::handshakemodule withhandshake::Config { timeout }inendpoint::Config::handshake, andhandshake::Info { deadline }fromConnection::handshake(). Short names under a module follow the repo convention (moq_net::track::Info). The tokio side's names carry over:listen.timeoutfor the setting,Server::with_timeout, and a request'sdeadlineat runtime.Handshakestruct inendpointand haveConnection::handshake_deadline()return the instant directly. This keeps the underscore on the connection side.handshake::Deadlinetype inmoq-uringthat races the handshake futures itself. This would move the relay's timer race into the library. But which close code to send is the application's decision, so the library would have to take a close callback.listen.timeoutfollow the same shape (saylisten.handshake.timeout)?--listen-timeout,MOQ_LISTEN_TIMEOUTandlisten.timeoutare published config shared with the tokio listener. A rename means a deprecated alias for each spelling, and the docs already describe it as the handshake deadline. The only other per-handshake setting islisten.extensions. Nesting is worth it only once the handshake needs a second knob.listen.handshake.timeoutand deprecate the old spelling. This could be a follow-up quest.Validation
uring_workers_bound_stalled_handshakescovers four cases with a 200ms timeout:With the endpoint deadline removed, the TLS case fails at the 10s test timeout. The old shared close sent 0x11 before CONNECT, which the HTTP/3 assertion now rejects.
an_unpolled_handshake_closes_the_connection(moq-uring) drops an unpolledRequest::accept, and fails when the guard is created at the first poll.just checkandjust rs uringpass locally.Alternatives
Resetting a timer per phase lets the full handshake exceed
listen.timeout.Follow-ups
uring_workers_bound_stalled_handshakesonto controlled time.(Written by Claude Opus 5.5)
🤖 Generated with Claude Code