Conversation
Fixes #29126. After process.stdin.on('readable', fn) → removeListener('readable', fn), Bun kept polling fd 0, so a stdio:'inherit' child raced the parent for input. Node releases fd 0 via backpressure: tty.ReadStream extends net.Socket with readableHighWaterMark: 0, so push() returns false on every chunk and the handle's readStop() fires; _read() re-acquires only when a consumer pulls. Bun's tty.ReadStream extended fs.ReadStream (with default hwm) and the stdin wrapper had no backpressure-driven stop. This rewrites the TTY path to match Node's architecture: Native: - New Zig TTY handle (tty.classes.ts + TTY.zig) backed by bun.io.BufferedReader + StreamingWriter, exposing readStart/readStop/ setRawMode/getWindowSize/ref/unref/close/onread plus the usocket surface net.Socket expects (pause/resume/$write/$end/bytesWritten). JSRef for GC liveness; fd reopened O_NONBLOCK with original-fd fallback (libuv-style). - ProcessBindingTTYWrap.cpp: deleted C++ TTYWrapObject; the codegen TTY is now process.binding('tty_wrap').TTY. JS: - tty.ReadStream: extends net.Socket, _handle = new TTY(fd), readableHighWaterMark: 0, manualStart: true — line-for-line Node lib/tty.js. - net.Socket: accepts {handle, manualStart}; initSocketHandle wires onread/ondrain when _handle.readStart is callable; new onStreamRead/tryReadStart (Node stream_base_commons pattern). - ProcessObjectInternals.ts getStdinStream: TTY branch is now just new tty.ReadStream(fd) + readStop + 'pause' listener (Node is_main_thread.js parity). Pipe/file path unchanged. After removeListener('readable'), Readable stops calling _read(); the in-flight read delivers one chunk, push() returns false, readStop() unregisters the poll, and the child reads fd 0 exclusively. No listener tracking, no _readableState pokes, no instance method overrides.
Fixes #29126. process.stdin is now constructed exactly as Node's getStdin() does: - TTY → tty.ReadStream (extends net.Socket, native TTY handle) - PIPE → net.Socket({fd, readable:true, writable:false, manualStart:true}) with native Pipe handle - FILE → fs.ReadStream(null, {fd, autoClose:false}) After on('readable')/removeListener('readable'), Readable stops calling _read(); the next chunk's push() returns false (highWaterMark 0 on TTY) → onStreamRead calls handle.readStop() → fd 0 is released and a stdio:'inherit' child reads it exclusively. No listener tracking, no _readableState pokes, no instance method overrides. Native: - node_util_binding.zig: guessHandleType(fd) (POSIX isatty/fstat/SO_TYPE, Windows uv_guess_handle) - Pipe.zig + pipe.classes.ts (new): two-step new Pipe(type) + .open(fd), readStart/readStop/ref/unref/close/onread, (nread, ArrayBuffer) encoding - TTY.zig: onread → (nread, ArrayBuffer); removed pause/resume/$write/$end shims (Node's wraps don't have them); negative-errno returns - ProcessBindingPipeWrap.{cpp,h} (new): exposes process.binding('pipe_wrap') with {Pipe, constants} JS: - net.ts: createHandle(fd) via guessHandleType; Socket({fd}) constructs a Pipe handle; Socket({handle, manualStart}); onStreamRead(nread, buf) per Node's stream_base_commons; pause/resume/read gated on kBuffer (Node's pattern, not handle shims); readable/writable options honored - ProcessObjectInternals.ts getStdinStream: replaced the Bun.stdin.stream()/own/disown/internalRead/listener-override bridge with Node's switch — −168 net lines Behavior changes that are deliberate Node-parity corrections: - process.stdin.end is a function (was the number Infinity for pipes) - process.stdin.writable === false for pipes - pause() alone does not let the process exit if the pipe is open (stdin-fixtures "pause allows exit" updated to runBoth Node-parity)
WalkthroughThis PR implements native Node.js TTY and Pipe stream-wrap bindings to resolve stdin backpressure issues. The changes introduce new Zig/C++ bindings for TTY and Pipe handles, refactor the stdin stream to use type-specific Node.js streams (TTY, File, Pipe, or generic Readable), and update Socket/TTY prototypes to properly integrate with the new native handles. Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
Found 6 issues this PR may fix:
🤖 Generated with Claude Code |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/bun.js/node/Pipe.zig`:
- Around line 233-234: The current code in Pipe.zig silently drops input when
allocations fail: change the allocation handling around
bun.default_allocator.dupe(u8, chunk) and
jsc.ArrayBuffer.createBuffer(globalThis, duped) so OOMs call bun.handleOom() and
other errors are propagated (not returning this.flags.reading); if createBuffer
fails, free the duplicated buffer (duped) via the allocator before returning or
propagating the error (use defer or explicit free) so no memory leaks occur;
ensure the function returns or propagates an error state instead of pretending
the chunk was consumed and only convert error.OutOfMemory to a crash via
bun.handleOom().
In `@src/bun.js/node/TTY.zig`:
- Around line 287-288: The read path currently swallows allocation/creation
failures by returning this.flags.reading and leaking duped when
jsc.ArrayBuffer.createBuffer fails; change it to call bun.handleOom(err) if
bun.default_allocator.dupe returns error.OutOfMemory, otherwise propagate or
return the error instead of pretending the chunk was consumed, and ensure duped
is freed (or deferred) if createBuffer fails before returning the error—adjust
the code around bun.default_allocator.dupe, duped, and
jsc.ArrayBuffer.createBuffer to use defer/cleanup and bun.handleOom() per
guidelines.
In `@src/js/node/net.ts`:
- Around line 70-77: The createHandle function misclassifies TCP fds as Pipe and
write support for fd-backed stream handles is missing; update createHandle
(which uses guessHandleType and handleTypes) to return a TCP wrapper instance
(new TCP(...)) when the guessed type is "TCP" instead of creating a Pipe, and
ensure Pipe still handles "PIPE"; then implement proper write/shutdown plumbing
in the fh-backed stream-wrap code paths (the _write method and related
shutdown/send handles used by net.Socket({ fd, writable: true })) so writable
fd-backed sockets call the TCP wrapper's write/send/shutdown operations rather
than rejecting, matching Node.js behavior—use symbols TCP, Pipe, PipeConstants,
createHandle, _write and the stream-wrap handling code to locate and update the
logic.
In `@test/js/node/process/stdin/run-with-pty-readable.py`:
- Around line 23-27: The timeout_handler function currently swallows all
exceptions with a bare except; change it to only catch the expected
process-termination errors—e.g., except (ProcessLookupError, OSError):—when
calling os.kill(pid, 9) and let other exceptions (like
KeyboardInterrupt/SystemExit) propagate; update timeout_handler to catch those
specific exceptions and avoid a bare except so real harness failures are not
hidden.
In `@test/regression/issue/29126.test.ts`:
- Around line 33-37: The test currently asserts an empty stderr in the object
comparison using expect({ stderr, childLines, exitCode }).toEqual(...); remove
the strict stderr: "" assertion so the test only checks childLines (retain the
expect.arrayContaining(['CHILD:"B"', 'CHILD:"C"', 'CHILD:"D"', 'CHILD:"E"'])
reference) and exitCode: 0; update the expectation to compare only childLines
and exitCode (or assert stderr is defined/ignored) to avoid ASAN spurious output
failures while keeping the childLines assertions intact.
🪄 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: 67de147a-d29d-4499-b7e6-5876f0e8e474
📒 Files selected for processing (25)
src/bun.js/bindings/BunProcess.cppsrc/bun.js/bindings/ProcessBindingPipeWrap.cppsrc/bun.js/bindings/ProcessBindingPipeWrap.hsrc/bun.js/bindings/ProcessBindingTTYWrap.cppsrc/bun.js/bindings/generated_classes_list.zigsrc/bun.js/bindings/webcore/DOMClientIsoSubspaces.hsrc/bun.js/bindings/webcore/DOMIsoSubspaces.hsrc/bun.js/node.zigsrc/bun.js/node/Pipe.zigsrc/bun.js/node/TTY.zigsrc/bun.js/node/node_util_binding.zigsrc/bun.js/node/pipe.classes.tssrc/bun.js/node/tty.classes.tssrc/js/builtins/ProcessObjectInternals.tssrc/js/internal/shared.tssrc/js/node/net.tssrc/js/node/tty.tstest/js/node/nodettywrap.test.tstest/js/node/process/process.test.jstest/js/node/process/stdin/readable-removed-releases-tty.mjstest/js/node/process/stdin/run-with-pty-readable.pytest/js/node/process/stdin/stdin-fixtures.test.tstest/js/node/process/stdin/stdin-pipe-prototype.test.tstest/js/node/tty-readstream-prototype.test.tstest/regression/issue/29126.test.ts
💤 Files with no reviewable changes (2)
- src/bun.js/bindings/webcore/DOMClientIsoSubspaces.h
- src/bun.js/bindings/webcore/DOMIsoSubspaces.h
| const duped = bun.default_allocator.dupe(u8, chunk) catch return this.flags.reading; | ||
| const buf = jsc.ArrayBuffer.createBuffer(globalThis, duped) catch return this.flags.reading; |
There was a problem hiding this comment.
Don't silently drop chunks on allocation failure.
Both failure paths currently just return this.flags.reading, which loses input bytes, and the createBuffer() failure path also leaves the duplicated slice without visible cleanup. This should either crash on OOM or surface an error, not continue as if the chunk was handled.
As per coding guidelines, "Use bun.handleOom() to convert error.OutOfMemory into a crash without swallowing other errors" and "Be careful with memory management in Zig code - use allocators and defer for cleanup".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/bun.js/node/Pipe.zig` around lines 233 - 234, The current code in
Pipe.zig silently drops input when allocations fail: change the allocation
handling around bun.default_allocator.dupe(u8, chunk) and
jsc.ArrayBuffer.createBuffer(globalThis, duped) so OOMs call bun.handleOom() and
other errors are propagated (not returning this.flags.reading); if createBuffer
fails, free the duplicated buffer (duped) via the allocator before returning or
propagating the error (use defer or explicit free) so no memory leaks occur;
ensure the function returns or propagates an error state instead of pretending
the chunk was consumed and only convert error.OutOfMemory to a crash via
bun.handleOom().
| const duped = bun.default_allocator.dupe(u8, chunk) catch return this.flags.reading; | ||
| const buf = jsc.ArrayBuffer.createBuffer(globalThis, duped) catch return this.flags.reading; |
There was a problem hiding this comment.
Don't swallow allocation failures in the read path.
Returning this.flags.reading here drops terminal input on allocation/buffer-creation failure, and the createBuffer() error path does not show cleanup for duped. This path should fail loudly instead of pretending the chunk was consumed.
As per coding guidelines, "Use bun.handleOom() to convert error.OutOfMemory into a crash without swallowing other errors" and "Be careful with memory management in Zig code - use allocators and defer for cleanup".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/bun.js/node/TTY.zig` around lines 287 - 288, The read path currently
swallows allocation/creation failures by returning this.flags.reading and
leaking duped when jsc.ArrayBuffer.createBuffer fails; change it to call
bun.handleOom(err) if bun.default_allocator.dupe returns error.OutOfMemory,
otherwise propagate or return the error instead of pretending the chunk was
consumed, and ensure duped is freed (or deferred) if createBuffer fails before
returning the error—adjust the code around bun.default_allocator.dupe, duped,
and jsc.ArrayBuffer.createBuffer to use defer/cleanup and bun.handleOom() per
guidelines.
| function createHandle(fd, isServer) { | ||
| validateInt32(fd, "fd", 0); | ||
| const type = handleTypes[guessHandleType(fd)]; | ||
| if (type === "PIPE" || type === "TCP") { | ||
| return new Pipe(isServer ? PipeConstants.SERVER : PipeConstants.SOCKET); | ||
| } | ||
| throw $ERR_INVALID_FD_TYPE(type); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In current Node.js lib/net.js, how does createHandle(fd, is_server)mapguessHandleType(fd)values"PIPE"and"TCP", and does net.Socket({ fd, writable: true }) support writing on fd-backed stream handles?
💡 Result:
In current Node.js lib/net.js, createHandle(fd, is_server) calls guessHandleType(fd) (from internal bindings like tty_wrap/util) to determine the handle type. For "PIPE", it creates a new Pipe(is_server ? PipeConstants.SERVER : PipeConstants.SOCKET). For "TCP", it creates a new TCP(is_server ? TCPConstants.SERVER : TCPConstants.SOCKET). net.Socket({ fd, writable: true }) supports writing on fd-backed stream handles (PIPE or TCP), as the writable: true option explicitly allows writes when an fd is passed, enabling the Duplex stream's write functionality on valid socket/pipe FDs.
Citations:
- 1: How to use fd with net.Socket? nodejs/node#8297
- 2: https://nodejs.cn/api-v12/net/new_net_socket_options.html
- 3: https://deno.land/std@0.177.0/node/net.ts
🏁 Script executed:
# First, let's examine the createHandle function at lines 70-77
sed -n '65,85p' src/js/node/net.tsRepository: oven-sh/bun
Length of output: 781
🏁 Script executed:
# Now let's look at the _write rejection around lines 1514-1519
sed -n '1510,1525p' src/js/node/net.tsRepository: oven-sh/bun
Length of output: 643
🏁 Script executed:
# Let's also check for TCP binding imports in the file
rg "tcp_wrap|TCP" src/js/node/net.ts -n | head -20Repository: oven-sh/bun
Length of output: 395
Fix createHandle() to map TCP descriptors to new TCP() and implement write support for fd-backed stream handles.
createHandle() returns new Pipe() for both "PIPE" and "TCP" types, misclassifying TCP descriptors. Additionally, _write() rejects every stream-wrap handle, leaving net.Socket({ fd, writable: true }) non-functional even though the constructor now accepts these fds. TCP requires the proper TCP wrapper, and writable stream-wrap handles need complete write/shutdown plumbing to match Node.js behavior.
Example fix for createHandle
function createHandle(fd, isServer) {
validateInt32(fd, "fd", 0);
const type = handleTypes[guessHandleType(fd)];
- if (type === "PIPE" || type === "TCP") {
+ if (type === "PIPE") {
return new Pipe(isServer ? PipeConstants.SERVER : PipeConstants.SOCKET);
}
+ if (type === "TCP") {
+ const { TCP, constants: TCPConstants } = process.binding("tcp_wrap");
+ return new TCP(isServer ? TCPConstants.SERVER : TCPConstants.SOCKET);
+ }
throw $ERR_INVALID_FD_TYPE(type);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/js/node/net.ts` around lines 70 - 77, The createHandle function
misclassifies TCP fds as Pipe and write support for fd-backed stream handles is
missing; update createHandle (which uses guessHandleType and handleTypes) to
return a TCP wrapper instance (new TCP(...)) when the guessed type is "TCP"
instead of creating a Pipe, and ensure Pipe still handles "PIPE"; then implement
proper write/shutdown plumbing in the fh-backed stream-wrap code paths (the
_write method and related shutdown/send handles used by net.Socket({ fd,
writable: true })) so writable fd-backed sockets call the TCP wrapper's
write/send/shutdown operations rather than rejecting, matching Node.js
behavior—use symbols TCP, Pipe, PipeConstants, createHandle, _write and the
stream-wrap handling code to locate and update the logic.
| def timeout_handler(signum, frame): | ||
| try: | ||
| os.kill(pid, 9) | ||
| except: | ||
| pass |
There was a problem hiding this comment.
Narrow the timeout-handler exception.
The bare except: here will also swallow KeyboardInterrupt/SystemExit and can hide real harness failures. Catch the expected process-termination error (ProcessLookupError/OSError) instead.
Suggested fix
def timeout_handler(signum, frame):
try:
os.kill(pid, 9)
- except:
+ except ProcessLookupError:
+ pass
+ except OSError:
pass
sys.exit(1)🧰 Tools
🪛 Ruff (0.15.9)
[warning] 23-23: Unused function argument: signum
(ARG001)
[warning] 23-23: Unused function argument: frame
(ARG001)
[warning] 24-27: Use contextlib.suppress(BaseException) instead of try-except-pass
Replace try-except-pass with with contextlib.suppress(BaseException): ...
(SIM105)
[error] 26-26: Do not use bare except
(E722)
[error] 26-27: try-except-pass detected, consider logging the exception
(S110)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/js/node/process/stdin/run-with-pty-readable.py` around lines 23 - 27,
The timeout_handler function currently swallows all exceptions with a bare
except; change it to only catch the expected process-termination errors—e.g.,
except (ProcessLookupError, OSError):—when calling os.kill(pid, 9) and let other
exceptions (like KeyboardInterrupt/SystemExit) propagate; update timeout_handler
to catch those specific exceptions and avoid a bare except so real harness
failures are not hidden.
| expect({ stderr, childLines, exitCode }).toEqual({ | ||
| stderr: "", | ||
| childLines: expect.arrayContaining(['CHILD:"B"', 'CHILD:"C"', 'CHILD:"D"', 'CHILD:"E"']), | ||
| exitCode: 0, | ||
| }); |
There was a problem hiding this comment.
Avoid asserting empty stderr in this regression test.
Line 34 can make this test fail on ASAN builds even when behavior is correct. Keep the child-line assertions and assert only exitCode for pass/fail.
Suggested assertion update
- expect({ stderr, childLines, exitCode }).toEqual({
- stderr: "",
- childLines: expect.arrayContaining(['CHILD:"B"', 'CHILD:"C"', 'CHILD:"D"', 'CHILD:"E"']),
- exitCode: 0,
- });
+ expect(childLines).toEqual(expect.arrayContaining(['CHILD:"B"', 'CHILD:"C"', 'CHILD:"D"', 'CHILD:"E"']));
expect(childLines.length).toBeGreaterThanOrEqual(4);
+ expect(exitCode).toBe(0);Based on learnings: in test/regression/issue/*.test.ts, do not assert stderr is empty for spawned subprocesses because ASAN may emit benign warnings.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/regression/issue/29126.test.ts` around lines 33 - 37, The test currently
asserts an empty stderr in the object comparison using expect({ stderr,
childLines, exitCode }).toEqual(...); remove the strict stderr: "" assertion so
the test only checks childLines (retain the expect.arrayContaining(['CHILD:"B"',
'CHILD:"C"', 'CHILD:"D"', 'CHILD:"E"']) reference) and exitCode: 0; update the
expectation to compare only childLines and exitCode (or assert stderr is
defined/ignored) to avoid ASAN spurious output failures while keeping the
childLines assertions intact.
Jarred-Sumner
left a comment
Jarred-Sumner
left a comment
There was a problem hiding this comment.
This has to be ref-counted. All the current usages of flags.finalized as UAF.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| const duped = bun.default_allocator.dupe(u8, chunk) catch return this.flags.reading; | ||
| const buf = jsc.ArrayBuffer.createBuffer(globalThis, duped) catch return this.flags.reading; |
There was a problem hiding this comment.
🔴 In both Pipe.zig:onReadChunk (lines 233-234) and TTY.zig:onReadChunk (lines 287-288), the buffer allocation from bun.default_allocator.dupe() is leaked on every successful chunk read because jsc.ArrayBuffer.createBuffer internally copies the data via Bun__createUint8ArrayForCopy and never takes ownership of the input slice. Additionally, if dupe() succeeds but createBuffer() fails (OOM), the allocated buffer is also leaked. The fix is to pass chunk directly to createBuffer (since it copies anyway), eliminating the unnecessary dupe() entirely.
Extended reasoning...
The Bug
In Pipe.zig (lines 233-234) and TTY.zig (lines 287-288), onReadChunk does:
const duped = bun.default_allocator.dupe(u8, chunk) catch return this.flags.reading;
const buf = jsc.ArrayBuffer.createBuffer(globalThis, duped) catch return this.flags.reading;
Why duped Always Leaks (the Normal Path)
jsc.ArrayBuffer.createBuffer (in src/bun.js/jsc/array_buffer.zig) takes a []const u8 slice and calls Bun__createUint8ArrayForCopy (in ZigGlobalObject.cpp). That C++ function creates a new JSUint8Array via createUninitialized() then memcpy's the input bytes into it — it makes its own copy of the data. The []const u8 signature confirms no ownership transfer. After createBuffer returns successfully, duped is an unreferenced heap allocation that nothing will ever free. This leak occurs on every successful read, not just under error conditions.
Why duped Also Leaks on createBuffer Failure
If createBuffer throws an exception (e.g., OOM), the catch return this.flags.reading silently discards both the exception and the already-allocated duped slice. There is no errdefer bun.default_allocator.free(duped) before the second line, so the allocation is abandoned.
Concrete Proof
For a process reading 1 KB/chunk from stdin, each call to onReadChunk allocates via dupe() and immediately loses the pointer. After 100 MB of data read, 100 MB of allocator memory has been abandoned. The heap grows unboundedly proportional to stdin/pipe throughput, never freed until process exit.
Why Existing Code Does Not Prevent It
The comment in TTY.zig says g6: dupe — the reader buffer is reused across reads, correctly identifying the reason to copy the data. But the chosen mechanism (dupe + createBuffer) copies the data twice: once into duped, once inside Bun__createUint8ArrayForCopy. The first copy is not only unnecessary but is the one that leaks.
Impact
Memory usage grows linearly with data read through any pipe or TTY stdin. Long-running processes that read substantial stdin (interactive REPLs, servers reading stdin for commands, etc.) will exhibit steady memory growth that cannot be GC'd because the allocations are from bun.default_allocator (not the JS heap).
Fix
Since createBuffer copies chunk internally anyway, pass chunk directly:
const buf = jsc.ArrayBuffer.createBuffer(globalThis, chunk) catch return this.flags.reading;
This eliminates the dupe() entirely. The same one-line fix applies in TTY.zig lines 287-288. The reader buffer remains valid for the duration of the createBuffer call since it copies synchronously before returning.
| const { Readable } = require("node:stream"); | ||
| stdin = new Readable({ read() {} }); | ||
| stdin.push(null); | ||
| } | ||
| } | ||
|
|
||
| function triggerRead(_size) { | ||
| $debug("_read();", reader); | ||
| stdin.fd = fd; | ||
|
|
||
| if (reader && !shouldDisown) { | ||
| internalRead(this); | ||
| } else { | ||
| // The stream has not been ref()ed yet. If it is ever ref()ed, | ||
| // run internalRead() | ||
| needsInternalReadRefresh = true; | ||
| } | ||
| // `stdin` starts out life in a paused state. Explicitly readStop() it to put |
There was a problem hiding this comment.
🔴 The new stdin 'pause' event handler in ProcessObjectInternals.ts schedules readStop() via process.nextTick() with only a handle-existence guard, dropping the old check for whether the stream has already been resumed before the tick fires. This race can permanently silence stdin when backpressure cycling causes pause→resume faster than a tick, particularly with the new TTY/Pipe handles whose hwm is 0, making every chunk trigger it.
Extended reasoning...
What the bug is and how it manifests
In the new getStdinStream() implementation (ProcessObjectInternals.ts lines 172–180), the pause handler queues readStop() asynchronously via process.nextTick():
stdin.on("pause", () => {
process.nextTick(() => {
if (\!stdin._handle) return;
stdin._handle.reading = false;
stdin._readableState.reading = false;
stdin._handle.readStop();
});
});The only guard is if (\!stdin._handle) return. The old code had an equivalent of if (\!stream.readableFlowing) { disown(); } which prevented the readStop call when the stream had already been resumed before the tick fired. The new code has no such guard, creating a window where a resume that happens synchronously between the 'pause' event and the nextTick callback fires will be silently undone.
The specific code path that triggers it
The most reliable trigger is backpressure cycling through onStreamRead, which for TTY handles fires on every chunk because highWaterMark is effectively 0 for raw TTY mode:
- onStreamRead fires with data; self.push(data) returns false (buffer full / hwm=0).
- onStreamRead checks: if (ret === false && this.reading) { this.reading = false; this.readStop(); }
- Duplex internals detect the push() return value and transition readable state, synchronously emitting 'pause'.
- Our handler queues process.nextTick(readStop).
- Before the tick fires, the consumer calls stream.read() or data flows through piped consumers. Duplex's flow() → stream.read(0) → _read() is called.
- Socket.prototype._read: if (!socket.reading) tryReadStart(this) → socket._handle.reading = true, readStart() succeeds.
- Now the nextTick fires: reading = false, readStop() — silently cancels the readStart. stdin is permanently stuck.
Why existing code does not prevent it
The refutation argued that Socket.prototype.resume/read/pause are all gated on this[kBuffer] and stdin is constructed without an onread option, so kBuffer is never set, meaning those Socket methods are no-ops and can never call tryReadStart(). This is correct for the Socket-level pause/resume methods — they will not call tryReadStart(). However, Socket.prototype._read is not gated on kBuffer:
Socket.prototype._read = function _read(size) {
const socket = this._handle;
if (this.connecting || \!socket) {
this.once("connect", () => this._read(size));
} else if ((socket.readStart)) {
if (\!socket.reading) tryReadStart(this); // ← NOT gated on kBuffer
} else {
socket?.resume?.();
}
};_read() is the method that Duplex's stream machinery calls. When flow() or a piped consumer triggers stream.read(), it goes through Readable.prototype.read() → this._read(), which calls tryReadStart(). This path exists independently of the kBuffer gating on the Socket-level methods.
Impact
Stdin can silently stop delivering data after the first backpressure cycle. Any program that pipes stdin through a Transform, reads chunks in a loop, or even just attaches a 'data' listener (which puts the stream in flowing mode and causes continuous push()/pause cycles) is vulnerable. Because readStop() ultimately holds a GC reference (JSRef) that keeps the process alive, a stuck stdin may also cause the process to hang or exit unexpectedly depending on whether the ref is still held. The bug is especially likely to manifest with the new TTY and Pipe handle types introduced in this PR, which use BufferedReader at the Zig level with aggressive backpressure.
How to fix it
Add a guard at the top of the nextTick callback to abort if the stream is no longer paused:
stdin.on("pause", () => {
process.nextTick(() => {
if (\!stdin._handle) return;
if (\!stdin.isPaused()) return; // ← add this guard
stdin._handle.reading = false;
stdin._readableState.reading = false;
stdin._handle.readStop();
});
});stdin.isPaused() returns true only when readableFlowing === false, which is exactly the condition the old code checked. Alternatively, checking stdin._readableState.flowing !== false is equivalent and slightly more direct.
Step-by-step proof
Setup: Node-faithful net.Socket stdin (fd=0, PIPE type). A consumer pipes stdin: stdin.pipe(process.stdout).
- readStart() fires for the first time via Socket constructor → tryReadStart → reading=true.
- First chunk arrives: onStreamRead(nread>0) → self.push(chunk). For a default Duplex, hwm=16384. push() returns true. No readStop.
- (hwm=0 scenario or buffer full:) push() returns false. onStreamRead: reading=false, readStop(). Duplex transitions paused. 'pause' emitted synchronously.
- Our handler: process.nextTick(fn) queued (fn will set reading=false and call readStop).
- Synchronously: pipe's internal drain handler or stdout's 'drain' triggers stdin.resume() → Duplex flow() → stream.read(0) → Socket._read() → !socket.reading is true → tryReadStart() → reading=true, readStart() called. stdin is reading again.
- Queued nextTick fires. stdin._handle exists ✓. No isPaused() guard. reading=false, readStop() called. stdin stops reading.
- No more data will arrive. If this is interactive stdin, the terminal appears frozen. If piped, the program silently stops processing input.
| Socket.$call(this, { | ||
| readableHighWaterMark: 0, | ||
| handle: tty, | ||
| manualStart: true, | ||
| ...options, | ||
| }); |
There was a problem hiding this comment.
🔴 tty.ReadStream does not pass writable: false to the Socket constructor, so process.stdin.writable === true on TTY stdin instead of the expected false. The fix requires both passing writable: false in src/js/node/tty.ts and honoring options.writable in the Socket handle-path in src/js/node/net.ts (which currently only reads options.readable/writable on the fd-path).
Extended reasoning...
What the bug is and how it manifests
tty.ReadStream is a read-only stream representing stdin on a terminal. In Node.js, process.stdin.writable is false when stdin is a TTY. In Bun's implementation, process.stdin.writable is true on TTY stdin because tty.ReadStream never tells the underlying Socket it is write-only.
The specific code path that triggers it
In src/js/node/tty.ts lines 38-43, ReadStream calls the Socket constructor with no writable: false in the options bag. Node.js lib/tty.js passes { readable: true, writable: false, handle: tty, manualStart: true } explicitly, so its Duplex base class sets this.writable = false on construction.
There is also a secondary issue in src/js/node/net.ts: the Socket constructor's handle branch only assigns this._handle = options.handle and calls initSocketHandle(this), but never reads options.writable or options.readable. The fd branch further down does read those options and sets this.readable / this.writable correctly. So even if tty.ReadStream were fixed to pass writable: false, the Socket constructor would still ignore it on the handle path -- both sites need a fix.
Why existing code does not prevent it
Duplex (and therefore Socket) defaults this.writable = true unless explicitly set otherwise. Because the handle path in Bun's Socket constructor never applies the caller-supplied writable option, the default is never overridden. The newly added test file test/js/node/tty-readstream-prototype.test.ts checks the prototype chain, readableHighWaterMark, and handle methods, but does not assert writable === false, so the regression has no test coverage.
Impact
Code that guards TTY writes with if (!process.stdin.writable) or checks stream.writable to decide whether piping is safe will behave incorrectly. Libraries like readline and interactive CLI tools frequently inspect stdin.writable; returning true instead of false can cause unexpected write attempts on a read-only handle.
How to fix it
- In src/js/node/tty.ts, add writable: false to the Socket options object passed to Socket.$call.
- In src/js/node/net.ts, apply options.readable / options.writable in the handle branch similarly to how the fd branch does.
- Add an assertion expect(stream.writable).toBe(false) in test/js/node/tty-readstream-prototype.test.ts.
Step-by-step proof
- Create a script: console.log(process.stdin.writable)
- Run it in a terminal: bun run script.ts
- Expected output (Node.js): false
- Actual output (Bun with this PR): true, because Socket.$call receives no writable: false, the handle path never sets this.writable, and the Duplex default of true persists.
|
|
||
| function createHandle(fd, isServer) { | ||
| validateInt32(fd, "fd", 0); | ||
| const type = handleTypes[guessHandleType(fd)]; | ||
| if (type === "PIPE" || type === "TCP") { | ||
| return new Pipe(isServer ? PipeConstants.SERVER : PipeConstants.SOCKET); | ||
| } | ||
| throw $ERR_INVALID_FD_TYPE(type); | ||
| } |
There was a problem hiding this comment.
🔴 In createHandle() (net.ts lines 73-74), both 'PIPE' and 'TCP' fd types return new Pipe(...), but Node.js returns new TCP() for TCP fds. Any user calling net.Socket({fd}) where fd is a connected TCP socket will get a Pipe handle with incorrect semantics — writes immediately fail with ERR_METHOD_NOT_IMPLEMENTED, and TCP-specific operations (setNoDelay, setKeepAlive, TCP shutdown semantics) are absent. Since tcp_wrap is not implemented in Bun, the correct fix is to throw ERR_INVALID_FD_TYPE for TCP fds rather than silently using the wrong handle type.
Extended reasoning...
What the bug is and how it manifests
In src/js/node/net.ts lines 73-74, createHandle() contains:
if (type === "PIPE" || type === "TCP") {
return new Pipe(isServer ? PipeConstants.SERVER : PipeConstants.SOCKET);
}When guessHandleType(fd) returns 'TCP' (i.e., the fd is a TCP socket), the function still constructs a Pipe handle. The || type === 'TCP' branch was added to mirror Node's createHandle interface signature, but the constructor was not replaced with a TCP-specific one.
The specific code path that triggers it
Any call to new net.Socket({ fd: tcpFd }) where tcpFd is a TCP socket will flow through createHandle() → returns new Pipe(SOCKET). The Socket constructor then stores this Pipe as this._handle, sets this._handle.onread = onStreamRead, and proceeds normally — silently using the wrong handle type.
Why existing code doesn't prevent it
The _write path does guard against stream-wrap handles:
if ((socket.readStart)) {
callback(("_write on stream-wrap handle"));
return false;
}But this only surfaces on writes. The handle is still created and used — reads proceed through the Pipe's BufferedReader, which has no TCP semantics (no MSG_WAITALL, no TCP backpressure management). Operations like setNoDelay, setKeepAlive, and TCP's FIN-based shutdown vs pipe close are also absent from the Pipe interface.
Impact
Any library or application that passes a TCP socket fd to net.Socket({fd}) — e.g., code inheriting a pre-connected socket, custom IPC over TCP, or any Node.js pattern that relies on fd-passing — will silently get a broken socket. Reads may work partially (Pipe's BufferedReader can drain a file-like fd), but the semantics differ. Writes fail with ERR_METHOD_NOT_IMPLEMENTED. This is a silent correctness failure with no obvious error at construction time.
How to fix it
Since process.binding('tcp_wrap') is currently PROCESS_BINDING_NOT_IMPLEMENTED in BunProcess.cpp, the proper Node-faithful fix (creating a real TCP handle) is not yet available. The correct interim fix is to throw ERR_INVALID_FD_TYPE for TCP fds rather than silently constructing a broken Pipe:
function createHandle(fd, isServer) {
validateInt32(fd, "fd", 0);
const type = handleTypes[guessHandleType(fd)];
if (type === "PIPE") {
return new Pipe(isServer ? PipeConstants.SERVER : PipeConstants.SOCKET);
}
+ // TCP fds require a tcp_wrap handle which is not yet implemented.
+ // Silently returning a Pipe would produce a handle with wrong semantics.
throw (type);
}This is consistent with Node's behavior: if the right handle type can't be constructed, fail loudly rather than silently misbehave.
Step-by-step proof
- User calls
net.Socket({ fd: someTCPSocketFd }) - Socket constructor reaches:
this._handle = createHandle(options.fd, false) guessHandleType(someTCPSocketFd)→ inspects fd viagetsockopt(SO_TYPE)+getsockname→ family is INET/INET6, type is SOCK_STREAM → returns index0→handleTypes[0]='TCP'createHandlehitsif (type === 'PIPE' || type === 'TCP')→ true → returnsnew Pipe(SOCKET)- Pipe is stored as
this._handle;this.writable = options.writable \!== false→ socket appears writable - User calls
socket.write('hello') _writechecks(socket.readStart)→ true (Pipe has readStart) → immediately callscallback((...))- The write silently fails with a method-not-implemented error; no TCP data is ever sent
🔬 also observed by verify-runtime
|
this was too big and risky of a change - will stack into a few prs. |
|
Split into a reviewable stack with the ref-counting fix applied:
Stack: |
3 participants
Fixes #29126. Supersedes #29121 and #29127.
What
process.stdinis now constructed exactly as Node'sgetStdin()does:tty.ReadStream(extendedfs.ReadStream)tty.ReadStreamextendsnet.Socket,_handle = native TTYfs.ReadStreamviaBun.stdin.stream()bridgenet.Socket({fd}),_handle = native Pipefs.ReadStreamfs.ReadStream(unchanged)After
on('readable')→removeListener('readable'), Readable stops calling_read(); the next chunk'spush()returns false (highWaterMark: 0on TTY) →onStreamReadcallshandle.readStop()→ fd 0 is released and astdio: 'inherit'child reads it exclusively. No listener tracking, no_readableStatepokes, no instance method overrides — Node's backpressure mechanism, driven by Node's handle architecture.How
Native:
node_util_binding.zig:guessHandleType(fd)(POSIXisatty/fstat/SO_TYPE, Windowsuv_guess_handle)Pipe.zig+pipe.classes.ts(new): two-stepnew Pipe(type)+.open(fd),readStart/readStop/ref/unref/close/onreadwith(nread, ArrayBuffer)encoding (Node's stream_base contract)TTY.zig:onread→(nread, ArrayBuffer); removedpause/resume/$write/$endshims (Node's wraps don't have them); negative-errno returnsProcessBindingPipeWrap.{cpp,h}(new):process.binding('pipe_wrap')with{Pipe, constants}JS:
net.ts:createHandle(fd)viaguessHandleType;Socket({fd})→ Pipe handle;Socket({handle, manualStart});onStreamRead(nread, buf)per Node'sstream_base_commons;pause/resume/readgated onkBuffer(Node's pattern);readable/writableoptions honoredtty.ts:ReadStreammatches Node'slib/tty.jsline-for-lineProcessObjectInternals.ts: theBun.stdin.stream()/own/disown/internalRead/listener-override bridge replaced by Node's switch — −168 net linesNode-parity corrections (deliberate behavior changes)
process.stdin.endis a function (was the numberInfinityfor pipes —.end()threw)process.stdin.writable === falsefor pipesprocess.stdin instanceof net.Socket === truefor TTY and pipepause()alone doesn't let the process exit if the pipe is open (Node doesn't either)Test plan
test/regression/issue/29126.test.ts— PTY-based fd-release; fails on system buntest/js/node/tty-readstream-prototype.test.ts—instanceof net.Socket,_handle: TTY, hwm=0test/js/node/process/stdin/stdin-pipe-prototype.test.ts—instanceof net.Socket,_handle: Pipe,.end()worksbun run zig:check-all— all platformsremoveAllListeners(), pause/resume cycles,once('readable')re-subscribe, 1MB pipe, empty pipe, write-to-readonly, destroy-then-read,net.Socket({fd:999})/{fd:-1}, TCP pause/resume regression — all match NodeFollow-ups
writeBuffer/shutdownareENOTSUPstubs (stdin iswritable:false; full duplexnet.Socket({fd})write support is a separate effort)process.binding('pipe_wrap')props are non-enumerable (Object.keysreturns[];getOwnPropertyNamesworks) — cosmeticprocess.channel.fdcomparison would need IPC fd surfaced from VirtualMachine)