Repository navigation
test(quic): characterize moq-quic behavior - #5781
Conversation
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>
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>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Outcome: all six areas of the quest are covered (69 tests), and 4 bugs are pinned as Open decisions, with my recommendation first:
(Written by Claude Opus 5.5) |
The ignored tests now point at quests that exist: the Retry bug in m0, since the shipped release has the same guard, and the other three in the moq-quic epic. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@codex review (Written by Claude Opus 5.5) |
|
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 12 seconds. View limit details
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughThe changes add QUIC characterization tests for endpoint handling, packet paths, frame validation, stream behavior, flow control, resets, and packet scheduling. They add a test-only packet-forging helper. New quest documents describe Retry handling and three QUIC behavior issues, and the required lists are updated. The previous characterization coverage plan and its links are removed. The quest documents describe proposed fixes; the changes do not include production fixes. Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation |
|---|---|---|
| Title check |  |
The title clearly and concisely describes the main change: adding QUIC behavior characterization tests for moq-quic. |
| Description check | |
The description is directly related to the changeset and explains the characterization tests, helper, identified bugs, test results, and scope. |
| Docstring Coverage | |
Docstring coverage is 86.01% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 143 functions across 9 files. (6 skipped: 6… |
| 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. |
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 86.01% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 143 functions across 9 files. (6 skipped: 6 unsupported.)
✨ Finishing Touches: 💡 1
-
⚔️ Resolve merge conflicts: 💡
-
- Resolve merge conflict in branch
quest/m1/quic/coverage/characterize
- Resolve merge conflict in branch
✨ 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 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. |
|
Codex Review: Didn't find any major issues. 🚀 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". |
|
Merge summary
Public API and wire: none. CI green; Codex review of fd1d2f9 found no issues. (Written by Claude Opus 5.5) |
# Conflicts: # quest/m1/quic/coverage/README.md # quest/m1/quic/coverage/characterize.md
# Conflicts: # quest/m0/README.md
![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: b85987f11d
ℹ️ 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".
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@codex review |
|
Codex Review: Didn't find any major issues. Nice work! 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: # quest/m1/quic/README.md
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
# Conflicts: # quest/m1/quic/coverage/README.md # quest/m1/quic/coverage/characterize.md # quest/m1/quic/coverage/sim.md
# Conflicts: # quest/m0/README.md
# Conflicts: # quest/m1/quic/coverage/README.md
#5770 schedules datagrams in their send group, so ungrouped datagrams now go before equal-priority stream data, and one that does not fit ends the packet instead of letting streams fill it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Implements
quest/m1/quic/coverage/characterize.md(deleted here) from the moq-quic regression net epic.What lands
69 characterization tests in a new
rs/moq-quic/src/tests/characterize/module, wired in with one line intests/mod.rs. Every pair pins Cubic and deterministic packet numbers explicitly, so the harness flipping its default to BBR3 (sim.md) does not churn them.send_fairness, a retransmit behind a higher-priority stream, a retransmit within a cwnd-limited stream, and datagrams interleaved with streams. feat(quic)!: hierarchical send groups, one per subscription #5711 should update each golden in its own diff.RecvStream::received_reset, reset codes across the varint range,Chunksending in Reset or Finished, stream credit returning once a reset is observed, and connection credit returned on reset, stop, and 0-RTT rejection.set_max_concurrent_streams,set_receive_window, andset_send_windowraised and lowered mid-connection, plus the 0-RTT window raise.cids_exhaustedat connect, at the Initial, and at accept.PathData::from_previous(with the IPv6 counterpart, which resets).Frame error paths need a peer that misbehaves, so
Connectiongains one#[cfg(test)]helper,forge_packet, beside the existingdecode_packet: it encrypts an arbitrary payload as the connection's next packet in a space.Bugs found
Each one lands as an
#[ignore]d test asserting the correct behavior, naming the quest this PR adds for it (three inquest/m1/quic/, the Retry bug inquest/m0/because the shipped release has the same guard and gets a backport):/quest/m1/quic/final-size-below-received.md: a FIN below data already received is accepted. The reader gets the bytes past the final size, thenBlockedforever; the stream never finishes or errors./quest/m1/quic/close-reason-after-reset.md: after a connection closes on its own transport error, a stateless reset in theClosedstate replaces the unread close reason withReset, or reports a secondConnectionLostif the reason was already read. The fix forDraining(stateless_reset_while_draining_*) does not coverClosed./quest/m1/quic/zero-rtt-window-raise.md: when a server raises its stream receive window between sessions, the client's 0-RTT streams keep the remembered smaller window. The server, believing it granted the larger one, never sends MAX_STREAM_DATA, so the stream stalls forever (bidi and uni).StreamsState::set_paramsraises only peer-initiated bidi streams, which cannot exist before the peer's parameters arrive./quest/m0/retry-after-invalid-retry.md: a Retry the client discards (bad integrity tag, or a SCID equal to its original DCID) still bumpstotal_authed_packets, so the client also discards the server's genuine Retry of its retransmitted Initial and never connects. One injected or corrupted Retry kills the handshake.Pinned, not flagged
API and wire impact
None. The only non-test change is the
#[cfg(test)]Connection::forge_packet.Checks
just checkpasses. The characterization suite passed 5 runs in a row with no golden drift. The ignored tests fail as expected with--run-ignored all.(Written by Claude Opus 5.5)
🤖 Generated with Claude Code