From 6678e7b7f1c243ab867c6de91bdc743e866d1cd9 Mon Sep 17 00:00:00 2001 From: adityaanikam Date: Tue, 25 Aug 2026 02:39:59 +0530 Subject: [PATCH 1/2] =?UTF-8?q?=EF=BB=BFCap=20connection-failure=20retries?= =?UTF-8?q?=20at=2021=20attempts?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 #9727 --- .../http/RetryAndFollowUpInterceptor.kt | 9 ++++++ okhttp/src/jvmTest/kotlin/okhttp3/CallTest.kt | 31 +++++++++++++++++++ 2 files changed, 40 insertions(+) diff --git a/okhttp/src/commonJvmAndroid/kotlin/okhttp3/internal/http/RetryAndFollowUpInterceptor.kt b/okhttp/src/commonJvmAndroid/kotlin/okhttp3/internal/http/RetryAndFollowUpInterceptor.kt index ca03741b6ca4..0f43fe1b8d3a 100644 --- a/okhttp/src/commonJvmAndroid/kotlin/okhttp3/internal/http/RetryAndFollowUpInterceptor.kt +++ b/okhttp/src/commonJvmAndroid/kotlin/okhttp3/internal/http/RetryAndFollowUpInterceptor.kt @@ -77,6 +77,7 @@ class RetryAndFollowUpInterceptor : Interceptor { call.eventListener.retryDecision(call, e, isRecoverable) if (!isRecoverable) throw e.withSuppressed(recoveredFailures) recoveredFailures += e + if (recoveredFailures.size > MAX_RECOVERED_FAILURES) throw e.withSuppressed(recoveredFailures) newRoutePlanner = false continue } @@ -353,5 +354,13 @@ class RetryAndFollowUpInterceptor : Interceptor { * curl, and wget follow 20; Safari follows 16; and HTTP/1.0 recommends 5. */ private const val MAX_FOLLOW_UPS = 20 + + /** + * How many connection failures should we retry before giving up? Without a cap, a route + * selector that keeps yielding usable routes (or keeps cycling through an exhausted set) + * lets this loop retry indefinitely, growing [recoveredFailures] without bound and repeatedly + * contending for locks a stuck attempt is still holding. + */ + private const val MAX_RECOVERED_FAILURES = 20 } } diff --git a/okhttp/src/jvmTest/kotlin/okhttp3/CallTest.kt b/okhttp/src/jvmTest/kotlin/okhttp3/CallTest.kt index e00c97a4e8e1..d2827dc21932 100644 --- a/okhttp/src/jvmTest/kotlin/okhttp3/CallTest.kt +++ b/okhttp/src/jvmTest/kotlin/okhttp3/CallTest.kt @@ -1316,6 +1316,37 @@ open class CallTest { recoverWhenRetryOnConnectionFailureIsTrue() } + @Test + fun doesNotRetryConnectionFailuresUnboundedly() { + // Provide more usable routes (25) than the cap (20 retries + the original attempt = 21 + // total), mirroring DoubleInetAddressDns's technique of resolving the same real address + // repeatedly to guarantee fallback routes. Without the cap, recover() would keep finding a + // usable route and the call would consume closer to all 25 queued failures before route + // exhaustion finally stopped it. Asserting the exact request count, not just that some + // IOException was eventually thrown, is what actually proves the cap fired at 21 rather than + // route exhaustion coincidentally doing the same job. + val dispatcher = QueueDispatcher() + repeat(25) { + dispatcher.enqueue(MockResponse.Builder().onResponseStart(CloseSocket()).build()) + } + server.dispatcher = dispatcher + client = + client + .newBuilder() + .dns( + object : Dns { + override fun lookup(hostname: String): List { + val address = Dns.SYSTEM.lookup(hostname)[0] + return List(25) { address } + } + }, + ).build() + assertFailsWith { + client.newCall(Request.Builder().url(server.url("/")).build()).execute() + } + assertThat(server.requestCount).isEqualTo(21) + } + @Test fun noRecoverWhenRetryOnConnectionFailureIsFalse() { server.enqueue(MockResponse(body = "seed connection pool")) From ee4b6d38542e88ce41b9bf1bea2c342a5ca612b7 Mon Sep 17 00:00:00 2001 From: adityaanikam Date: Wed, 26 Aug 2026 17:13:02 +0530 Subject: [PATCH 2/2] Check the recovered-failures cap before appending the current failure 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. --- .../http/RetryAndFollowUpInterceptor.kt | 2 +- okhttp/src/jvmTest/kotlin/okhttp3/CallTest.kt | 18 ++++++++++-------- 2 files changed, 11 insertions(+), 9 deletions(-) diff --git a/okhttp/src/commonJvmAndroid/kotlin/okhttp3/internal/http/RetryAndFollowUpInterceptor.kt b/okhttp/src/commonJvmAndroid/kotlin/okhttp3/internal/http/RetryAndFollowUpInterceptor.kt index 0f43fe1b8d3a..4f8782041f9c 100644 --- a/okhttp/src/commonJvmAndroid/kotlin/okhttp3/internal/http/RetryAndFollowUpInterceptor.kt +++ b/okhttp/src/commonJvmAndroid/kotlin/okhttp3/internal/http/RetryAndFollowUpInterceptor.kt @@ -76,8 +76,8 @@ class RetryAndFollowUpInterceptor : Interceptor { val isRecoverable = recover(e, call, chain, request) call.eventListener.retryDecision(call, e, isRecoverable) if (!isRecoverable) throw e.withSuppressed(recoveredFailures) - recoveredFailures += e if (recoveredFailures.size > MAX_RECOVERED_FAILURES) throw e.withSuppressed(recoveredFailures) + recoveredFailures += e newRoutePlanner = false continue } diff --git a/okhttp/src/jvmTest/kotlin/okhttp3/CallTest.kt b/okhttp/src/jvmTest/kotlin/okhttp3/CallTest.kt index d2827dc21932..9029ef65b4b3 100644 --- a/okhttp/src/jvmTest/kotlin/okhttp3/CallTest.kt +++ b/okhttp/src/jvmTest/kotlin/okhttp3/CallTest.kt @@ -1318,13 +1318,15 @@ open class CallTest { @Test fun doesNotRetryConnectionFailuresUnboundedly() { - // Provide more usable routes (25) than the cap (20 retries + the original attempt = 21 - // total), mirroring DoubleInetAddressDns's technique of resolving the same real address - // repeatedly to guarantee fallback routes. Without the cap, recover() would keep finding a - // usable route and the call would consume closer to all 25 queued failures before route - // exhaustion finally stopped it. Asserting the exact request count, not just that some - // IOException was eventually thrown, is what actually proves the cap fired at 21 rather than - // route exhaustion coincidentally doing the same job. + // Provide more usable routes (25) than the cap allows. The cap check runs before the current + // failure is appended to recoveredFailures (so the thrown exception is never suppressed by + // itself), which means the throw fires one request later than MAX_RECOVERED_FAILURES retries + // plus the original attempt would suggest: 22, not 21. Mirrors DoubleInetAddressDns's + // technique of resolving the same real address repeatedly to guarantee fallback routes. + // Without the cap, recover() would keep finding a usable route and the call would consume + // closer to all 25 queued failures before route exhaustion finally stopped it. Asserting the + // exact request count, not just that some IOException was eventually thrown, is what actually + // proves the cap fired rather than route exhaustion coincidentally doing the same job. val dispatcher = QueueDispatcher() repeat(25) { dispatcher.enqueue(MockResponse.Builder().onResponseStart(CloseSocket()).build()) @@ -1344,7 +1346,7 @@ open class CallTest { assertFailsWith { client.newCall(Request.Builder().url(server.url("/")).build()).execute() } - assertThat(server.requestCount).isEqualTo(21) + assertThat(server.requestCount).isEqualTo(22) } @Test