Attach the prior-attempt suppressed trail on terminal failure in the recovery-chain retry step (RETRY-34)

[OmarAlJarrah]

Summary

The recovery-chain retry step (RetryRecovery) surfaces only the last attempt's throwable when retries are exhausted. It never attaches the earlier attempts to the surfaced exception as suppressed — in fact it never accumulates a trail at all. Its per-call state holds only a start instant and an attempt counter, and the retry loop keeps a single lastError that each iteration overwrites; the terminal exception is then surfaced bare.

This violates RETRY-34 (a MUST), which requires that on terminal failure every prior failed attempt's exception be attached to the surfaced exception as suppressed (skipping the surfaced instance itself), and that the trail be discarded only on eventual success. Both stage-based retry stacks (DefaultRetryStep, DefaultAsyncRetryStep) already build and attach this trail; the recovery stack — a third, independent retry implementation, and the resilience layer meant to observe every outcome uniformly — does not.

Impact

This is a platform-wide defect in a core primitive. RetryRecovery (org.dexpace.sdk.core.pipeline.step.retry.RetryRecovery) is public API and the retry element of the recovery-chain resilience layer (§8.2 / RECOV), which consumers wire into a ResponseRecoveryChain to get uniform failure handling. Every external consumer — generated service clients, downstream SDKs, application code — that uses the recovery stack for retries loses all diagnostic context from every attempt but the last on terminal failure.

Consumers cannot work around this. The earlier attempts' throwables never escape the primitive: they are overwritten inside runRetryLoop and are unreachable by the time the terminal Failure/throw reaches the caller. No consumer-side interceptor, error mapper, or logging layer can reconstruct a trail that was discarded inside sdk-core. When a request fails after a connection reset, then a 503, then a socket timeout, the caller sees only the socket timeout — the reset and the 503 are gone, which is exactly the debugging information RETRY-34 exists to preserve.

Where

All references verified against the current source.

sdk-core/src/main/kotlin/org/dexpace/sdk/core/pipeline/step/retry/RetryRecovery.kt

• AttemptState (RetryRecovery.kt:126–130) carries only startInstant: Instant and var attempt: Int = 1. There is no accumulator for prior-attempt throwables.
• runRetryLoop (RetryRecovery.kt:176–196) declares var lastError: Throwable = initialError (line 180) and overwrites it on each retryable failure: lastError = nextError (line 192). Previous errors are dropped, not accumulated.
• prepareNextAttempt returns AttemptStep.Abort(lastError) on cap/deadline exhaustion (RetryRecovery.kt:213, 215, 218) and AttemptStep.Abort(e) on an interrupted wait (RetryRecovery.kt:223), each carrying a single throwable.
• The terminal failure is surfaced bare: runRetryLoop returns ResponseOutcome.Failure(readyState.error) (RetryRecovery.kt:183) with no suppressed attachment, and attempt() rethrows it directly via is ResponseOutcome.Failure -> throw final.error (RetryRecovery.kt:165).
• There is no addSuppressed call anywhere in the file.

ResponseOutcome.Failure — sdk-core/src/main/kotlin/org/dexpace/sdk/core/pipeline/ResponseOutcome.kt:49 — is the terminal failure wrapper the surfaced (bare) throwable travels in.

For contrast, the stage stacks do attach the trail:

• sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/pipeline/steps/DefaultRetryStep.kt:247 — suppressed?.forEach(exception::addSuppressed) (sync; attaches unconditionally, no skip-self guard), and DefaultRetryStep.kt:234 attaches the same trail to the interrupted-I/O exception on the interrupt path.
• sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/pipeline/steps/DefaultAsyncRetryStep.kt:342 — suppressed?.forEach { prior -> if (prior !== error) error.addSuppressed(prior) } in failTerminally, applied on every terminal path (async; with the skip-self guard). Line 321 (if (t !== exception) t.addSuppressed(exception)) applies the same guard when the retry-decision/delay computation itself throws.

Expected vs. actual

Given a request that fails on attempt 1 (e1), retries and fails on attempt 2 (e2), then exhausts on attempt 3 (e3):

• Expected (RETRY-34): the surfaced e3 carries e1 and e2 on e3.getSuppressed(), with the surfaced instance skipped so a transport that reuses one instance across attempts cannot trip addSuppressed's self-suppression guard.
• Actual: e3 is surfaced with an empty suppressed array. e1 and e2 were overwritten in lastError and are unrecoverable.

The RETRY-34 success clause (discard the trail on eventual success) is trivially satisfied today only because no trail is ever built.

Proposed fix

Core-only, no new dependency:

1. Add a lazily-allocated MutableList<Throwable>? accumulator to AttemptState (mirroring the suppressed field in DefaultRetryStep/DefaultAsyncRetryStep).
2. In runRetryLoop, append each failed attempt's throwable to the accumulator as the loop advances — the initial error, then each retryable nextError — so no attempt's throwable is lost when lastError is reassigned (line 192).
3. On every terminal path — both the cap/deadline Abort and the interrupted-wait Abort — attach the accumulated trail to the surfaced throwable before it is wrapped in ResponseOutcome.Failure (line 183), applying the RETRY-34 skip-self guard (if (prior !== error) error.addSuppressed(prior)) so the surfaced instance is never suppressed onto itself. Because attempt() rethrows the Failure.error (line 165), attaching before the Failure wrap covers both the ResponseOutcome return and the throwing entry point with one change.

The interrupted-wait abort (RetryRecovery.kt:223) is also a terminal failure and should carry the prior-attempt trail, matching the stage stacks (DefaultRetryStep.kt:234 attaches the trail to the interrupted-I/O exception it raises). This preserves RETRY-23 semantics (interrupt is terminal, not retried) while still surfacing the trail that led up to the cancellation.

Because RetryRecovery may see a transport that reuses one exception instance across attempts, the skip-self guard is required here (not optional): without it, a reused instance appearing in the accumulated trail would trip addSuppressed's self-suppression guard and raise IllegalArgumentException.

Acceptance criteria

• A recovery-chain retry that fails on attempts 1..N and exhausts on attempt N surfaces the attempt-N throwable with attempts 1..N-1 present on its getSuppressed(), via both invoke() (terminal ResponseOutcome.Failure) and attempt() (rethrow).
• The surfaced instance is never present in its own suppressed array, and a transport that returns the same throwable instance on every attempt does not raise IllegalArgumentException from addSuppressed.
• A retry that eventually succeeds surfaces the Response with no prior-attempt trail attached anywhere.
• The interrupted-wait terminal path surfaces the InterruptedIOException with the prior-attempt trail attached as suppressed, the interrupt flag restored, and no further attempt dispatched.

References

• docs/product-spec.md RETRY-34 (MUST, §9.4 and the requirement table) — terminal-failure suppressed-trail attachment with the skip-self guard, discard-on-success. The spec's reference note ("only the async stack currently implements the skip-self guard; a port MUST apply it to both stacks") describes the two stage stacks; the recovery stack does not implement RETRY-34 at all.
• docs/product-spec.md RETRY-25 (MUST) — non-recoverable JVM errors are surfaced "with no suppressed-trail attachment," which presupposes that ordinary retryable terminal failures do carry the trail; the recovery stack currently attaches nothing in either case.
• docs/product-spec.md RETRY-27 / RETRY-28 and §8.3 — the recovery and stage stacks are intentionally kept separate and MUST NOT be collapsed into one. That separation is why the trail behavior has to be implemented in the recovery stack directly rather than inherited from the stage stacks.