test(web): mark three open server-function gaps as expected failures - #3112
Merged
Conversation
Each is verified against `next` and stated as the behaviour that is wanted, so the suite stays green while the gap is open and turns red the day it closes. - A result the codec cannot encode is delivered as `undefined`. The function already ran; only the encoding failed, and it failed after the head was committed, so the status is spent and no error tag can be added. The truncated body decodes to the same answer a void function gives, so a write that succeeded is indistinguishable from one that returned nothing. - A streamed result has no backpressure: the stream is built with no `pull` and no queuing strategy, and every codec node is enqueued as soon as it is parsed. A consumer reading three chunks over 60ms left the producer 3695 items ahead; on a large or infinite stream one slow client buffers the whole result in server memory. - The decode depth cap guards the seroval path only, and the body format is chosen by the caller, so selecting the JSON format opts out of it: depth 5000 decodes where the capped path answers 400. `.fails` rather than the repo's `test.skip` idiom because the point is to notice the fix. Tests only; no runtime change, so no changeset.
|
Merging this PR will not alter performance
Comparing 
Footnotes
|
Review of the first commit turned up three things, one of which made a
test worthless:
- BODY_FORMAT_HEADER is not exported from the server entry, so the import
was `undefined` and the request carried a header literally named
"undefined". The JSON format was never selected and the function
received no argument at all: the handler answered {"depth":0} where the
gap needs {"depth":500}. The test failed, but not for its own reason —
exactly the failure mode `.fails` cannot show you. Local const, as
`server-functions-failure-signal.spec.tsx` already does.
- Depth 5000 sat on the repo's own cliff (shared.ts notes ~5900 nested
objects overflow V8's default stack on CI). 500 is comfortably past the
64-level cap and nowhere near it.
- The backpressure ceiling was wall-clock, and under `.fails` a starved CI
that produced fewer than 500 would have turned red for no reason.
Counted in event-loop turns instead: bounded stays near the queue size
on any machine, unbounded tracks the turn count.
Also adopts MATRIX.md's spelling (`test.fails` with a `// GAP:` comment),
which is the repo's documented idiom for this and which the first commit
wrongly described as absent, and drops a dead assertion after
rejects.toThrow().
This was referenced Aug 30, 2026
Closed
MATRIX.md's convention is a `// GAP:` comment; naming the issue in it means whoever closes one finds the test that turns red.
ryansolid
pushed a commit
that referenced
this pull request
Aug 31, 2026
* fix(web): pull a streamed result behind a demand gate The stream was built with no `pull` and no queuing strategy, and every codec node is enqueued as soon as it is parsed, so the producer ran as fast as it could resolve whether or not anyone read. One slow consumer buffered the entire result in server memory — unbounded, and invisible to application code. The gate sits in the iterator wrapper the runtime already installs, which its own comment calls "the only seam where a dropped consumer can stop the producer": the same seam lets a SLOW consumer slow it. A read drives `pull`, `pull` releases one source pull. Teardown releases a parked pull too, or an aborted stream would hang on it. Measured with an async generator and no reads for 200 event-loop turns: the producer advanced by 1, where it previously tracked the turn count (and reached ~17,000 items, +19 MiB, in the wall-clock shape the issue reports). Reading 20 chunks advances it by 20, and cancelling stops it within the pull in flight. The #3112 marker for this gap comes off, per the convention the other two followed when they closed. * fix(web): release a parked pull when the stream ends, and say what the gate covers Review of the first shape found a regression I introduced. `onDone` and `onError` set `closed` directly rather than through `teardown()`, so a pull parked on the demand gate was stranded: `desiredSize` is 0 after close and null after error, both failing the `> 0` check, so the gate never reopened and the source's `finally` never ran — leaking generator, cursor and file-handle cleanup once per failed request. Every path that ends the stream now runs `finishSource()`, which closes the source and releases the parked pull. Also from review: - The gate rests on the default high-water mark of 1, and `releaseDemand` holds a single resolver, safe only because the pump is sequential. Both were load-bearing and unstated; both are now in the comment. - `!streamController` in `wantsMore` was unreachable — `start()` assigns the controller before the iterator is ever opened. - The spec header still called #3118 open, and the changeset claimed the producer "advanced by one step" where the test asserts it stays near the queue size. - The liveness assertion was `> 1` under a comment claiming it tracked the reads; a gate that resumed twice in twenty reads would have passed. Scope is now stated rather than implied: only the result itself is gated. A nested `{ items: rows() }` is pumped by the codec directly and still runs ahead — measured at 200 items over 200 idle turns against 1 for the top-level shape. * test(web): make the teardown guard actually discriminate The first version of this test passed with finishSource() removed from both codec-end paths — it never reached the defect. Two reasons, and the second is the one worth writing down: - the source was NESTED (`{ rows: gen(), … }`), and a nested iterable never enters the wrapper, so nothing parked; - a top-level source needs no sibling branch to carry the failure. The deferred failure rides INSIDE a yielded chunk: a pending promise in the first value resolving to an object whose getter throws. The pump enqueues the chunk, asks for the second, parks because the consumer stopped reading, and only then does the promise settle and onError fire against a parked pull. `produced` is asserted alongside the cleanup so the nested-shape mistake cannot quietly recur: if the source never parks, the test says so instead of passing for the wrong reason. Verified both ways — with finishSource() in the codec-end paths the cleanup runs, without it the assertion fails. * fix(web): check finished before the gate, and make a test that parks Third review round, both findings real. The gate was checked before `finished`, so teardown landing while a pull was in flight stranded the codec's pump: the release it fires finds nothing parked, the in-flight pull then resolves, the pump asks for the next item, `wantsMore()` is false — 0 after close, null after error — and it parks on a resolver nobody will ever call. `push()` never returns. Impossible before this PR, where `finished` was checked first. One token, and it mirrors the defect the previous round fixed: that one stranded the source, this one stranded the pump. Worth stating plainly: this half is not guarded by a test. The failure is a leaked pending promise inside seroval's pump with no outward symptom — the source still runs its `finally`, the response still ends, nothing observable differs. It was found with instrumented park/release counters, and that is the evidence it rests on. The tests also did not park at all. Both read in a tight loop, so a read request is always pending, `desiredSize` never drops and the producer never reaches the gate — deleting `pull()` outright left all eight green, including the one whose comment claimed it would catch a deadlock. The new test pauses between reads, which is what puts the producer on the gate, and reads to completion with a deadline so a gate that never reopens fails fast instead of hanging. It counts arrivals by its own key rather than by the codec's node shapes. * test(web): say what the resume test actually pins Its comment claimed it was the case that would catch a deadlock. It is not: it reads in a tight loop, so a read request is always pending, `desiredSize` never drops and nothing parks — the same blindness review found in the original pair. What it pins is the cancel half. The pausing test is the liveness one.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Three gaps that are open on
nexttoday. Each test states the behaviour that is wanted and is markedtest.fails, pertest/lifecycle-matrix/MATRIX.md— "cells that FAIL against the current runtime are kept astest.failswith a// GAP:comment — those markers are the point of the suite".The suite stays green while a gap is open and turns red the day it closes, at which point the marker comes off and the test becomes an ordinary guard. Tests only, no runtime change, no changeset.
A result the codec cannot encode is delivered as
undefined(#3117)The function already ran and committed its side effects; only the encoding failed, and it failed after the head was committed — so the status is spent and no error tag can be added. The truncated body decodes to
undefined, the same answer a void function gives.A caller cannot tell "this mutation returned nothing" from "this mutation's result was lost". For a non-idempotent write that is the worst available reading, and a data layer may retry it.
The test drains the body into a buffer first, which is what a socket does — in-process the error surfaces live, and that difference is why this is invisible in a harness but not in a deployment.
A streamed result has no backpressure (#3118)
The response stream is built with no
pulland no queuing strategy, and every codec node is enqueued the moment it is parsed, so the producer runs as fast as it can resolve regardless of whether anyone is reading. On a large or infinite stream, one slow client buffers the whole result in server memory — invisible to application code and unbounded.Counted in event-loop turns rather than wall-clock, so the assertion means the same thing on any machine: a bounded producer stays near the queue size, an unbounded one tracks the turn count. (Measured on an idle machine: ~3700 items ahead of a consumer reading three chunks over 60 ms; ~17 000 items and +19 MiB of heap over 300 ms.)
The decode depth cap is opt-out (#3119)
depthLimit: 64is set "because payloads may come from an untrusted peer", and it guards the seroval path only. The body format is chosen by the caller, so selecting the JSON format hands the payload to a bareJSON.parseand skips the cap entirely.Each fails for its own reason
Run with the markers removed:
Worth recording why that check matters: in the first commit the depth test imported
BODY_FORMAT_HEADER, which the server entry does not export, so the request carried a header namedundefined, the JSON format was never selected, and the function received no argument. The test failed — but not for its own reason, andtest.failsreports that identically. It is the one real cost of this convention.