diff --git a/docs/sdk-documentation/write-a-paging-strategy.md b/docs/sdk-documentation/write-a-paging-strategy.md index 40d1700..5913cbf 100644 --- a/docs/sdk-documentation/write-a-paging-strategy.md +++ b/docs/sdk-documentation/write-a-paging-strategy.md @@ -95,7 +95,7 @@ for await (const page of paginator.pages()) { /* page by page */ } independent walks, not two views of one. That is also why `@dexpace/rx`'s `pageItems$`/`pages$` are cold and repeatable while its SSE observables are not. -## The four rules +## The five rules **1. Take everything you need from the response before your promise settles** (`PAGE-5`). The response you are handed is live and single-use; the engine may close it the moment `parse` resolves. @@ -135,6 +135,20 @@ which is an erratum recorded in `docs/knowledge/notes/pagination.md` and `docs/w `maxPages` on `PaginatorInit` is the backstop, not the design. Loop detection is not the paginator's job. +**5. Always return a well-formed `PageInfo`** (`PAGE-4`). `items` must be an array — an empty one is +fine and is a perfectly valid non-terminal page — and `nextRequest === undefined` is the **single, +exclusive** end-of-stream signal. A `PageInfo` that is itself `null` or `undefined`, or whose `items` +is either, is a programmer error and the engine treats it as one: it closes the response and throws +an assertion naming the invariant you broke. It does **not** end the walk quietly, because "the +strategy forgot to `return`" and "the server ran out of pages" must not look the same from the +outside. Use `pageInfo(items, next?)` and this cannot happen; the check exists because `parse` +crosses a seam, where an `any`-typed decode or a trusted server field can produce a shape the types +say is impossible. + +Terminating and failing are different acts. To *end* the walk, return `pageInfo(items)` with no next +request. To *fail* it, throw — the engine closes the response and your error reaches the consumer +unwrapped (`PAGE-13`, `PAGE-28`). + `PaginationError` is reserved for engine misuse and precondition violations — not for "the server returned a page I did not understand", which is your `extract`'s error to raise. diff --git a/docs/sdk-documentation/write-a-serde.md b/docs/sdk-documentation/write-a-serde.md index daea858..5893836 100644 --- a/docs/sdk-documentation/write-a-serde.md +++ b/docs/sdk-documentation/write-a-serde.md @@ -61,6 +61,33 @@ import { const TEXT = new TextEncoder(); +/** + * Settle when `operation` settles, or as soon as `signal` aborts — whichever comes first, with the + * caller's own `reason` surfaced verbatim. `throwIfAborted()` alone cannot interrupt a `read()` or + * `write()` that never resolves, which is the case that leaves a caller's stream locked forever. + */ +const raceAbort = async ( + operation: Promise, + signal: AbortSignal | undefined, +): Promise => { + if (signal === undefined) return operation; + signal.throwIfAborted(); + let onAbort = (): void => undefined; + const aborted = new Promise((_resolve, reject) => { + onAbort = (): void => { + reject(signal.reason as unknown); + }; + signal.addEventListener('abort', onAbort, {once: true}); + }); + try { + // The loser of the race keeps `Promise.race`'s own handler, so a `read()` that rejects after + // the lock is released never becomes an unhandled rejection. + return await Promise.race([operation, aborted]); + } finally { + signal.removeEventListener('abort', onAbort); + } +}; + export function csvSerde(): Serde { const encode = (value: unknown): string => { if (!Array.isArray(value)) { @@ -106,7 +133,8 @@ export function csvSerde(): Serde { options?.signal?.throwIfAborted(); // before the lock: an aborted call leaves the sink free const writer = sink.getWriter(); // TypeError if contended — a programmer error, not re-typed try { - await writer.write(TEXT.encode(encode(value))); + // Raced, not just checked: a slow sink parks this write, and the abort must reach it. + await raceAbort(writer.write(TEXT.encode(encode(value))), options?.signal); } finally { writer.releaseLock(); // never close or abort: the caller owns the sink (SERDE-3) } @@ -115,14 +143,15 @@ export function csvSerde(): Serde { deserializer: { deserialize: (data, target) => decode(new TextDecoder().decode(data), target), async deserializeFrom(source, target, options) { - options?.signal?.throwIfAborted(); // before the lock, and again after every read below + options?.signal?.throwIfAborted(); // before the lock: an aborted call never takes one const reader = source.getReader(); const chunks: Uint8Array[] = []; try { for (;;) { - const {done, value} = await reader.read(); + // Raced, not checked between chunks: a source that stalls mid-body parks the loop + // inside `read()`, where a between-chunks check never runs again. + const {done, value} = await raceAbort(reader.read(), options?.signal); if (done) break; - options?.signal?.throwIfAborted(); chunks.push(value); } } finally { @@ -141,7 +170,7 @@ export function csvSerde(): Serde { } ``` -Six rules, all visible above: +Seven rules, all visible above: 1. **Raise `SerializationError` / `DeserializationError`, never a raw error** — with one stated exception: `serializeInto`'s out-of-range or does-not-fit case is a plain `RangeError` with no @@ -160,6 +189,17 @@ Six rules, all visible above: target, on **every** entry point (`SERDE-13`), never return a `null` that detonates at a later field access. The fallback label is the literal `'the target type'`; each codec repeats it, because `SEAM-1` leaves core with no exported constant to share. +7. **An abort must race the pending operation, not sit between two of them** (`SERDE-3`). The seam + promises that "an aborted call never leaves the caller's source locked", and a + `throwIfAborted()` between chunks cannot keep it: a source that stalls mid-body parks the drain + inside `read()`, so the call never settles and the lock is never released. Race each pending + `read()`/`write()` against the signal, remove the listener in a `finally`, then release the lock + as usual. Releasing a reader with a read still outstanding is legal on every supported runtime + and does unlock the stream — the outstanding read rejects, differently per runtime + (`AbortError` on Bun 1.3.14, `TypeError: Invalid state: Releasing reader` on Node 20.3 and 26, + measured 2026-09-05), which is why the caller must see the signal's `reason` instead. + `@dexpace/codec-json` holds **one** listener for the whole drive rather than one per chunk; the + example above takes the simpler per-operation form. `mediaType` is the default `Content-Type` — `serdeBody(value, serde)` reads it, and a caller may override per body. diff --git a/packages/codec-json/src/abort-race.ts b/packages/codec-json/src/abort-race.ts new file mode 100644 index 0000000..357158b --- /dev/null +++ b/packages/codec-json/src/abort-race.ts @@ -0,0 +1,84 @@ +// SPDX-License-Identifier: MIT +// packages/codec-json/src/abort-race.ts + +/** + * One abort listener held for the length of a whole stream drive, plus the race that lets it settle + * an operation that is already *pending*. + * + * @internal + */ +export interface AbortRace { + /** + * Settle with `operation`, or reject with the signal's `reason` the moment it aborts — whichever + * happens first. + * + * Also rejects before `operation` is even consulted when the signal is already aborted, which is + * the between-chunks check the loop used to make for itself. + */ + race(operation: Promise): Promise; + + /** Drop the abort listener. Call from the `finally` that releases the stream lock. */ + release(): void; +} + +/** The no-signal case: no listener to install, no race to run, no allocation per chunk. */ +const UNRACED: AbortRace = Object.freeze({ + race: (operation: Promise): Promise => operation, + release: (): void => undefined, +}); + +/** + * Bind `signal` to a single listener that can interrupt any number of pending operations + * (SERDE-3, audit #67 / #79). + * + * `throwIfAborted()` between chunks is not enough on its own: a `reader.read()` that never resolves + * is never raced against anything, so the drain parks inside it, the call never settles, and + * `source.locked` stays `true` for the rest of the process — the opposite of the seam's promise that + * "an aborted call never leaves the caller's source locked". Racing the pending operation is what + * makes that promise true rather than aspirational. + * + * The signal's `reason` is surfaced verbatim, never re-typed: a caller aborting with its own error + * gets that error back, and a bare `abort()` gets the platform's `AbortError` `DOMException`, which + * is exactly what `throwIfAborted()` would have thrown. + * + * One listener per call, not one per chunk — a 10 000-chunk body would otherwise register and remove + * 10 000 listeners on a signal the caller may hold for the life of a request. + * + * @param signal - the caller's signal, or `undefined` when the call took none. + * @returns a race bound to `signal`, whose `release()` removes the listener. + * @internal + */ +export function abortRace(signal: AbortSignal | undefined): AbortRace { + if (signal === undefined) return UNRACED; + + let onAbort = (): void => undefined; + const aborted = new Promise((_resolve, reject) => { + onAbort = (): void => { + // The seam documents that a caller sees its own abort `reason` verbatim, and a caller may + // abort with any value at all — `controller.abort('gone')` is legal. Re-typing it here would + // break that contract, and it is also exactly what `throwIfAborted()` throws. + // eslint-disable-next-line @typescript-eslint/prefer-promise-reject-errors -- see above; re-enable if the seam ever narrows `reason` to an Error + reject(signal.reason as unknown); + }; + signal.addEventListener('abort', onAbort, {once: true}); + }); + // An abort can land while nothing is racing this promise — between two reads, or after the last + // one and before `release()`. That rejection would be an unhandled one, which takes the process + // down under Node's default policy (`docs/knowledge/harvested/cancellation-and-timeouts.md:26`). + // A no-op handler marks it handled without stopping `Promise.race` below from seeing it. + void aborted.catch(() => undefined); + + return Object.freeze({ + async race(operation: Promise): Promise { + signal.throwIfAborted(); + // A pending `operation` that rejects after losing the race is still settled through + // `Promise.race`'s own handler, so it never becomes an unhandled rejection either — measured + // on Bun 1.3.14 and Node 20.3/26, where releasing a reader with a read outstanding rejects + // that read (`AbortError` on Bun, `TypeError` on Node). + return Promise.race([operation, aborted]); + }, + release(): void { + signal.removeEventListener('abort', onAbort); + }, + }); +} diff --git a/packages/codec-json/src/json-serde.test.ts b/packages/codec-json/src/json-serde.test.ts index 2de27f1..db85038 100644 --- a/packages/codec-json/src/json-serde.test.ts +++ b/packages/codec-json/src/json-serde.test.ts @@ -32,6 +32,37 @@ async function rejection(promise: Promise): Promise { } } +/** How long a raced abort is given to settle a parked read or write before the case fails. */ +const SETTLE_MS = 250; + +/** + * Fails with a named error instead of letting the runner time out, so a regression reads as "the + * abort never settled the call" rather than as a five-second stall with no diagnosis. + * + * `Promise.race` keeps a handler on `promise`, so a later rejection of the losing side is never an + * unhandled one. + */ +async function settleWithin(promise: Promise, ms: number): Promise { + let timer: ReturnType | undefined; + const deadline = new Promise((_resolve, reject) => { + timer = setTimeout(() => { + reject(new Error(`did not settle within ${String(ms)}ms`)); + }, ms); + }); + try { + return await Promise.race([promise, deadline]); + } finally { + clearTimeout(timer); + } +} + +/** One macrotask, which is long enough for a drain loop to reach its second, parked read. */ +function untilParked(): Promise { + return new Promise(resolve => { + setTimeout(resolve, 5); + }); +} + test('declares application/json as its wire media type', () => { expect(jsonSerde().mediaType).toBe('application/json'); }); @@ -673,3 +704,140 @@ describe('the options argument stays optional on both stream methods', () => { ).toEqual({a: 1}); }); }); + +// Module-scope, not describe-local: the pending-abort suite is split across sibling describes to +// stay inside `max-lines-per-function`, and both halves need these. +const PENDING_ABORT_SERDE = jsonSerde(); +const passthroughSchema: Schema = {parse: (i: unknown) => i}; + +/** + * Hands over one chunk and then never produces another, so the drain parks *inside* + * `reader.read()` — the state a between-chunks signal check structurally cannot observe. + */ +function stallingSource( + first: string, + onCancel?: () => void, +): ReadableStream { + return new ReadableStream({ + start(controller) { + controller.enqueue(new TextEncoder().encode(first)); + }, + pull() { + return new Promise(() => undefined); + }, + cancel() { + onCancel?.(); + }, + }); +} + +describe('an abort that lands while a READ is pending (audit #67 / #79)', () => { + test('deserializeFrom settles with the caller reason and unlocks the source (SERDE-3)', async () => { + let cancelled = false; + const source = stallingSource('{"a":', () => { + cancelled = true; + }); + const controller = new AbortController(); + const reason = new Error('the caller gave up mid-drain'); + + const settled = rejection( + PENDING_ABORT_SERDE.deserializer.deserializeFrom( + source, + {schema: passthroughSchema}, + {signal: controller.signal}, + ), + ); + await untilParked(); + controller.abort(reason); + + expect(await settleWithin(settled, SETTLE_MS)).toBe(reason); + // The whole point of the fix: the caller gets its stream back, still usable. + expect(source.locked).toBe(false); + expect(cancelled).toBe(false); + }); + + test('an abort with no reason surfaces the platform AbortError the seam documents', async () => { + const source = stallingSource('{"a":'); + const controller = new AbortController(); + + const settled = rejection( + PENDING_ABORT_SERDE.deserializer.deserializeFrom( + source, + {schema: passthroughSchema}, + {signal: controller.signal}, + ), + ); + await untilParked(); + controller.abort(); + + expect(await settleWithin(settled, SETTLE_MS)).toMatchObject({ + name: 'AbortError', + }); + expect(source.locked).toBe(false); + }); +}); + +describe('an abort that lands while a WRITE is pending (audit #67 / #79)', () => { + test('serializeTo settles with the caller reason and unlocks the sink (SERDE-3)', async () => { + let closed = false; + let aborted = false; + const sink = new WritableStream({ + write() { + return new Promise(() => undefined); + }, + close() { + closed = true; + }, + abort() { + aborted = true; + }, + }); + const controller = new AbortController(); + const reason = new Error('the caller gave up mid-write'); + + const settled = rejection( + PENDING_ABORT_SERDE.serializer.serializeTo({a: 1}, sink, { + signal: controller.signal, + }), + ); + await untilParked(); + controller.abort(reason); + + expect(await settleWithin(settled, SETTLE_MS)).toBe(reason); + expect(sink.locked).toBe(false); + expect(closed).toBe(false); + expect(aborted).toBe(false); + }); + + test('a signal that never fires leaves both directions unchanged', async () => { + const controller = new AbortController(); + const source = new ReadableStream({ + start(streamController) { + streamController.enqueue(new TextEncoder().encode('{"a":')); + streamController.enqueue(new TextEncoder().encode('1}')); + streamController.close(); + }, + }); + const written: string[] = []; + const sink = new WritableStream({ + write(chunk) { + written.push(new TextDecoder().decode(chunk)); + }, + }); + + expect( + await PENDING_ABORT_SERDE.deserializer.deserializeFrom( + source, + {schema: passthroughSchema}, + {signal: controller.signal}, + ), + ).toEqual({a: 1}); + await PENDING_ABORT_SERDE.serializer.serializeTo({a: 1}, sink, { + signal: controller.signal, + }); + + expect(written.join('')).toBe('{"a":1}'); + expect(source.locked).toBe(false); + expect(sink.locked).toBe(false); + }); +}); diff --git a/packages/codec-json/src/json-serde.ts b/packages/codec-json/src/json-serde.ts index cd7086d..d33cf67 100644 --- a/packages/codec-json/src/json-serde.ts +++ b/packages/codec-json/src/json-serde.ts @@ -8,6 +8,7 @@ import { type Serde, type Serializer, } from '@dexpace/core'; +import {abortRace} from './abort-race.js'; import { degradeTopLevelTristate, tristateReplacer, @@ -128,14 +129,22 @@ function makeSerializer(wiring: TristateWiring): Serializer { // Encoded before the lock is taken: a failed encode then leaves the caller's sink untouched // and still usable, rather than locked-and-released around a write that never happened. const bytes = encodeToBytes(value, wiring); - // Checked after the encode and before the lock, so an aborted call leaves the sink neither - // locked nor closed (SERDE-3). One write follows, so there is no loop to check inside. - options?.signal?.throwIfAborted(); + // Checked after the encode and before the lock, so an aborted call never takes the lock at + // all (SERDE-3). + const signal = options?.signal; + signal?.throwIfAborted(); const writer = sink.getWriter(); + // After `getWriter()`, so a contended sink leaves no listener behind: the `TypeError` is + // thrown before there is one to remove. + const race = abortRace(signal); try { - await writer.write(bytes); + // Raced, not merely checked before: one write is still one operation that can park + // indefinitely against a slow sink, and the abort has to reach it (audit #67 / #79). + await race.race(writer.write(bytes)); } finally { - // Release the lock, never close: the sink is caller-owned (SERDE-3). + race.release(); + // Release the lock, never close: the sink is caller-owned (SERDE-3). A write still + // outstanding at this point stays outstanding — aborting it is the owner's call, not ours. writer.releaseLock(); } }, @@ -209,24 +218,31 @@ function makeDeserializer(): Deserializer { // A streaming TextDecoder keeps multi-byte characters intact across chunk boundaries; decoding // each chunk independently would corrupt any character split across two reads. // Checked before the lock so an aborted call leaves the source neither locked nor cancelled - // (SERDE-3), and again after every read so a long drain stops promptly. + // (SERDE-3), and raced against every read so a stalled one stops too. const signal = options?.signal; signal?.throwIfAborted(); const decoder = new TextDecoder('utf-8'); const reader = source.getReader(); + // After `getReader()`, so a contended source leaves no listener behind: the `TypeError` is + // thrown before there is one to remove. + const race = abortRace(signal); let text = ''; try { for (;;) { - // Serial by necessity: each read depends on the previous one advancing the cursor. - const {done, value} = await reader.read(); + // Serial by necessity: each read depends on the previous one advancing the cursor. Raced + // rather than checked between reads: a source that stalls mid-body parks the loop inside + // `read()`, where a between-chunks check never runs again (audit #67 / #79). + const {done, value} = await race.race(reader.read()); if (done) break; - signal?.throwIfAborted(); text += decoder.decode(value, {stream: true}); } text += decoder.decode(); } finally { + race.release(); // Release the lock, never cancel: the source is caller-owned (SERDE-3). A stream failure // surfaces from `read()` and propagates unwrapped (SERDE-12) — it is not caught here. + // Releasing with a read outstanding is legal on every supported runtime and unlocks the + // stream; the outstanding read rejects, and `Promise.race` above still owns that rejection. reader.releaseLock(); } return decodeText(text, target); diff --git a/packages/codec-json/src/tristate-schema.test.ts b/packages/codec-json/src/tristate-schema.test.ts index c6c248c..2c68193 100644 --- a/packages/codec-json/src/tristate-schema.test.ts +++ b/packages/codec-json/src/tristate-schema.test.ts @@ -1,10 +1,16 @@ // SPDX-License-Identifier: MIT // packages/codec-json/src/tristate-schema.test.ts -// Exercises: SERDE-16 (missing → Absent, explicit null → Null, value → Present with element type preserved), -// SERDE-17 (a missing key resolves to Absent via the combinator's own default, not a JSON.parse reviver), -// SERDE-29 (both combinators return frozen schemas, so a shared schema cannot acquire state). +// Exercises: SERDE-14 (three states and only three — a Present can never carry null), SERDE-16 (missing → +// Absent, explicit null → Null, value → Present with element type preserved), SERDE-17 (a missing key resolves +// to Absent via the combinator's own default, not a JSON.parse reviver), SERDE-29 (both combinators return +// frozen schemas, so a shared schema cannot acquire state). import {expect, test} from 'bun:test'; -import {valueOrNull, type Schema, type Tristate} from '@dexpace/core'; +import { + DeserializationError, + valueOrNull, + type Schema, + type Tristate, +} from '@dexpace/core'; import {expectTypeOf} from 'expect-type'; import {MISSING, tristate, tristateObject} from './tristate-schema.js'; @@ -219,3 +225,38 @@ test('both combinators return frozen schemas, like the bundle itself', () => { expect(Object.isFrozen(tristate(identity))).toBe(true); expect(Object.isFrozen(tristateObject({a: identity}))).toBe(true); }); + +// SERDE-14 has three states. `present(null)` is a fourth, and the type system alone cannot keep it +// out: `present` takes `NonNullable`, but `inner.parse`'s declared `T` is unconstrained, so the +// cast that satisfies the compiler is exactly where a normalizing schema slips through +// (audit #67 / #79). +const nullifying: Schema = {parse: () => null}; +const erasing: Schema = {parse: () => undefined}; + +test('an inner schema that normalizes a value to null is a decode failure (SERDE-14)', () => { + expect(() => tristate(nullifying).parse('a value')).toThrow( + DeserializationError, + ); + expect(() => tristate(nullifying).parse('a value')).toThrow( + /present Tristate cannot carry null/, + ); +}); + +test('an inner schema that normalizes a value to undefined is rejected the same way', () => { + expect(() => tristate(erasing).parse('a value')).toThrow( + DeserializationError, + ); +}); + +test('the wire null and missing-key paths still decode ahead of that check (SERDE-16)', () => { + // Neither reaches `inner.parse`, so a normalizing inner schema cannot turn a legitimate Null or + // Absent into a failure. + expect(tristate(nullifying).parse(null).kind).toBe('null'); + expect(tristate(nullifying).parse(MISSING).kind).toBe('absent'); +}); + +test('a nullifying field schema fails the whole tristateObject decode (SERDE-14)', () => { + expect(() => tristateObject({age: nullifying}).parse({age: 30})).toThrow( + DeserializationError, + ); +}); diff --git a/packages/codec-json/src/tristate-schema.ts b/packages/codec-json/src/tristate-schema.ts index d201233..1e473b1 100644 --- a/packages/codec-json/src/tristate-schema.ts +++ b/packages/codec-json/src/tristate-schema.ts @@ -2,6 +2,7 @@ // packages/codec-json/src/tristate-schema.ts import { absent, + DeserializationError, nullValue, present, type Schema, @@ -31,6 +32,8 @@ export const MISSING: unique symbol = Symbol('@dexpace/codec-json.missing'); * @param inner - the schema for the value a Present carries. * @returns a schema producing `Tristate`: a missing-key sentinel or `undefined` yields Absent, a wire * `null` yields Null, anything else runs through `inner` and yields Present. + * @throws DeserializationError when `inner` resolves a present value to `null` or `undefined`. SERDE-14 + * has three states; a Present carrying nothing would be a fourth, and the wire said the key was there. * @public */ export function tristate(inner: Schema): Schema> { @@ -40,9 +43,23 @@ export function tristate(inner: Schema): Schema> { parse(input: unknown): Tristate { if (input === MISSING || input === undefined) return absent(); if (input === null) return nullValue(); - // `as NonNullable`: the null and undefined branches returned above, so the value cannot be - // nullish — a fact the compiler cannot derive through `inner.parse`'s unconstrained `T`. - return present(inner.parse(input) as NonNullable); + const value = inner.parse(input); + // The type system alone cannot hold SERDE-14's third state to non-null values: `present` takes + // `NonNullable`, but `T` here is whatever the caller's schema declares, so a schema that + // NORMALIZES to null — a Zod `.transform()`, a "" → null cleanup — type-checks and produces + // `{kind: 'present', value: null}`, the fourth state the union exists to forbid. Checked at + // run time, and reported as a decode failure rather than an assertion: the input came off the + // wire, and the pairing of that input with that schema is what has no Tristate (audit #67 / + // #79). `undefined` is rejected with it — `NonNullable` excludes both, and a Present of + // `undefined` is Absent wearing the wrong label. + if (value === null || value === undefined) { + throw new DeserializationError( + 'a present Tristate cannot carry null or undefined; the inner schema resolved a wire value to one (SERDE-14)', + ); + } + // `as NonNullable`: the nullish cases have all returned or thrown above, a fact the + // compiler cannot derive through `inner.parse`'s unconstrained `T`. + return present(value); }, }); } @@ -62,6 +79,8 @@ export function tristate(inner: Schema): Schema> { * @param shape - a schema per Tristate-decoded field, keyed by wire name. * @returns a schema producing an object whose named keys are `Tristate`-wrapped. * @throws TypeError when the value being parsed is not a non-null object, or is an array. + * @throws DeserializationError when a field's own schema resolves a present wire value to `null` or + * `undefined`, which is {@link tristate}'s check applied per field (SERDE-14). * @public */ export function tristateObject>>( diff --git a/packages/core/src/http/errors.ts b/packages/core/src/http/errors.ts index 66c0f83..bc8e01a 100644 --- a/packages/core/src/http/errors.ts +++ b/packages/core/src/http/errors.ts @@ -140,6 +140,10 @@ export class ProtocolParseError extends DexpaceError {} * fail; `QueryParamsBuilder.add` rejects it at the call that supplied it instead. `cause` is not * set on this path — nothing was caught, the input was inspected (HTTP-29, audit #67 / #76). * `QueryParams.parse` does NOT throw it: HTTP-31 makes parsing lenient, so it substitutes U+FFFD. + * Pagination's query splice — `spliceQueryParam` and `readQueryParam`, behind `cursorStrategy` + * and `pageNumberStrategy` — rejects the same input for the same reason, through the same + * predicate (PAGE-22, audit #67 / #79). That one is reachable without any caller mistake, since + * the cursor is server-supplied; its message names the parameter and never echoes the value. * * @public */ diff --git a/packages/core/src/pagination/lifecycle.test.ts b/packages/core/src/pagination/lifecycle.test.ts index 5ea37ab..9a5cb27 100644 --- a/packages/core/src/pagination/lifecycle.test.ts +++ b/packages/core/src/pagination/lifecycle.test.ts @@ -1,9 +1,10 @@ // SPDX-License-Identifier: MIT // packages/core/src/pagination/lifecycle.test.ts -// Exercises: PAGE-11 (close BEFORE yielding items — the assertion appendix B does not make), PAGE-12 -// (close-on-abandon), PAGE-13 (parse failure closes inline, close error suppressed), PAGE-14 (single-use page -// view), PAGE-15 (close errors surface), PAGE-27 (exactly once on every path), PAGE-32 (consumer throw keeps -// consumer error primary, discarding return-phase close error). +// Exercises: PAGE-4 (a malformed parse result closes the response and names the invariant), PAGE-11 (close +// BEFORE yielding items — the assertion appendix B does not make), PAGE-12 (close-on-abandon), PAGE-13 (parse +// failure closes inline, close error suppressed), PAGE-14 (single-use page view), PAGE-15 (close errors +// surface), PAGE-27 (exactly once on every path), PAGE-32 (consumer throw keeps consumer error primary, +// discarding return-phase close error). import {expect, test} from 'bun:test'; import {FakeTransport, countingResponse} from '../testing/fake-transport.js'; import {IoError} from '../io/errors.js'; @@ -451,3 +452,78 @@ test.each([ } }, ); + +// A strategy is caller code, and `parse`'s declared return type does not survive the seam: an +// `any`-typed JSON decode, a forgotten `return`, or a server field the caller trusted all land here +// as a shape the engine's own types say cannot exist (PAGE-4). The casts below ARE the test — they +// reproduce the four values that reach `#walk` in practice. +function malformedStrategy(result: unknown): PaginationStrategy { + return {parse: () => Promise.resolve(result as PageInfo)}; +} + +test.each([ + ['undefined', undefined, /never null or undefined/], + ['null', null, /never null or undefined/], + ['{items: null}', {items: null, nextRequest: undefined}, /PageInfo\.items/], + ['{items: undefined}', {nextRequest: undefined}, /PageInfo\.items/], +])( + 'a strategy that returns %s closes the response exactly once and names the invariant (PAGE-4, PAGE-27)', + async (_name, result, message) => { + const closed: number[] = []; + const transport = transportOf(1, index => closed.push(index)); + const paginator = new Paginator({ + transport, + initialRequest: initialRequest(), + strategy: malformedStrategy(result), + }); + + let caught: unknown; + try { + for await (const page of paginator.pages()) { + void page; + } + } catch (e: unknown) { + caught = e; + } + + // Before the fix `items: null` reached the spread in `Page`'s constructor and surfaced as a + // bare `TypeError` from array iteration, which names nothing a caller can act on. + expect((caught as Error).message).toMatch(message); + expect(closed).toEqual([0]); + }, +); + +test('a close failure while rejecting a malformed PageInfo is suppressed, not masking (PAGE-4, PAGE-13)', async () => { + const closeFailure = new IoError('close failed'); + const transport = new FakeTransport([ + countingResponse({ + status: 200, + headers: {}, + body: '{}', + onCancel: () => { + throw closeFailure; + }, + }), + ]); + const paginator = new Paginator({ + transport, + initialRequest: initialRequest(), + strategy: malformedStrategy(undefined), + }); + + let caught: unknown; + try { + for await (const page of paginator.pages()) { + void page; + } + } catch (e: unknown) { + caught = e; + } + + const suppressed = caught as SuppressedErrorLike; + expect(suppressed.name).toBe('SuppressedError'); + expect((suppressed.error as Error).message).toMatch( + /never null or undefined/, + ); + expect(suppressed.suppressed).toBe(closeFailure); +}); diff --git a/packages/core/src/pagination/page.test.ts b/packages/core/src/pagination/page.test.ts index 9bf8487..3c4901a 100644 --- a/packages/core/src/pagination/page.test.ts +++ b/packages/core/src/pagination/page.test.ts @@ -1,7 +1,8 @@ // SPDX-License-Identifier: MIT // packages/core/src/pagination/page.test.ts -// Exercises: PAGE-2 (items and metadata survive close; items never null), PAGE-3 (one owned response, closed -// exactly once), PAGE-4 (PageInfo shape, undefined next-request is the end signal). +// Exercises: PAGE-2 (items and metadata survive close; items never null, at construction too), PAGE-3 (one +// owned response, never null, closed exactly once), PAGE-4 (PageInfo shape, undefined next-request is the end +// signal). import {expect, test} from 'bun:test'; import {Page, pageInfo} from './page.js'; @@ -125,3 +126,25 @@ test('no "undefined" prototype key survives the guarded install (PAGE-12)', () = 'undefined', ); }); + +// `Page` is `@public`, so these guards are reachable from consumer code, not only from the walk — +// and until audit #67 / #79 their messages said "never null" while the check tested `!== undefined`. +// A `null` therefore reached the item copy and surfaced as a bare `TypeError` from spread. +test.each([ + ['null items', null, /items must never be null/], + ['undefined items', undefined, /items must never be null/], +])('a Page rejects %s at construction (PAGE-2)', (_name, items, message) => { + const {response} = fakeResponse(); + expect(() => + makePage(response, items as unknown as readonly number[]), + ).toThrow(message); +}); + +test('a Page rejects a null response at construction (PAGE-3)', () => { + expect(() => + makePage( + null as unknown as ConstructorParameters>[0], + [1], + ), + ).toThrow(/must own a response/); +}); diff --git a/packages/core/src/pagination/page.ts b/packages/core/src/pagination/page.ts index 9553812..0391257 100644 --- a/packages/core/src/pagination/page.ts +++ b/packages/core/src/pagination/page.ts @@ -67,12 +67,15 @@ export class Page { readonly #response: Response; constructor(response: Response, items: readonly T[]) { + // `!== null` as well as `!== undefined`: both messages have always said "null", and testing only + // for `undefined` let a `null` through to the item copy below, where it surfaced as a bare + // `TypeError` from spread — outside the error tree and naming neither field (audit #67 / #79). invariant( - (response as unknown) !== undefined, + (response as unknown) !== undefined && (response as unknown) !== null, 'a Page must own a response (PAGE-3)', ); invariant( - (items as unknown) !== undefined, + (items as unknown) !== undefined && (items as unknown) !== null, 'a Page’s items must never be null (PAGE-2)', ); diff --git a/packages/core/src/pagination/paginator.ts b/packages/core/src/pagination/paginator.ts index e331316..2a41f91 100644 --- a/packages/core/src/pagination/paginator.ts +++ b/packages/core/src/pagination/paginator.ts @@ -196,20 +196,7 @@ export class Paginator { response, request, ); - - // PAGE-4: parse must always return a well-formed result and must never signal termination through a - // side channel. A strategy that returns nothing is a programmer error, so it crashes at the fault - // rather than silently ending the walk as if the server had run out of pages. - invariant( - (info as unknown) !== undefined, - 'PaginationStrategy.parse must return a PageInfo, never undefined', - ); - invariant( - (info.items as unknown) !== undefined, - 'PageInfo.items must never be null or absent (PAGE-2)', - ); - - held = new Page(response, info.items); + held = await pageOrClose(response, info); request = info.nextRequest; yield held; } @@ -236,17 +223,67 @@ async function parseOrClose( try { return await strategy.parse(response, template); } catch (parseError: unknown) { - try { - await response.close(); - } catch (closeError: unknown) { - throw suppress( - parseError, - closeError, - 'pagination parse failed and releasing the response also failed', - ); - } - throw parseError; + return closeThenRethrow(response, parseError, 'pagination parse failed'); + } +} + +/** + * PAGE-4: `parse` must always return a well-formed result, and must never signal termination through a side + * channel. A strategy that returns nothing is a programmer error, so the walk crashes at the fault rather than + * silently ending as if the server had run out of pages. + * + * PAGE-27: and it crashes *after* releasing the response. `parse` returning a malformed value is the one exit + * from this loop the `finally` in `#walk` cannot cover — `held` is still `undefined` there, because assigning it + * is precisely what failed — so, like PAGE-13's parse rejection, the release happens inline (audit #67 / #79). + * + * Both checks reject `null` as well as `undefined`, which is what their messages have always claimed. Testing + * only for `undefined` let `{items: null}` through to `Page`'s constructor, where the item copy surfaced as a + * bare `TypeError` from spread — naming nothing a caller could act on, and leaking the response on the way. + * + * Not async: the only asynchrony here is the close, and only on the failure path. + */ +function pageOrClose( + response: Response, + info: PageInfo, +): Promise> { + try { + invariant( + (info as unknown) !== undefined && (info as unknown) !== null, + 'PaginationStrategy.parse must return a PageInfo, never null or undefined', + ); + invariant( + (info.items as unknown) !== undefined && (info.items as unknown) !== null, + 'PageInfo.items must never be null or absent (PAGE-2)', + ); + return Promise.resolve(new Page(response, info.items)); + } catch (buildError: unknown) { + return closeThenRethrow( + response, + buildError, + 'the pagination strategy returned a malformed PageInfo', + ); + } +} + +/** + * Release `response`, then rethrow `primary`. Shared by the two inline-close paths so they cannot drift: a close + * failure is attached as suppressed and never masks the failure that got here first (PAGE-13, PAGE-15). + */ +async function closeThenRethrow( + response: Response, + primary: unknown, + context: string, +): Promise { + try { + await response.close(); + } catch (closeError: unknown) { + throw suppress( + primary, + closeError, + `${context} and releasing the response also failed`, + ); } + throw primary; } /** PAGE-26: on an already-settled cancellation path, a close error is swallowed — nothing is left to report to. */ diff --git a/packages/core/src/pagination/query-splice.property.test.ts b/packages/core/src/pagination/query-splice.property.test.ts index f6c6761..56a51af 100644 --- a/packages/core/src/pagination/query-splice.property.test.ts +++ b/packages/core/src/pagination/query-splice.property.test.ts @@ -1,7 +1,8 @@ // SPDX-License-Identifier: MIT // packages/core/src/pagination/query-splice.property.test.ts -import {test} from 'bun:test'; +import {expect, test} from 'bun:test'; import fc from 'fast-check'; +import {UrlConstructionError} from '../http/errors.js'; import {readQueryParam, spliceQueryParam} from './query-splice.js'; /** @@ -51,3 +52,40 @@ test('write-then-read is the identity for any value (PAGE-22)', () => { }), ); }); + +/** + * Strings that mix ordinary query text with UNPAIRED surrogate code units — the same generator + * `http/query-params.test.ts` uses, for the same reason: `fc.string()`'s default unit is printable + * ASCII, so the `URIError` path would otherwise go ungenerated. That is exactly why the identity + * property above never caught it. + */ +const surrogateBearingString = fc.string({ + unit: fc.oneof( + fc.constantFrom('a', 'b', ' ', '=', '&', '%', '+', '\u{1F600}'), + fc + .integer({min: 0xd800, max: 0xdfff}) + .map(code => String.fromCharCode(code)), + ), + maxLength: 8, +}); + +test('no URIError escapes the splice or the read, whatever a server sent (PAGE-22)', () => { + fc.assert( + fc.property( + surrogateBearingString, + surrogateBearingString, + (name, value) => { + const url = new URL('https://h/p?a=1'); + try { + const out = spliceQueryParam(url, name, value); + expect(readQueryParam(out, name)).toBe(value); + } catch (e: unknown) { + // The one sanctioned failure: inside the error tree, from the call that was handed the + // value. A `URIError` here means the guard was bypassed. + expect(e).toBeInstanceOf(UrlConstructionError); + } + }, + ), + {numRuns: 500}, + ); +}); diff --git a/packages/core/src/pagination/query-splice.test.ts b/packages/core/src/pagination/query-splice.test.ts index dc1ca1c..ca546a7 100644 --- a/packages/core/src/pagination/query-splice.test.ts +++ b/packages/core/src/pagination/query-splice.test.ts @@ -1,9 +1,10 @@ // SPDX-License-Identifier: MIT // packages/core/src/pagination/query-splice.test.ts // Exercises: PAGE-21 (verbatim splice, untargeted params byte-for-byte), PAGE-22 (RFC 3986 component encoding, -// literal + is data), PAGE-23 (replace-first / append / remove, order preserved), PAGE-24 (non-query components -// preserved exactly). -import {expect, test} from 'bun:test'; +// literal + is data; a component with no UTF-8 form is rejected as UrlConstructionError), PAGE-23 +// (replace-first / append / remove, order preserved), PAGE-24 (non-query components preserved exactly). +import {describe, expect, test} from 'bun:test'; +import {UrlConstructionError} from '../http/errors.js'; import {readQueryParam, spliceQueryParam} from './query-splice.js'; const at = (href: string): URL => new URL(href); @@ -139,3 +140,57 @@ test('stray empty segments are skipped, matching HTTP-31 query parsing', () => { query(spliceQueryParam(at('https://h/p?a=1&&b=2&page=1'), 'page', '2')), ).toBe('a=1&b=2&page=2'); }); + +describe('a cursor with no UTF-8 form is rejected here, not inside encodeURIComponent (PAGE-22)', () => { + // The splice shares `HTTP-29`'s component encoder, and `encodeURIComponent` throws a bare + // `URIError: URI malformed` on a string carrying an unpaired surrogate. A cursor is SERVER + // -supplied — `{"next":"\ud800"}` is well-formed JSON — so this is reachable without any caller + // mistake, and until audit #67 / #79 it escaped the `DexpaceError` tree entirely. #76 closed the + // same hole at `QueryParamsBuilder.add` and left this one named. + const LONE_HIGH = '\uD800'; + const LONE_LOW = '\uDFFF'; + + test.each([ + ['a lone high surrogate', LONE_HIGH], + ['a lone low surrogate', LONE_LOW], + ['a lone surrogate inside a longer cursor', `ok${LONE_HIGH}ok`], + ])('spliceQueryParam rejects %s as a value', (_label, value) => { + expect(() => + spliceQueryParam(at('https://h/p?a=1'), 'cursor', value), + ).toThrow(UrlConstructionError); + }); + + test('the message names the parameter and never echoes the value', () => { + expect(() => + spliceQueryParam(at('https://h/p?a=1'), 'cursor', `secret${LONE_HIGH}`), + ).toThrow(/value of query parameter "cursor"/); + expect(() => + spliceQueryParam(at('https://h/p?a=1'), 'cursor', `secret${LONE_HIGH}`), + ).not.toThrow(/secret/); + }); + + test('spliceQueryParam rejects a lone surrogate in the parameter NAME', () => { + expect(() => + spliceQueryParam(at('https://h/p?a=1'), LONE_HIGH, '2'), + ).toThrow(UrlConstructionError); + }); + + test('readQueryParam rejects a lone surrogate in the parameter NAME', () => { + expect(() => readQueryParam(at('https://h/p?a=1'), LONE_HIGH)).toThrow( + UrlConstructionError, + ); + }); + + test('removing a parameter still validates the name', () => { + expect(() => + spliceQueryParam(at('https://h/p?a=1'), LONE_HIGH, undefined), + ).toThrow(UrlConstructionError); + }); + + test('a well-formed surrogate PAIR is ordinary text and splices normally', () => { + // Rejecting this too would make the rule "no astral characters", which PAGE-22 does not say. + const out = spliceQueryParam(at('https://h/p?a=1'), 'cursor', '\u{1F600}'); + expect(query(out)).toBe('a=1&cursor=%F0%9F%98%80'); + expect(readQueryParam(out, 'cursor')).toBe('\u{1F600}'); + }); +}); diff --git a/packages/core/src/pagination/query-splice.ts b/packages/core/src/pagination/query-splice.ts index ba38d77..754cc6a 100644 --- a/packages/core/src/pagination/query-splice.ts +++ b/packages/core/src/pagination/query-splice.ts @@ -1,9 +1,40 @@ // SPDX-License-Identifier: MIT // packages/core/src/pagination/query-splice.ts +import {UrlConstructionError} from '../http/errors.js'; import { decodeQueryComponent, encodeQueryComponent, } from '../http/query-params.js'; +import {hasLoneSurrogate} from '../http/rfc3986.js'; + +/** + * `encodeQueryComponent` is `encodeURIComponent`, which throws a bare `URIError: URI malformed` on a string + * carrying an unpaired surrogate — such a string has no UTF-8 form, so RFC 3986 percent-encoding is undefined + * for it (PAGE-22, HTTP-29). + * + * Reachable here without any caller mistake, which is what separates this call site from the others #76 closed: + * a cursor is SERVER-supplied, and `{"next":"\ud800"}` is well-formed JSON that `JSON.parse` hands back + * verbatim. The failure was a bare `URIError` from inside a strategy's `parse`, outside the `DexpaceError` tree + * and naming neither the parameter nor the page it came from (audit #67 / #79). + * + * The same class `QueryParamsBuilder.add` throws for the same input, since it is the same defect in the same + * encoder; `hasLoneSurrogate` is the single-sourced predicate, so the two cannot drift. The value itself is + * never echoed — a cursor is opaque server state and can carry a session token. + */ +function requireEncodable( + what: 'name' | 'value', + parameterName: string, + text: string, +): void { + if (!hasLoneSurrogate(text)) return; + const subject = + what === 'name' + ? 'a query parameter name' + : `the value of query parameter "${parameterName}"`; + throw new UrlConstructionError( + `${subject} contains an unpaired surrogate and cannot be percent-encoded`, + ); +} /** * Rewrite one query parameter, splicing the raw query string rather than re-rendering it (PAGE-21–PAGE-24). @@ -19,6 +50,9 @@ import { * Passing `undefined` removes the parameter. Setting replaces the first occurrence in place and drops later * duplicates — the single-value convention paging parameters follow. Everything else is copied byte-for-byte. * + * @throws UrlConstructionError when `name` or `value` carries an unpaired surrogate, and so has no + * percent-encoded form. + * * @internal */ export function spliceQueryParam( @@ -26,6 +60,8 @@ export function spliceQueryParam( name: string, value: string | undefined, ): URL { + requireEncodable('name', name, name); + if (value !== undefined) requireEncodable('value', name, value); const encodedName = encodeQueryComponent(name); const segments = splitQuery(url.search); @@ -60,9 +96,12 @@ export function spliceQueryParam( * A literal `+` reads back as `+`, `%20` as a space, a value-less flag as the empty string, and an absent name * as `undefined`. First match wins. * + * @throws UrlConstructionError when `name` carries an unpaired surrogate, and so has no percent-encoded form. + * * @internal */ export function readQueryParam(url: URL, name: string): string | undefined { + requireEncodable('name', name, name); const encodedName = encodeQueryComponent(name); for (const segment of splitQuery(url.search)) { if (nameOf(segment) !== encodedName) continue; diff --git a/packages/core/src/pagination/strategies.test.ts b/packages/core/src/pagination/strategies.test.ts index 513f522..8eebdf1 100644 --- a/packages/core/src/pagination/strategies.test.ts +++ b/packages/core/src/pagination/strategies.test.ts @@ -5,8 +5,10 @@ // PAGE-18/19/20 (link header: rel=next, RFC 3986 reference resolution, query-only reference preserves the path, // unresolvable target ends the stream without throwing, and the spec's own `` conformance fixture // resolving as a relative reference instead -- recorded as a deliberate reading in docs/deviations.md under -// "Deviations recorded outside a phase" (2026-09-04, audit #67 / #69)). +// "Deviations recorded outside a phase" (2026-09-04, audit #67 / #69)), PAGE-22 (a server-supplied cursor with +// no UTF-8 form fails inside the error tree). import {expect, test} from 'bun:test'; +import {DexpaceError, UrlConstructionError} from '../http/errors.js'; import type {Request} from '../http/request.js'; import type {Response} from '../http/response.js'; import { @@ -294,3 +296,40 @@ test('one strategy instance is safe across two concurrent walks (PAGE-5)', async expect(first.nextRequest?.url.search).toBe('?page=2'); expect(second.nextRequest?.url.search).toBe('?page=10'); }); + +// ---- a server-supplied component with no UTF-8 form (PAGE-22, audit #67 / #79) ---- + +test('a cursor carrying an unpaired surrogate fails as UrlConstructionError, not URIError', async () => { + // `{"next":"\ud800"}` is well-formed JSON, so `extract` can hand one back without the caller + // having done anything wrong. Before the fix this surfaced as a bare `URIError: URI malformed` + // from inside `encodeURIComponent`, outside the `DexpaceError` tree. + const strategy = cursorStrategy({ + extract: () => Promise.resolve({items: ['a'], cursor: 'next\uD800'}), + }); + + let caught: unknown; + try { + await strategy.parse(response({}), template('https://api.test/items')); + } catch (e: unknown) { + caught = e; + } + + expect(caught).toBeInstanceOf(UrlConstructionError); + expect(caught).toBeInstanceOf(DexpaceError); +}); + +test('a page-number parameter name carrying an unpaired surrogate fails the same way', async () => { + const strategy = pageNumberStrategy({ + extract: () => Promise.resolve(['a']), + parameterName: 'p\uD800', + }); + + let caught: unknown; + try { + await strategy.parse(response({}), template('https://api.test/items')); + } catch (e: unknown) { + caught = e; + } + + expect(caught).toBeInstanceOf(UrlConstructionError); +}); diff --git a/packages/core/src/pagination/strategies.ts b/packages/core/src/pagination/strategies.ts index d2f0ecf..84bf409 100644 --- a/packages/core/src/pagination/strategies.ts +++ b/packages/core/src/pagination/strategies.ts @@ -23,6 +23,10 @@ function withUrl(template: Request, url: URL): Request { * A `null` **or empty** cursor ends the stream — both, because a server returning `""` for "no more pages" is * common enough that treating it as a real cursor produces an infinite walk. * + * @throws UrlConstructionError from `parse` when the cursor the server sent, or `parameterName`, carries an + * unpaired surrogate: such a string has no UTF-8 form and so no percent-encoded form either (PAGE-22). The + * engine closes the response on that path like any other parse failure (PAGE-13). + * * @public */ export function cursorStrategy(init: { @@ -62,6 +66,9 @@ export function cursorStrategy(init: { * rewrite a step applied on the way out, and that is the page number worth incrementing. An absent, empty, * or non-numeric value falls back to `startPage`; `startPage: 0` supports 0-based servers. * + * @throws UrlConstructionError from `parse` when `parameterName` carries an unpaired surrogate, which has no + * percent-encoded form (PAGE-22). + * * @public */ export function pageNumberStrategy(init: { diff --git a/packages/core/src/seams/serde.ts b/packages/core/src/seams/serde.ts index 0e26146..adced9b 100644 --- a/packages/core/src/seams/serde.ts +++ b/packages/core/src/seams/serde.ts @@ -94,8 +94,10 @@ export interface Serializer { * because the caller owns it (SERDE-3). * * @throws Whatever `options.signal` was aborted with — its `reason`, or a `DOMException` named - * `'AbortError'` when none was given. Checked before the writer lock is taken, so an aborted call - * never leaves the caller's sink locked and never closes it (SERDE-3). + * `'AbortError'` when none was given. Checked before the writer lock is taken, and then raced + * against each pending write, so an aborted call never leaves the caller's sink locked and never + * closes it (SERDE-3). A write parked against a slow sink is the case the pre-check cannot cover; + * the write itself is left outstanding, because aborting it would be taking ownership. * * @remarks Takes `{signal}` because this method drives a stream it did not open, which is the * project-wide test for whether an API owes one. Buffered-bytes APIs — `serialize`, @@ -210,8 +212,11 @@ export interface Deserializer { * not re-typed, because a contended source is a programmer error rather than a decode failure. * * @throws Whatever `options.signal` was aborted with — its `reason`, or a `DOMException` named - * `'AbortError'` when none was given. Checked before the reader lock is taken and between reads, - * so an aborted call never leaves the caller's source locked and never cancels it (SERDE-3). + * `'AbortError'` when none was given. Checked before the reader lock is taken, and then raced + * against each pending read, so an aborted call never leaves the caller's source locked and never + * cancels it (SERDE-3). Racing is the load-bearing half: a source that stalls mid-body parks the + * drain inside a read that a between-reads check can never reach again, and an implementation + * that only checks between reads leaves that call unsettled and that source locked forever. * * @remarks Takes `{signal}` because this method drives a stream it did not open, which is the * project-wide test for whether an API owes one. The abort reaches the drain loop; the CPU-bound diff --git a/packages/core/src/sse/stream.test.ts b/packages/core/src/sse/stream.test.ts index 7f3b866..1c361a4 100644 --- a/packages/core/src/sse/stream.test.ts +++ b/packages/core/src/sse/stream.test.ts @@ -169,6 +169,47 @@ test('a mid-stream read failure releases before propagating, with the close erro expect(closeCount).toBe(1); }); +test('a release failure during an in-flight error is reported exactly once (SSE-29, SSE-30)', async () => { + const readFailure = new IoError('socket reset'); + const closeFailure = new IoError('close failed too'); + const reported: unknown[] = []; + let closeCount = 0; + const web = new ReadableStream({ + start(controller) { + controller.enqueue(new TextEncoder().encode('data: a\n\n')); + controller.error(readFailure); + }, + }); + const stream = new SseStream( + new SseParser(BufferedSource.overStream(web)), + { + close(): Promise { + closeCount += 1; + return Promise.reject(closeFailure); + }, + }, + {onReleaseFailure: e => reported.push(e)}, + ); + + let caught: unknown; + try { + for await (const event of stream) { + void event; + } + } catch (e: unknown) { + caught = e; + } + + const suppressed = caught as SuppressedErrorLike; + expect(suppressed.error).toBe(readFailure); + expect(suppressed.suppressed).toBe(closeFailure); + // SSE-30 scopes the hook to the automatic CLEAN terminal path. With an error already in flight the + // release failure is on the thrown error, and calling the hook as well makes one failure arrive + // twice — once in whatever logs `onReleaseFailure`, once in whatever logs the caught error. + expect(reported).toEqual([]); + expect(closeCount).toBe(1); +}); + test('sseStreamFrom binds lifecycle to the response body (SSE-32)', async () => { let responseClosed = 0; const response = { diff --git a/packages/core/src/sse/stream.ts b/packages/core/src/sse/stream.ts index 980f4ad..919827e 100644 --- a/packages/core/src/sse/stream.ts +++ b/packages/core/src/sse/stream.ts @@ -28,6 +28,11 @@ export interface SseStreamOptions { * Called when a release fails on a clean automatic terminal path, where SSE-30 requires the failure to be * reported out-of-band and swallowed rather than thrown (throwing would discard events already delivered). * + * Called on **that** path only, which is SSE-30's own scope: "no error in flight". A release that fails + * while an error is already propagating is attached to that error as `suppressed` instead, and a release + * that fails during an explicit `close()` rejects that call — reporting either one here as well would + * deliver a single failure twice. + * * Defaults to a no-op. Phase 7 wires a real `Logger` in here without reshaping this class — the same * "mechanism now, wiring later" split Phase 3b used for its logging tees. */ @@ -121,6 +126,9 @@ export class SseStream implements AsyncIterable { } async *#iterate(): AsyncGenerator { + // Whether the catch below already released and already accounted for a release failure. A local, + // not a field: it is read exactly once, by the `finally` of this one generator activation. + let releasedWithError = false; try { for (;;) { // A close observed between pulls ends iteration cleanly, without reading from a torn-down resource. @@ -132,10 +140,17 @@ export class SseStream implements AsyncIterable { } catch (e: unknown) { // SSE-29: release BEFORE the error propagates, and attach a release failure as suppressed rather than // letting it mask the real cause. + releasedWithError = true; await this.#releaseWithInFlightError(e); } finally { // Covers clean end-of-stream and early `break` (the runtime calls `.return()`, which runs this block). - await this.#releaseQuietly(); + // + // Skipped after the catch, which has already released. Running it there awaited the same rejected + // `#closing` promise and handed the close failure to `onReleaseFailure` as well — so one failure was + // reported twice, once out-of-band and once as `suppressed` on the error the consumer catches. SSE-30 + // scopes the hook to the automatic CLEAN terminal path, where there is nothing to throw to; with an + // error in flight there is (audit #67 / #79). + if (!releasedWithError) await this.#releaseQuietly(); } } @@ -176,7 +191,10 @@ export class SseStream implements AsyncIterable { } } - /** SSE-29 / SSE-36: an error is already in flight, so it stays primary and the close error is suppressed. */ + /** + * SSE-29 / SSE-36: an error is already in flight, so it stays primary and the close error is suppressed — + * and NOT also handed to `onReleaseFailure`, which is the clean-terminal path's channel. + */ async #releaseWithInFlightError(primary: unknown): Promise { this.#closed = true; const releasePromise = (this.#closing ??= this.#resource.close()); diff --git a/tests/node-conformance/serde.test.mjs b/tests/node-conformance/serde.test.mjs index cdf48f1..a10141b 100644 --- a/tests/node-conformance/serde.test.mjs +++ b/tests/node-conformance/serde.test.mjs @@ -261,3 +261,122 @@ describe("decodeResponse's close-failure path on the declared Node floor (SERDE- ); }); }); + +/** + * Fails the case instead of hanging it. `node --test` has no default per-test timeout, so a + * regression in the abort race would park the runner for as long as CI allows rather than reporting + * anything. The timer is ref'd, which also holds the loop open while the abort is in flight. + */ +async function settleWithin(promise, ms) { + let timer; + const deadline = new Promise((_resolve, reject) => { + timer = setTimeout( + () => reject(new Error(`did not settle within ${ms}ms`)), + ms, + ); + }); + try { + return await Promise.race([promise, deadline]); + } finally { + clearTimeout(timer); + } +} + +/** Whatever `promise` rejected with, or the string marker when it resolved. */ +async function rejection(promise) { + try { + await promise; + return 'RESOLVED'; + } catch (e) { + return e; + } +} + +describe('an abort landing on a PENDING read or write (SERDE-3, audit #67 / #79)', () => { + // Runtime-divergent twice over. `AbortSignal` and Web Streams are independent implementations + // here, and the two disagree on what a reader release does to an outstanding read: measured + // 2026-09-05, Bun 1.3.14 rejects it with an `AbortError` and Node 20.3/26 with + // `TypeError: Invalid state: Releasing reader`. Neither may reach the caller in place of its own + // abort reason, and neither may escape as an unhandled rejection — which `node --test` would + // report as a failure of this file even if every assertion below passed. + it('settles deserializeFrom with the caller reason and unlocks the source', async () => { + let cancelled = false; + const source = new ReadableStream({ + start(controller) { + controller.enqueue(Buffer.from('{"id"')); + }, + // Parks the drain inside its second `read()`: the state a between-chunks check cannot see. + pull() { + return new Promise(() => {}); + }, + cancel() { + cancelled = true; + }, + }); + const controller = new AbortController(); + const reason = new Error('the caller gave up mid-drain'); + const settled = rejection( + jsonSerde().deserializer.deserializeFrom( + source, + {schema: identity, typeName: 'Dto'}, + {signal: controller.signal}, + ), + ); + const abortAt = setTimeout(() => controller.abort(reason), 5); + + try { + assert.equal(await settleWithin(settled, 2000), reason); + } finally { + clearTimeout(abortAt); + } + assert.equal(source.locked, false, 'the caller must get its source back'); + assert.equal(cancelled, false, 'the source is caller-owned (SERDE-3)'); + }); + + it('settles serializeTo with the caller reason and unlocks the sink', async () => { + let closed = false; + let aborted = false; + const sink = new WritableStream({ + write() { + return new Promise(() => {}); + }, + close() { + closed = true; + }, + abort() { + aborted = true; + }, + }); + const controller = new AbortController(); + const reason = new Error('the caller gave up mid-write'); + const settled = rejection( + jsonSerde().serializer.serializeTo({a: 1}, sink, { + signal: controller.signal, + }), + ); + const abortAt = setTimeout(() => controller.abort(reason), 5); + + try { + assert.equal(await settleWithin(settled, 2000), reason); + } finally { + clearTimeout(abortAt); + } + assert.equal(sink.locked, false, 'the caller must get its sink back'); + assert.equal(closed, false, 'the sink is caller-owned (SERDE-3)'); + assert.equal(aborted, false); + }); + + it('leaves a completed drain untouched when the signal never fires', async () => { + const controller = new AbortController(); + const value = await jsonSerde().deserializer.deserializeFrom( + streamOf(Buffer.from('{"id"'), Buffer.from(':42}')), + {schema: identity}, + {signal: controller.signal}, + ); + + assert.deepEqual(value, {id: 42}); + // The listener is removed on the way out, so a later abort reaches nothing at all — an + // unremoved one would reject a promise nobody is waiting on any more. + controller.abort(new Error('too late')); + }); +});