Skip to content

child_process: support piping subprocess stdio streams between spawns#30850

Open
robobun wants to merge 19 commits into
mainfrom
farm/e310776e/spawn-stdio-stream-forwarding
Open

child_process: support piping subprocess stdio streams between spawns#30850
robobun wants to merge 19 commits into
mainfrom
farm/e310776e/spawn-stdio-stream-forwarding

Conversation

@robobun

@robobun robobun commented May 15, 2026

Copy link
Copy Markdown
Collaborator

Repro

const { spawn } = require('child_process');

const p2 = spawn('perl', ['-ne', 'print uc'], {
  stdio: ['pipe', process.stdout, process.stderr],
});

const p1 = spawn('node', ['-p', '"hello world"'], {
  stdio: ['ignore', p2.stdin, process.stderr],
});

p1.on('exit', () => { p2.stdin.end(); });
  • Node: prints HELLO WORLD.
  • Bun (before): error: TODO: stream.Readable stdio @ 1.

Also broken in reverse: spawn(..., { stdio: [p1.stdout, 'pipe', 'inherit'] }).

Cause

nodeToBun() in node:child_process looks for .fd or ._handle.fd on stream stdio. Node's subprocess.stdin is a net.Socket that exposes its pipe fd on ._handle.fd; Bun's is a WriteStream wrapping a FileSink, and .stdout/.stderr is a native Readable wrapping a ReadableStreamSource — neither surfaced a fd where nodeToBun looks. Just forwarding the fd isn't enough either — two more problems follow (see test/regression/issue/30831.test.ts for the full reasoning).

Fix

Three changes, all required — removing any one breaks one of the three cases in the test file:

  1. Expose .fd on the wrappers. writableFromFileSink now reads fileSink._getFd(); constructNativeReadable calls a new getFd() method on FileInternalReadableStreamSource (returns reader.get_fd()). nodeToBun's existing .fd / ._handle.fd lookup then finds it.
  2. Quiesce the source stream. When nodeToBun extracts an fd from a stream, it also pauses the stream (setFlowing(false) + updateRef(false) on the native ptr, .pause() on the JS Readable) and tags it kIsUsedAsStdio. #handleOnExit skips stdout.resume?.() on tagged streams. Mirrors Node's getValidStdio post-spawn hook (readStop() + pause() + kIsUsedAsStdio in lib/internal/child_process.js) — without it the parent's own PipeReader races the child for the pipe's bytes.
  3. Clear O_NONBLOCK before dup2. Bun sets pipe fds non-blocking in the parent for its own async reads; dup2 shares the open file description so the child inherits non-blocking mode. A synchronous reader like cat sees EAGAIN on its first read(2) and exits 1 with 'Resource temporarily unavailable'. PosixStdio::Pipe(fd) now clears O_NONBLOCK on caller-supplied fds before dup2; safe because step (2) has paused the parent-side reader.

Verification

test/regression/issue/30831.test.ts — three tests covering forward piping (p2.stdin as p1's stdout slot), reverse (p1.stdout as p2's stdin slot), and a regression-guard for process.stdout/process.stderr passthrough.

Fixes #30831
Fixes #25498

@robobun

robobun commented May 15, 2026

Copy link
Copy Markdown
Collaborator Author
Updated 11:40 PM PT - Jun 20th, 2026

@robobun, your commit 4ddde40 has 3 failures in Build #63723 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 30850

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

bun-30850 --bun

@coderabbitai

coderabbitai Bot commented May 15, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

Walkthrough

Expose readable/writable stream fds to JS and forward them into child stdio with POSIX prep and JS-side quiescing; add regression tests for multiple piping scenarios; and JSON-stringify the built OVERLAY_CSS define to avoid lexer issues on older Bun versions.

Changes

Stream fd exposure and child_process stdio integration

Layer / File(s) Summary
Rust readable stream fd extraction contract
src/runtime/webcore/ReadableStream.rs, src/runtime/api/streams.classes.ts, src/runtime/webcore/FileReader.rs
SourceContext trait gains get_fd() returning underlying file descriptor or -1; NewSource exposes host binding get_fd_from_js; FileReader implements POSIX and Windows fd exposure.
Native and JS stream fd exposure
src/js/internal/streams/native-readable.ts, src/js/internal/fs/streams.ts
NativePtr.getFd() method signature; constructNativeReadable calls getFd() and assigns result to stream.fd; writableFromFileSink extracts _getFd() and populates stream.fd.
System-level fd preparation for child inheritance
src/spawn_sys/spawn_process.rs
Reformatted platform-conditional attributes; POSIX Pipe handler explicitly clears non-blocking mode on caller-supplied fd before dup2 to ensure child inherits blocking semantics.
Child process stdio integration and stream marking
src/js/node/child_process.ts
kIsUsedAsStdio marker identifies transferred streams; nodeToBun extracts stream fds via extractStreamFd; getBunStdioFromOptions returns { bunStdio, streamsToQuiesce }; spawnSync and spawn post-spawn call markStreamsAsStdio to quiesce; handleOnExit destroys marked streams instead of resuming them.
Regression tests for stdio piping scenarios
test/regression/issue/30831.test.ts
Forward/reverse piping between source and filter processes; process.stdout/stderr forwarding through nested runner spawn; spawn-failure path ensuring sources remain consumable; includes inline filter script and collect() helper.

CSS build serialization fix

Layer / File(s) Summary
CSS build JSON serialization fix
src/codegen/bake-codegen.ts, src/parsers/json_lexer.rs, test/bundler/bun-build-api.test.ts
OVERLAY_CSS define wrapped in JSON.stringify(...) to pre-quote minified CSS; adjacent parser and test comments updated to reflect the change and rationale.
  • Possibly related PRs:

    • oven-sh/bun#30679: Addresses the same define auto-quote lexer failure triggered by minified CSS starting with *{...}.
  • Suggested reviewers:

    • RiskyMH
    • dylan-conway
🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed Title clearly and concisely summarizes the main change: enabling subprocess stdio stream piping between spawns.
Description check ✅ Passed Description is comprehensive with repro, cause, detailed fix explanation, and verification; exceeds template requirements.
Linked Issues check ✅ Passed All code changes fully satisfy the requirements from #30831 and #25498: fd exposure, stream quiescing, blocking mode fixes, and comprehensive regression tests.
Out of Scope Changes check ✅ Passed All changes are directly scoped to the stdio piping feature; documentation updates and test comments support the core implementation.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions

Copy link
Copy Markdown
Contributor

Found 1 issue this PR may fix:

  1. TODO: stream.Readable stdio and TODO: stream.Writable stdio (node compat) #25498 - Directly reports the TODO: stream.Readable stdio and TODO: stream.Writable stdio errors that this PR resolves by exposing .fd on stream wrappers and implementing fd hand-off in nodeToBun()

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

Fixes #25498

🤖 Generated with Claude Code

@robobun robobun force-pushed the farm/e310776e/spawn-stdio-stream-forwarding branch from 17d22b5 to 452d9c4 Compare May 15, 2026 17:42
Comment thread src/js/node/child_process.ts
Comment thread src/js/internal/fs/streams.ts
Comment thread src/spawn_sys/spawn_process.rs Outdated
Comment thread src/js/node/child_process.ts
Comment thread test/regression/issue/30831.test.ts Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/js/node/child_process.ts`:
- Around line 515-516: The call to markStreamsAsStdio is happening after
Bun.spawnSync returns, so it cannot prevent parent-side readers racing the
child; move the quiescing step to before the synchronous spawn. Specifically, in
the spawnSync flow that uses getBunStdioFromOptions (referencing bunStdio and
streamsToQuiesce) call markStreamsAsStdio(streamsToQuiesce) (and perform any
necessary quiesce/forwarding setup) before invoking Bun.spawnSync(...) so
parent-side stream readers are marked/stopped while the child runs; apply the
same change to the parallel block around the 553-556 region.

In `@test/regression/issue/30831.test.ts`:
- Around line 105-118: The test never asserts that the source process (pSource)
exited successfully, so add an explicit exit-code capture and assertion for
pSource similar to how pFilter's exit is captured: create a Promise for
sourceExit that resolves to pSource.exitCode or waits for the "exit" event,
include that Promise in the Promise.all alongside
filterStdout/filterStderr/sourceStderr, then assert sourceExit === 0 (place this
assertion after asserting stdout/stderr and before asserting the filter exit) in
both the reverse-path and failure-path tests that reference
pSource/pFilter/out/filterErr/sourceErr/filterExit so the source process exit is
verified and exit-code assertions remain last.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 9e89d03f-c765-4fc8-81e1-fc1601b7579e

📥 Commits

Reviewing files that changed from the base of the PR and between bb1973e and 86ef21d.

📒 Files selected for processing (18)
  • src/codegen/bake-codegen.ts
  • src/crash_handler/lib.rs
  • src/errno/lib.rs
  • src/js/internal/fs/streams.ts
  • src/js/internal/streams/native-readable.ts
  • src/js/node/child_process.ts
  • src/perf/tracy.rs
  • src/runtime/api/streams.classes.ts
  • src/runtime/cli/Arguments.rs
  • src/runtime/cli/run_command.rs
  • src/runtime/cli/upgrade_command.rs
  • src/runtime/jsc_hooks.rs
  • src/runtime/webcore/FileReader.rs
  • src/runtime/webcore/ReadableStream.rs
  • src/runtime/webview/ChromeProcess.rs
  • src/spawn/process.rs
  • src/spawn_sys/spawn_process.rs
  • test/regression/issue/30831.test.ts

Comment thread src/js/node/child_process.ts Outdated
Comment thread test/regression/issue/30831.test.ts Outdated

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additional findings (outside current diff — PR may have been updated during review):

  • 🔴 src/js/node/child_process.ts:1091-1102 — When p1.stdout is handed to another spawn, p1 never emits 'close': accessing p1.stdout bumped #closesNeeded and registered pipe.once('close', …), but markStreamAsStdio's setFlowing(false) unregisters the FilePoll and this guard now skips the resume() that would have re-armed it — the NativeReadable never sees EOF, never emits 'close', and #closesGot stalls one short of #closesNeeded. The reverse-direction regression test only awaits 'exit'/exitCode so this isn't caught. When skipping resume() for a kIsUsedAsStdio stream, also .destroy() it (or otherwise call #maybeClose() for that slot) so the source process's 'close' event can fire.

    Extended reasoning...

    What the bug is

    The source ChildProcess whose stdout/stderr is passed to another spawn's stdio will never emit its 'close' event. 'exit' fires correctly, but 'close' is gated on #closesGot === #closesNeeded, and the handed-off stream's slot in that count is never satisfied.

    This is the success-path counterpart to the already-filed 🔴 comment on markStreamAsStdio (which covers the spawn-failure path leaving the stream irreversibly paused). Here both spawns succeed, the pipeline works, both children exit — but p1.on('close', cb) never fires.

    The code path

    1. #closesNeeded is bumped. Accessing p1.stdout runs #getBunSpawnIo(1), which does this.#closesNeeded++ and pipe.once('close', () => this.#maybeClose()) (child_process.ts:1206-1208). p1 now needs that NativeReadable to emit 'close' before it can emit its own.

    2. The native poll is unregistered. Passing p1.stdout to spawn(..., {stdio:[p1.stdout,...]}) triggers nodeToBun → markStreamAsStdio, which calls ptr.setFlowing(false). FileReader::set_flowing(false) (FileReader.rs:1040) calls self.reader().pause(), and PosixBufferedReader::pause (PipeReader.rs:301-312) sets IS_PAUSED and unregisters the FilePoll (poll.unregister(loop, false)). On Windows the equivalent calls uv_read_stop. The parent's read end of the pipe is now off the event loop.

    3. Nothing re-arms it. When p1 exits, #handleOnExit runs. The new !stdout[kIsUsedAsStdio] guard (correctly) skips stdout.resume?.() so the parent doesn't steal bytes from the child. But that resume() was also the only thing that would have driven the stream toward EOF after exit. Even if it did run, JS-side resume()_readptr.pullFileReader::on_pull would see flowing == false and return Pending without issuing a read(2), so it wouldn't help anyway — setFlowing(false) is sticky at the native layer.

    4. No EOF, no 'close'. With the poll unregistered, the parent never receives a readable/HUP notification when p1's write end closes, so on_reader_done never runs and the NativeReadable never push(null)s. Nothing destroys it either. It never emits 'end' or 'close'.

    5. 'close' on p1 never fires. #closesGot reaches #closesNeeded - 1 (the base #maybeClose() from #handleOnExit plus any other stdio that did close) and stops. p1.emit('close', exitCode, signalCode) is never reached.

    Why nothing else catches it

    • Subprocess.on_process_exit's "kick a final read" fallback (if let Readable::Pipe(pipe) = self.stdout.get() { reader.read() }) doesn't fire: once p1.stdout was accessed, Readable::to_js replaced the native Subprocess.stdout slot with Readable::Closed, so the match arm doesn't apply.
    • updateRef(false) in markStreamAsStdio means the stuck stream doesn't keep the event loop alive, so a script that only awaits 'close' will exit without the callback firing rather than hang forever — but the callback still never runs.
    • The PR's reverse-direction test (stdio: [pSource.stdout!, ...]) exercises exactly this topology but only awaits 'exit' and exitCode, never 'close', so it passes.

    Step-by-step proof

    const p1 = spawn(bunExe(), ['-e', 'process.stdout.write("hi")'], { stdio: ['ignore','pipe','pipe'] });
    // p1.stdout getter → #getBunSpawnIo(1):
    //   #closesNeeded = 2; pipe.once('close', () => #maybeClose())
    const p2 = spawn(bunExe(), ['-e', '...'], { stdio: [p1.stdout, 'pipe', 'pipe'] });
    // nodeToBun(p1.stdout, 0) → markStreamAsStdio:
    //   p1.stdout[kIsUsedAsStdio] = true
    //   ptr.setFlowing(false) → reader().pause() → FilePoll unregistered
    //   ptr.updateRef(false); p1.stdout.pause()
    
    p1.on('close', () => console.log('close'));   // ← never fires
    
    // p1 exits → onExit → #handleOnExit:
    //   stdout is defined, kIsUsedAsStdio=true → resume() SKIPPED
    //   emit('exit', 0, null)
    //   #maybeClose(): #closesGot=1, #closesNeeded=2 → no 'close'
    // p1.stdout's poll is unregistered; never sees EOF; never emits 'close'
    // #closesGot stuck at 1.

    (In the actual regression test #closesNeeded is 3 because pSource.stderr is also accessed; stderr does close normally, leaving the count stuck at 2/3.)

    Comparison with Node

    Node's flushStdio also skips kIsUsedAsStdio streams, and readStop() similarly stops libuv reads. The difference is that Node's net.Socket._read calls handle.readStart() on demand, so the pause is recoverable, and Node tracks stdio close via the handle's 'close' rather than the stream's flowing state. In Bun, setFlowing(false) is sticky (no user-land path flips it back) and the FilePoll is fully removed from the loop, so there is no recovery path at all.

    How to fix

    In #handleOnExit, when the kIsUsedAsStdio guard suppresses resume(), the stream's role in close-accounting still needs to be satisfied. The simplest option is to destroy the wrapper at that point:

    } else if (stdout && this.#stdioOptions[1] === "pipe" && !stdout?.destroyed) {
      if (stdout[kIsUsedAsStdio]) {
        // fd was handed to another child; we'll never read it again. Destroy so
        // 'close' fires and #maybeClose can complete.
        stdout.destroy?.();
      } else {
        stdout.resume?.();
      }
    }

    (and the same for stderr). Alternatively, call this.#maybeClose() directly for the handed-off slot, or have markStreamAsStdio arrange for the source ChildProcess to decrement #closesNeeded — but destroy() here is closest to the existing structure and also releases the parent's now-useless read fd.

Comment thread src/runtime/webcore/FileReader.rs
@robobun

robobun commented May 15, 2026

Copy link
Copy Markdown
Collaborator Author

For the close event bug from the claude[bot] review above — fixed in ed21ede. #handleOnExit now calls stream.destroy() on kIsUsedAsStdio streams instead of skipping both resume and destroy, so close fires on the next tick and the source ChildProcess's #closesGot / #closesNeeded accounting completes. Extended the reverse-direction regression test to await close (with a 5s timeout guard).

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@test/regression/issue/30831.test.ts`:
- Around line 132-135: The test currently uses a timer-based guard via
Promise.race([... setTimeout(...)]) around sourceClose; remove the timing-based
logic and await the close condition directly by replacing the Promise.race block
with a direct await of sourceClose (i.e., await sourceClose) so the test waits
for the close event instead of relying on setTimeout and the "pSource 'close'
never fired" error.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: e17bbef9-51f4-48f0-853a-ebbe0070d212

📥 Commits

Reviewing files that changed from the base of the PR and between ed21ede and 47f2b68.

📒 Files selected for processing (2)
  • src/runtime/webcore/FileReader.rs
  • test/regression/issue/30831.test.ts

Comment thread test/regression/issue/30831.test.ts Outdated
Comment thread test/regression/issue/30831.test.ts Outdated
@robobun

robobun commented May 15, 2026

Copy link
Copy Markdown
Collaborator Author

CI flaking on different single shards each run — a single 1/20 shard of :ubuntu: 25.04 aarch64 - test-bun on 86ef21d (all other aarch64 lanes green, ASAN green), then a single 1/2 shard of :darwin: 14 x64 - test-bun on 2da8234 (aarch64 now green). The diff touches POSIX subprocess-stdio handling and cannot differentially affect both of those lanes while leaving the other 20+ platforms green; same platform two runs in a row would be actionable, different one each time is infrastructure noise. Needs a maintainer to re-trigger or merge.

@robobun robobun force-pushed the farm/e310776e/spawn-stdio-stream-forwarding branch from 2da8234 to 0d72f4f Compare May 22, 2026 07:46
Comment thread src/codegen/bake-codegen.ts
robobun and others added 10 commits June 21, 2026 02:02
spawn({ stdio: [..., proc.stdin, ...] }) and spawn({ stdio: [proc.stdout, ...] })
both threw `TODO: stream.Readable/Writable stdio @ N`. Node's `subprocess.stdin`
is a `net.Socket` that exposes its pipe fd via `._handle.fd`; Bun's
`subprocess.stdin` is a WriteStream wrapping a FileSink, and `.stdout`/`.stderr`
is a native Readable wrapping a ReadableStreamSource — neither surfaced a fd
where `nodeToBun()` looks.

Three changes, all required — removing any one breaks a real shape:

1. Expose fd on the wrappers
   - `writableFromFileSink` in internal/fs/streams.ts calls
     `fileSink._getFd()` and sets `.fd` on the returned WriteStream.
   - `constructNativeReadable` in internal/streams/native-readable.ts calls
     `bunNativePtr.getFd()` (new method added to
     FileInternalReadableStreamSource) and sets `.fd` on the returned
     Readable.
   Now `nodeToBun`'s existing `.fd` / `._handle.fd` lookup returns a fd
   for both shapes.

2. Quiesce the source stream when its fd is handed off
   `nodeToBun` now also marks the stream with a `kIsUsedAsStdio` symbol,
   calls the native ptr's `setFlowing(false)` / `updateRef(false)`
   (libuv-`readStop` equivalent), and `.pause()`s the JS Readable.
   `#handleOnExit`'s drain logic skips `stdout.resume?.()` on
   `kIsUsedAsStdio` streams. This mirrors Node's `getValidStdio`
   post-spawn hook in `lib/internal/child_process.js` — without it our
   parent-side PipeReader races the child for the pipe's bytes and the
   child usually loses.

3. Clear `O_NONBLOCK` before dup2
   Bun's `Bun.spawn` sets parent-side pipe fds non-blocking for its own
   async reads. `dup2` shares the open file description so the child
   inherits `O_NONBLOCK` and its first `read(2)` returns `EAGAIN` —
   `cat` exits 1 with "Resource temporarily unavailable". For
   `PosixStdio::Pipe(fd)` (caller-supplied fd), clear O_NONBLOCK before
   dup2; the parent's own reader has been paused by (2) so it won't need
   non-blocking anymore.

Fixes #30831
The diff-hygiene check flags any 'TODO' in added lines. The three hits were
prose describing the old error message, not markers of undone work — rewrite
them so the hygiene check is clean. No behavior change.
`OVERLAY_CSS` is a minified CSS string that starts with `*{...}`. The JSON
lexer rejects `*` / `?` / `(` / `)` before `parse_env_json`'s auto-quote
fallback runs, so any bun without the #30679 lexer recovery fix fails at
bake-codegen when building bun itself:

    error: Unsupported syntax: Operators are not allowed in JSON
      at defines.json:1:1

The real fix is already on main (lexer recovery), but build pipelines using
an older release bun to bootstrap can't bundle bake until the release bun is
re-rolled. Pre-stringifying here is cheap, forward-compatible (the new lexer
accepts a quoted JSON string unchanged), and unblocks those build lanes.
Fixes a correctness issue raised in review:
`markStreamAsStdio` ran inline in `nodeToBun` during option normalization,
\*before\* `Bun.spawn` runs. If spawn then threw
(ENOENT/EACCES/EMFILE/ENFILE/EAGAIN, or any other error), the source
stream's `$bunNativePtr.setFlowing(false)` had already been applied — and
that call is sticky at the native layer with no user-recoverable
counterpart. The user would be left with a paused subprocess stdout
that no public API can restart.

Refactor:
- `nodeToBun` now takes a `streamsToQuiesce` out-array and records the
  stream instead of pausing it inline.
- `getBunStdioFromOptions` returns `{ bunStdio, streamsToQuiesce }`.
- Both `spawn` (async, in `ChildProcess#spawn`) and `spawnSync` apply
  `markStreamsAsStdio(streamsToQuiesce)` only after the Bun.spawn call
  returns successfully — inside the try block, before the `spawn` event
  nextTick. The catch block never runs it, so a failed spawn leaves the
  source stream untouched.

Also:
- Rewrite the `update_nonblocking` comment in `spawn_process.rs` to stop
  using `markStreamAsStdio` as the load-bearing justification — the
  `PosixStdio::Pipe(fd)` arm is shared by every caller-supplied fd
  (direct numeric stdio, `Bun.file(fd)`, memfd, stream-fd extraction),
  and `O_NONBLOCK` is per-OFD so the parent's fd unavoidably flips with
  the child's. This matches libuv's post-fork `uv__nonblock_fcntl` and
  is documented Node parity.
- Rewrite the third test to actually exercise the `process.stdout` /
  `process.stderr` passthrough path via a sub-bun runner (previously the
  test passed `"ignore", "pipe", "pipe"` strings and didn't exercise
  `extractStreamFd` at all).
- Add a fourth test that asserts a passed-in source stream is still
  fully consumable after a spawn failure (ENOENT) — this test fails
  without the pre-spawn-quiesce fix.
- Drop the stale "three shapes" count from `extractStreamFd`'s JSDoc
  (it only handles two).
When `p1.stdout` is passed as stdio to another spawn:
  1. Accessing `p1.stdout` in the parent bumped `#closesNeeded` and
     registered `pipe.once('close', #maybeClose)`.
  2. `markStreamsAsStdio` then called `setFlowing(false)` →
     `reader().pause()` → FilePoll unregistered. The stream never
     sees EOF on its own.
  3. At exit, `#handleOnExit`'s drain logic skipped `resume()` for
     `kIsUsedAsStdio` streams (to avoid racing the child for bytes).
  4. With no EOF and no resume, the NativeReadable never emitted
     `'close'`; `#closesGot` stalled one short of `#closesNeeded`;
     `p1` never emitted its own `'close'`.

Fix: in the skipped branch, call `.destroy()` on the handed-off
stream. `destroy()` fires `'close'` on the next tick and the
source ChildProcess's close-accounting completes. Releases the
parent-side read fd too. Libraries that wait on `'close'`
(e.g. `execa`) no longer hang.

Also extends the reverse-direction test to assert `'close'` fires
on pSource (with a 5s timeout guard) and adds explicit
`sourceExit` assertions to the reverse and failure-path tests per
review comment.
`FileReader::get_fd()` called `Fd::uv()` on the pipe handle. On
Windows the pipe is a system-kind `Fd` (`Fd::from_system(pipe.fd())`
from `WindowsBufferedReader::Source::Pipe`), and `Fd::uv()` panics
for non-stdio system HANDLEs. Result: any `child_process.spawn` with
piped stdout/stderr would crash the parent on Windows as soon as the
user accessed `proc.stdout` — a regression to all existing piped
spawn usage, not just the new inter-process feature.

Guard the Windows path: return `-1` for now. `constructNativeReadable`
then skips setting `stream.fd`, `extractStreamFd` returns undefined,
and `nodeToBun` falls back to the stream-stdio-unsupported error —
graceful degradation of the new feature rather than a crash.

Also skip the 4 regression tests on Windows; the feature isn't
implemented there yet (would need a HANDLE→uv-fd round-trip via
`makeLibUVOwned`). `FileSink::get_fd()` already had the right shape
(`DecodeWindows::Windows(_) => -1`) so no fix needed on the
WriteStream side.
Review feedback (coderabbit): tests shouldn't use setTimeout to guard a
condition — the bun:test per-test timeout already bounds the wait. If
pSource's 'close' event never fires (pre-fix behavior), awaiting it
directly times out rather than racing a self-imposed 5s deadline.
The two cross-references to bake-codegen.ts's OVERLAY_CSS as the
example of a raw-unquoted-CSS define value are now stale — the
previous commit wraps it in JSON.stringify(). Soften the two call
sites to 'originally' / 'now pre-JSON.stringify'd' so readers
clicking through don't get confused. Lexer recovery and test coverage
still valid.
@robobun robobun force-pushed the farm/e310776e/spawn-stdio-stream-forwarding branch from 0931695 to cdc2cdb Compare June 21, 2026 02:07
@robobun

robobun commented Jun 21, 2026

Copy link
Copy Markdown
Collaborator Author

The red Lint JavaScript check is a repo-wide CI breakage rather than something in this PR. The job runs bun lint with no prior bun install, and #32522 changed the lint script from bunx oxlint ... to plain oxlint .... Without bunx to fetch it, the oxlint binary is not on PATH, so the step exits 127 with oxlint: command not found on every PR (and on main pushes) since that commit.

The code here is clean under the linter: running oxlint 1.70.0 locally surfaced 3 real no-duplicate-conditional-property-access violations (the rule #32522 added) in extractStreamFd, fixed in ea929bf. The three changed src/js files now report 0 warnings / 0 errors.

Comment thread src/js/node/child_process.ts Outdated
Comment thread src/js/internal/streams/native-readable.ts
robobun and others added 2 commits June 21, 2026 02:49
NativeReadable.destroy() now nulls the cached fd that
constructNativeReadable exposes for stdio forwarding. ptr.cancel()
closes the underlying pipe fd, so leaving stream.fd set let a destroyed
subprocess stdout report a stale (closed or kernel-reused) descriptor to
a later spawn, which would dup2 the wrong fd into the child. Mirrors the
WriteStream fast-path close().

spawnSync no longer quiesces source streams. Bun.spawnSync blocks the JS
thread until the child exits, so there is no parent/child read race to
prevent and markStreamsAsStdio only ran once the child was already gone;
its irreversible setFlowing(false) permanently bricked the source stream
(e.g. process.stdin). Node likewise does not mark streams in spawnSync.

Adds regression coverage for both.
Comment thread src/spawn_sys/spawn_process.rs Outdated
Comment thread src/js/node/child_process.ts Outdated
Two documentation fixes for comments that went stale across this PR's
own follow-up commits:

- spawn_process.rs: the O_NONBLOCK comment said the parent's reader is
  "already paused" at the clear site, but markStreamsAsStdio now runs
  after Bun.spawn returns. Reword to "paused on the same JS tick once
  spawn returns" (no FilePoll fires in between), which is the accurate
  justification for why parent-going-blocking is moot.

- child_process.ts: markStreamsAsStdio's JSDoc still listed Bun.spawnSync
  as a caller, but the sync path no longer quiesces. Drop the stale
  reference and note the sync path deliberately skips it.
Comment thread src/js/internal/fs/streams.ts
…leak

extractStreamFd now returns undefined for a destroyed stream. The
WriteStream fast-path only nulls the cached fd in close(), which the
async _destroy branch skips when the sink still has buffered bytes, so a
destroyed subprocess.stdin could otherwise forward a stale (closed or
kernel-reused) fd to a later spawn. Guarding at extractStreamFd covers
the readable and writable sides and any future destroy-path drift.

Also redesign the spawnSync regression test: its old runner did
process.stdin.resume() + process.exit(0), which triggers a pre-existing
LeakSanitizer report for the lazily-created Bun.stdin store Blob
(BunObject.rs make_blob, unrelated to this PR) and failed the x64-asan
lane. It now hands a live subprocess's stdout to spawnSync and asserts
its flowing state is untouched, exercising the same fix without touching
process.stdin. Adds a writable-side regression test for the destroyed
stdin path.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks — 5476676 addresses the last stale-fd concern by guarding item.destroyed in extractStreamFd (with a writable-side regression test). No further issues from my side, but given the scope (fd hand-off across spawn, O_NONBLOCK clearing in spawn_process_posix, native stream quiescing) this still warrants a human look before merge.

Extended reasoning...

Overview

This PR enables passing one subprocess's stdio stream as another subprocess's stdio (e.g. spawn(..., { stdio: [p1.stdout, ...] })), matching Node.js. It touches: node:child_process stdio normalization (nodeToBun/extractStreamFd/markStreamsAsStdio/#handleOnExit); native stream fd exposure (FileReader::get_fd in Rust, getFd host-fn in ReadableStream.rs/streams.classes.ts, .fd assignment in native-readable.ts and fs/streams.ts); POSIX spawn fd prep (clearing O_NONBLOCK in spawn_process_posix); plus an unrelated drive-by fix wrapping OVERLAY_CSS in JSON.stringify in bake-codegen.ts with two cross-reference comments updated. ~300 lines of new regression tests.

Status of prior feedback

This PR has been through ~10 review rounds. All of my prior inline comments are resolved. The last one (🔴 writable-side stale-fd-after-destroy) was addressed in commit 5476676 via option (b) — extractStreamFd now rejects item.destroyed === true up front, with a JSDoc explaining why and a new test (a destroyed subprocess stdin does not leak a stale fd to a later spawn). The bug-hunting system found nothing this round.

Security risks

The change manipulates fd inheritance into child processes (dup2) and clears O_NONBLOCK on caller-supplied fds in the parent. The destroyed-stream guard prevents the obvious stale-fd hazard. No auth/crypto/permissions surface. Risk is correctness/lifecycle (fd reuse, hung streams, parent/child read races) rather than a security boundary.

Level of scrutiny

High. This is core child_process plumbing that affects every Bun user spawning subprocesses, with subtle cross-layer interactions (JS stream state ↔ native FilePoll ↔ posix_spawn fd actions), platform divergence (Windows returns -1 from get_fd to gracefully degrade), and several edge cases that took multiple rounds to get right (spawn-failure ordering, spawnSync, destroyed streams, 'close' accounting). Not appropriate for bot-only approval.

Other factors

Test coverage is good (7 regression tests covering forward/reverse piping, process.stdout passthrough, ENOENT, destroyed readable, destroyed writable, spawnSync). The bake-codegen change is an unrelated drive-by but trivial. CI status was previously flaky on unrelated shards per the author's note; the lint failure is a repo-wide issue (#32522) unrelated to this PR. No human reviewer has weighed in yet.

@robobun

robobun commented Jun 21, 2026

Copy link
Copy Markdown
Collaborator Author

CI status (for a maintainer deciding to merge)

The diff is green on the lanes it actually affects, and all review threads are resolved (CodeRabbit + Claude, last review: no further issues). The remaining red checks are all unrelated to this change and I cannot clear them from here:

  • Lint JavaScript: repo-wide infra regression from lint(src/js): flag duplicate property reads between an if condition and its body #32522, which changed the lint script from bunx oxlint ... to plain oxlint .... The lint workflow runs bun lint with no prior bun install, so oxlint is not on PATH and the job exits 127 with oxlint: command not found on every PR since that commit. The changed src/js files lint clean locally (0 warnings / 0 errors under oxlint 1.70.0).
  • buildkite test/js/node/zlib/zlib.test.js (x64-asan): panic: N JavaScript functions were called outside of the microtask queue. This file uses zero spawn / stream / process.stdin, runs in its own isolated process, and cannot reach this PR's code paths (child_process stdio, stream fd exposure, spawn O_NONBLOCK). It passed on main (build 63692); it looks like a latent event-loop issue surfaced by the recently-enabled ASAN debug-assertions (build: enable Rust debug-assertions for asan/assertions builds; decouple IS_DEBUG #32520).
  • buildkite rotating flaky lane: a different flaky test on each of the last four builds (dev-and-prod.test.ts Windows, hot.test.ts aarch64, test/package.json install-timeout x64-asan, napi.test.ts Windows), all CI-flagged flaky and none related to subprocess stdio. A different test flaking each run is the signature of infra flake, not a real failure.

The originally-reported x64-asan LeakSanitizer failure in the new test is fixed (the spawnSync test was redesigned off a pre-existing Bun.stdin stdio-store Blob leak in BunObject.rs make_blob; a bare bun -e 'process.stdout.write("x")' reproduces that leak). The tests now live at test/js/node/child_process/child-process-stdio-streams.test.ts. Ready for a human to merge (the lint check stays red until the #32522 infra issue is fixed, so a merge has to override it).

Comment thread src/js/node/child_process.ts
Comment thread test/js/node/child_process/child-process-stdio-streams.test.ts
nodeToBun now throws "Cannot use a destroyed stream as stdio[N]" when a
destroyed stream is passed as stdio, instead of the generic "TODO:
stream.Readable/Writable stdio" which reads as "this stream type is not
implemented". The generic message still covers genuinely unsupported
shapes (no fd available, Windows, custom streams).

Move the stdio-stream regression tests from
test/regression/issue/30831.test.ts to
test/js/node/child_process/child-process-stdio-streams.test.ts. Per
CLAUDE.md, test/regression/issue/ is for true regressions; #30831 and
#25498 are a feature gap (the behavior never worked, it threw a TODO
placeholder), so the tests belong in the child_process module's dir.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

1 participant