fs: fail WriteStream fast-path write() with ERR_STREAM_DESTROYED after destroy - #34267
Conversation
…r destroy child.stdin (and other FileSink-backed WriteStreams) install writeFast as an own .write that bypasses Writable.prototype.write. It already deferred to the real Writable machinery when state.ending was set, but not when state.destroyed was, so writing to an exited child's stdin returned true and called the callback with null while stdin.destroyed === true. Route through Writable.prototype.write when state.destroyed is set so the existing ERR_STREAM_DESTROYED handling runs.
|
Reproduced with Gate passed (fail-before/pass-after verified on ASAN and release). The new |
|
Updated 4:05 PM PT - Jul 15th, 2026
❌ @robobun, your commit c1207a6 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 34267That installs a local version of the PR into your bun-34267 --bun |
Walkthrough
ChangesDestroyed stdin write handling
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Overlaps with #34268, which includes this |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — one-condition addition to an existing guard, routing to already-correct Writable machinery.
What was reviewed:
- Confirmed
state.destroyedis a valid getter on WritableState (bitmap descriptor forkDestroyed), matching the existingstate.endingpattern. - Traced the deferred path:
Writable.prototype.write→_writecheckskDestroyed→ERR_STREAM_DESTROYED+ nextTick callback +errorOrDestroy, which no-ops on already-destroyed streams (so test 1 doesn't need an error listener). - Tests await real conditions (
once(child, 'close'/'exit')), usebunExe/bunEnv, and robobun confirmed they fail on system bun and pass on the debug build.
Extended reasoning...
Overview
The PR adds || state.destroyed to the existing state.ending guard in writeFast() (src/js/internal/fs/streams.ts). When either flag is set, the fast path defers to Writable.prototype.write instead of writing directly to the FileSink. Two new tests in test/js/node/child_process/child-process-stdio.test.js cover write-after-close and write-after-exit on a child's stdin.
Security risks
None. This is a Node.js compatibility fix in stream state handling; no auth, crypto, path handling, or untrusted input parsing involved.
Level of scrutiny
Low-to-medium. The source change is a single boolean condition mirroring the adjacent state.ending check that already exists on the same line. I verified in src/js/internal/streams/writable.ts that WritableState.prototype.destroyed is defined via makeBitMapDescriptor(kDestroyed), so the property read is valid, and that _write() (the target of the deferred call) already produces ERR_STREAM_DESTROYED, calls the callback via process.nextTick, and returns the error so Writable.prototype.write returns false. errorOrDestroy early-returns when the stream is already destroyed, so the first test (no error listener) won't crash on an unhandled error event; the second test adds an error listener defensively since it writes before the stream is fully torn down.
Other factors
- robobun confirmed both new tests fail with
USE_SYSTEM_BUN=1(ret: true, cbCode: undefined) and pass withbun bd test, satisfying the repo's test-validity requirement. - Tests follow harness conventions:
bunExe()/bunEnv,await once(...)on real events, nosetTimeout,Promise.withResolvers()for callback bridging, and combined-object assertions for readable failure messages. - The PR description notes #33508 will remove
writeFastentirely; this is the minimal targeted fix in the meantime, which is the right call. - No prior reviewer comments to address; only bot comments in the timeline.
There was a problem hiding this comment.
I didn't find any issues with the change itself — routing to Writable.prototype.write when state.destroyed is set is correct and the tests are solid. Deferring to a human only because of the noted overlap with #34268 (and #33508), which is a landing-order decision a maintainer should make.
What was reviewed:
- The
writeFastguard — adding|| state.destroyedonly widens the existing bail-to-prototype path; no new behavior beyond whatWritable.prototype.writealready implements for destroyed streams. - Confirmed
_writableState.destroyedis the right flag (matches theautoClosegetter and Node's internal_writehelper check). - Tests: both await real lifecycle events, wire spawn/write errors to reject, and assert exact
{ret, code}— no timing sleeps.
Extended reasoning...
Overview
One-line logic change in src/js/internal/fs/streams.ts: the writeFast fast-path override now bails to Writable.prototype.write when state.destroyed is set, in addition to the existing state.ending check. Two new tests in child-process-stdio.test.js cover write-after-close and write-after-exit on a child's stdin, asserting ret === false and callback error code ERR_STREAM_DESTROYED.
Security risks
None. This is a Node-compat behavioral fix in stream lifecycle handling; no auth, crypto, path, or untrusted-input parsing involved.
Level of scrutiny
Medium. The code change is mechanically trivial — it extends an existing guard with one more condition and delegates to already-correct shared machinery. The subtlety is entirely in stream semantics, and the fix defers to the canonical implementation rather than reimplementing anything. The PR includes fail-on-main / pass-on-PR evidence for both debug+ASAN and release builds.
Other factors
The change is correct and self-contained, and I would otherwise approve. However, robobun flagged that #34268 is a superset (same state.destroyed bail plus an errorOrDestroy fix for the EPIPE-while-alive case), and the PR description itself notes #33508 removes writeFast entirely. Which of these to land, and in what order, is a maintainer coordination call rather than a code-correctness question — so deferring rather than approving.
2 participants
Repro
Writing to a spawned child's
stdinafter the child has exited returnstrueand calls the write callback withnull, even thoughstdin.destroyed === trueandstdin.writable === false. Node returnsfalseand calls back withERR_STREAM_DESTROYED.Any code feeding a child pipeline (gzip, ffmpeg, a formatter) that flushes after the child dies believes the bytes were delivered: silent data loss with a success callback.
Cause
child.stdinis afs.WriteStreamon the FileSink fast path. That path installswriteFastas an own.writewhich bypassesWritable.prototype.writeentirely. It already deferred to the real Writable machinery whenstate.endingwas set (soend()thenwrite()correctly raisedERR_STREAM_WRITE_AFTER_END), but it never checkedstate.destroyed, so writes afterdestroy()reached the closed sink, which returned synchronously and mapped tocb(null); return true.Fix
Also defer to
Writable.prototype.writewhenstate.destroyedis set. The existing Writable_writehelper already handles this case (ERR_STREAM_DESTROYED, callback viaprocess.nextTick,errorOrDestroy), so routing through it gets the full Node contract for free.Verification
Added two tests in
test/js/node/child_process/child-process-stdio.test.jscovering write-after-closeand write-after-exit. Both fail on stock bun with{ ret: true, cbCode: undefined }and pass with the fix.Note: #33508 removes
writeFastentirely in favor of going throughWritable.prototype.writefor buffer accounting, which would also fix this. This PR is the minimal targeted change in case that one takes longer to land.[review] gate passed · iteration 1 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 1
evidence per changed file