Repository navigation
Conversation
child.stdin of node:child_process is a fast-path WriteStream over a FileSink that is already open. writableFromFileSink built the stream with no fd and stored the sink after the constructor returned, so the stream kept the async _construct step that only an fd skipped. Until that tick Writable buffered the chunk of end(chunk), and a destroy(), a stream error or a process.exit() in the same tick dropped it. The constructor now receives the sink as $fastPath and treats it like an open fd: the stream is born constructed. An fd that the for-in copy of the options took from Object.prototype does not replace the sink. child.stdin no longer emits 'open' and 'ready', and its `pending` is false until it is destroyed, as in Node.js. The chunk now reaches the sink before destroy(), so _destroy can see the flush fail. A failure of sink.end() on a stream destroyed before 'finish' is a write error on a destroyed stream, which Node.js drops: _destroy reports only its own error there.
|
Updated 11:55 AM PT - Oct 16th, 2026
✅ @robobun, your commit 3572b216c9a4faefcb29b61f1845d7fcd8b1a2b6 passed in 🧪 To try this PR locally: bunx bun-pr 45195That installs a local version of the PR into your bun-45195 --bun |
|
Status Reproduction on linux-x64, with Node.js v26.3.0 as the reference: const { spawn } = require("node:child_process");
const child = spawn("cat", [], { stdio: ["pipe", "inherit", "inherit"] });
child.stdin.end("hello");
process.exit(0);
Pull request: #45195 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughWriteStream now adopts an existing FileSink as its fast path and updates fast-path destruction and pending-state handling. New child-process tests exercise stdin call sequences, buffered input, stream state, and results compared with Node.js v26.3.0. ChangesWritable stream fast paths
Suggested reviewers: Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation |
|---|---|---|
| Linked Issues check |  |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
| Title check | |
The title clearly and concisely identifies the primary change: making child.stdin constructed immediately to match Node.js behavior. |
| Description check | |
The description explains the problem, implementation, verification, known differences, and trade-offs. It does not use the exact template headings, but it provides the required content and is sufficie… |
Full details: Description check
-
Explanation
The description explains the problem, implementation, verification, known differences, and trade-offs. It does not use the exact template headings, but it provides the required content and is sufficiently complete.
✨ Finishing Touches: 💡 1
-
🛠️ Fix failing CI checks: 💡
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
The fast-path WriteStream that wraps an open FileSink (child.stdin) takes no option. Its options object now has a null prototype, so an fd, fs or autoClose on Object.prototype does not reach it. This replaces the rule that dropped only an inherited fd. The test for the polluted prototype checks fd and autoClose on the stream. The tests with a child that has closed its stdin reject when the child fails or exits before it is ready.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Each comment that this branch adds in streams.ts is now one line. The note about path.resolve("") goes: no caller passes a path to resolve.
There was a problem hiding this comment.
I re-reviewed the two follow-up commits and found no new bugs; the earlier inline points about the readiness wait, the inherited-fd test assertion and the wider inherited-option class are addressed by cc0815c/3572b216. A maintainer look is still worthwhile because this changes user-observable child.stdin semantics in the spawn tick and the new table test pins five deliberate divergences from Node (notably end(cb); destroy() delivering cb(null) and 'finish' on a destroyed stream, deferred to #45002) — whether to lock those in is a judgment call.
What was reviewed:
- Tri-state
$fastPathagainst everykWriteStreamFastPathconsumer (write/_write/_writev/_final/_destroy/close/pending, ProcessObjectInternals stdout, child_process kill path): tty/process.stdout keep thetrue-then-open-from-fd flow, andtruewith no fd still keeps_constructand reportspending === true. _destroy:cbruns exactly once on the sync-throw, rejected, resolved and non-promise paths; the pre-'finish' guard matches Node'serrorOrDestroydropping errors on an already-destroyed stream.{ __proto__: null }options for the sink case now coverfd,autoClose,fs,startandflushtogether, and the pollution test observesfd/autoClosedirectly rather than only the echoed bytes.
Extended reasoning...
The diff touches src/js/internal/fs/streams.ts (fs.WriteStream constructor, _destroy, pending getter, writableFromFileSink) plus new child_process and tty tests and a call-order fixture; it is the node:child_process stdin stream used by every spawn() with a piped stdin. No security-sensitive surface beyond the prototype-pollution hardening of the option copy, which now uses a null-prototype object for the sink case. Approve was ruled out because the change alters observable Node-compat behavior (callback timing, 'finish' after destroy(), pending, error reporting on destroy) and the test suite hard-codes five documented divergences from Node v26.3.0 that a maintainer should accept or reject; it is neither mechanical nor small. No CODEOWNERS entry covers the changed paths and the hunt ran dry with no findings, so a short defer rather than silence is warranted given new commits since the prior review.
Problem
child.stdin.end(chunk)loses the chunk whenprocess.exit(),destroy()or a stream error follows in the same tick asspawn(). Afterend("hello"); process.exit(0)the child reads 5 bytes in Node.js, 0 since Bun 1.2.0 (node fs compat pr #2 #16422). No user report.writableFromFileSink(src/js/internal/fs/streams.ts:830) stored the sink after the constructor returned. The constructor skips_constructonly for an fd (:480), soWritableheld the chunk for one tick.Fix
pendingfalse untildestroy(), as in Node.js._destroydrops a flush error before 'finish', as Node.js does. Without this,end(chunk); destroy()to an exited child throws EPIPE.test/js/node/child_process/child_process.test.ts(9 new tests, 7 fail without it),tty.test.ts, 161 vendored Node.js tests.Background
Writablewith a_constructmethod buffers until it calls back, one tick after creation. The fast path (aWriteStreamover aFileSink, Bun's native writer) skips it for process.stdout since node:stream: sync the stream suite to Node v26.3.0 and fix the gaps it surfaces — stream/iter + zlib/iter behind --experimental-stream-iter, read()-one-buffer semantics, compose/pipeline/web-adapter/http/fs/net parity (247/251 upstream tests, +63 vendored) #31826.fd != nullcondition (also changesnew tty.WriteStream()with no fd). Anet.Socketstdin (child_process: make piped stdio streams instances of net.Socket #36316, or fd-backed) is larger: this is the stopgap.Downsides
end(cb); destroy()givescb(null)and 'finish' until child_process: end the stdin pipe in child.stdin's _final #45002.end(1 MiB); destroy()to a child that never reads: the parent waits for the child, as afterwrite().Notes
Versions, and what #16422 changed
end("hello"); process.exit(0)and inend("hello"); destroy(). Bun 1.2.0 delivers 0 bytes in both. Before node fs compat pr #2 #16422 child.stdin was aNativeWritable. It assigned_constructafterWritablehad run, so the stream was constructed from birth.child.stdin.ref()andunref()(functions in Node.js and in Bun 1.1.45,undefinedsince). This PR does not restore them.end(buffer); buffer.fill(0)gives the child zeroes. The stream held the chunk by reference until the construct step.pendingchild.stdin.pendingis true for the whole life of the stream, and 'ready' fires once, one tick after creation. With no 'ready' left,if (stdin.pending) stdin.once("ready", go); else go();would never callgo.pendingis now false for a stream over an open sink until the stream is destroyed. Node.js reports the same values for child.stdin (false, then true after destroy). Path streams, fd streams andnew tty.WriteStream()with no fd keep their values._destroydestroy()can see the flush fail.spawn("true"); stdin.end("hello"); stdin.destroy()with no 'error' listener: non-zero exit in 0 of 10 runs on Node.js and on main, 10 of 10 with the constructor change alone (uncaughtEPIPE: broken pipe, write), 0 of 10 with the guard.sink.end()fails on a stream that was destroyed before 'finish',_destroyreports the error it was given, not the flush error. After 'finish' it still reports the flush error, which is the only report of a failed gracefulend().write(chunk); destroy()to a child that closed its stdin. Node.js emits none there.Inherited options
Object.prototype, so anfd,fsorautoCloseset there reaches everyWriteStream. With the sink in the constructor, an inheritedfdwould replace the sink: the input of the child would go to that fd.fddid not redirect the bytes, but an inheritedfsmadespawn()throw andautoClose: falseleft child.stdin open after 'finish'.What still differs from Node.js after this change
src/io/PipeWriter.rs:944).end("hello"); spawnSync("sleep", ["0.3"])leaves the child with 0 bytes while the parent blocks (Node.js: 5).write("hello")has the same delay on main.end(8192 bytes)followed by the same block is fixed (main 0 bytes, now 8192)._finalof this stream completes synchronously when the sink has no backlog. That is the cause of the call orders below. child_process: end the stdin pipe in child.stdin's _final #45002 changes_final.write()on child.stdin ignorescork()and calls its callback synchronously.child.stdin.ref()andunref()are undefined.end(1 MiB); destroy(); child.unref()with a child that never reads: the sink keeps the backlog and the parent waits for the child (2.2 s withsleep 2, Node.js 19 ms). Main does the same forwrite(1 MiB); destroy()in that tick and forend(1 MiB); destroy()one macrotask later.new tty.WriteStream()with no fd (Node.js:ERR_INVALID_FD) still creates a fast-path stream with nothing to write to. It keeps the construct step. Its 10 measured call shapes are identical to main.Call orders in the creating tick
test/js/node/child_process/fixtures/stdin-call-orders-fixture.jslogs, for 18 call orders, the bytes the child read and the events and callbacks of the stream.noderuns it unchanged. The table test pins the output and lists the rows that differ from Node.js v26.3.0.The rows that differ on this PR:
end(cb); destroy()cb(null), 'finish', 'close'cb(ERR_STREAM_DESTROYED), 'close'end("hello", cb); nextTick(destroy)cb(null), 'finish', 'close'cb(ERR_STREAM_DESTROYED), 'close'end("hello", cb); queueMicrotask(destroy)cb(null), 'finish', 'close'cb(ERR_STREAM_DESTROYED), 'close'write("hello", cb); end(); destroy()cb(null), 'prefinish', 'finish', 'close'cb(null), 'close'cork(); write("hello", cb); destroy()cb(null), 'close'cb(ERR_STREAM_DESTROYED), 'close'On main, with 'open' and 'ready' ignored, the first and fourth rows equal Node.js, and the second has Node's log with 0 bytes delivered. The third and fifth rows differ on main too. Each "this PR" cell is what main already does when the same calls run one macrotask after
spawn().Measurements (linux-x64, Node.js v26.3.0, taken on b92e2f4 and this change on top of it)
spawn()to the firstsetImmediate1 (main 3, node 0). GC cells at creation 18 (main 40) on a debug build. A release build of the first revision gave 13 (main 31).writeFast,underscoreWriteFast,finalFast) in the release bundle: 0 changed lines. Instructions perprocess.stdout.write("x"): 1,020 (main 1,020, run-to-run range 0.2%). Perchild.stdin.write(50 B): 767 (main 767). Per child.stdin creation with the turns after it: about 15,000 (main about 33,000). Counted with a ptrace single-step counter over 200 calls, interleaved runs.perfis not installed on this machine.spawn()+stdin.end("hello"), 200 children:sendto1,socketpair1,fcntl3 (main: the same). Counted with a ptrace counter.straceis not installed on this machine.internal/fs/streams.jsrelease bundle: 17,753 bytes (main 17,505), +248. The binary embeds the bundle: on the first revision the release text grew by the 252 bytes that the bundle grew by (105,076,379 against 105,076,127).end(1 MiB); destroy()in the spawn tick: bytes at a reading child 1,048,576 (main 0, node 146,176). stdin 'error': none (main none, node none). Exit code with no 'error' listener: 0 (main 0, node 0).tty.WriteStream,fs.WriteStream, child stdio, with stdout as a pipe, a file and a pty): 3 of 37 cases differ from main, all child.stdin.new tty.WriteStream()with no fd: 0 of 10 call shapes changed.Tests
child_process.test.tsfail on main. The other 2 (a child that has closed its stdin) pass on main and fail with the constructor change alone: they pin the_destroyguard. "when Object.prototype has stream options" also fails without the null-prototype options. The new test intty.test.tspinspendingof the stream with no fd.child_process.test.ts,child-process-stdio.test.js,child_process-node.test.js,child-process-exec.test.ts,child_process_ipc.test.js,tty.test.ts,node-stream.test.js,process-stdout-write-after-end.test.ts,process-stdio.test.ts,process-stdin.test.ts,fs.test.ts.test-child-process-*,test-stream-construct,test-fs-write-stream*,test-stdout*,test-stdin*,test-tty*,test-process-std*,test-stdio*,test-pipe*): 161 pass on both.child.stdio[3], a stream this PR does not change. The call-order table has 18 rows, not 39, to keep the test under 2 s on a debug build.Other designs
net.Socketsubclass over the same FileSink. It is open with changes requested: its review asks for fd-backed sockets.net.Socketfor stdin needs native work first:Bun.spawnrejects"socket-fd"below index 3 (src/runtime/api/bun/spawn/stdio.rs:419). Any replacement must keeppendingfalse and pass these tests. They assert bytes and state, not the mechanism.new tty.WriteStream()with no fd as Node.js does. Then thetrueplaceholder, its readers and the fast-path branch ofstreamConstructcan go.Related open PRs
_final. It editswritableFromFileSinktoo. If it lands first, itsw._final = finalEndSinkline stays after the constructor call here. Then four rows of the table test equal Node.js and leave thebunlist._construct = undefined..paththat this PR rewords.child.stderrearly for that reason.no test proof · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/node/tty.test.ts, test/js/node/child_process/child_process.test.ts