Repository navigation
fix(net): escalate the backoff when GOAWAY redirects loop between peers - #6115
Conversation
Two relays that each redirect to the other once a session outlives `delay.initial` bounced a Reload between them indefinitely: every hop looked healthy, so the immediate-redirect damping never applied and the backoff reset after each migration. Returning to an endpoint that already sent us elsewhere in the current run of migrations now goes through the backoff without the healthy reset, so the delay escalates to `max` and `delay.timeout` still applies. One-way moves and GOAWAYs back to the same place (a restart) are unaffected. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Signed-off-by: HangPlayerEngineer <hangplayerengineer@paperclip.blockcast.net>
…y.max A session that lived at least the capped delay already redials no faster than the backoff allows, so its GOAWAY starts a new chain. Without this, rolling restarts days apart add up to a loop, and with delay.timeout set they give up for good. Move the redirect-loop tests onto the manual clock, asserting each hop's wait against the jittered backoff instead of counting dials in a second of wall time, and fix their Time.Milli references broken by the merge. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughThe change adds a bounded Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 75.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 3 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Title check The title clearly and concisely describes the main change: escalating backoff when GOAWAY redirects form a loop between peers. Description check The description directly explains the redirect-loop problem, implementation approach, behavior changes, and verification for the changeset. 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 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: 728164f8f5
ℹ️ 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".
| const next = (this.#redirect.peek() ?? url).href; | ||
| const moved = next !== dialing.href; | ||
| const loop = moved && this.#departed.has(next); |
There was a problem hiding this comment.
When relays issue fresh signed query credentials on each handoff, which Drain.uri explicitly supports in connection/goaway.ts, an A→B→A loop produces a different href on every visit. This exact-string lookup therefore never finds the previously departed relay, so sessions that live between delay.initial and delay.max continue cycling without backoff or timeout enforcement. Track a stable endpoint identity separately from the full URL used for dialing.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, fixed in 20992765f9. Endpoints are now compared without their query or fragment, so a relay that mints a fresh credential on every handoff still reads as the same endpoint. This also stops a same-endpoint GOAWAY that only refreshes the token from counting as a move. The A↔B escalation test now also runs with a new ?jwt= on every redirect, and fails if the query is kept.
(Written by Claude Opus 5.5)
| const next = (this.#redirect.peek() ?? url).href; | ||
| const moved = next !== dialing.href; | ||
| const loop = moved && this.#departed.has(next); | ||
| if (moved) this.#departed.add(dialing.href); |
There was a problem hiding this comment.
When a peer supplies an endless sequence of distinct accepted redirects and each session lives less than delay.max, every hop retains another attacker-controlled URL while none of the clearing conditions runs. Since each GOAWAY URI may be up to 8,192 bytes, a long-lived client can accumulate unbounded memory; cap this history or replace it with bounded cycle detection.
AGENTS.md reference: AGENTS.md:L18-L18
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, fixed in 20992765f9. The set of departed endpoints is now Brent's cycle detection: it keeps one endpoint, replaced at doubling intervals, so memory is constant however many distinct redirects the peer sends, and it still catches a cycle of any length. Once caught, a run stays a loop until it resets. A unit test covers cycles of 2 to 8 endpoints behind one-way tails, and a 1000-hop run that never repeats.
(Written by Claude Opus 5.5)
…ed memory Compare redirect endpoints without their query or fragment, so relays that mint a fresh credential on every handoff still read as the same endpoint when the loop comes back around. Replace the set of departed endpoints with Brent's cycle detection. The peer picks every hop, so the set grew by one attacker-chosen URL per redirect for as long as each session lived under delay.max. Once caught, a run stays a loop until it resets. 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: 20992765f9
ℹ️ 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".
| // never add up to a loop. | ||
| const lived = Instant.durationSince(now(), connected); | ||
| if (lived >= Nano.fromMilli(this.delay?.max ?? DEFAULT_DELAY.max)) this.#redirects.reset(); | ||
| const from = endpoint(dialing); |
There was a problem hiding this comment.
When a configured websocket.url differs from the primary URL and wins the transport race, dialing still names the unused primary even though #migrate correctly judges the GOAWAY against the WebSocket endpoint. For example, if fallback B explicitly redirects to B and the replacement then redirects to primary A, this records the first restart as A→B and incorrectly declares B→A a loop, imposing backoff on a sequence that never revisited a departed endpoint. Reuse the dialed(...) URL as from for loop tracking.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, fixed in 021d1548ab. Loop tracking now uses the dialed(...) URL as from. That alone would add the mirror false positive: a fallback that won the race and sent an empty GOAWAY would read as fallback→primary. So a hop now counts as a move only when this GOAWAY named a new redirect; an empty or ignored one reconnects in place. No Reload-level test, because the mock harness can't make a WebSocket fallback win the race yet.
(Written by Claude Opus 5.5)
When a WebSocket fallback wins the race, the session lives at the fallback URL, which `#migrate` already judges the GOAWAY against. Loop tracking used the primary instead, so a restart on the fallback followed by a redirect to the primary read as a return to a departed endpoint. Count a move only when this GOAWAY named a new redirect. An empty or ignored one reconnects in place, even when that reruns the race the fallback won, so it no longer reads as leaving the fallback for the primary. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…eers Mirrors #6115: while migrations are chained, a redirect back to an endpoint that already sent the client elsewhere skips the healthy-session reset, so the backoff escalates to its cap and the retry window ends the loop with a redirect-loop error. Brent's cycle detection keeps the state constant, endpoints compare without their query or fragment, and a session that outlived the capped delay starts a new run. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Supersedes #5959. The original work is by @kkroo; their commit is kept unchanged so authorship is preserved. This PR exists only because the fork is owned by an organization, so maintainers can't push to it.
Problem
Reloaddamps a GOAWAY redirect only when it arrives withindelay.initialof connecting. Two relays that each redirect to the other after a session outlives that delay bounce the client between them forever: every hop looks healthy, so the backoff resets after each migration and no error is ever reported.Approach (from moq-dev/moq#5959)
While migrations are chained,
Reloadtracks which endpoints sent it elsewhere. A redirect back to one of them goes through the backoff without the healthy-session reset, so the delay escalates todelay.maxanddelay.timeoutstill ends the sequence withredirect loop. The redirect itself is still honoured. A one-way rolling restart (A to B) and a GOAWAY back to the same place (a restart) never count as a loop. The chain clears when a session ends without a GOAWAY or a new sequence starts.Maintainer-side commits
main, fixing the test'sMilliimports that the merge broke.delay.max. Such a session already redials no faster than the backoff would allow, so rolling restarts days apart no longer add up to a loop (and, withdelay.timeoutset, a permanent give-up). This answers the follow-up question in moq-dev/moq#5959.Manualclock. Each hop's wait is asserted against the jittered backoff (half to all of the escalating delay) instead of counting dials in a second of wall time. Added a test that a loop no faster than the capped delay migrates immediately.Impact
ReloadDelaydoc comments change to describe the loop behaviour.delay.max, now waits out the jittered backoff before dialing. The draining session keeps serving during that wait, up to its handover cap.Follow-ups
Verification
just check: passes.(Written by Claude Opus 5.5)
🤖 Generated with Claude Code