fix: cancellation, timeout classification, and error-handling correctness#209
Merged
Conversation
.superpowers/sdd/progress.md was a local planning note that should never have been tracked; drop it from the branch.
… edges - Futures: raise the interrupting flag before clearing the worker reference in InterruptibleFuture.cancel. The previous order let a worker finish its task, skip the unbind spin, return to its pool, and only then receive an interrupt aimed at the cancelled call — landing it on an unrelated task. - JDK transport: stop misclassifying a read timeout as a cancellation. SocketTimeoutException extends InterruptedIOException, so the sync catch set the caller's interrupt flag on a plain timeout and the async mapper returned it un-normalized. Both now surface it as a retryable NetworkException, matching the OkHttp transport and the retry steps' carve-out. - Jackson: JacksonSerde.from() no longer fails construction for an ObjectMapper subclass whose copy() is unsupported — it falls back to using the supplied mapper. - OperationRequestAssembler: catch IllegalArgumentException (what url(String) now throws) so a base URL that resolves to a malformed URL keeps its enriched base-URL/operation context instead of the builder's generic message. - RequestOptions.maxRetries: reject a negative override at build time rather than letting it be silently reinterpreted downstream as "use the configured default", mirroring timeout()'s validation. 0 still disables retries. - throwOnHttpError recovery: buffer the error body up to the shared 1 MiB bound instead of 4 KiB, so a large error payload is not truncated mid-value only on the recovery path.
Consolidate copy-pasted logic that had drifted or could drift: - Single-source the idempotent HTTP method set as Method.IDEMPOTENT_METHODS; RetrySettings.DEFAULT_RETRYABLE_METHODS and RetryPolicySupport both derive from it instead of maintaining parallel literals. - Hoist the interrupt/timeout retry-classification predicate into RetryPolicySupport.isNonRetryableInterrupt, shared by the sync and async retry steps so they cannot classify cancellation differently. - Extract the URL-valued-header redaction into a single HeaderValueRedactor used by both the sync and async instrumentation steps. - Extract the OkHttp Content-Type resolution into resolveOkHttpContentType, shared by the two request-body adapters. - Drop the now-dead negative guard in RetryPolicySupport.effectiveMaxRetries, since the builder now rejects a negative per-call maxRetries.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up correctness fixes on top of the developer-experience work in #208, plus a
small internal consolidation and one file-cleanup. No public API changes.
Correctness fixes
InterruptibleFuture.cancel. The interrupting flag is nowraised before the worker reference is cleared. The old order left a window where a
worker could finish its task, skip the unbind spin, return to its pool, and only then
receive an interrupt aimed at the cancelled call — delivering it to an unrelated task.
SocketTimeoutExceptionextends
InterruptedIOException, so the sync path was setting the caller's interruptflag on a plain read timeout and the async mapper returned it un-normalized. Both now
surface it as a retryable
NetworkException, matching the OkHttp transport and theretry steps' interrupt carve-out.
JacksonSerde.from()on anObjectMappersubclass with unsupportedcopy().Construction no longer fails; it falls back to the supplied mapper.
OperationRequestAssemblererror context. It now catchesIllegalArgumentException(what
url(String)throws) so a base URL that resolves to a malformed URL keeps itsenriched base-URL/operation context instead of the builder's generic message.
RequestOptions.maxRetriesnegative override. Rejected at build time rather thanbeing silently reinterpreted downstream as "use the configured default", mirroring
timeout()'s validation.0still disables retries.throwOnHttpErrorerror-body buffering. Buffers up to the shared 1 MiB boundinstead of 4 KiB, so a large error payload is not truncated mid-value only on the
recovery path.
Internal consolidation
Removes copy-pasted logic that had drifted or could drift apart:
Method.IDEMPOTENT_METHODS;RetrySettings.DEFAULT_RETRYABLE_METHODSandRetryPolicySupportderive from it.RetryPolicySupport.isNonRetryableInterrupt, so the sync and async retry steps cannotclassify cancellation differently.
HeaderValueRedactorused by boththe sync and async instrumentation steps.
Content-Typeresolution intoresolveOkHttpContentType, shared bythe two request-body adapters.
RetryPolicySupport.effectiveMaxRetries(thebuilder now rejects a negative per-call
maxRetries).Cleanup
Removes
.superpowers/sdd/progress.md, a local planning note that should never have beentracked. It reached
mainthrough #208's squash merge, so it is deleted here.Verification
Full gated build passes locally (
./gradlew build): tests, ktlint, detekt,apiCheck,and the Kover coverage floor. Regression tests for the behavioral fixes were added under
#208 and remain green.