Fix HxClient.Builder.config() dropping settings, and stop retrying request timeouts (lib-httpx 2.5.0)

[pditommaso]

Fixes #113 — both halves of it. #116 was merged into this branch (460af80), so this PR now carries the two layers that make up lib-httpx 2.5.0:

1. config(HxConfig) silently dropped settings — the reported bug.
2. The default retry condition retried request timeouts — the issue's Impact section.

---

1. config() dropped three settings

HxClient.Builder holds an HxConfig.Builder, so config(HxConfig) had to hand-transcribe a built config back into a builder. That transcription listed 15 of the 18 fields HxConfig.Builder.build() assigns, so retryCondition, tokenRefreshTimeout and refreshCookiePolicy reverted to their defaults — silently, and with no delegate on HxClient.Builder for the first two, making them unsettable on an HxClient.

The root cause is the hand-written field list, not those three fields, so this replaces the transcription rather than patching it:

• HxConfig.newBuilder(HxConfig) — a copy factory, backed by a private copyFrom() placed immediately above build() so the two field lists are adjacent and drift is visible in review. config() reduces to that copy (its two proxy assignments stay: build() writes them back onto the config builder on the no-explicit-client path).
• shouldRetryOnException(Throwable) is now wired. It was protected and documented as the retry-on-exception decision point, but sendWithRetry/sendWithRetryAsync passed config.getRetryCondition() straight to Retryable, so nothing outside tests called it. It now delegates to that condition — behaviour-identical for a default config — and overriding it takes effect.
• Javadoc on config() no longer claims the config "will be used directly" (the sentence the reporter trusted), and now names proxy(ProxySelector) / authenticator(Authenticator) explicitly, since config() replaces those too — .proxy(x).config(cfg) discards x when cfg carries no proxy. build()'s proxy paragraph records the propagation delta below.

One intended behaviour change: with config(cfg) and an explicit httpClient(...), proxy settings carried by the config now reach HxConfig and therefore the internal token-refresh clients, where previously they were dropped. The explicit HttpClient is still used verbatim.

2. Request timeouts are no longer retried by default

HttpTimeoutException extends IOException, so the default 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 against the pre-change code: a request that times out is contacted 3 of 3 attempts.

HxConfig.defaultRetryCondition(Throwable) — public, so callers can compose with it — now reads:

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. A post-send timeout is ambiguous: the server may have received the request and still be working on it, so re-sending risks running a non-idempotent operation twice. HxClient.shouldRetryOnException uses the same method as its null-condition fallback, so the documented default and the fallback cannot drift.

Opting back in is one line, and composing with the default is also supported:

.retryCondition(t -> t instanceof IOException)                                   // pre-2.5.0 behaviour
.retryCondition(t -> HxConfig.defaultRetryCondition(t) || t instanceof MyEx)     // default plus extra

How other clients default — surveyed from source

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, matched with isInstance
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

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 adopts the first, the narrower change; the budget option is filed as #115. Note the exclusion here is narrower than Apache's — scoped to the HttpTimeoutException that java.net.http actually raises, so SocketTimeoutException stays retryable.

---

Consumer impact

Surveyed every consumer available locally — platform, nextflow, sched, wave:

• No consumer calls HxClient.Builder.config(HxConfig) — its only callers are this repo's own tests. So layer 1 is behaviour-neutral for all of them; where it isn't (ffq-java-sdk), the change is the fix.
• No consumer sets retryCondition or tokenRefreshTimeout — the retryCondition matches in platform/nextflow are on Retryable and ThrottlingExecutor. So all of them do inherit the new timeout default: nf-tower, nf-wave, the registry and plugin-repo clients, sched-client and platform-client stop burning attempts on timeouts. That is the intent.
• Every refreshCookiePolicy caller uses the HxClient.Builder delegate (sched-client, sched-app, platform-client, nf-wave, nf-tower), a path neither layer touches.
• wave has no lib-httpx dependency at all.

Tests

• HxConfigRoundTripTest (new) — reflective: every declared HxConfig field survives newBuilder(HxConfig) and both HxClient.Builder.build() paths, for a JWT and a basic-auth fixture. A coverage test asserts each field is moved off its default by some fixture, so a field added later fails until it is covered rather than silently repeating HxClient.Builder.config() silently drops retryCondition, tokenRefreshTimeout and refreshCookiePolicy #113. Verified by temporarily dropping retryCondition from copyFrom() (7 failures) and by adding a dummy field (the coverage test failed naming it). Also pins that .proxy(x).config(cfg) discards x.
• HxConfigTest — a where-driven test over defaultRetryCondition: 10 cases covering IOException, ConnectException, SocketTimeoutException, FileNotFoundException, HttpConnectTimeoutException (retried) and HttpTimeoutException plus four non-I/O throwables (not retried), each also asserting the wired config agrees with the static rule. Plus the opt-back-in and composition patterns.
• HxClientRetryIntegrationTest — WireMock: retryCondition rejecting everything through config() contacts the upstream once; a subclass overriding shouldRetryOnException contacts it once; a request timeout under the default contacts it once; with the opt-in, 3 times. Each fails against the corresponding pre-change code, and no existing test does.
• ./gradlew :lib-httpx:build :lib-cloudinfo:test green.

Release

VERSION 2.4.0 → 2.5.0, one changelog section covering both layers led by BEHAVIOUR CHANGE, README dependency snippet bumped, and a "Which failures are retried" section documenting the default, the HttpTimeoutException extends IOException trap, the precedent, and both escape hatches.

⚠️ The [release] marker now has to be on this PR. It previously lived on #116 so that the two stacked PRs would publish exactly once; #116 is merged into this branch, so this PR's merge commit is the only remaining trigger. Without the marker in the merge commit message, 2.5.0 is not published.

Not included: #115 — retries are not gated on idempotency (a POST is retried like a GET, and 429/500 responses are retried for any method), and there is no overall retry budget.

Review

Both layers were reviewed by claude[bot] (approved). All actionable notes applied: the config() proxy/authenticator javadoc scope, four stale "Retry on IOException" javadoc bullets, the README example that would have silently reverted the new default, defaultRetryCondition made public to match what the changelog and doc links promise, and the over-broad Apache precedent scoped. Declined with reasons: restoring a debug log inside the decision predicate, importing Objects against the file's own existing idiom, a pre-existing raw-Predicate cast unchanged by this PR, and adding builder delegates that were explicitly out of scope.

Design doc: docs/superpowers/specs/2026-08-11-hxclient-config-lossless-copy-design.md.

🤖 Generated with Claude Code

[pditommaso]

@claude review

[claude]

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

---

Review: lossless HxClient.Builder.config()

• Read the PR diff
• Verify copyFrom() covers every field build() assigns
• Review the shouldRetryOnException retry wiring
• Review the new tests
• Check version/changelog/README consistency and CI

Verdict: looks good — no correctness, security or data-loss issue found. The fix is surgical and targets the actual root cause (the hand-written field list), and the reflective round-trip test makes the class of bug non-recurring. CI is green on this head (Libseqera CI run 31525518010).

---

What I verified

1. Field lists actually match. I diffed copyFrom() (HxConfig.java:715-741) against build() (HxConfig.java:752-778) field by field: both assign the same 17 fields, and those are exactly the 17 declared instance fields of HxConfig (HxConfig.java:74-96). The name-mapping asymmetries are handled correctly — bearerToken ↔ jwtToken, wwwAuthenticationEnabled ↔ wwwAuthenticateEnabled, wwwAuthenticationCallback ↔ authenticationCallback. The comment explaining why direct field assignment is used instead of the fluent setters is warranted and appreciated.

2. The retry wiring is behaviour-preserving for a default config. HxClient.java:480,553 now pass this::shouldRetryOnException. Tracing the null paths:

• HxConfig.retryCondition defaults to t -> t instanceof IOException (HxConfig.java:72,80), so the default is identical to the old inline check.
• If a caller explicitly passes retryCondition(null), shouldRetryOnException falls back to throwable instanceof IOException (HxClient.java:625-627), which matches Retryable.DEFAULT_CONDITION (Retryable.java:263) — the value the old code would have landed on via condition != null ? condition : DEFAULT_CONDITION at Retryable.java:339. So the null path is preserved too, not accidentally diverged.
• config being null would NPE, but shouldRetryOnResponse (HxClient.java:640) already dereferences config unconditionally on the same code path, so this is not a new failure mode.

3. No consumer-visible break. HxConfig.newBuilder(HxConfig) is a purely additive overload (no ambiguity with the no-arg newBuilder()). config(null) now throws NullPointerException explicitly where it previously NPE'd on config.getJwtToken() — same observable contract. The proxy mirroring in config() (HxClient.java:1080-1081) is needed because build()'s non-explicit-client path writes those two builder fields back onto the config builder (HxClient.java:1436-1437); without the mirror, a config's proxy would be wiped on that path. Correct as written.

4. Tests are the right shape. The coverage test in HxConfigRoundTripTest — asserting every declared field is moved off its default by some fixture — is what turns this from a one-time fix into a regression guard; a field added later fails loudly instead of silently repeating #113. Both HxClientRetryIntegrationTest additions exercise paths no existing test covered, and the NeverRetryClient subclass test is what pins the newly-wired extension point.

5. Release hygiene matches CLAUDE.md: VERSION 2.4.0 → 2.5.0 (minor is right for an additive API plus a behaviour fix), changelog.txt entry, README.md dependency snippet bumped, [release] in the title. The design doc follows the existing docs/superpowers/specs/ convention.

---

Non-blocking notes

a. Lost debug log on retry-because-exception. The old shouldRetryOnException logged "Retrying on IOException: {}"; the new one logs nothing, while its sibling shouldRetryOnResponse still logs (HxClient.java:641). The onRetry handler covers the observability in practice, so this is cosmetic — but the asymmetry between the two hooks is a little odd now.

b. Fully-qualified java.util.Objects.requireNonNull in both HxConfig.java:208 and HxClient.java:1075, where every other reference in these files goes through an import. Purely style. Fix this →

c. Raw-Predicate cast risk is now inside HxClient. getRetryCondition() is typed Predicate<? extends Throwable>, so a caller may legally supply e.g. a Predicate<IOException>; testing it against a non-IOException throwable would throw ClassCastException from the lambda's synthetic cast. This is pre-existing — Retryable did the same toChecked(condition).test(...) — so the PR introduces no regression, and the @SuppressWarnings is honest about it. Worth knowing that the trap now lives one frame closer to HxClient; a future signature tightening to Predicate<Throwable> would remove it.

d. config() javadoc scope. "HxConfig-specific builder methods called after this one override the copied values; those called before it are discarded" is accurate for the config setters, but proxy(ProxySelector) / authenticator(Authenticator) are also overwritten by config() even though they read as HttpClient.Builder passthroughs. The build() javadoc clarifies the propagation matrix, so a reader who follows the trail gets it — naming those two explicitly on config() would save the trip.

e. Still no HxClient.Builder delegate for retryCondition / tokenRefreshTimeout. The PR calls this out, and the HxConfig.newBuilder(existing) route plus the README section make it reachable, so this doesn't block. Reasonable follow-up alongside the deferred default-retryCondition question.

One thing I could not verify locally

I was unable to run ./gradlew :lib-httpx:test in this sandbox (the command required approval that isn't available in this run), so my analysis of the test files is by reading, not execution. The green Libseqera CI run covers it.

Note: I can't submit a formal GitHub review or approval — this comment is my review feedback. More detail in the FAQ.
• branch fix/hxclient-config-lossless-copy

[pditommaso]

@claude validate the changes after the last review

[claude]

Claude finished @pditommaso's task in 4m 28s —— View job

---

Validation of the changes made since the last review

• Identify what changed on the branch since the previous review
• Re-verify the field lists (copyFrom() vs build())
• Re-verify the retry wiring and the new default-retryCondition behaviour
• Check the non-blocking notes from the last review (a–e) for follow-up
• Check version / changelog / README consistency and CI

Verdict: validated — the changes since my last review hold up. No correctness, security or data-loss issue found. CI is green on this head (Libseqera CI run 31530469312).

---

What changed since the last review

One commit: 460af80 "Stop retrying request timeouts by default (#116)" — the squash of stacked PR #116, whose base was this branch. It lands the item my last review recorded as deferred, plus the two follow-ups raised in review of #116 (public defaultRetryCondition, javadoc/README alignment). Commit a6df901 also picked up note (d) from my last review — config()'s javadoc now names proxy(ProxySelector) and authenticator(Authenticator) explicitly (HxClient.java:1074-1078).

What I re-verified

1. The original fix is intact. copyFrom() (HxConfig.java:770-787) and build() (HxConfig.java:795-812) still assign the same 17 fields, and that is still exactly the set of declared non-static instance fields of HxConfig. The new commit added no instance field (defaultRetryCondition is a static method; DEFAULT_RETRY_COND at HxConfig.java:77 is static and filtered out by configFields() in HxConfigRoundTripTest), so the reflective round-trip guard still covers everything.

2. The new default is exactly the delta claimed, no wider. HxConfig.defaultRetryCondition (HxConfig.java:103-114):

• non-IOException → false (unchanged)
• HttpConnectTimeoutException → true — correct, and it must be tested before the general HttpTimeoutException branch, which it is (it's a subclass; the instanceof HttpTimeoutException guard returns throwable instanceof HttpConnectTimeoutException, so the ordering is inherently right rather than accidentally right)
• bare HttpTimeoutException → false — the single behavioural difference from pre-2.5.0
• every other IOException, including SocketTimeoutException → true (unchanged)

3. No drift between the two definitions of "the default". This was the failure mode worth checking, since there are now three candidate defaults in play. DEFAULT_RETRY_COND is HxConfig::defaultRetryCondition, and shouldRetryOnException's null-fallback (HxClient.java:634) calls the same static — so a configured default and a retryCondition(null) config now agree by construction. And because sendWithRetry/sendWithRetryAsync pass this::shouldRetryOnException (never null) to Retryable, lib-retry's own DEFAULT_CONDITION (t instanceof IOException) is unreachable from HxClient — so the one place the old and new rules could have diverged silently is closed. HxConfigTest's new table asserts both definitions agree for all 10 cases.

4. Sync and async agree. The async path unwraps ExecutionException and rethrows an IOException cause verbatim (HxClient.java:602-604), so sendAsync surfaces the real HttpTimeoutException to the condition, not a wrapper — same decision as send. (Only the sync path has a timeout integration test; see note D.)

5. Blast radius inside this repo. The only in-repo consumer is lib-cloudinfo's CloudInfoClient, which sets 30s/60s per-request timeouts on a default-retry client — so it is squarely in the class of caller this change fixes (bounded by timeout, not 5 × timeout). Its tests pass in CI.

6. Release hygiene. VERSION 2.5.0, both changes folded into one 2.5.0 changelog section led by an explicit BEHAVIOUR CHANGE line with the opt-back-in recipe, README dependency snippet bumped, [release] in the title. Folding rather than opening a 2.6.0 is right — the two commits publish as one artifact.

---

Non-blocking notes

A. Two checked-in scope statements are now stale — worth fixing before merge. docs/superpowers/specs/2026-08-11-hxclient-config-lossless-copy-design.md:235-244 §"Out of scope" still says changing the default retryCondition is not part of this work, and the PR body's "Not in this PR" says the same. The design doc lands in the repo and will read as wrong to the next person. Fix this →

B. An inline comment contradicts the javadoc directly above it. HxClient.java:1441-1442 says an explicitly supplied client means proxy settings are "neither applied to it nor propagated to the internal token refresh clients" — but on that path configBuilder.build() retains the config-carried proxySelector/proxyAuthenticator copied by copyFrom(), so they do reach the token-refresh clients. That is precisely the intended behaviour change this PR advertises, and the javadoc at HxClient.java:1428-1435 states it correctly. Only the comment is wrong; the code is right. Fix this →

C. A behaviour change shipping as a minor bump. 2.4.0 → 2.5.0 for a change that alters retry behaviour for every consumer on a default config. It is additive at the API level, the changelog leads with BEHAVIOUR CHANGE, and there is a one-line opt-back-in, so a minor is defensible — but consumers who upgrade transitively won't read the changelog. Your call; flagging it as a judgment call, not a defect.

D. Async timeout path untested. Both new WireMock tests use client.send(...). Given note 4 the async behaviour follows, but a sendAsync case would pin the ExecutionException-unwrapping that makes it follow. Also, the two new tests are timing-based (150ms request timeout against a 1000ms withFixedDelay); margins look comfortable, but the opt-in test asserting exactly 3 requests is the one to watch if CI ever gets noisy.

E. Carried over from the last review, all still open and all still non-blocking: no debug log on the retry-because-exception branch while shouldRetryOnResponse still logs (a); fully-qualified java.util.Objects.requireNonNull at HxConfig.java:252, HxClient.java:1065 and HxClient.java:1085 where the files otherwise import (b); the raw-Predicate ClassCastException trap, pre-existing (c); no HxClient.Builder delegate for retryCondition/tokenRefreshTimeout, reachable via HxConfig.newBuilder(existing) (e).

What I could not verify

./gradlew :lib-httpx:test still requires an approval unavailable in this run, so my analysis of the tests is by reading, not execution. The green Libseqera CI run above covers them.

I'm unable to submit a formal GitHub review or approval — this comment is my review feedback. More detail in the FAQ.
• branch fix/hxclient-config-lossless-copy