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..0ff6532 100644 --- a/docs/sdk-documentation/pipelines.md +++ b/docs/sdk-documentation/pipelines.md @@ -153,8 +153,50 @@ 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) { + 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 + 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. 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..b526913 --- /dev/null +++ b/packages/core/src/retry/attempt-trail.ts @@ -0,0 +1,101 @@ +// 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, 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 + * 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..3405c96 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,90 @@ describe('cancellation while an attempt is in flight (RETRY-32)', () => { }); }); +describe('a cancelled backoff surfaces the cancellation TYPE (XCUT-1)', () => { + /** 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}), + 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]); + }); + + 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)', () => { test('a throw from inside the attempt is folded into a failure outcome, trail intact', async () => { const calls: number[] = []; @@ -681,16 +801,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 +823,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..9a58392 100644 --- a/packages/core/src/retry/retry-step.ts +++ b/packages/core/src/retry/retry-step.ts @@ -113,6 +113,15 @@ 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: 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. * 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(); + }); +}); 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); }); });