Skip to content

fix(node): prevent uncatchable crash when canceling a request body stream - #76

Merged
dinwwwh merged 2 commits into
mainfrom
claude/standardserver-stream-cancel-crash-1b827c
Aug 26, 2026
Merged

fix(node): prevent uncatchable crash when canceling a request body stream#76
dinwwwh merged 2 commits into
mainfrom
claude/standardserver-stream-cancel-crash-1b827c

Conversation

@dinwwwh

@dinwwwh dinwwwh commented Aug 26, 2026

Copy link
Copy Markdown
Member

Canceling a streamed request body mid-upload (e.g. rejecting an oversized upload while the client is still sending) could crash the entire server process. Node's Readable.toWeb adapter enqueues from 'data' events, so a chunk arriving after cancel hits an already-closed controller and throws an uncatchable ERR_INVALID_STATE (nodejs/node#54205) — no try/catch in user code can reach it. Wrapping the adapter in a TransformStream turned out to shield the crash only on some Node releases (fine on 22 and 26, still crashed on 24), so the final fix drops Readable.toWeb entirely.

Fixes

  • body.ts and event-stream.ts now convert via a hand-rolled pull-based toWebReadableStream util: chunks are only enqueued inside pull(), which never runs after cancel, so the crash is impossible by construction regardless of Node's adapter internals.
  • Canceling the body still aborts the upload and tears the source down, with one deliberate exception: http1 server requests share their socket with the response, so they are abandoned (stalled by backpressure, reclaimed on connection teardown) instead of destroyed — in-flight responses stay deliverable.
  • Consumers now receive plain Uint8Array chunks backed by exact-sized buffers instead of pooled Node Buffers, so touching chunk.buffer can no longer expose unrelated pool memory.

Testing

  • Full package and e2e suites (including the signal-and-cancel contract tests) pass on Node 22, 24, and 26.
  • Flood-and-abort upload servers over http1 and http2 confirm 25 consecutive aborted uploads complete with zero crashes; companion tests document that bare Readable.toWeb still crashes under the same load, so they double as the signal for when the workaround can be dropped.
  • body and event-stream tests assert the returned stream is exactly the wrapper's output; unit tests cover byte fidelity, chunk copying, and mid-read cancellation.

…ream

A consumer canceling a streamed request body mid-upload could crash the
whole process: Node's Readable.toWeb adapter enqueues from 'data' events,
so a chunk arriving after cancel hits an already-closed controller and
throws an uncaught ERR_INVALID_STATE (nodejs/node#54205).

Route all Readable-to-web conversions in body and event-stream through a
new cancel-safe toWebReadableStream util, which pipes the adapter through
a TransformStream so cancellation never reaches its controller mid-chunk,
and copies each chunk out of Node's pooled Buffer memory. Pinned by
flood-and-abort upload tests over http1 and http2, including tests
documenting that the bare adapter still crashes.
@pkg-pr-new

pkg-pr-new Bot commented Aug 26, 2026

Copy link
Copy Markdown
@standardserver/aws-lambda

npm i https://pkg.pr.new/@standardserver/aws-lambda@76

@standardserver/core

npm i https://pkg.pr.new/@standardserver/core@76

@standardserver/fastify

npm i https://pkg.pr.new/@standardserver/fastify@76

@standardserver/fetch

npm i https://pkg.pr.new/@standardserver/fetch@76

@standardserver/node

npm i https://pkg.pr.new/@standardserver/node@76

@standardserver/peer

npm i https://pkg.pr.new/@standardserver/peer@76

@standardserver/shared

npm i https://pkg.pr.new/@standardserver/shared@76

commit: 65fd468

@codspeed-hq

codspeed-hq Bot commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

Merging this PR will not alter performance

✅ 26 untouched benchmarks
⏩ 108 skipped benchmarks1


Comparing claude/standardserver-stream-cancel-crash-1b827c (65fd468) with main (74ba31d)2

Open in CodSpeed

Footnotes

  1. 108 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

  2. No successful run was found on main (e5e775f) during the generation of this report, so 74ba31d was used instead as the comparison base. There might be some changes unrelated to this pull request in this report.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

The TransformStream wrap does not reliably prevent the crash, and toWebReadableStream's own unit test currently fails: utils.test.tsdoes not throw when a raw buffer stream is cancelled mid-read deterministically leaks an uncatchable ERR_INVALID_STATE ("Controller is already closed") on Node v24.18.0.

Reviewed changes

  • Cancel-safe toWebReadableStream util — new wrapper in packages/node/src/utils.ts:15 that pipes Readable.toWeb(stream) through a TransformStream, copying each chunk into a fresh Uint8Array.
  • Routed all node request-body stream conversions through itbody.ts (toStandardBody, octet-stream path) and event-stream.ts (toAsyncIteratorObject) no longer call bare Readable.toWeb(req).
  • Flood-and-abort tests — http1/http2 upload servers; a "documents the bug" bare Readable.toWeb negative test and a "keeps ... from crashing" wrapped positive test per protocol, plus unit tests for byte fidelity, copy semantics, and mid-read cancellation.

Pullfrog  | Fix all ➔Fix 👍s ➔View workflow run | Using DeepSeek Flash (default — pick a model for stronger reviews) | 𝕏

Comment thread packages/node/src/utils.test.ts
The Readable.toWeb + TransformStream approach only shielded the adapter's
controller on some Node releases: on Node 24 a chunk arriving after cancel
still hit the closed controller and crashed the process with an uncaught
ERR_INVALID_STATE, even for plain Readable sources.

Hand-roll the adapter instead of wrapping Readable.toWeb: chunks are only
enqueued inside pull(), which never runs after cancel, so the crash is
impossible by construction regardless of Node's adapter internals. Cancel
destroys the source, except http1 server requests — they share their
socket with the response, so they are abandoned (stalled by backpressure,
reclaimed on connection teardown) to keep in-flight responses deliverable.

All package and e2e suites pass on Node 22, 24, and 26.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes

  • Replaced the TransformStream wrap with a hand-rolled pull-based ReadableStream in toWebReadableStream (packages/node/src/utils.ts) that reads the source via its async iterator, so chunks are only enqueued inside pull and never after a consumer cancel — eliminating the ERR_INVALID_STATE process crash by construction.
  • Reworked cancel(reason) to tear the source down with stream.destroy(reason), except http1 server IncomingMessages (shared socket with the response), which are deliberately abandoned to keep in-flight responses deliverable.
  • Updated the toWebReadableStream unit/flood-and-abort tests; the mid-read cancel test that previously failed deterministically on Node 24 now passes.

The prior review's critical concern (mid-read cancel leaking an uncatchable ERR_INVALID_STATE on Node v24.18.0 through the TransformStream wrap) is fully addressed and verified: I ran packages/node tests on this exact Node version — the does not throw when a raw buffer stream is cancelled mid-read test and both "keeps an aborted upload from crashing the process" flood tests pass, and the full 73-test suite is green.

Pullfrog  | View workflow run | Using DeepSeek Flash (default — pick a model for stronger reviews) | 𝕏

@dinwwwh
dinwwwh merged commit cd7c12f into main Aug 26, 2026
6 of 10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant