From 36378b392baa69e652e26ba337bd243a2d01df8d Mon Sep 17 00:00:00 2001 From: Mohammad Wahbeh Date: Sat, 5 Sep 2026 10:48:18 +0300 Subject: [PATCH 1/5] fix(retry): surface the final typed error, not a SuppressedError wrapper The retry engine wrapped its terminal failure in `suppress(error, folded, 'retry attempts exhausted')`, which made the surfaced CLASS a function of how many attempts ran: one attempt gave `TransportFailureError`, three gave a wrapper with it at `.error`. The row that catches it is XCUT-1's conformance clause -- "assert the surfaced error is the cancellation type" -- because a cancellation during backoff always has a non-empty trail, so `abortToSdkError` mapped the abort to `CancellationError` at engine.ts:385 and the wrapper undid the mapping at :386. RETRY-34 asks for the prior failures to be "attached to the surfaced exception as suppressed", which is the JVM's `addSuppressed`: the exception stays what it is and grows a list. It does not ask for the exception to be replaced by a container. So `withTrail` becomes `attachTrail`: the outcome's error is returned unchanged, and the priors go into a module-private WeakMap in the new `retry/attempt-trail.ts`, read back through a `@public` `retryAttempts(error)`. A side table rather than an own property, because the engine did not construct the throwable it is surfacing: it may be frozen (so `defineProperty` in the failure path would itself throw and replace the failure the caller cares about), it may be a primitive (which carries nothing at all, and passes through unannotated), and `.suppressed` already means "the one secondary" on `SuppressedErrorLike`. `suppress()` keeps its RECOV-12 job -- `withReleaseFailure` still pairs a release failure with the primary it must not mask. RETRY-34's skip-self guard is unchanged, and an empty trail now DELETES any entry a previous run left, so a transport reusing one error instance reports the run that just surfaced it rather than a stale one. Decision D10 of docs/audit-67-decisions.md. Refs #72, #67. --- packages/core/etc/core.api.md | 3 + packages/core/src/index.ts | 5 + packages/core/src/retry/attempt-trail.test.ts | 138 ++++++++++++++++ packages/core/src/retry/attempt-trail.ts | 91 +++++++++++ packages/core/src/retry/engine.test.ts | 147 ++++++++++++------ packages/core/src/retry/engine.ts | 49 +++--- packages/core/src/retry/errors.ts | 5 +- packages/core/src/retry/retry-dispatch.ts | 4 +- packages/core/src/retry/retry-step.ts | 8 + 9 files changed, 381 insertions(+), 69 deletions(-) create mode 100644 packages/core/src/retry/attempt-trail.test.ts create mode 100644 packages/core/src/retry/attempt-trail.ts diff --git a/packages/core/etc/core.api.md b/packages/core/etc/core.api.md index 3431dbb..ac916ca 100644 --- a/packages/core/etc/core.api.md +++ b/packages/core/etc/core.api.md @@ -1176,6 +1176,9 @@ export type ResponseStep = (response: Response_2) => Promise; // @public export const RETRYABLE_STATUSES: ReadonlySet; +// @public +export function retryAttempts(error: unknown): readonly unknown[]; + // @public export class RetryDiscardedResponseError extends DexpaceError { constructor(status: number, options?: ErrorOptions); diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index eff7f03..43cc879 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -113,6 +113,11 @@ export { ReservedStageError, } from './pipeline/errors.js'; export {retryStep} from './retry/retry-step.js'; +// RETRY-34's read side. The retry pillar surfaces the FINAL attempt's own error, so `instanceof` +// against it does not depend on how many attempts ran; this is how the earlier ones are reached. +// Exported alongside `retryStep` because the two are one contract: nothing else in the barrel can +// tell a caller that the error they caught is the third of three. +export {retryAttempts} from './retry/attempt-trail.js'; // The trail entry for a response the engine discarded whose status is outside 400-599 — reachable // only through a caller-widened `retryableStatuses`. export {RetryDiscardedResponseError} from './retry/errors.js'; diff --git a/packages/core/src/retry/attempt-trail.test.ts b/packages/core/src/retry/attempt-trail.test.ts new file mode 100644 index 0000000..b5835be --- /dev/null +++ b/packages/core/src/retry/attempt-trail.test.ts @@ -0,0 +1,138 @@ +// SPDX-License-Identifier: MIT +// packages/core/src/retry/attempt-trail.test.ts +// Exercises: RETRY-34 (the prior-attempt trail rides alongside the surfaced error rather than +// replacing it; the surfaced instance is skipped; a run with no priors leaves no trail behind), +// XCUT-1 (recording a trail never changes the surfaced value's class). +import {describe, expect, test} from 'bun:test'; +import {IoError} from '../io/errors.js'; +import {CancellationError} from '../seams/transport.js'; +import {recordAttempts, retryAttempts} from './attempt-trail.js'; + +describe('retryAttempts -- the read side', () => { + test('returns an empty list for an error that never went through the engine', () => { + expect(retryAttempts(new IoError('never retried'))).toEqual([]); + }); + + test('returns an empty list for a primitive, which cannot carry a trail at all', () => { + // RETRY-34's trail is keyed by identity, so a string, a number or a symbol throw passes + // through the engine unchanged and unannotated rather than being wrapped to make room. + expect(retryAttempts('a bare string throw')).toEqual([]); + expect(retryAttempts(42)).toEqual([]); + expect(retryAttempts(Symbol('thrown'))).toEqual([]); + expect(retryAttempts(null)).toEqual([]); + expect(retryAttempts(undefined)).toEqual([]); + }); + + test('returns the recorded attempts oldest first', () => { + const first = new IoError('first'); + const second = new IoError('second'); + const surfaced = new IoError('third'); + + recordAttempts(surfaced, [first, second]); + + expect(retryAttempts(surfaced)).toEqual([first, second]); + }); + + test('hands back a frozen list, so one caller cannot edit a trail another caller holds', () => { + const surfaced = new IoError('surfaced'); + recordAttempts(surfaced, [new IoError('prior')]); + + const attempts = retryAttempts(surfaced); + + expect(Object.isFrozen(attempts)).toBe(true); + }); + + test('does not read a trail off the cause chain -- only off the instance itself', () => { + const inner = new IoError('inner'); + recordAttempts(inner, [new IoError('prior')]); + const outer = new IoError('outer', {cause: inner}); + + expect(retryAttempts(outer)).toEqual([]); + }); +}); + +describe('recordAttempts -- the write side', () => { + test('leaves the class and identity of the error untouched (XCUT-1)', () => { + const surfaced = new CancellationError('operation cancelled'); + + recordAttempts(surfaced, [new IoError('prior')]); + + expect(surfaced).toBeInstanceOf(CancellationError); + expect(surfaced.name).toBe('CancellationError'); + }); + + test('adds no own property, so a JSON or structured-clone round trip is unchanged', () => { + const surfaced = new IoError('surfaced'); + const before = Object.getOwnPropertyNames(surfaced).sort(); + + recordAttempts(surfaced, [new IoError('prior')]); + + expect(Object.getOwnPropertyNames(surfaced).sort()).toEqual(before); + }); + + test('copies the trail, so a later push by the engine cannot mutate a published list', () => { + const surfaced = new IoError('surfaced'); + const trail: unknown[] = [new IoError('prior')]; + + recordAttempts(surfaced, trail); + trail.push(new IoError('added afterwards')); + + expect(retryAttempts(surfaced)).toHaveLength(1); + }); +}); + +describe('recordAttempts -- the values it can and cannot key on', () => { + test('is a no-op on a frozen error rather than throwing', () => { + // The reason the trail is a side table and not an own property: a foreign error may be frozen + // or non-extensible, and `defineProperty` inside the failure path of the engine would then replace + // the failure the caller cares about with a TypeError. + const surfaced = Object.freeze(new IoError('frozen by its author')); + const prior = new IoError('prior'); + + expect(() => { + recordAttempts(surfaced, [prior]); + }).not.toThrow(); + expect(retryAttempts(surfaced)).toEqual([prior]); + }); + + test('is a no-op for a primitive surfaced value', () => { + expect(() => { + recordAttempts('a bare string throw', [new IoError('prior')]); + }).not.toThrow(); + expect(retryAttempts('a bare string throw')).toEqual([]); + }); + + test('records against a thrown function, which is an object for these purposes', () => { + const surfaced = (): void => undefined; + const prior = new IoError('prior'); + + recordAttempts(surfaced, [prior]); + + expect(retryAttempts(surfaced)).toEqual([prior]); + }); +}); + +describe('recordAttempts -- a reused error instance', () => { + test('an empty trail clears any entry a previous run left on a reused instance', () => { + // Error singletons are ordinary in fakes and in transports that reuse one instance. The RETRY-34 + // clause "on eventual success the prior trail MUST be discarded" is worth nothing if the NEXT run + // to surface that same instance still reports the old one. + const reused = new IoError('reused across runs'); + recordAttempts(reused, [new IoError('from the first run')]); + + recordAttempts(reused, []); + + expect(retryAttempts(reused)).toEqual([]); + }); + + test('the latest recording wins for a reused instance', () => { + const reused = new IoError('reused across runs'); + const older = new IoError('from the first run'); + const newer = new IoError('from the second run'); + + recordAttempts(reused, [older]); + recordAttempts(reused, [newer]); + + expect(retryAttempts(reused)).toEqual([newer]); + }); +}); diff --git a/packages/core/src/retry/attempt-trail.ts b/packages/core/src/retry/attempt-trail.ts new file mode 100644 index 0000000..2885160 --- /dev/null +++ b/packages/core/src/retry/attempt-trail.ts @@ -0,0 +1,91 @@ +// SPDX-License-Identifier: MIT +// packages/core/src/retry/attempt-trail.ts + +/** + * RETRY-34's prior-attempt trail, held in a side table keyed by the surfaced throwable rather than + * written onto it. + * + * A `WeakMap` because the key is the error a caller is about to catch: the entry has to disappear + * when that error does, and a `Map` here would pin every failed request's error graph -- including + * whatever the buffered `HttpStatusError` bodies hold -- for the life of the process. + */ +const attemptTrails = new WeakMap(); + +/** One shared frozen empty list, so the common "no trail" answer allocates nothing. */ +const NO_ATTEMPTS: readonly unknown[] = Object.freeze([]); + +/** + * The subset of throwables a `WeakMap` can key on. Objects and functions qualify; primitives do + * not, and a registered symbol throws when used as a weak key, so symbols are excluded outright + * rather than probed. + */ +function trailKey(value: unknown): object | undefined { + if (typeof value === 'function') return value; + return typeof value === 'object' && value !== null ? value : undefined; +} + +/** + * Records the errors of the earlier attempts against the error the retry engine is about to + * surface (`RETRY-34`). + * + * Written into a side table instead of onto the error for three reasons the engine cannot rule out + * about a throwable it did not construct: it may be frozen or otherwise non-extensible, so a + * `defineProperty` in the failure path would itself throw and replace the failure the caller cares + * about; it may be a primitive, which can carry nothing at all; and `suppressed` already means + * "the one secondary" on `SuppressedErrorLike`, so reusing the name would collide with + * `RECOV-12`'s pairing. + * + * `attempts` MUST already have the surfaced instance filtered out -- `RETRY-34`'s skip-self clause, + * applied by the engine's `attachTrail` (`engine.ts`), which is the one caller. An empty + * `attempts` DELETES any entry a previous run left, so a transport that reuses a single error + * instance across calls reports the trail of the run that just surfaced it rather than a stale one. + * + * The list is copied and frozen, so the engine's own mutable `trail` array cannot be observed + * growing after the fact. + * + * @param error - the throwable the engine is surfacing; a primitive is silently ignored. + * @param attempts - the earlier attempts' errors, oldest first, surfaced instance excluded. + * + * @internal + */ +export function recordAttempts( + error: unknown, + attempts: readonly unknown[], +): void { + const key = trailKey(error); + if (key === undefined) return; + if (attempts.length === 0) { + attemptTrails.delete(key); + return; + } + attemptTrails.set(key, Object.freeze([...attempts])); +} + +/** + * The errors of the attempts that came before the one you caught. + * + * The retry pillar surfaces the **final** attempt's own error, unwrapped: `instanceof` against it + * answers the same for one attempt as for ten, and a cancellation that ended a backoff wait arrives + * as `CancellationError` rather than as something carrying one. The earlier attempts are not + * discarded — they are recorded here, so `retryAttempts(caught).length + 1` is how many sends the + * pillar made. A worked example is in `docs/sdk-documentation/pipelines.md`. + * + * Oldest first, and the error you passed in is never a member of its own trail (`RETRY-34`'s + * skip-self clause, which matters because a transport may reuse one error instance across + * attempts). A run that succeeded, a failure that was never retried, and any error this SDK did not + * surface from a retry loop all answer with an empty list — this never throws and never returns + * `undefined`. + * + * The result is frozen, and it is read by identity: an error reached through another error's + * `cause` has its own trail or none, never its wrapper's. + * + * @param error - the throwable a retrying pipeline surfaced; any value, not necessarily an `Error`. + * @returns the earlier attempts' errors, oldest first, or an empty list when there are none. + * + * @public + */ +export function retryAttempts(error: unknown): readonly unknown[] { + const key = trailKey(error); + if (key === undefined) return NO_ATTEMPTS; + return attemptTrails.get(key) ?? NO_ATTEMPTS; +} diff --git a/packages/core/src/retry/engine.test.ts b/packages/core/src/retry/engine.test.ts index cbb1919..55d6350 100644 --- a/packages/core/src/retry/engine.test.ts +++ b/packages/core/src/retry/engine.test.ts @@ -3,7 +3,9 @@ // Exercises: RETRY-7/8 (both axes gate), RETRY-20 (a hint replaces the schedule, unjittered), RETRY-22 // (a pacing failure never masks the upstream failure), RETRY-26/31 (cancellable wait, zero delay // inline), RETRY-27/RECOV-20 (total-timeout budget with per-attempt shrinking), RETRY-32 (no attempts -// after cancellation), RETRY-34 (suppressed trail on failure, discarded on success, skip-self), +// after cancellation), RETRY-34 (the prior-attempt trail rides BESIDE the surfaced error, discarded +// on success, skip-self), XCUT-1 (the surfaced type does not depend on how many attempts ran -- the +// final attempt's own error is what the engine hands back, cancellation included), // RETRY-35/RECOV-16 (body released before the wait, bounded buffering), RETRY-36/RECOV-19 (503,503,200 // terminates on the 200; a surviving response is returned LIVE), RETRY-39/40 (delay precedence; a // throwing override is non-fatal), RETRY-42/RECOV-28 (per-call state). @@ -15,10 +17,12 @@ import {Protocol} from '../http/protocol.js'; import {Request} from '../http/request.js'; import {Response} from '../http/response.js'; import {Status} from '../http/status.js'; -import {IoError} from '../io/errors.js'; +import {IoError, TransportFailureError} from '../io/errors.js'; import {failure, success, type Outcome} from '../recovery/outcome.js'; +import {CancellationError} from '../seams/transport.js'; import type {SuppressedErrorLike} from '../suppress.js'; import {countingResponse} from '../testing/fake-transport.js'; +import {retryAttempts} from './attempt-trail.js'; import {runWithRetry, type RetryConfig, type RetryDispatch} from './engine.js'; import {retrySettings, type RetrySettings} from './settings.js'; @@ -29,9 +33,13 @@ const BARE_POST = Request.newBuilder() .build(); /** - * The suppressed pair is asserted on SHAPE, never `instanceof SuppressedError`: the native class is - * absent on this package's Node floor (>=20.3), where `suppress()` returns a structural stand-in and - * an `instanceof` assertion would silently assert nothing. + * The one remaining pairing this engine can build is RECOV-12's -- a release failure riding along + * with the primary it must not mask. The retry TRAIL is no longer a `SuppressedError` chain, so the + * only assertions below that use this predicate are the release ones. + * + * Asserted on SHAPE, never `instanceof SuppressedError`: the native class is absent on this + * package's Node floor (>=20.3), where `suppress()` returns a structural stand-in and an + * `instanceof` assertion would silently assert nothing. */ function isSuppressedShape(value: unknown): value is SuppressedErrorLike { return ( @@ -364,12 +372,11 @@ describe('cancellation (RETRY-26/32)', () => { }); }); -describe('suppressed trail (RETRY-34)', () => { - test('prior attempt failures ride along as suppressed on the surfaced error', async () => { - const dispatch = scriptedDispatch([ - failure(new IoError('first')), - failure(new IoError('second')), - ]); +describe('the prior-attempt trail (RETRY-34)', () => { + test('the FINAL attempt error is surfaced, with the priors reachable beside it', async () => { + const first = new IoError('first'); + const second = new IoError('second'); + const dispatch = scriptedDispatch([failure(first), failure(second)]); const outcome = await runWithRetry( GET, @@ -379,12 +386,45 @@ describe('suppressed trail (RETRY-34)', () => { expect(outcome.kind).toBe('failure'); if (outcome.kind !== 'failure') return; - expect(isSuppressedShape(outcome.error)).toBe(true); + // Not a wrapper: the surfaced value IS attempt 2's own error, so a caller's `instanceof` reads + // the same here as it does after a single attempt. + expect(outcome.error).toBe(second); + expect(retryAttempts(outcome.error)).toEqual([first]); + }); + + test('the surfaced TYPE does not depend on how many attempts ran (XCUT-1)', async () => { + const one = await runWithRetry( + GET, + scriptedDispatch([failure(new TransportFailureError('refused'))]), + configOf({maxAttempts: 1, fixedDelayMs: 0}), + ); + // Distinct instances, because `scriptedDispatch` repeats its last entry and RETRY-34's + // skip-self guard would otherwise empty the trail this row needs to be non-empty. + const three = await runWithRetry( + GET, + scriptedDispatch([ + failure(new TransportFailureError('refused')), + failure(new TransportFailureError('refused')), + failure(new TransportFailureError('refused')), + ]), + configOf({maxAttempts: 3, fixedDelayMs: 0}), + ); + + expect(one.kind).toBe('failure'); + expect(three.kind).toBe('failure'); + if (one.kind !== 'failure' || three.kind !== 'failure') return; + // Until 2026-09-05 the second of these was a `SuppressedError` and this row was false: the + // surfaced CLASS was a function of the attempt budget, which is what XCUT-1's conformance + // clause ("assert the surfaced error is the cancellation type") catches on the abort path. + expect(one.error).toBeInstanceOf(TransportFailureError); + expect(three.error).toBeInstanceOf(TransportFailureError); + expect(retryAttempts(three.error)).toHaveLength(2); }); test('the trail is discarded entirely on eventual success', async () => { + const first = new IoError('first'); const dispatch = scriptedDispatch([ - failure(new IoError('first')), + failure(first), success(countingResponse(200).response), ]); @@ -395,10 +435,12 @@ describe('suppressed trail (RETRY-34)', () => { ); expect(outcome.kind).toBe('success'); + // Nothing was recorded anywhere: the discarded failure carries no trail of its own either. + expect(retryAttempts(first)).toEqual([]); }); }); -describe('suppressed trail -- skip-self and single-attempt shapes (RETRY-34)', () => { +describe('the trail -- skip-self and single-attempt shapes (RETRY-34)', () => { test('a reused instance never suppresses itself (RETRY-34 skip-self)', async () => { const reused = new IoError('same instance every time'); const dispatch = scriptedDispatch([failure(reused)]); @@ -410,6 +452,8 @@ describe('suppressed trail -- skip-self and single-attempt shapes (RETRY-34)', ( ); expect(outcome).toEqual(failure(reused)); + // Skip-self leaves nothing at all for a transport that reuses one instance across attempts. + expect(retryAttempts(reused)).toEqual([]); }); test('a single failed attempt surfaces its error unwrapped', async () => { @@ -419,6 +463,7 @@ describe('suppressed trail -- skip-self and single-attempt shapes (RETRY-34)', ( expect(await runWithRetry(GET, dispatch, configOf())).toEqual( failure(only), ); + expect(retryAttempts(only)).toEqual([]); }); test('a discarded 503 becomes a buffered HttpStatusError in the trail (RECOV-16)', async () => { @@ -438,10 +483,8 @@ describe('suppressed trail -- skip-self and single-attempt shapes (RETRY-34)', ( expect(outcome.kind).toBe('failure'); if (outcome.kind !== 'failure') return; - expect(isSuppressedShape(outcome.error)).toBe(true); - if (!isSuppressedShape(outcome.error)) return; - expect(outcome.error.error).toBeInstanceOf(IoError); - expect(outcome.error.suppressed).toBeInstanceOf(HttpStatusError); + expect(outcome.error).toBeInstanceOf(IoError); + expect(retryAttempts(outcome.error)[0]).toBeInstanceOf(HttpStatusError); }); }); @@ -469,13 +512,10 @@ describe('a discarded response OUTSIDE the 400-599 band (V14/N2, XCUT-8)', () => expect(outcome.kind).toBe('failure'); if (outcome.kind !== 'failure') return; - expect(isSuppressedShape(outcome.error)).toBe(true); - if (!isSuppressedShape(outcome.error)) return; - expect(outcome.error.suppressed).toBeInstanceOf( - RetryDiscardedResponseError, - ); - expect(outcome.error.suppressed).not.toBeInstanceOf(HttpStatusError); - expect((outcome.error.suppressed as {status: number}).status).toBe(200); + const [discarded] = retryAttempts(outcome.error); + expect(discarded).toBeInstanceOf(RetryDiscardedResponseError); + expect(discarded).not.toBeInstanceOf(HttpStatusError); + expect((discarded as {status: number}).status).toBe(200); }); test('a discarded 404 still becomes an HttpStatusError -- the band is unchanged', async () => { @@ -496,9 +536,7 @@ describe('a discarded response OUTSIDE the 400-599 band (V14/N2, XCUT-8)', () => expect(outcome.kind).toBe('failure'); if (outcome.kind !== 'failure') return; - expect(isSuppressedShape(outcome.error)).toBe(true); - if (!isSuppressedShape(outcome.error)) return; - expect(outcome.error.suppressed).toBeInstanceOf(HttpStatusError); + expect(retryAttempts(outcome.error)[0]).toBeInstanceOf(HttpStatusError); }); }); @@ -599,9 +637,7 @@ describe('the inter-attempt wait -- degenerate and hostile delays', () => { // instead of silently becoming an extra attempt. expect(outcome.kind).toBe('failure'); if (outcome.kind !== 'failure') return; - expect(isSuppressedShape(outcome.error)).toBe(true); - if (!isSuppressedShape(outcome.error)) return; - expect(outcome.error.error).toBeInstanceOf(RangeError); + expect(outcome.error).toBeInstanceOf(RangeError); expect(dispatch.calls).toHaveLength(1); }); }); @@ -663,6 +699,30 @@ describe('cancellation while an attempt is in flight (RETRY-32)', () => { }); }); +describe('a cancelled backoff surfaces the cancellation TYPE (XCUT-1)', () => { + test('the surfaced error is CancellationError, with the prior attempt beside it', async () => { + const controller = new AbortController(); + const first = new IoError('reset'); + const dispatch: RetryDispatch = () => { + controller.abort(); + return Promise.resolve(failure(first)); + }; + + const outcome = await runWithRetry(GET, dispatch, { + ...configOf({fixedDelayMs: 60_000, maxAttempts: 5}), + signal: controller.signal, + }); + + expect(outcome.kind).toBe('failure'); + if (outcome.kind !== 'failure') return; + // `abortToSdkError` maps the abort to this type one line before the trail is attached; until + // 2026-09-05 the trail wrapper undid the mapping immediately, and a cancelled backoff ALWAYS + // has a non-empty trail -- so `instanceof CancellationError` was false for every one of them. + expect(outcome.error).toBeInstanceOf(CancellationError); + expect(retryAttempts(outcome.error)).toEqual([first]); + }); +}); + describe('a throwing attempt still carries the trail (RETRY-33/34)', () => { test('a throw from inside the attempt is folded into a failure outcome, trail intact', async () => { const calls: number[] = []; @@ -681,16 +741,14 @@ describe('a throwing attempt still carries the trail (RETRY-33/34)', () => { expect(calls).toEqual([1, 2]); expect(outcome.kind).toBe('failure'); if (outcome.kind !== 'failure') return; - expect(isSuppressedShape(outcome.error)).toBe(true); - if (!isSuppressedShape(outcome.error)) return; - expect(outcome.error.error).toBeInstanceOf(RangeError); + expect(outcome.error).toBeInstanceOf(RangeError); // RETRY-34: attempt 1's failure would have been lost had the throw escaped as a rejection. - expect((outcome.error.suppressed as Error).message).toBe('first'); + expect((retryAttempts(outcome.error)[0] as Error).message).toBe('first'); }); }); -describe('the suppressed trail with more than two entries (RETRY-34)', () => { - test('three distinct attempt failures fold into a nested chain', async () => { +describe('the trail with more than two entries (RETRY-34)', () => { + test('three distinct attempt failures list flat, oldest first', async () => { const dispatch = scriptedDispatch([ failure(new IoError('first')), failure(new IoError('second')), @@ -705,15 +763,12 @@ describe('the suppressed trail with more than two entries (RETRY-34)', () => { expect(outcome.kind).toBe('failure'); if (outcome.kind !== 'failure') return; - expect(isSuppressedShape(outcome.error)).toBe(true); - if (!isSuppressedShape(outcome.error)) return; - expect((outcome.error.error as Error).message).toBe('third'); - // The two priors folded into a nested pair, oldest innermost. - const folded = outcome.error.suppressed; - expect(isSuppressedShape(folded)).toBe(true); - if (!isSuppressedShape(folded)) return; - expect((folded.error as Error).message).toBe('second'); - expect((folded.suppressed as Error).message).toBe('first'); + expect((outcome.error as Error).message).toBe('third'); + // A flat list in wire order. The `SuppressedError` pair is binary, so N priors used to fold + // into a nested chain a caller had to walk; nothing about RETRY-34 asked for that shape. + expect( + retryAttempts(outcome.error).map(entry => (entry as Error).message), + ).toEqual(['first', 'second']); }); }); diff --git a/packages/core/src/retry/engine.ts b/packages/core/src/retry/engine.ts index 0403d76..a394c17 100644 --- a/packages/core/src/retry/engine.ts +++ b/packages/core/src/retry/engine.ts @@ -8,7 +8,7 @@ import type {Request} from '../http/request.js'; import type {Response} from '../http/response.js'; import {failure, type Outcome} from '../recovery/outcome.js'; import {releaseQuietly, withReleaseFailure} from '../recovery/release.js'; -import {suppress} from '../suppress.js'; +import {recordAttempts} from './attempt-trail.js'; import {stampAttempt} from './attempt-stamp.js'; import {computeDelay} from './backoff.js'; import {RetryDiscardedResponseError} from './errors.js'; @@ -247,27 +247,35 @@ async function decideRetry( } /** - * RETRY-34: prior failures ride along as `suppressed` on the surfaced error; the surfaced instance - * itself is skipped, so a reused throwable cannot suppress itself. On success the trail is discarded - * whole. + * RETRY-34: prior failures ride ALONGSIDE the surfaced error, recorded in `attempt-trail.ts`'s side + * table and read back through the public `retryAttempts()`. The surfaced instance itself is skipped, + * so a reused throwable never appears in its own trail. On success the trail is discarded whole -- + * nothing is written, and the outcome is returned untouched. * - * The suppressed pair is a binary shape, so N entries fold into a nested chain. Built through Phase - * 4b's `suppress()` helper rather than `new SuppressedError(...)`: the native class reached Node only - * in 24.0.0 and this package's floor is `>=20.3`, so the direct form neither type-checks nor runs - * there. Argument order is controlled explicitly -- native `using` disposal builds the pair the other - * way round, making the *later* error primary. + * **The outcome's error is returned unchanged, class and identity intact.** Until 2026-09-05 this + * function wrapped it in a `SuppressedError` pair instead, which made the surfaced TYPE a function of + * how many attempts ran: one attempt surfaced `TransportFailureError`, three surfaced a wrapper with + * the `TransportFailureError` at `.error`. XCUT-1's conformance clause -- "assert the surfaced error + * is the cancellation type" -- is the row that catches it, because a cancellation during backoff + * ALWAYS has a non-empty trail: `abortToSdkError` maps the abort to `CancellationError` below, and + * the wrapper undid that mapping on the very next line. RETRY-34 asks for the prior failures to be + * "attached to the surfaced exception", which is the JVM's `addSuppressed` -- the exception stays + * what it is and grows a list -- not for the exception to be replaced by a container. + * + * `suppress()` keeps its RECOV-12 job elsewhere in this file: `withReleaseFailure` pairs a release + * failure with the primary it must not mask. That is a genuine two-value pairing; an N-entry attempt + * history folded into a binary shape was never one. */ -function withTrail( +function attachTrail( outcome: Outcome, trail: readonly unknown[], ): Outcome { if (outcome.kind === 'success') return outcome; - const prior = trail.filter(entry => entry !== outcome.error); - if (prior.length === 0) return outcome; - const folded = prior.reduce((accumulated, entry) => - suppress(entry, accumulated, 'earlier retry attempt failed'), + recordAttempts( + outcome.error, + trail.filter(entry => entry !== outcome.error), ); - return failure(suppress(outcome.error, folded, 'retry attempts exhausted')); + return outcome; } /** @@ -346,12 +354,13 @@ function maybeEmitExhausted( * iterative, so N retries build no continuation chain and no stack growth. RETRY-33's "every * terminal path returns an Outcome" is honored literally -- an attempt that throws is folded into a * failure outcome carrying the trail, rather than left to surface as a bare rejected promise that - * would drop RETRY-34's suppressed attempts on the floor. + * would drop RETRY-34's prior attempts on the floor. * * @param request - the captured template every attempt re-sends. * @param dispatch - performs one attempt and reports its outcome without throwing. * @param config - settings, clock, randomness, signal, and the optional delay override. - * @returns the terminal outcome, with RETRY-34's suppressed trail attached on failure. + * @returns the terminal outcome. On failure the error is the FINAL attempt's own, unwrapped, with + * RETRY-34's prior-attempt trail recorded beside it for `retryAttempts()`. * * @internal */ @@ -383,7 +392,7 @@ export async function runWithRetry( // and silently missed a cancelled backoff. The raw reason is kept as `.cause`. if (config.signal?.aborted === true) { const cancellation = abortToSdkError(config.signal, config.signal.reason); - return withTrail(failure(cancellation), trail); + return attachTrail(failure(cancellation), trail); } try { @@ -396,7 +405,7 @@ export async function runWithRetry( const decision = await runAttempt(dispatch, state); if (decision.kind === 'stop') { maybeEmitExhausted(decision.outcome, trail.length, state); - return withTrail(decision.outcome, trail); + return attachTrail(decision.outcome, trail); } trail.push(decision.error); @@ -418,7 +427,7 @@ export async function runWithRetry( // `stampAttempt`'s header build, `toHttpError`'s body drain, and a misbehaving injected // clock's `sleep` -- and letting any of them escape would discard the whole suppressed trail // RETRY-34 requires the surfaced failure to carry. - return withTrail(failure(error), trail); + return attachTrail(failure(error), trail); } } } diff --git a/packages/core/src/retry/errors.ts b/packages/core/src/retry/errors.ts index ed0ba07..bfaeccc 100644 --- a/packages/core/src/retry/errors.ts +++ b/packages/core/src/retry/errors.ts @@ -7,8 +7,9 @@ import {DexpaceError} from '../http/errors.js'; * * Reachable only when a caller widens `RetrySettings.retryableStatuses` to include a non-error code: * the engine then retries a 2xx or 3xx, and every response it discards still owes `RETRY-34` an - * entry in the suppressed trail. `toHttpError` correctly returns `null` for such a status - * (`BODY-31` hands a non-error response back intact), so there is nothing for it to build. + * entry in the prior-attempt trail `retryAttempts()` reads. `toHttpError` correctly returns `null` + * for such a status (`BODY-31` hands a non-error response back intact), so there is nothing for it + * to build. * * Until 2026-09-02 the engine fabricated `new HttpStatusError(200, …)` here — precisely the * "successful exception" `XCUT-8` forbids, constructed by core itself, and contradicting diff --git a/packages/core/src/retry/retry-dispatch.ts b/packages/core/src/retry/retry-dispatch.ts index b87b29e..9672fca 100644 --- a/packages/core/src/retry/retry-dispatch.ts +++ b/packages/core/src/retry/retry-dispatch.ts @@ -42,7 +42,9 @@ function attemptVia(config: RetryDispatchConfig): RetryDispatch { * @param request - the request to prepare, send, and possibly re-send. * @param config - the recovery chains, transport, and retry policy. * @returns the response of the terminal successful attempt. - * @throws Whatever the terminal Failure carries, with RETRY-34's suppressed trail attached. + * @throws Whatever the FINAL attempt failed with, unwrapped -- the same class a single-attempt run + * would have thrown. RETRY-34's earlier attempts are recorded beside it and read back through + * `retryAttempts()`. * * @internal */ diff --git a/packages/core/src/retry/retry-step.ts b/packages/core/src/retry/retry-step.ts index da146c0..3199311 100644 --- a/packages/core/src/retry/retry-step.ts +++ b/packages/core/src/retry/retry-step.ts @@ -113,6 +113,14 @@ function configFrom( * `ctx.fork` is asserted rather than checked -- RETRY is in `PILLAR_STAGES`, so its absence means the * descriptor was installed somewhere it cannot be, which is a programmer error. * + * **What it throws when it gives up is the FINAL attempt's own error, unwrapped.** The class you + * catch does not depend on how many attempts ran: a transport failure surfaces as + * `TransportFailureError` whether `maxAttempts` was 1 or 3, and an abort that ended a backoff wait + * surfaces as `CancellationError` (`XCUT-1`). The earlier attempts' errors are not lost -- read them + * with `retryAttempts(caught)`, oldest first (`RETRY-34`). A response the loop discards is always + * closed first; the response that ENDS the loop is returned live and unread, and closing it is + * yours. + * * @param options - settings overrides and the injected clock, randomness, and delay override. * @returns the descriptor to install in a pipeline's RETRY slot. * From 32c9f2b9141022dba7ae29934c39ac4a7ee96730 Mon Sep 17 00:00:00 2001 From: Mohammad Wahbeh Date: Sat, 5 Sep 2026 10:48:18 +0300 Subject: [PATCH 2/5] test(xcut): conformance rows for the surfaced retry error and its trail Four rows across the two suites the issue names, all four red against the parent commit's engine and green against this one: retry-safety.conformance.test.ts - one GET at a closed port surfaces `TransportFailureError` for maxAttempts 1 AND for 3; the dispatch count is what proves the budgets actually differed (XCUT-1) - the two priors are reachable through `retryAttempts()`, oldest first, with the surfaced instance excluded (RETRY-34) cancellation-and-timeout.conformance.test.ts - `expect(surfaced).toBeInstanceOf(CancellationError)` at the TOP level, which is XCUT-1's conformance clause unqualified. The existing row asserted only on a chain walk, and its own comment said why: the top level was the `SuppressedError`. A caller writing `catch` has no walk. - the 500 that provoked the retry is still reachable in the trail as a buffered `HttpStatusError` (RETRY-34/RECOV-16) `chainOf`'s doc comment is rewritten rather than deleted: the walk still earns its place proving the raw abort survived as `cause` and that no `TimeoutError` hides one hop down (XCUT-3), but it is no longer the only assertion. Refs #72, #67. --- ...ncellation-and-timeout.conformance.test.ts | 65 ++++++++++++++---- .../xcut/retry-safety.conformance.test.ts | 67 ++++++++++++++++++- 2 files changed, 117 insertions(+), 15 deletions(-) diff --git a/tests/conformance/xcut/cancellation-and-timeout.conformance.test.ts b/tests/conformance/xcut/cancellation-and-timeout.conformance.test.ts index c1a1952..a97008a 100644 --- a/tests/conformance/xcut/cancellation-and-timeout.conformance.test.ts +++ b/tests/conformance/xcut/cancellation-and-timeout.conformance.test.ts @@ -9,7 +9,12 @@ // These run the invariants through the fully composed retry+redirect+auth+logging pipeline over a // real socket, which is what this suite adds over 5a's and Phase 2's own unit-level coverage. import {afterAll, beforeAll, describe, expect, test} from 'bun:test'; -import {CancellationError, Request} from '@dexpace/core'; +import { + CancellationError, + HttpStatusError, + Request, + retryAttempts, +} from '@dexpace/core'; import {buildComposedPipeline} from './fixtures/composed-pipeline.js'; import {startFixtureServer, type XcutFixtureServer} from './fixtures/server.js'; import {rejectionOf} from './fixtures/settle.js'; @@ -20,10 +25,14 @@ let server: XcutFixtureServer; * Walks everything a surfaced failure can nest a prior error under, visiting by identity so a cyclic * chain terminates -- the same discipline `XCUT-9` puts on the classifier. * - * The walk is necessary because 5a's engine folds the retry trail into a `SuppressedError` - * (`RETRY-34`), so the cancellation that ended a backoff wait arrives as `.error` beneath a wrapper - * rather than as the top-level throwable. Asserting on the top level alone would test the wrapping, - * not the invariant. + * Kept after #72 made the top-level assertion possible, because the two answer different questions. + * `CancellationError` carries the raw `AbortSignal.reason` as its `cause`, so the walk is what + * proves the ambient abort was not swallowed on the way out, and it is also what catches a + * `TimeoutError` hiding one hop down where `XCUT-3` forbids one. What it must no longer be is the + * ONLY assertion: 5a's engine folded the retry trail into a `SuppressedError` (`RETRY-34`), the + * cancellation arrived as `.error` beneath that wrapper, and a chain walk was the only way to find + * it -- which is precisely the defect, since a caller writing `catch (e) { e instanceof + * CancellationError }` has no walk. Every row below asserts the top level too. */ function* chainOf(error: unknown): Generator { const seen = new Set(); @@ -176,13 +185,17 @@ describe('XCUT-3: an inter-attempt wait is promptly cancellable', () => { }, 50).unref(); const surfaced = await rejectionOf(pending); - // Asserted on the chain rather than the top-level type, because the top level is a - // `SuppressedError` pairing the cancellation with the prior attempt's failure. What the chain - // must carry is the SDK's OWN type: until 2026-09-02 the retry engine surfaced `signal.reason` - // verbatim, so a cancelled backoff arrived as a bare DOMException `AbortError` while the - // transport path mapped the identical abort to `CancellationError` -- one requirement, two - // types, depending on which layer noticed. Both layers now map through the same shape, so this - // asserts the type as well as XCUT-3's letter. + // XCUT-1's conformance clause, at the top level and unqualified: "assert the surfaced error is + // the cancellation type". Two separate defects had to be fixed for this line to hold. Until + // 2026-09-02 the engine surfaced `signal.reason` verbatim, so a cancelled backoff arrived as a + // bare DOMException `AbortError` while the transport mapped the identical abort to + // `CancellationError` -- one requirement, two types, depending on which layer noticed. Until + // 2026-09-05 the mapping was then undone one line later by the retry trail's `SuppressedError` + // wrapper, and a cancelled backoff ALWAYS has a non-empty trail, so this row was false for + // every reachable case. + expect(surfaced).toBeInstanceOf(CancellationError); + // The chain still has to carry the raw abort (the ambient flag survived the mapping) and must + // not carry a timeout, which is XCUT-3's own letter. expect(carriesCancellation(surfaced)).toBe(true); expect(carriesSdkCancellationError(surfaced)).toBe(true); expect(carriesTimeout(surfaced)).toBe(false); @@ -211,3 +224,31 @@ describe('XCUT-3: an inter-attempt wait is promptly cancellable', () => { await pipeline.close(); }); }); + +describe('RETRY-34: the trail survives the unwrapped cancellation', () => { + test('the attempt the cancelled wait was scheduled for stays reachable', async () => { + const pipeline = buildComposedPipeline({ + retry: {settings: {maxAttempts: 5, initialDelayMs: 60_000}}, + }); + const controller = new AbortController(); + const pending = pipeline.runtime.send( + Request.newBuilder().url(`${server.url}/fail-500`).build(), + undefined, + controller.signal, + ); + setTimeout(() => { + controller.abort(); + }, 50).unref(); + const surfaced = await rejectionOf(pending); + + // Surfacing the cancellation unwrapped is not allowed to LOSE the 500 that provoked the retry + // in the first place -- that was the one thing the `SuppressedError` wrapper did buy. It rides + // in the trail instead, and the retired response is a buffered `HttpStatusError` (RECOV-16). + const priors = retryAttempts(surfaced); + expect(priors).toHaveLength(1); + expect(priors[0]).toBeInstanceOf(HttpStatusError); + expect((priors[0] as HttpStatusError).status).toBe(500); + + await pipeline.close(); + }); +}); diff --git a/tests/conformance/xcut/retry-safety.conformance.test.ts b/tests/conformance/xcut/retry-safety.conformance.test.ts index 67aa4d7..0be9dd0 100644 --- a/tests/conformance/xcut/retry-safety.conformance.test.ts +++ b/tests/conformance/xcut/retry-safety.conformance.test.ts @@ -2,15 +2,25 @@ // tests/conformance/xcut/retry-safety.conformance.test.ts // Exercises: XCUT-10 (retry-SAFETY is decided at the retry step independently of retryability, and // applies uniformly to protocol AND transport failures -- the gate must not special-case a transport -// error that never reached the server). +// error that never reached the server), XCUT-1 (the class of the surfaced error does not depend on +// how many attempts the pillar spent), RETRY-34 (the earlier attempts stay reachable beside it). // -// The five rows are the ones XCUT-10's own conformance clause names, run for the first time against +// The five XCUT-10 rows are the ones its own conformance clause names, run for the first time against // the composed pipeline rather than 5a's unit-level harness. Each asserts on dispatches that actually // reached the terminal transport, which is the only vantage point where "was it re-sent?" is visible. +// The two XCUT-1/RETRY-34 rows below need the same vantage point for the opposite reason: the budget +// they vary is only observable as a dispatch count. import {afterAll, beforeAll, describe, expect, test} from 'bun:test'; -import {Request, streamBody, stringBody} from '@dexpace/core'; +import { + Request, + retryAttempts, + streamBody, + stringBody, + TransportFailureError, +} from '@dexpace/core'; import {buildComposedPipeline} from './fixtures/composed-pipeline.js'; import {startFixtureServer, type XcutFixtureServer} from './fixtures/server.js'; +import {rejectionOf} from './fixtures/settle.js'; let server: XcutFixtureServer; @@ -27,6 +37,16 @@ function retrying(): {settings: {maxAttempts: number; initialDelayMs: number}} { return {settings: {maxAttempts: 3, initialDelayMs: 1}}; } +/** + * A GET at a closed port. Retry-SAFE (idempotent, body-less) and retryABLE (the transports map a + * refused connection to `TransportFailureError`, an `IoError`), so the pillar spends its whole + * budget and every attempt fails the same way -- which is what makes the surfaced CLASS the only + * variable between the two budgets below. + */ +function unreachable(): Request { + return Request.newBuilder().url('http://127.0.0.1:1/').method('GET').build(); +} + describe('XCUT-10: retry-safety on a body-less request follows method idempotence', () => { test('retries a body-less GET against a retryable protocol failure', async () => { const pipeline = buildComposedPipeline({retry: retrying()}); @@ -104,3 +124,44 @@ describe('XCUT-10: retry-safety on a body-bearing request follows body replayabi await pipeline.close(); }); }); + +describe('XCUT-1/RETRY-34: what a retrying pipeline surfaces when it gives up', () => { + test('the same failure surfaces as TransportFailureError for maxAttempts 1 and for 3', async () => { + const once = buildComposedPipeline({ + retry: {settings: {maxAttempts: 1}}, + }); + const thrice = buildComposedPipeline({retry: retrying()}); + + const afterOne = await rejectionOf(once.runtime.send(unreachable())); + const afterThree = await rejectionOf(thrice.runtime.send(unreachable())); + + expect(once.dispatches()).toBe(1); + expect(thrice.dispatches()).toBe(3); + // The row #72 exists for. Until 2026-09-05 the three-attempt case surfaced a `SuppressedError` + // holding the transport failure at `.error`, so one condition had two surfaced classes and the + // discriminator was the attempt budget -- something no caller writing `catch` can see. + expect(afterOne).toBeInstanceOf(TransportFailureError); + expect(afterThree).toBeInstanceOf(TransportFailureError); + + await once.close(); + await thrice.close(); + }); + + test('the earlier attempts are reachable beside it, oldest first, self excluded', async () => { + const pipeline = buildComposedPipeline({retry: retrying()}); + + const surfaced = await rejectionOf(pipeline.runtime.send(unreachable())); + + // RETRY-34 through the composed pipeline: three sends, so two priors, and the surfaced instance + // is not a member of its own trail. + const priors = retryAttempts(surfaced); + expect(pipeline.dispatches()).toBe(3); + expect(priors).toHaveLength(2); + expect(priors.every(prior => prior instanceof TransportFailureError)).toBe( + true, + ); + expect(priors).not.toContain(surfaced); + + await pipeline.close(); + }); +}); From 0778a882407aaa62c5072c02d62091a36fc1cb50 Mon Sep 17 00:00:00 2001 From: Mohammad Wahbeh Date: Sat, 5 Sep 2026 10:48:18 +0300 Subject: [PATCH 3/5] docs: the retry pillar throws the last attempt's error, and retryAttempts reads the rest - pipelines.md gains the note as the FIRST of the retry bullets, with a worked example that `check-fences.mjs` typechecks against the built package. The lead sentence said "two notes each"; it is three now. - errors.md: the `CancellationError`/`TransportFailureError` narrowing section states outright that installing a retry pillar does not change either check, which is the whole point of #72. The `RetryDiscardedResponseError` row at :188 stops calling the trail "suppressed". - write-a-response-handler.md: RECOV-12's release pairing is now documented as the ONLY reason this SDK builds a `SuppressedError`. Verified against every `suppress()` and `withReleaseFailure()` call site in packages/core/src, all of which are a close() that threw with an error already in flight. Both new cross-links resolve against pipelines.md's real headings (checked by script, not by eye). Refs #72, #67. --- docs/sdk-documentation/errors.md | 7 +++- docs/sdk-documentation/pipelines.md | 38 ++++++++++++++++++- .../write-a-response-handler.md | 12 ++++++ 3 files changed, 54 insertions(+), 3 deletions(-) diff --git a/docs/sdk-documentation/errors.md b/docs/sdk-documentation/errors.md index b9985fa..fd62950 100644 --- a/docs/sdk-documentation/errors.md +++ b/docs/sdk-documentation/errors.md @@ -112,6 +112,11 @@ export async function call(): Promise { the two apart at the point of abort — which is exactly how a transport decides which of the two errors to raise. +**Neither narrowing changes when a retry pillar is installed.** What `retryStep` throws once it gives +up is the final attempt's own error, unwrapped, so the two `instanceof` checks above read the same at +`maxAttempts: 1` and at `maxAttempts: 3`. The earlier attempts ride beside it and are read with +`retryAttempts(error)` — see [`pipelines.md`](./pipelines.md#the-four-shipped-pillars). + ## Narrowing helpers Three predicates exist for the cases where `instanceof` on a union is tedious: @@ -185,7 +190,7 @@ Two more joined them on the same date, both from `XCUT-8`'s "never fabricate a s | Error | Raised by | |---|---| | `HttpStatusValidationError` | `new HttpStatusError(status, …)` when `status` is not an integer in 400–599. The constructor validated nothing before, so a consumer could build an `HttpStatusError` claiming a `200`. `toHttpError` is the total form — it returns `null` instead of throwing | -| `RetryDiscardedResponseError` | the retry engine's suppressed trail, for a response it discarded whose status is outside 400–599. Reachable only if you widen `RetrySettings.retryableStatuses` to include a non-error code; the trail previously said `HttpStatusError` for it, which claimed an HTTP failure that had not happened | +| `RetryDiscardedResponseError` | the retry engine's prior-attempt trail (`retryAttempts()`), for a response it discarded whose status is outside 400–599. Reachable only if you widen `RetrySettings.retryableStatuses` to include a non-error code; the trail previously said `HttpStatusError` for it, which claimed an HTTP failure that had not happened | All of them descend from `DexpaceError`, so the broad catch works too. diff --git a/docs/sdk-documentation/pipelines.md b/docs/sdk-documentation/pipelines.md index 1336c5e..f6fe142 100644 --- a/docs/sdk-documentation/pipelines.md +++ b/docs/sdk-documentation/pipelines.md @@ -153,8 +153,42 @@ All three carry the same `InstrumentationBundle`, so trace and span identity sur | Auth | `authStep(settings)` | `credentials`, `tiers`, `challengeHook`, `bearerMarginMs` — see [`auth.md`](./auth.md) | | Logging | `loggingStep(settings?)` | `granularity`, `severity`, `previewSizeBytes`, `droppedHeaderPolicy`, `logger`, `meter`, `tracerFactory` | -Retry and redirect are worth two notes each, because both surprise people: - +Retry and redirect are worth a few notes each, because both surprise people: + +- **What retry throws is the last attempt's own error.** The class you catch does not depend on how + many attempts ran: a refused connection is a `TransportFailureError` whether `maxAttempts` was 1 or + 3, and an abort that lands during a backoff wait is a `CancellationError` (`XCUT-1`). The earlier + attempts are not thrown away — `retryAttempts(caught)` returns them, oldest first, with the error + you passed in excluded from its own trail (`RETRY-34`): + + ```typescript + import { + retryAttempts, + TransportFailureError, + type Request, + type Runtime, + } from '@dexpace/core'; + + declare const runtime: Runtime; + declare const request: Request; + + export async function send(): Promise { + try { + await runtime.send(request); + } catch (error) { + if (error instanceof TransportFailureError) { + const spent = retryAttempts(error).length + 1; + console.error(`gave up after ${String(spent)} sends`, error.message); + } + throw error; + } + } + ``` + + The trail is a side table keyed by the error, not a property on it, so nothing is added to an + object you may not own; an error that never went through a retry loop answers with an empty list. + Until 2026-09-05 the pillar wrapped its terminal failure in a `SuppressedError` instead, which made + the surfaced class a function of the attempt budget. - **Retry pacing honours the server.** `Retry-After`, `X-RateLimit-Reset` and friends are parsed in a fixed precedence and win over computed backoff. Every computed delta is clamped to a 365-day ceiling that `RETRY-18` mandates — so a server that sends `X-RateLimit-Reset` in milliseconds diff --git a/docs/sdk-documentation/write-a-response-handler.md b/docs/sdk-documentation/write-a-response-handler.md index da5d364..7611147 100644 --- a/docs/sdk-documentation/write-a-response-handler.md +++ b/docs/sdk-documentation/write-a-response-handler.md @@ -82,6 +82,18 @@ release failure. **`instanceof SuppressedError` is not a valid test**: the class project's declared Node floor and a structurally identical stand-in is built there instead. Test the shape, or read `.error` unconditionally. +**A failed release is the only thing in this SDK that builds one.** Every site that constructs the +pairing — the response chain, the redirect and auth and retry pillars, the serde handlers, the SSE +stream, the paginator — is doing this one job: a `close()` that threw while an error was already in +flight. It is a genuine two-value shape, which is why it fits. + +The retry pillar used to build one for a *second* reason, folding N prior attempt errors into a +nested chain of pairs so that what you caught after three attempts was a wrapper rather than the +failure. It no longer does: it surfaces the last attempt's error unwrapped and records the earlier +ones beside it, read back with `retryAttempts()` (see [`pipelines.md`](./pipelines.md#the-four-shipped-pillars)). +So if you catch a `'SuppressedError'` from this SDK, `.suppressed` is a teardown failure, never an +earlier attempt. + **3. Only payload failures are re-typed.** `SERDE-12`: a malformed body or a shape mismatch becomes a `DeserializationError` with the original chained; a genuine stream failure propagates untouched, because re-wrapping it would tell a caller their payload was malformed when their socket dropped. From c377fea3cee76c992a9eac1684b81cc0437b2101 Mon Sep 17 00:00:00 2001 From: Mohammad Wahbeh Date: Sat, 5 Sep 2026 10:48:18 +0300 Subject: [PATCH 4/5] test(node): the retry trail case moves from "same wrapper" to "no wrapper" `tests/node-conformance/retry.test.mjs` names four runtime-divergent points in its header, and point 2 was RETRY-34's trail going through `suppress()` -- whose native-vs-fallback branch is decided by the runtime, and whose two legs this suite's matrix actually runs (`lts/*` has the global, the pinned `20.3.0` floor does not). #72 takes that branch off the retry path, so the case that asserted "the wrapper has the same shape on either runtime" would now be asserting a shape nothing builds. It becomes the stronger claim: neither leg produces a wrapper, `outcome.error` IS the last attempt's error, and `retryAttempts()` reads the trail through the `@dexpace/core` specifier and the built `dist/`, as a consumer does. The real-timer abort case gains one line: `outcome.error instanceof CancellationError`, over a real `AbortSignal` and a real `setTimeout`, which is the half the unit suite's injected clock cannot reach. OUT OF THE TASK FILE'S STATED PARTITION, deliberately and reported: the partition lists the two `tests/conformance/xcut/` files but not the Node tree, and this case asserts the exact behaviour D10 changes, so `test:node` (and therefore the preflight) is red without it. No wave-3 sibling touches this file -- D2 gives #74 auth plus transport-conformance and #75 rx. Refs #72, #67. --- tests/node-conformance/retry.test.mjs | 44 +++++++++++++++++++-------- 1 file changed, 31 insertions(+), 13 deletions(-) diff --git a/tests/node-conformance/retry.test.mjs b/tests/node-conformance/retry.test.mjs index 8fb48d7..c686241 100644 --- a/tests/node-conformance/retry.test.mjs +++ b/tests/node-conformance/retry.test.mjs @@ -9,9 +9,13 @@ // `AbortSignal.timeout()`, whose class and `name` are the runtime's, not this package's -- if // Node named it anything but `TimeoutError`, every timed-out request would silently stop being // retried and `bun test` would still be green. -// 2. RETRY-34's suppressed trail goes through `suppress()`, which picks the native `SuppressedError` -// or the shape-compatible fallback depending on the runtime. Bun has the global; the declared -// floor (`engines.node >=20.3`) does not. The trail's SHAPE has to be identical either way. +// 2. RETRY-34's trail used to go through `suppress()`, which picks the native `SuppressedError` or +// the shape-compatible fallback depending on the runtime -- Bun has the global, the declared +// floor (`engines.node >=20.3`) does not, and this suite's matrix runs both legs. #72 took that +// branch off the retry path entirely: the final attempt's own error is surfaced and the trail +// rides in a side table. So the assertion moved with it, from "the wrapper has the same shape on +// either runtime" to "neither runtime produces a wrapper", which is the stronger claim and the +// one a reintroduced `suppress()` would break differently on Node 20 than on Node 24. // 3. RETRY-35/RECOV-16's "release the discarded response" rides on Web Streams: a retired response is // drained to EOF by `toHttpError()`, an abandoned one is cancelled by `Response.close()`. Node's // `cancel()`/`pull()` timing is an independent implementation of Bun's. @@ -24,7 +28,14 @@ // `dist/` file path, per this suite's import rule. import assert from 'node:assert/strict'; import {describe, it} from 'node:test'; -import {Protocol, Request, Response, Status} from '@dexpace/core'; +import { + CancellationError, + Protocol, + Request, + Response, + retryAttempts, + Status, +} from '@dexpace/core'; import { isRetryableFailure, RETRYABLE_STATUSES, @@ -123,13 +134,12 @@ describe('retry classification on the declared Node floor', () => { }); describe('the retry engine on the declared Node floor', () => { - it('folds the suppressed trail into the same shape whether or not the runtime has SuppressedError', async () => { + it('surfaces the final attempt error itself, on a runtime that may lack SuppressedError', async () => { // Timeout aborts, because they are the retryable throwable this suite can build without reaching - // into another `dist/` module -- two of them exhaust the budget and produce a two-entry trail. - const dispatch = scriptedDispatch([ - failure(new DOMException('timed out', 'TimeoutError')), - failure(new DOMException('timed out again', 'TimeoutError')), - ]); + // into another `dist/` module -- two of them exhaust the budget and produce a one-entry trail. + const first = new DOMException('timed out', 'TimeoutError'); + const last = new DOMException('timed out again', 'TimeoutError'); + const dispatch = scriptedDispatch([failure(first), failure(last)]); const outcome = await runWithRetry( GET, @@ -139,9 +149,13 @@ describe('the retry engine on the declared Node floor', () => { assert.equal(dispatch.calls.length, 2); assert.equal(outcome.kind, 'failure'); - assert.equal(outcome.error.name, 'SuppressedError'); - assert.ok('error' in outcome.error); - assert.ok('suppressed' in outcome.error); + assert.equal(outcome.error, last); + assert.notEqual(outcome.error.name, 'SuppressedError'); + // RETRY-34's trail, read through the accessor as a CONSUMER reaches it -- the `@dexpace/core` + // specifier and the built `dist/`, not the engine's own module path. + const priors = retryAttempts(outcome.error); + assert.equal(priors.length, 1); + assert.equal(priors[0], first); }); it('releases a discarded response through the drain route, over Node Web Streams (RETRY-35)', async () => { @@ -217,5 +231,9 @@ describe('the retry engine on the declared Node floor', () => { assert.equal(outcome.kind, 'failure'); // The point of the case: it returned instead of sleeping out the full 60s backoff. assert.ok(defaultClock.monotonic() - startedAt < 5_000); + // XCUT-1 over a REAL AbortSignal and a REAL timer, which is the half the unit suite's injected + // clock cannot reach. The trail is non-empty here by construction, so before #72 this was a + // `SuppressedError` and the assertion below was false. + assert.ok(outcome.error instanceof CancellationError); }); }); From 4578841168ba499a21776dcc26ff19996433e556 Mon Sep 17 00:00:00 2001 From: Mohammad Wahbeh Date: Sat, 5 Sep 2026 10:48:18 +0300 Subject: [PATCH 5/5] =?UTF-8?q?docs(retry):=20the=20trail=20is=20not=20an?= =?UTF-8?q?=20attempt=20count=20=E2=80=94=20drop=20the=20false=20arithmeti?= =?UTF-8?q?c?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `retryAttempts(caught).length + 1` was documented as "how many sends the pillar made". It is not, and it is wrong on exactly the path #72 exists for: a cancellation observed at the RETRY-32 gate is synthesized by the engine (`engine.ts:394`), never raised by a send, so the trail already covers every attempt and the sum overstates by one. Two more reachable paths make the same sum wrong, both in the engine's own catch: `stampAttempt` throws before `dispatch` is called (`runAttempt` stamps first), and a `Clock.sleep` rejecting for something other than an abort fails after the attempt it followed is already in the trail. And narrowing the catch does not rescue it. `abortToSdkError` branches on `isTimeoutSignal` (`cancellation.ts:37-39`), so the RETRY-32 gate can synthesize a `TransportFailureError` too — which means `pipelines.md`'s worked example was wrong under the very class it narrowed to. That example no longer counts anything; it iterates the priors, which is what the accessor is for. Three prose sites reworded to state what the trail actually holds — one entry per attempt that failed BEFORE the surfaced error — plus the caveat: the surfaced error is an attempt's own only when it came from one. Two new engine cases pin it mechanically rather than by assertion in prose: a caller abort and a timeout abort each after ONE send, both asserting `dispatch.sends === 1` beside `retryAttempts(...).length === 1`, so any future `+ 1` claim has a red test under it. `core.api.md` unchanged: TSDoc prose only, api-extractor reports no signature change. Round 2 of #72. Refs #72, #67. --- docs/sdk-documentation/pipelines.md | 12 ++++- packages/core/src/retry/attempt-trail.ts | 14 ++++- packages/core/src/retry/engine.test.ts | 68 ++++++++++++++++++++++-- packages/core/src/retry/retry-step.ts | 7 +-- 4 files changed, 90 insertions(+), 11 deletions(-) diff --git a/docs/sdk-documentation/pipelines.md b/docs/sdk-documentation/pipelines.md index f6fe142..0ff6532 100644 --- a/docs/sdk-documentation/pipelines.md +++ b/docs/sdk-documentation/pipelines.md @@ -177,14 +177,22 @@ Retry and redirect are worth a few notes each, because both surprise people: await runtime.send(request); } catch (error) { if (error instanceof TransportFailureError) { - const spent = retryAttempts(error).length + 1; - console.error(`gave up after ${String(spent)} sends`, error.message); + for (const prior of retryAttempts(error)) { + console.error('an earlier attempt failed:', prior); + } } throw error; } } ``` + One entry per attempt that failed *before* the one you caught — which is not an attempt count, so + resist writing `length + 1`. The surfaced error is an attempt's own only when it came from one, and + sometimes it did not: a cancellation or timeout the engine observes between attempts is synthesized + at that gate, and so is a failure from stamping the attempt header, which runs before the request + goes out. On those paths the trail already covers every send. Narrowing the catch does not help — + a timeout signal is mapped to `TransportFailureError`, the same class a real send failure raises. + The trail is a side table keyed by the error, not a property on it, so nothing is added to an object you may not own; an error that never went through a retry loop answers with an empty list. Until 2026-09-05 the pillar wrapped its terminal failure in a `SuppressedError` instead, which made diff --git a/packages/core/src/retry/attempt-trail.ts b/packages/core/src/retry/attempt-trail.ts index 2885160..b526913 100644 --- a/packages/core/src/retry/attempt-trail.ts +++ b/packages/core/src/retry/attempt-trail.ts @@ -67,8 +67,18 @@ export function recordAttempts( * The retry pillar surfaces the **final** attempt's own error, unwrapped: `instanceof` against it * answers the same for one attempt as for ten, and a cancellation that ended a backoff wait arrives * as `CancellationError` rather than as something carrying one. The earlier attempts are not - * discarded — they are recorded here, so `retryAttempts(caught).length + 1` is how many sends the - * pillar made. A worked example is in `docs/sdk-documentation/pipelines.md`. + * discarded — they are recorded here, one entry per attempt that failed BEFORE the error you caught. + * A worked example is in `docs/sdk-documentation/pipelines.md`. + * + * **That is not an attempt count, and `length + 1` is not one either.** The arithmetic holds only + * when the surfaced error is itself an attempt's, and on three reachable paths it is not: a + * cancellation or timeout the engine observes at its `RETRY-32` gate is synthesized there rather + * than raised by a send; a failure from stamping the attempt header is raised before the request + * goes out; and a `Clock.sleep` that rejects for something other than an abort fails after the + * attempt it followed is already in the trail. On each of those the trail already accounts for every + * send, so adding one overstates it. Narrowing the catch does not rescue the sum — `abortToSdkError` + * yields `TransportFailureError` for a timeout signal, so even that class can reach you without a + * send behind it. * * Oldest first, and the error you passed in is never a member of its own trail (`RETRY-34`'s * skip-self clause, which matters because a transport may reuse one error instance across diff --git a/packages/core/src/retry/engine.test.ts b/packages/core/src/retry/engine.test.ts index 55d6350..3405c96 100644 --- a/packages/core/src/retry/engine.test.ts +++ b/packages/core/src/retry/engine.test.ts @@ -700,13 +700,24 @@ describe('cancellation while an attempt is in flight (RETRY-32)', () => { }); describe('a cancelled backoff surfaces the cancellation TYPE (XCUT-1)', () => { - test('the surfaced error is CancellationError, with the prior attempt beside it', async () => { - const controller = new AbortController(); - const first = new IoError('reset'); - const dispatch: RetryDispatch = () => { + /** Aborts from inside attempt 1, so the loop reaches its RETRY-32 check with a one-entry trail. */ + function abortAfterOneAttempt( + controller: AbortController, + first: unknown, + ): RetryDispatch & {sends: number} { + const dispatch = (): Promise> => { + dispatch.sends += 1; controller.abort(); return Promise.resolve(failure(first)); }; + dispatch.sends = 0; + return dispatch; + } + + test('the surfaced error is CancellationError, with the prior attempt beside it', async () => { + const controller = new AbortController(); + const first = new IoError('reset'); + const dispatch = abortAfterOneAttempt(controller, first); const outcome = await runWithRetry(GET, dispatch, { ...configOf({fixedDelayMs: 60_000, maxAttempts: 5}), @@ -721,6 +732,55 @@ describe('a cancelled backoff surfaces the cancellation TYPE (XCUT-1)', () => { expect(outcome.error).toBeInstanceOf(CancellationError); expect(retryAttempts(outcome.error)).toEqual([first]); }); + + test('the trail already covers every send, so length is NOT one less than the count', async () => { + const controller = new AbortController(); + const first = new IoError('reset'); + const dispatch = abortAfterOneAttempt(controller, first); + + const outcome = await runWithRetry(GET, dispatch, { + ...configOf({fixedDelayMs: 60_000, maxAttempts: 5}), + signal: controller.signal, + }); + + expect(outcome.kind).toBe('failure'); + if (outcome.kind !== 'failure') return; + // The surfaced error is SYNTHESIZED at the RETRY-32 gate, not raised by a send, so it is not an + // attempt's error and the trail already accounts for all of them. `length + 1` would say two + // sends where one happened -- which is why no TSDoc here offers that arithmetic. + expect(dispatch.sends).toBe(1); + expect(retryAttempts(outcome.error)).toHaveLength(1); + }); +}); + +describe('the RETRY-32 gate can synthesize a TransportFailureError too', () => { + test('a TIMEOUT signal takes the same synthesized path, as TransportFailureError', async () => { + // `abortToSdkError` branches on `isTimeoutSignal`, so the engine's own RETRY-32 gate can + // synthesize a `TransportFailureError` too. Narrowing a catch to that class therefore does NOT + // guarantee the caught error came from a send. + const controller = new AbortController(); + const first = new IoError('reset'); + const dispatch: RetryDispatch & {sends: number} = Object.assign( + (): Promise> => { + dispatch.sends += 1; + controller.abort(new DOMException('timed out', 'TimeoutError')); + return Promise.resolve(failure(first)); + }, + {sends: 0}, + ); + + const outcome = await runWithRetry(GET, dispatch, { + ...configOf({fixedDelayMs: 60_000, maxAttempts: 5}), + signal: controller.signal, + }); + + expect(outcome.kind).toBe('failure'); + if (outcome.kind !== 'failure') return; + expect(outcome.error).toBeInstanceOf(TransportFailureError); + expect(outcome.error).not.toBeInstanceOf(CancellationError); + expect(dispatch.sends).toBe(1); + expect(retryAttempts(outcome.error)).toHaveLength(1); + }); }); describe('a throwing attempt still carries the trail (RETRY-33/34)', () => { diff --git a/packages/core/src/retry/retry-step.ts b/packages/core/src/retry/retry-step.ts index 3199311..9a58392 100644 --- a/packages/core/src/retry/retry-step.ts +++ b/packages/core/src/retry/retry-step.ts @@ -117,9 +117,10 @@ function configFrom( * catch does not depend on how many attempts ran: a transport failure surfaces as * `TransportFailureError` whether `maxAttempts` was 1 or 3, and an abort that ended a backoff wait * surfaces as `CancellationError` (`XCUT-1`). The earlier attempts' errors are not lost -- read them - * with `retryAttempts(caught)`, oldest first (`RETRY-34`). A response the loop discards is always - * closed first; the response that ENDS the loop is returned live and unread, and closing it is - * yours. + * with `retryAttempts(caught)`, oldest first: one entry per attempt that failed BEFORE the error you + * caught, which is not the same as an attempt count (`RETRY-34`, and see `retryAttempts` for why the + * difference bites). A response the loop discards is always closed first; the response that ENDS the + * loop is returned live and unread, and closing it is yours. * * @param options - settings overrides and the injected clock, randomness, and delay override. * @returns the descriptor to install in a pipeline's RETRY slot.