Stop retrying request timeouts by default (lib-httpx 2.5.0) [release]

[pditommaso]

Stacked on #114 — base is fix/hxclient-config-lossless-copy. Review/merge #114 first; the diff shown here is only this layer.

Implements the second half of #113's Impact section, which #114 deliberately left out of scope.

The problem

HttpTimeoutException extends IOException, so the default retry condition (t instanceof IOException) retried request timeouts. The real bound on a call was therefore maxAttempts × timeout, not timeout — with the default of 5 attempts, a 2 minute timeout was a 10 minute worst case. Verified on the pre-change code: a request that times out is contacted 3 of 3 attempts.

Retrying such a timeout is also ambiguous — the server may have received the request and still be working on it, so re-sending can run a non-idempotent operation twice.

The change

HxConfig.defaultRetryCondition(Throwable) now retries any IOException except an HttpTimeoutException that is not an HttpConnectTimeoutException:

if (!(throwable instanceof IOException))
    return false;
if (throwable instanceof HttpTimeoutException)
    // Retry only a connect timeout. It is raised while establishing the connection, so the
    // request never reached the server: nothing ran, re-sending cannot duplicate anything...
    return throwable instanceof HttpConnectTimeoutException;
return true;

A connect timeout is raised before the request is sent, so re-sending is safe and may reach a healthy endpoint — it stays retryable. The rule lives in one named method that HxClient.shouldRetryOnException also uses as its fallback when the configured condition is null, so the documented default and the fallback cannot drift.

Why this shape — how other clients default

Surveyed from source, not docs:

Client	Connect-phase timeout	Post-send timeout
OkHttp	retried (isRecoverable: e is SocketTimeoutException && !requestSendStarted)	not retried since 3.3.0
Apache HttpClient 5	not retried (ConnectException non-retriable)	not retried — InterruptedIOException is in the default non-retriable list and the check uses isInstance, so SocketTimeoutException is covered
JDK java.net.http	retried, idempotent methods only	not retried
AWS SDK v2	retried (IOException)	retried, but ApiCallTimeoutException (whole call) is deliberately not retryable — an overall budget terminates the sequence
gRPC	per policy	deadline is absolute and shared across attempts, so retries cannot extend the total

Two coherent designs exist — exclude post-send timeouts (OkHttp, Apache, JDK), or retry them under an absolute overall budget (AWS, gRPC). lib-httpx did neither. This PR adopts the first, which is the narrower change; the budget option is filed as #115.

Release: both layers ship as 2.5.0

This change is folded into 2.5.0 together with #114 — VERSION stays at 2.5.0 here and the changelog entries join that section, led by BEHAVIOUR CHANGE.

That requires the two PRs to publish once, from this one. publish.sh skips any version already in the repo, so if #114 merged with a [release] marker it would publish 2.5.0 without this change, and this change could then never be published under 2.5.0. To avoid that, the [release] marker lives only on this PR: merge #114 first (no publish), then this one (publishes 2.5.0 with both layers).

It is still a behaviour change — a call that previously succeeded on a retry after a timeout now fails on the first one. That's consistent with how this repo has treated retry-behaviour changes before (2.3.0 changed retry timing by honouring Retry-After), but if you'd rather it be a major, both the version and the marker are one-line changes.

No consumer in platform, nextflow, sched or wave sets a retryCondition, so all of them inherit the new default when they upgrade. That is the intent, but it is worth knowing: their timeout-heavy calls (nf-tower, nf-wave, the registry and plugin-repo clients, sched-client, platform-client) will stop burning attempts on timeouts.

Opting back in is one line:

.retryCondition(t -> t instanceof IOException)   // includes HttpTimeoutException

Tests

• HxConfigTest — a where-driven test over HxConfig.defaultRetryCondition: 10 cases covering IOException, ConnectException, SocketTimeoutException, FileNotFoundException, HttpConnectTimeoutException (all retried), HttpTimeoutException (not retried) and four non-I/O throwables. Each case also asserts the condition wired into a default HxConfig agrees with the static rule.
• HxClientRetryIntegrationTest — WireMock pair: a request timeout under the default condition contacts the upstream once; with retryCondition(t -> t instanceof IOException) it contacts it 3 times. The first fails against the pre-change code, the second passes on both — together they pin the change and its escape hatch.
• ./gradlew :lib-httpx:build :lib-cloudinfo:test green.

Docs

README gains a "Which failures are retried" section explaining the default, the HttpTimeoutException extends IOException trap, the OkHttp/Apache precedent, and how to opt back in. It also notes that retries are not gated on request method — a POST is retried like a GET — which is filed separately as #115.

🤖 Generated with Claude Code

[pditommaso]

@claude review

[claude]

Claude finished @pditommaso's task in 3m 34s —— View job

---

Review — Stop retrying request timeouts by default (lib-httpx 2.5.0)

• Read the trigger comment and repo guidelines (CLAUDE.md)
• Diff this layer only (06bf209..HEAD, i.e. vs fix/hxclient-config-lossless-copy)
• Verify the new rule's logic and JDK exception hierarchy assumptions
• Check the default / fallback cannot drift (HxConfig field, Builder field, HxClient.shouldRetryOnException)
• Check tests and docs
• Check release mechanics (VERSION, changelog.txt, README version)
• Post review

---

Verdict: looks correct — approve. The change is surgical (one root cause, one named rule), tested from both sides, and the escape hatch is documented. Everything below is a nit, none of it blocking.

What I verified

The rule itself is right. HttpConnectTimeoutException extends HttpTimeoutException extends IOException, so the ordering in HxConfig.defaultRetryCondition (HxConfig.java:100-113) — reject non-IOException, then special-case HttpTimeoutException and allow only the connect subclass, then allow the rest — covers the hierarchy correctly. SocketTimeoutException, ConnectException, FileNotFoundException and plain IOException all stay retryable, as the where: block asserts.

No drift between "the documented default" and "the fallback". All three sites now resolve to the same method:

• HxConfig.java:77 — DEFAULT_RETRY_COND = HxConfig::defaultRetryCondition
• HxConfig.java:121 and HxConfig.java:265 — both the config field and the Builder field initialise from DEFAULT_RETRY_COND
• HxClient.java:629 — the condition == null fallback now calls HxConfig.defaultRetryCondition instead of an inline duplicate

That was the actual latent hazard here (two copies of the rule), and it's closed. The method-ref assignment to Predicate<? extends Throwable> is fine — the ground target type is Predicate<Throwable>, same as the lambda it replaces.

The exception reaches the condition unwrapped. Worth confirming since the sync path goes through sendAsync(...).get(): HxClient.java:597-606 unwraps ExecutionException and rethrows an IOException cause as-is, so a real HttpTimeoutException (not a CompletionException) is what the predicate sees. The integration tests corroborate this — 1 contact by default, 3 with the opt-in.

Nits

1. defaultRetryCondition is package-private, but the changelog advertises it as new API. HxConfig.java:100 is static boolean (no modifier), yet changelog.txt says "Add HxConfig.defaultRetryCondition(Throwable)" — a consumer outside io.seqera.http cannot call it. Two knock-on effects: composing with the default is impossible (t -> HxConfig.defaultRetryCondition(t) || t instanceof MyException won't compile for callers), and the {@link #defaultRetryCondition(Throwable)} references from the public class javadoc (HxConfig.java:48) and from HxClient.java:617 won't resolve for external readers of the generated docs. Making it public seems more in line with what the changelog and README promise. Fix this →

2. Stale javadoc bullets still say the old rule. HxClient.java:63 ("Network errors (IOException)"), and HxClient.java:259, :381, :404 ("Retry on IOException (network errors)"). The one on shouldRetryOnException was updated; these four public-facing ones weren't, so the class-level and per-send/sendAsync docs now contradict the new default. Fix this →

3. The README's headline config example now demonstrates the opt-out. README.md:262-263 changed the illustrative value from the narrowing predicate to .retryCondition(t -> t instanceof IOException). That's the opt back in to the old behaviour line — someone copy-pasting the "Custom Retry Configuration" block silently reverts this fix without noticing. Either drop the line from that example (the default is now the good one, so the example doesn't need it) or make it visibly a widening choice. Fix this →

4. The Apache precedent is stated slightly wider than the rule implements. README.md (and the javadoc) say Apache HttpClient 5 "treats the whole InterruptedIOException family as non-retriable" — but the new default still retries SocketTimeoutException, which is an InterruptedIOException, and the test asserts that (HxConfigTest, new SocketTimeoutException(...) || true). In practice java.net.http surfaces timeouts as HttpTimeoutException, not SocketTimeoutException, so this is unreachable rather than wrong — but as written the cited precedent looks broader than what the code does. A half-sentence ("we scope the exclusion to the HttpTimeoutException the JDK client actually raises") would remove the mismatch.

5. Test timing is tight but should hold. 150 ms request timeout against a withFixedDelay(1000) stub is a 6.7× margin, and the opt-in test parks three WireMock handler threads for ~1 s each. Low risk, but if these ever flake on a loaded runner the margin is the first thing to widen. Note I could not run ./gradlew :lib-httpx:test in this environment (Gradle invocation wasn't permitted), so I'm reading the tests, not their results — CI is the source of truth for the "both layers green" claim.

Behaviour change / release mechanics

The [release]-only-on-this-PR reasoning is sound and matches publish.sh's skip-if-present semantics: if #114 merged carrying the marker, 2.5.0 would publish without this layer and could never be republished under that version. Worth making sure the marker really is absent from #114's title at merge time — that's the one step of this plan that isn't enforced by code.

On minor-vs-major: I'd keep 2.5.0. The changelog leads with BEHAVIOUR CHANGE, the public surface is unchanged (nothing removed, renamed or repurposed), the escape hatch is one line, and the repo set this precedent in 2.3.0 with the Retry-After timing change. The consumer-impact note is honest — timeout-heavy callers in platform/nextflow/sched/wave will start failing at timeout instead of at 5 × timeout, which is the point.

One thing the doc gets right that's easy to get wrong: retries are not gated on request method, so the reason for excluding post-send timeouts (a non-idempotent POST could run twice) is the honest one, and #115 is the right home for the remaining half.
• branch fix/default-retry-condition-timeouts

[claude]

Approving — the new default retry rule is correct against the JDK exception hierarchy (HttpConnectTimeoutException extends HttpTimeoutException extends IOException), and the rule now lives in one place used by both HxConfig's default and HxClient.shouldRetryOnException's null fallback, so the documented default and the fallback can't drift. The sync path unwraps ExecutionException (HxClient.java:597-606), so the predicate sees the real HttpTimeoutException — confirmed by the WireMock pair (1 contact by default, 3 with the opt-in).

Non-blocking nits, detailed in my comment on the PR:

1. HxConfig.defaultRetryCondition is package-private, but the changelog advertises it as added API and the public javadoc {@link}s it — consumers can't call or compose with it. Consider making it public.
2. Stale javadoc bullets still describe the old rule: HxClient.java:63, :259, :381, :404.
3. README.md:262-263 — the "Custom Retry Configuration" example now demonstrates .retryCondition(t -> t instanceof IOException), i.e. the opt-back-in; copy-pasting it silently reverts this fix.
4. The Apache precedent is stated as "the whole InterruptedIOException family", but SocketTimeoutException is still retried (unreachable via java.net.http, so not wrong — just wider than the rule).
5. 150ms-vs-1000ms test margins are fine but are the first thing to widen if they flake.

Keeping it at 2.5.0 seems right: no public surface removed or repurposed, changelog leads with BEHAVIOUR CHANGE, one-line escape hatch, and 2.3.0 set the precedent for retry-behaviour changes in a minor. The one step not enforced by code is the [release] marker staying off #114 at merge time.

I could not run ./gradlew :lib-httpx:test in this environment, so CI is the source of truth for the green claim.