Repository navigation
fix(proto): repeat pending Initial and Handshake ACKs in every packet - #38
Conversation
noq writes ACK frames only while PendingAcks::can_send() holds, which is false once an ACK was sent and no new ack-eliciting packet arrived. If that ACK is lost, the receiver's next packets in the space, such as its PTO probes, carry no ACK, so the sender keeps resending its whole multi-datagram flight. On a path that drops the second datagram of every back-to-back burst, the tail is lost each time and the handshake stalls until a later ACK gets through. In the Initial and Handshake spaces, every packet now repeats the pending ACK ranges, as quinn's can_send_with_other_frames bundles them. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
moq-quic already bundles pending ACK ranges into packets carrying other frames. Pin that for the handshake, so the switch off noq keeps the fix moq-dev/noq#38 ports for the impaired cluster drill. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Maintainer decisions (2026-10-13):
@codex review (Written by Claude Opus 5.5) |
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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughThe packet builder now includes pending Initial and Handshake ACK ranges in eligible packets, even when the ACK scheduler does not permit an ACK. Data-space ACKs still require scheduler permission. A regression test checks that a client repeats a handshake ACK after the server does not receive the first ACK. Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation |
|---|---|---|
| Description check |  |
The description clearly explains the problem, fix, scope, wire and API impact, testing, and remaining PTO behavior. It does not use the template headings or include the change checklist, but it provid… |
| Title check | |
The title is concise, specific, and accurately summarizes the main change: repeating pending Initial and Handshake ACKs in every packet. |
| Docstring Coverage | |
Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 2 files. |
| Linked Issues check | |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
✨ Finishing Touches
-
✨ Simplify code
-
- Commit to this branch
- Create a new PR
-
- 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.
Comment @coderabbitai help to get the list of available commands.
|
Codex Review: Didn't find any major issues. What shall we delve into next? 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". |
1 participant
Follows #32 and #34, the last two handshake fixes for the same moq-dev/moq drill.
Problem
noq writes ACK frames only while
PendingAcks::can_send()holds, and that is false once an ACK was sent and no new ack-eliciting packet arrived. If that ACK is lost, the receiver's next packets in the space (its PTO probes, for one) carry no ACK, so the sender learns nothing until its own backed-off PTO resends the flight and a fresh ACK gets through.That stalls a handshake whose flights span two datagrams on a path that cannot take both back to back. moq-dev/moq's impaired cluster drill shapes to 100 kbit/s with a 2125-byte bucket, and the hybrid X25519MLKEM768 key share makes each side's first flight 2 x 1200 bytes, so the second datagram of every flight and every two-packet probe overflows. A connect only progresses when an ACK tells the sender which half arrived. At
MOQ_SHAPER_SEED=13876846873975217772, 5% random loss drops the client's three ACKs of the server's ServerHello head (+0.2s, +1.2s, +4.2s). The client's Initial probes reach the server in between, but without an ACK, so the server keeps resending head and tail together, the tail keeps overflowing, and the 10s connect budget runs out. It fails 3 of 3 runs on moqmain.Fix
In the Initial and Handshake spaces, every packet now repeats the pending ACK ranges, not just when
can_send()holds. These spaces carry a handful of packets, so the extra ACK frames cost nothing that matters. The Data space is unchanged.quinn already does this, more broadly: quinn-rs/quinn#2747 bundles pending ACK ranges into any packet carrying other frames, in every space, paced by
next_bundled_ack_time. noq forked before it. This takes only the handshake half, without the pacing, so 1-RTT ACK behavior (and multipathPATH_ACK) is untouched. moq-dev/moq's own quinn fork,moq-quic, passes the same test unchanged.Not changed
When an ACK resets
pto_countafter a long backoff, the Handshake PTO deadline is already in the past, andhandle_timeout'swhile let ... expire_before(now)loop refires it until the backoff passesnow. The drill's server fired six times in one instant and queued 12 probes. quinn'shandle_timeoutfires each timer at most once per call, and the probes it sends move the deadline forward, somoq-quicdoes not cascade. This PR leaves it alone, since the ACK fix alone makes the drill pass and the switch tomoq-quicdrops the loop.Tests
lost_handshake_ack_repeats_in_next_packet: only the head of the server's first flight arrives, the client's ACK of it is lost, and the client's next Handshake packet (its PTO probe) must carry an ACK. Without the fix the probe is a bare PING and the server counts 0 new ACK frames; with it, 1.cargo test -p moq-noq-proto --lib: 495 passed.cargo test -p moq-noq -p web-transport-moq: passed.cargo clippy -p moq-noq-proto --tests: clean. The CI rustfmt check passes.bursts_cross_a_cluster::impairedat the failing seed passes 3 of 3 runs in 6.3 to 6.8s, and 50 of 50 random seeds pass (deps: bump moq-noq for the lost handshake ACK fix moq#5457).API: none. Wire: no new frames; Initial and Handshake packets may carry an ACK frame they previously omitted.
(Written by Claude Opus 5.5)
🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmith-botwith what you need.