fix(core): an abandoned stream cancels its reader instead of leaking the socket (#686) - #713
Open
rejifald wants to merge 1 commit into
Open
fix(core): an abandoned stream cancels its reader instead of leaking the socket (#686)#713rejifald wants to merge 1 commit into
rejifald wants to merge 1 commit into
Conversation
…the socket (#686) A consumer that `break`s out of a `.stream()` loop leaked the connection on the DEFAULT decoder. `break` calls the generator's `.return()`, which runs the `finally` — and the `finally` only did `reader.releaseLock()`. Releasing a lock does not cancel the body, so the underlying response stream was never torn down: the vendor kept writing (and billing) into a connection nobody was reading. Two decoders were affected — `'bytes'` (stream.ts, the default, so this is reachable through the most natural consumer idiom without opting into anything) and `'json'` (json-stream.ts). `'lines'`/`'ndjson'` were already correct: they share `lineReader`, whose `finally` has done `await reader.cancel().catch(…)` on every exit path since the sse teardown work. So this is not a new pattern, it is bringing the other two decoders into line with the third. The three `finally` bodies are now identical, comment included, deliberately: three decoders doing the same job should not each carry their own idea of how a reader is let go. Cancelling unconditionally is safe — `cancel()` on an already-closed stream is a spec no-op — and the `.catch` swallows a reject from a body an abort already tore down. Tested with a cancel-recording endless body, which is what makes the leak observable at all: a real HTTP body cannot report its own cancellation back to a test, and an endless one is only ever ended by the consumer. An early `break` now cancels on the `bytes` default and on `json`, asserted at the surface level in stream.spec.ts and directly against the tokenizer in json-stream.spec.ts; all three of those fail on the pre-fix tree. A normal drain still delivers every chunk on both decoders, pinning that the unconditional cancel costs nothing. Scope is §2 of the issue only. §1 (nothing bounds a live stream) and §3 (no `done` event on abandon) are untouched and stay open, which is why this is `Refs` and not `Fixes`. Refs #686 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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
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.
The bug
A consumer who
breaks out of a.stream()loop leaks the socket on the default decoder.breakout of afor awaitcalls the generator's.return(), which runs thefinally. Thatfinallyonly released the reader lock — and releasing a lock does not cancel the body. Theunderlying response stream is therefore never torn down, so the vendor keeps writing (and billing)
into a connection nobody is reading.
Evidence, on
origin/main:packages/core/src/stream.ts:105-107— the'bytes'decoder:finally { reader.releaseLock(); }packages/core/src/json-stream.ts:264-266— identical.'bytes'is the defaultstream.decode, so this is reachable through the most natural consumeridiom without opting into anything.
The fix
The correct pattern already existed in the same package.
packages/core/src/line-reader.ts:70-76has done
await reader.cancel().catch(() => undefined)beforereleaseLock()on every exit pathsince the sse teardown work — which is why
'lines'and'ndjson', which sharelineReader, werenever affected.
So this is not a new pattern; it brings the other two decoders into line with the third. The three
finallybodies are now byte-identical, comment included, deliberately: three decoders doing thesame job should not each carry their own idea of how a reader is let go.
Cancelling unconditionally is safe —
cancel()on an already-closed stream is a spec no-op — and the.catchswallows a reject from a body an abort already tore down.What I tested
New tests, driven by a cancel-recording endless body. That shape is what makes the leak
observable at all: a real HTTP body cannot report its own cancellation back to a test, and an endless
one is only ever ended by the consumer, so a missing
cancel()is a stream left open.packages/core/test/stream.spec.ts— surface level: an earlybreakout of.stream()cancelsthe body on the
bytesdefault and ondecode: 'json'; a normal drain still delivers every chunkon both, pinning that the unconditional cancel costs nothing.
packages/core/test/json-stream.spec.ts— plumbing level, directly against the tokenizer, mirroringthe
lineReaderteardown tests that already live insse.spec.ts.All three
breaktests fail on the pre-fix tree (verified by stashing only the twosrcchangesand re-running: 3 failed / 59 passed).
Gates, all green from the repo root:
prettier --writeon every touched file,check:lint,check:types, the fullstitchapisuite (1494 passed, 2 skipped, 138 files),check-changelog.mjs,check-contract.mjs,check-unknown-keys.mjs. Also rancheck:size— the root entry does not exportthe
streamsubpath, so the bundle budgets are untouched (whole entryandimport { stitch }bothstill within budget, unchanged).
Scope
Refs #686, notFixes— this is §2 only. §1 (nothing bounds a live stream), §3 (nodoneeventon abandon) and §4–§6 are untouched and stay open. §3 in particular is adjacent and deliberately left
alone.
🤖 Generated with Claude Code