Skip to content

chore: collapse duplicated helpers in recovery-aware pipeline + retry#194

Merged
OmarAlJarrah merged 1 commit into
mainfrom
chore/pipeline-retry-dedupe-helpers
Jun 28, 2026
Merged

chore: collapse duplicated helpers in recovery-aware pipeline + retry#194
OmarAlJarrah merged 1 commit into
mainfrom
chore/pipeline-retry-dedupe-helpers

Conversation

@OmarAlJarrah

Copy link
Copy Markdown
Member

What

Collapses four small, self-contained duplications that crept into the recovery-aware pipeline
primitives (pipeline) and the retry step (pipeline/step/retry) as the two stacks grew. None of
these change behavior — each removes copy-pasted logic in favor of a single definition.

1. One interrupt-aware failure wrapper (×3 → 1)

ExecutionPipeline, ResponsePipeline, and RetryStep.executeOnce each independently re-implemented
the same step: wrap a throwable in ResponseOutcome.Failure, restoring the thread interrupt flag first
when it is an InterruptedException. This is now a single module-internal failureOf(t) placed next to
ResponseOutcome (the type it constructs); all three call sites route through it, and
RetryStep.executeOnce folds its two catch arms into one.

The cancellation contract is preserved exactly. RetryStep.attempt() is deliberately not routed
through failureOf: it rethrows the wrapped throwable to its caller, so an InterruptedException
propagates as itself and carries the cancellation directly — pre-restoring the flag there is
unnecessary. failureOf exists for the opposite path, where the failure is buried in an outcome that
flows on through the pipeline and may be swallowed or recovered, so the flag must be restored.

2. One Duration validator for the three Duration setters

RetrySettings' totalTimeout, initialDelay, and maxDelay setters carried the same two require
checks (non-negative + nanosecond-representable). Extracted into a private requireRepresentable(name, value) that names the offending field; the rejection messages are carried verbatim (so a rejected
initialDelay still reads initialDelay must be representable in nanoseconds (≤ ~292 years); got …).
The delayMultiplier setter validates a Double and is left as-is.

3. Loop over the millisecond-variant headers

RetryAfterParser's retry-after-ms and x-ms-retry-after-ms branches were verbatim copies. They now
iterate the two header names. Array order keeps retry-after-ms ahead of x-ms-retry-after-ms, and the
first usable value still wins, so header precedence is unchanged.

4. Elvis for the scheduler fallback

RetryStep.resolveScheduler() collapses to settings.scheduler ?: DEFAULT_SCHEDULER. The by lazy
default is still only dereferenced when no caller scheduler is supplied, so the once-per-VM lazy-init
timing is preserved.

Why

One definition of each helper instead of two or three byte-for-byte copies, following the existing
top-level internal Duration.toNanosSaturating() precedent in the same area of the codebase.

API / build

failureOf and requireRepresentable are internal/private, and all setter/parser/scheduler
signatures are unchanged, so the public .api surface is untouched — no apiDump. ./gradlew :sdk-core:build passes (ktlint, detekt, apiCheck, tests, and the coverage gate).

Closes #172

Three call sites independently re-implemented the same interrupt-aware
failure wrapper — wrap a throwable in ResponseOutcome.Failure, restoring
the thread interrupt flag first when it is an InterruptedException. Hoist
it to a single module-internal failureOf() next to ResponseOutcome (the
type it constructs), and route ExecutionPipeline, ResponsePipeline, and
RetryStep.executeOnce through it. RetryStep.executeOnce folds its two
catch arms into one as a result.

RetryStep.attempt() is deliberately left untouched: it rethrows the
wrapped throwable to its caller, so an InterruptedException propagates as
itself and carries the cancellation directly — pre-restoring the flag
there is unnecessary. failureOf() exists for the opposite path, where the
failure is buried in an outcome that flows on through the pipeline and may
be swallowed or recovered, so the flag must be restored to preserve
cancellation.

Also collapse three more local duplications in the retry package:
- RetrySettings' totalTimeout/initialDelay/maxDelay setters shared the
  same non-negative + nanosecond-representability checks; extract a
  requireRepresentable(name, value) helper that names the offending field.
  Rejection messages are preserved verbatim.
- RetryAfterParser's retry-after-ms and x-ms-retry-after-ms branches were
  verbatim copies; iterate the two header names instead. Array order keeps
  retry-after-ms ahead of x-ms-retry-after-ms, preserving precedence.
- resolveScheduler() collapses to an elvis expression.

All changes are behavior-preserving. failureOf and requireRepresentable
are internal/private, so the public API surface is unchanged.
@OmarAlJarrah OmarAlJarrah merged commit a6d4ce2 into main Jun 28, 2026
1 check passed
@OmarAlJarrah OmarAlJarrah deleted the chore/pipeline-retry-dedupe-helpers branch June 28, 2026 01:51
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.

pipeline + retry: collapse duplicated helpers across the recovery-aware primitives

1 participant