Serde and pagination: abortable reads, paginator close on malformed PageInfo, tristate null, SSE single report (#79) by Wahbeh-Mohammad · Pull Request #94 · dexpace/nodejs-sdk

Wahbeh-Mohammad · 2026-09-05T09:07:38Z

Closes #79 (audit #67, milestone 4). Five defects, five commits, test-first; every case was watched red
first. Decisions are D17's; docs/deviations.md gets no row — see "Deviations" below.

What changed

1. deserializeFrom / serializeTo race the abort signal (1254707, SERDE-3).
throwIfAborted() before the lock and between chunks cannot interrupt an operation that is already
pending. A source that enqueues {"a": and stalls parked the drain inside reader.read(): the call
never settled and source.locked stayed true for the life of the process, against the seam's
promise that "an aborted call never leaves the caller's source locked". Both methods now race each
pending operation against the signal through a module-private abortRace() in @dexpace/codec-json
— one listener per drive, not per chunk, removed in the same finally that releases the lock. Core
exports no such utility and codec-json has no dependencies (SEAM-1), so it lives there. The signal's
reason is surfaced verbatim, as throwIfAborted() did.

Three facts measured on Bun 1.3.14 and Node 20.3/26 and now recorded in the code and the guide:
releasing a reader with a read outstanding is legal, does not throw, and does unlock the stream; the
outstanding read then rejects, differently per runtime (AbortError on Bun,
TypeError: Invalid state: Releasing reader on Node), which is why the caller must see its own
reason; that rejection is handled by Promise.race, but the abort promise itself needs a no-op
catch, because an abort landing between two reads has nothing racing it.

2. The paginator closes the response on every exit (f08bb19, PAGE-4, PAGE-27). #walk clears
held before dispatching, so a parse that resolves to something malformed threw from the
invariants with held === undefined and the finally closed nothing. Both invariants also tested
!== undefined while their messages said "never null", so parse → null died on info.items and
{items: null} died on the spread in Page's constructor — bare TypeErrors, naming nothing. The
checks now reject both nullish values and run inside pageOrClose, which shares
closeThenRethrow with PAGE-13's existing parseOrClose. Page's own guards had the same mismatch
and Page is @public, so they are fixed too.

3. A present Tristate can never carry null (b757a55, SERDE-14). present(inner.parse(input) as NonNullable<T>): the cast is what the compiler needs and is also the hole. An inner schema that
NORMALIZES to null produced the fourth state the union exists to forbid, after which isPresent(t)
narrows t.value to a non-null T that is null. Checked at run time and reported as a
DeserializationError — the input came off the wire. undefined is rejected with it.

4. An SSE release failure is reported once (ead5a62, SSE-30). #iterate's catch attached the
close error to the thrown one as suppressed, and then the finally ran #releaseQuietly, which
awaited the same rejected promise and handed the failure to onReleaseFailure as well. SSE-30's
scope is explicit — an automatic clean-terminal path is one with "no error in flight"
(docs/product-spec/13-server-sent-events-and-streaming.md:52) — so the finally now skips the
quiet release when the catch already released.

5. A pagination cursor with no UTF-8 form fails inside the error tree (80e16d7, PAGE-22). The
last URIError escaping core, handed over by #76. Unlike the sites #76 closed, this one needs no
caller mistake: the cursor is server-supplied and {"next":"\ud800"} is well-formed JSON.
spliceQueryParam / readQueryParam now reject a non-well-formed name or value with
UrlConstructionError, through the same hasLoneSurrogate predicate QueryParamsBuilder.add uses.
The message names the parameter and never echoes the value (D8's rule: a cursor can carry a token).

Test rows added

Where	Rows	What is pinned
`packages/codec-json/src/json-serde.test.ts`	4	abort during a parked read and a parked write settles with the caller's reason and unlocks; a bare `abort()` yields the platform `AbortError`; a signal that never fires changes nothing
`tests/node-conformance/serde.test.mjs`	3	the same two, on Node's Web Streams and `AbortSignal`, plus the completed-drain case. Both abort rows are red without the fix (they hung to their 2 s deadline)
`packages/codec-json/src/tristate-schema.test.ts`	4	a nullifying and an erasing inner schema are decode failures; wire-`null` and missing-key still decode ahead of the check; the same through `tristateObject`
`packages/core/src/pagination/lifecycle.test.ts`	5	`parse` → `undefined` / `null` / `{items: null}` / `{items: undefined}` each close exactly once and name the invariant; the suppressed pairing when that close also fails
`packages/core/src/pagination/page.test.ts`	3	`Page` rejects null/undefined items and a null response at construction
`packages/core/src/pagination/query-splice.test.ts`	7	lone surrogate in a value, in a name, on the remove path; the message names the parameter and not the value; a surrogate PAIR still splices
`packages/core/src/pagination/query-splice.property.test.ts`	1	over surrogate-bearing names and values, only `UrlConstructionError` — never a `URIError`
`packages/core/src/pagination/strategies.test.ts`	2	the same through `cursorStrategy` and `pageNumberStrategy`, asserted to be in the `DexpaceError` tree
`packages/core/src/sse/stream.test.ts`	1	mid-stream failure + failing close + hook installed → one report, on the thrown error

bun run test: 2415 pass, 0 fail. bun run test:node: 173 pass, 0 fail.

Deviations

None recorded. All five fixes move the code toward the spec text: SSE-30 names "no error in
flight" itself, SERDE-3 and SERDE-14 are being enforced rather than reinterpreted, and PAGE-4 /
PAGE-27 are what the invariant messages already claimed. docs/deviations.md is untouched, which is
also what the task partition says.

One departure from D17's letter, accepted and stated in the commit: tristate() rejects undefined
alongside null. NonNullable<T> excludes both, and a Present of undefined is an Absent wearing
the wrong label; D17's === null named the one shape the audit reproduced.

API reports

api:local run in core and codec-json; both reports are byte-identical. The reports carry
signatures, not prose, and every change here is a @throws or a scope clarification —
tristate, tristateObject, cursorStrategy, pageNumberStrategy, spliceQueryParam,
readQueryParam, SseStreamOptions.onReleaseFailure.

Docs

docs/sdk-documentation/write-a-serde.md: the reference CSV codec races its abort too, and rule 7
("an abort must race the pending operation, not sit between two of them") states why, with the
per-runtime release behaviour measured. Rule count corrected from six to seven. Fences typecheck
(check-fences.mjs).

Deferred — release machinery

Suspended under D1; nothing below was written.

Patch changeset for @dexpace/codec-json: an aborted deserializeFrom / serializeTo now
settles while a read or write is pending and releases the caller's lock; tristate() raises
DeserializationError when an inner schema resolves a present value to null or undefined
(previously produced {kind: 'present', value: null}); .d.ts @throws prose changed on
tristate and tristateObject.
Patch changeset for @dexpace/core: the paginator closes the response when a strategy returns
a malformed PageInfo, and both its invariants plus Page's constructor now reject null as well
as undefined; an SSE release failure during an in-flight error is no longer also passed to
onReleaseFailure; spliceQueryParam / readQueryParam reject an unpaired surrogate with
UrlConstructionError where a bare URIError used to escape; .d.ts @throws prose changed on
cursorStrategy and pageNumberStrategy. The release pass should decide patch vs minor: the SSE
hook and the pagination error class are both observable behaviour changes, not only prose.
No docs/first-release.md edit, no version bump.

Notes for the supervisor

A second double-report still exists, deliberately left. bindAbort (sse/stream.ts:263-284)
routes a failing abort-triggered close() to onReleaseFailure. If an iterator is parked in a
read at that moment, the same failure also arrives as suppressed on the error the consumer
catches — measured: one hook call plus one SuppressedError. D17 named
#releaseWithInFlightError only, and the abort listener cannot know whether an iterator is parked,
so this is a decision rather than an oversight fix. Flagging it rather than taking it.
UrlConstructionError's own TSDoc (packages/core/src/http/errors.ts:129-146) says "Three cases"
and names QueryParamsBuilder.add as the site that rejects an unpaired surrogate. The category
still covers the pagination splice, but the site list is now short by one. http/errors.ts is
outside this task's partition, so it is untouched; the one-clause fix is to append
"…and spliceQueryParam / readQueryParam do the same for a paging parameter" to the third
bullet.

Gate

node .claude/skills/ci-preflight/run-ci.mjs --clean (no --node-floor; other agents were running
alongside): all 20 steps passed, from a swept tree on the pinned Bun 1.3.14.

…e (SERDE-3) `deserializeFrom` checked `signal.throwIfAborted()` before the lock and after each chunk, and `serializeTo` checked once before the lock. Neither can interrupt an operation that is already pending: a source that enqueues `{"a":` and then stalls parks the drain inside `reader.read()`, so an abort 20 ms later reaches nothing, the call never settles, and `source.locked` stays `true` for the rest of the process. `seams/serde.ts:210-212` and `write-a-serde.md` both promise that "an aborted call never leaves the caller's source locked"; the promise was aspirational. Both methods now race each pending operation against the signal through a module-private `abortRace()` — one listener for the whole drive rather than one per chunk, removed in the same `finally` that releases the lock. Core exports no abort-race utility and codec-json has no dependencies (SEAM-1), so the helper lives here. Three measured facts the code and the guide now record: - Releasing a reader with a read outstanding is legal on Bun 1.3.14 and Node 20.3/26, does not throw, and does unlock the stream. The outstanding read then rejects — `AbortError` on Bun, `TypeError: Invalid state: Releasing reader` on Node — which is why the caller must see the signal's own `reason` instead of whichever the runtime picked. - That rejection is not an unhandled one: `Promise.race` keeps a handler on the loser. - The abort promise itself needs a no-op `catch`, because an abort landing between two reads has nothing racing it and Node's default policy would take the process down. The signal's `reason` is surfaced verbatim, which is what `throwIfAborted()` did and what the seam documents; a bare `abort()` still yields the platform `AbortError`. Tests: four cases in `json-serde.test.ts` and three in `tests/node-conformance/serde.test.mjs` — runtime-divergent twice over (Web Streams and `AbortSignal`), and the Node tree is where the release-rejection difference shows. All were red before the change: the two pending-abort cases hung to their deadline on both runtimes. `write-a-serde.md`'s reference implementation races too, and gains rule 7 for it. Found by: audit #67 / #79. D17.

…lformed PageInfo (PAGE-4, PAGE-27) `#walk` clears `held` before dispatching, so between the transport call and the assignment of the next page there is a window in which nothing owns the response but the loop body. `parseOrClose` covered the part of that window where `parse` *rejects*. It did not cover the part where `parse` *resolves* to something malformed: the two invariants below it threw with `held === undefined`, so the `finally` closed nothing and the response leaked. Reproduced with a strategy returning `undefined` and one returning `{items: null}`. Both invariants also tested `!== undefined` while their messages said "never null", so: - `parse` returning `null` never reached its own invariant — it died on `info.items` with a bare `TypeError` from property access. - `{items: null}` passed both invariants and died in `Page`'s constructor, on the spread that copies the item list. `Page`'s own guard had the same mismatch, and `Page` is `@public`, so a consumer constructing one directly saw it too. The checks now reject `null` and `undefined` alike, and run inside `pageOrClose`, which applies PAGE-13's close-then-rethrow discipline to the construction path as well. `closeThenRethrow` is the shared tail of both, so the two paths cannot drift: the primary failure stays primary and a close failure rides as suppressed (PAGE-15). Tests: four malformed shapes through the page view, each asserting the invariant message and `close` called exactly once; the suppressed pairing when that close also fails; and two direct `Page` construction cases. All were red — two on the leaked response, three on the error class or message. Found by: audit #67 / #79. D17.

`tristate()` returned `present(inner.parse(input) as NonNullable<T>)`. The cast is what the compiler needs — `T` is the caller's, unconstrained, so it cannot know `inner.parse` returns non-null — and it is also the hole: an inner schema that NORMALIZES a value to `null` (a Zod `.transform()`, a "" → null cleanup) produced `{kind: 'present', value: null}`, the fourth state SERDE-14's union exists to forbid. Downstream, `isPresent(t)` then narrows `t.value` to a non-null `T` that is null. Checked at run time after `inner.parse`, and reported as a `DeserializationError`: the input came off the wire, and it is the pairing of that input with that schema that has no Tristate — not a broken invariant of this package. `undefined` is rejected alongside `null`, since `NonNullable<T>` excludes both and a Present of `undefined` is an Absent wearing the wrong label. Deliberately wider than D17's `=== null`, which named the one shape the audit reproduced. The wire-`null` and missing-key branches return before `inner.parse` runs, so a normalizing schema cannot turn a legitimate Null or Absent into a failure; a case pins that. `@throws` added to both `tristate` and `tristateObject`; `codec-json.api.md` is byte-identical, because the report carries signatures and not prose. Found by: audit #67 / #79. D17.

…SSE-30) A mid-stream read failure whose release also fails was reported twice. `#iterate`'s catch called `#releaseWithInFlightError`, which attaches the close error to the thrown one as `suppressed` — and then the `finally` ran `#releaseQuietly` anyway, which awaited the same already-rejected `#closing` promise and handed the close error to `onReleaseFailure` as well. A consumer with a logger wired into that hook saw one failure logged out-of-band and caught the same failure a second time on the error it was already handling. SSE-30's scope is explicit: an automatic clean-terminal path is "natural end-of-stream or a done-sentinel, **with no error in flight**" (`docs/product-spec/13-server-sent-events-and-streaming.md:52`). With an error in flight there is something to attach the failure to, and SSE-29 says to attach it there. The `finally` now skips the quiet release when the catch already released — a local flag on the one generator activation that reads it, rather than a field, since nothing outside `#iterate` has a question to ask of it. `#releaseQuietly` keeps reporting unconditionally, which is what its name promises; the clean-terminal case and the explicit-`close()` case are unchanged. The hook's `@public` TSDoc now states the scope it always had. `core.api.md` is unchanged: the report carries signatures, not prose. Test: mid-stream failure, failing close, hook installed — one report, on the thrown error. Found by: audit #67 / #79. D17.

…ror tree (PAGE-22) The last `URIError` escaping core, handed over by #76. `spliceQueryParam` and `readQueryParam` share HTTP-29's component encoder, which is `encodeURIComponent`, which throws a bare `URIError: URI malformed` on a string carrying an unpaired surrogate. Unlike the call sites #76 closed, this one needs no caller mistake: the cursor is SERVER-supplied and `{"next":"\ud800"}` is well-formed JSON that `JSON.parse` hands back verbatim, so a hostile or merely broken server could make a walk die with a platform error naming neither the parameter nor the page. Repro from D17: `spliceQueryParam(new URL('https://h/?a=1'), 'cursor', '\uD800')`. Both functions now reject a non-well-formed name or value with `UrlConstructionError` — the class `QueryParamsBuilder.add` throws for the same input, through the same `hasLoneSurrogate` predicate, so the two cannot drift. The message names the parameter and never echoes the value: a cursor is opaque server state and can carry a session token (D8's rule, applied here). A well-formed surrogate pair stays ordinary text. The engine already closes the response on this path — a strategy's `parse` rejecting is PAGE-13's case — so nothing leaks. Tests: seven direct cases, two through `cursorStrategy` / `pageNumberStrategy` asserting the error is in the `DexpaceError` tree, and a property over surrogate-bearing names and values that admits only `UrlConstructionError`. All were red, on `URIError: String contained an illegal UTF-16 sequence`. `@throws` added to both public strategies and both internal helpers; `core.api.md` is unchanged, since the report carries signatures and not prose. Found by: audit #67 / #79. D17, D14's rule for which class.

Two prose corrections in `@dexpace/core`'s public surface, both of them claims the round-1 fixes made incomplete. Round 2 of #79; these files were outside the task partition and are now in it. `UrlConstructionError`'s surrogate bullet named `QueryParamsBuilder.add` as the site that rejects a string with no UTF-8 form. Pagination's splice now rejects the same input for the same reason through the same predicate, so the bullet names it too — with the part a consumer actually needs: that path is reachable with no caller mistake, because the cursor is server-supplied, and its message names the parameter rather than echoing the value. Backticked prose, not `{@link}`: `spliceQueryParam` and `readQueryParam` are `@internal` and `api:ci` fails on the unresolved link even where `api:local` only warns. `Deserializer.deserializeFrom` and `Serializer.serializeTo` said the signal is "checked before the lock is taken and between reads". That describes what an implementation may do and not what the promise on the same line requires: only racing each pending operation makes "an aborted call never leaves the caller's source locked" true, because a stalled source parks the drain inside a read that a between-reads check can never reach again. The clause now says raced, and says which half is load-bearing — a codec author reading only this seam had no way to know. `core.api.md` is unchanged: the report carries signatures, not prose.

`write-a-paging-strategy.md` had four rules and no statement of the one obligation the engine enforces with an assertion. A strategy that returns `undefined`, `null`, or `{items: null}` now closes the response and throws, naming the invariant (#79) — and the guide should say so before an author meets it, along with the reason it is not treated as a quiet end of stream: "the strategy forgot to `return`" and "the server ran out of pages" must not look the same from the outside. The rule also draws the line the guide left implicit, and which the `PaginationError` paragraph below it only half states: ending a walk is `pageInfo(items)` with no next request, failing one is a throw. Both are supported; neither is `undefined`. Round 2 of #79; this file was outside the task partition and is now in it. Fences typecheck.

Wahbeh-Mohammad added 7 commits September 5, 2026 11:53

Wahbeh-Mohammad merged commit 4576658 into audit/remediation-67 Sep 5, 2026
3 checks passed

This was referenced Sep 5, 2026

Audit #67 remediation: milestones 1–5 (#68–#82) #98

Merged

Serde and pagination: deserializeFrom abort, paginator leak on malformed PageInfo, tristate null cast, SSE double-reported release failure #79

Closed

Wahbeh-Mohammad deleted the audit/67/79-serde-pagination-sse branch September 7, 2026 18:42

Navigation Menu

Uh oh!

Serde and pagination: abortable reads, paginator close on malformed PageInfo, tristate null, SSE single report (#79) - #94

Serde and pagination: abortable reads, paginator close on malformed PageInfo, tristate null, SSE single report (#79)#94
Wahbeh-Mohammad merged 7 commits into
audit/remediation-67from
audit/67/79-serde-pagination-sse

Wahbeh-Mohammad commented Sep 5, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

Wahbeh-Mohammad commented Sep 5, 2026

What changed

Test rows added

Deviations

API reports

Docs

Deferred — release machinery

Notes for the supervisor

Gate

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant