Cap connection-failure retries at 21 attempts - #9729
Open
adityaanikam wants to merge 2 commits into
Open
Conversation
RetryAndFollowUpInterceptor's connection-failure retry loop had no cap, unlike its sibling follow-up loop (redirects and auth challenges), which already gives up after MAX_FOLLOW_UPS. A route selector that keeps reporting a usable route available, whether because routes genuinely keep cycling back or because the same route is retried many times before ever exhausting, let recover() retry indefinitely. In a production incident, two threads accumulated tens of thousands of recovered failures each on a single stuck call, growing recoveredFailures without bound, while both held a per-connection Http2Writer monitor and contended for Okio's global AsyncTimeout lock. Because the calls never returned, the timeout mechanism meant to cancel them needed that same contended lock to update its own scheduling state, so cancellation stalled too. Added MAX_RECOVERED_FAILURES, mirroring MAX_FOLLOW_UPS's value and placement, and thrown the same way an unrecoverable failure already is. This bounds how long any single call can spend retrying and contending for locks a stuck attempt is still holding, regardless of how the route selector behaves. Added a regression test providing more usable routes (25) than the cap allows, and asserting the exact request count rather than only that some IOException is eventually thrown: without the fix, the loop consumes routes until exhaustion (25 requests); with it, the cap fires at 21. Verified locally by reverting only the interceptor change and confirming the test fails with exactly that 21-vs-25 mismatch, then confirmed the full CallTest suite (217 tests) still passes with the fix applied. Fixes lysine-dev#9727
swankjesse
requested changes
Aug 26, 2026
| call.eventListener.retryDecision(call, e, isRecoverable) | ||
| if (!isRecoverable) throw e.withSuppressed(recoveredFailures) | ||
| recoveredFailures += e | ||
| if (recoveredFailures.size > MAX_RECOVERED_FAILURES) throw e.withSuppressed(recoveredFailures) |
Collaborator
There was a problem hiding this comment.
Swap this line with the one above? Otherwise e is in its own suppressed list.
Appending the current exception to recoveredFailures before checking the cap meant the thrown exception, once the cap fired, was included in its own suppressed list. Throwable.addSuppressed() throws IllegalArgumentException on self-suppression, so the cap could crash with the wrong exception type instead of surfacing the intended IOException. Swap the order: check the cap against the failures accumulated so far, then append. This also shifts the boundary by one request (22 total instead of 21), since the failure that trips the cap is no longer counted before the check; updated the test and its comment to match, and reverified with a negative control that the old ordering now fails against the corrected expectation.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #9727
RetryAndFollowUpInterceptor's connection failure retry loop had no cap, unlike its sibling follow up loop (redirects and auth challenges), which already gives up after MAX_FOLLOW_UPS. A route selector that keeps reporting a usable route available let recover() retry indefinitely.
In a production incident, two threads accumulated tens of thousands of recovered failures each on a single stuck call, growing recoveredFailures without bound, while both held a per connection Http2Writer monitor and contended for Okio's global AsyncTimeout lock. Because the calls never returned, the timeout mechanism meant to cancel them needed that same contended lock to update its own scheduling state, so cancellation stalled too. Full details and thread dump analysis are in the issue.
Added MAX_RECOVERED_FAILURES, mirroring MAX_FOLLOW_UPS's value and placement, and thrown the same way an unrecoverable failure already is. This bounds how long any single call can spend retrying and contending for locks a stuck attempt is still holding, regardless of how the route selector behaves.
Added a regression test providing more usable routes (25) than the cap allows, and asserting the exact request count rather than only that some IOException is eventually thrown: without the fix, the loop consumes routes until exhaustion (25 requests); with it, the cap fires at 21. Verified locally by reverting only the interceptor change and confirming the test fails with exactly that 21 vs 25 mismatch, then confirmed the full CallTest suite (217 tests) still passes with the fix applied.
I am in the process of signing the Individual Contributor License Agreement per CONTRIBUTING.md.