From 3c72ef0617ce69d3d9aa754bbd7008acc2e32b89 Mon Sep 17 00:00:00 2001 From: Mohammad Wahbeh Date: Sat, 5 Sep 2026 13:10:11 +0300 Subject: [PATCH 1/6] fix(transport-shared): CONTROL_BYTE was excepting LF as well as HTAB MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `degradeInboundHeaders`' inbound-value gate read `/[\x00-\x08\x0B-\x1F\x7F]/u`, which skips `\x0A` along with the intended `\x09`. Nothing observable changed — `Headers.addInbound` applies core's own `hasForbiddenInboundValueByte`, which does reject LF, and the `try`/`catch` two lines down records the same drop — so the two gates were redundant and only one of them was right. The class is now `/[\x00-\x08\x0A-\x1F\x7F]/u`, identical to core's. The constant is exported from the module (not from the barrel) so its test can read the character class directly: a test that went through `degradeInboundHeaders` would have passed against the broken class, which is how this survived from Phase 8a to audit #67 / #82. Found by: audit #67 / #82. --- .../src/header-mapping.test.ts | 46 ++++++++++++++++++- .../transport-shared/src/header-mapping.ts | 19 +++++++- 2 files changed, 63 insertions(+), 2 deletions(-) diff --git a/packages/transport-shared/src/header-mapping.test.ts b/packages/transport-shared/src/header-mapping.test.ts index 4ee6219..6d02976 100644 --- a/packages/transport-shared/src/header-mapping.test.ts +++ b/packages/transport-shared/src/header-mapping.test.ts @@ -4,7 +4,11 @@ // TRANSPORT-12 (per-header graceful degradation), TRANSPORT-14 (lenient inbound copy, obs-text preserved, control-byte header dropped) import {describe, expect, test} from 'bun:test'; import {Headers} from '@dexpace/core'; -import {degradeInboundHeaders, mapOutboundHeaders} from './header-mapping.js'; +import { + CONTROL_BYTE, + degradeInboundHeaders, + mapOutboundHeaders, +} from './header-mapping.js'; describe('mapOutboundHeaders', () => { test('drops framing headers the native client computes', () => { @@ -67,7 +71,47 @@ describe('mapOutboundHeaders graceful degradation (TRANSPORT-12)', () => { }); }); +describe('CONTROL_BYTE (TRANSPORT-14)', () => { + // Asserted on the character class itself, not through `degradeInboundHeaders`, because the two + // gates that reject an inbound value are redundant by construction: the regex below and + // `Headers.addInbound`'s own `hasForbiddenInboundValueByte`, whose throw the `try`/`catch` turns + // into the same drop. A value only the second one caught looked identical from outside, which is + // how `\x0A` stayed out of this class from Phase 8a to audit #67 / #82. + test('every C0 control byte except HTAB is refused, LF included', () => { + for (let code = 0x00; code <= 0x1f; code += 1) { + const value = `v${String.fromCharCode(code)}alue`; + expect([code, CONTROL_BYTE.test(value)]).toEqual([code, code !== 0x09]); + } + expect(CONTROL_BYTE.test('v\x7falue')).toBe(true); + }); + + test('LF is refused whether or not it is preceded by CR', () => { + // The obs-fold shape (`\r\n ` continuation) and a bare LF are both header injection on the + // inbound path; RFC 9110 5.5 forbids either from reaching a field value. + expect(CONTROL_BYTE.test('one\nvalue')).toBe(true); + expect(CONTROL_BYTE.test('one\r\n two')).toBe(true); + }); + + test('HTAB and obs-text are carried, not refused', () => { + // TRANSPORT-14's own SHOULD: a non-ASCII byte in a value is preserved rather than stripped, and + // HTAB is legal whitespace inside a field value (RFC 9110 5.5). + expect(CONTROL_BYTE.test('one\ttwo')).toBe(false); + expect(CONTROL_BYTE.test('café')).toBe(false); + }); +}); + describe('degradeInboundHeaders', () => { + test('drops a header whose value carries a line feed, keeps the rest', () => { + const {headers, dropped} = degradeInboundHeaders([ + ['x-injected', 'value\nx-forged: yes'], + ['x-good', 'value'], + ]); + expect(headers.get('x-injected')).toBeUndefined(); + expect(headers.get('x-forged')).toBeUndefined(); + expect(headers.get('x-good')).toBe('value'); + expect(dropped).toEqual(['x-injected']); + }); + test('drops a header whose value carries a control byte, keeps the rest', () => { const {headers, dropped} = degradeInboundHeaders([ ['x-bad', 'v\x01alue'], diff --git a/packages/transport-shared/src/header-mapping.ts b/packages/transport-shared/src/header-mapping.ts index fbf72db..d570e4c 100644 --- a/packages/transport-shared/src/header-mapping.ts +++ b/packages/transport-shared/src/header-mapping.ts @@ -2,8 +2,25 @@ // packages/transport-shared/src/header-mapping.ts import {Headers} from '@dexpace/core'; +/** + * Every byte TRANSPORT-14 refuses in an *inbound* header value: the C0 controls except HTAB, plus + * DEL. Deliberately the same character class as `@dexpace/core`'s `hasForbiddenInboundValueByte`, + * which `Headers.addInbound` applies a few lines later \u2014 obs-text (\u2265 0x80) is carried, HTAB is + * carried, everything else below 0x20 is not. + * + * `\x0A` was missing from Phase 8a until audit #67 / #82: the class read `\x0B-\x1F`, excepting LF + * alongside the intended HTAB. Nothing observable changed, because `addInbound` rejected the value + * anyway and the `try`/`catch` in {@link degradeInboundHeaders} recorded the same drop \u2014 which is + * exactly why it survived, and why the test for this constant reads the class directly rather than + * going through that function. + * + * Exported for that test only. The package barrel deliberately does not re-export it: it is one + * half of a redundant pair, not plumbing another transport should reach for. + * + * @internal + */ /* eslint-disable no-control-regex -- RFC 9110 requires testing for ASCII control characters */ -const CONTROL_BYTE = /[\x00-\x08\x0B-\x1F\x7F]/u; +export const CONTROL_BYTE = /[\x00-\x08\x0A-\x1F\x7F]/u; const NON_ASCII_OR_CONTROL = /[\x00-\x1F\x7F-\uFFFF]/u; /* eslint-enable no-control-regex -- re-enable */ From 73af7c5010426d9875483cba2bef794ba7a782a4 Mon Sep 17 00:00:00 2001 From: Mohammad Wahbeh Date: Sat, 5 Sep 2026 13:17:45 +0300 Subject: [PATCH 2/6] fix(transport): one classification table for a permanent native failure MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `@dexpace/transport-fetch` wrapped every native rejection as `TransportFailureError`, which `retry/classify.ts` reports retryable for being an `IoError`. `Request` accepts any absolute URL, so `ftp://example.com` reached `fetch`, was refused permanently, and spent the caller's whole retry budget re-proving it. `@dexpace/transport-undici` refused the same condition through `TERMINAL_ARGUMENT_CODES`: the two adapters classified one condition oppositely. The decision moves to `@dexpace/transport-shared`'s new `dispatch-classification.ts` — the precedent is `abort-mapping.ts` — as `isPermanentDispatchFailure` plus `toDispatchFailure`, and both adapters call it. It is an allow-list of three positive recognitions, so an unrecognised rejection stays the retryable `TransportFailureError` TRANSPORT-20 makes a MUST: - a terminal argument code on the error or its immediate cause, which is undici's whole dispatcher leg (`UND_ERR_INVALID_ARG`, `UND_ERR_NOT_SUPPORTED`) and Bun's `fetch` (`ERR_INVALID_ARG_VALUE` and friends); - a `TypeError` with no `cause`, which is how undici's `fetch` — Node's global `fetch` — reports argument validation, network failures always carrying one; - a cause naming one of three WHATWG scheme refusals, which is the only way that same `fetch` can report `ftp://` at all. `bad port` is deliberately excluded: port 1 is on WHATWG's blocked list, so TRANSPORT-20's own dead-port probe arrives with that reason and must stay retryable. A `DexpaceError` is passed through unchanged — it was classified at its source. Rows added, red against the unfixed fetch transport and already green against undici: `an unsupported URL scheme fails outside the IoError tree` in the shared suite (asserting `isIoError(e) === false`, which is exactly what the retry engine asks), the Node-runtime twin in `tests/node-conformance/transport.test.mjs` because the two runtimes use entirely different error shapes for it, five unit rows in `transport-fetch`, and twelve in `transport-shared`. Found by: audit #67 / #82. --- docs/sdk-documentation/write-a-transport.md | 44 ++++-- .../transport-conformance/src/run-suite.ts | 37 +++++ .../src/fetch-transport.test.ts | 64 ++++++++- .../transport-fetch/src/fetch-transport.ts | 11 +- .../etc/transport-shared.api.md | 10 ++ .../src/dispatch-classification.test.ts | 128 +++++++++++++++++ .../src/dispatch-classification.ts | 135 ++++++++++++++++++ packages/transport-shared/src/index.ts | 4 + .../transport-undici/src/undici-transport.ts | 51 ++----- tests/node-conformance/transport.test.mjs | 36 ++++- 10 files changed, 463 insertions(+), 57 deletions(-) create mode 100644 packages/transport-shared/src/dispatch-classification.test.ts create mode 100644 packages/transport-shared/src/dispatch-classification.ts diff --git a/docs/sdk-documentation/write-a-transport.md b/docs/sdk-documentation/write-a-transport.md index 39f88c5..076d649 100644 --- a/docs/sdk-documentation/write-a-transport.md +++ b/docs/sdk-documentation/write-a-transport.md @@ -44,7 +44,7 @@ export function echoTransport(): Transport { Note `setInbound`, not `set`: values a server sent are accepted leniently. Using the strict setter on a real server's headers means a response with an obs-text byte in it becomes unreadable. -## Eleven rules a real transport must follow +## Twelve rules a real transport must follow The full contract is `docs/product-spec/17-transport-adapter-conformance-contract.md`, thirty `TRANSPORT-N` clauses. These are the ones that are easy to get wrong. @@ -73,24 +73,43 @@ does not look like a bug in your transport — it looks like a retryable network the caller's whole retry budget re-proving a permanent misconfiguration. The shared suite has a row per name. -**4. Map aborts to exactly two errors** (`TRANSPORT-3`/`TRANSPORT-4`/`TRANSPORT-8`). A timeout is the +**4. Classify a native rejection with the shared table, not by hand** (`TRANSPORT-20`, `RETRY-2`). +There are two kinds, and they are not the same kind of thing. An *exchange* that failed — connection +refused, DNS, TLS, peer reset, read timeout — is the retryable `TransportFailureError`, which +`TRANSPORT-20` makes a MUST. A *request* the client refused before dispatching — an unsupported +scheme, a forbidden method, an argument its own validation rejects — can never succeed on a retry, +and `retry/classify.ts` is an allow-list over `IoError`, so reporting it as anything outside that +tree makes it non-retryable for free. Both shipped transports report it as a bare `TypeError` with +the native error as `cause`, matching the `TypeError` they already raise for a misconfiguration +caught at construction. + +Telling the two apart is runtime-specific enough that you should not: call +`toDispatchFailure(error, fallbackMessage)` from `@dexpace/transport-shared`. Node's global `fetch` +reports an unsupported scheme as `TypeError: fetch failed` with an `unknown scheme` *cause* — the +same top-level shape as a DNS failure — while Bun 1.3.14 reports it as +`TypeError [ERR_INVALID_ARG_VALUE]` with no cause, and undici's dispatcher as +`UND_ERR_INVALID_ARG`. The two shipped transports disagreed about `ftp://` until audit #67 / #82 for +exactly that reason. The default is retryable, so a shape the table does not recognize keeps +`TRANSPORT-20`'s MUST. + +**5. Map aborts to exactly two errors** (`TRANSPORT-3`/`TRANSPORT-4`/`TRANSPORT-8`). A timeout is the retryable `TransportFailureError`; a caller abort is the terminal `CancellationError`. A raw `DOMException` must never surface. `isTimeoutSignal(signal)` is how you tell them apart. -**5. An abort after delivery must not close the delivered body** (`SEAM-16`). Both native clients tie +**6. An abort after delivery must not close the delivered body** (`SEAM-16`). Both native clients tie a response body's lifetime to the signal they were given, so dispatch over a **fork** of the signal and detach it at delivery. Get this wrong and a caller who aborts a moment after `send()` resolves finds the body they already own torn out from under them. -**6. The caller owns the response body** (`BODY-15`). Return it live and unread. Do not buffer it, do +**7. The caller owns the response body** (`BODY-15`). Return it live and unread. Do not buffer it, do not close it. -**7. Ownership decides who closes what** (`SEAM-14`). A dispatcher or client the caller supplied is +**8. Ownership decides who closes what** (`SEAM-14`). A dispatcher or client the caller supplied is never touched by your `close()`. One you constructed is yours to close. Make that decision once, at construction, and make supplying both a caller-owned client *and* an option that would build one a construction-time `TypeError` rather than a silent win for one of them. -**8. `close()` must be idempotent, concurrent-safe, and non-blocking** (`TRANSPORT-15`/`TRANSPORT-16`). +**9. `close()` must be idempotent, concurrent-safe, and non-blocking** (`TRANSPORT-15`/`TRANSPORT-16`). No unbounded await — a graceful drain would stall teardown for as long as one in-flight send against a slow peer takes. Destroying is the sanctioned choice; in-flight sends then reject with `CancellationError`, and so does a `send()` issued after `close()`, because it cannot succeed over a @@ -98,7 +117,7 @@ dispatcher that no longer exists and so is not a retryable failure. Declare your (`SEAM-15`) either way: `@dexpace/transport-fetch`'s `close()` is a documented no-op over a runtime global it does not own, and `send()` keeps working after it. -**9. Recognize a file body structurally, and still write it through `writeTo`** +**10. Recognize a file body structurally, and still write it through `writeTo`** (`TRANSPORT-28`, `BODY-13`). `body.kind === 'file'` widens the body to `FileBodyDescriptor` — `path`, `start`, `count`. Never `instanceof` against `@dexpace/body-file`: a transport must not depend on it. @@ -112,14 +131,14 @@ path, treat a file body as an ordinary `Body` and let `writeTo` produce the byte zero-copy clause is a SHOULD, and its MUSTs — replayable, and exactly the declared range on the wire — are the descriptor's to keep, not yours. -**10. Refuse a proxy you cannot honour, at construction** (`TRANSPORT-30`). `ProxyType` admits +**11. Refuse a proxy you cannot honour, at construction** (`TRANSPORT-30`). `ProxyType` admits `socks4` and `socks5`, and core resolves both from `ALL_PROXY`, so a configuration can hand you a proxy your client cannot build. Reject it in the factory with a typed error that names the type, before you allocate anything — not on the first send, where it arrives as whatever the native client raises. Keep it outside the `IoError` tree: `retry/classify.ts` is an allow-list, so a misconfiguration no retry can fix is then non-retryable for free. Declare it in `@throws`. -**11. Send a real `User-Agent`** (`NFR-15`), never a placeholder. `getBuildInfo()` supplies the tokens. +**12. Send a real `User-Agent`** (`NFR-15`), never a placeholder. `getBuildInfo()` supplies the tokens. ## Prove it @@ -152,15 +171,16 @@ The package is `private` and its `exports` name `./src/index.ts`, so it resolves `@dexpace/transport-shared` exists so the algorithm both adapters need exists once. Its exports are `@internal` and it is not a package to install directly, but reading it is the fastest way to see -what a correct implementation of rules 2, 3, 4, 5 and 7 looks like: +what a correct implementation of rules 2, 3, 4, 5, 6 and 8 looks like: | Module | Concern | |---|---| | `header-mapping.ts` | Rules 2 and 3: the outbound drop-and-degrade pass, and the lenient inbound copy | | `drop-log.ts` | Bounded, case-insensitive, drain-to-cap dedup of already-logged drop names | -| `abort-mapping.ts` | The single mapping from an aborted signal to `TransportFailureError` or `CancellationError` | +| `dispatch-classification.ts` | Rule 4: the one table deciding permanent-versus-retryable for a native rejection | +| `abort-mapping.ts` | Rule 5's single mapping from an aborted signal to `TransportFailureError` or `CancellationError` | | `body-pump.ts` | Turning a `Body` into a request stream the transport owns, plus idempotent teardown for an abandoned producer | -| `signal-fork.ts` | Rule 5's fork-and-detach | +| `signal-fork.ts` | Rule 6's fork-and-detach | ## Package it diff --git a/packages/transport-conformance/src/run-suite.ts b/packages/transport-conformance/src/run-suite.ts index 50d66bc..9e050ce 100644 --- a/packages/transport-conformance/src/run-suite.ts +++ b/packages/transport-conformance/src/run-suite.ts @@ -2,6 +2,7 @@ // packages/transport-conformance/src/run-suite.ts // The single TRANSPORT-N conformance suite, run once per transport package so the two adapters cannot // drift. Exercises: TRANSPORT-1..9, TRANSPORT-11..17, TRANSPORT-20..21, TRANSPORT-23..29, BODY-13, +// RETRY-2 (a permanent misconfiguration is outside the retryable IoError tree), // SEAM-12, SEAM-16, SEAM-30, NFR-15, and AUTH-12/AUTH-25 to the extent a transport is // answerable for them (the repeated-challenge-header row). TRANSPORT-10..13's SHARED half -- the one // outbound header pass both adapters call -- is asserted at its source in @@ -350,6 +351,41 @@ function registerProducerRows(ctx: SuiteContext): void { }); } +/** + * A scheme every runtime under test refuses and `@dexpace/core` accepts. + * + * `Request` validates a URL by handing it to WHATWG `URL`, which parses `ftp:` perfectly well and + * gives it a real origin — so an `ftp://` request reaches the native client, which refuses it. + * `foo://` would be refused too, but its `origin` is the string `"null"`, which changes what undici + * is even asked; `ftp:` keeps the two adapters answering the same question. + */ +const UNSUPPORTED_SCHEME_URL = 'ftp://example.com/anything'; + +function registerPermanentFailureRows(ctx: SuiteContext): void { + describe('TRANSPORT-20, RETRY-2: a permanent misconfiguration is not a retryable failure', () => { + test('an unsupported URL scheme fails outside the IoError tree', async () => { + // The two adapters answered this oppositely until audit #67 / #82. undici's dispatcher + // rejects `ftp://` with `UND_ERR_INVALID_ARG`, which `transport-undici` already mapped to a + // TypeError; `fetch` rejects with a TypeError whose shape depends on the runtime -- Node's + // undici-backed one says `fetch failed` with an `unknown scheme` cause, Bun 1.3.14 says + // `protocol must be http:, https: or s3:` with `code: ERR_INVALID_ARG_VALUE` -- and + // `transport-fetch` classified all of it as the RETRYABLE TransportFailureError. + // + // `classify.ts` returns true for every IoError, so that verdict spends the caller's entire + // retry budget re-proving a URL no retry can fix. `isIoError(e) === false` is the assertion + // because it is exactly what the retry engine asks. + await withTransport(ctx.makeTransport, async transport => { + const request = Request.newBuilder() + .url(UNSUPPORTED_SCHEME_URL) + .build(); + const error = await rejection(transport.send(request)); + expect(isIoError(error)).toBe(false); + expect(error).toBeInstanceOf(TypeError); + }); + }); + }); +} + function registerFailureRows(ctx: SuiteContext): void { describe('TRANSPORT-4/5/6/20: failure classification and per-call timeouts', () => { test('a dead port surfaces the retryable TransportFailureError', async () => { @@ -939,6 +975,7 @@ export function runTransportConformanceSuite( registerBodyRows(ctx); registerProducerRows(ctx); registerFailureRows(ctx); + registerPermanentFailureRows(ctx); registerCancellationRows(ctx); registerLifecycleRows(ctx); registerHeaderRows(ctx); diff --git a/packages/transport-fetch/src/fetch-transport.test.ts b/packages/transport-fetch/src/fetch-transport.test.ts index 8eea329..3cb39fb 100644 --- a/packages/transport-fetch/src/fetch-transport.test.ts +++ b/packages/transport-fetch/src/fetch-transport.test.ts @@ -5,15 +5,19 @@ // its close owns nothing to release), // TRANSPORT-2 (no retrying/redirecting dispatcher is ever composed), TRANSPORT-15/16 // (close is a documented no-op), TRANSPORT-17/19 (single-use body written once, abandoned producer -// unblocked), TRANSPORT-22 (an adaptation throw still closes the native response), TRANSPORT-30 +// unblocked), TRANSPORT-22 (an adaptation throw still closes the native response), +// TRANSPORT-20 with RETRY-2 (a permanent misconfiguration is classified outside the IoError tree, +// a failed exchange inside it), TRANSPORT-30 // (no proxy option exists at all), SEAM-30 (no producer is left running for its rejection to reach // Node's default unhandledRejection policy) import {describe, expect, test} from 'bun:test'; import { byteArrayBody, Headers, + isIoError, Request, streamBody, + TransportFailureError, type Body, } from '@dexpace/core'; import {fetchTransport} from './fetch-transport.js'; @@ -237,6 +241,64 @@ describe('fetchTransport request-body failures', () => { }); }); +describe('fetchTransport failure classification (TRANSPORT-20, RETRY-2)', () => { + /** Every shape a runtime's `fetch` uses to say "these arguments can never work". */ + const permanent: readonly (readonly [string, Error])[] = [ + // Node's undici-backed `fetch`, thrown out of the `Request` constructor: no cause, because no + // dispatch was ever attempted. + [ + 'a forbidden method', + new TypeError("'CONNECT' HTTP method is unsupported."), + ], + [ + 'a non-token method', + new TypeError("'BAD METHOD' is not a valid HTTP method."), + ], + // The same runtime's scheme refusal, which it can only report as a network error. + [ + 'an unsupported scheme', + new TypeError('fetch failed', {cause: new Error('unknown scheme')}), + ], + // Bun 1.3.14's shape for the same scheme refusal: a code, and no cause at all. + [ + "Bun's coded scheme refusal", + Object.assign(new TypeError('protocol must be http:, https: or s3:'), { + code: 'ERR_INVALID_ARG_VALUE', + }), + ], + ]; + + for (const [what, cause] of permanent) { + test(`${what} is terminal, outside the IoError tree`, async () => { + // A permanent misconfiguration classified as TransportFailureError is an IoError, and + // `classify.ts` returns true for every IoError -- so the caller's whole retry budget goes on + // re-proving it. The undici twin has refused its own equivalents since Phase 8a; this + // transport refused none of them until audit #67 / #82. + const transport = fetchTransport({fetch: () => Promise.reject(cause)}); + const request = Request.newBuilder().url('http://127.0.0.1:1/x').build(); + const error = await rejection(transport.send(request)); + expect(error).toBeInstanceOf(TypeError); + expect(isIoError(error)).toBe(false); + expect((error as Error).cause).toBe(cause); + }); + } + + test('a network failure reported the same way stays retryable', async () => { + // The twin of the rows above: `fetch failed` is also how every genuine connect/DNS/TLS failure + // arrives, so the cause is the only discriminator and narrowing must not swallow this. + const cause = new TypeError('fetch failed', { + cause: Object.assign(new Error('getaddrinfo ENOTFOUND h.invalid'), { + code: 'ENOTFOUND', + }), + }); + const transport = fetchTransport({fetch: () => Promise.reject(cause)}); + const request = Request.newBuilder().url('http://h.invalid/x').build(); + const error = await rejection(transport.send(request)); + expect(error).toBeInstanceOf(TransportFailureError); + expect(isIoError(error)).toBe(true); + }); +}); + describe('fetchTransport lifecycle', () => { test('TRANSPORT-15/16: close is a no-op and send still works afterwards (SEAM-15)', async () => { const recorder = recordingFetch(); diff --git a/packages/transport-fetch/src/fetch-transport.ts b/packages/transport-fetch/src/fetch-transport.ts index 2cae39e..56c6c03 100644 --- a/packages/transport-fetch/src/fetch-transport.ts +++ b/packages/transport-fetch/src/fetch-transport.ts @@ -21,6 +21,7 @@ import { materializeBody, producerFailure, pumpBody, + toDispatchFailure, type ForkedSignal, type HeaderDropLogging, } from '@dexpace/transport-shared'; @@ -296,10 +297,12 @@ class FetchTransport implements Transport { } catch (error) { await prepared.abandon(error); if (signal?.aborted) throw abortToSdkError(signal, error); - throw new TransportFailureError( - error instanceof Error ? error.message : 'fetch failed', - {cause: error}, - ); + // TRANSPORT-20 versus RETRY-2, decided by the table in `@dexpace/transport-shared` rather + // than here: until audit #67 / #82 every native rejection became `TransportFailureError`, + // which `classify.ts` reports retryable for being an `IoError`, so an `ftp://` URL or a + // `CONNECT` method spent the caller's whole retry budget re-proving a permanent + // misconfiguration. The undici twin already refused those; the two must not disagree. + throw toDispatchFailure(error, 'fetch failed'); } } diff --git a/packages/transport-shared/etc/transport-shared.api.md b/packages/transport-shared/etc/transport-shared.api.md index 2ee005e..de04799 100644 --- a/packages/transport-shared/etc/transport-shared.api.md +++ b/packages/transport-shared/etc/transport-shared.api.md @@ -58,6 +58,11 @@ export type HeaderDropLogging = 'all' | 'first-per-name' | 'quiet'; // @internal export function isMaterializable(body: Body_2, maxBytes: number): boolean; +// Warning: (ae-internal-missing-underscore) The name "isPermanentDispatchFailure" should be prefixed with an underscore because the declaration is marked as @internal +// +// @internal +export function isPermanentDispatchFailure(error: unknown): boolean; + // Warning: (ae-internal-missing-underscore) The name "mapOutboundHeaders" should be prefixed with an underscore because the declaration is marked as @internal // // @internal @@ -88,6 +93,11 @@ export function producerFailure(done: Promise | undefined): Promise // @internal export function pumpBody(body: Body_2): BodyPump; +// Warning: (ae-internal-missing-underscore) The name "toDispatchFailure" should be prefixed with an underscore because the declaration is marked as @internal +// +// @internal +export function toDispatchFailure(error: unknown, fallbackMessage: string): Error; + // (No @packageDocumentation comment for this package) ``` diff --git a/packages/transport-shared/src/dispatch-classification.test.ts b/packages/transport-shared/src/dispatch-classification.test.ts new file mode 100644 index 0000000..9a61ef1 --- /dev/null +++ b/packages/transport-shared/src/dispatch-classification.test.ts @@ -0,0 +1,128 @@ +// SPDX-License-Identifier: MIT +// packages/transport-shared/src/dispatch-classification.test.ts +// Exercises: TRANSPORT-20 (a failure that produced no response is the retryable transport failure), +// RETRY-2 (the retryable set is an allow-list, so a permanent misconfiguration outside the IoError +// tree is non-retryable for free), TRANSPORT-8 (an argument the native client can never accept is +// told apart from an exchange that failed) +import {describe, expect, test} from 'bun:test'; +import {isIoError, TransportFailureError} from '@dexpace/core'; +import { + isPermanentDispatchFailure, + toDispatchFailure, +} from './dispatch-classification.js'; + +/** An error carrying a native `code`, the shape undici and Bun both attach one to. */ +function coded(message: string, code: string): Error { + return Object.assign(new Error(message), {code}); +} + +describe('isPermanentDispatchFailure', () => { + test("undici's two argument-validation codes are permanent", () => { + expect( + isPermanentDispatchFailure( + coded( + 'Invalid URL protocol: the URL must start with `http:` or `https:`.', + 'UND_ERR_INVALID_ARG', + ), + ), + ).toBe(true); + expect( + isPermanentDispatchFailure(coded('expect', 'UND_ERR_NOT_SUPPORTED')), + ).toBe(true); + }); + + test("Bun's coded fetch refusal for an unsupported scheme is permanent", () => { + // Bun 1.3.14, measured: `fetch('ftp://…')` rejects with this exact shape, where Node's + // undici-backed `fetch` rejects with `fetch failed` and an `unknown scheme` cause instead. + const error = Object.assign( + new TypeError('protocol must be http:, https: or s3:'), + {code: 'ERR_INVALID_ARG_VALUE'}, + ); + expect(isPermanentDispatchFailure(error)).toBe(true); + }); + + test("a causeless TypeError is undici's argument validation, so it is permanent", () => { + // Node's global `fetch` throws these out of the `Request` constructor, before any dispatch. + for (const message of [ + "'CONNECT' HTTP method is unsupported.", + "'BAD METHOD' is not a valid HTTP method.", + 'Request with GET/HEAD method cannot have body.', + ]) { + expect(isPermanentDispatchFailure(new TypeError(message))).toBe(true); + } + }); + + test('a scheme refusal reported through `fetch failed` is permanent', () => { + const error = new TypeError('fetch failed', { + cause: new Error('unknown scheme'), + }); + expect(isPermanentDispatchFailure(error)).toBe(true); + }); + + test('a network failure reported through `fetch failed` is NOT permanent', () => { + const error = new TypeError('fetch failed', { + cause: coded('getaddrinfo ENOTFOUND example.invalid', 'ENOTFOUND'), + }); + expect(isPermanentDispatchFailure(error)).toBe(false); + }); + + test('a blocked port stays retryable (TRANSPORT-20 probes one by name)', () => { + // `http://127.0.0.1:1` is the dead-port probe §17 names for TRANSPORT-20, and port 1 is on + // WHATWG's blocked list, so Node's `fetch` refuses it before connecting and says so in the + // cause. Classifying that reason as permanent would turn the SDK's headline retryable case + // terminal, which is why the reason table excludes it explicitly. + const error = new TypeError('fetch failed', {cause: new Error('bad port')}); + expect(isPermanentDispatchFailure(error)).toBe(false); + }); + + test('a plain connection failure and a non-Error rejection stay retryable', () => { + expect( + isPermanentDispatchFailure( + coded('connect ECONNREFUSED 127.0.0.1:1', 'ECONNREFUSED'), + ), + ).toBe(false); + expect(isPermanentDispatchFailure('a string nobody typed')).toBe(false); + }); +}); + +describe('toDispatchFailure', () => { + test('a permanent misconfiguration is a TypeError outside the IoError tree', () => { + const cause = coded('invalid request method', 'UND_ERR_INVALID_ARG'); + const mapped = toDispatchFailure(cause, 'dispatch failed'); + expect(mapped).toBeInstanceOf(TypeError); + // RETRY-2's allow-list is what makes this non-retryable; the class is how it stays outside it. + expect(isIoError(mapped)).toBe(false); + expect(mapped.cause).toBe(cause); + }); + + test('an exchange failure is the retryable TransportFailureError, cause intact', () => { + const cause = coded('connect ECONNREFUSED 127.0.0.1:1', 'ECONNREFUSED'); + const mapped = toDispatchFailure(cause, 'dispatch failed'); + expect(mapped).toBeInstanceOf(TransportFailureError); + expect(isIoError(mapped)).toBe(true); + expect(mapped.message).toBe('connect ECONNREFUSED 127.0.0.1:1'); + expect(mapped.cause).toBe(cause); + }); + + test('a permanent verdict taken from the cause names the cause in its message', () => { + // `fetch failed` names nothing; the reason that made the verdict is the useful half. + const mapped = toDispatchFailure( + new TypeError('fetch failed', {cause: new Error('unknown scheme')}), + 'fetch failed', + ); + expect(mapped.message).toBe('fetch failed: unknown scheme'); + }); + + test('an error already in the SDK vocabulary is passed through untouched', () => { + // A request-body producer failure racing the dispatch arrives here already classified; the + // table knows nothing about the producer and must not re-answer for it. + const already = new TransportFailureError('producer exploded'); + expect(toDispatchFailure(already, 'fetch failed')).toBe(already); + }); + + test('a non-Error rejection falls back to the caller-supplied message', () => { + const mapped = toDispatchFailure(Symbol('nope'), 'fetch failed'); + expect(mapped).toBeInstanceOf(TransportFailureError); + expect(mapped.message).toBe('fetch failed'); + }); +}); diff --git a/packages/transport-shared/src/dispatch-classification.ts b/packages/transport-shared/src/dispatch-classification.ts new file mode 100644 index 0000000..cd838de --- /dev/null +++ b/packages/transport-shared/src/dispatch-classification.ts @@ -0,0 +1,135 @@ +// SPDX-License-Identifier: MIT +// packages/transport-shared/src/dispatch-classification.ts +import {DexpaceError, TransportFailureError} from '@dexpace/core'; + +/** + * Error codes a native client uses for "these arguments can never work", as opposed to "this + * exchange failed". + * + * `UND_ERR_INVALID_ARG` and `UND_ERR_NOT_SUPPORTED` are undici's two argument-validation codes, + * raised by `Dispatcher.request` before a socket is touched (`lib/core/errors.js` in 6.28.0): a + * non-`http(s)` origin, `CONNECT` as a method, a non-token method, a per-request + * `Proxy-Authorization` on a `ProxyAgent`. `ERR_INVALID_ARG_VALUE`, `ERR_INVALID_ARG_TYPE` and + * `ERR_INVALID_URL` are the Node-style codes Bun's `fetch` sets on the same class of refusal — Bun + * 1.3.14 rejects an `ftp://` URL with a `TypeError` carrying `ERR_INVALID_ARG_VALUE`, where Node's + * undici-backed `fetch` rejects with a causing network error instead. Measured on both, 2026-09-05. + */ +const TERMINAL_ARGUMENT_CODES: ReadonlySet = new Set([ + 'UND_ERR_INVALID_ARG', + 'UND_ERR_NOT_SUPPORTED', + 'ERR_INVALID_ARG_VALUE', + 'ERR_INVALID_ARG_TYPE', + 'ERR_INVALID_URL', +]); + +/** + * WHATWG network-error reasons that describe the *request* rather than the exchange. + * + * undici's `fetch` funnels every failure into one `TypeError('fetch failed', {cause})` + * (`lib/web/fetch/index.js:230`), so the top-level error cannot tell a refused scheme from a + * refused connection — the cause's message is the only discriminator the runtime offers. These + * three are `makeNetworkError` reasons raised before any dispatch (`:620`, `:793`, `:962` in + * 6.28.0); a scheme this SDK cannot speak is the same permanent misconfiguration undici's + * *dispatcher* reports as `UND_ERR_INVALID_ARG`. + * + * `'bad port'` is deliberately **not** here. WHATWG blocks a fixed list of ports, `1` among them, + * so on Node's `fetch` the canonical dead-port probe (`http://127.0.0.1:1`) arrives with that + * reason — and TRANSPORT-20's own conformance sentence is "connect to a dead port; assert the + * retryable type". Adding it would turn that row, and the SDK's headline retryable case, terminal. + */ +const TERMINAL_NETWORK_REASONS: ReadonlySet = new Set([ + 'unknown scheme', + 'URL scheme must be a HTTP(S) scheme', + 'about scheme is not supported', +]); + +function errorCode(error: unknown): string | undefined { + const code = (error as {code?: unknown} | null | undefined)?.code; + return typeof code === 'string' ? code : undefined; +} + +function hasTerminalCode(error: unknown): boolean { + const code = errorCode(error); + return code !== undefined && TERMINAL_ARGUMENT_CODES.has(code); +} + +/** + * Whether a native rejection is a permanent misconfiguration rather than a failed exchange. + * + * Three positive recognitions, in one place so the two shipped adapters cannot answer differently + * for the same condition (the `ftp://` row asserts they do not). Everything else falls through to + * retryable, which is both the safe default and the behaviour every adapter had before audit #67 / + * #82 — TRANSPORT-20 makes "no response was produced" a MUST-retryable, so a rejection this table + * does not recognise must stay one. + * + * 1. A **terminal argument code** on the error or its immediate cause, per the + * `TERMINAL_ARGUMENT_CODES` table above. This is the whole undici-dispatcher leg, and Bun's + * `fetch`. + * 2. A **`TypeError` with no `cause`**. undici's `fetch` — which is also Node's global `fetch` — + * builds every *network* rejection with a cause, and every argument rejection as a bare + * `TypeError` thrown out of the `Request`/`Headers` constructors before a dispatch is attempted: + * an unsupported method, a non-token method, a body on a GET. The presence of a cause is + * therefore the runtime's own line between the two, and it needs no message matching. + * 3. A cause whose message is one of the `TERMINAL_NETWORK_REASONS` above — the scheme refusals + * that undici's `fetch` can only report through its fixed `fetch failed` message. + * + * @param error - whatever the native call rejected with. + * @returns `true` when no retry of the same request could succeed. + * + * @internal + */ +export function isPermanentDispatchFailure(error: unknown): boolean { + if (hasTerminalCode(error)) return true; + if (!(error instanceof Error)) return false; + const {cause} = error; + if (error instanceof TypeError && cause === undefined) return true; + if (hasTerminalCode(cause)) return true; + return cause instanceof Error && TERMINAL_NETWORK_REASONS.has(cause.message); +} + +/** + * The message to put on the mapped error: the native message, plus the cause's when the native + * layer's own message is a fixed placeholder. `fetch failed` names nothing on its own, and the + * reason that made the verdict permanent is the only useful thing to say. + */ +function describe(error: unknown, fallbackMessage: string): string { + if (!(error instanceof Error)) return fallbackMessage; + const {cause} = error; + if (!(cause instanceof Error) || error.message.includes(cause.message)) { + return error.message; + } + return `${error.message}: ${cause.message}`; +} + +/** + * Maps one native dispatch rejection onto the SDK's error vocabulary. + * + * A permanent misconfiguration becomes a bare `TypeError` carrying the native error as `cause`, + * deliberately **outside** the `IoError` tree: `retry/classify.ts` is an allow-list that returns + * `true` for every `IoError`, so a condition no retry can fix is non-retryable for free (RETRY-2), + * and `TypeError` is already what both transports raise for a caller misconfiguration caught at + * construction. Anything else becomes the retryable `TransportFailureError` TRANSPORT-20 requires. + * + * An error that already descends from `DexpaceError` is returned unchanged: it was classified at + * its own source — a request-body producer failure racing the dispatch is the live case — and + * re-classifying it here would answer for a layer this table knows nothing about. + * + * @param error - whatever the native call rejected with. + * @param fallbackMessage - the message to use when the rejection is not an `Error` at all. + * @returns the error to throw; the caller always throws it. + * + * @internal + */ +export function toDispatchFailure( + error: unknown, + fallbackMessage: string, +): Error { + if (error instanceof DexpaceError) return error; + if (isPermanentDispatchFailure(error)) { + return new TypeError(describe(error, fallbackMessage), {cause: error}); + } + return new TransportFailureError( + error instanceof Error ? error.message : fallbackMessage, + {cause: error}, + ); +} diff --git a/packages/transport-shared/src/index.ts b/packages/transport-shared/src/index.ts index 9c88c43..71b6812 100644 --- a/packages/transport-shared/src/index.ts +++ b/packages/transport-shared/src/index.ts @@ -8,6 +8,10 @@ export { pumpBody, type BodyPump, } from './body-pump.js'; +export { + isPermanentDispatchFailure, + toDispatchFailure, +} from './dispatch-classification.js'; export {createDropLogger, type HeaderDropLogging} from './drop-log.js'; export { degradeInboundHeaders, diff --git a/packages/transport-undici/src/undici-transport.ts b/packages/transport-undici/src/undici-transport.ts index 46ec7bc..d1b3c9c 100644 --- a/packages/transport-undici/src/undici-transport.ts +++ b/packages/transport-undici/src/undici-transport.ts @@ -28,6 +28,7 @@ import { materializeBody, producerFailure, pumpBody, + toDispatchFailure, type BodyPump, type ForkedSignal, type HeaderDropLogging, @@ -276,35 +277,22 @@ const NATIVE_CANCEL_CODES: ReadonlySet = new Set([ 'UND_ERR_CLOSED', ]); -/** - * undici's codes for "these arguments can never work", as opposed to "this exchange failed". Both are - * raised by argument validation and are perfectly reproducible, so classifying them as - * `TransportFailureError` would hand `classify.ts` an always-retryable verdict (it returns `true` for - * every `IoError`) and spend a caller's whole retry budget re-proving a permanent misconfiguration. - * The commonest way to reach one is a bring-your-own `ProxyAgent` plus a per-request - * `Proxy-Authorization`: `UNDICI_PROXIED_FORBIDDEN_HEADERS` only drops that header when this - * transport constructed the proxy itself, so with a BYO dispatcher it reaches `dispatch` and is - * rejected outright. - */ -const TERMINAL_ARGUMENT_CODES: ReadonlySet = new Set([ - 'UND_ERR_INVALID_ARG', - 'UND_ERR_NOT_SUPPORTED', -]); - -function errorCode(error: unknown): string | undefined { - const code = (error as {code?: unknown} | null | undefined)?.code; - return typeof code === 'string' ? code : undefined; -} - function isNativeCancel(error: unknown): boolean { - const code = errorCode(error); - return code !== undefined && NATIVE_CANCEL_CODES.has(code); + const code = (error as {code?: unknown} | null | undefined)?.code; + return typeof code === 'string' && NATIVE_CANCEL_CODES.has(code); } /** - * Maps one dispatch failure onto the SDK's error vocabulary. Extracted from `#dispatch` so the four + * Maps one dispatch failure onto the SDK's error vocabulary. Extracted from `#dispatch` so the * branches read as one classification table rather than as control flow wrapped around a call. * + * Only the first two branches are this transport's own. The permanent-versus-retryable question the + * third asks is `@dexpace/transport-shared`'s {@link toDispatchFailure}, because the two shipped + * adapters answered it oppositely for the same condition until audit #67 / #82: `ftp://` is + * `UND_ERR_INVALID_ARG` here and a `fetch failed` with an `unknown scheme` cause over there, and + * only this transport treated it as permanent. A shared table is what keeps that from recurring — + * the same reason `abort-mapping.ts` exists. + * * @param error - whatever the dispatch rejected with. * @param signal - the forked signal the dispatch was given, if any. * @returns the error to throw; never returns normally without one. @@ -321,22 +309,7 @@ function toDispatchError( cause: error, }); } - const code = errorCode(error); - if (code !== undefined && TERMINAL_ARGUMENT_CODES.has(code)) { - // Deliberately outside the IoError tree: `classify.ts` is an allow-list, so anything that is not - // an IoError, a timeout, or a retryable status is non-retryable for free (RETRY-2). `TypeError` - // matches `selectDispatchers`, which already reports a caller misconfiguration that way. - return new TypeError( - error instanceof Error - ? error.message - : 'undici rejected the request arguments', - {cause: error}, - ); - } - return new TransportFailureError( - error instanceof Error ? error.message : 'undici dispatch failed', - {cause: error}, - ); + return toDispatchFailure(error, 'undici dispatch failed'); } /** What undici accepts as a request body; `undefined` is not one of them, `null` is. */ diff --git a/tests/node-conformance/transport.test.mjs b/tests/node-conformance/transport.test.mjs index b481f13..7360281 100644 --- a/tests/node-conformance/transport.test.mjs +++ b/tests/node-conformance/transport.test.mjs @@ -11,6 +11,8 @@ // `fileBody()` crossing a real transport has no home inside either package's own suite. // // Exercises: TRANSPORT-1 (redirects not followed), TRANSPORT-4/20 (timeout and no-response classification), +// TRANSPORT-20 with RETRY-2 (an unsupported URL scheme is a permanent misconfiguration outside the IoError +// tree -- Node and Bun report it with entirely different error shapes), // TRANSPORT-17 (a single-use body written once, its bytes on the wire), TRANSPORT-24 (vendor status codes), // TRANSPORT-11/12 (a header the native layer refuses is dropped, not a failed send -- Node's undici-backed // `fetch` rejects three names Bun's forwards), @@ -25,7 +27,7 @@ import {mkdtemp, rm, truncate, writeFile} from 'node:fs/promises'; import {tmpdir} from 'node:os'; import {join} from 'node:path'; import {createHash} from 'node:crypto'; -import {Headers, Request, RequestOptions} from '@dexpace/core'; +import {Headers, isIoError, Request, RequestOptions} from '@dexpace/core'; import {fileBody} from '@dexpace/body-file'; import {fetchTransport} from '@dexpace/transport-fetch'; import {undiciTransport} from '@dexpace/transport-undici'; @@ -245,6 +247,38 @@ describe('the transport adapters on the Node runtime', () => { } }); + it('classifies an unsupported URL scheme as permanent, not retryable (TRANSPORT-20, RETRY-2)', async () => { + // Runtime-divergent in the strongest sense: the two runtimes do not merely word this + // differently, they use different error shapes. Node's undici-backed `fetch` rejects + // `ftp://` with `TypeError: fetch failed` carrying `Error: unknown scheme` as its cause -- + // byte-identical, at the top level, to a DNS or connect failure -- while Bun 1.3.14 rejects + // with `TypeError [ERR_INVALID_ARG_VALUE]: protocol must be http:, https: or s3:` and no + // cause at all. The Bun conformance row therefore proves nothing about this runtime, which + // is the runtime the SDK ships to. undici's dispatcher agrees with itself on both + // (`UND_ERR_INVALID_ARG`) and is here for the pairing (audit #67 / #82). + const transport = makeTransport(); + try { + await assert.rejects( + transport.send( + Request.newBuilder().url('ftp://example.com/anything').build(), + ), + error => { + // `classify.ts` is an allow-list over `IoError`, so the class IS the retry verdict: + // a `TransportFailureError` here would spend the caller's whole budget re-proving a + // URL no retry can fix. + assert.ok( + error instanceof TypeError, + `expected a TypeError, got ${error?.constructor?.name}`, + ); + assert.equal(isIoError(error), false); + return true; + }, + ); + } finally { + await transport.close(); + } + }); + it('classifies a dead port as a retryable transport failure (TRANSPORT-20)', async () => { const transport = makeTransport(); try { From 55f06ac34b714a30e7abde3aaab4631486902571 Mon Sep 17 00:00:00 2001 From: Mohammad Wahbeh Date: Sat, 5 Sep 2026 13:22:29 +0300 Subject: [PATCH 3/6] fix(transport): a body-less response reports body === null on both adapters MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 204, 304, 205, 101, 103, every HEAD and a 2xx CONNECT can carry no body, and three of the four native combinations the two adapters meet disagreed about how to say so. undici's dispatcher always hands back a `BodyReadable`, so `@dexpace/transport-undici` wrapped an empty stream; Node's `fetch` returns `null` per the spec; Bun 1.3.14's `fetch` returns a live `ReadableStream` for all three (measured 2026-09-05), so `@dexpace/transport-fetch` was reporting the runtime's answer rather than the contract's. `hasNoResponseBody(method, status)` in `@dexpace/transport-shared` is now the rule and both adapters apply it, so `body === null` is a property of the SDK on every runtime. It is the WHATWG shape and the one `http/response.ts:18` already types; the rejected alternative, an empty stream on both, makes a consumer read to learn there is nothing to read. Each adapter releases the native handle it declines to expose — `cancel()` on fetch's, `dump()` on undici's. `Response.close()` is a no-op on a null body, so nobody else would, and an undrained `BodyReadable` holds the pooled connection open until the dispatcher times it out (TRANSPORT-25, SEAM-30). Rows: 204, 304 and HEAD in the shared suite, each asserting `body === null`, the `content-length` the case does or does not justify, and `reasonPhrase` as `undefined`-or-string — the fetch/undici divergence there is D7's ledger row beside §10 item 13 and is not re-ledgered. A GET over the same route is the twin, so nulling a body-less response cannot quietly null an ordinary one. All six were red on both adapters. `tests/node-conformance/transport.test.mjs` gets the runtime-divergent case, red on undici and green on fetch there, which is the asymmetry the Bun rows cannot show. `fixtures.ts`'s `route` passed the 70-line cap, so the three body-less fixtures are their own function. Found by: audit #67 / #82. --- docs/sdk-documentation/write-a-transport.md | 30 +++++-- .../transport-conformance/src/fixtures.ts | 54 ++++++++++++ .../transport-conformance/src/run-suite.ts | 83 +++++++++++++++++++ .../transport-fetch/src/fetch-transport.ts | 20 ++++- .../etc/transport-shared.api.md | 6 ++ .../transport-shared/src/body-less.test.ts | 66 +++++++++++++++ packages/transport-shared/src/body-less.ts | 49 +++++++++++ packages/transport-shared/src/index.ts | 1 + .../transport-undici/src/undici-transport.ts | 17 +++- tests/node-conformance/transport.test.mjs | 61 ++++++++++++++ 10 files changed, 377 insertions(+), 10 deletions(-) create mode 100644 packages/transport-shared/src/body-less.test.ts create mode 100644 packages/transport-shared/src/body-less.ts diff --git a/docs/sdk-documentation/write-a-transport.md b/docs/sdk-documentation/write-a-transport.md index 076d649..8c2ce2e 100644 --- a/docs/sdk-documentation/write-a-transport.md +++ b/docs/sdk-documentation/write-a-transport.md @@ -44,7 +44,7 @@ export function echoTransport(): Transport { Note `setInbound`, not `set`: values a server sent are accepted leniently. Using the strict setter on a real server's headers means a response with an obs-text byte in it becomes unreadable. -## Twelve rules a real transport must follow +## Thirteen rules a real transport must follow The full contract is `docs/product-spec/17-transport-adapter-conformance-contract.md`, thirty `TRANSPORT-N` clauses. These are the ones that are easy to get wrong. @@ -104,12 +104,27 @@ finds the body they already own torn out from under them. **7. The caller owns the response body** (`BODY-15`). Return it live and unread. Do not buffer it, do not close it. -**8. Ownership decides who closes what** (`SEAM-14`). A dispatcher or client the caller supplied is +**8. A response that can carry no body must report `body === null`** (`TRANSPORT-24`, +`TRANSPORT-25`). The WHATWG null-body statuses — `101`, `103`, `204`, `205`, `304` — plus every +`HEAD` response and a 2xx `CONNECT`. Do not forward whatever your native client produced: three of +the four combinations the two shipped adapters meet disagree. undici's dispatcher always hands back +a `BodyReadable`; Node's `fetch` returns `null`; Bun 1.3.14's `fetch` returns a live +`ReadableStream`. `hasNoResponseBody(method, status)` in `@dexpace/transport-shared` is the rule, so +that a consumer can branch on `null` instead of reading to discover there is nothing there. + +Whatever handle you then decline to expose is yours to release — `cancel()` it, `dump()` it — before +you return. `Response.close()` is a no-op on a null body, so nobody else will, and an undrained +`BodyReadable` holds a pooled connection open until the dispatcher times it out. + +A `Content-Length` on a body-less response is not a lie to correct: on a `HEAD` it describes the +body a `GET` would have returned, and it must survive verbatim. Only the body is absent. + +**9. Ownership decides who closes what** (`SEAM-14`). A dispatcher or client the caller supplied is never touched by your `close()`. One you constructed is yours to close. Make that decision once, at construction, and make supplying both a caller-owned client *and* an option that would build one a construction-time `TypeError` rather than a silent win for one of them. -**9. `close()` must be idempotent, concurrent-safe, and non-blocking** (`TRANSPORT-15`/`TRANSPORT-16`). +**10. `close()` must be idempotent, concurrent-safe, and non-blocking** (`TRANSPORT-15`/`TRANSPORT-16`). No unbounded await — a graceful drain would stall teardown for as long as one in-flight send against a slow peer takes. Destroying is the sanctioned choice; in-flight sends then reject with `CancellationError`, and so does a `send()` issued after `close()`, because it cannot succeed over a @@ -117,7 +132,7 @@ dispatcher that no longer exists and so is not a retryable failure. Declare your (`SEAM-15`) either way: `@dexpace/transport-fetch`'s `close()` is a documented no-op over a runtime global it does not own, and `send()` keeps working after it. -**10. Recognize a file body structurally, and still write it through `writeTo`** +**11. Recognize a file body structurally, and still write it through `writeTo`** (`TRANSPORT-28`, `BODY-13`). `body.kind === 'file'` widens the body to `FileBodyDescriptor` — `path`, `start`, `count`. Never `instanceof` against `@dexpace/body-file`: a transport must not depend on it. @@ -131,14 +146,14 @@ path, treat a file body as an ordinary `Body` and let `writeTo` produce the byte zero-copy clause is a SHOULD, and its MUSTs — replayable, and exactly the declared range on the wire — are the descriptor's to keep, not yours. -**11. Refuse a proxy you cannot honour, at construction** (`TRANSPORT-30`). `ProxyType` admits +**12. Refuse a proxy you cannot honour, at construction** (`TRANSPORT-30`). `ProxyType` admits `socks4` and `socks5`, and core resolves both from `ALL_PROXY`, so a configuration can hand you a proxy your client cannot build. Reject it in the factory with a typed error that names the type, before you allocate anything — not on the first send, where it arrives as whatever the native client raises. Keep it outside the `IoError` tree: `retry/classify.ts` is an allow-list, so a misconfiguration no retry can fix is then non-retryable for free. Declare it in `@throws`. -**12. Send a real `User-Agent`** (`NFR-15`), never a placeholder. `getBuildInfo()` supplies the tokens. +**13. Send a real `User-Agent`** (`NFR-15`), never a placeholder. `getBuildInfo()` supplies the tokens. ## Prove it @@ -171,13 +186,14 @@ The package is `private` and its `exports` name `./src/index.ts`, so it resolves `@dexpace/transport-shared` exists so the algorithm both adapters need exists once. Its exports are `@internal` and it is not a package to install directly, but reading it is the fastest way to see -what a correct implementation of rules 2, 3, 4, 5, 6 and 8 looks like: +what a correct implementation of rules 2, 3, 4, 5, 6, 8 and 9 looks like: | Module | Concern | |---|---| | `header-mapping.ts` | Rules 2 and 3: the outbound drop-and-degrade pass, and the lenient inbound copy | | `drop-log.ts` | Bounded, case-insensitive, drain-to-cap dedup of already-logged drop names | | `dispatch-classification.ts` | Rule 4: the one table deciding permanent-versus-retryable for a native rejection | +| `body-less.ts` | Rule 8: which method/status pairs can carry no response body at all | | `abort-mapping.ts` | Rule 5's single mapping from an aborted signal to `TransportFailureError` or `CancellationError` | | `body-pump.ts` | Turning a `Body` into a request stream the transport owns, plus idempotent teardown for an abandoned producer | | `signal-fork.ts` | Rule 6's fork-and-detach | diff --git a/packages/transport-conformance/src/fixtures.ts b/packages/transport-conformance/src/fixtures.ts index 6088a21..c4a1bf7 100644 --- a/packages/transport-conformance/src/fixtures.ts +++ b/packages/transport-conformance/src/fixtures.ts @@ -35,11 +35,65 @@ export const REPEATED_CHALLENGES: readonly string[] = [ 'Digest realm="conformance", nonce="n1", algorithm=SHA-256, qop="auth"', ]; +/** + * `/fixed-length`'s payload. Its length is what a HEAD response advertises and does not deliver, so + * it is exported: the row asserts the header survived the body-less decision rather than asserting + * a number written twice. + */ +export const FIXED_LENGTH_BODY = 'seventeen-bytes!!'; + +/** `/not-modified`'s validator, the one header a 304 exists to carry. */ +export const NOT_MODIFIED_ETAG = '"conformance-v1"'; + +/** + * The three fixtures whose responses can carry no body at all, in their own function because the + * main switch is at the 70-line lint cap -- and because they are one topic (TRANSPORT-24/25). + * `req` is not needed: `node:http` suppresses the body of a HEAD response by itself. + * + * @param pathname - the requested path. + * @param res - the response to write. + * @returns `true` when this function answered, `false` to fall through to {@link route}. + */ +function routeBodyless(pathname: string, res: ServerResponse): boolean { + switch (pathname) { + case '/no-content': + // TRANSPORT-24 with the WHATWG null-body rule: a 204 has no body and no framing to describe + // one. Node's `node:http` sends no `Content-Length` here at all; Bun 1.3.14's sends `0`. The + // row therefore asserts the header is absent-or-zero, never a positive length -- what a + // transport is answerable for is the body SHAPE, which is the same on both. + res.writeHead(204); + res.end(); + return true; + case '/not-modified': + // Deliberately WITHOUT a `Content-Length`, though RFC 9110 15.4.5 permits a 304 to carry the + // one a 200 would have had. undici 6.28.0 believes it: a 304 declaring 17 bytes leaves the + // dispatcher waiting for a body that cannot come, and the exchange dies with + // `UND_ERR_SOCKET: other side closed` (measured 2026-09-05, Node and Bun alike). That is + // undici's bug to have, not this suite's to provoke -- the row is about the ETag surviving. + res.writeHead(304, {etag: NOT_MODIFIED_ETAG}); + res.end(); + return true; + case '/fixed-length': + // The HEAD row's target. The declared length describes the body a GET would return, so the + // header promises bytes the HEAD response will not deliver: a transport that framed a stream + // from it hands the caller a read that never completes. + res.writeHead(200, { + 'content-type': 'text/plain', + 'content-length': String(FIXED_LENGTH_BODY.length), + }); + res.end(FIXED_LENGTH_BODY); + return true; + default: + return false; + } +} + function route( pathname: string, req: IncomingMessage, res: ServerResponse, ): void { + if (routeBodyless(pathname, res)) return; switch (pathname) { case '/echo-headers': res.writeHead(200, {'content-type': 'application/json'}); diff --git a/packages/transport-conformance/src/run-suite.ts b/packages/transport-conformance/src/run-suite.ts index 9e050ce..12a498e 100644 --- a/packages/transport-conformance/src/run-suite.ts +++ b/packages/transport-conformance/src/run-suite.ts @@ -34,6 +34,8 @@ import { } from '@dexpace/core'; import { fileBodyFixture, + FIXED_LENGTH_BODY, + NOT_MODIFIED_ETAG, REPEATED_CHALLENGES, startFixtureServer, type TestServer, @@ -811,6 +813,86 @@ function challengeList(values: readonly string[]): readonly string[] { return values.join(', ').split(/,\s+(?=Digest )/u); } +function registerBodylessRows(ctx: SuiteContext): void { + describe('TRANSPORT-24/25/27: a response that can carry no body reports none', () => { + // `body === null` is the WHATWG shape and the one `@dexpace/core` already types + // (`http/response.ts:18`); it is also the only shape a consumer can branch on without reading. + // Three of the four native combinations disagreed until audit #67 / #82 -- undici's dispatcher + // always hands back a `BodyReadable`, Node's `fetch` returns `null`, Bun 1.3.14's `fetch` + // returns a live `ReadableStream` -- so each adapter decides for itself now and these rows are + // what say so. The alternative, an empty stream on both, makes a consumer read to learn there + // is nothing there. + // + // `reasonPhrase` is `undefined`-or-string on purpose: `fetch` surfaces `statusText` and undici's + // `ResponseData` has no such field, a divergence recorded beside §10 item 13. Asserting the + // union is what keeps this row about the body shape. + test('a 204 carries a null body and no positive length', async () => { + await withTransport(ctx.makeTransport, async transport => { + const response = await transport.send( + Request.newBuilder().url(ctx.url('/no-content')).build(), + ); + expect(response.status.code).toBe(204); + expect(response.body).toBeNull(); + // Absent on Node's `node:http`, `'0'` on Bun 1.3.14's -- what a transport is answerable for + // is that it never invented a length for a body that does not exist. + expect([undefined, '0']).toContain( + response.headers.get('content-length'), + ); + expect(['undefined', 'string']).toContain(typeof response.reasonPhrase); + // Idempotent and non-blocking over a response the transport already released. + await response.close(); + await response.close(); + }); + }); + + test('a 304 carries a null body and still delivers its validator', async () => { + await withTransport(ctx.makeTransport, async transport => { + const response = await transport.send( + Request.newBuilder().url(ctx.url('/not-modified')).build(), + ); + expect(response.status.code).toBe(304); + expect(response.body).toBeNull(); + // A 304 exists to carry validators; dropping the body must not drop them. + expect(response.headers.get('etag')).toBe(NOT_MODIFIED_ETAG); + expect(['undefined', 'string']).toContain(typeof response.reasonPhrase); + await response.close(); + }); + }); + + test('a HEAD carries a null body and keeps the length it advertises', async () => { + await withTransport(ctx.makeTransport, async transport => { + const response = await transport.send( + Request.newBuilder() + .method('HEAD') + .url(ctx.url('/fixed-length')) + .build(), + ); + expect(response.status.code).toBe(200); + expect(response.body).toBeNull(); + // The header describes the body a GET would have returned and must survive verbatim -- + // this is the one body-less case where a length is meaningful (TRANSPORT-27). + expect(response.headers.get('content-length')).toBe( + String(FIXED_LENGTH_BODY.length), + ); + expect(['undefined', 'string']).toContain(typeof response.reasonPhrase); + await response.close(); + }); + }); + + test('the same resource over GET does carry its body', async () => { + // The twin of the three rows above: nulling a body-less response must not null an ordinary + // one, and `/fixed-length` is the same route the HEAD row just read nothing from. + await withTransport(ctx.makeTransport, async transport => { + const response = await transport.send( + Request.newBuilder().url(ctx.url('/fixed-length')).build(), + ); + expect(response.body).toBeInstanceOf(ReadableStream); + expect(await response.text()).toBe(FIXED_LENGTH_BODY); + }); + }); + }); +} + function registerInboundHeaderRows(ctx: SuiteContext): void { describe('TRANSPORT-14, AUTH-12/AUTH-25: a repeated inbound header keeps every value', () => { test('two WWW-Authenticate lines reach the pipeline as the same challenge list', async () => { @@ -981,6 +1063,7 @@ export function runTransportConformanceSuite( registerHeaderRows(ctx); registerNativeRejectionRows(ctx); registerFileBodyRows(ctx); + registerBodylessRows(ctx); registerInboundHeaderRows(ctx); registerDropSetRows(ctx); registerProxyRefusalRows(ctx); diff --git a/packages/transport-fetch/src/fetch-transport.ts b/packages/transport-fetch/src/fetch-transport.ts index 56c6c03..e934412 100644 --- a/packages/transport-fetch/src/fetch-transport.ts +++ b/packages/transport-fetch/src/fetch-transport.ts @@ -16,6 +16,7 @@ import { createDropLogger, degradeInboundHeaders, forkSignal, + hasNoResponseBody, isMaterializable, mapOutboundHeaders, materializeBody, @@ -194,7 +195,16 @@ function adaptResponse( .status(Status.of(fetchResponse.status)) .reasonPhrase(fetchResponse.statusText || undefined) .headers(headers) - .body(fetchResponse.body) + // Decided here, not inherited from the runtime. Node's `fetch` returns `null` for 204, 304 + // and HEAD as the spec requires, and Bun 1.3.14's returns a live `ReadableStream` for all + // three (measured 2026-09-05) -- so forwarding `fetchResponse.body` made the SHAPE of a + // body-less response a property of the runtime rather than of this SDK. `#exchange` releases + // whatever handle this declines (audit #67 / #82). + .body( + hasNoResponseBody(request.method, fetchResponse.status) + ? null + : fetchResponse.body, + ) .build() ); } @@ -261,7 +271,13 @@ class FetchTransport implements Transport { try { // TRANSPORT-22: a live socket is in hand, so any throw here must release it before propagating. - return adaptResponse(request, fetchResponse, this.#logDrops); + const response = adaptResponse(request, fetchResponse, this.#logDrops); + if (response.body === null && fetchResponse.body !== null) { + // A runtime handed a body for a response that cannot have one. Nothing references it any + // more, so releasing it is this transport's, not the caller's (TRANSPORT-25, SEAM-30). + await fetchResponse.body.cancel().catch(() => undefined); + } + return response; } catch (error) { await fetchResponse.body?.cancel().catch(() => undefined); // TRANSPORT-19: nothing is delivered on this path either, so the producer is owed its teardown diff --git a/packages/transport-shared/etc/transport-shared.api.md b/packages/transport-shared/etc/transport-shared.api.md index de04799..d838409 100644 --- a/packages/transport-shared/etc/transport-shared.api.md +++ b/packages/transport-shared/etc/transport-shared.api.md @@ -7,6 +7,7 @@ import type { Body as Body_2 } from '@dexpace/core'; import { DexpaceError } from '@dexpace/core'; import { Headers as Headers_2 } from '@dexpace/core'; +import type { Method } from '@dexpace/core'; // Warning: (ae-internal-missing-underscore) The name "abortToSdkError" should be prefixed with an underscore because the declaration is marked as @internal // @@ -48,6 +49,11 @@ export interface ForkedSignal { // @internal export function forkSignal(source: AbortSignal | undefined): ForkedSignal; +// Warning: (ae-internal-missing-underscore) The name "hasNoResponseBody" should be prefixed with an underscore because the declaration is marked as @internal +// +// @internal +export function hasNoResponseBody(method: Method, status: number): boolean; + // Warning: (ae-internal-missing-underscore) The name "HeaderDropLogging" should be prefixed with an underscore because the declaration is marked as @internal // // @internal diff --git a/packages/transport-shared/src/body-less.test.ts b/packages/transport-shared/src/body-less.test.ts new file mode 100644 index 0000000..a7ff678 --- /dev/null +++ b/packages/transport-shared/src/body-less.test.ts @@ -0,0 +1,66 @@ +// SPDX-License-Identifier: MIT +// packages/transport-shared/src/body-less.test.ts +// Exercises: TRANSPORT-24 (every status is surfaced faithfully, including the ones that carry no +// body), TRANSPORT-25 (a response whose body a transport declines to expose still has its native +// handle released), TRANSPORT-27 (an absent length is the unknown-length case, not a failure) +import {describe, expect, test} from 'bun:test'; +import type {Method} from '@dexpace/core'; +import {hasNoResponseBody} from './body-less.js'; + +describe('hasNoResponseBody', () => { + test('the WHATWG null-body statuses carry none, whatever the method', () => { + for (const status of [101, 103, 204, 205, 304]) { + expect([status, hasNoResponseBody('GET', status)]).toEqual([ + status, + true, + ]); + expect([status, hasNoResponseBody('POST', status)]).toEqual([ + status, + true, + ]); + } + }); + + test('an ordinary status carries one', () => { + for (const status of [200, 201, 206, 302, 400, 404, 500, 520]) { + expect([status, hasNoResponseBody('GET', status)]).toEqual([ + status, + false, + ]); + } + }); + + test('HEAD never carries one, whatever the status', () => { + // The Content-Length of a HEAD response describes the body a GET would have returned, so a + // transport that framed a stream from it would hand the caller a read that never completes. + for (const status of [200, 206, 404, 500]) { + expect([status, hasNoResponseBody('HEAD', status)]).toEqual([ + status, + true, + ]); + } + }); + + test('a 2xx CONNECT is a tunnel, a failed CONNECT is an ordinary error response', () => { + expect(hasNoResponseBody('CONNECT', 200)).toBe(true); + expect(hasNoResponseBody('CONNECT', 299)).toBe(true); + expect(hasNoResponseBody('CONNECT', 407)).toBe(false); + expect(hasNoResponseBody('CONNECT', 502)).toBe(false); + }); + + test('every other method the model admits is decided by the status alone', () => { + const methods: readonly Method[] = [ + 'GET', + 'POST', + 'PUT', + 'DELETE', + 'OPTIONS', + 'TRACE', + 'PATCH', + ]; + for (const method of methods) { + expect([method, hasNoResponseBody(method, 204)]).toEqual([method, true]); + expect([method, hasNoResponseBody(method, 200)]).toEqual([method, false]); + } + }); +}); diff --git a/packages/transport-shared/src/body-less.ts b/packages/transport-shared/src/body-less.ts new file mode 100644 index 0000000..6cbdb5a --- /dev/null +++ b/packages/transport-shared/src/body-less.ts @@ -0,0 +1,49 @@ +// SPDX-License-Identifier: MIT +// packages/transport-shared/src/body-less.ts +import type {Method} from '@dexpace/core'; + +/** + * The statuses that can never carry a body, whatever the request was: WHATWG fetch's null-body + * status set, which is RFC 9110's own list of body-less statuses (`101`, `103`, `204`, `205`, `304`). + * A `Content-Length` on one of them describes the body a `200` would have had and frames nothing. + */ +const NULL_BODY_STATUSES: ReadonlySet = new Set([ + 101, 103, 204, 205, 304, +]); + +/** The lower and upper bounds of the 2xx range, inside which a `CONNECT` response is body-less. */ +const OK_MIN = 200; +const OK_MAX = 299; + +/** + * Whether the response to `method` with `status` can carry a body at all. + * + * The WHATWG rule, and what `@dexpace/core`'s model already types: `Response.body` is + * `ReadableStream | null` (`http/response.ts:18`), and `null` is what a consumer can + * branch on without reading. Both shipped adapters apply this rather than forwarding whatever their + * native client happened to produce, because three of the four combinations disagreed until audit + * #67 / #82: + * + * - undici's dispatcher always hands back a `BodyReadable`, so `@dexpace/transport-undici` wrapped + * an empty stream for 204, 304 and HEAD alike; + * - Node's global `fetch` returns `null` for all three, per the spec; + * - Bun 1.3.14's `fetch` returns a live `ReadableStream` for all three (measured 2026-09-05), so + * `@dexpace/transport-fetch` inherited the runtime's answer rather than the contract's. + * + * A transport that decides here instead reports the same shape on every runtime, which is what a + * conformance row can assert. Whatever native handle it then declines to expose is its own to + * release — an undrained `BodyReadable` holds the pooled connection open (TRANSPORT-25, SEAM-30). + * + * @param method - the request method; always a canonical uppercase token (HTTP-9). + * @param status - the response status code as the server sent it. + * @returns `true` when the adapted response must carry `body === null`. + * + * @internal + */ +export function hasNoResponseBody(method: Method, status: number): boolean { + if (method === 'HEAD') return true; + // A 2xx CONNECT switches the connection to a tunnel; anything after the blank line is tunnelled + // bytes, not a body. A non-2xx CONNECT is an ordinary error response and may carry one. + if (method === 'CONNECT') return status >= OK_MIN && status <= OK_MAX; + return NULL_BODY_STATUSES.has(status); +} diff --git a/packages/transport-shared/src/index.ts b/packages/transport-shared/src/index.ts index 71b6812..3712363 100644 --- a/packages/transport-shared/src/index.ts +++ b/packages/transport-shared/src/index.ts @@ -1,6 +1,7 @@ // SPDX-License-Identifier: MIT // packages/transport-shared/src/index.ts export {abortToSdkError} from './abort-mapping.js'; +export {hasNoResponseBody} from './body-less.js'; export { isMaterializable, materializeBody, diff --git a/packages/transport-undici/src/undici-transport.ts b/packages/transport-undici/src/undici-transport.ts index d1b3c9c..e882dee 100644 --- a/packages/transport-undici/src/undici-transport.ts +++ b/packages/transport-undici/src/undici-transport.ts @@ -23,6 +23,7 @@ import { createDropLogger, degradeInboundHeaders, forkSignal, + hasNoResponseBody, isMaterializable, mapOutboundHeaders, materializeBody, @@ -424,7 +425,15 @@ function adaptResponse( .protocol(Protocol.HTTP_1_1) .status(Status.of(result.statusCode)) .headers(headers) - .body(toDemandDrivenStream(result.body)) + // undici's dispatcher always hands back a `BodyReadable`, even for a 204, a 304 or a HEAD -- + // so wrapping it unconditionally gave a caller an empty stream it had to read to discover + // was empty, where the fetch twin on Node gave `null`. The two adapters now decide by the + // same rule; `#exchange` dumps whatever this declines (audit #67 / #82). + .body( + hasNoResponseBody(request.method, result.statusCode) + ? null + : toDemandDrivenStream(result.body), + ) .build() ); } @@ -538,6 +547,12 @@ class UndiciTransport implements Transport { try { // TRANSPORT-22: a live socket is in hand, so any throw here must release it before propagating. const response = adaptResponse(request, result, this.#logDrops); + if (response.body === null) { + // Nothing references the `BodyReadable` any more, and an undrained one holds the pooled + // connection open until the dispatcher times it out (TRANSPORT-25, SEAM-30). `dump` reads + // and discards, which is what returns the socket to the pool. + await result.body.dump().catch(() => undefined); + } this.#reportProxyChallenge(response); return response; } catch (error) { diff --git a/tests/node-conformance/transport.test.mjs b/tests/node-conformance/transport.test.mjs index 7360281..ff3fe94 100644 --- a/tests/node-conformance/transport.test.mjs +++ b/tests/node-conformance/transport.test.mjs @@ -18,6 +18,8 @@ // `fetch` rejects three names Bun's forwards), // TRANSPORT-28/BODY-11 (a real fileBody() over the wire, whole and ranged), BODY-13 (a truncate-after-stat // short write fails the send on the streamed path, which only this runtime can assert), +// TRANSPORT-24/25 (a 204 and a HEAD carry a null body on this runtime as well -- Node's `fetch` +// returns null where Bun's returns a stream, and undici's dispatcher always returns a readable), // TRANSPORT-25 (the response body is a lazily-read stream and close releases it), TRANSPORT-29/SEAM-12 // (concurrent sends), SEAM-16 (an abort after delivery must not close the delivered body). import assert from 'node:assert/strict'; @@ -71,6 +73,9 @@ function fixtureBytes(size) { const sha = bytes => createHash('sha256').update(bytes).digest('hex'); +/** `/fixed-length`'s payload; its length is what the HEAD response advertises and never delivers. */ +const FIXED_LENGTH_BODY = 'seventeen-bytes!!'; + // Every hook and test lives inside this suite rather than at the file root, and that is // load-bearing on the declared floor. Under Node 20.3.0 -- `engines.node`, and the floor leg of // CI's node-conformance matrix -- an async ROOT-level `before` does not finish before subtests @@ -101,6 +106,21 @@ describe('the transport adapters on the Node runtime', () => { res.end('vendor status body'); return; } + if (pathname === '/no-content') { + res.writeHead(204); + res.end(); + return; + } + if (pathname === '/fixed-length') { + // `node:http` suppresses the body for a HEAD request by itself and keeps the declared + // length, which is the trap: the header promises bytes no response will deliver. + res.writeHead(200, { + 'content-type': 'text/plain', + 'content-length': String(FIXED_LENGTH_BODY.length), + }); + res.end(FIXED_LENGTH_BODY); + return; + } const chunks = []; req.on('data', chunk => chunks.push(chunk)); req.on('end', () => { @@ -215,6 +235,47 @@ describe('the transport adapters on the Node runtime', () => { } }); + it('reports a null body for a 204 and a HEAD, on this runtime too (TRANSPORT-24/25)', async () => { + // The one place the WHATWG null-body rule can be checked against the runtime the SDK ships + // to. Node's `fetch` returns `null` for 204/304/HEAD by itself, Bun 1.3.14's returns a live + // `ReadableStream` for all three, and undici's dispatcher always hands back a + // `BodyReadable` -- so the Bun conformance rows prove the adapters normalise Bun's answers + // and this proves they did not normalise into Bun's shape (audit #67 / #82). + const transport = makeTransport(); + try { + const empty = await transport.send( + Request.newBuilder().url(`${origin}/no-content`).build(), + ); + assert.equal(empty.status.code, 204); + assert.equal(empty.body, null); + await empty.close(); + + const head = await transport.send( + Request.newBuilder() + .method('HEAD') + .url(`${origin}/fixed-length`) + .build(), + ); + assert.equal(head.status.code, 200); + assert.equal(head.body, null); + // The advertised length survives; only the body a GET would have returned is absent. + assert.equal( + head.headers.get('content-length'), + String(FIXED_LENGTH_BODY.length), + ); + await head.close(); + + // A body-less decision that also nulled an ordinary response would pass every assertion + // above, so the same route is read once more over GET. + const full = await transport.send( + Request.newBuilder().url(`${origin}/fixed-length`).build(), + ); + assert.equal(await full.text(), FIXED_LENGTH_BODY); + } finally { + await transport.close(); + } + }); + it('exposes the response body as a stream that close() releases (TRANSPORT-25)', async () => { const transport = makeTransport(); try { From deb32e093b0a83474fe1bb6605a1f12a9d18712d Mon Sep 17 00:00:00 2001 From: Mohammad Wahbeh Date: Sat, 5 Sep 2026 13:28:51 +0300 Subject: [PATCH 4/6] fix(transport): a producer failure aborts the native call it raced MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When a streaming request-body producer lost the race in `#dispatch`, `send()` rejected while the native call was still pending, and nothing cancelled it. A response arriving afterwards was dropped with its body neither read nor released — TRANSPORT-9's leak, from the request side. `abandon` unwound the producer; it could not reach the fork. It could not reach the fork because for a send with no caller signal and no composed timeout there was none: `forkSignal(undefined)` returned `{signal: undefined}` and both transports dispatched with no signal at all, which is exactly the case with nothing left to cancel with. `ForkedSignal.signal` is now always a live `AbortSignal` — one controller nobody may ever abort, and indistinguishable to the native client from no signal — and the interface gains `abort(reason)`. `detach()` latches it, so the new direction cannot become the SEAM-16 violation the fork's original direction exists to prevent. Both transports read whether the *caller* aborted before pulling the fork themselves; reading it after would surface every producer failure as a `CancellationError`. `producerFailure` now classifies its own rejection as the retryable `TransportFailureError`, which is what both catches already produced for it. That is not cosmetic: the same catch now runs native rejections through a table that reads a bare `TypeError` as a permanent misconfiguration, and a producer that threw one would have been mistaken for the wire refusing the request. `prepareBody`'s buffered branch has classified the same failure at its source since Phase 8a. Instrumented rows on both transports — a `FetchLike` and a bring-your-own `Dispatcher` whose native call resolves 30ms after the producer fails — assert the dispatched signal is aborted with the producer's error and that the late response never settles into the send. Both were red. Their twins assert a delivered response leaves the fork unaborted. `signal-fork.test.ts` gains four rows for the two-way fork and the latch. The fetch `defaultTimeoutMs` row is rewritten: "a signal was handed over" no longer discriminates anything, so it asserts the deadline is honoured instead. Found by: audit #67 / #82. --- docs/sdk-documentation/write-a-transport.md | 19 +++- .../src/fetch-transport.test.ts | 87 +++++++++++++++++-- .../transport-fetch/src/fetch-transport.ts | 13 ++- .../etc/transport-shared.api.md | 5 +- packages/transport-shared/src/body-pump.ts | 27 ++++-- .../transport-shared/src/signal-fork.test.ts | 49 +++++++++-- packages/transport-shared/src/signal-fork.ts | 64 ++++++++++---- .../src/undici-transport.test.ts | 84 +++++++++++++++++- .../transport-undici/src/undici-transport.ts | 27 +++--- 9 files changed, 320 insertions(+), 55 deletions(-) diff --git a/docs/sdk-documentation/write-a-transport.md b/docs/sdk-documentation/write-a-transport.md index 8c2ce2e..f8177c1 100644 --- a/docs/sdk-documentation/write-a-transport.md +++ b/docs/sdk-documentation/write-a-transport.md @@ -96,10 +96,21 @@ exactly that reason. The default is retryable, so a shape the table does not rec retryable `TransportFailureError`; a caller abort is the terminal `CancellationError`. A raw `DOMException` must never surface. `isTimeoutSignal(signal)` is how you tell them apart. -**6. An abort after delivery must not close the delivered body** (`SEAM-16`). Both native clients tie -a response body's lifetime to the signal they were given, so dispatch over a **fork** of the signal -and detach it at delivery. Get this wrong and a caller who aborts a moment after `send()` resolves -finds the body they already own torn out from under them. +**6. Dispatch over a fork of the caller's signal — and keep the fork even when there is no signal** +(`SEAM-16`, `TRANSPORT-9`). Both native clients tie a response body's lifetime to the signal they +were given, so a caller who aborts a moment after `send()` resolves would find the body they already +own torn out from under them. Fork the signal, forward the caller's abort through it, and detach at +delivery. + +The fork runs the other way too, and that half is easy to miss. When a streaming request-body +producer fails while the native call is still pending, your `send()` rejects and nothing is left +awaiting that call: a response arriving afterwards is dropped with its body neither read nor +released, which is the leak `TRANSPORT-9` names. Abort the fork before you rethrow. That is why +`forkSignal()` hands back a live signal even when the caller supplied none and no timeout was +composed — a send with no signal at all is precisely the case where nothing could cancel it. Read +whether the *caller* aborted before you pull the fork yourself, or every producer failure surfaces +as a `CancellationError`; and let `detach()` latch the abort, so the second direction cannot become +the `SEAM-16` violation the first one exists to prevent. **7. The caller owns the response body** (`BODY-15`). Return it live and unread. Do not buffer it, do not close it. diff --git a/packages/transport-fetch/src/fetch-transport.test.ts b/packages/transport-fetch/src/fetch-transport.test.ts index 3cb39fb..6b3c036 100644 --- a/packages/transport-fetch/src/fetch-transport.test.ts +++ b/packages/transport-fetch/src/fetch-transport.test.ts @@ -7,7 +7,8 @@ // (close is a documented no-op), TRANSPORT-17/19 (single-use body written once, abandoned producer // unblocked), TRANSPORT-22 (an adaptation throw still closes the native response), // TRANSPORT-20 with RETRY-2 (a permanent misconfiguration is classified outside the IoError tree, -// a failed exchange inside it), TRANSPORT-30 +// a failed exchange inside it), TRANSPORT-9 (a producer that loses the race cancels the native call +// it raced, so no response is stranded), TRANSPORT-30 // (no proxy option exists at all), SEAM-30 (no producer is left running for its rejection to reach // Node's default unhandledRejection policy) import {describe, expect, test} from 'bun:test'; @@ -345,14 +346,86 @@ describe('fetchTransport lifecycle', () => { expect(recorder.calls.length).toBe(0); }); - test('defaultTimeoutMs applies when the call supplies no timeout of its own', async () => { - const recorder = recordingFetch(); + test('defaultTimeoutMs bounds a call that supplies no timeout of its own', async () => { + // Asserted through the outcome, not through "a signal was handed over": every send dispatches + // with a forked signal since audit #67 / #82, so the presence of one no longer discriminates. + // The double honours its signal the way a real `fetch` does, which is what makes the composed + // deadline observable. const transport = fetchTransport({ - fetch: recorder.fetch, - defaultTimeoutMs: 5_000, + fetch: (_input, init) => + new Promise((_resolve, reject) => { + init.signal?.addEventListener('abort', () => { + reject(init.signal?.reason as Error); + }); + }), + defaultTimeoutMs: 20, }); const request = Request.newBuilder().url('http://127.0.0.1:1/x').build(); - await (await transport.send(request)).close(); - expect(recorder.calls[0]?.signal).toBeInstanceOf(AbortSignal); + // TRANSPORT-4: a timeout is the retryable failure, never a cancellation, and the fork carries + // the source's `TimeoutError` reason through for `isTimeoutSignal` to read. + expect(await rejection(transport.send(request))).toMatchObject({ + name: 'TransportFailureError', + message: 'request timed out', + }); + }); +}); + +describe('fetchTransport producer-failure race (TRANSPORT-9, SEAM-30)', () => { + test('a producer that loses the race takes the pending fetch down with it', async () => { + // The send rejects the moment `writeTo` fails, while `fetch` is still pending -- and until + // audit #67 / #82 nothing then cancelled it. With no caller signal and no timeout the transport + // dispatched with NO signal at all, so a response arriving afterwards was dropped with its body + // neither read nor cancelled: a connection held for as long as the pool would keep it. + let dispatched: AbortSignal | undefined; + let settled = false; + const transport = fetchTransport({ + fetch: (_input, init) => { + dispatched = init.signal ?? undefined; + return new Promise(resolve => { + setTimeout(() => { + settled = true; + resolve(new globalThis.Response('late', {status: 200})); + }, 30); + }); + }, + }); + const failing: Body = { + kind: 'stream', + mediaType: undefined, + contentLength: -1, + replayable: false, + writeTo: () => Promise.reject(new Error('producer exploded')), + }; + const request = Request.newBuilder() + .method('POST') + .url('http://127.0.0.1:1/x') + .body(failing) + .build(); + + const error = await rejection(transport.send(request)); + // The producer's own classification, not the native table's: `writeTo` failing is a transport + // failure, and the send must not be waiting on `fetch` to say so. + expect(error).toBeInstanceOf(TransportFailureError); + expect(settled).toBe(false); + expect(dispatched?.aborted).toBe(true); + expect((dispatched?.reason as Error | undefined)?.message).toBe( + 'producer exploded', + ); + }); + + test('a delivered response is never aborted by the same handle (SEAM-16)', async () => { + // The twin: `abort` is latched by `detach`, so the fork's second direction cannot become the + // very violation its first direction exists to prevent. + let dispatched: AbortSignal | undefined; + const transport = fetchTransport({ + fetch: (_input, init) => { + dispatched = init.signal ?? undefined; + return Promise.resolve(new globalThis.Response('ok', {status: 200})); + }, + }); + const request = Request.newBuilder().url('http://127.0.0.1:1/x').build(); + const response = await transport.send(request); + expect(await response.text()).toBe('ok'); + expect(dispatched?.aborted).toBe(false); }); }); diff --git a/packages/transport-fetch/src/fetch-transport.ts b/packages/transport-fetch/src/fetch-transport.ts index e934412..f63c19b 100644 --- a/packages/transport-fetch/src/fetch-transport.ts +++ b/packages/transport-fetch/src/fetch-transport.ts @@ -301,7 +301,7 @@ class FetchTransport implements Transport { }; if (prepared.init !== undefined) init.body = prepared.init; if (prepared.duplex !== undefined) init.duplex = prepared.duplex; - if (signal !== undefined) init.signal = signal; + init.signal = signal; try { // Raced, not sequenced: a producer failure must surface even while `fetch` is still pending, @@ -311,8 +311,17 @@ class FetchTransport implements Transport { producerFailure(prepared.done), ]); } catch (error) { + // Read BEFORE the fork is pulled below, or every producer failure would look like a caller + // abort and surface as a CancellationError. + const abortedByCaller = signal.aborted; await prepared.abandon(error); - if (signal?.aborted) throw abortToSdkError(signal, error); + if (abortedByCaller) throw abortToSdkError(signal, error); + // TRANSPORT-9: when the producer lost the race, `fetch` is still pending. Nothing awaits it + // any more, so a response that arrives later would be dropped with its body neither read nor + // cancelled -- a leaked connection for as long as the pool keeps it. Pulling the fork takes + // the native call down instead. On the path where `fetch` itself rejected there is nothing + // left to cancel and this is inert (audit #67 / #82). + plan.fork.abort(error); // TRANSPORT-20 versus RETRY-2, decided by the table in `@dexpace/transport-shared` rather // than here: until audit #67 / #82 every native rejection became `TransportFailureError`, // which `classify.ts` reports retryable for being an `IoError`, so an `ftp://` URL or a diff --git a/packages/transport-shared/etc/transport-shared.api.md b/packages/transport-shared/etc/transport-shared.api.md index d838409..1db54b9 100644 --- a/packages/transport-shared/etc/transport-shared.api.md +++ b/packages/transport-shared/etc/transport-shared.api.md @@ -4,7 +4,7 @@ ```ts -import type { Body as Body_2 } from '@dexpace/core'; +import { Body as Body_2 } from '@dexpace/core'; import { DexpaceError } from '@dexpace/core'; import { Headers as Headers_2 } from '@dexpace/core'; import type { Method } from '@dexpace/core'; @@ -40,8 +40,9 @@ export function degradeInboundHeaders(raw: Iterable): // // @internal export interface ForkedSignal { + abort(reason: unknown): void; detach(): void; - readonly signal: AbortSignal | undefined; + readonly signal: AbortSignal; } // Warning: (ae-internal-missing-underscore) The name "forkSignal" should be prefixed with an underscore because the declaration is marked as @internal diff --git a/packages/transport-shared/src/body-pump.ts b/packages/transport-shared/src/body-pump.ts index a7e7f6d..f657484 100644 --- a/packages/transport-shared/src/body-pump.ts +++ b/packages/transport-shared/src/body-pump.ts @@ -1,6 +1,6 @@ // SPDX-License-Identifier: MIT // packages/transport-shared/src/body-pump.ts -import type {Body} from '@dexpace/core'; +import {TransportFailureError, type Body} from '@dexpace/core'; /** * A streaming request body in flight: the stream to hand the native client, the producer's own @@ -81,9 +81,17 @@ export function pumpBody(body: Body): BodyPump { * unhandled one. Without it that late rejection reaches Node's default `unhandledRejection` policy * and takes the process down — the exact hazard SEAM-30 names, arriving from the request side. * + * The rejection is classified **here**, as the retryable `TransportFailureError` both transports + * already reported for it, and not left raw for the caller's catch to guess at. A body that could + * not be written is a failure of this layer, and the catch that receives it is the one that also + * receives the *native* client's rejections — which since audit #67 / #82 go through a table that + * can call a bare `TypeError` a permanent misconfiguration. A producer that happened to throw one + * would have been read as the wire refusing the request. Classifying at the source is what + * `prepareBody`'s buffered branch already does with the same failure. + * * @param done - the producer settlement from {@link pumpBody}, or `undefined` when the body was not * streamed. - * @returns a promise that rejects with the producer's failure and never resolves. + * @returns a promise that rejects with the producer's failure, wrapped, and never resolves. * * @internal */ @@ -91,9 +99,18 @@ export function producerFailure( done: Promise | undefined, ): Promise { if (done === undefined) return new Promise(() => undefined); - // `then` with no rejection handler: a producer *success* says nothing about the response, so the - // derived promise only ever carries the failure onward. - return done.then(() => new Promise(() => undefined)); + return done.then( + // A producer *success* says nothing about the response, so only the failure is carried onward. + () => new Promise(() => undefined), + (cause: unknown) => { + throw new TransportFailureError( + cause instanceof Error + ? cause.message + : 'request body could not be written', + {cause}, + ); + }, + ); } /** diff --git a/packages/transport-shared/src/signal-fork.test.ts b/packages/transport-shared/src/signal-fork.test.ts index 6fa63e8..37ff026 100644 --- a/packages/transport-shared/src/signal-fork.test.ts +++ b/packages/transport-shared/src/signal-fork.test.ts @@ -1,14 +1,19 @@ // SPDX-License-Identifier: MIT // packages/transport-shared/src/signal-fork.test.ts // Exercises: SEAM-16 (an abort after delivery must not reach the native client), SEAM-13/TRANSPORT-7 -// (an abort before delivery must) +// (an abort before delivery must), TRANSPORT-9 (the transport can cancel a native call it abandons, +// with or without a caller signal) import {describe, expect, test} from 'bun:test'; import {forkSignal} from './signal-fork.js'; describe('forkSignal', () => { - test('returns no signal when the caller supplied none', () => { + test('still yields a live signal when the caller supplied none', () => { + // The transport's own cancellation handle. Until audit #67 / #82 this returned `undefined`, so + // a send with no caller signal and no timeout dispatched with none -- and a request-body + // producer that failed mid-flight had no way to take the native call down with it. const fork = forkSignal(undefined); - expect(fork.signal).toBeUndefined(); + expect(fork.signal).toBeInstanceOf(AbortSignal); + expect(fork.signal.aborted).toBe(false); expect(() => { fork.detach(); }).not.toThrow(); @@ -19,15 +24,17 @@ describe('forkSignal', () => { const fork = forkSignal(controller.signal); const reason = new Error('caller changed their mind'); controller.abort(reason); - expect(fork.signal?.aborted).toBe(true); - expect(fork.signal?.reason).toBe(reason); + expect(fork.signal.aborted).toBe(true); + // Carried verbatim because `isTimeoutSignal` reads `reason.name`: a fork that invented its own + // reason would turn every per-call timeout into a CancellationError (TRANSPORT-4). + expect(fork.signal.reason).toBe(reason); }); test('an already-aborted source forks as already aborted', () => { const controller = new AbortController(); controller.abort(new Error('too late')); const fork = forkSignal(controller.signal); - expect(fork.signal?.aborted).toBe(true); + expect(fork.signal.aborted).toBe(true); }); test('an abort after detach never reaches the fork (SEAM-16)', () => { @@ -36,6 +43,34 @@ describe('forkSignal', () => { fork.detach(); fork.detach(); // idempotent controller.abort(new Error('after delivery')); - expect(fork.signal?.aborted).toBe(false); + expect(fork.signal.aborted).toBe(false); + }); +}); + +describe('forkSignal.abort (TRANSPORT-9)', () => { + test('cancels the native call with the reason the transport gave up for', () => { + const fork = forkSignal(undefined); + const reason = new Error('producer exploded'); + fork.abort(reason); + expect(fork.signal.aborted).toBe(true); + expect(fork.signal.reason).toBe(reason); + }); + + test('cancels a fork that has a source too, without touching the source', () => { + const controller = new AbortController(); + const fork = forkSignal(controller.signal); + fork.abort(new Error('producer exploded')); + expect(fork.signal.aborted).toBe(true); + // The caller's own signal is not the transport's to abort; only the fork it dispatched with. + expect(controller.signal.aborted).toBe(false); + }); + + test('is a no-op after detach, so a delivered body is never torn out (SEAM-16)', () => { + // The latch is what keeps the second direction of the fork from becoming the very violation + // the first direction exists to prevent. + const fork = forkSignal(undefined); + fork.detach(); + fork.abort(new Error('too late to matter')); + expect(fork.signal.aborted).toBe(false); }); }); diff --git a/packages/transport-shared/src/signal-fork.ts b/packages/transport-shared/src/signal-fork.ts index 5561906..55c84d3 100644 --- a/packages/transport-shared/src/signal-fork.ts +++ b/packages/transport-shared/src/signal-fork.ts @@ -2,15 +2,39 @@ // packages/transport-shared/src/signal-fork.ts /** - * A caller signal, forwarded to the native client only for as long as the transport wants it. + * A caller signal, forwarded to the native client only for as long as the transport wants it — and + * a handle the transport can pull itself. * * @internal */ export interface ForkedSignal { - /** Hand this to the native client instead of the caller's own signal. */ - readonly signal: AbortSignal | undefined; - /** Stops forwarding. Idempotent; later aborts of the source no longer reach the native client. */ + /** + * Hand this to the native client instead of the caller's own signal. + * + * Always present, even when the caller supplied no signal and no timeout was composed. That is + * not symmetry for its own sake: {@link ForkedSignal.abort} is the only way a transport can + * cancel a native call it has decided to abandon, and a send with no caller signal is exactly + * the case where a failed request-body producer would otherwise leave one running forever + * (TRANSPORT-9, SEAM-30). A controller nobody ever aborts costs one allocation and is + * indistinguishable, to the native client, from no signal at all. + */ + readonly signal: AbortSignal; + /** + * Stops forwarding, and latches the fork: a later {@link ForkedSignal.abort} is a no-op too. + * Idempotent. Called at delivery, which is the moment the response stops being the transport's. + */ detach(): void; + /** + * Cancels the in-flight native call, so a response that arrives afterwards is refused rather + * than stranded with its body neither read nor released (TRANSPORT-9). + * + * A no-op after {@link ForkedSignal.detach}, which is what keeps this from becoming the SEAM-16 + * violation the fork exists to prevent: once a body has been handed to the caller, nothing in + * this transport may close it. + * + * @param reason - the abort reason; the failure that made the transport give up. + */ + abort(reason: unknown): void; } /** @@ -24,29 +48,37 @@ export interface ForkedSignal { * delivery keeps cancellation live for the whole in-flight window (SEAM-13, TRANSPORT-7) and inert * afterwards. * + * The fork is two-way. It carries the caller's abort *in*, and it lets the transport cancel the + * native call *out* — the second direction added by audit #67 / #82, because a request-body + * producer that fails while the native call is still pending has to take that call down with it. + * * @param source - the composed caller/timeout signal, if any. - * @returns the signal to dispatch with, plus the detach the transport calls on delivery. + * @returns the signal to dispatch with, the detach the transport calls on delivery, and the abort + * it calls when it abandons the exchange. * * @internal */ export function forkSignal(source: AbortSignal | undefined): ForkedSignal { - if (source === undefined) { - return {signal: undefined, detach: () => undefined}; - } const controller = new AbortController(); - if (source.aborted) { - controller.abort(source.reason); - return {signal: controller.signal, detach: () => undefined}; - } + let detached = false; const forward = (): void => { - controller.abort(source.reason); + controller.abort(source?.reason); }; - source.addEventListener('abort', forward, {once: true}); + if (source !== undefined) { + if (source.aborted) forward(); + else source.addEventListener('abort', forward, {once: true}); + } return { signal: controller.signal, - // removeEventListener is idempotent, so a detach on both the success and failure path is safe. detach: () => { - source.removeEventListener('abort', forward); + detached = true; + // removeEventListener is idempotent and a no-op for a listener never added, so a detach on + // both the success and failure path, with or without a source, is safe. + source?.removeEventListener('abort', forward); + }, + abort: (reason: unknown) => { + if (detached) return; + controller.abort(reason); }, }; } diff --git a/packages/transport-undici/src/undici-transport.test.ts b/packages/transport-undici/src/undici-transport.test.ts index e43390c..c8fe3b5 100644 --- a/packages/transport-undici/src/undici-transport.test.ts +++ b/packages/transport-undici/src/undici-transport.test.ts @@ -9,7 +9,8 @@ // native body), TRANSPORT-20 (a permanent argument error is terminal, a no-response failure is // retryable), TRANSPORT-28 (a file body dispatches its declared byte range), SEAM-14, // TRANSPORT-19 (a header-mapping throw leaves no started body producer stranded), SEAM-30 (so no -// producer rejection reaches Node's default unhandledRejection policy) +// producer rejection reaches Node's default unhandledRejection policy), TRANSPORT-9 (a producer that +// loses the race cancels the dispatch it raced, so no response is stranded) import {createRequire} from 'node:module'; import {mkdtemp, readFile, rm, writeFile} from 'node:fs/promises'; import {createServer, type Server} from 'node:http'; @@ -686,6 +687,87 @@ describe('undiciTransport failure classification (TRANSPORT-20)', () => { }); }); +describe('undiciTransport producer-failure race (TRANSPORT-9, SEAM-30)', () => { + /** + * A `Dispatcher` whose `request()` resolves only after `delayMs`, recording the signal it was + * handed. Nothing awaits that promise once the producer has lost the race, so the signal is the + * only thing that can still stop the exchange. + */ + function lateDispatcher( + seen: {signal?: AbortSignal | null; settled: boolean}, + delayMs: number, + ): Dispatcher { + return { + request: (options: Dispatcher.RequestOptions) => { + seen.signal = options.signal as AbortSignal | null; + return new Promise(resolve => { + setTimeout(() => { + seen.settled = true; + resolve({ + statusCode: 200, + headers: {}, + body: { + destroy: () => undefined, + dump: () => Promise.resolve(), + [Symbol.asyncIterator]: () => ({ + next: () => Promise.resolve({done: true, value: undefined}), + }), + }, + } as unknown as Dispatcher.ResponseData); + }, delayMs); + }); + }, + close: () => Promise.resolve(), + } as unknown as Dispatcher; + } + + test('a producer that loses the race takes the pending dispatch down with it', async () => { + // Until audit #67 / #82 this send dispatched with `signal: null` -- the fork only existed when + // the caller supplied a signal or a timeout was composed -- so undici kept dispatching after + // `send()` rejected and whatever came back was dropped with its `BodyReadable` neither read nor + // destroyed, holding the pooled connection open. + const seen: {signal?: AbortSignal | null; settled: boolean} = { + settled: false, + }; + const transport = undiciTransport({dispatcher: lateDispatcher(seen, 30)}); + const failing: Body = { + kind: 'stream', + mediaType: undefined, + contentLength: -1, + replayable: false, + writeTo: () => Promise.reject(new Error('producer exploded')), + }; + const request = Request.newBuilder() + .method('POST') + .url(`${origin}/upload`) + .body(failing) + .build(); + + const error = await rejection(transport.send(request)); + expect(error).toBeInstanceOf(TransportFailureError); + expect(seen.settled).toBe(false); + expect(seen.signal?.aborted).toBe(true); + expect((seen.signal?.reason as Error | undefined)?.message).toBe( + 'producer exploded', + ); + await transport.close(); + }); + + test('a delivered response is never aborted by the same handle (SEAM-16)', async () => { + const seen: {signal?: AbortSignal | null; settled: boolean} = { + settled: false, + }; + const transport = undiciTransport({dispatcher: lateDispatcher(seen, 0)}); + const request = Request.newBuilder().url(`${origin}/anything`).build(); + const response = await transport.send(request); + await response.close(); + // The fork is latched at delivery, so its abort direction can no longer reach a body the + // caller now owns. + expect(seen.signal?.aborted).toBe(false); + await transport.close(); + }); +}); + describe('undiciTransport proxy dispatch (TRANSPORT-30)', () => { test('TRANSPORT-30: a SOCKS proxy is refused at the factory, before any Agent is built', () => { // Both SOCKS values `ProxyType` admits, because core resolves both from the environment diff --git a/packages/transport-undici/src/undici-transport.ts b/packages/transport-undici/src/undici-transport.ts index e882dee..7241146 100644 --- a/packages/transport-undici/src/undici-transport.ts +++ b/packages/transport-undici/src/undici-transport.ts @@ -295,14 +295,11 @@ function isNativeCancel(error: unknown): boolean { * the same reason `abort-mapping.ts` exists. * * @param error - whatever the dispatch rejected with. - * @param signal - the forked signal the dispatch was given, if any. + * @param signal - the forked signal the dispatch was given. * @returns the error to throw; never returns normally without one. */ -function toDispatchError( - error: unknown, - signal: AbortSignal | undefined, -): Error { - if (signal?.aborted) return abortToSdkError(signal, error); +function toDispatchError(error: unknown, signal: AbortSignal): Error { + if (signal.aborted) return abortToSdkError(signal, error); if (isNativeCancel(error)) { // TRANSPORT-8: terminal, never retryable -- the dispatcher this send was routed over no longer // exists, so a retry over it cannot succeed. @@ -537,7 +534,7 @@ class UndiciTransport implements Transport { // which is exactly the in-flight window this check is about. const dispatched = context.fork.signal; - if (dispatched?.aborted) { + if (dispatched.aborted) { // TRANSPORT-9 / SEAM-30: this response will never reach a caller, so this producer closes it. await result.body.dump().catch(() => undefined); await pump?.abandon(dispatched.reason); @@ -586,9 +583,9 @@ class UndiciTransport implements Transport { method: request.method, headers: context.headers, body: context.body, - // `?? null` rather than an omitted key: `exactOptionalPropertyTypes` makes an explicit - // `undefined` a distinct, rejected value here, and undici reads `null` as "no signal". - signal: context.fork.signal ?? null, + // Always a real signal since audit #67 / #82: the fork is this transport's own + // cancellation handle, not merely a relay for the caller's. + signal: context.fork.signal, // TRANSPORT-1: pinned explicitly rather than inherited -- a BYO dispatcher may carry a // redirect interceptor, and the pipeline is the single redirect authority. maxRedirections: 0, @@ -596,8 +593,16 @@ class UndiciTransport implements Transport { producerFailure(pump?.done), ]); } catch (error) { + // Read BEFORE the fork is pulled below, or every producer failure would look like a caller + // abort and surface as a CancellationError. + const mapped = toDispatchError(error, context.fork.signal); await pump?.abandon(error); - throw toDispatchError(error, context.fork.signal); + // TRANSPORT-9: when the producer lost the race, undici is still dispatching. Nothing awaits + // it any more, so a response that arrives later would be dropped with its `BodyReadable` + // neither read nor destroyed, holding the pooled connection. Pulling the fork takes the + // dispatch down instead; on the path where undici itself rejected it is inert. + context.fork.abort(error); + throw mapped; } } From ac26cb05fb52abb1e06bb079ea4e177bb4b3d45a Mon Sep 17 00:00:00 2001 From: Mohammad Wahbeh Date: Sat, 5 Sep 2026 13:33:17 +0300 Subject: [PATCH 5/6] fix(transport): validate defaultTimeoutMs at both factories MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `defaultTimeoutMs` was unchecked on both transports and reached `AbortSignal.timeout()` untouched. Node throws `RangeError` on `1.5`, `2**32` and `-1`; Bun 1.3.14 accepts the first two. The same misconfigured transport therefore failed every send on one runtime and used a deadline nobody asked for on the other. It is also the last such path. `RequestOptionsBuilder.timeoutMs` has enforced the integer `1 .. 2**32 - 1` range at its setter since audit #67 / #76, on HTTP-35's reading that a timeout a setter accepted and a transport then refused belongs at the call site. `requireValidDefaultTimeoutMs` in `@dexpace/transport-shared` applies the identical rule with the identical wording, and both factories call it first thing — before `selectDispatchers` allocates, so a refusal cannot leak an `Agent` with no transport to close it through. A `TypeError`, matching the two construction-time refusals `undiciTransport` already raises and asserted the same way. `@throws` on both factories, and the `defaultTimeoutMs` TSDoc now states the range. `TransportCapabilities` gains a required `buildWithDefaultTimeoutMs(value)`: required rather than a flag because §17 assumes every transport has a default (TRANSPORT-5 is written against one), and typed `number` because every value the rows supply legitimately is one. Twelve rows, red on both adapters, plus the in-range twin that proves narrowing did not reject a legitimate default, plus the Node-runtime case — that one matters because Node is the runtime that used to fail late and loudly where Bun failed silently. Carried from audit #67 / #76 (D14's hand-off). Found by: audit #67 / #82. --- docs/sdk-documentation/write-a-transport.md | 27 +++++-- .../transport-conformance/src/run-suite.ts | 70 +++++++++++++++++++ .../src/fetch-transport.conformance.test.ts | 2 + .../transport-fetch/src/fetch-transport.ts | 15 +++- .../etc/transport-shared.api.md | 5 ++ .../src/default-timeout.test.ts | 51 ++++++++++++++ .../transport-shared/src/default-timeout.ts | 46 ++++++++++++ packages/transport-shared/src/index.ts | 1 + .../src/undici-transport.conformance.test.ts | 3 + .../transport-undici/src/undici-transport.ts | 17 ++++- tests/node-conformance/transport.test.mjs | 44 +++++++++++- 11 files changed, 268 insertions(+), 13 deletions(-) create mode 100644 packages/transport-shared/src/default-timeout.test.ts create mode 100644 packages/transport-shared/src/default-timeout.ts diff --git a/docs/sdk-documentation/write-a-transport.md b/docs/sdk-documentation/write-a-transport.md index f8177c1..27c2576 100644 --- a/docs/sdk-documentation/write-a-transport.md +++ b/docs/sdk-documentation/write-a-transport.md @@ -157,13 +157,21 @@ path, treat a file body as an ordinary `Body` and let `writeTo` produce the byte zero-copy clause is a SHOULD, and its MUSTs — replayable, and exactly the declared range on the wire — are the descriptor's to keep, not yours. -**12. Refuse a proxy you cannot honour, at construction** (`TRANSPORT-30`). `ProxyType` admits -`socks4` and `socks5`, and core resolves both from `ALL_PROXY`, so a configuration can hand you a -proxy your client cannot build. Reject it in the factory with a typed error that names the type, -before you allocate anything — not on the first send, where it arrives as whatever the native +**12. Refuse at construction what you cannot honour** (`TRANSPORT-30`, `HTTP-35`). `ProxyType` +admits `socks4` and `socks5`, and core resolves both from `ALL_PROXY`, so a configuration can hand +you a proxy your client cannot build. Reject it in the factory with a typed error that names the +type, before you allocate anything — not on the first send, where it arrives as whatever the native client raises. Keep it outside the `IoError` tree: `retry/classify.ts` is an allow-list, so a misconfiguration no retry can fix is then non-retryable for free. Declare it in `@throws`. +A transport-wide default timeout is the same shape of decision. It ends up in +`AbortSignal.timeout()`, whose range is an integer in `1 .. 2**32 - 1`, and nothing downstream will +check it for you: `RequestOptions.timeoutMs` is validated at its setter, so an unchecked +`defaultTimeoutMs` is the last path by which `1.5` or `2**32` reaches a deadline — where Node throws +a `RangeError` on the first send and Bun 1.3.14 quietly accepts it. Call +`requireValidDefaultTimeoutMs(value)` from `@dexpace/transport-shared` first thing in your +constructor, before anything is allocated. + **13. Send a real `User-Agent`** (`NFR-15`), never a placeholder. `getBuildInfo()` supplies the tokens. ## Prove it @@ -179,14 +187,18 @@ runTransportConformanceSuite('my-transport', () => myTransport(), { supportsInternalCancel: false, // TRANSPORT-8: a cancel path distinct from a caller abort supportsProxy: false, // TRANSPORT-30 dropsConnectionHeader: true, // TRANSPORT-11: is `Connection` in your drop set? + // HTTP-35, required: the rows hand this values `AbortSignal.timeout()` refuses and expect your + // factory to refuse them too, rather than deferring the failure to the first send. + buildWithDefaultTimeoutMs: value => myTransport({defaultTimeoutMs: value}), // TRANSPORT-30, optional: a proxy type your configuration can express and your client cannot // honour. Omit it and the row asserts `supportsProxy` is false, rather than skipping. // unsupportedProxy: {type: 'socks5', build: () => myTransport({proxy: socks5Proxy})}, }); ``` -Those capability entries are the only clauses §17 scopes to a subset of transports; everything else -runs unconditionally. The suite starts its own fixture server, and a second one on a separate origin +Those capability entries are the clauses §17 scopes to a subset of transports, plus the one builder +the suite needs to construct a deliberately misconfigured transport; everything else runs +unconditionally. The suite starts its own fixture server, and a second one on a separate origin for the rows that deliberately leave a connection unusable — a client that reuses a poisoned connection otherwise fails thirty rows downstream, which is a debugging problem of a different order. @@ -197,12 +209,13 @@ The package is `private` and its `exports` name `./src/index.ts`, so it resolves `@dexpace/transport-shared` exists so the algorithm both adapters need exists once. Its exports are `@internal` and it is not a package to install directly, but reading it is the fastest way to see -what a correct implementation of rules 2, 3, 4, 5, 6, 8 and 9 looks like: +what a correct implementation of rules 2, 3, 4, 5, 6, 8, 9 and 12 looks like: | Module | Concern | |---|---| | `header-mapping.ts` | Rules 2 and 3: the outbound drop-and-degrade pass, and the lenient inbound copy | | `drop-log.ts` | Bounded, case-insensitive, drain-to-cap dedup of already-logged drop names | +| `default-timeout.ts` | Rule 12: the range check a transport-wide default timeout has to pass | | `dispatch-classification.ts` | Rule 4: the one table deciding permanent-versus-retryable for a native rejection | | `body-less.ts` | Rule 8: which method/status pairs can carry no response body at all | | `abort-mapping.ts` | Rule 5's single mapping from an aborted signal to `TransportFailureError` or `CancellationError` | diff --git a/packages/transport-conformance/src/run-suite.ts b/packages/transport-conformance/src/run-suite.ts index 12a498e..bbe2402 100644 --- a/packages/transport-conformance/src/run-suite.ts +++ b/packages/transport-conformance/src/run-suite.ts @@ -3,6 +3,8 @@ // The single TRANSPORT-N conformance suite, run once per transport package so the two adapters cannot // drift. Exercises: TRANSPORT-1..9, TRANSPORT-11..17, TRANSPORT-20..21, TRANSPORT-23..29, BODY-13, // RETRY-2 (a permanent misconfiguration is outside the retryable IoError tree), +// HTTP-35 (a transport-wide default timeout outside AbortSignal.timeout()'s range is refused at the +// factory), // SEAM-12, SEAM-16, SEAM-30, NFR-15, and AUTH-12/AUTH-25 to the extent a transport is // answerable for them (the repeated-challenge-header row). TRANSPORT-10..13's SHARED half -- the one // outbound header pass both adapters call -- is asserted at its source in @@ -76,6 +78,19 @@ export interface TransportCapabilities { * be handed — the row then asserts that omission is the truth rather than skipping. */ readonly unsupportedProxy?: UnsupportedProxy; + /** + * HTTP-35: builds a transport whose transport-wide default timeout is `value`. + * + * Required, not a capability flag, because §17 assumes every transport has one — TRANSPORT-5 is + * written as "a per-call override … overriding the configured default for that call only". The + * rows hand it values `AbortSignal.timeout()` cannot take and expect the factory to refuse them, + * because a default nobody checked is the last path by which such a value reaches a deadline + * (`RequestOptions.timeoutMs` has been checked at its setter since audit #67 / #76). + * + * Typed `number` on purpose: `0`, `-1`, `1.5`, `2**32` and `NaN` are all legitimately `number`, + * so the row needs no cast to express what it is testing. + */ + buildWithDefaultTimeoutMs(value: number): Transport; } /** What every row below needs: a transport factory, the live fixture origin, and the capability flags. */ @@ -976,6 +991,60 @@ function registerProxyRefusalRows(ctx: SuiteContext): void { }); } +/** + * Defaults `AbortSignal.timeout()` refuses. `1.5` and `2**32` are the two Bun 1.3.14 accepts and + * Node rejects with a `RangeError`, which is what made an unvalidated default a per-runtime + * behaviour rather than a per-caller error. + */ +const UNHONOURABLE_TIMEOUTS: readonly number[] = [ + 0, + -1, + 1.5, + 2 ** 32, + Number.NaN, + Number.POSITIVE_INFINITY, +]; + +function registerDefaultTimeoutRows(ctx: SuiteContext): void { + describe('HTTP-35, TRANSPORT-5: an unhonourable default timeout is refused at construction', () => { + for (const value of UNHONOURABLE_TIMEOUTS) { + test(`a default of ${String(value)} fails the factory, not the first send`, () => { + let thrown: unknown; + try { + // A transport that returns instead of throwing has deferred the failure to the first + // send, where it arrives as a raw `RangeError` out of `AbortSignal.timeout()` on Node -- + // or, on Bun, as no failure at all and a deadline nobody asked for. + void ctx.capabilities.buildWithDefaultTimeoutMs(value); + } catch (error) { + thrown = error; + } + expect(thrown).toBeInstanceOf(TypeError); + // The same shape every other construction-time refusal in these transports has, and + // outside the IoError tree for the same reason (RETRY-2). + expect(isIoError(thrown)).toBe(false); + // "Discoverable": the message names the value that was refused. + expect((thrown as Error).message).toContain(String(value)); + }); + } + + test('a default inside the range builds a transport that still sends', async () => { + // The twin: narrowing the accepted range must not reject a legitimate default. 30s is the + // shape a caller actually configures, and the send proves the value reached `composeSignal` + // without tripping it. + await withTransport( + () => ctx.capabilities.buildWithDefaultTimeoutMs(30_000), + async transport => { + const response = await transport.send( + Request.newBuilder().url(ctx.url('/echo-headers')).build(), + ); + expect(response.status.code).toBe(200); + await response.close(); + }, + ); + }); + }); +} + function registerScopedRows(ctx: SuiteContext): void { if (ctx.capabilities.supportsInternalCancel) { describe('TRANSPORT-8: an internal cancel is told apart from a timeout', () => { @@ -1067,6 +1136,7 @@ export function runTransportConformanceSuite( registerInboundHeaderRows(ctx); registerDropSetRows(ctx); registerProxyRefusalRows(ctx); + registerDefaultTimeoutRows(ctx); registerScopedRows(ctx); }); } diff --git a/packages/transport-fetch/src/fetch-transport.conformance.test.ts b/packages/transport-fetch/src/fetch-transport.conformance.test.ts index 4ab7aa1..260693b 100644 --- a/packages/transport-fetch/src/fetch-transport.conformance.test.ts +++ b/packages/transport-fetch/src/fetch-transport.conformance.test.ts @@ -11,4 +11,6 @@ runTransportConformanceSuite('fetchTransport', () => fetchTransport(), { supportsProxy: false, // TRANSPORT-11: `Connection` is a WHATWG forbidden request header, so fetch drops it either way. dropsConnectionHeader: true, + // HTTP-35: the factory is where a default `AbortSignal.timeout()` could not take is refused. + buildWithDefaultTimeoutMs: value => fetchTransport({defaultTimeoutMs: value}), }); diff --git a/packages/transport-fetch/src/fetch-transport.ts b/packages/transport-fetch/src/fetch-transport.ts index f63c19b..784c33b 100644 --- a/packages/transport-fetch/src/fetch-transport.ts +++ b/packages/transport-fetch/src/fetch-transport.ts @@ -22,6 +22,7 @@ import { materializeBody, producerFailure, pumpBody, + requireValidDefaultTimeoutMs, toDispatchFailure, type ForkedSignal, type HeaderDropLogging, @@ -85,7 +86,13 @@ const MAX_MATERIALIZED_BODY_BYTES = 1_000_000; export interface FetchTransportOptions { /** How dropped header names are logged (TRANSPORT-13); defaults to `'first-per-name'`. */ readonly headerDropLogging?: HeaderDropLogging; - /** A timeout applied to every call that supplies no `RequestOptions.timeoutMs` of its own. */ + /** + * A timeout applied to every call that supplies no `RequestOptions.timeoutMs` of its own. + * + * An integer number of milliseconds in `1 .. 2**32 - 1`, which is `AbortSignal.timeout()`'s + * range and so the only one any transport can honour; anything else is refused by + * {@link fetchTransport} rather than by the first send (HTTP-35). + */ readonly defaultTimeoutMs?: number; /** A custom `fetch` implementation; defaults to `globalThis.fetch`. */ readonly fetch?: FetchLike; @@ -223,6 +230,9 @@ class FetchTransport implements Transport { readonly #defaultTimeoutMs: number | undefined; constructor(options: FetchTransportOptions) { + // Before anything else: a default no `AbortSignal.timeout()` can take is a caller error, and + // HTTP-35 puts it where it was supplied rather than at the first send (audit #67 / #82). + requireValidDefaultTimeoutMs(options.defaultTimeoutMs); this.#logDrops = createDropLogger( options.headerDropLogging ?? 'first-per-name', ); @@ -377,6 +387,9 @@ if (typeof Symbol.asyncDispose === 'symbol') { * * @param options - optional transport settings. * @returns a transport ready to send; release it with `close()`. + * @throws `TypeError` when `defaultTimeoutMs` is not an integer number of milliseconds in + * `1 .. 2**32 - 1` — `AbortSignal.timeout()`'s range, and therefore the only one a per-call + * deadline can be built from. * * @public */ diff --git a/packages/transport-shared/etc/transport-shared.api.md b/packages/transport-shared/etc/transport-shared.api.md index 1db54b9..4ac3d3d 100644 --- a/packages/transport-shared/etc/transport-shared.api.md +++ b/packages/transport-shared/etc/transport-shared.api.md @@ -100,6 +100,11 @@ export function producerFailure(done: Promise | undefined): Promise // @internal export function pumpBody(body: Body_2): BodyPump; +// Warning: (ae-internal-missing-underscore) The name "requireValidDefaultTimeoutMs" should be prefixed with an underscore because the declaration is marked as @internal +// +// @internal +export function requireValidDefaultTimeoutMs(value: number | undefined): void; + // Warning: (ae-internal-missing-underscore) The name "toDispatchFailure" should be prefixed with an underscore because the declaration is marked as @internal // // @internal diff --git a/packages/transport-shared/src/default-timeout.test.ts b/packages/transport-shared/src/default-timeout.test.ts new file mode 100644 index 0000000..a4637ef --- /dev/null +++ b/packages/transport-shared/src/default-timeout.test.ts @@ -0,0 +1,51 @@ +// SPDX-License-Identifier: MIT +// packages/transport-shared/src/default-timeout.test.ts +// Exercises: HTTP-35 (a timeout outside the range a transport can honour is refused where it was +// supplied, not where it is used), TRANSPORT-5 (a per-call override replaces a transport default, +// so the default is a real configuration value and answerable for its own range) +import {describe, expect, test} from 'bun:test'; +import {isIoError} from '@dexpace/core'; +import {requireValidDefaultTimeoutMs} from './default-timeout.js'; + +describe('requireValidDefaultTimeoutMs', () => { + test('accepts undefined and every integer in the honourable range', () => { + for (const value of [undefined, 1, 50, 30_000, 2 ** 32 - 1]) { + expect(() => { + requireValidDefaultTimeoutMs(value); + }).not.toThrow(); + } + }); + + test('refuses everything AbortSignal.timeout() cannot take', () => { + // The full range, not merely its lower bound. `1.5` and `2**32` are the two Bun 1.3.14 accepts + // and Node rejects with a `RangeError`, which is the divergence that made an unvalidated + // default a per-runtime behaviour rather than a per-caller error. + for (const value of [ + 0, + -1, + 1.5, + 2 ** 32, + Number.NaN, + Number.POSITIVE_INFINITY, + ]) { + let thrown: unknown; + try { + requireValidDefaultTimeoutMs(value); + } catch (error) { + thrown = error; + } + expect([value, thrown instanceof TypeError]).toEqual([value, true]); + // Outside the `IoError` tree, like every other construction-time refusal these transports + // raise: nothing retries a factory, and one class for all of them is easier to catch. + expect([value, isIoError(thrown)]).toEqual([value, false]); + // "Discoverable": the message names the value that was refused, not merely that one was. + expect((thrown as Error).message).toContain(String(value)); + } + }); + + test('the message names the range as well as the value', () => { + expect(() => { + requireValidDefaultTimeoutMs(0); + }).toThrow('1..4294967295'); + }); +}); diff --git a/packages/transport-shared/src/default-timeout.ts b/packages/transport-shared/src/default-timeout.ts new file mode 100644 index 0000000..afa6907 --- /dev/null +++ b/packages/transport-shared/src/default-timeout.ts @@ -0,0 +1,46 @@ +// SPDX-License-Identifier: MIT +// packages/transport-shared/src/default-timeout.ts + +/** + * `AbortSignal.timeout()`'s upper bound, and therefore every transport's. Duplicated from + * `@dexpace/core`'s `http/request-options.ts:12`, which is not exported: the two must agree, and + * `RequestOptionsBuilder.timeoutMs`'s own rejection message is the wording copied below so a caller + * who trips either one reads the same sentence. + */ +const MAX_TIMEOUT_MS = 2 ** 32 - 1; + +/** + * Rejects a transport-wide default timeout that no transport could honour. + * + * The range is `AbortSignal.timeout()`'s — an integer in `1 .. 2**32 - 1` — because that is the + * only range the thing this value ends up in accepts. `RequestOptionsBuilder.timeoutMs` has checked + * exactly this since audit #67 / #76, on HTTP-35's reading that a timeout a setter accepted and a + * transport then refused is a failure belonging at the call site. `defaultTimeoutMs` was left + * unchecked on both transports and so became the last path by which `1.5`, `0` or `2**32` reached + * `composeSignal` — where Node throws `RangeError` and Bun 1.3.14 accepts the first two, so the same + * misconfiguration was a failed send on one runtime and a silently different deadline on the other + * (audit #67 / #82). + * + * A `TypeError`, matching the construction-time refusals both transports already raise for a + * caller misconfiguration (`undiciTransport`'s two) and deliberately outside the `IoError` tree — + * though nothing retries a factory, the conformance row asserts the same shape for both, and a + * transport is easier to reason about when every construction-time refusal is one class. + * + * @param value - the configured default, or `undefined` for none. + * @throws `TypeError` when a defined value is zero, negative, not finite, not an integer, or + * greater than `2**32 - 1`. + * + * @internal + */ +export function requireValidDefaultTimeoutMs(value: number | undefined): void { + if ( + value === undefined || + (Number.isInteger(value) && value > 0 && value <= MAX_TIMEOUT_MS) + ) { + return; + } + throw new TypeError( + `defaultTimeoutMs must be an integer number of milliseconds in 1..${String(MAX_TIMEOUT_MS)}, ` + + `got ${String(value)}: it is handed to AbortSignal.timeout(), which accepts nothing else`, + ); +} diff --git a/packages/transport-shared/src/index.ts b/packages/transport-shared/src/index.ts index 3712363..ebfcf58 100644 --- a/packages/transport-shared/src/index.ts +++ b/packages/transport-shared/src/index.ts @@ -9,6 +9,7 @@ export { pumpBody, type BodyPump, } from './body-pump.js'; +export {requireValidDefaultTimeoutMs} from './default-timeout.js'; export { isPermanentDispatchFailure, toDispatchFailure, diff --git a/packages/transport-undici/src/undici-transport.conformance.test.ts b/packages/transport-undici/src/undici-transport.conformance.test.ts index 1f5106a..24f0ef1 100644 --- a/packages/transport-undici/src/undici-transport.conformance.test.ts +++ b/packages/transport-undici/src/undici-transport.conformance.test.ts @@ -12,6 +12,9 @@ runTransportConformanceSuite('undiciTransport', () => undiciTransport(), { dropsConnectionHeader: false, // TRANSPORT-30: core resolves `ALL_PROXY=socks5://host:1080` to this type (CFG-22), and undici's // `ProxyAgent` is an HTTP CONNECT tunnel that cannot carry it. + // HTTP-35: the factory is where a default `AbortSignal.timeout()` could not take is refused. + buildWithDefaultTimeoutMs: value => + undiciTransport({defaultTimeoutMs: value}), unsupportedProxy: { type: 'socks5', build: () => diff --git a/packages/transport-undici/src/undici-transport.ts b/packages/transport-undici/src/undici-transport.ts index 7241146..0fc67a2 100644 --- a/packages/transport-undici/src/undici-transport.ts +++ b/packages/transport-undici/src/undici-transport.ts @@ -29,6 +29,7 @@ import { materializeBody, producerFailure, pumpBody, + requireValidDefaultTimeoutMs, toDispatchFailure, type BodyPump, type ForkedSignal, @@ -111,7 +112,13 @@ export interface UndiciTransportOptions { readonly proxy?: ProxyOptions; /** How dropped header names are logged (TRANSPORT-13); defaults to `'first-per-name'`. */ readonly headerDropLogging?: HeaderDropLogging; - /** A timeout applied to every call that supplies no `RequestOptions.timeoutMs` of its own. */ + /** + * A timeout applied to every call that supplies no `RequestOptions.timeoutMs` of its own. + * + * An integer number of milliseconds in `1 .. 2**32 - 1`, which is `AbortSignal.timeout()`'s + * range and so the only one any transport can honour; anything else is refused by + * {@link undiciTransport} rather than by the first send (HTTP-35). + */ readonly defaultTimeoutMs?: number; /** `Agent` options, used only when no `dispatcher` is supplied. */ readonly agentOptions?: Agent.Options; @@ -472,6 +479,9 @@ class UndiciTransport implements Transport { #closing: Promise | undefined; constructor(options: UndiciTransportOptions) { + // Before `selectDispatchers` allocates anything: a refusal afterwards would leak the agents it + // built, with no transport for the caller to close them through (audit #67 / #82). + requireValidDefaultTimeoutMs(options.defaultTimeoutMs); this.#dispatchers = selectDispatchers(options); this.#proxy = options.proxy; this.#logDrops = createDropLogger( @@ -684,9 +694,10 @@ if (typeof Symbol.asyncDispose === 'symbol') { * * @param options - optional transport settings. * @returns a transport ready to send; release it with `close()`. - * @throws `TypeError` when both `dispatcher` and `proxy` are supplied, or when `proxy.type` is + * @throws `TypeError` when both `dispatcher` and `proxy` are supplied; when `proxy.type` is * anything but `http` — undici's `ProxyAgent` cannot carry a SOCKS proxy, and neither can - * `@dexpace/transport-fetch`, which has no proxy option at all. + * `@dexpace/transport-fetch`, which has no proxy option at all; or when `defaultTimeoutMs` is not + * an integer number of milliseconds in `1 .. 2**32 - 1`, which is `AbortSignal.timeout()`'s range. * * @public */ diff --git a/tests/node-conformance/transport.test.mjs b/tests/node-conformance/transport.test.mjs index ff3fe94..92541c0 100644 --- a/tests/node-conformance/transport.test.mjs +++ b/tests/node-conformance/transport.test.mjs @@ -20,6 +20,8 @@ // short write fails the send on the streamed path, which only this runtime can assert), // TRANSPORT-24/25 (a 204 and a HEAD carry a null body on this runtime as well -- Node's `fetch` // returns null where Bun's returns a stream, and undici's dispatcher always returns a readable), +// HTTP-35 (a defaultTimeoutMs AbortSignal.timeout() cannot take is refused at the factory -- Node +// throws RangeError for two of the values Bun accepts), // TRANSPORT-25 (the response body is a lazily-read stream and close releases it), TRANSPORT-29/SEAM-12 // (concurrent sends), SEAM-16 (an abort after delivery must not close the delivered body). import assert from 'node:assert/strict'; @@ -146,9 +148,14 @@ describe('the transport adapters on the Node runtime', () => { }); }); + // `makeTransport` takes the transport-wide default timeout so the HTTP-35 case can build a + // misconfigured transport; every other case passes nothing and gets today's shape. for (const [name, makeTransport] of [ - ['transport-fetch', () => fetchTransport()], - ['transport-undici', () => undiciTransport()], + ['transport-fetch', defaultTimeoutMs => fetchTransport({defaultTimeoutMs})], + [ + 'transport-undici', + defaultTimeoutMs => undiciTransport({defaultTimeoutMs}), + ], ]) { describe(`${name} on the Node runtime`, () => { it('returns a 302 raw and never follows it (TRANSPORT-1)', async () => { @@ -290,6 +297,39 @@ describe('the transport adapters on the Node runtime', () => { } }); + it('refuses an unhonourable defaultTimeoutMs at the factory (HTTP-35)', async () => { + // Runtime-divergent, and the reason the check exists at all: `AbortSignal.timeout(1.5)` and + // `AbortSignal.timeout(2**32)` throw `RangeError` on Node and are accepted by Bun 1.3.14, + // so before audit #67 / #82 the same misconfigured transport failed every send here and + // silently used a different deadline there. The factory now answers identically on both, + // which is what this pins on the runtime that used to be the strict one. + for (const value of [0, -1, 1.5, 2 ** 32, Number.NaN]) { + assert.throws( + () => makeTransport(value), + error => { + assert.ok( + error instanceof TypeError, + `expected a TypeError for ${value}, got ${error?.constructor?.name}`, + ); + assert.equal(isIoError(error), false); + assert.ok(error.message.includes(String(value)), error.message); + return true; + }, + ); + } + // And a legitimate default still builds something that sends. + const transport = makeTransport(30_000); + try { + const response = await transport.send( + Request.newBuilder().url(`${origin}/echo`).build(), + ); + assert.equal(response.status.code, 200); + await response.close(); + } finally { + await transport.close(); + } + }); + it('classifies a per-call timeout as retryable, not cancellation (TRANSPORT-4)', async () => { const transport = makeTransport(); try { From a889d5d34c0b99f44552ed892b24729ab9b95494 Mon Sep 17 00:00:00 2001 From: Mohammad Wahbeh Date: Sat, 5 Sep 2026 13:39:12 +0300 Subject: [PATCH 6/6] docs(transport): record the parity work, and the TRANSPORT-20 reading MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `docs/deviations.md` gains one row for the reading the classification table rests on: `TRANSPORT-20`'s "any transport failure that produced no HTTP response" is read as an exchange that failed, not as a request the native client refused to make, and such a refusal is reported outside the `IoError` tree so `retry/classify.ts`'s allow-list makes it non-retryable. The reading is not new — undici has applied it since Phase 8a — but it lived only in that phase's checklist, and until this run `@dexpace/transport-fetch` did the opposite for the identical condition. The row records that the MUST is still the default: the table is an allow-list, and `bad port` is excluded by name because TRANSPORT-20's own dead-port probe arrives with that reason on Node. `reasonPhrase` is deliberately not re-ledgered — it is already a row beside §10 item 13. Two citations this branch moved are re-anchored: the SOCKS row's `undici-transport.ts:138,151-158,192` -> `:147,160-167,201` and `fetch-transport.ts:76-79` -> `:79-82`. `run-suite.ts`'s header re-anchors the two TRANSPORT-22 test citations for the same reason, and says why TRANSPORT-9's producer race is not a shared row: only an instrumented native client can show that a pending call was cancelled. READMEs: `transport-shared`'s module table gains the three new modules and the fork's second direction; both adapters' behaviour lists gain the classification rule, the body-less contract and the `defaultTimeoutMs` range. --- docs/deviations.md | 3 ++- packages/transport-conformance/src/run-suite.ts | 8 ++++++-- packages/transport-fetch/README.md | 12 ++++++++++++ packages/transport-shared/README.md | 7 +++++-- packages/transport-undici/README.md | 13 +++++++++++++ 5 files changed, 38 insertions(+), 5 deletions(-) diff --git a/docs/deviations.md b/docs/deviations.md index ea1d822..dc59fb7 100644 --- a/docs/deviations.md +++ b/docs/deviations.md @@ -502,7 +502,8 @@ frozen tree and is amended only deliberately, by hand. When §10 is next amended | **`HTTP-35`'s timeout check is read as the FULL range `AbortSignal.timeout()` accepts, not the lower bound the requirement enumerates.** `HTTP-35` says the options builder "MUST reject a non-null timeout that is zero or negative". `RequestOptionsBuilder.timeoutMs` rejects three more classes: non-finite (shipped unledgered before this audit), non-integer, and anything above `2**32 - 1`. **Strictly stricter than the letter, and deliberately so.** The field has exactly one consumer — `composeSignal` hands it to `AbortSignal.timeout()` — so a value this setter admits and that function refuses is `HTTP-35`'s own failure mode with the seam moved: the error surfaces inside a transport, as an unwrapped platform `RangeError`, one frame away from the call that supplied it. The earlier reading accepted `1.5` and argued in TSDoc that "a timeout is a duration and a fractional millisecond is meaningful"; no consumer of the field can express one. **The range checked is Node's, and that is the point:** `AbortSignal.timeout(1.5)` and `AbortSignal.timeout(2 ** 32)` raise `RangeError` on Node and are ACCEPTED on Bun, and a negative delay is `RangeError` on Node against `TypeError` on Bun (measured 2026-09-05), so leaving the check to the runtime would make an SDK-level contract depend on which runtime the caller happens to be on. *Rejected:* rounding with `Math.ceil` and clamping inside `composeSignal`, which hides the caller's mistake in the one place `HTTP-35` exists to surface it. `composeSignal` is documented as still able to raise, because a transport's own `defaultTimeoutMs` construction option bypasses this setter and is not validated by core — recorded for #81/#82, not fixed here | audit #67 / #76 | 2026-09-05 | `docs/product-spec/04-core-http-domain-model.md:48` is `HTTP-35`'s wording. `packages/core/src/http/request-options.ts:12` (`MAX_TIMEOUT_MS`) and `:204-214` (the check and the rewritten TSDoc paragraph); `packages/core/src/seams/transport.ts:86-92` is `composeSignal`'s new `@throws`, which states the two-runtime divergence rather than naming one error class. Pinned by "rejects a fractional timeout, which no transport deadline can honor" (`packages/core/src/http/request-options.test.ts:128`, the FLIPPED case — it pinned acceptance until this audit), "rejects a timeout above AbortSignal.timeout()'s ceiling of 2**32 - 1" (`:134`), "accepts the ceiling itself" (`:143`) and the `every accepted timeout is an integer in 1..2**32 - 1` property (`:157`); the Node half is `composeSignal timeout range on Node (HTTP-35)` in `tests/node-conformance/seams.test.mjs:105`, which cannot live in `bun test` because Bun accepts both rejected values | not yet in §10 | | **`HTTP-31`'s "falls back to raw text rather than throwing" is satisfied for an unpaired surrogate by SUBSTITUTING U+FFFD, not by keeping the raw text.** `HTTP-31` (MUST) makes `QueryParams.parse` lenient and enumerates the lenient cases, ending with "malformed percent-encoding falling back to raw text rather than throwing". An unpaired surrogate is a fourth kind of malformed input the enumeration does not name, and the fallback it prescribes is not available for it: the raw text has no UTF-8 form, so keeping it produces a `QueryParams` whose `encode()` throws `URIError` — the throw merely deferred out of `parse` and into an accessor that documents no throw at all. **The port repairs instead.** `parse` runs `toWellFormed()` over each decoded name and value, so every instance it returns is encodable, which is what "parsing MUST invert encode" needs to mean. The strict half of the rule is unaffected and is where `#76` puts the rejection: `QueryParamsBuilder.add` throws `UrlConstructionError` for the same input, and `substitutePathParams` throws `OperationAssemblyError`. That asymmetry is not new to the query model — it is exactly the outbound/inbound split `Headers` already draws for `HTTP-18` against `HTTP-19`, applied to the one requirement pair that needs it here. Substitution matches the platform rather than inventing a policy: `new URL('https://x/?a=\uD800').search` is `?a=%EF%BF%BD` (measured 2026-09-05). *Rejected:* letting `parse` throw the builder's error, which breaks a MUST. *Rejected:* dropping the offending parameter, which loses a name the caller may be matching on | audit #67 / #76 | 2026-09-05 | `docs/product-spec/04-core-http-domain-model.md:42` carries `HTTP-31`'s wording (shared with `HTTP-30`). `packages/core/src/http/rfc3986.ts:17-18` are the two patterns, `:31` `hasLoneSurrogate` (strict) and `:44` `toWellFormed` (lenient) — one rule, two entry points, so no caller can pick the wrong one; `packages/core/src/http/query-params.ts:144-150` is `parse`'s repair with the `HTTP-18`/`HTTP-19` comparison stated inline, against `:44-50` and `:240-241` for the strict `add` path; `packages/core/src/seams/operation.ts:139-144` is the path-param half. `/\p{Surrogate}/u` rather than `String.prototype.isWellFormed()` because the latter is ES2024 and `tsconfig.base.json:5-11` pins `lib: ES2023`, though the `engines.node >= 20.3` runtime has it. Pinned by the `lone surrogates are rejected where they are supplied (HTTP-29, HTTP-31)` block in `packages/core/src/http/query-params.test.ts:170` — "parse() stays lenient and substitutes U+FFFD, because HTTP-31 forbids throwing" (`:197`) and the `no anything escapes parse()` property (`:233`) | not yet in §10 | | **`OBS-35`'s "MUST NOT bake in a default config key name" is satisfied by making the key configurable, not by removing the default.** `OBS-35` (SHOULD) asks for a tolerant, layered log-level resolution and adds one MUST: no baked-in default key name. The port ships `CFG_KEY_LOG_LEVEL` (`DEXPACE_LOG_LEVEL`) as `CFG-14`'s well-known key and, until 2026-09-05, read it unconditionally. It is now `LoggingStepSettings.configKey`'s default: a caller names their own key and the resolution is otherwise identical. **Why the default stays.** A required key would mean no caller gets ambient granularity without naming one first, which trades a MUST about *naming* for a worse default experience, and `CFG-14` — which this port also implements — exists precisely to standardise the name. The layered resolution itself is `CFG-1`'s (override → environment → normalised property → default) and is tolerant as the requirement asks. **A second, quieter half:** the process-wide configuration slot starts empty (`CFG-13`), so no key of any name resolves until a host calls `setGlobalConfiguration(defaultConfiguration())`. That is deliberate — defaulting the slot to a configuration that reads `process.env` would make an import-time environment read the SDK's default behaviour — and it is now documented as the required wiring rather than left to be discovered | audit #67 / #80 | 2026-09-05 | `packages/core/src/observability/logging-step.ts:64-79,94-104` (the setting and the resolution); `packages/core/src/config/configuration.ts:311,354-358,368` (`CFG_KEY_LOG_LEVEL`, `defaultConfiguration`, the empty default slot); `docs/sdk-documentation/pipelines.md` "Turning logging on from the environment"; `docs/product-spec/15-instrumentation-and-observability.md:66` (the requirement) | not yet in §10 | -| **`CFG-22`'s SOCKS proxy types are resolved by the configuration layer and supported by neither shipped transport; the refusal is at the transport factory, and `ProxyType` keeps them.** `CFG-22` (MUST) requires the proxy model to carry "the proxy protocol type (HTTP, SOCKS4, SOCKS5)", and the port implements it in full: `ProxyType` is `'http' \| 'socks4' \| 'socks5'`, and `resolveProxyOptions` maps `ALL_PROXY`/`HTTPS_PROXY`'s `socks:`, `socks4:`, `socks4a:`, `socks5:` and `socks5h:` schemes onto it. Nothing can then send over one. `@dexpace/transport-undici` builds undici's `ProxyAgent`, which is an HTTP `CONNECT` tunnel reading its `uri` as a URL, and `@dexpace/transport-fetch` ships no `proxy` option at all because Node's bare global `fetch` exposes no proxy hook outside undici internals. So a configuration that resolves cleanly has no transport that can honour it. **What changed on 2026-09-05 (audit #67 / #81).** Until then the discovery was `new ProxyAgent({uri: 'socks5://…'})` throwing undici's `InvalidArgumentError('Invalid URL protocol: socks5:')` out of a public factory — untyped, undocumented, and outside the SDK's error vocabulary. `undiciTransport()` now refuses `proxy.type !== 'http'` at construction with a `TypeError` naming the type, before any dispatcher is allocated, deliberately outside the `IoError` tree so `retry/classify.ts`'s allow-list makes it non-retryable (RETRY-2). That is `TRANSPORT-30`'s "make the limitation discoverable rather than silently misbehaving" applied at the earliest point that can. **Why `ProxyType` still admits `socks4`/`socks5`.** Narrowing a `@public` union is a breaking change and therefore a release-pass decision, which this run is not taking (the run's release machinery is suspended); and `CFG-22`'s MUST is about the *model*, which would then no longer satisfy it. The honest state is a configuration layer that is complete and a transport layer that is not, which is what this row records. A future transport — a `node:net` SOCKS dialer, or a `ProxyAgent` replacement — closes the gap without a model change. | audit #67 / #81 | 2026-09-05 | `packages/core/src/config/proxy.ts:34` (`ProxyType`), `:372-380` (the scheme map); `packages/transport-undici/src/undici-transport.ts:138,151-158,192` (the supported type, the refusal, and where it runs); `packages/transport-fetch/src/fetch-transport.ts:76-79` (no `proxy` option, and why); `docs/product-spec/16-configuration.md:42` (`CFG-22`); `docs/product-spec/17-transport-adapter-conformance-contract.md:48` (`TRANSPORT-30`) | not yet in §10 | +| **`CFG-22`'s SOCKS proxy types are resolved by the configuration layer and supported by neither shipped transport; the refusal is at the transport factory, and `ProxyType` keeps them.** `CFG-22` (MUST) requires the proxy model to carry "the proxy protocol type (HTTP, SOCKS4, SOCKS5)", and the port implements it in full: `ProxyType` is `'http' \| 'socks4' \| 'socks5'`, and `resolveProxyOptions` maps `ALL_PROXY`/`HTTPS_PROXY`'s `socks:`, `socks4:`, `socks4a:`, `socks5:` and `socks5h:` schemes onto it. Nothing can then send over one. `@dexpace/transport-undici` builds undici's `ProxyAgent`, which is an HTTP `CONNECT` tunnel reading its `uri` as a URL, and `@dexpace/transport-fetch` ships no `proxy` option at all because Node's bare global `fetch` exposes no proxy hook outside undici internals. So a configuration that resolves cleanly has no transport that can honour it. **What changed on 2026-09-05 (audit #67 / #81).** Until then the discovery was `new ProxyAgent({uri: 'socks5://…'})` throwing undici's `InvalidArgumentError('Invalid URL protocol: socks5:')` out of a public factory — untyped, undocumented, and outside the SDK's error vocabulary. `undiciTransport()` now refuses `proxy.type !== 'http'` at construction with a `TypeError` naming the type, before any dispatcher is allocated, deliberately outside the `IoError` tree so `retry/classify.ts`'s allow-list makes it non-retryable (RETRY-2). That is `TRANSPORT-30`'s "make the limitation discoverable rather than silently misbehaving" applied at the earliest point that can. **Why `ProxyType` still admits `socks4`/`socks5`.** Narrowing a `@public` union is a breaking change and therefore a release-pass decision, which this run is not taking (the run's release machinery is suspended); and `CFG-22`'s MUST is about the *model*, which would then no longer satisfy it. The honest state is a configuration layer that is complete and a transport layer that is not, which is what this row records. A future transport — a `node:net` SOCKS dialer, or a `ProxyAgent` replacement — closes the gap without a model change. | audit #67 / #81 | 2026-09-05 | `packages/core/src/config/proxy.ts:34` (`ProxyType`), `:372-380` (the scheme map); `packages/transport-undici/src/undici-transport.ts:147,160-167,201` (the supported type, the refusal, and where it runs); `packages/transport-fetch/src/fetch-transport.ts:79-82` (no `proxy` option, and why); `docs/product-spec/16-configuration.md:42` (`CFG-22`); `docs/product-spec/17-transport-adapter-conformance-contract.md:48` (`TRANSPORT-30`) | not yet in §10 | +| **`TRANSPORT-20`'s "any transport failure that produced no HTTP response" is read as an exchange that failed, not as a request the native client refused to make.** `TRANSPORT-20` (MUST) names four instances — connection refused, DNS/TLS failure, peer reset, connect/read timeout — and requires the retryable `TransportFailureError`. A scheme the client will not speak (`ftp://`), a forbidden method (`CONNECT`), a method that is not a token, a body on a GET: all of them also produce no HTTP response, so the literal reading makes them retryable too. The port refuses that. `retry/classify.ts:90` is an allow-list over `instanceof IoError`, so retryable would mean the caller's entire retry budget spent re-proving a URL that cannot change between attempts, and the requirement's own enumeration is four ways an *exchange* fails, not four ways an argument is rejected. Such a refusal surfaces as a bare `TypeError` carrying the native error as `cause`, outside the `IoError` tree, which is the same class both transports already raise for a misconfiguration caught at construction. **What changed on 2026-09-05 (audit #67 / #82).** The reading is not new — `@dexpace/transport-undici` has applied it to undici's `UND_ERR_INVALID_ARG` / `UND_ERR_NOT_SUPPORTED` since Phase 8a — but it was recorded only in that phase's checklist, and `@dexpace/transport-fetch` did the opposite for the identical condition: every native rejection became `TransportFailureError`. The decision now lives in one table in `@dexpace/transport-shared` that both adapters call, so the two cannot answer differently again, and `docs/sdk-documentation/write-a-transport.md`'s rule 4 tells a third transport to use it. **The MUST is still the default.** The table is an allow-list of three positive recognitions and everything else falls through to retryable; `'bad port'` is excluded by name, because port 1 is on WHATWG's blocked list and so `TRANSPORT-20`'s own dead-port conformance probe arrives with that reason on Node's `fetch`. | audit #67 / #82 | 2026-09-05 | `packages/transport-shared/src/dispatch-classification.ts:17,40,81,123` (the two tables, the predicate and the mapping, with the `'bad port'` exclusion documented at `:35`); `packages/transport-fetch/src/fetch-transport.ts:340` and `packages/transport-undici/src/undici-transport.ts:308,317` (the two call sites); `packages/core/src/retry/classify.ts:89-90` (the allow-list this is answerable to); `docs/product-spec/17-transport-adapter-conformance-contract.md:38` (the requirement); `docs/work/mvp/phase8/phase8a/2026-07-28-phase8a-transport-checklist.md:57` (where the reading was recorded before this row). Pinned by `packages/transport-conformance/src/run-suite.ts:387` on both adapters and by `tests/node-conformance/transport.test.mjs:351`, whose runtimes report the same refusal in entirely different shapes | not yet in §10 | ### Proposed erratum for `PIPE-40` (drafted 2026-09-04, not applied) diff --git a/packages/transport-conformance/src/run-suite.ts b/packages/transport-conformance/src/run-suite.ts index bbe2402..1e28538 100644 --- a/packages/transport-conformance/src/run-suite.ts +++ b/packages/transport-conformance/src/run-suite.ts @@ -16,8 +16,12 @@ // full proxy-challenge flow is transport-undici's challenge-handler.test.ts, and only its // unsupported-type refusal is a row here. TRANSPORT-22 is NOT driven from here -- // forcing an adaptation throw needs a per-transport hook into the native response, so each adapter -// asserts it against its own (transport-fetch's fetch-transport.test.ts:118, transport-undici's -// undici-transport.test.ts:614). +// asserts it against its own (transport-fetch's fetch-transport.test.ts:123, transport-undici's +// undici-transport.test.ts:615). TRANSPORT-9's producer-failure race is not driven from here +// either, and for the same reason: proving that a native call still pending when the producer fails +// is CANCELLED needs the signal the adapter handed it, which only an instrumented native client can +// show -- a `FetchLike` in transport-fetch's suite and a bring-your-own `Dispatcher` in +// transport-undici's, each with a call that resolves after the producer has already lost the race. import {mkdtemp, rm, truncate, writeFile} from 'node:fs/promises'; import {tmpdir} from 'node:os'; import {join} from 'node:path'; diff --git a/packages/transport-fetch/README.md b/packages/transport-fetch/README.md index cf97160..60891f4 100644 --- a/packages/transport-fetch/README.md +++ b/packages/transport-fetch/README.md @@ -69,6 +69,18 @@ records the decision and the four reasons the floor does not move instead. it (`SEAM-16`). Cancellation stays live for the whole in-flight window. - A timeout surfaces as the retryable `TransportFailureError`; a caller abort as the terminal `CancellationError` (`TRANSPORT-3`/`TRANSPORT-4`). A raw `DOMException` is never surfaced. +- A request `fetch` refused to make — an unsupported scheme such as `ftp://`, a forbidden method, an + argument its own validation rejects — is a bare `TypeError` outside the `IoError` tree, so + `retry/classify.ts`'s allow-list makes it non-retryable (`RETRY-2`). A failed *exchange* stays the + retryable `TransportFailureError` (`TRANSPORT-20`). The table that tells them apart is + `@dexpace/transport-shared`'s, shared with `@dexpace/transport-undici`, because the runtimes report + the same refusal in three different shapes (audit #67 / #82). +- A 204, a 304 and every HEAD response carry `body === null`. Node's `fetch` says so itself; Bun + 1.3.14's returns a live `ReadableStream` for all three, which this transport cancels and replaces + with `null` so the shape is the SDK's rather than the runtime's. +- `defaultTimeoutMs` must be an integer number of milliseconds in `1 .. 2**32 - 1` — + `AbortSignal.timeout()`'s range. Anything else is a `TypeError` out of `fetchTransport()`, not a + failure on the first send (`HTTP-35`). ## Conformance diff --git a/packages/transport-shared/README.md b/packages/transport-shared/README.md index dc04786..91242ee 100644 --- a/packages/transport-shared/README.md +++ b/packages/transport-shared/README.md @@ -18,5 +18,8 @@ other, not merely of the rest of the tree. | `header-mapping.ts` | `TRANSPORT-10`/`TRANSPORT-12`'s outbound drop-and-degrade pass and `TRANSPORT-14`'s lenient inbound copy, which preserves obs-text values rather than rejecting them | | `drop-log.ts` | `TRANSPORT-13`'s bounded, case-insensitive, drain-to-cap dedup of already-logged drop names. Names only — never values | | `abort-mapping.ts` | The single mapping from an aborted signal to a canonical SDK error: `TransportFailureError` on timeout, `CancellationError` otherwise. A raw `DOMException` is never surfaced | -| `body-pump.ts` | Turning a `Body` into a request stream the transport owns the closing of, plus `TRANSPORT-19`'s idempotent teardown for an abandoned producer | -| `signal-fork.ts` | `SEAM-16`'s abort-after-delivery rule: both native clients tie a response body's lifetime to the signal they were given, so the transport dispatches over a fork it detaches at delivery | +| `dispatch-classification.ts` | The single mapping from a *native rejection* to one: the retryable `TransportFailureError` `TRANSPORT-20` requires for a failed exchange, and a bare `TypeError` outside the `IoError` tree for a request the client refused to make. An allow-list, so an unrecognised rejection stays retryable | +| `body-less.ts` | Which method/status pairs can carry no response body at all, so `Response.body` is `null` for a 204, a 304 or a HEAD on every runtime rather than on whichever ones agree with the spec | +| `default-timeout.ts` | `HTTP-35`'s range check for a transport-wide default timeout: an integer in `1 .. 2**32 - 1`, which is `AbortSignal.timeout()`'s and therefore every transport's | +| `body-pump.ts` | Turning a `Body` into a request stream the transport owns the closing of, plus `TRANSPORT-19`'s idempotent teardown for an abandoned producer, and the classification of a producer's own failure | +| `signal-fork.ts` | `SEAM-16`'s abort-after-delivery rule: both native clients tie a response body's lifetime to the signal they were given, so the transport dispatches over a fork it detaches at delivery — and `TRANSPORT-9`'s other direction, a handle the transport pulls to cancel a native call it has abandoned | diff --git a/packages/transport-undici/README.md b/packages/transport-undici/README.md index c67c411..54edeee 100644 --- a/packages/transport-undici/README.md +++ b/packages/transport-undici/README.md @@ -110,6 +110,19 @@ silently misbehaving (`TRANSPORT-30`): (`TRANSPORT-12`), never a failed send. - Destroying the dispatcher mid-flight surfaces as the terminal `CancellationError`, while a timeout on the same path stays the retryable `TransportFailureError` (`TRANSPORT-8`). +- An argument undici refuses outright — a non-`http(s)` origin such as `ftp://`, `CONNECT` as a + method, a per-request `Proxy-Authorization` on a bring-your-own `ProxyAgent` — is a bare + `TypeError` outside the `IoError` tree, so `retry/classify.ts`'s allow-list makes it non-retryable + (`RETRY-2`). A failed *exchange* stays the retryable `TransportFailureError` (`TRANSPORT-20`). The + table that tells them apart moved to `@dexpace/transport-shared` on 2026-09-05 so + `@dexpace/transport-fetch` answers identically (audit #67 / #82). +- A 204, a 304 and every HEAD response carry `body === null`. undici's dispatcher always hands back a + `BodyReadable`, so this transport `dump()`s the one it declines to expose — returning the + connection to the pool — rather than wrapping an empty stream the caller would have to read to + discover was empty. +- `defaultTimeoutMs` must be an integer number of milliseconds in `1 .. 2**32 - 1` — + `AbortSignal.timeout()`'s range. Anything else is a `TypeError` out of `undiciTransport()`, raised + before any dispatcher is allocated, not a failure on the first send (`HTTP-35`). - `Response.protocol` is always `HTTP_1_1`: undici's `ResponseData` does not surface the negotiated version. A Deviation Ledger row, not a silent gap.