native-readable: accept a stream with no native handle

Closed
RO
19d ago
+43-1

Problem

  • A debug build asserts after a short-lived child exits when one of its piped stdio streams was never touched: [internal:streams/native-readable] ASSERTION FAILED: typeof bunNativePtr === "object" Invalid native ptr at constructNativeReadable (internal:streams/native-readable:51). Repro: spawn("/bin/sh", ["-c", "echo hello"], { stdio: ["inherit", "pipe"] }), read stdout, never touch stderr. The assertion throws out of the exit handler, so 'exit' and 'close' never fire. The in-tree Node test test/js/node/test/parallel/test-stdin-from-file-spawn.js fails under bun bd for this reason.
  • The cause: when the child's stderr pipe closes with no output before JS reads the stream, the native side turns the pipe into an empty buffer (subprocess.rs:on_close_io) and Readable::to_js returns ReadableStream::empty (subprocess/Readable.rs:235). That stream has no $bunNativePtr. #handleOnExit in child_process.ts constructs the node stream at that point, and constructNativeReadable asserted on the missing handle.
  • Release builds strip $assert. There, read() already ends a handle-less NativeReadable (if (!ptr) push(null)), and destroy, ref, unref already guard a missing handle. The only defect is the assertion.

Fix

  • constructNativeReadable (src/js/internal/streams/native-readable.ts:59) accepts undefined as the handle. The rest of the module already treats undefined as "nothing left to read".
  • child_process.ts keeps its single construction path, so child.stdout and child.stderr stay one class.
  • Verified: test/js/node/child_process/child_process.test.ts (new block "piped stdio first touched after the child exits", the subprocess test fails on the unfixed debug build). Also test/js/node/child_process/, test/js/node/stream/, test/js/node/tty/, process-stdin.test.ts, and 27 upstream test-child-process-{stdio,exec,spawn}-* and test-stdin-from-file-spawn files.

Background

  • Bun.spawn reads a pipe stdout/stderr into a buffered PipeReader until JS touches the stream. At EOF before that, the bytes move into Readable::Buffer. An empty buffer becomes a plain closed ReadableStream. A non-empty one becomes a blob-backed stream with a native handle.
  • node:child_process wraps a native stream with native-readable.ts, a Readable whose _read pulls from the $bunNativePtr handle. With no handle, _read pushes null, so the stream ends and then closes like an empty pipe.
Notes

Self-reviewed: 11 concerns raised, all addressed by the rework. The first version of this PR branched in child_process.ts to Readable.fromWeb when the handle was missing, with stub ref()/unref(). The review found that shape wrong: it duplicated the tryTransferToNativeReadable predicate, made the stream class depend on whether EOF beat waitpid, and sat on the line that #36316, #39760 and #30850 rewrite (and #36316's unconditional constructNativeReadable call would trip the same assert). The one-line change in native-readable.ts covers all callers.

The second new test (in-process, state checks after 'close') passes on the unfixed build for every stdio shape tried (["ignore","pipe"], ["ignore","pipe","pipe"], ["inherit","pipe"]): inside the test runner the exit notification arrives before the pipe EOF, so the stream takes the native path. It is kept as a check of the end state, not as the fail-before proof. The subprocess test is the proof.

Pre-existing in this container, with or without the change: child_process.test.ts "spawn in the default shell" fails, "set env" and "extra stdio pipes are not double-closed on GC" exceed their 5 s budgets (four sequential debug-bun spawns at 1.5 s each, and 7.5 s standalone for the GC test).

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/child_process/child_process.test.ts