Skip the surfaced exception when attaching the prior-attempt trail in the synchronous retry step

[OmarAlJarrah]

Summary

The synchronous retry step attaches the prior-attempt failure trail to the terminal exception without skipping the surfaced instance itself. If a transport reuses (or re-throws) a single exception instance across attempts, that instance is already in the accumulated trail, and Throwable.addSuppressed(self) throws IllegalArgumentException. The IllegalArgumentException then propagates out of the retry step and masks the real terminal failure the caller was supposed to see.

In DefaultRetryStep.kt, the terminal-failure path attaches the trail unconditionally:

// Terminal failure path — every prior attempt's exception is attached as
// suppressed on the rethrown exception so callers see the full trail.
suppressed?.forEach(exception::addSuppressed)

The asynchronous stack already guards this exact case in DefaultAsyncRetryStep.failTerminally:

suppressed?.forEach { prior -> if (prior !== error) error.addSuppressed(prior) }

The two stacks diverge, and only the async one is correct.

Impact

This is a platform-wide defect in a core retry primitive. Every consumer that runs a synchronous request through the stage-based retry pipeline — generated service clients, downstream SDKs, and application code built on top of sdk-core — is exposed, and none of them can work around it: the faulty attach happens inside the pipeline step, before any exception is surfaced to the caller.

The trigger is not exotic. A transport is free to reuse or re-throw one exception instance across attempts (RETRY-34 explicitly anticipates this). The sequence that fails:

1. A retryable attempt throws exception E; decideRetryException records E into the trail (DefaultRetryStep.kt:332) and the loop retries.
2. A later attempt fails again with the same instance E, and this attempt is terminal (retry budget exhausted or the failure is non-retryable), so the loop reaches the terminal path at DefaultRetryStep.kt:247.
3. E is already an element of suppressed, so E.addSuppressed(E) throws IllegalArgumentException: Self-suppression not permitted.

The caller then receives that IllegalArgumentException instead of the actual IOException/HttpException from the failed request. The genuine failure — status, cause, and the whole point of the surfaced error — is lost, turning a diagnosable transport failure into a misleading contract violation.

Where

• sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/pipeline/steps/DefaultRetryStep.kt:247 — terminal-failure path (lines 245–259), suppressed?.forEach(exception::addSuppressed). No identity check against the surfaced exception. This is the only reachable self-suppression site in the sync step.
• sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/pipeline/steps/DefaultAsyncRetryStep.kt:341 — failTerminally(error), which at line 342 already applies the if (prior !== error) guard. This is the correct reference implementation.

For completeness, the two other suppression sites in DefaultRetryStep.kt are not reachable self-suppression sites and do not need changing:

• Line 418 (sleepOrAbort) always attaches the trail to a freshly constructed InterruptedIOException("retry interrupted") (built at line 417), so the target can never be an element of the trail.
• Line 234 (non-retryable-interrupt path) attaches the trail to support.asInterruptedIo(exception), which returns the same instance only when exception is already an InterruptedIOException (RetryPolicySupport.kt:77). But an interrupt is intercepted at DefaultRetryStep.kt:231 (RetryPolicySupport.isNonRetryableInterrupt, RetryPolicySupport.kt:135) and rethrown immediately, before the loop can reach decideRetryException — the only place the trail is appended (DefaultRetryStep.kt:332). An interrupt therefore terminates the loop before it can be recorded. The trail at line 234 may hold earlier retryable failures, but never the surfaced interrupt instance itself, so addSuppressed cannot self-suppress there.

Expected vs. actual

Expected (RETRY-34): on terminal failure, every prior failed attempt's exception is attached to the surfaced exception as suppressed, and the surfaced instance itself is skipped, so a transport that reuses one exception instance across attempts cannot trip a self-suppression error. The caller sees the real terminal failure with the prior trail attached.

Actual: the sync step attaches the trail unconditionally. When the surfaced instance is also present in the trail (reused-instance transport), exception.addSuppressed(exception) throws IllegalArgumentException, which propagates out of the retry step (unchecked, so it is not caught or wrapped by the @Throws(IOException::class) path) and replaces the real terminal failure.

Proposed fix

Mirror the async guard at DefaultRetryStep.kt:247:

suppressed?.forEach { if (it !== exception) exception.addSuppressed(it) }

This is the single reachable site, so no other change is required in the sync step.

Acceptance criteria

• Driving the sync retry loop (DefaultRetryStep.process) with a transport that throws the same exception instance on every attempt, up to the terminal attempt, surfaces that original exception — not an IllegalArgumentException.
• On that same run, prior attempts that contributed distinct instances to the trail are present in the surfaced exception's getSuppressed(), and the surfaced instance does not appear in its own suppressed list.
• The existing behaviour for distinct-instance transports (each attempt throws a fresh exception) is unchanged: all prior instances appear as suppressed on the terminal exception.
• A regression test in sdk-core covers the reused-instance sync path and asserts both the surfaced type and the suppressed trail.

References

• docs/product-spec.md — RETRY-34 (MUST): the skip-self guard is required on both the sync and async stacks; the spec text itself flags that only the async stack currently implements it and that a port must apply the guard to both (lines 382 and 1453).
• Related requirement in the same area: RETRY-33 (MUST) — every terminal path of the async loop must complete the returned future; DefaultAsyncRetryStep.failTerminally is where both RETRY-33 and the RETRY-34 skip-self guard are satisfied, and is the model for this fix.