Skip to content

fix: cancellation, timeout classification, and error-handling correctness#209

Merged
OmarAlJarrah merged 3 commits into
mainfrom
dx-followup-fixes
Jul 7, 2026
Merged

fix: cancellation, timeout classification, and error-handling correctness#209
OmarAlJarrah merged 3 commits into
mainfrom
dx-followup-fixes

Conversation

@OmarAlJarrah

Copy link
Copy Markdown
Member

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

  • Cancellation ordering in InterruptibleFuture.cancel. The interrupting flag is now
    raised 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.
  • Read-timeout misclassification in the JDK transport. SocketTimeoutException
    extends InterruptedIOException, so the sync path was setting the caller's interrupt
    flag 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 the
    retry steps' interrupt carve-out.
  • JacksonSerde.from() on an ObjectMapper subclass with unsupported copy().
    Construction no longer fails; it falls back to the supplied mapper.
  • OperationRequestAssembler error context. It now catches IllegalArgumentException
    (what url(String) 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 negative override. Rejected at build time rather than
    being silently reinterpreted downstream as "use the configured default", mirroring
    timeout()'s validation. 0 still disables retries.
  • throwOnHttpError error-body buffering. Buffers 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.

Internal consolidation

Removes copy-pasted logic that had drifted or could drift apart:

  • Single-sources the idempotent HTTP method set as Method.IDEMPOTENT_METHODS;
    RetrySettings.DEFAULT_RETRYABLE_METHODS and RetryPolicySupport derive from it.
  • Hoists the interrupt/timeout retry-classification predicate into a shared
    RetryPolicySupport.isNonRetryableInterrupt, so the sync and async retry steps cannot
    classify cancellation differently.
  • Extracts URL-valued-header redaction into a single HeaderValueRedactor used by both
    the sync and async instrumentation steps.
  • Extracts OkHttp Content-Type resolution into resolveOkHttpContentType, shared by
    the two request-body adapters.
  • Drops a now-dead negative guard in RetryPolicySupport.effectiveMaxRetries (the
    builder now rejects a negative per-call maxRetries).

Cleanup

Removes .superpowers/sdd/progress.md, a local planning note that should never have been
tracked. It reached main through #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.

.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.
@OmarAlJarrah OmarAlJarrah merged commit c85d1b8 into main Jul 7, 2026
1 check passed
@OmarAlJarrah OmarAlJarrah deleted the dx-followup-fixes branch July 7, 2026 11:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant