Skip to content
PR Quorum
Sign inStart free
← Review gallery
remix-run/react-router #15324openreviewed 6h ago

Prevent streaming disconnects from crashing Node

by brophdawg11+46 23 filesreviewed in 24sView PR on GitHub
needs attentionThe panel surfaced issues that deserve attention before merge.1 finding · 1 high+

Reviewer panel

Correctness
deepseek/deepseek-v4-flash
1 found
Security
deepseek/deepseek-v4-flash
0 found
Architecture
deepseek/deepseek-v4-flash
0 found

Findings

01Hhighconf 95%

destroyWritable may silently swallow legitimate writable errors

packages/react-router-node/stream.ts:60

The destroyWritable function attaches a no-op once('error') listener before calling writable.destroy(error). This listener consumes the asynchronous 'error' event that Node emits when destroy() is called with an error on a writable that is not yet destroyed. However, if the writable is already destroyed (the early return on line 61-63), the function returns without attaching the error listener. In that case, if writable.destroy(error) was called elsewhere (or the writable was destroyed by some other mechanism) and the error event fires asynchronously, there is no listener to catch it, potentially crashing the process. The early return should also attach a no-op error listener to the already-destroyed writable to ensure any pending error events are consumed.

Suggested fix

function destroyWritable(writable: Writable, error: Error) { if (writable.destroyed) { // Ensure any pending error events from a previous destroy are consumed writable.once("error", () => {}); return; }

// The write promise carries this error to the caller. Also consume the // asynchronous error event from destroy so it cannot crash the process // after the writable error monitor has been cleaned up. writable.once("error", () => {}); writable.destroy(error); }

Want this on your own pull requests?
The same reviewer panel runs on every PR — one focused review, no noise.
Start freeLive demo