diff --git a/docs/deviations.md b/docs/deviations.md index 6bf260b..ded2cde 100644 --- a/docs/deviations.md +++ b/docs/deviations.md @@ -143,7 +143,7 @@ non-bridge clause survives and is enforced as an ordinary obligation on `send()` **Verified.** One engine — `runWithRetry` (`packages/core/src/retry/engine.ts:367`) — with exactly two thin callers: the pillar step (`packages/core/src/retry/retry-step.ts:151`) and the dispatch adapter -(`packages/core/src/retry/retry-dispatch.ts:55`). `totalTimeoutMs` is `readonly totalTimeoutMs?: number | +(`packages/core/src/retry/retry-dispatch.ts:85`). `totalTimeoutMs` is `readonly totalTimeoutMs?: number | undefined` and undefined by default (`packages/core/src/retry/settings.ts:27`), pinned by a test named for `RETRY-28` (`packages/core/src/retry/settings.test.ts:20`). @@ -482,6 +482,7 @@ frozen tree and is amended only deliberately, by hand. When §10 is next amended | **`XCUT-16`'s replay guard is keyed on whether the hop was guarded, not on whether the replacement looks credentialed.** `XCUT-16` and `AUTH-28` say the guard applies "on any path where a credential will be attached", and carve out "a deliberately credential-free re-issue MAY proceed over any scheme". Deciding which of the two a challenge replacement is cannot be done by reading header names: the step's own `ApiKeyCredentialConfig.headerName` stamps whatever header the caller names, and a `challengeHook` may invent a carrier this step has never been told about. The port therefore reads "a credential will be attached" as a property of the HOP — if the outbound pass ran the HTTPS guard, so does the replay, whatever URL and headers the hook chose. **Strictly wider than the requirement's letter**, and knowingly so: it refuses a downgraded replacement that carries no credential at all, on a hop that is credentialed. The carve-out is preserved where it is observable — a `NO_AUTH` hop is never guarded outbound, and its replay is guarded only when the replacement carries `Authorization` or `Proxy-Authorization`, which is the previous rule kept as a second arm. *Rejected:* deriving the credential-carrying header names from configuration, which misses the hook-invented carrier and is the shape that let the reported leak through | audit #67 / #71 | 2026-09-04 | `docs/product-spec/19-cross-cutting-invariants-and-policies.md:44` is the requirement and its carve-out. `packages/core/src/auth/auth-step.ts:389` sets `OutboundPlan.guarded`; `:564-575` is `guardReplayScheme` and its two arms. Pinned by "a replacement carrying a NON-standard credential header over plaintext is refused" and "a header-free replacement over plaintext is refused too" in `packages/core/src/auth/auth-step.test.ts`, and by the "XCUT-16: a guarded hop stays guarded across a challenge replay" block in `tests/conformance/xcut/security-by-default.conformance.test.ts` | not yet in §10 | | **`ASYNC-21`'s "MUST NOT close the caller-owned source on any termination" is not honoured: the RxJS SSE adapter takes ownership and closes.** `sseEvents$` and `typedSse$` pass `() => stream.close()` as `fromAsyncIterable`'s `release`, and RxJS runs a subscriber's finalizer on *every* termination — unsubscription, end-of-source and a source error alike, which is the complete list the clause names. **Kept, deliberately, on two grounds.** (1) **The clause has no subject on this platform.** It presumes a source whose iterator return leaves the source open; this port's `SseStream` is deliberately not that one. `#iterate`'s `finally` calls `#releaseQuietly()`, so the resource is released whenever the runtime drives `return()` — which `fromAsyncIterable` must do exactly once (`ASYNC-6`), and which a plain `for await` with `break` does too. Removing the callback would change which channel reports a release failure and when the release runs, not whether the caller-owned source ends up closed. (2) **The ordering is load-bearing.** The release runs *ahead of* `iterator.return()` because an async generator's `return()` queues behind a suspended `next()`, and an SSE stream idling between events is parked in exactly that pull — so without the callback an `unsubscribe()` stays pending until the server next sends a byte, holding the socket open indefinitely. Measured: deleting the two `release` arguments turns four cases red — the two suspended-pull ones, as "the teardown did not settle within 500ms", and the two pre-existing idle-unsubscribe assertions — while every exactly-once release count stays green, which is the shape of the claim. Pagination attaches no release for the complementary reason: a `Paginator`'s pulls are bounded HTTP exchanges, never a wait on a server that may never answer. *Rejected:* dropping the callback to match the letter (reintroduces the hang for no change in what closes). *Rejected:* a caller-facing `{ownership}` option (two behaviours to document for a case with one correct answer). The public TSDoc and `packages/rx/README.md` now state the transfer outright — subscribing hands the stream over, do not close it yourself and do not iterate it afterwards — rather than leaving the `ASYNC-21` citation on the doc comment's first line to read as satisfied | audit #67 / #75 | 2026-09-05 | `packages/rx/src/sse.ts:46` and `:73-75` are the two `release` arguments; `packages/rx/src/from-async-iterable.ts:103-108` is the teardown that runs one on every termination; `docs/product-spec/18-asynchronous-runtime-adapter-contract.md:42` is the requirement. Ground 1: `packages/core/src/sse/stream.ts:136-139` (`#iterate`'s `finally` → `#releaseQuietly()`) with `:117-121` (`close()` memoized, `SSE-28`). Ground 2: `packages/rx/src/from-async-iterable.ts:44-48` states the ordering and why. Pinned by the two `resource ownership` blocks in `packages/rx/src/sse.test.ts`, which count the release the OWNED resource sees rather than `SseStream.close()` calls — the facade memoizes, so a facade-level count reads "once" however many paths call it — and by "SSE ownership transfer releases once on Node" in `tests/node-conformance/rx-bridge.test.mjs`. Phase 8b marked `ASYNC-21` ✅ with this clause dropped from its gist (`docs/work/mvp/phase8/phase8b/2026-07-28-phase8b-async-runtime-checklist.md:67`); that is a dated record and is left as written. The other half is `SSE-41`'s own "documented source ownership" clause, which the same checklist marked ✅ (`:74`) on the strength of documentation that named unsubscription only — completed by the TSDoc and README rewrite this row accompanies | not yet in §10 | | **`AUTH-22`'s "emit cnonce/nc/qop only when qop is negotiated" is not applied to `cnonce` for a `-sess` algorithm.** A `-sess` HA1 is `H(H(user:realm:pass):nonce:cnonce)` (RFC 7616 §3.4.2), so the client nonce is an *input to the hash* for `MD5-sess` and `SHA-256-sess` whatever `qop` the challenge offered. The port implemented AUTH-22 to the letter: it drew a fresh cnonce, folded it into HA1, and then omitted it from the header whenever `qop` was absent — a response no server can verify, because it has no way to reconstruct HA1. AUTH-30 bounds the re-challenge replay to one 401, so every such exchange simply failed. **`cnonce` is now emitted for any `-sess` algorithm; `nc` and `qop` stay conditional exactly as AUTH-22 says**, because RFC 2069's response input is `H(HA1:nonce:HA2)` and carries no nonce count, so emitting one would advertise a count the response was not computed over. RFC 7616 §3.4 states the wider rule outright — "cnonce: This parameter MUST be used by all implementations". AUTH-22's clause is RFC 2617's RFC 2069-compatibility form, written before `-sess` existed, and the requirement's own AUTH-15 mandates both `-sess` algorithms, so the two sentences cannot both be followed. *Rejected:* declining a `-sess`-without-`qop` challenge instead, which turns every such server into a guaranteed 401 for no security gain, when the value the server needs has already been computed | audit #67 / #74 | 2026-09-05 | `packages/core/src/auth/digest.ts:345-350` (the `-sess` HA1 that consumes the cnonce) against `:405-408` (`buildHeaderValue`, where the `else if` now emits it); `docs/product-spec/11-authentication.md:18` and `docs/product-spec/appendix-c-consolidated-normative-requirement-index.md:357` are AUTH-22's wording. Pinned by the `digestHandler -sess without qop (AUTH-17/AUTH-22)` block in `packages/core/src/auth/digest.test.ts` — one row asserting the header carries `cnonce` and neither `nc` nor `qop`, one recomputing the response from the header's OWN cnonce so a value drawn twice would fail — and by the `MD5-sess, no qop` vector in the same file | not yet in §10 | +| **`RETRY-44`'s "downstream chain" is read as everything BELOW the retry point, which in the recovery stack excludes the request chain.** The requirement has two clauses: each attempt re-executes the downstream chain with fresh per-attempt state, and "upstream steps MUST NOT mutate the shared in-flight request between attempts". The port originally read the first clause as covering the *whole* recovery chain and re-ran `RequestRecoveryChain.apply()` per attempt, with a test that said so by name. That makes `packages/core/src/recovery/idempotency-key.ts` generate a fresh key on every attempt, so three attempts of one logical request reach the server as three unrelated writes — the precise failure `RECOV-32` exists to prevent, and the opposite of what that step's own `@public` TSDoc promises. **The chain is now applied once, above the loop; each attempt re-executes transport plus response chain over `stampAttempt`'s fresh copy of the prepared request.** Under this reading both clauses hold and the second holds *by construction*: upstream steps cannot mutate the in-flight request between attempts because they no longer run between attempts. The pillar stack is untouched — there "downstream" is the forked continuation (`ctx.fork()`), and `retryStep` still re-drives it per attempt. *Rejected:* memoizing the key on the template (a `WeakMap` keyed by the `Request` instance) — a caller who deliberately sends one immutable `Request` value twice would replay the key and have the server drop a genuine second call. *Rejected:* re-running the chain over the *prepared* request each attempt — the chain would read its own output, which is clause two's mutation in different clothes, and every shipped and caller-written step would have to be proven idempotent. One consequence recorded rather than assumed: the re-send gate (`RETRY-5`/`RECOV-18`) now judges the prepared request rather than the caller's, which is what a retry would actually re-send | audit #67 / #73 | 2026-09-05 | `packages/core/src/retry/retry-dispatch.ts:83-88` (the chain applied once, then `runWithRetry`) against `:27-33` (the per-attempt half); `packages/core/src/recovery/orchestrator.ts:62` (`prepareRequest`) and `:117` (`dispatchPrepared`); `packages/core/src/retry/engine.ts:243` is the gate that now reads the prepared request. `RETRY-44`'s wording is `docs/product-spec/09-retry-and-resilience.md:35` and `docs/product-spec/appendix-c-consolidated-normative-requirement-index.md:306`. Pinned by `packages/core/src/retry/retry-dispatch.test.ts:126` (chain applied once), `:169` (one `generate()`, one key on three sends), `:201` (the `RETRY-38` ordinal varies while the key does not) and `:227` (a request-chain throw is not retried and meets the recovery phase exactly once) | not yet in §10 | ### Proposed erratum for `PIPE-40` (drafted 2026-09-04, not applied) diff --git a/packages/core/src/recovery/idempotency-key.ts b/packages/core/src/recovery/idempotency-key.ts index 4f81df0..e2b967f 100644 --- a/packages/core/src/recovery/idempotency-key.ts +++ b/packages/core/src/recovery/idempotency-key.ts @@ -30,9 +30,17 @@ export interface IdempotencyKeyOptions { /** * A `RequestStep` that stamps an idempotency key on write requests (RECOV-32). * - * Runs ONCE per call, upstream of retry -- not per attempt. `retry/attempt-stamp.ts` is its sibling: - * that one writes the attempt ordinal on each per-attempt copy and preserves whatever this wrote - * (RETRY-38), so the server sees one stable key across every retry of the same logical request. + * Runs ONCE per logical request, upstream of retry -- not per attempt. `retry/attempt-stamp.ts` is + * its sibling: that one writes the attempt ordinal on each per-attempt copy and preserves whatever + * this wrote (RETRY-38), so the server sees one stable key across every retry of the same logical + * request. + * + * **That is a property of the composition, and the SDK's own retry adapter is what supplies it**: + * `retry/retry-dispatch.ts` applies the `RequestRecoveryChain` once, above the retry loop, and each + * attempt re-sends a copy of the request it produced. On its own a step can only promise RECOV-32's + * letter -- `generate()` is invoked at most once per *application* to an applicable request -- so a + * caller who re-applies their own chain per attempt will get a fresh key per attempt. Install the + * chain once and let the retry layer sit below it. * * @param options - the key strategy plus the header name, method set, and existing-key policy. * @returns the request step to install in a `RequestRecoveryChain`. diff --git a/packages/core/src/recovery/orchestrator.ts b/packages/core/src/recovery/orchestrator.ts index 3f3e9c8..16e9645 100644 --- a/packages/core/src/recovery/orchestrator.ts +++ b/packages/core/src/recovery/orchestrator.ts @@ -29,47 +29,98 @@ export interface DispatchConfig { } /** - * The unified recovery-chain orchestrator (RECOV-2, RECOV-10, RECOV-11). + * {@link DispatchConfig} without the request chain: everything the phases BELOW that chain need. * - * One `try`/`catch` wraps both the request chain's `apply()` and the transport invocation, so every - * throwable from either is caught and converted into a Failure before the response chain runs — a - * before-request throw cannot skip after-error handling. That conversion goes through - * {@link wrapCancellation} (RECOV-11), this orchestrator's catch being its only call site; RECOV-2's - * guarantee rests on that helper never throwing, since this catch clause is the last place a - * throwable could escape without meeting the recovery hooks. + * A retry loop drives {@link dispatchPrepared} through this, having already run the request chain + * once above itself. Naming it as a subset rather than duplicating the fields keeps the two shapes + * from drifting when `DispatchConfig` grows. * - * The final unwrap returns the response on a Success, or rethrows the Failure's throwable - * **unchanged** — no wrapping, no substitution (RECOV-10). Surfacing a typed exception is a - * recovery step's own responsibility, never this function's. + * @internal + */ +export type PreparedDispatchConfig = Omit; + +/** + * The request chain, run ONCE per logical request, with RECOV-2's conversion already applied: a + * throwing step becomes a Failure here rather than propagating, so no caller has to catch it. That + * conversion goes through {@link wrapCancellation} (RECOV-11); RECOV-2's guarantee rests on that + * helper never throwing, since this `catch` is the last place a request-chain throwable could + * escape without meeting the recovery hooks. * - * @param request - the request to prepare and send. - * @param config - transport, chains, and the per-call options and signal. - * @returns the response the terminal outcome carries. - * @throws Whatever the terminal Failure carries, by identity — any value, not necessarily an - * `Error`. + * Split out of {@link dispatchWithRecovery} on 2026-09-05 so `retry/retry-dispatch.ts` can run this + * half once for a whole logical request and repeat only the half below it per attempt. Before that + * split every retry attempt re-ran the request chain, and `recovery/idempotency-key.ts` generated a + * fresh key on each one — three attempts of one logical request reached the server as three + * distinct idempotency keys, which is exactly what RECOV-32's key is bought to prevent. * - * @public + * @param request - the request to prepare. + * @param requestChain - the ordered request steps (RECOV-3). + * @returns a Success carrying the prepared request, or a Failure carrying whatever a step threw. + * Never throws, for any input. + * + * @internal */ -export async function dispatchWithRecovery( +export async function prepareRequest( request: Request, - config: DispatchConfig, -): Promise { - let outcome: Outcome; + requestChain: RequestRecoveryChain, +): Promise> { try { - const preparedRequest = await config.requestChain.apply(request); - outcome = success( + return success(await requestChain.apply(request)); + } catch (error) { + return wrapCancellation(error); + } +} + +/** + * The transport hop, with RECOV-2's conversion applied to whatever it throws. + * + * A `prepared` that is already a Failure short-circuits it: the transport is not called, and the + * failure is handed on for the response chain to see. Both branches widen `Outcome` and + * `Outcome` to `Outcome` without a cast, because the failure variant does not + * mention the type parameter — and neither branch throws, which is what keeps RECOV-2 absolute. + */ +async function sendPrepared( + prepared: Outcome, + config: PreparedDispatchConfig, +): Promise> { + if (prepared.kind === 'failure') return prepared; + try { + return success( await config.transport.send( - preparedRequest, + prepared.value, config.options, config.signal, ), ); } catch (error) { - // RECOV-11: `Outcome` widens to `Outcome` without a cast, and never throws, - // which is what keeps RECOV-2 absolute. - outcome = wrapCancellation(error); + return wrapCancellation(error); } - const finalOutcome = await config.responseChain.apply(outcome); +} + +/** + * Everything below the request chain — the transport hop, the response chain, and RECOV-10's + * terminal unwrap. This is the part a retry loop repeats, once per wire send (RETRY-44's + * "downstream chain"). + * + * It takes {@link prepareRequest}'s outcome rather than a bare `Request` because a request-chain + * failure still owes RECOV-2 a trip through the response and recovery chains before it surfaces. + * On that input the transport is not called at all, which is the whole difference between the two + * variants. + * + * @param prepared - {@link prepareRequest}'s result for this logical request. + * @param config - transport, response chain, and the per-call options and signal. + * @returns the response the terminal outcome carries. + * @throws Whatever the terminal Failure carries, by identity — any value, not necessarily an + * `Error`. + * + * @internal + */ +export async function dispatchPrepared( + prepared: Outcome, + config: PreparedDispatchConfig, +): Promise { + const finalOutcome = await config.responseChain.apply( + await sendPrepared(prepared, config), + ); return fold( finalOutcome, response => response, @@ -78,3 +129,39 @@ export async function dispatchWithRecovery( }, ); } + +/** + * The unified recovery-chain orchestrator (RECOV-2, RECOV-10, RECOV-11). + * + * The two halves it composes are named: `prepareRequest` runs the request chain, and + * `dispatchPrepared` runs the transport hop and the response chain. Neither is exported from the + * package, so both are backticked rather than `{@link}`ed — api-extractor cannot resolve a + * reference out of the published surface into one, and the unresolved link is an error, not a + * warning to live with. Every throwable from either half is caught and converted into a Failure + * before the response chain runs — a before-request throw cannot skip after-error handling. + * + * The final unwrap returns the response on a Success, or rethrows the Failure's throwable + * **unchanged** — no wrapping, no substitution (RECOV-10). Surfacing a typed exception is a + * recovery step's own responsibility, never this function's. + * + * **One dispatch is one wire send.** Nothing here retries; a caller that wants retries composes the + * two halves itself so that the request chain runs once and only the second half repeats + * (`retry/retry-dispatch.ts`). + * + * @param request - the request to prepare and send. + * @param config - transport, chains, and the per-call options and signal. + * @returns the response the terminal outcome carries. + * @throws Whatever the terminal Failure carries, by identity — any value, not necessarily an + * `Error`. + * + * @public + */ +export async function dispatchWithRecovery( + request: Request, + config: DispatchConfig, +): Promise { + return dispatchPrepared( + await prepareRequest(request, config.requestChain), + config, + ); +} diff --git a/packages/core/src/recovery/request-chain.ts b/packages/core/src/recovery/request-chain.ts index 7876e87..3426126 100644 --- a/packages/core/src/recovery/request-chain.ts +++ b/packages/core/src/recovery/request-chain.ts @@ -13,8 +13,13 @@ export type RequestStep = (request: Request) => Promise; /** * A sequential left-to-right fold over request steps (RECOV-3): the output of step N is the input * of step N+1, an empty chain returns its input unchanged, and a throwing step aborts the remainder - * and propagates — `dispatchWithRecovery` (`orchestrator.ts`) converts that propagation into a - * `Failure` per RECOV-2, which is the only reason propagating here is safe. + * and propagates — `prepareRequest` (`orchestrator.ts`) converts that propagation into a `Failure` + * per RECOV-2, which is the only reason propagating here is safe. Every entry point runs the chain + * through that one helper, the retry adapter included. + * + * **Apply the chain once per logical request, not once per wire send.** Steps here are the ones + * whose output must stay stable across a retry — `idempotencyKeyStep` above all — and the SDK's own + * retry adapter sits below this chain for that reason (`retry/retry-dispatch.ts`). * * Safe under concurrent `apply()` calls (RECOV-14): after construction the instance holds nothing * but its step array, and every piece of per-call state lives in `apply()`'s locals. A later phase diff --git a/packages/core/src/retry/engine.ts b/packages/core/src/retry/engine.ts index a394c17..edce8be 100644 --- a/packages/core/src/retry/engine.ts +++ b/packages/core/src/retry/engine.ts @@ -356,7 +356,10 @@ function maybeEmitExhausted( * failure outcome carrying the trail, rather than left to surface as a bare rejected promise that * would drop RETRY-34's prior attempts on the floor. * - * @param request - the captured template every attempt re-sends. + * @param request - the captured template every attempt re-sends. Whatever the caller captured is + * already final: the pillar adapter passes the request arriving at the RETRY stage, and the + * recovery adapter passes the output of a request chain it applied ONCE, above this loop + * (RECOV-32 -- one idempotency key per logical request). This loop only ever copies it. * @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. On failure the error is the FINAL attempt's own, unwrapped, with diff --git a/packages/core/src/retry/retry-dispatch.test.ts b/packages/core/src/retry/retry-dispatch.test.ts index 4d82b0b..de83837 100644 --- a/packages/core/src/retry/retry-dispatch.test.ts +++ b/packages/core/src/retry/retry-dispatch.test.ts @@ -1,42 +1,101 @@ // SPDX-License-Identifier: MIT // packages/core/src/retry/retry-dispatch.test.ts -// Exercises: RECOV-17..20 (the recovery stack's retry lands here), RETRY-44 (each attempt re-runs the -// WHOLE recovery chain -- request chain, transport, response chain), RETRY-13/14/RECOV-30 (both entry -// points share one engine, so the schedule cannot drift). +// Exercises: RECOV-17..20 (the recovery stack's retry lands here), RECOV-32 (one idempotency key per +// LOGICAL request -- the strategy runs once and every wire send carries its result), RETRY-38 (the +// per-attempt ordinal is stamped on a fresh copy and preserves that key), RETRY-44 (fresh per-attempt +// state below the retry point; upstream steps do not run between attempts), RECOV-2 (a request-chain +// throw still meets the response and recovery hooks), RETRY-13/14/RECOV-30 (both entry points share +// one engine, so the schedule cannot drift). import {describe, expect, test} from 'bun:test'; +import {stringBody} from '../body/simple-bodies.js'; import type {Clock} from '../config/clock.js'; import {Request} from '../http/request.js'; +import type {Response} from '../http/response.js'; import {IoError} from '../io/errors.js'; +import {idempotencyKeyStep} from '../recovery/idempotency-key.js'; +import {failure, success, type Outcome} from '../recovery/outcome.js'; import {RequestRecoveryChain} from '../recovery/request-chain.js'; -import {ResponseRecoveryChain} from '../recovery/response-chain.js'; +import { + ResponseRecoveryChain, + type RecoveryStep, +} from '../recovery/response-chain.js'; import {FakeTransport, countingResponse} from '../testing/fake-transport.js'; import {dispatchWithRetry, type RetryDispatchConfig} from './retry-dispatch.js'; -import {retrySettings} from './settings.js'; +import {retrySettings, type RetrySettings} from './settings.js'; const GET = Request.newBuilder().url('https://example.com').build(); +/** A POST the re-send gate lets through: RETRY-5 wants a body, and a replayable one. */ +function replayablePost(): Request { + return Request.newBuilder() + .method('POST') + .url('https://example.com') + .body(stringBody('payload')) + .build(); +} + const zeroClock: Clock = { now: () => 0, monotonic: () => 0, sleep: () => Promise.resolve(), }; +/** The third parameter is an object because `max-params` errors at four. */ +interface ConfigExtras { + readonly settings?: Partial; + readonly recoverySteps?: readonly RecoveryStep[]; +} + function configOf( transport: FakeTransport, requestSteps = new RequestRecoveryChain([]), + extras: ConfigExtras = {}, ): RetryDispatchConfig { return { transport, requestChain: requestSteps, - responseChain: new ResponseRecoveryChain([], []), + responseChain: new ResponseRecoveryChain([], extras.recoverySteps ?? []), retry: { - settings: retrySettings({maxAttempts: 3, fixedDelayMs: 0}), + settings: retrySettings({ + maxAttempts: 3, + fixedDelayMs: 0, + ...extras.settings, + }), clock: zeroClock, random: () => 0.5, }, }; } +/** + * A one-step request chain whose key strategy is COUNTED and whose keys are DISTINCT. Both matter: + * a strategy returning one constant value would pass every assertion below even if it were called + * once per attempt, which is the bug these cases exist to catch. + */ +function countingKeyChain(): { + chain: RequestRecoveryChain; + generated: () => number; +} { + let generated = 0; + const chain = new RequestRecoveryChain([ + idempotencyKeyStep({ + generate: () => { + generated += 1; + return `key-${String(generated)}`; + }, + }), + ]); + return {chain, generated: () => generated}; +} + +/** The named header seen by each wire send, in order. */ +function headerSent( + transport: FakeTransport, + name = 'Idempotency-Key', +): (string | undefined)[] { + return transport.calls.map(call => call.request.headers.get(name)); +} + /** * Captures a rejection reason. `expect(...).rejects` is typed as returning `void` under this * runner's type definitions, so awaiting it trips `@typescript-eslint/await-thenable`; this helper @@ -64,7 +123,7 @@ describe('dispatchWithRetry', () => { expect(transport.sendCount).toBe(2); }); - test('re-runs the request recovery chain on every attempt (RETRY-44)', async () => { + test('applies the request recovery chain ONCE per logical request, not per attempt (RETRY-44)', async () => { let applications = 0; const chain = new RequestRecoveryChain([ request => { @@ -79,7 +138,8 @@ describe('dispatchWithRetry', () => { await dispatchWithRetry(GET, configOf(transport, chain)); - expect(applications).toBe(2); + expect(transport.sendCount).toBe(2); + expect(applications).toBe(1); }); test('rethrows the terminal failure unchanged in shape', async () => { @@ -104,3 +164,97 @@ describe('dispatchWithRetry', () => { expect(transport.sendCount).toBe(1); }); }); + +describe('dispatchWithRetry and the idempotency key (RECOV-32)', () => { + test('generates ONE key for three attempts and sends it on all three', async () => { + const {chain, generated} = countingKeyChain(); + const transport = new FakeTransport([ + new IoError('reset'), + new IoError('reset'), + countingResponse(200).response, + ]); + + const response = await dispatchWithRetry( + replayablePost(), + configOf(transport, chain), + ); + + expect(response.status.code).toBe(200); + expect(transport.sendCount).toBe(3); + expect(generated()).toBe(1); + expect(headerSent(transport)).toEqual(['key-1', 'key-1', 'key-1']); + }); + + test('the run that exhausts the budget sends the same key on every attempt too', async () => { + const {chain, generated} = countingKeyChain(); + const transport = new FakeTransport([new IoError('reset')]); + + await rejectionOf( + dispatchWithRetry(replayablePost(), configOf(transport, chain)), + ); + + expect(transport.sendCount).toBe(3); + expect(generated()).toBe(1); + expect(headerSent(transport)).toEqual(['key-1', 'key-1', 'key-1']); + }); + + test('the attempt ordinal varies per send while the key does not (RETRY-38)', async () => { + const {chain, generated} = countingKeyChain(); + const transport = new FakeTransport([ + new IoError('reset'), + new IoError('reset'), + countingResponse(200).response, + ]); + + await dispatchWithRetry( + replayablePost(), + configOf(transport, chain, {settings: {attemptHeaderName: 'X-Attempt'}}), + ); + + // The ordinal is the ENGINE's, written per attempt on `stampAttempt`'s fresh copy; the key is + // the request chain's, written once above the loop. Both survive on every send. + expect(generated()).toBe(1); + expect(headerSent(transport)).toEqual(['key-1', 'key-1', 'key-1']); + expect(headerSent(transport, 'X-Attempt')).toEqual(['1', '2', '3']); + }); +}); + +describe('dispatchWithRetry and a failing request chain (RECOV-2)', () => { + const boom = new IoError('request step failed'); + const throwingChain = (): RequestRecoveryChain => + new RequestRecoveryChain([() => Promise.reject(boom)]); + + test('does not retry it, never reaches the transport, and runs the recovery phase once', async () => { + const seen: Outcome[] = []; + const recovery: RecoveryStep = outcome => { + seen.push(outcome); + return Promise.resolve(outcome); + }; + const transport = new FakeTransport([countingResponse(200).response]); + + const thrown = await rejectionOf( + dispatchWithRetry( + GET, + configOf(transport, throwingChain(), {recoverySteps: [recovery]}), + ), + ); + + expect(thrown).toBe(boom); + expect(transport.sendCount).toBe(0); + expect(seen).toEqual([failure(boom)]); + }); + + test('a recovery step may still convert that failure into a success', async () => { + const substitute = countingResponse(204).response; + const recovery: RecoveryStep = () => Promise.resolve(success(substitute)); + const transport = new FakeTransport([countingResponse(200).response]); + + const response = await dispatchWithRetry( + GET, + configOf(transport, throwingChain(), {recoverySteps: [recovery]}), + ); + + expect(response).toBe(substitute); + expect(transport.sendCount).toBe(0); + }); +}); diff --git a/packages/core/src/retry/retry-dispatch.ts b/packages/core/src/retry/retry-dispatch.ts index 9672fca..f641b44 100644 --- a/packages/core/src/retry/retry-dispatch.ts +++ b/packages/core/src/retry/retry-dispatch.ts @@ -3,7 +3,8 @@ import type {Request} from '../http/request.js'; import type {Response} from '../http/response.js'; import { - dispatchWithRecovery, + dispatchPrepared, + prepareRequest, type DispatchConfig, } from '../recovery/orchestrator.js'; import {failure, fold, success} from '../recovery/outcome.js'; @@ -18,10 +19,15 @@ export interface RetryDispatchConfig extends DispatchConfig { readonly retry: RetryConfig; } +/** + * One attempt: the transport hop and the response chain over the ALREADY-prepared request the + * engine handed back from `stampAttempt`. The request chain is deliberately not in here — see + * {@link dispatchWithRetry}. + */ function attemptVia(config: RetryDispatchConfig): RetryDispatch { return async request => { try { - return success(await dispatchWithRecovery(request, config)); + return success(await dispatchPrepared(success(request), config)); } catch (error) { return failure(error); } @@ -32,9 +38,31 @@ function attemptVia(config: RetryDispatchConfig): RetryDispatch { * The recovery-chain entry point for retry (RECOV-17..RECOV-20). * * NOT a `RecoveryStep` -- a recovery step receives an outcome and has no way to re-dispatch. This - * wraps 4b's orchestrator instead, mirroring its `(request, config)` shape, so each attempt re-runs - * the ENTIRE recovery chain: request chain, transport, response chain. That is the recovery-side - * mirror of what `ctx.fork()` does for the pillar step (RETRY-44). + * composes 4b's orchestrator halves instead, mirroring its `(request, config)` shape. + * + * **The request chain runs ONCE, above the loop; each attempt repeats only what is below it** -- + * the transport hop and the response chain, over `stampAttempt`'s fresh copy of the one prepared + * request. That is the layering `recovery/idempotency-key.ts` documents and RECOV-32 needs: its + * `generate()` is invoked once per logical request, so all N attempts reach the server under one + * key, and RETRY-38's per-attempt ordinal is written on the copy without disturbing it. Until + * 2026-09-05 this function re-ran the whole recovery chain per attempt and three attempts produced + * three different keys, defeating the header's entire purpose (audit #67, issue #73). + * + * RETRY-44 is satisfied, not traded away. Its "downstream chain" is whatever sits below the retry + * point, which here is transport plus response chain, and that is re-executed with fresh + * per-attempt state every time. Its second clause -- upstream steps MUST NOT mutate the shared + * in-flight request between attempts -- holds by construction now, because upstream steps no longer + * run between attempts at all. + * + * A request-chain failure is NOT retried: it never reached the wire, so RETRY-5's re-send gate has + * nothing to judge and re-running the step that just threw would only throw again. It still passes + * through the response and recovery chains exactly once, so RECOV-2's "no throwable bypasses the + * recovery hooks" and RECOV-10's unwrap are unchanged. + * + * One consequence worth naming: the engine's re-send gate (RETRY-5/RECOV-18, `isResendable`) now + * reads the PREPARED request rather than the caller's, so a request step that swaps in a + * non-replayable body makes the call non-retryable -- which is the honest answer, since the + * prepared request is what a retry would have to re-send. * * Shares `runWithRetry` with the pillar adapter, which is what makes RETRY-13/RETRY-14/RECOV-30's * "the two stacks must not drift" structural rather than a discipline. @@ -44,7 +72,7 @@ function attemptVia(config: RetryDispatchConfig): RetryDispatch { * @returns the response of the terminal successful attempt. * @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()`. + * `retryAttempts()`. A request-chain throwable surfaces the same way, with no trail. * * @internal */ @@ -52,7 +80,13 @@ export async function dispatchWithRetry( request: Request, config: RetryDispatchConfig, ): Promise { - const outcome = await runWithRetry(request, attemptVia(config), config.retry); + const prepared = await prepareRequest(request, config.requestChain); + if (prepared.kind === 'failure') return dispatchPrepared(prepared, config); + const outcome = await runWithRetry( + prepared.value, + attemptVia(config), + config.retry, + ); return fold( outcome, response => response,