Optional retry policy for transient connection failures (#158) - #162
Conversation
New builder setting retries(int, Duration): a WSMan round trip is retried only when it failed to establish and authenticate the connection (TCP connect, DNS, TLS handshake, or a transport error during the authentication handshake) - the one phase where the request provably never reached the server, so at-most-once execution semantics hold for non-idempotent commands. Credential rejections, WSMan faults, and any failure after the request reached the wire are never retried; WQL queries deliberately get no broader surface (a re-issued Pull could skip or duplicate rows). The policy is connection-scoped and applies to every round trip, including transparent reconnections mid-operation. Retries stay inside the blocking operations' wall-clock deadline (the pause is interrupted when the deadline fires) and inside a bounded poll's hard budget (a pause that no longer fits is skipped). Default: no retry - behavior unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8a742d1ac1
ℹ️ 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".
| Thread.sleep(retryDelayMs); | ||
| continue; |
There was a problem hiding this comment.
Recheck closure after each retry delay
When another thread calls close() while an operation is sleeping here, close() sets closed and returns, but it does not interrupt this worker. Once the delay ends, continue re-enters send() without passing through request()'s closure check, so the worker can reconnect, authenticate, and send the original SOAP request—including a non-idempotent command—after the client has closed. Check closed again before every retry attempt so closing during a pause cannot revive the transport or execute delayed side effects.
Useful? React with 👍 / 👎.
| pendingAuthorization = auth.authenticate(transport); | ||
| } catch (final IOException e) { |
There was a problem hiding this comment.
Preserve retryable I/O failures from authentication fallback
With an ordered HTTPS authentication list such as Kerberos then NTLM, auth is a FallbackAuthScheme, whose authenticate() catches each candidate's exception and ultimately wraps the last failure in IllegalStateException. Consequently, if Kerberos is unavailable and the NTLM handshake then encounters a transient IOException, this catch never sees it and the configured retry policy is skipped. Preserve or inspect the retryable transport failure after fallback exhaustion so .retries(...) also works with ordered authentication schemes.
Useful? React with 👍 / 👎.
…lback failures (#158) - A worker sleeping between retries cannot be interrupted by close(): recheck the closed flag after the pause so a retry never revives the hard-closed transport nor sends the delayed operation after the client was closed. - An ordered authentication fallback reports "all schemes failed" as an IllegalStateException wrapping the last scheme's failure: classify retryability by walking the cause chain for an IOException so the retry policy also works with ordered authentication lists. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Both findings addressed in cbea78a:
@codex review |
|
Codex Review: Didn't find any major issues. Keep them coming! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
Closes #158.
What
A new opt-in, connection-scoped builder setting:
Default stays no retry — current behavior unchanged.
Design decisions (from the issue's open points)
WsmanClient.send()and wraps only the connect-and-authenticate phase — TCP connect, DNS resolution, the TLS handshake (forced inensureConnected()), and the authentication handshake round trips. The transport now connects explicitly before authenticating (HttpTransport.connect()), so every "could not reach the endpoint" failure surfaces in that phase, where the operation's request provably never reached the server. Anything after the operation's own POST started is never retried, preserving at-most-once execution for non-idempotent commands. Credential rejections (WinRMAuthenticationException) and WSMan faults are never retried either.Pullwhose response was lost could skip or duplicate rows, because the server-side enumeration context advances.Utils.executecancels the worker with an interrupt), so a retry pause is cut short and the operation reports the documentedWinRMTimeoutException. A deadline-bounded poll (pollChunk) skips a retry whose pause no longer fits its hard budget (HttpTransport.remainingPollBudgetMillis()).close()'s best-effort cleanup never retries.Tests
WsmanRetryTestexercises the policy end to end against the in-processFakeWsmanServer(extended withdropNextConnections(n)andenqueueDrop()):Plus builder validation tests, and documentation in
timeouts-and-errors.md(including the winrm4jfailureRetryPolicyequivalence for migrating users).🤖 Generated with Claude Code