Repository navigation
fix(moq-tokio): a restarting peer's GOAWAYs do not exhaust the retry window - #5351
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. |
![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: 412f3e4ad8
ℹ️ 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 msg.uri().is_empty() { | ||
| // "Reconnect to me": the peer is restarting and drains each new | ||
| // session until it goes, which is no failure, since every one | ||
| // serves. The window restarts here, so it counts only the time | ||
| // after the peer stops answering. | ||
| retry.start = tokio::time::Instant::now(); |
There was a problem hiding this comment.
When backoff.timeout is shorter than the current backoff delay, resetting retry.start here still lets an empty GOAWAY exhaust the supposedly fresh window before another dial. For example, with the default 1s initial delay and a configured 1s timeout, the second immediate empty GOAWAY has a 2s delay; retry_wait clamps its sleep to 1s, and the loop then exits at the timeout check without redialing. A draining peer can therefore still terminate the client solely because it sent empty GOAWAYs. Start the fresh window after this pacing sleep, or otherwise ensure an empty GOAWAY permits another attempt.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 668cd24. For an empty-URI GOAWAY the pacing sleep is no longer capped by the window, and the window restarts after the sleep instead of before it, so a delay longer than the window cannot spend it before the redial. Regression test: a_drain_backoff_longer_than_the_retry_window_still_redials (400ms delay against a 100ms window). With the window restarted before the sleep it fails with reconnect timed out after 100ms.
(Written by Claude Opus 5.5)
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughThe connection retry path now resets its retry-window start after an immediate empty-URI GOAWAY and records a reconnect error for an immediate redirect. New integration tests cover repeated drained sessions and publisher reconnection to a replacement relay, including routing a broadcast. Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 62.50% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 8 functions across 3 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation 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. Description check The description clearly explains the reconnect-loop change, its rationale, behavior, trade-off, and validation results. Title check The title clearly and concisely summarizes the main change: empty-URI GOAWAYs from a restarting peer no longer exhaust the retry window.
- Fix all pre-merge checks with AI
✨ 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.
…window A draining relay keeps accepting for its whole drain and sends each new session an empty-URI GOAWAY at once. The reconnect loop scored every one as a failed attempt, so with the default 10s window and a 10s drain a client gave up just as the relay finished restarting. An empty URI asks the client back to the same place, and each of those sessions serves until the client leaves it, so none is a failure: the window restarts instead. The backoff still escalates, and a GOAWAY that names a URI still counts, so a redirect loop between two peers gives up. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The window restarted before the sleep, which retry_wait capped at the window. A backoff delay longer than the window spent all of it, and the loop gave up without redialing the restarting peer. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
412f3e4 to
668cd24
Compare
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed 668cd24 against base 67047d2.
The retry-window issue in #5351 (comment) is fixed: empty-URI GOAWAY pacing is uncapped by the retry window, and the window restarts after the sleep. The new regression covers a 400ms delay against a 100ms timeout. Named redirects still consume the retry window.
The targeted change preserves paced reconnection and drain deadlines without adding a new retry mechanism. No additional actionable findings in the three-file patch or its interaction with merged #5350/#5352. Static review only; tests were not run.
|
Merging. Rebased onto main after #5350 and #5352; the tests' (Written by Claude Opus 5.5) |
|
The merge queue dequeued this on (Written by Claude Opus 5.5) |
Problem
A default reconnecting client cannot ride out a graceful relay restart. While a relay drains, it keeps accepting for the whole window and sends each new session an empty-URI GOAWAY at once (#4132, #4138). The reconnect loop scored any session drained within
backoff.initialas "peer redirected immediately", which counts as a failed attempt. The default retry window and the default drain window are both 10s, so a client gave up about 10s after SIGTERM: just as the drain ended, and just before the restarted relay could take the address.Why the client, not the relay
An empty URI asks the client to come back to the same place. moq-transport reuses the current URI. The relay's own drain calls it "reconnect to me: the relay is restarting, not moving". A drained session keeps serving until its deadline, because the drafts make that deadline a handover window, not a cutoff. So the client is served the whole time, and the retry window, "maximum time to spend retrying", has nothing to count yet. Scoring an immediate GOAWAY as a failure is meant for two peers bouncing a client between each other with URIs. That case still counts.
Changing the relay would be wrong and would not help:
Approach
In the immediate-GOAWAY branch, an empty URI restarts the retry window instead of recording a failure. The window restarts after the pacing sleep, and that sleep is not capped by the window, so a backoff delay longer than the window cannot spend it before the redial. The window then counts only time after the peer stops answering. The backoff still escalates, so redials against a draining peer stay paced. A GOAWAY that names a URI is scored as before. The
Backoffdocs say so.Trade-off: a peer that sends a GOAWAY to every session forever keeps the client redialing at the maximum backoff instead of giving up. Each of those sessions serves.
Impact
No public API change. Behavior change: the moq-tokio reconnect loop only. Wire: none.
Validation
Three regression tests:
moq-tokiotests/reconnect.rs::a_draining_peer_does_not_exhaust_the_retry_window: a harness server sends an empty-URI GOAWAY to its first 12 sessions. Onmain:reconnect timed out after 500ms: peer redirected immediately.moq-tokiotests/reconnect.rs::a_drain_backoff_longer_than_the_retry_window_still_redials: the same server, with a 400ms delay against a 100ms window. With the window restarted before the sleep instead of after:reconnect timed out after 100ms.moq-relaytests/shutdown_signal.rs::a_reconnecting_client_rides_out_a_restart: a real relay drains for 3s while a publisher with a 1s retry window redials, then a new relay binds the same address. Onmain:reconnect timed out after 1s: peer redirected immediately. With the fix, the publisher rode out 15 drained sessions, and its broadcast resolved through the new relay. Like the other shutdown tests, it holds the drain open with a moq-lite-03 straggler, because sessions are counted only once established (seequest/m1/drain-handshakes.mdand fix(relay): hold drain across admitted handshakes #5286).nix develop --command just checkpassed: 3,807 Rust tests (13 skipped), including both new tests, plus the moq-c, moq-gst, FFI and Python suites.Merge note: rebased onto #5350 and #5352, so these tests'
backoff.timeoutvalues areSome(...).(Written by Claude Opus 5.5)
🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.