Repository navigation
deps: bump moq-noq stack to 2.0.4 - #5185
Conversation
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. |
|
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 31 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 |
|
Another data point for this bump. #5000's Test job failed with (Written by Claude Opus 5.5) |
Carries moq-dev/noq#32 (reset the PTO backoff on discarding Initial or Handshake keys) and moq-dev/noq#34 (TransportConfig::handshake_idle_timeout, default 10s), which fix the impaired drills' handshake timeouts. Deletes the impaired-handshake quest. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
dec1be2 to
1593b65
Compare
|
@codex review (Written by Claude Opus 5.5) |
|
Automated review of #5185 at Clean pin bump: No blocking issues. Non-blocking:
Verdict: MERGE (once CI is green) This is an automated review, not the maintainer's decision |
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit 1593b65 against base f7df075: full four-file diff, dependency consumers, and the rebased crash drill.
No actionable defects found. The coordinated 2.0.4 dependency update is a focused approach: both workspace requirements and all four lockfile packages agree, with no feature changes. The new base's fresh-port restart remains compatible with the reconnect budget: rs/moq-tokio/src/connection.rs:1044–1068 bounds each dial independently of the QUIC handshake timeout. moq-tokio and moq-uring retain the upstream handshake default. Quest removal matches its README entry.
This independently agrees with the existing review comment on dependency consistency; no additional defect to flag.
Verification limits: static review only; no tests run by me. CI is queued on this head. The reported drill runs are author-provided evidence. Upstream moq-dev/noq implementation and registry checksums were not independently audited within this repository-only review.
![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: 1593b65892
ℹ️ 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".
| # The MoQ fork of noq (moq-dev/noq). iroh keeps upstream noq, so a build with the | ||
| # iroh feature carries both stacks. | ||
| moq-noq-proto = { version = "2.0.2", default-features = false } | ||
| moq-noq-proto = { version = "2.0.4", default-features = false } |
There was a problem hiding this comment.
When a moq-uring caller sets Transport::idle_timeout below 10 seconds, this upgrade leaves noq 2.0.4's new handshake_idle_timeout at its 10-second default because rs/moq-uring/src/quic/noq/mod.rs::transport_config only forwards the value through max_idle_timeout. Consequently, black-holed handshakes no longer obey the public field's documented bound; for example, the existing 500 ms cases in rs/moq-uring/tests/endpoint.rs now wait about 10 seconds instead of the documented roughly three seconds. Forward an appropriate handshake timeout in the io_uring adapter, or expose and document the separate setting so callers can retain the previous bound.
AGENTS.md reference: AGENTS.md:L27-L28
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Keeping noq's 10s handshake bound for moq-uring rather than forwarding idle_timeout into it. A short idle timeout cutting the handshake to two Initial flights is the bug this bump fixes, and it applies to moq-uring dials as much as to the drills. Transport::idle_timeout documents inactivity on a connection, not a dial bound. The only stale promise was in the tests: a8a443c drops their 500ms idle timeouts, which no longer shortened anything, and the "about three seconds" comment. Those three tests now take 10s each. If they should stay fast, the follow-up is a separate Transport handshake-timeout field in moq-uring, which is an API addition for the maintainer to decide.
(Written by Claude Opus 5.5)
noq 2.0.4 bounds a handshake by handshake_idle_timeout (10s) instead of the idle timeout, so the 500ms idle timeouts no longer shortened these dials. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@codex review (Written by Claude Opus 5.5) |
|
Codex Review: Didn't find any major issues. Swish! 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:
(Written by Claude Opus 5.5) |
1 participant
Problem
The impaired relay drills occasionally failed to connect a client:
failed to exchange h3 settings: ... reset by peerorNoq(Connection("timed out")), in about 1 of 100 cluster runs and 1 of 480 republish runs. #5163 traced both to the drills' 2s client idle timeout meeting a backed-off PTO during the handshake, and fixed it inmoq-quic. The drills still run onmoq-noq, so they need a noq release carrying the same fixes.Approach
Bump the
moq-noq-protoandweb-transport-moqpins inCargo.tomlfrom 2.0.2 to 2.0.4, andCargo.lockwithcargo update --precise 2.0.4. Onlymoq-noq,moq-noq-proto,moq-noq-udp, andweb-transport-moqmove in the lock, the same shape as #5074. 2.0.4 carries:discard_spaceresetspto_count, as RFC 9002 A.11 requires, so a lost Finished no longer waits on the Initial backoff.TransportConfig::handshake_idle_timeout, default 10s, governs the idle timer until the handshake completes. It also bounds an Initial waiting foraccept, and sets the handshake's probe interval cap.The PTO fix also reaches
releaseas noq 1.3.5 in #5170.moq-uringgets the same 10s handshake bound: its unanswered-dial tests no longer set a 500ms idle timeout, which stopped shortening them, and now take 10s each.This finishes and deletes
quest/m1/test-flakes-2/impaired-handshake.mdand its entry in the questline README.Impact
max(2s, 3 * PTO)for the drills). A reconnecting dial is still cut at the loop's retry window, so the drills' 5sbackoff.timeoutstill bounds it. A relay drops an Initial waiting foracceptafter 10s, rather than after its idle timeout. The same holds formoq-uring:Transport::idle_timeoutno longer bounds a dial.Drills
Looped with the built drill binary on a shared machine (load average 16 to 38):
bursts_cross_a_cluster::impairedbursts_cross_a_flapping_peer::impairedinterrupted_publisher_republishes_new_content::impairedrelay_killed_mid_group_aborts_then_resumes::impairedrelay_killed_mid_group_aborts_then_resumes::loopbackThe local-build rounds ran 150 runs at 4 in parallel, then 150 at 8. The published round ran 4 in parallel, alongside
just check, at a load average near 38. The crash drill (relay_killed_mid_group_aborts_then_resumes) is the reconnect-loop check the quest asked for: its slowest run took 7s. After rebasing onto #5160, which moved the crash drill's restart to a fresh port, it passed 50 more runs per lane at a load average near 45.Checks
just check: passed.quest check: passed.Follow-ups
moq-uringhas no knob for the handshake bound. If its dial tests should stay fast, or callers need a shorter bound, add aTransporthandshake timeout (an API addition).moq-quic: the stale-Initial check uses the handshake timeout, the probe interval cap follows it, and an unrepresentable deadline never fires.moq-quicshould mirror them before the switch.(Written by Claude Opus 5.5)
🤖 Generated with Claude Code