child_process: make piped stdio streams instances of net.Socket by robobun · Pull Request #36316 · oven-sh/bun

robobun · 2026-07-29T03:53:11Z

What does this PR do?

Node.js wraps each piped child stdio fd in a net.Socket (via lib/internal/child_process.js createSocket), so child.stdin/stdout/stderr are Duplex streams with net.Socket in their prototype chain. Bun was returning a plain fs.WriteStream for stdin and a plain Readable for stdout/stderr:

import { spawn } from 'node:child_process';
import { Socket } from 'node:net';

const cp = spawn('node', [], { stdio: 'pipe' });
console.log(cp.stdout.constructor.name);        // 'Readable'  (Node: 'Socket')
console.log(cp.stdout instanceof Socket);        // false       (Node: true)
console.log(typeof cp.stdin.setEncoding);        // 'undefined' (Node: 'function')
cp.kill();

Packages rely on this:

Nx checks instanceof net.Socket on worker stdio to decide whether to unref() the stream (fix(core): do not throw error if worker.stdout is not instanceof socket nrwl/nx#34224).
python-shell / say call setEncoding on all three stdio streams unconditionally and throw on stdin because it was a plain Writable.

Fix

Wrap stdin/stdout/stderr in thin net.Socket subclasses that call Duplex directly and route reads/writes to the existing FileSink / native readable, so the prototype chain matches Node while the data path is unchanged. constructNativeReadable now accepts an optional base class so child_process can supply the Socket subclass for stdout/stderr. The now-unused writableFromFileSink helper is removed.

This also fixes child.stdin.write(str, encoding), which previously always wrote the string as UTF-8 regardless of the requested encoding because FileSink.write does not take an encoding argument.

After:

stdin  constructor: Socket  instanceof Socket: true  (readable=false writable=true)
stdout constructor: Socket  instanceof Socket: true
stderr constructor: Socket  instanceof Socket: true
typeof stdin.setEncoding === 'function'
typeof stdin.ref / stdin.unref === 'function'

Supersedes #32432 (which made stdin a Duplex but not a net.Socket); #32432 is closed and its remaining assertions (the pause/resume/write/end/destroySoon surface and the signal === null check) are folded into the tests here.

After merging main, streamFdOf() (added by #31829 so that one child's stdin can be passed as another child's stdio target) also reads the fd from the new stdin socket's sink, since the sink no longer lives on fs.WriteStream's fast-path slot. child_process.test.ts "accepts another subprocess's stdin as a stdio target" covers that path.

How did you verify your code works?

Added tests in test/js/node/child_process/child-process-stdio.test.js that assert all three streams are instanceof net.Socket / Duplex with the expected surface, that writes and reads still flow end to end, that stdin.write honours the encoding argument, and that stdin.destroySoon flushes then closes. The tests fail on the current release and pass with this change. Existing child_process tests continue to pass.

[review] gate passed · iteration 1 · 4 files touched

fails on main (without fix)

ASAN without fix: 3 FAILED
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" test/js/node/child_process/child-process-stdio.test.js
bun test v1.4.0 (5299c8ab9)

test/js/node/child_process/child-process-stdio.test.js:
(pass) process.stdout > should allow us to write to it [1130.74ms]
(pass) process.stdin > should allow us to read from stdin in readable mode [1369.83ms]
(pass) process.stdin > should allow us to read from stdin via flowing mode [1351.11ms]
(pass) process.stdin > should allow us to read > 65kb from stdin [1394.03ms]
(pass) process.stdin > should allow us to read from a file [1368.81ms]
(pass) child.stdin > write() after child 'close' returns false and calls back with ERR_STREAM_DESTROYED [168.69ms]
(pass) child.stdin > write() after child 'exit' (before 'close') returns false and calls back with ERR_STREAM_DESTROYED [1356.23ms]
185 |         "instanceof Readable": child[name] instanceof Readable,
186 |         "instanceof Writable": child[name] instanceof Writable,
187 |         "typeof ref": typeof child[name].ref,
188 |         "typeof unref": typeof child[name].unref,
189 |         "typeof setEn
... (truncated)

release without fix: all passed
bun test v1.4.0-canary.1 (c6a39ad6f)

test/js/node/child_process/child-process-stdio.test.js:
(pass) process.stdout > should allow us to write to it [20.66ms]
(pass) process.stdin > should allow us to read from stdin in readable mode [25.03ms]
(pass) process.stdin > should allow us to read from stdin via flowing mode [23.73ms]
(pass) process.stdin > should allow us to read > 65kb from stdin [30.67ms]
(pass) process.stdin > should allow us to read from a file [24.22ms]
(pass) child.stdin > write() after child 'close' returns false and calls back with ERR_STREAM_DESTROYED [2.40ms]
(pass) child.stdin > write() after child 'exit' (before 'close') returns false and calls back with ERR_STREAM_DESTROYED [22.31ms]
(pass) ChildProcess stdio pipe streams are net.Socket > stdin/stdout/stderr pass instanceof and shape checks [2.53ms]
(pass) ChildProcess stdio pipe streams are net.Socket > stdin still delivers writes to the child and stdout still delivers reads [24.28ms]
(pass) ChildProcess stdio pipe streams are net.Socket > stdin.write respects the encoding argument [24.00ms]
(pass) ChildProcess stdio pipe streams are net.Socket > stdin.destroySoon flushes pending writes then 
... (truncated)

passes on PR (with fix)

ASAN with fix: all passed
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" test/js/node/child_process/child-process-stdio.test.js
bun test v1.4.0 (5299c8ab9)

test/js/node/child_process/child-process-stdio.test.js:
(pass) process.stdout > should allow us to write to it [1134.94ms]
(pass) process.stdin > should allow us to read from stdin in readable mode [1379.72ms]
(pass) process.stdin > should allow us to read from stdin via flowing mode [1364.12ms]
(pass) process.stdin > should allow us to read > 65kb from stdin [1405.67ms]
(pass) process.stdin > should allow us to read from a file [1338.98ms]
(pass) child.stdin > write() after child 'close' returns false and calls back with ERR_STREAM_DESTROYED [139.95ms]
(pass) child.stdin > write() after child 'exit' (before 'close') returns false and calls back with ERR_STREAM_DESTROYED [1346.25ms]
(pass) ChildProcess stdio pipe streams are net.Socket > stdin/stdout/stderr pass instanceof and shape checks [152.99ms]
(pass) ChildProcess stdio pipe streams are net.Socket > stdin still delivers writes to the child and stdout still delivers reads [1384.83ms]
(pass) ChildPr
... (truncated)

release with fix: all passed
$ bun scripts/build.ts --profile=release
[configured] bun-profile → bun (stripped) in 684ms (unchanged)
ninja: Entering directory `/workspace/bun/build/release'
[1/21] gen JS modules (bundle-modules)
Preprocess modules (9015ms)
Bundle modules (94ms)
Postprocesss modules (232ms)
Bundle Functions (776ms)
Generate Code (35ms)

[10.17s] Bundled "src/js" for production
  2573 kb
  193 internal modules
  13 native modules
  90 internal functions across 19 files
[1/8] cargo bun_bin → libbun_rust.a (--target x86_64-unknown-linux-gnu)

  nightly-2026-07-20-x86_64-unknown-linux-gnu unchanged - rustc 1.99.0-nightly (9f36de775 2026-07-19)

^[[1m^[[92m   Compiling^[[0m bun_core v0.0.0 (/workspace/bun/src/bun_core)
^[[1m^[[92m   Compiling^[[0m bun_errno v0.0.0 (/workspace/bun/src/errno)
^[[1m^[[92m   Compiling^[[0m bun_ptr v0.0.0 (/workspace/bun/src/ptr)
^[[1m^[[92m   Compiling^[[0m bun_boringssl_sys v0.0.0 (/workspace/bun/src/boringssl_sys)
^[[1m^[[92m   Compiling^[[0m bun_safety v0.0.0 (/workspace/bun/src/safety)
^[[1m^[[92m   Compiling^[[0m bun_zlib_sys v0.0.0 (/workspace/bun/src/zlib_sys)
^[[1m^[[92m   Compiling^[[0m bun_cares_sys v0.0.0 (/workspace/bun/src/cares_sys)
^[[1m^[[92m   Compiling^[[0
... (truncated)

diff hotspot

src/js/internal/fs/streams.ts                      |  12 --
 src/js/internal/streams/native-readable.ts         |  12 +-
 src/js/node/child_process.ts                       | 223 +++++++++++++++++----
 .../node/child_process/child-process-stdio.test.js | 148 ++++++++++++++
 4 files changed, 336 insertions(+), 59 deletions(-)

gate history · 5 passed · 0 rejected · iteration 1

evidence per changed file

file                                                    reads  edits  tests
src/js/internal/fs/streams.ts                               2      1      0
src/js/internal/streams/native-readable.ts                  3      4      0
src/js/node/child_process.ts                                9     10      0
test/js/node/child_process/child-process-stdio.test.js      2      5      0

Node.js wraps each piped child stdio fd in a net.Socket (via lib/internal/child_process.js createSocket). Bun was returning a plain fs.WriteStream for stdin and a plain Readable for stdout/stderr, so `child.stdout instanceof net.Socket` was false and stdin lacked the Duplex surface (setEncoding, pause, resume, ref, unref). Packages like Nx rely on the instanceof check to decide whether a stdio stream can be unref'd. python-shell and say call setEncoding on all three stdio streams unconditionally and threw on stdin. Wrap stdin/stdout/stderr in thin net.Socket subclasses that call Duplex directly and route reads/writes to the existing FileSink / native readable, so the prototype chain matches Node while the data path is unchanged. constructNativeReadable now accepts an optional base class so child_process can supply the Socket subclass. The now-unused writableFromFileSink helper is removed. Also fixes stdin.write(str, encoding) which previously always wrote UTF-8 regardless of the requested encoding. Fixes #26505 Fixes #11011

coderabbitai · 2026-07-29T03:53:18Z

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 21 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: c88ab0ab-5e0c-4a88-b1d7-39b10bceebe9

📥 Commits

Reviewing files that changed from the base of the PR and between f426a8e and e7ce0d1.

📒 Files selected for processing (4)

src/js/internal/fs/streams.ts
src/js/internal/streams/native-readable.ts
src/js/node/child_process.ts
test/js/node/child_process/child-process-stdio.test.js

_{Comment @coderabbitai help to get the list of available commands.}

robobun · 2026-07-29T03:53:22Z

^{Updated 2:31 AM PT - Aug 13th, 2026}

✅ @robobun, your commit e7ce0d11839fd7a9babe6df7a2af4fb9f0ed5bc4 passed in Build #94093! 🎉

🧪 To try this PR locally:

bunx bun-pr 36316

That installs a local version of the PR into your bun-36316 executable, so you can run:

bun-36316 --bun

github-actions · 2026-07-29T03:58:37Z

Found 2 issues this PR may fix:

TODO: stream.Readable stdio and TODO: stream.Writable stdio (node compat) #25498 - Reports TODO: stream.Readable stdio and TODO: stream.Writable stdio errors when passing streams as child_process stdio options; this PR makes stdio streams proper net.Socket instances
child_process.spawn stdio piping between processes fails in Bun with "TODO: stream.Readable stdio" error #30831 - Piping one child's stdout into another child's stdin fails with TODO: stream.Readable stdio; fixed by stdio streams now being recognized net.Socket/Duplex instances

If this is helpful, copy the block below into the PR description to auto-close these issues on merge.

Fixes #25498
Fixes #30831

🤖 Generated with Claude Code

robobun · 2026-07-29T03:59:33Z

This PR does not fix #25498 or #30831. Those are about passing a stream object as an element of the stdio option array (which needs an fd to be extracted from the stream); the stdio wrappers here don't expose one. #30850 is the open PR for that.

github-actions · 2026-07-29T04:02:03Z

This PR may be a duplicate of:

child_process: make child.stdin a Duplex so setEncoding works #32432 - Also makes child_process stdin a proper stream type with encoding support; PR child_process: make piped stdio streams instances of net.Socket #36316 explicitly supersedes it

🤖 Generated with Claude Code

Now that child_process stdout/stderr are net.Socket instances, these need to honor net.Socket's documented contract (ref/unref return the socket). Previously they returned undefined which broke chaining like child.stdout.unref().on('data', ...).

claude

No new issues found — the earlier ref()/unref() return-value nit is fixed in 96d0dab and now covered by the test. Deferring to a human reviewer given this replaces the entire child.stdin write path and puts net.Socket.prototype (with its ~30 methods/getters) into every piped stdio stream's chain; a maintainer should sign off on the subclass-but-skip-Socket-constructor approach.

What was reviewed:

writableFromFileSink removal — confirmed no remaining callers in src/.
_write/_final/_destroy each null the sink before calling sink.end() and settle cb on every path (sync throw, promise reject, no-sink).
Encoding transcode in stdinSocketWrite — decodeStrings: false is set, and the utf8/buffer fast-path skips the Buffer.from copy.
constructNativeReadable Base param: only opt-in from child_process; Readable.fromWeb and other callers are unchanged.

Extended reasoning...

Overview

This PR changes child_process piped stdio from plain fs.WriteStream (stdin) / Readable (stdout/stderr) to thin net.Socket subclasses so instanceof net.Socket and Duplex checks pass, matching Node's createSocket wrapping. It touches four files: src/js/node/child_process.ts (+~170 lines: new StdinSocket/StdoutSocket lazy classes with hand-written _write/_writev/_final/_destroy routing to the existing FileSink), src/js/internal/streams/native-readable.ts (optional Base ctor param; ref/unref now return this), src/js/internal/fs/streams.ts (deletes now-unused writableFromFileSink), and adds four tests.

Security risks

None identified. No parsing of untrusted input, no auth/crypto, no new syscall surface. The change is prototype-chain reshaping around existing data paths.

Level of scrutiny

High. child.stdin/stdout/stderr are touched by essentially every child_process.spawn consumer, and this PR:

Fully replaces the stdin write path — the old WriteStream fast-path (writeFast/underscoreWriteFast) is gone for stdin; writes now flow through standard Writable machinery into a new _write that manually handles encoding transcoding and promise-vs-sync FileSink returns. Backpressure semantics, cork/uncork, and the write-after-destroy contract all shift to a different code path.
Exposes the full net.Socket prototype on stdout/stderr — setTimeout, setNoDelay, setKeepAlive, address(), remoteAddress, bytesRead/bytesWritten getters, connect(), etc. are now all reachable. The subclass skips the net.Socket constructor entirely (calls Duplex directly) and hand-initializes only _handle/_parent/connecting/server/_server; whether that subset is sufficient for every prototype method that reads instance state is a design call a maintainer should confirm.

Other factors

My earlier finding (native-readable ref/unref returning undefined) was fixed in 96d0dab and the test now asserts unref() === this for all three streams.
Verified writableFromFileSink has no remaining callers.
$ERR_SOCKET_CLOSED is a registered error code.
The comment-cop bot flags are all resolved.
Tests exercise instanceof/shape, end-to-end read/write, encoding, and destroySoon, and the PR shows they fail on main and pass with the fix.

Given the breadth of downstream consumers and the architectural choice (Socket subclass that bypasses Socket's own constructor), this warrants a human sign-off rather than bot approval.

FileSink's JS ref counter starts at 1, and spawn's eager stdio ref loop now reaches stdin.ref() (it was a no-op before this branch because WriteStream had no ref). Forwarding every call meant a single user unref() only brought the sink back to its initial state instead of reaching updateRef(false). Track a boolean so ref/unref behave like net.Socket's idempotent flag and the eager ref() is a no-op.

net.Socket's unref()/ref() are documented idempotent; unref();unref();ref() on stdout left the counter at -1->0 without re-refing. Also initialize kStdinUnrefed in the StdinSocket constructor so the first unref() doesn't add a new own property.

claude

I didn't find any new issues after the last round of fixes, but this replaces the class of every piped child.stdin/stdout/stderr with a net.Socket subclass — a wide user-facing surface change that deserves a human look.

What was reviewed:

ref()/unref() semantics for all three streams (return this, idempotent, eager-ref loop interaction with FileSink's m_refCount) — fixed across three iterations.
Inherited net.Socket.prototype methods (read/pause/resume/_read/destroySoon/end) reachable on the new stdio sockets — all fall through safely with _handle = null / connecting = false.
writableFromFileSink removal — no remaining callers.
New stdin _write encoding path and _writev/_final/_destroy error handling — cb called on every branch.

Extended reasoning...

Overview

The PR changes child_process piped stdio streams from a plain fs.WriteStream (stdin) and Readable (stdout/stderr) into thin net.Socket subclasses, so instanceof net.Socket and Duplex succeed and methods like setEncoding/ref/unref/destroySoon are available on all three, matching Node's createSocket wrapper. constructNativeReadable gains an optional Base parameter so stdout/stderr can be constructed as the Socket subclass while keeping the existing native pull path. Stdin gets a fresh _write/_writev/_final/_destroy implementation over FileSink, and writableFromFileSink (the old stdin path) is deleted. ~180 net new lines in child_process.ts, small edits to native-readable.ts and internal/fs/streams.ts, plus 4 new tests.

Security risks

None identified. This is stream-class plumbing in the Node compat layer; no parsing of untrusted input, no auth/crypto/permissions.

Level of scrutiny

High. child_process stdio is one of the most-exercised code paths in the Node compat surface, and this PR changes both the prototype chain (every net.Socket.prototype method — read, pause, resume, setTimeout, setNoDelay, address, _read, _final, _destroy, etc. — is now reachable on these objects) and stdin's entire write path (previously the fs.WriteStream fast path, now a custom Duplex _write with decodeStrings: false and manual encoding handling). The $toClass(…, net.Socket) approach — subclassing for instanceof while calling Duplex directly and setting _handle = null — is a design decision a maintainer should sign off on; the alternative would be to actually construct a net.Socket({ fd }) like Node does.

Other factors

I reviewed this PR across three prior runs and each finding (native-readable ref/unref not returning this; stdin unref() off-by-one against the eager-ref loop; stdout unref() counter going negative; missing kStdinUnrefed field initializer) was fixed in follow-up commits. This run I additionally traced the inherited Socket.prototype.read/pause/resume through drainOnreadTail and confirmed they no-op with _handle = null and kOnreadTail unset, and confirmed writableFromFileSink has no remaining callers. The new tests cover the shape checks, end-to-end write/read, encoding, and destroySoon, but the broader ecosystem impact of swapping the stream class (packages that duck-type on the old shape, or that call other net.Socket methods now newly reachable) is best judged by a human.

robobun · 2026-07-29T10:06:27Z

CI on 5299c8a: 192/196 jobs passed. The 4 remaining are :darwin: 14 test lanes that have been stuck in scheduled for 3h+ (agent availability). The only red annotation is step-failed-outside-runner on the android/freebsd-aarch64/windows-aarch64 build-bun lanes, which is the build agent failing to start, not a compile error. Every test failure listed is marked flaky and passed on retry; none of them touch node:child_process and the new child-process-stdio.test.js assertions pass on every lane that ran.

Ready for review.

Jarred-Sumner

If we're going this approach, can we instead make it so stdout and stderr are file descriptors we then pass to net.Socket?

robobun · 2026-07-29T11:23:53Z

Looked into the fd approach. On POSIX it's clean: stdin/stdout/stderr are already full-duplex socketpair(AF_UNIX, SOCK_STREAM) fds (spawn_process.rs:809-823), so net.connect({ fd }) works with them exactly as it already does for extra stdio at i >= 3. Today the native side blocks it:

stdio.rs:434-437 throws on "socket-fd" at i < 3
spawn_process.rs:894-897 marks SocketFd unreachable in the 0..3 loop
Writable.rs:289,393 / Readable.rs:176 mark SocketFd unreachable
subprocess.rs:885-889 hardcodes handle.stdio[0..3] to null

Lifting those and having child_process map "pipe" → "socket-fd" for 0-2 on POSIX would let all three streams be real net.Sockets via net.connect({ fd }), same code path as extra stdio. That also makes child.stdout.writable === true match Node (it's a duplex socketpair there too).

Windows is the catch: us_socket_from_fd returns 0 there (socket.c:435) and the i >= 3 path already guards on process.platform !== "win32". Windows stdio is a libuv named-pipe handle, not a raw fd. So on Windows the subclass-over-FileSink/native-readable wrapper from this PR would still be needed for instanceof net.Socket to hold.

Want me to go ahead with: POSIX uses real fd-backed net.Socket for all three, Windows keeps the wrapper subclass? Or hold off on Windows for now?

…child-process-stdio-socket

…n stdin shape assertions from #32432

…lper

github-actions · 2026-08-13T04:16:17Z

+    // Internal $fastPath callers discard .path; do not resolve it -
+    // path.resolve("") needs process.cwd(), which throws when the cwd has
+    // been deleted (Node still spawns children in that state).


If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code

claude · 2026-08-13T04:50:17Z

 function ref(this: NativeReadable) {
  const ptr = this.$bunNativePtr;
-  if (ptr === undefined) return;
-  if (this[kRefCount]++ === 0) {
+  if (ptr !== undefined && this[kRefCount]++ === 0) {
    ptr.updateRef(true);
  }
+  return this;
 }


🟡 child.stdout.ref() is not idempotent: 5299c8a clamped unref() at 0 but ref() still does this[kRefCount]++ uncapped, so after spawn's eager ref (kRefCount=1) a redundant ref() bumps it to 2 and a subsequent unref() only brings it back to 1 — updateRef(false) never fires. Node documents socket.ref() as idempotent, and child.stdin already uses the fully-idempotent kStdinUnrefed flag, so the three stdio streams disagree. Cap ref() symmetrically (treat kRefCount as a 0/1 flag) or route stdout/stderr through the same flag path stdin uses.

Extended reasoning...

What the bug is

net.Socket.prototype.ref() is documented as idempotent: "If the socket is refed calling ref again will have no effect." Commit 5299c8a in this PR fixed the mirror case for unref() by clamping the decrement at 0 (this[kRefCount] > 0 && --this[kRefCount] === 0), but left ref() at native-readable.ts:263-269 unchanged:

function ref(this: NativeReadable) { const ptr = this.$bunNativePtr; if (ptr !== undefined && this[kRefCount]++ === 0) { ptr.updateRef(true); } return this; }

The post-increment has no upper cap, so a redundant ref() on an already-refed stream silently drifts the counter above 1, and the next unref() fails to reach the 1→0 transition that fires updateRef(false).

Code path / step-by-step proof

For a live child.stdout pipe, constructNativeReadable installs the instance-level stream.ref = ref; stream.unref = unref; (native-readable.ts:82-83), which shadow StdoutSocket.prototype.ref/unref (the idempotent stdioSocketRef/Unref). Then:

ChildProcess#spawn's eager loop for (let item of this.stdio) item?.ref?.(); calls native-readable's ref() → kRefCount 0→1 (post-increment returns 0, so updateRef(true) fires). ✓

User (redundantly) calls child.stdout.ref() → kRefCount++ post-increment returns 1 (≠ 0), no updateRef; kRefCount = 2.

User calls child.stdout.unref() → kRefCount > 0 && --kRefCount === 0 → 2→1, 1 ≠ 0, no updateRef(false).

Result: after ref(); unref() the pipe stays refed. In Node the same sequence leaves the socket unrefed.

Why existing code doesn't prevent it

The 5299c8a fix only touched the decrement side. Nothing caps the increment. And the fully-idempotent StdoutSocket.prototype.ref/unref (which use the kStdinUnrefed flag) are unreachable for live pipes because constructNativeReadable installs own-property ref/unref that shadow them — the comment in getStdoutSocket() ("Live pipes get instance _read/_destroy/ref/unref from constructNativeReadable") acknowledges this.

Internal inconsistency

Within this PR's own diff, the three stdio streams now disagree on ref/unref semantics: child.stdin uses the kStdinUnrefed boolean flag (fully idempotent both ways), while live child.stdout/stderr use a counter that's clamped only on decrement. REVIEW.md's "fix the whole class in the same PR" applies — this is the exact same pattern as the already-fixed unref() side, just on the increment.

Repro

const { spawn } = require('child_process'); const c = spawn(process.execPath, ['-e', 'setTimeout(()=>{},1e6)'], { stdio: 'pipe' }); c.stdin.ref(); c.stdin.unref(); // stdin: unrefed (flag path) c.stdout.ref(); c.stdout.unref(); // stdout: still refed (counter went 1→2→1)

Impact / severity

Nit. Triggering it requires a redundant ref() call on a stream that's already refed, which is uncommon (the motivating Nx use case only calls unref()), and pre-PR child.stdout had no ref/unref contract at all — so this is a gap in newly-added surface, not a regression. But it's a real Node-compat divergence on a stream this PR now advertises as instanceof net.Socket, and the asymmetry with stdin invites confusion.

Fix

Cap ref() symmetrically so kRefCount is effectively a 0/1 flag:

function ref(this: NativeReadable) { const ptr = this.$bunNativePtr; if (ptr !== undefined && this[kRefCount] === 0) { this[kRefCount] = 1; ptr.updateRef(true); } return this; }

Or drop the instance-level ref/unref install when Base is supplied and let StdoutSocket.prototype.ref/unref (the kStdinUnrefed flag path) handle it, so all three stdio streams share one idempotent implementation.

claude · 2026-08-13T04:50:17Z

+    // Internal $fastPath callers discard .path; do not resolve it -
+    // path.resolve("") needs process.cwd(), which throws when the cwd has
+    // been deleted (Node still spawns children in that state).
    this.path = fastPath ? path : getValidatedPath(path);


🟡 Removing writableFromFileSink leaves no $fastPath: true caller that can reach the fd == null branch — the two remaining callers (ProcessObjectInternals.ts:55, tty.ts:116) both pass an fd. The fastPath ? path : getValidatedPath(path) ternary and its 3-line comment are now dead; per REVIEW.md "delete dead code in the same PR that makes it dead", drop them rather than rewording the comment (which also resolves the open comment-cop finding on line 389).

Extended reasoning...

What the finding is

This PR removes writableFromFileSink, which was the only caller that constructed a WriteStream with $fastPath: true and no fd:

// removed by this PR const w = new WriteStream("", { $fastPath: true }); // no fd → takes the fd == null branch

The two remaining $fastPath callers both pass an fd:

src/js/builtins/ProcessObjectInternals.ts:55 — new fs.WriteStream(null, { autoClose: false, fd, $fastPath: true })

src/js/node/tty.ts:116 — fs.WriteStream.$call(this, null, { fd, $fastPath: true, autoClose: false })

Both take the typeof options.fd === "number" branch at streams.ts:397, never the fd == null branch. $fastPath is a builtin-private name (registered in BunBuiltinNames.h), so user code cannot set it on an options object. That leaves the fastPath value inside the fd == null branch always undefined.

The specific dead code

if (fd == null) { this[kFs] = customFs || fs; this.fd = null; // Internal $fastPath callers discard .path; do not resolve it - // path.resolve("") needs process.cwd(), which throws when the cwd has // been deleted (Node still spawns children in that state). this.path = fastPath ? path : getValidatedPath(path); ...

The ternary at line 390 now always evaluates to getValidatedPath(path), and the 3-line comment (which this PR just reworded to drop the writableFromFileSink mention) describes a "child_process spawn from deleted cwd" scenario that no longer routes through this line. This PR touched exactly these lines to update the comment rather than delete it.

Note that fastPath is still live in the later if (fastPath) { this[kWriteStreamFastPath] = ... } block further down in WriteStream(), which the fd-bearing callers do reach — so only this ternary + comment are dead, not the whole $fastPath mechanism.

Step-by-step proof

Grep confirms exactly two $fastPath sites remain in src/js/ after this PR: ProcessObjectInternals.ts:55 and tty.ts:116.

ProcessObjectInternals.ts:55 is the process.stdout/stderr constructor; fd is asserted to be 1 or 2 before the call.

tty.ts:116 is tty.WriteStream(fd); fd is the user's argument. If a user passed undefined, the code would reach the fd == null branch with fastPath truthy — but that's a degenerate user error Node rejects with ERR_INVALID_FD, and removing the ternary would just make Bun throw at construction (ERR_INVALID_ARG_TYPE for path=null) instead of deferring the failure to write time — arguably an improvement, and certainly not a case the comment's "spawn from deleted cwd" rationale covers.

$fastPath compiles to a private symbol, so no external options object can set it.

Therefore, on every path that reaches fd == null in WriteStream(), fastPath is undefined, and fastPath ? path : getValidatedPath(path) reduces to getValidatedPath(path).

Why it should change in this PR

REVIEW.md, Code style & idioms: "Delete dead code in the same PR that makes it dead." This PR's own diff (the writableFromFileSink removal) is what kills this branch, and the PR already touched these exact lines to reword the comment — so it's in scope.

This is also the root-cause fix for the still-open comment-cop finding on streams.ts:389 ("If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong"): the workaround the comment justifies has no caller, so the right fix is deletion, not shortening.

Impact

Zero functional impact — the ternary's dead arm is never taken, so behavior is unchanged. This is a nit: harmless but directly created by (and touched in) this PR.

Fix

Replace lines 387–390 with:

this.path = getValidatedPath(path);

…#43879) Fixes #43875 ### Problem - `child.stdin.end(cb)` with no data fires `cb` and `'finish'` on the next tick while earlier writes still wait behind pipe backpressure. Node waits for the drain. A parent that exits in `cb` truncates the child's input. - `child.stdin` is a fast-path `fs.WriteStream` (`src/js/internal/fs/streams.ts:474`). `writeFast` hands chunks to the native `FileSink` and never updates `_writableState`, so `end()` sees nothing pending. #28635 fixed this for `process.stdout` only, with a per-instance `_final`. ### Fix - Install `finalFast` as `_final` where the constructor installs the fast path. It awaits `fileSink.flush()`, which returns the backlog's promise when writes are pending. - Delete the copy in `getStdioWriteStream`. `process.stdout` and `process.stderr` now get it from the constructor. - Suppress `'drain'` from the fast-path sites once the stream is ending or destroyed, as `Writable`'s `afterWrite` does. The stream now stays alive while the backlog drains, which made that late `'drain'` visible. - Verified: `test/js/node/child_process/child_process.test.ts` and `test/js/node/tty.test.ts` (one new case each, both fail on 1.4.3). Also `test/regression/issue/25432.test.ts`, the `process` and `child_process` suites, and the stdio cases in `test/js/node/test/parallel`. - Self-reviewed: 1 concern raised, 1 addressed (the tty test held the backlog with a `sleep`. It now gates the read on an IPC message). ### Background - The fast path (`$fastPath: true`) has three callers: `child.stdin`, `process.stdout`/`stderr`, and `tty.WriteStream`. It replaces `write` with `writeFast`, which skips the `Writable` bookkeeping. `Writable` emits `'finish'` when `_final(cb)` calls back, or at once when there is no `_final`. - Considered a `_final` on `child.stdin` alone: it repeats #28635 and leaves `tty.WriteStream` broken. Considered routing `writeFast` through `Writable.prototype.write`: it adds bookkeeping to every `process.stdout.write`. ### Downsides - `child.stdin` and `tty.WriteStream` gain one own property: 16 to 17 and 15 to 16 own keys (`Object.getOwnPropertyNames`, 1.4.3 vs this build). - Each `end()` on those streams makes one `flush()` host call. On an empty buffer that is 0 syscalls (`IOWriter::flush` returns `Wrote(0)`, `src/io/PipeWriter.rs:969`). `strace` is not installed here, so the count is from the code. - Code that relied on `'finish'` from `child.stdin` firing before the pipe drained now sees it later. - On Windows a single in-flight `uv_write` is not reported by `flush()` (`src/io/PipeWriter.rs:2099`), so one unsent chunk can still be lost when `cb` exits. This PR fixes the multi-chunk backlog. The single-chunk case is pre-existing and needs a change in `FileSink`. <details><summary>Notes</summary> Ordering on the repro (3 untracked writes, `write(chunk, cb)`, `'finish'` listener, `end(cb)`): ``` bun 1.4.3: end(cb) 1ms, 'finish' 1ms, write(cb) 243ms this PR: write(cb), end(cb), 'finish' (same order as node) ``` `FileSink.flush()` (`src/runtime/webcore/FileSink.rs:983`) returns the pending write promise when one exists, and a number when the buffer is empty. Open PR #36316 makes `child.stdin` a `net.Socket` subclass whose `_write` goes through `Duplex`, which would also fix this for `child.stdin`. It is a larger parity change and leaves `tty.WriteStream` on the fast path. This PR does not depend on it. Related issues with the same shape in other layers: #43155 (`http.ServerResponse`, fix in #41822) and #43874 (`TLSSocket` over a `Duplex`). `bun x tsc --noEmit -p src/js/tsconfig.json` reports the same 385 pre-existing errors with and without this change. Two tests in the `child_process` suite fail on main in this container without the change: "should allow us to spawn in the default shell" and "extra stdio pipes are not double-closed on GC". </details>  --- **no test proof** · iteration 1 · 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

robobun assigned Jarred-Sumner Jul 29, 2026

This was referenced Jul 29, 2026

Child processes spawned with node:child_process and piped stdout have incorrect type data #26505

Open

child_process: make child.stdin a Duplex so setEncoding works #32432

Closed

github-actions Bot added the claude label Jul 29, 2026

claude Bot reviewed Jul 29, 2026

View reviewed changes

Comment thread src/js/internal/streams/native-readable.ts