Repository navigation
fix(quic): send a client's queued Finished ahead of its close - #5721
Conversation
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
noq raises Connected on the client with its Finished still queued, and paces it like any other packet. Under load the first RTT sample is long, so the pacer can hold the Finished just as `connect` resolves. A close in that gap replaced it: the server never completed its handshake, saw only a handshake-level close without the application code, and its `accept` never resolved. That is the 60 s hang in the teardown close test. `connect` now resolves once the client has staged one more CRYPTO frame than it had at Connected. A new teardown test reproduces the gap on an idle host through a relay that slows the server-to-client leg. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Outcome: the 60 s hang was reproduced under load (on the pre-#5626 worker, about 1 in 500 runs). The cause was a close that took the place of the client's paced Finished, so the server's (Written by Claude Opus 5.5) |
# Conflicts: # quest/m1/test-flakes-2/README.md # quest/m1/test-flakes-2/uring-teardown-close.md # rs/moq-uring/src/quic/driver/connection.rs
A client is Connected with its Finished only queued, and pacing can hold it there. A close in that gap replaced it: the server never completed its handshake and only read an Initial or Handshake close, which carries no application code, so its accept never resolved. That was the 60 s hang in moq-uring's teardown close test, and moq-tokio shares it. The close flight now carries the queued Finished in its Handshake packet instead of a handshake-level close, and the server reads the 1-RTT close behind it. This replaces the moq-uring driver gate, which only covered one backend. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
kixelated
changed the title
|
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 6 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: 2edc2a34ca
ℹ️ 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".
A close resent after the first flight was lost carried a Handshake close instead, since the Finished had been taken off the queue. Leave it queued so every close flight carries it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 059bd6d08a
ℹ️ 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 buf.len() + frame::Crypto::SIZE_BOUND + frame.data.len() | ||
| > builder.max_size | ||
| { | ||
| break; |
There was a problem hiding this comment.
When a queued Handshake CRYPTO frame does not fit intact, this branch emits none of it and continues with the 1-RTT close. This occurs with a custom TLS client whose client-certificate flight exceeds one packet, and can also occur when the preceding ACK consumes the space that was reserved only for a CONNECTION_CLOSE; unlike populate_packet, this loop never fragments the CRYPTO frame. The server therefore still cannot complete its handshake, so this path must prioritize and split the queued CRYPTO data across packets.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Partly agreed, fixed in c5136b6 without fragmenting. Including part of the flight is worse than today: the server still cannot complete its handshake, and it no longer gets a Handshake close either, so it would idle out. So it is now all or nothing: if the unacknowledged CRYPTO does not fit in the close packet after its ACK, the client sends the plain Handshake close as before.
That leaves one gap: an mTLS client (moq-tokio and moq-uring both support client certificates) whose Certificate flight exceeds one packet and that closes before the server has it still loses the application code, exactly as before this PR. Splitting it across several close packets is a larger change to the close path; I have left it to the maintainer as a follow-up decision rather than grow this PR.
(Written by Claude Opus 5.5)
A Finished already sent but lost left the same gap: the close went out as a Handshake close the server cannot complete its handshake from. The close flight now carries every unacknowledged Handshake CRYPTO frame, queued or in flight, until the client discards its Handshake keys. A flight too big for one packet keeps the plain close, since part of it would not complete the handshake either. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A client flight that fit the close packet but not the coalesced 1-RTT close made a one-datagram poll resend the Handshake packet forever. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
# Conflicts: # quest/m1/test-flakes-2/README.md
|
Merging at 6827e4f: main merged after #5872 (one quest README conflict). Iterating found a close flood the head introduced, where a client flight fit the close packet but left no room to coalesce the 1-RTT close; fixed in 774045d by falling back to the plain close (decision 5a, approved by the maintainer), with a certificate-size sweep regression test. Codex thumbs-up after the push; CI green; moq-quic 521/521; moq-uring teardown 20/20 under stress. (Written by Claude Opus 5.5) |
Problem
a_published_close_has_already_left_the_client(moq-uring) hung until nextest's 60 s timeout under load (#5483'sjust check). Neither suspected cause was it: the close did leave the client, and no wake was lost.The client QUIC stack raises
Connectedonce it has processed the server's flight, before the server has the client's Finished (the last TLS handshake message, which the server needs to complete its side). The Finished may still be queued behind the pacer, or lost in flight. If the application closes in that gap, the close flight replaces the Finished. The server never completes its handshake and only reads the Initial and Handshake copies of the close, which RFC 9000 §10.2.3 strips of the application code (APPLICATION_ERROR, code 12). The incoming connection fails and the server'sacceptnever resolves.Reproduced under load on the pre-#5626 moq-uring worker, about once per 500 runs. Since #5664 both moq-uring and moq-tokio run on moq-quic, so both backends share the race.
Approach
In moq-quic: until a client discards its Handshake keys, a close carries every unacknowledged Handshake CRYPTO frame (queued or in flight, normally just the Finished) in its Handshake packet, in place of the Initial and Handshake closes. The server completes its handshake on the Finished and reads the 1-RTT close coalesced behind it, application code intact. Nothing is dequeued, so a close resent after a lost flight (in answer to the server's retransmitted handshake) carries it again.
It is all or nothing: the CRYPTO must fit in the close packet with room left to coalesce the 1-RTT close, or the client sends the plain Handshake close as before, since part of the flight would not let the server complete either. Nothing else about close changes: a close with no unacknowledged Finished, and every server close, still go out in every space as before.
The room for the 1-RTT close matters: without it, a client certificate flight that fit the packet but crowded out the close made every
poll_transmitwith a one-datagram budget (moq-tokio without GSO, a short moq-uring buffer) resend the Handshake packet and never reach the close, flooding the peer until the drain timer.Regression
Deterministic moq-quic tests: the client processes the server's flight without transmitting, closes on
Connected, and the server must seeConnectedthen the application close.close_before_the_client_finished_leaves: Finished still queued. Without the fix the server seesConnectionClosed(APPLICATION_ERROR)and never connects.close_after_the_client_finished_is_lost: the datagram carrying the Finished is dropped before the close. Fails if only the queued Finished is carried.close_before_the_client_finished_leaves_survives_a_lost_flight: the first close flight is dropped. Fails if the Finished is only sent once.close_before_a_client_certificate_leaves_ends: sweeps an mTLS client certificate's size across the close packet's room, polling with a one-datagram budget. Each close must leave in one datagram, and a server that completes must read the application close. Without the room check, a 512-byte padded certificate takes 3+ datagrams and never sends the 1-RTT close.The first revision's moq-uring relay test (
a_close_right_after_dialing_follows_the_handshake) is dropped: on currentmain, without any fix, it passes 5/5, because moq-quic's pacing no longer holds the Finished on that path. It no longer guards anything, and it depended on wall-clock sleeps.Validation
just check(scoped, after mergingmainwith refactor(quic)!: send options for streams and datagrams, private send domains #5872): passes, 5249 tests.cargo nextest run -p moq-quic: 521/521.--stress-count 20on io_uring: 20/20.Impact
[Handshake: ACK, CRYPTO(Finished)]+[1-RTT: CONNECTION_CLOSE]instead of[Initial: CONNECTION_CLOSE]+[Handshake: CONNECTION_CLOSE]+[1-RTT: CONNECTION_CLOSE]. Standard QUIC; any peer handles it.Alternatives / Decisions
Settled by the iterating agents on the recommended option (no maintainer prompt was available); reopen any you disagree with.
Connectedevent until the Finished is staged. Also misses a lost Finished, and the event would be raised frompoll_transmit, which both drivers would need restructuring to notice.poll_transmitcalls.Follow-ups
(Written by Claude Opus 5.5)
🤖 Generated with Claude Code