Prevent streaming disconnects from crashing Node - #15324
Conversation
Assisted-By: devx/06090486-2b20-4fb8-99f0-cdf7db642619
Preview Build AvailablePreview builds have been created for this PR. You can install pnpm install "remix-run/react-router#preview/pr-15324&path:packages/react-router"And/or install other packages via: pnpm install "remix-run/react-router#preview/pr-15324&path:packages/react-router-dev"
pnpm install "remix-run/react-router#preview/pr-15324&path:packages/react-router-express"
pnpm install "remix-run/react-router#preview/pr-15324&path:packages/react-router-node"
pnpm install "remix-run/react-router#preview/pr-15324&path:packages/react-router-serve"These preview builds will be updated automatically as you push new commits. |
✅ CLA SignedThanks for signing the Contributor License Agreement. |
✅ Change File FoundOne or more change files found.
|
| } | ||
| } catch (error: unknown) { | ||
| try { | ||
| reader.cancel(error).catch(() => {}); |
There was a problem hiding this comment.
This covers the res side, but reader.cancel(error) here still passes the same
synthetic close error upstream. It goes through createReadableStreamFromReadable's
StreamPump.cancel into body.destroy(error) on the source stream. If there's a
Transform between the render and body (CSP nonce injection, or
@sentry/react-router's getMetaTagTransformer). React's .pipe() never put an
'error' listener on that stream, so the destroy throws the same unhandled 'error',
just on the upstream stream instead of the response. That's the Sentry stack marilari88
posted in #15287.
I applied this change on top of 8.2.0 and ran it through a real bare-express pipeline with
a transform in entry.server and got a real client abort mid-stream:
SERVER EXIT code 1
Error: Writable closed before stream finished
at ServerResponse.onClose (@react-router/node/dist/index.js)
Emitted 'error' event on PassThrough instance
Repro: https://github.com/guiterguy219/react-router-15287-repro
Same class of fix covers it: cancel the reader without the synthetic reason, or tag the
benign close in onClose and skip both destroy and the error-carrying cancel for that
tag, while real write/render errors still destroy + throw.
This pull request is one approval away from merging a high-severity bug.remix-run/react-router · PR #15324 — Prevent streaming disconnects from crashing Node — was open right now. We ran PR Quorum's review panel over the same diff, read-only, live. Total cost: $0.0011. Here is what it caught: 🟠 destroyWritable may silently swallow legitimate writable errors
In remix-run/react-router · Prevent streaming disconnects from crashing Node. The Suggested fix
The receipts
remix-run/react-router merges hundreds of pull requests a year. At about $0.0011 a review, checking every one of them costs a few dollars.
|
Client disconnects can close a Node writable while a response is still streaming. The stream pump then destroys the writable with an error after its monitor has removed the error listener, allowing the asynchronous
errorevent to terminate the process.This keeps destroy errors handled for both readable streams and async iterables, while avoiding redundant destruction of already-destroyed writables.
Closes #15287