Retry network errors in Kotlin through the idempotency gate - #539
Conversation
![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: fddd45d650
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c1df600897
ℹ️ 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 Kotlin transport surfaced every transport-level exception immediately as BasecampException.Network, so a single connection blip failed even a GET that would happily retry a 503. Route network failures through the same SPEC §7 eligibility gate as HTTP-status retries: naturally idempotent methods (GET/PUT/DELETE/HEAD) and idempotent-flagged POSTs retry with the shared backoff and attempt ceiling, while non-idempotent POSTs stay single-attempt. Modeled on the Swift directive loop (#517): the catch clause only classifies the attempt outcome; on_retry (with the failed/upcoming attempt pair), the backoff sleep, and the next attempt all run outside any catch, so cancellation propagates raw and no phantom events fire. Auth headers are rebuilt per attempt, so each retry re-authenticates. The deliberate carve-out is Ktor's HttpRequestTimeoutException: the caller's whole-request time budget is already spent when it surfaces, so retrying would extend latency past what they configured. Everything else the transport throws — connect/socket timeouts, connection resets, DNS failures including CIO's UnresolvedAddressException (not an IOException, which is why the gate is a carve-out rather than an allowlist) — retries. Closes the "Network error on an idempotent POST is retried then succeeds" conformance skip (red-first: Expected 2 requests, got 1) and flips the native GET network test from pinning single-attempt to pinning the retry, with new pins for body replay on a retried idempotent POST, attempt-budget exhaustion, the timeout carve-out, enableRetry=false, and the on_retry pair.
The HttpRequestTimeoutException carve-out's comments claimed the caller's whole-request allowance was "already spent", implying a shared deadline across attempts. HttpTimeout is installed on the client, so each attempt gets a fresh requestTimeoutMillis window. The carve-out is deliberate for a different reason: an attempt that consumed its entire time budget is a slowness shape a retry tends to repeat, and each retry burns another full budget, multiplying worst-case wall-clock time by the attempt count. Transient shapes (connect/socket timeouts, resets, DNS) still retry.
A throwing custom AuthStrategy was classified as a transport failure: the broad Exception catch converted it to a retryable Network error, re-driving the strategy through the full retry budget (3 auth calls for a default GET) and misreporting a configuration or credential-provider fault as a network error. Classify at the source instead: authenticateClassified tags an auth-phase throw as an internal AuthPhaseFailure, and both retry entry points unwrap it and rethrow the strategy's own exception raw on the first attempt — the wire is never reached and no retry budget is spent. Raw propagation matches the sibling SDKs (Swift authenticates outside the attempt's catch, Go returns the strategy's error before the attempt, TypeScript rethrows the original value so its identity survives) and this SDK's own service layer, which rethrows non-BasecampException exceptions raw. A BasecampException thrown by a strategy still propagates as-is.
c1df600 to
52ac1da
Compare
|
Merging on green CI: 38 checks pass (4 expected skips) on 52ac1da. Review status: Codex reviewed both earlier commits (its P2 threads are addressed and resolved — the timeout carve-out comment fix in the second commit, and the auth-phase classification fix with red proofs in the third) and posted no fresh round for the final push after a 10-minute window; Copilot errored on every attempt including the post-push re-run. Zero unresolved threads. |
Closes the last Kotlin retry-capability gap (wave 1, rock B3): transport-level network errors now retry through the same SPEC §7 idempotency gate as HTTP-status retries, modeled on the Swift directive loop (#517).
What changed
BasecampHttpClient.requestWithRetryclassifies each attempt into anAttemptOutcomeinside the catch, then acts on it outside any catch:onRetry(failed/upcoming attempt pair), backoff sleep, and the next attempt run at the tail, so aCancellationExceptionfrom the sleep propagates raw and no phantom events fire for an attempt that already ended.idempotent: true, bounded by the existingmin(caller cap, operation max)ceiling andenableRetry. Non-idempotent POSTs stay single-attempt.request()invokesauthStrategy.authenticatewhen building every attempt.authenticateClassifiedtags a throwingAuthStrategyas an internalAuthPhaseFailure, and both retry entry points unwrap it — the strategy's own exception propagates raw on the first attempt, the wire is never reached, and the strategy is never re-driven by retries. Matches Swift (authenticates outside the attempt's catch), Go (returns the strategy's error before the attempt), and TypeScript (rethrows the original value so its identity survives). ABasecampExceptionthrown by a strategy still propagates as-is.:439), the §7 Cross-SDK Kotlin sentence (:473), the two "(unlike Kotlin/TS)" Swift mentions (§7 + Appendix F), the Appendix F Kotlin row, the §19 roster line, thenetwork-retry.jsondescription, and a Kotlin README retry bullet.Verify-first findings
Body replayability. The request body is a
Stringparameter re-setBody()on every attempt, so replay after a thrown exception is structural — and now pinned:retriesIdempotentPostNetworkErrorWithFullBodyThroughGeneratedServicedrives the generatedmyAssignments.prioritizeAssignment(idempotent POST with a JSON body) through MockEngine throw-then-204 and asserts both attempts carry the identical full body{"id":123}.Exception taxonomy (Ktor 3.5.1, JVM target). Verified against the shipped jar:
io.ktor.client.plugins.HttpRequestTimeoutException extends java.io.IOException— whole-request time budget (requestTimeoutMillis); deliberately NOT retried — the timeout re-arms per attempt, so a retry burns another full budget on a slowness shape it tends to repeat (comments made honest about the per-attempt mechanics in the second commit, after Codex review).io.ktor.client.network.sockets.ConnectTimeoutException extends java.net.ConnectException— connect-phase, retried.java.net.*IOExceptions — retried.java.nio.channels.UnresolvedAddressException(CIO DNS failure) — extendsIllegalArgumentException, notIOException. This is why the gate is a carve-out (cause !is HttpRequestTimeoutException), not an IOException allowlist: an allowlist would silently drop DNS failures. Matches Swift's broad classification (everything non-BasecampErrorretries).CancellationException— rethrown raw, first catch clause (unchanged).UnresolvedAddressException, but the auth strategy is the SDK's own extension point, invoked at a known point, so its failures are tagged there and surfaced raw.Red proofs (pre-fix, verbatim)
Conformance, skip removed, before the fix:
Native
RetryTest, new/flipped tests before the fix (23 tests completed, 4 failed):The three pins that must NOT flip passed before and after the fix:
nonIdempotentPostNetworkErrorIsSingleAttempt,requestTimeoutExceptionIsNotRetried,enableRetryFalseDisablesNetworkRetry.Auth-phase red proofs —
authStrategyFailureIsNotRetriedagainst the un-fixed retry loop (network-retry commits without the third commit):and, with the auth-call assertion ordered first to expose the retry consumption:
The throwing strategy was driven 3 times through the retry budget and misreported as a retryable network error; with the fix it runs once, the wire is never reached (
requestCount == 0), and the caller sees their ownIllegalStateException.After the fix
:basecamp-sdk:jvmTest: BUILD SUCCESSFUL, 390 jvm testcases (all suites,authStrategyFailureIsNotRetriedincluded).make conformance-kotlin(post-rebase):Passed: 116, Failed: 0, Skipped: 8, Total: 124.scripts/check-retry-metadata-parity.py(post-rebase, includes Run the TypeScript retry loop beneath the middleware chain #538's TS runtime-consumption update): OK.make conformance-fixtures-check: ok.