Skip to content

console, process.stdout/stderr: one stdio sink per fd; console writes through the stream like Node - #37128

Open
dylan-conway wants to merge 25 commits into
mainfrom
claude/node-console-stdout-compat-7d6001
Open

console, process.stdout/stderr: one stdio sink per fd; console writes through the stream like Node#37128
dylan-conway wants to merge 25 commits into
mainfrom
claude/node-console-stdout-compat-7d6001

Conversation

@dylan-conway

@dylan-conway dylan-conway commented Aug 7, 2026

Copy link
Copy Markdown
Member

What

console.* and process.stdout / process.stderr now share one sink per fd, and the global console delivers through process.stdout.write / process.stderr.write whenever doing so is observable — the way Node's console does (kWriteToConsolethis._stdout.write(chunk)), without ever building a JS string or entering JS when it isn't.

That single change is what all of these have in common:

before after (= node)
process.stdout.write = fn; console.log("x") not captured captured, one write() per console call, every method
console._stdout = writable / worker port streams ignored / worker swapped globalThis.console for a JS Console (lost Bun formatting, console.write) native console rebinds its stream
process.stdout.cork(); console.log() printed immediately held with the stream's other writes
void process.stdout; console.log(1 MiB) into a slow pipe everything past 64 KiB silently dropped, exit 0 (#36419, #21516) delivered
write(256K); console.log("A"); write(256K); console.log("B") into a slow pipe A lands inside the first 256K, B is lost call order
void process.stdout; spawnSync("cat", …, {stdio:"inherit"}) cat: stdout: Resource temporarily unavailable fine
fd 1/2 flags after touching process.stdout (file / tty) both gain O_NONBLOCK, never restored untouched. Only a FIFO goes O_NONBLOCK, only once a JS writer (process.stdout, Bun.stdout.writer()) exists, and it is restored at exit / on SIGINT·SIGTERM and cleared for children
process.stdout.write(big); process.exit() into a slow pipe truncated at 64 KiB fully written (node truncates; Bun already guaranteed this for console.log). Trade-off: if the reader is alive but never reads, exit now waits for it — signal handlers are reset to default first (node's ResetSignalHandlers), so ^C / SIGTERM still end it
writableLength / writableNeedDrain / cork() on stdio always 0 / false / no-op honest (writeFast bypass removed)
process.stdout.write(arrayBuffer / sharedArrayBuffer) accepted (Bun-only) still accepted (Bun-only; node throws) — kept deliberately
EPIPE on stdout with an 'error' listener console: never; write(): once one 'error' per failing call, syscall: 'write'
console.timeLog/timeEnd (#12031) stderr, [1.23ms] label stdout, label: 1.234ms, missing-label warnings
console.trace stdout writer, stderr colours (#19952) stderr
console.assert(false, "x") Assertion failed + message on separate write Assertion failed: x
console.clear() real isatty only _stdout.isTTY, ESC[1;1H ESC[0J through the stream (#8036)

| diagnostics_channel console.log/info/debug/warn/error | — | published (live args) while subscribed |
| Bun.stdout.writer() / Bun.file(1).writer() | new FileSink + new dup + flag flip per call | the shared sink (still coalesces small writes until flush()/end of tick); end()/close() flush and leave stdout usable |
| Bun.write(Bun.stdout, x) with stdout → file / pipe | thread pool, LIFO (reversed) | through the sink, in order; stays async (queues behind a slow pipe, loop keeps running) and resolves with its own byte count |

Fixes #36419, fixes #21516, fixes #19952, fixes #12031, fixes #8036. Supersedes #33560, #36066, #35956, #35949, #34347, #33508, #35064, #36226, #32423 and the console half of #35391.

How

  • FileSink::create_stdio / stdio_sink_for(vm, fd) (src/runtime/webcore/FileSink.rs): one FileSink per (VM, fd ∈ {1,2}) held by RareData, created natively on first use (no JS objects, no kqueue/epoll registration — the poll is registered only while a write is actually backed up, PosixStreamingWriter::start_lazy/unregister_poll). tty / file: blocking write(2), flags untouched. FIFO: blocking too until a JS writer handle is handed out (stdio_js), at which point it goes non-blocking so write() can return false/'drain' instead of stalling the loop — RWF_NOWAIT on Linux ≥ 6.4, O_NONBLOCK elsewhere. Socket: MSG_DONTWAIT. Windows: the existing SyncFile path. write_all_sync is the console path (drain whatever is queued, then write the caller's buffer, poll(POLLOUT) on EAGAIN); drain_sync runs before anything Bun itself prints to fd 1/2 (bun_sys::set_stdio_write_hook, so error reports, the test reporter and prompts can't overtake queued output), at exit, and on end(). process.stdout's stream, Bun.stdout.writer(), Bun.file(1|2).writer(), console.write and Bun.write(Bun.stdout) are all handles to it; the stream's _write uses a non-coalescing writeNow entry point (a syscall per call, like Node), the FileSink API keeps coalescing. Bun__ForceFileSinkToBeSynchronousForProcessObjectStdio (whose O_NONBLOCK undo was dead code — FileSink.fd was never set) is gone.
  • Console (src/jsc/ConsoleObject.rs): the two 4 KiB per-VM buffers and QuietWriterAdapters are replaced by formatting each message into a reusable scratch Vec (taken for the call, so a re-entrant console.log during formatting emits first, as in node) and delivering it: Bun__Process__consoleStream(fd) says whether user code could observe the write; empty → the sink (spilling every 64 KiB for huge messages, so memory stays bounded), otherwise → one JS string to the consoleObjectWriteToObservedStream builtin (a port of kWriteToConsole). A formatting exception propagates without writing, as in node.
  • Observability (BunProcess.{h,cpp}, ProcessObjectInternals.ts): Process records Bun's own stdio stream object and its pristine StructureID when the lazy process.stdout/stderr property is built. Per console call: never materialised → native (0 cost); structureID unchanged (and not a dictionary) → native (1 compare); changed → own write? JS : re-cache. console._stdout/_stderr are get/set accessors (node kBindStreamsLazy), bound on first use exactly like node — a process.stdout replaced before first use (data property or accessor) is honoured, after is not. The stdio stream instances flag themselves observed while corked / ending / holding chunks behind a backed-up write (so a native write can't jump their queue), and hand any still-buffered chunks to the sink on 'exit' (Writable.takeBuffered).
  • stdio streams are honest Writables (internal/fs/streams.ts): the own write = writeFast override is removed; _write/_writev bridge into the sink, completing synchronously when the fd took the chunk and on the sink's promise when it queued; decodeStrings: false; autoDestroy on so _undestroy semantics match node's dummyDestroy.
  • Nothing drops bytes on EAGAIN any more: fd_write_all_quiet (the Output writer every thread uses), non-pollable FileSink writes and the fd→fd copy loops poll and retry; open_for_writing no longer sets O_NONBLOCK on non-pollable fds; bun_initialize_process/bun_restore_stdio snapshot and restore the stdio O_NONBLOCK bits (node ResetStdio) — the SIGINT/SIGTERM restore handler is installed for FIFOs too now, but never over an inherited SIG_IGN; spawn clears the bit on our fds 0–2 handed to a child (libuv's rule); process.execve restores stdio first.
  • worker_threads: assigns process.std* (no defineProperty, which reified — and dup'd — the native streams it was replacing) and rebinds the native console; the globalThis.console = new Console(...) swap is deleted. bun test isolation drops the sink's JS wrapper with the outgoing global.

Performance

Release builds, macOS arm64, min of 6–8 runs, 300k iterations each; node 25.9 for scale.

case dest before after node
console.log("hello world") /dev/null 181 ms 182 ms 267 ms
file 369 375 475
pipe → cat 92 106 203
console.log("%s %d", …) pipe 145 150 282
console.log({…}) pipe 312 298 550
console.log("a", 1, "b", true) pipe 168 165 260
process.stdout.write("short\n") pipe 98 95 143
process.stdout.write(buf16) pipe 105 94 122
process.stdout.write(64 KiB) ×4.7k pipe 60 5.3 4.4
mixed log/write pipe 194 193 270
process.stdout.write("short\n") /dev/null / file 191 / 399 201 / 417 (+5 %: honest Writable state machine instead of the own writeFast) 243 / 476

User+sys CPU per console.log is within ~1% of before in every row (measured with process.cpuUsage()); the one wall-clock outlier is the tightest loop into a maximally fast pipe reader (+8–14% wall at equal CPU, from how the writer and cat interleave — with a slower reader the new build is faster). bench/log unchanged.

Behaviour changes called out

  • console.timeLog/timeEnd/trace fds and formats as above; test/js/web/console/console-timeLog.* and the console._stdout descriptor assertion in test/js/node/console/console.test.ts are corrected with node citations. console.time() on an existing label and timeEnd/timeLog on a missing one now emit process warnings (node parity) — hot-reload loops that re-time() a label will see them.
  • Queued stdout/stderr is drained on process.exit() / fatal error (documented in docs/guides/write-file/stdout.mdx); see the trade-off in the table. Not covered: SIGKILL / hard crashes (nothing can be), and the FIFO's O_NONBLOCK bit is likewise only restored on orderly exits and SIGINT/SIGTERM.
  • Existing test suites that spyOn(process.stdout, "write") will now see console output (that is the point) — call-count assertions written against the old behaviour change.
  • After the first spawn with inherited stdio, a FIFO stdout/stderr goes back to blocking for the rest of the process on macOS / Linux < 6.4 (the child needs it blocking and the description is shared — libuv makes the same choice): write() there stops returning false/'drain' and simply completes.
  • When Bun is spawned with socket stdio (node child_process, Bun.spawn), process.stdout.write is async with a queue (was force-sync); a producer that ignores false grows memory instead of being throttled.
  • The first console/stdio use dups fd 1/2 (one extra fd each; bun test does it up front), so fs.closeSync(1) no longer gives a downstream reader EOF while the process lives.
  • The SIGINT/SIGTERM stdio-restore handler is now installed when a stdio fd is a FIFO too (previously TTY only); an inherited SIG_IGN is still respected.
  • console.clear() follows node: only when the console's stdout isTTY, and ESC[1;1H ESC[0J (no scrollback clear) through the stream, instead of ESC[2J ESC[3J ESC[H to whichever of stdout/stderr had colours.
  • Under pipe backpressure a console.log issued while process.stdout still holds queued chunks is delivered through the stream (behind them, as in node) rather than synchronously; and a giant message on that path is materialised whole (node does the same) instead of spilling every 64 KiB.
  • Known, same as node: replacing write on the stream's prototype (or re-setPrototypeOf-ing process.stdout) is not observed by the console. Windows keeps its synchronous SyncFile path throughout; the fd-mode rows above are POSIX.

Tests

  • test/js/node/process/process-stdio.test.ts — "stdio sink": ordering under real pipe backpressure (parent-owned pipe(2), read only once full), every-writer ordering, exit drain via process.exit / throw / natural, uncaught-exception ordering, fd flags (parent, child-inherit, idle worker), console.log into a full O_NONBLOCK pipe, Bun.stdout.writer() identity/end(), writableLength/needDrain/cork/'drain', inherited write + encoding arg, write-after-end/_undestroy, EPIPE × {console, write} × {listener, none}.
  • test/js/node/console/console.test.ts — capture of every method through a replaced write, spyOn(process.stdout, "write"), _stdout setter, first-use binding both directions, cork, throwing/overflowing write, structure re-cache, clear()/_ignoreErrors, diagnostics_channel.
  • test/js/node/worker_threads/worker_threads.test.ts — worker keeps Bun's console surface/formatting; parent capture.
  • Newly vendored from node and passing: test-console-stdio-setters.js, test-console-clear.js, test-console-count.js.
  • All of the above fail on the previous build.

… through the stream like Node

console.* and process.stdout / process.stderr now share a single per-thread
FileSink per fd, and the global console delivers through the stream's
write() whenever user code could observe it (patched write, console._stdout
= x, corked/ended/backed-up stream) — Node's kWriteToConsole — while staying
fully native otherwise.

- FileSink stdio mode (create_stdio / stdio_sink_for / write_all_sync /
  drain_sync); Bun.stdout.writer(), Bun.file(1|2).writer(), console.write and
  Bun.write(Bun.stdout) are handles to the same sink; poll registered only
  while backed up; FIFO goes non-blocking only once a JS writer exists.
- Console formats each message into a scratch buffer and delivers it once
  (spilling every 64 KiB on the native path); timeLog/timeEnd move to stdout
  with Node's format and warnings, trace to stderr, assert gets the
  "Assertion failed" prefix inline, clear() honours _stdout.isTTY.
- process.stdout/stderr are honest Writables (no own write override):
  writableLength / needDrain / cork / 'drain' / EPIPE 'error' behave; chunks
  still buffered at exit are flushed; queued output is drained before exit,
  before fatal-error printing and before anything Bun prints to fd 1/2.
- console._stdout/_stderr are lazy get/set accessors; process._rawDebug;
  diagnostics_channel console.* channels; _ignoreErrors.
- EAGAIN never drops bytes: Output writer, non-pollable FileSink writes and
  fd copy loops poll and retry; O_NONBLOCK on stdio is snapshotted/restored
  at exit and on SIGINT/SIGTERM (never over an inherited SIG_IGN) and cleared
  on fds handed to children; open_for_writing no longer sets it on
  non-pollable fds.
- worker_threads rebinds the native console to the port streams instead of
  replacing globalThis.console.

Fixes #36419, fixes #21516, fixes #19952, fixes #12031, fixes #8036.
main gained its own process._rawDebug (#31831); keep that one and drop the
native implementation and its duplicate test from this branch.
@coderabbitai

coderabbitai Bot commented Aug 7, 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

Changes

Shared stdio sinks now coordinate stdout and stderr writes across console methods, Bun.stdout.writer(), Bun.write, workers, and process-exit paths. The implementation adds retrying I/O, buffering, polling, descriptor restoration, stream rebinding, diagnostics-channel publication, sticky errors, and synchronous draining. Tests cover ordering, backpressure, flushing, errors, timers, console routing, and workers.

Shared stdio and console output

Layer / File(s) Summary
Retrying writes and stdio synchronization
src/io/*, src/sys/*, src/runtime/node/node_fs.rs, src/runtime/webcore/blob/copy_file.rs, src/spawn_sys/*
Adds retrying I/O, stdio locking, poll management, descriptor handling, and blocking-mode restoration.
Shared FileSink implementation and lifecycle
src/runtime/webcore/FileSink.rs, src/runtime/webcore/Blob.rs, src/jsc/rare_data.rs, src/runtime/jsc_hooks.rs, src/codegen/generate-jssink.ts, src/runtime/cli/test_command.rs, src/jsc/bindings/c-bindings.cpp
Adds per-VM stdout and stderr sinks with shared wrappers, immediate writes, synchronous draining, sticky errors, cleanup, and descriptor restoration.
JavaScript streams and console behavior
src/js/builtins/*, src/js/internal/*, src/js/node/*, src/jsc/*, test/js/*, docs/guides/write-file/stdout.mdx
Connects Writable streams, console routing, workers, diagnostics channels, VM shutdown, timing behavior, and output documentation to the shared stdio pipeline.

Possibly related PRs

Suggested reviewers: robobun

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed The implementation and tests address all five linked issues: truncation, ordering, trace routing, timeEnd behavior, and console.clear output.
Out of Scope Changes check ✅ Passed The changes align with the stated stdio, console, worker, buffering, and regression-test objectives; no unrelated changes are evident.
Title check ✅ Passed The title clearly summarizes the primary change: shared per-file-descriptor stdio sinks and console writes routed through Node-compatible streams.
Description check ✅ Passed The description explains the changes, behavior differences, implementation details, performance, verification coverage, and related issues.

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

@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: 21

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/runtime/node/node_fs.rs (1)

4892-4972: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win

Fix the unbounded copy loop to use retrying read/write.

The bounded loop (lines 4892-4930) now retries EINTR/EAGAIN through Syscall::read_retrying and Syscall::write_retrying. The unbounded tail loop (lines 4931-4968), used when the source size is unknown (stat_size == 0), still calls plain Syscall::read/Syscall::write.

stat_size == 0 skips the bounded loop entirely and falls straight into the unbounded loop. read_write_fallback in src/runtime/webcore/blob/copy_file.rs calls this function with stat_size = 0 for the "unknown size" case, which covers FIFO/pipe copies such as bun run foo.js | bun run bar.js. A pipe copy that hits EAGAIN or EINTR in this loop returns a hard error instead of retrying, which reproduces the exact backpressure/truncation failure this PR targets.

Use the retrying calls in the unbounded loop too.

🐛 Proposed fix for the unbounded copy loop
         if !broke {
             'outer: loop {
-                let amt = match Syscall::read(src_fd, buf) {
+                let amt = match Syscall::read_retrying(src_fd, buf) {
                     Ok(result) => result,
                     Err(err) => {
                         return Err(if !src.is_empty() {
                             err.with_path(src)
                         } else {
                             err
                         });
                     }
                 };
                 // we don't know the size
                 // so we just go forever until we get an EOF
                 if amt == 0 {
                     break;
                 }
                 *wrote += amt as u64;
 
                 let mut slice = &buf[..amt];
                 while !slice.is_empty() {
-                    let written = match Syscall::write(dest_fd, slice) {
+                    let written = match Syscall::write_retrying(dest_fd, slice) {
                         Ok(result) => result,
                         Err(err) => {
                             return Err(if !dest.is_empty() {
                                 err.with_path(dest)
                             } else {
                                 err
                             });
                         }
                     };
                     slice = &slice[written..];
                     if written == 0 {
                         break 'outer;
                     }
                 }
             }
         }
🤖 Prompt for 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.

In `@src/runtime/node/node_fs.rs` around lines 4892 - 4972, Update the unbounded
tail loop in the copy function, identified by the `if !broke` branch, to use
`Syscall::read_retrying` and `Syscall::write_retrying` instead of the plain
read/write calls. Preserve the existing EOF, zero-write, byte-counting, and
path-aware error behavior.
🤖 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/io/stdio_lock.rs`:
- Around line 43-60: Make StdioLock explicitly !Send by adding a thread-affine
marker field, such as PhantomData tied to a non-Send type, and update acquire to
initialize it while preserving the existing Option<usize> slot behavior and Drop
logic.

In `@src/js/builtins/ConsoleObject.ts`:
- Around line 132-140: Update the observed-stream branch in Console.write to
route every chunk through writeToObservedStream, preserving the required chunk
representation while retaining the existing byte-count calculation. Ensure
synchronous write failures and emitted stream errors are swallowed using the
same temporary error-listener guard as writeToObservedStream, extending that
helper only if necessary to forward binary chunks unchanged.

In `@src/js/builtins/ProcessObjectInternals.ts`:
- Around line 93-97: Add a comment immediately above the emitClose check in the
_undestroy() flow explaining that _undestroy() resets the state before normal
close handling, so the next-tick "close" emission remains observable to finished
and pipeline when emitClose is false. Preserve the existing conditional and
event-emission behavior.

In `@src/js/internal/fs/streams.ts`:
- Around line 605-634: Update the FileSink fast-path write handling in
underscoreWriteFast and its corresponding _writev path to increment
this.bytesWritten by the number of bytes successfully written, including after
asynchronous completion. Preserve the counter for fast-path streams such as
process.stdout, process.stderr, and tty.WriteStream, and do not modify it when a
write fails.

In `@src/js/internal/streams/writable.ts`:
- Line 413: Remove the takeBuffered assignment from the default Writable export
in internal/streams/writable, preserving Writable as the constructor object.
Expose takeBuffered through a separate internal export path, then update
ProcessObjectInternals.ts to import it from that path while leaving all
node:stream and default Writable consumers unchanged.

In `@src/js/node/worker_threads.ts`:
- Around line 454-462: Update the stdout and stderr setup in the worker stream
initialization to pass the local writable values directly to setConsoleStream
after creating them, rather than reading process.stdout or process.stderr back.
Keep the existing property assignments and slot mappings unchanged.

In `@src/jsc/bindings/BunProcess.h`:
- Around line 78-81: Update the comment above Process::consoleStream() to remove
the claim that it never runs user code and accurately state that resolving the
console stream may invoke a JavaScript accessor and throw. Keep the declaration
and consoleStreamIsResolved() behavior unchanged.

In `@src/jsc/ConsoleObject.rs`:
- Around line 329-344: Verify whether bun_io::StdioLock::acquire is reentrant
for the same thread. If it is not, update emit so the lock is not held while
f(&mut writer) executes: release it before formatting and reacquire it around
deliver_to; otherwise preserve the current behavior only when recursive
acquisition is explicitly supported.
- Around line 306-323: Update the doc comment immediately above the
message-formatting function to state that formatting failures stop further
writes but may leave previously spilled partial output, including a truncated
message without a terminating newline, while the exception still propagates.
Keep the existing bounded-memory spilling behavior unchanged.
- Around line 6216-6226: Update the error handling around the `result` from
`format2` in `_rawDebug` so failed formatting does not write the partially
populated `buf` to stderr; only perform the `write_all_retrying` call and
`ConsoleObject::put_scratch` on successful formatting, matching `emit`’s discard
behavior while preserving the existing exception propagation.

In `@src/jsc/VirtualMachine.rs`:
- Around line 1471-1480: Avoid overlapping mutable borrows at both FFI call
sites: in src/jsc/VirtualMachine.rs lines 1471-1480 within drain_stdio, capture
a raw pointer with core::ptr::from_mut::<VirtualMachine>(self) before the guard
body and pass it to __bun_stdio_sink_drain; in src/jsc/VirtualMachine.rs lines
4656-4663 within swap_global_for_test_isolation, capture and pass the equivalent
raw pointer to __bun_stdio_sink_release_js.
- Around line 1512-1516: Update global_exit() to drain both stdout and stderr
sinks before calling release_js_handles(), ensuring queued output from direct
exits is flushed. Do not rely on on_exit() or a single fd-specific drain; invoke
the sink-draining behavior for each stream before releasing JS handles.

In `@src/runtime/webcore/Blob.rs`:
- Around line 1802-1811: Update the comment above the shared stdio wrapper
branch in the Blob writer creation flow to explicitly document that writer
options are intentionally ignored for file descriptors 1 and 2 because the
shared sink cannot support per-handle options. Keep the existing return behavior
unchanged.
- Around line 5093-5104: Update the synchronous settlement branch in the
`to_result` flow to release the pending keep-alive reference after
`webcore::FileSink::drain_sync(sink)` completes, including the successful drain
path while preserving existing error propagation. Ensure the cleanup occurs for
settled pending writes so `release_stdio_sinks` can reclaim the sink and
duplicated descriptor.

In `@src/runtime/webcore/FileSink.rs`:
- Around line 731-737: Update the start_lazy error path in the writer.with_mut
closure to clear the writer’s stored fd ownership before calling fd.close().
Preserve the existing invalidation, dereference, and error return flow so sink
teardown cannot close the descriptor again.
- Around line 893-910: Update the write loop around sys::write_retrying in
FileSink’s sink write path so Ok(0) with a non-empty bytes slice is treated as
an explicit failure, not a successful break. Propagate the resulting error
through stdio_latch_error and writer.fail, matching the existing Err(e) path, so
__bun_stdio_sink_write reports the stalled write instead of silent truncation.
- Around line 2121-2144: Update write_now to use the same nullish-argument error
behavior as FileSink.prototype.write: ensure null and undefined are reported
with STREAM_NULL_VALUES, while preserving INVALID_ARG_TYPE for other unsupported
values. Reuse the existing validation or error-handling path in write_js_value
or the corresponding FileSink write implementation rather than introducing
divergent logic.

In `@src/spawn_sys/posix_spawn.rs`:
- Around line 618-631: Update the Dup2 handling in the spawn action loop so
clearing O_NONBLOCK affects only the child’s inherited descriptor, not the
parent stdio descriptor and its cached poll state. Preserve non-blocking
behavior for parent PipeWriter stdout/stderr paths, using the existing
descriptor-duplication or metadata-synchronization mechanism where appropriate.

In `@test/js/node/process/process-stdio.test.ts`:
- Around line 213-265: Update runWithSlowStdout to create the spawned process
with await using, ensuring its async disposal terminates the child on every exit
path, including errors and assertion failures. Keep the existing stdout polling,
stderr collection, exit-code handling, and descriptor cleanup unchanged.
- Around line 172-202: Guard the POSIX-only fixture initialization in the “stdio
sink” suite before calling tempDirWithFiles or cc(). Ensure non-POSIX
environments return before compiling fdutil.c, while preserving the existing
isPosix skip behavior for POSIX tests.

In `@test/js/node/worker_threads/worker_threads.test.ts`:
- Around line 1779-1788: Update the worker test around the worker exit await to
also await the `end` events for both `worker.stdout` and `worker.stderr`,
together with `exit`, before asserting `{ out, err }`. Preserve the existing
data handlers and expected output while ensuring all stream chunks are received
deterministically.

---

Outside diff comments:
In `@src/runtime/node/node_fs.rs`:
- Around line 4892-4972: Update the unbounded tail loop in the copy function,
identified by the `if !broke` branch, to use `Syscall::read_retrying` and
`Syscall::write_retrying` instead of the plain read/write calls. Preserve the
existing EOF, zero-write, byte-counting, and path-aware error behavior.
🪄 Autofix

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: 65425516-cd4f-40f6-b9a9-2a5306674edc

📥 Commits

Reviewing files that changed from the base of the PR and between 45ee955 and 90e0621.

📒 Files selected for processing (42)
  • docs/guides/write-file/stdout.mdx
  • src/bun_core/output.rs
  • src/codegen/generate-jssink.ts
  • src/io/PipeWriter.rs
  • src/io/lib.rs
  • src/io/openForWriting.rs
  • src/io/stdio_lock.rs
  • src/js/builtins/BunBuiltinNames.h
  • src/js/builtins/ConsoleObject.ts
  • src/js/builtins/ProcessObjectInternals.ts
  • src/js/internal/fs/streams.ts
  • src/js/internal/streams/writable.ts
  • src/js/node/diagnostics_channel.ts
  • src/js/node/worker_threads.ts
  • src/jsc/ConsoleObject.rs
  • src/jsc/VirtualMachine.rs
  • src/jsc/bindings/BunProcess.cpp
  • src/jsc/bindings/BunProcess.h
  • src/jsc/bindings/ConsoleObject.cpp
  • src/jsc/bindings/ZigGlobalObject.cpp
  • src/jsc/bindings/c-bindings.cpp
  • src/jsc/rare_data.rs
  • src/runtime/cli/test_command.rs
  • src/runtime/jsc_hooks.rs
  • src/runtime/node/node_fs.rs
  • src/runtime/webcore/Blob.rs
  • src/runtime/webcore/FileSink.rs
  • src/runtime/webcore/blob/copy_file.rs
  • src/spawn_sys/posix_spawn.rs
  • src/sys/lib.rs
  • src/sys/sys_uv.rs
  • test/js/node/console/console.test.ts
  • test/js/node/process/process-stdio.test.ts
  • test/js/node/test/parallel/test-console-clear.js
  • test/js/node/test/parallel/test-console-count.js
  • test/js/node/test/parallel/test-console-stdio-setters.js
  • test/js/node/test/parallel/test-process-raw-debug.js
  • test/js/node/worker_threads/worker_threads.test.ts
  • test/js/web/console/console-timeLog.expected.txt
  • test/js/web/console/console-timeLog.test.ts
  • test/js/web/workers/structured-clone.test.ts
  • test/js/web/workers/structuredClone-classes.test.ts

Comment thread src/io/stdio_lock.rs Outdated
Comment thread src/js/builtins/ConsoleObject.ts
Comment thread src/js/builtins/ProcessObjectInternals.ts
Comment thread src/js/internal/fs/streams.ts
Comment thread src/js/internal/streams/writable.ts Outdated
Comment thread src/runtime/webcore/FileSink.rs
Comment thread src/spawn_sys/posix_spawn.rs
Comment thread test/js/node/process/process-stdio.test.ts Outdated
Comment thread test/js/node/process/process-stdio.test.ts
Comment thread test/js/node/worker_threads/worker_threads.test.ts
- StdioLock is !Send; consoleStream() header comment reflects that first
  resolution may run a user getter; emit() doc notes spilled messages can
  be torn by a formatting exception
- FileSink: Ok(0) from write(2) is an error, not a silent stop; start_lazy
  failure leaves fd ownership with the writer; stdio deinit drops the
  backpressure keep-alive ref; writeNow reports STREAM_NULL_VALUES like
  write(); a spawn that put fd 1/2 back into blocking mode is noticed
  (STDIO_MADE_BLOCKING) so the sink stops assuming EAGAIN
- drain stdio at the top of global_exit(); pass raw VM pointers to the
  sink externs
- Console.write on an observed stream uses the console's ignore-errors
  policy; fast-path streams maintain bytesWritten; takeBuffered is a
  private name, not a public Writable static; worker_threads binds the
  local port stream
- tests: guard POSIX-only fixture setup, dispose the child in
  runWithSlowStdout, await worker stdout/stderr end

@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

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (3)
src/js/internal/fs/streams.ts (1)

615-618: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

Resolve the file path before creating the FileSink.

Bun.file(this.path).writer() can receive a relative path. Resolve and store an absolute path before this file operation.

As per coding guidelines, use absolute paths in file operations.

🤖 Prompt for 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.

In `@src/js/internal/fs/streams.ts` around lines 615 - 618, Update the file-sink
initialization in the write-stream path around kWriteStreamFastPath to resolve
this.path to an absolute path and store the resolved value before calling
Bun.file(...).writer(); use that absolute path for the FileSink creation while
preserving the existing fd assignment.

Source: Coding guidelines

src/jsc/bindings/BunProcess.h (1)

55-63: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Add GC-stress coverage for persistent console handles.

Process::visitChildrenImpl already traces m_consoleStream, m_stdioStream, and m_consolePublish. Existing tests cover rebinding and diagnostics publishing, but do not force GC during these operations. Add a focused GC test for these paths.

🤖 Prompt for 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.

In `@src/jsc/bindings/BunProcess.h` around lines 55 - 63, Add focused GC-stress
coverage for the persistent console handles used by Process::visitChildrenImpl,
exercising m_consoleStream, m_stdioStream, and m_consolePublish during console
rebinding and diagnostics publishing. Force garbage collection while performing
these operations, then verify the handles remain valid and behavior is
preserved.

Source: Coding guidelines

src/runtime/webcore/FileSink.rs (1)

908-925: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Use poll-before-write for direct console output after stdio becomes blocking.

After spawn_z changes the shared descriptor mode, this loop calls sys::write_retrying before any readiness wait. If the pipe is full, that first write can block the JS thread instead of returning EAGAIN. Apply the blocking-pipe strategy before each direct write in this mode, and add a slow-pipe regression test that writes through console.* after an inherited-stdio spawn.

🤖 Prompt for 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.

In `@src/runtime/webcore/FileSink.rs` around lines 908 - 925, Update the
non-Windows direct-output loop in the relevant FileSink write method to poll for
write readiness before every sys::write_retrying call after the descriptor
becomes blocking, preserving existing error and zero-progress handling. Add a
regression test covering console.* output through an inherited-stdio spawn with
a slow/full pipe.
🤖 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/builtins/ConsoleObject.ts`:
- Around line 137-150: Update the emitter setup around isEmitter to also
validate that observed.removeListener is callable, and track whether the noop
error listener was actually added. In the finally block, call removeListener
only when that tracked state is true, preventing custom streams lacking
removeListener from masking the original write failure.

In `@src/sys/lib.rs`:
- Around line 7452-7456: The stdio blocking setup currently discards errors,
allowing spawn to continue after failure. Update make_stdio_blocking in
src/sys/lib.rs:7452-7456 to return sys::Result<()> and propagate
update_nonblocking failures while marking success only after completion; update
the spawn path in src/spawn_sys/posix_spawn.rs:624-630 to propagate that result
before creating the child.

---

Outside diff comments:
In `@src/js/internal/fs/streams.ts`:
- Around line 615-618: Update the file-sink initialization in the write-stream
path around kWriteStreamFastPath to resolve this.path to an absolute path and
store the resolved value before calling Bun.file(...).writer(); use that
absolute path for the FileSink creation while preserving the existing fd
assignment.

In `@src/jsc/bindings/BunProcess.h`:
- Around line 55-63: Add focused GC-stress coverage for the persistent console
handles used by Process::visitChildrenImpl, exercising m_consoleStream,
m_stdioStream, and m_consolePublish during console rebinding and diagnostics
publishing. Force garbage collection while performing these operations, then
verify the handles remain valid and behavior is preserved.

In `@src/runtime/webcore/FileSink.rs`:
- Around line 908-925: Update the non-Windows direct-output loop in the relevant
FileSink write method to poll for write readiness before every
sys::write_retrying call after the descriptor becomes blocking, preserving
existing error and zero-progress handling. Add a regression test covering
console.* output through an inherited-stdio spawn with a slow/full pipe.
🪄 Autofix

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: 0dd86ad1-06e0-47c7-ad4c-68ce238d6ee5

📥 Commits

Reviewing files that changed from the base of the PR and between 584ddb3 and 40db416.

📒 Files selected for processing (17)
  • src/io/lib.rs
  • src/io/stdio_lock.rs
  • src/js/builtins/BunBuiltinNames.h
  • src/js/builtins/ConsoleObject.ts
  • src/js/builtins/ProcessObjectInternals.ts
  • src/js/internal/fs/streams.ts
  • src/js/internal/streams/writable.ts
  • src/js/node/worker_threads.ts
  • src/jsc/ConsoleObject.rs
  • src/jsc/VirtualMachine.rs
  • src/jsc/bindings/BunProcess.h
  • src/runtime/webcore/Blob.rs
  • src/runtime/webcore/FileSink.rs
  • src/spawn_sys/posix_spawn.rs
  • src/sys/lib.rs
  • test/js/node/process/process-stdio.test.ts
  • test/js/node/worker_threads/worker_threads.test.ts

Comment thread src/js/builtins/ConsoleObject.ts Outdated
Comment thread src/sys/lib.rs
- Bun__Process__consoleStream / consoleStreamObject use a top-level scope
  (the Rust caller learns of a throw via the out-param); consoleStreamForGetter
  keeps one function-wide ThrowScope with RELEASE_AND_RETURN
- VirtualMachine::destroy() releases the stdio sinks itself, for VMs torn
  down without release_js_handles() (bake)
- node:fs copyFile fallback: the unknown-size loop retries EAGAIN/EINTR too
- tests: stdio-sink suite reads through a mkfifo instead of bun:ffi in the
  runner and skips the cc()-using cases under ASAN; strip ANSI where the
  runner's stdout may be a colour TTY; timeLog warning test uses
  --no-warnings; write-after-end (piped) expectation matches node
  (dummyDestroy -> _undestroy leaves the stream writable)
…O_MADE_BLOCKING; Console.write only removes the error guard it added
@dylan-conway

Copy link
Copy Markdown
Member Author

Re: the outside-diff note on src/js/internal/fs/streams.ts (Bun.file(this.path).writer() with a relative path): not changed — that lazy-open line predates this PR and intentionally resolves a relative path against the cwd at open time, exactly as fs.createWriteStream(relativePath) / fs.open do; making it absolute at construction would change when the cwd is sampled.

🤖 Addressed by Claude Code

@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/runtime/webcore/FileSink.rs:1421-1425 — For Wrote(n) | Done(n), write_with sets accepted = n directly, but when the writer already holds coalesced bytes (e.g. Bun.stdout.writer().write("hello") earlier in the same tick on a pollable stdio), write_impl(may_buffer=false) appends the new chunk to outgoing and try_write_newly_buffered_data() flushes the whole buffer — so n includes the prior bytes and Bun.write(Bun.stdout, "abc") resolves with 8 instead of 3. Compute accepted for Wrote/Done the same way as Pending (e.g. (n as u64).saturating_sub(buffered_before as u64), or reuse bytes_accepted's (buffered_after + written) - buffered_before for every variant); the new test only exercises the Pending branch via a 256K backpressure fill so it doesn't catch this.

    Extended reasoning...

    What the bug is

    FileSink::write_with (introduced in this PR) returns (streams::Writable, u64) where the second element — accepted — is meant to be the number of encoded bytes this call handed the writer. For WriteResult::Pending it correctly computes that via bytes_accepted() ((buffered_after + written) - buffered_before), but for Wrote(n) | Done(n) it just uses n directly:

    let accepted = match rc {
        WriteResult::Pending(_) => self.bytes_accepted(buffered_before, &rc),
        WriteResult::Wrote(n) | WriteResult::Done(n) => n as u64,
        WriteResult::Err(_) => 0,
    };

    The .1 value is what write_file_internal (Blob.rs) resolves the Bun.write(Bun.stdout | Bun.stderr, data) promise with — the PR's stated "resolves with its own byte count" contract. When n includes bytes that were already sitting in the writer's coalesce buffer before this call, the resolved value is over-counted.

    The specific code path

    On a stdio pipe/socket, create_stdio sets force_sync = false and poll_flushes_buffer = false. So:

    1. Bun.stdout.writer().write("hello")FileSink::writewrite_with(|w| w.write(...))write_impl(buf, may_buffer=true). should_buffer(5) is true (small write, PipeWriter.rs:735), so the 5 bytes are appended to outgoing and Wrote(5) is returned. Because poll_flushes_buffer=false, no poll is registered; the bytes stay in outgoing until the end-of-tick AutoFlusher.

    2. In the same tick, Bun.write(Bun.stdout, "abc")write_file_internalwrite_js_value(global, data, /*now=*/true)write_with(|w| w.write_latin1_now(b"abc")). buffered_before = 5. write_latin1_impl(may_buffer=false) sees all-ASCII → write_impl(b"abc", false).

    3. In write_impl (PipeWriter.rs:965), may_buffer=false skips the coalesce branch, and self.outgoing.size() == 5 > 0, so it appends "abc" (outgoing now 8 bytes) and calls try_write_newly_buffered_data(). That does try_write(force_sync=false, outgoing.slice()) — writing all 8 bytes to a fast fd — and at line 903-906 resets outgoing and returns Wrote(8).

    4. Back in write_with, rc = Wrote(8)accepted = 8. to_result returns Owned(8) (via the WroteTemporary/Owned mapping in to_result).

    5. In write_file_internal, result is not Pending (so no uncredit_pending), drain_sync is a no-op (buffer already empty), and the promise resolves with accepted = 8 — not 3.

    The same over-count applies to the write_utf16_now / non-ASCII write_latin1_now paths via maybe_write_newly_buffered_data(buf_len, false)try_write_newly_buffered_data(), which returns Wrote(before_len + buf_len) on a full drain.

    Why existing code doesn't prevent it

    • bytes_accepted() handles exactly this case ((buffered_after + written).saturating_sub(buffered_before)), but it is only called for Pending — its body even early-returns 0 for anything else.
    • uncredit_pending in write_file_internal only runs for the Pending variant.
    • The new test at process-stdio.test.ts ("Bun.write(Bun.stdout, x) resolves with its own byte count …") first writes 256K via process.stdout.write, which backs the pipe up so the Bun.write call takes the Pending branch. The fully-drained Wrote path with prior coalesced bytes is not covered.
    • Other consumers of write_with (FileSink.write/write_latin1/write_utf16 and the writeNow hostfn) discard .1; only the new Bun.write(Bun.stdout|stderr, ..) routing (also introduced in this PR) reads it.

    Impact

    User-visible incorrect return value from a documented API. The bytes on the fd are correct and in order; only the number the promise resolves with is wrong. Anyone relying on the resolved value (e.g. summing bytes written, progress reporting, or asserting n === chunk.length) will see a value larger than the chunk they passed whenever they mix Bun.stdout.writer().write() (coalesced) with Bun.write(Bun.stdout, ..) in the same tick on piped stdio. This directly contradicts the PR's stated contract in the description table ("resolves with its own byte count").

    Fix

    Compute accepted for Wrote/Done with the same delta the Pending branch uses. The simplest change is:

    let buffered_before = self.writer.get().buffered_len();
    let rc = self.writer.with_mut(f);
    let accepted = match rc {
        WriteResult::Wrote(n) | WriteResult::Done(n) | WriteResult::Pending(n) => {
            let buffered_after = self.writer.get().buffered_len();
            (buffered_after + n).saturating_sub(buffered_before) as u64
        }
        WriteResult::Err(_) => 0,
    };

    (equivalently, generalize bytes_accepted to take the written count from any variant and call it for all three).

  • 🔴 src/jsc/bindings/BunProcess.cpp:1885-1887bun_restore_stdio() runs before the execve attempt, but when execve fails and control returns to JS the failure path only restores FD_CLOEXEC (via savedStdioFlags / F_SETFD) — it never undoes the termios and O_NONBLOCK reset. So a program that had process.stdin.setRawMode(true) and then calls process.execve('/nonexistent') catches the throw with its terminal already put back into cooked mode (while stream.isRaw is still true), and a stdio FIFO's O_NONBLOCK bit is cleared while the sink's internal nonblocking flag stays set. The failure path should snapshot/restore F_GETFL and termios around bun_restore_stdio(), or use a variant that only touches O_NONBLOCK here.

    Extended reasoning...

    What the bug is

    This PR adds a bun_restore_stdio() call at BunProcess.cpp:1887 inside Process_functionExecve, before the execve(2) / posix_spawn attempt. bun_restore_stdio() does two things (c-bindings.cpp): it calls bun_restore_stdio_nonblock() to reset each stdio fd's O_NONBLOCK bit to the startup snapshot, and it tcsetattr()s each TTY stdio fd back to the termios captured at bun_initialize_process. That's exactly right when execve succeeds — the new image should inherit stdio in the state we found it.

    But execve can fail (ENOENT, EACCES, …), and this function is explicitly designed for that: the failure path at lines ~1957–1972 restores the FD_CLOEXEC flags it cleared (fcntl(fd, F_SETFD, savedStdioFlags[fd])), restores the signal mask, and throws back to JS with the comment "the original image is still running… throw back to JS so the caller can handle it and recover". The failure path does not undo what bun_restore_stdio() did — it never touches F_GETFL or termios.

    Code path that triggers it

    1. A TUI/shell-like app calls process.stdin.setRawMode(true). This puts the TTY into raw mode via tcsetattr and sets bun_stdio_modified[0] = 1 (so bun_restore_stdio() will act on fd 0 even in the pipeline-producer case).
    2. The app calls process.execve('/nonexistent', …) (e.g. "run external command" with a bad path).
    3. Process_functionExecve reaches line 1887 and calls bun_restore_stdio(), which tcsetattrs fd 0 back to the cooked termios captured at process startup.
    4. execve(2) fails with ENOENT.
    5. The failure path restores FD_CLOEXEC and the signal mask, then throws.
    6. The JS caller catches the error and continues — but the terminal is now cooked while process.stdin.isRaw is still true. Keystrokes echo, arrow keys send escape sequences the app never sees, etc.

    The O_NONBLOCK half is analogous: if the stdio sink had set O_NONBLOCK on a FIFO stdout (via stdio_go_nonblocking(), which this PR added), bun_restore_stdio_nonblock() clears it on the shared open file description (the sink's dup'd fd shares that description), while FileSink::nonblocking and FilePollFlag::Nonblocking remain set — so stdio_go_nonblocking() will never re-arm it (it early-returns on self.nonblocking.get()), and subsequent process.stdout.write() on a full pipe blocks the JS thread instead of returning false/queueing.

    Why existing code doesn't prevent it

    The pre-existing savedStdioFlags restore uses F_GETFD/F_SETFD, which is the file-descriptor flag word (only FD_CLOEXEC). Termios and O_NONBLOCK (which is F_GETFL/F_SETFL, the file-status flag word on the shared description) are a different namespace and are not touched by that restore. Before this PR nothing on the execve path modified them, so there was nothing to undo; now there is.

    Impact

    A concrete regression on a recovery path the code itself documents as recoverable. The trigger is narrow (raw-mode TTY or non-blocking FIFO stdio + process.execve failure + caller catches and continues), but it's exactly the shape of a shell/TUI app offering "run external command" — the class of program most likely to use both raw mode and process.execve. No crash or data loss; the failure mode is a silently mis-configured terminal (very confusing to debug) and, for the O_NONBLOCK case, an event loop that blocks on stdout backpressure where before it queued.

    Fix

    Snapshot fcntl(fd, F_GETFL) for fds 0–2 and (for TTY fds) tcgetattr before calling bun_restore_stdio(), and restore both on the failure path alongside the existing F_SETFD restore. Alternatively, split out a bun_restore_stdio_for_execve() that only calls bun_restore_stdio_nonblock() and skips termios (execve doesn't need termios reset — the new image inherits the TTY device state regardless, and if it wants cooked mode it can set it itself; Node's ResetStdio() on execve only handles the flag bits). Either approach keeps the failure path's existing "caller can handle it and recover" contract intact.

Comment thread src/runtime/webcore/Blob.rs
…total

A write that also pushes out bytes an earlier Bun.stdout.writer().write()
had coalesced returned their combined length, so Bun.write(Bun.stdout, s)
could over-report. Callers that need the count (Bun.write) now supply the
input's UTF-8 length; the pending credit used by to_result is unchanged.
Also puts write_file_internal's doc comment back on write_file_internal.
@dylan-conway

Copy link
Copy Markdown
Member Author

Re: the write_with accepted-count finding (FileSink.rs, outside the diff): fixed in e6f489eWrote(n)/Done(n) can include bytes coalesced by an earlier Bun.stdout.writer().write() (or be a partial write with the rest queued), so callers that need a count (Bun.write) now pass the input's UTF-8 length and get exactly that back; the pending credit to_result uses is computed as before. The stdio-sink test now covers the coalesced case (writer().write('[coalesced]') then Bun.write(Bun.stdout, 'hé!\\n') → 5).

🤖 Addressed by Claude Code

@robobun

robobun commented Aug 7, 2026

Copy link
Copy Markdown
Collaborator
Updated 7:21 AM PT - Aug 8th, 2026

@dylan-conway, your commit f07b71e has 1 failures in Build #90586 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 37128

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

bun-37128 --bun

…sed behind our back at teardown

- Named FIFOs don't get FMODE_NOWAIT, so pwritev2(RWF_NOWAIT) on them is
  EOPNOTSUPP (and flips RWFFlagSupport off process-wide); a FIFO stdout on
  Linux >= 6.4 therefore fell back to blocking writes. Probe fstatfs for
  PIPEFS_MAGIC and use O_NONBLOCK for anything else.
- If user code closed our dup'd stdio fd by number (or something reused and
  closed that number), teardown detaches instead of close()ing it again.
Comment thread src/js/builtins/ConsoleObject.ts Outdated
Comment thread src/runtime/webcore/FileSink.rs
Comment thread src/jsc/bindings/BunProcess.cpp
Comment thread src/js/node/diagnostics_channel.ts Outdated
…a pre-execve stdio restore; primordial indexOf

- on_auto_flush's error arm was the one terminal-error site not routed
  through the stdio latch, so an EPIPE hit while autoflushing a coalesced
  Bun.stdout.writer().write() closed the shared sink silently
- bun_restore_stdio_nonblock records the fds it puts back to blocking
  (Bun__stdioMadeBlocking), so a process that survives a failed execve
  stops expecting EAGAIN on them
- diagnostics_channel: ArrayPrototypeIndexOf.$call like the rest of the file

@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.

No further findings after 7bd8448 — the autoflush stdio-latch, execve O_NONBLOCK bookkeeping, and primordial indexOf fixes all landed as described. Deferring to human review given the scope: this rewires console/stdio delivery end-to-end across FileSink lifetime/refcounting, JSC Process state, VM teardown ordering, signal-time fd-flag restoration, and per-platform write strategies, with documented user-visible behaviour changes.

What was reviewed:

  • Every terminal-error site on stdio sinks now routes through stdio_latch_error (incl. on_auto_flush, drain_sync, write_all_sync, to_result, flush_from_js, on_error) — no path leaves the sink done with stdio_error=None.
  • The ConsoleWriter spill path holds StdioLock for the whole message and reports errors once via report_stdio_error; the ruled-out "N+1 'error' events" candidate doesn't hold because only the final deliver_to return propagates.
  • bun_restore_stdio_nonblock now sets STDIO_MADE_BLOCKING per fd flipped, so a failed execve leaves sinks in a state refresh_stdio_mode will downgrade.
  • RareData sink ownership: +1 stored by stdio_sink_for, released in release_stdio_sinks before loop/JSC teardown; destroy() and global_exit() cover the paths that skip on_exit().
Extended reasoning...

Overview

This PR unifies all stdout/stderr output on a JS thread — console.*, process.stdout/stderr, Bun.stdout.writer(), Bun.write(Bun.stdout), console.write — through one per-VM FileSink per fd, held by RareData. The console formats natively into a scratch buffer and either writes straight to the sink or, when user code could observe it (patched write, corked, custom _stdout), delivers one JS string through a port of Node's kWriteToConsole. It touches 42 files across Rust (FileSink.rs +~460 lines, ConsoleObject.rs largely rewritten, PipeWriter.rs, sys/lib.rs, VirtualMachine.rs, rare_data.rs), C++ (BunProcess.{h,cpp}, ConsoleObject.cpp, ZigGlobalObject.cpp, c-bindings.cpp), builtin JS (ConsoleObject.ts, ProcessObjectInternals.ts, streams.ts, writable.ts, worker_threads.ts, diagnostics_channel.ts), codegen, spawn, and docs.

Security risks

No new attack surface in the security sense — this is stdio plumbing. The relevant risk classes are memory safety (extensive raw-pointer / unsafe work in FileSink.rs around the shared-sink lifetime, JsCell borrows during drain_sync/write_all_sync, and the Strong root for stdio_js), signal-handler safety (bun_restore_stdio runs from a SIGINT/SIGTERM handler and now also from pre-execve), and cross-thread correctness (the new StdioLock reentrant mutex, the STDIO_MADE_BLOCKING atomic read from any thread that spawns). None of these are user-input validation issues.

Level of scrutiny

High. This is hot-path, production-critical code that every Bun program exercises on every console.log. It changes fd flag management (O_NONBLOCK on shared open file descriptions affects parent shells and sibling processes), adds a blocking exit-time drain, rewires Writable semantics for process.stdout, alters signal-handler installation, and introduces documented behaviour changes (ArrayBuffer rejection, console.timeLog fd/format, console.clear escape sequence). The FileSink refcounting across RareData ↔ JS wrapper ↔ keep-alive, and the ordering of release_stdio_sinks vs. loop/JSC teardown, are exactly the kind of lifetime work that benefits from a maintainer's read.

Other factors

Test coverage is extensive and well-designed (real FIFO backpressure via parent-owned mkfifo, EPIPE matrix, fd-flag probes via compiled C, exit-drain via all three paths, worker rebinding), and four Node parallel tests are newly vendored. All five prior review findings across three passes were addressed with targeted commits and verified. The one candidate raised this run (spilled >64 KiB console message to a broken pipe emitting N+1 'error' events) was ruled out: __bun_stdio_sink_write calls report_stdio_error once per write_all_sync invocation, and after the first failure stdio_error is latched so subsequent spill chunks fail fast without re-reporting — but the outer emit() loop calls __bun_stdio_sink_write once per spill and once for the tail via deliver_to, each of which reports; however stdio_latch_error makes every call after the first return the same latched error, and report_stdio_errorreportStdioSinkErrorstream.destroy(err) on an already-destroying stream is a no-op inside one tick, so the observable 'error' count is bounded by the destroy/undestroy cycle, matching the tested per-turn behaviour. Given the breadth, cross-platform branches (Windows SyncFile, Linux RWF_NOWAIT ≥6.4, macOS), and the number of interacting subsystems, this should not merge on automated review alone.

…AT, the one bound the native path already has

@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.

No new findings this pass; all earlier inline comments (autoflush stdio latch, execve stdio restore, writeToObservedStream guard, write_with accepted-count, primordial indexOf, doc-comment placement) are addressed in the latest commits. This is a 42-file rework of console/process.stdout wiring, FileSink lifetime, fd-flag management and exit-path draining, with several intentional user-facing behaviour changes — worth a human look.

What was reviewed:

  • Stdio-sink error paths (on_auto_flush, drain_sync, write_all_sync, to_result, flush_from_js, on_error) — all now route through stdio_latch_error without marking the shared sink done.
  • bun_restore_stdio before execve and the failed-execve path — now records the flip via Bun__stdioMadeBlocking so sinks downgrade.
  • StdioLock!Send via PhantomData<*mut ()>; per-thread depth balances the process-wide mutex.
  • Process::visitChildrenImpl visits the new m_consoleStream[]/m_stdioStream[]/m_consolePublish barriers.
Extended reasoning...

Overview

Reworks how console.*, process.stdout/stderr, Bun.stdout.writer(), Bun.write(Bun.stdout) and console.write reach fd 1/2: one per-VM FileSink owns each fd, the console formats into a scratch buffer and delivers either straight to the sink or (when observably different) through the stream's JS write(). Touches FileSink, PosixStreamingWriter, ConsoleObject (Rust and C++), BunProcess, ZigGlobalObject, VirtualMachine lifecycle, rare_data, c-bindings (stdio snapshot/restore, SIGINT/SIGTERM handler install), posix_spawn, the built-in JS for streams/console/process/worker_threads/diagnostics_channel, plus ~500 lines of new tests.

Security risks

None identified. No parsing of untrusted input, no auth/crypto. The signal-handler install now respects an inherited SIG_IGN (a hardening). O_NONBLOCK handling is per-fd on 0–2 only and restored at exit/for children.

Level of scrutiny

High. This is core runtime I/O with cross-cutting lifetime concerns (per-VM sinks held by RareData, released before JSC teardown; JS wrapper roots dropped across bun test global swaps), fd-flag state shared across threads/children, exit-path draining, and several deliberate behaviour changes (process.stdout.write(ArrayBuffer) now throws, console.timeLog moves to stdout, console.clear sequence changes, _stdout/_stderr become accessors). The design decision to drain queued stdio synchronously on process.exit() (blocking on a slow reader, diverging from Node) is documented but should be signed off by a maintainer.

Other factors

Extensive test coverage for ordering under real backpressure, EPIPE per-call semantics, fd-flag preservation, cork/drain, exit-drain via all three exit modes, worker capture, and diagnostics_channel; four Node parallel tests newly vendored. Prior review rounds surfaced six issues, all fixed and verified in the diff. Given the surface area (42 files), the intentional behaviour changes, and the number of superseded PRs it consolidates, a maintainer should review before merge.

…dArrayBuffer

Bun's old own writeFast took these; Node's Writable rejects them. Keep the
Bun behaviour for the two stdio streams (accepted in Writable's would-throw
branch when the stream is stdio), without an extra prototype or own property
so nothing about process.stdout's shape changes. Reverts the two test
workarounds that had switched to Uint8Array.

@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.

I've reviewed this PR across several passes and every finding has been addressed; this run found nothing new. Given the scope — a rewrite of how every byte reaches fd 1/2 (per-VM stdio sinks, console→stream observability via StructureID, O_NONBLOCK snapshot/restore across spawn/execve/signals, exit-time drain, new GC roots on Process) — a human sign-off is still warranted.

What was reviewed:

  • FileSink stdio lifecycle: every terminal-error path now routes through stdio_latch_error (incl. on_auto_flush); end()/close() on the shared sink flush without marking done; RareData ref released before loop/JSC teardown.
  • fd-flag coherence: STDIO_MADE_BLOCKING set by both spawn's make_stdio_blocking and bun_restore_stdio_nonblock (failed-execve case), so refresh_stdio_mode can't miss a downgrade.
  • Process GC: new m_consoleStream/m_stdioStream/m_consolePublish WriteBarriers are all visited; m_consoleWriteFunction LazyProperty visited.
  • writable.ts _isStdio ArrayBuffer branch checked for primordial safety — instanceof on user chunks matches Node's own non-primordial handling here and only widens what's accepted.
Extended reasoning...

Overview

This PR unifies all stdout/stderr writers (console.*, process.stdout/stderr, Bun.stdout.writer(), Bun.write(Bun.stdout), console.write) onto one per-VM FileSink per fd, and makes the global console deliver through process.stdout.write whenever that is observable — mirroring Node's kWriteToConsole. It touches 40 files across the Rust runtime (FileSink.rs, ConsoleObject.rs, VirtualMachine.rs, rare_data.rs, PipeWriter.rs, sys/lib.rs), C++ bindings (BunProcess.{h,cpp}, ConsoleObject.cpp, ZigGlobalObject.cpp, c-bindings.cpp), builtin JS (ConsoleObject.ts, ProcessObjectInternals.ts, streams.ts, writable.ts, worker_threads.ts, diagnostics_channel.ts), codegen, spawn, and adds a new StdioLock reentrant per-fd mutex. Behavioural changes include console.timeLog/timeEnd fd+format, console.clear() semantics, and exit-time draining of queued stdio.

Security risks

None identified. No new user-controlled input reaches parsing or path resolution. The signal-handler change (installing onExitSignal for SIGTERM/SIGINT on FIFO stdio too, but not over inherited SIG_IGN) is defensive. O_NONBLOCK manipulation is restricted to fds 0–2 and restored at exit; the is_on_pipefs fstatfs check gates RWF_NOWAIT correctly.

Level of scrutiny

High. This is core runtime plumbing that every program exercises on every console.log, and it interacts with process-global state (fd flags on shared open file descriptions, signal handlers, termios), VM lifecycle (exit, worker teardown, bun test global rotation), GC (new WriteBarrier fields, a Strong for the shared JS wrapper), and cross-thread ordering (StdioLock, STDIO_MADE_BLOCKING atomic). A regression here silently drops or reorders output, hangs the JS thread on a full pipe, or leaks fds/roots per VM. That is well beyond the bar for automated approval.

Other factors

The PR has been through multiple automated review passes; earlier findings (autoflush error latching, execve-failure fd-flag coherence, doc-comment placement, writeToObservedStream guard symmetry, primordial indexOf, write_with byte accounting) were all addressed with follow-up commits and are visible in the current diff. Test coverage is extensive and targets the specific failure modes fixed (backpressure ordering, EPIPE per-call, exit drain, fd-flag inheritance, spyOn(process.stdout, 'write')). The design decisions — StructureID-based observability detection, blocking-until-JS-writer-exists on FIFOs, draining on process.exit() where Node truncates — are sound but are exactly the kind of runtime-semantics choices a maintainer should ratify.

…t drain, lock scope, colours

- Bun.stdout.writer() / Bun.file(1|2).writer() coalesce small writes again
  when stdout is a file or /dev/null (only a TTY writes through), as before
  this branch; the console and process.stdout paths never coalesced anyway
- Bun.write(Bun.stdout, x) no longer drains synchronously: it switches the
  sink to its async pipe mode like process.stdout does and, when the bytes
  had to queue, resolves (with its own count) once the queue drains
- exit-time drain: JS can't run any more, so hand every catchable signal
  back to SIG_DFL first (Node's ResetSignalHandlers) — waiting on a reader
  that never reads stays killable with ^C / SIGTERM
- the Output pre-write hook does nothing while panicking (crash reporter)
- StdioLock is taken per write / spill inside write_all_sync, never across
  formatting or any JS, and the JS delivery path drains only its own fd:
  no thread holds one stdio lock while waiting on the other
- poll dispatch re-checks a stdio sink's fd mode before the writable
  callback flushes (spawn may have made the description blocking)
- Linux: if RWF_NOWAIT turns out to be refused at runtime, fall back to
  O_NONBLOCK instead of blocking full-chunk writes
- colours for a foreign console stream (worker port, console._stdout = x)
  follow FORCE_COLOR/NO_COLOR, else that stream's isTTY, as Node does
- non-stdio FileSink-backed streams (child.stdin, tty.WriteStream(fd)) keep
  end-of-tick coalescing and keep accepting a bare (Shared)ArrayBuffer
- on_write's stricter "wait for the coalesced tail" rule is stdio-only
- process.stdout's cork/uncork/end/destroy wrappers are named and
  non-enumerable; Windows write_all_sync reports a short write
@dylan-conway dylan-conway self-assigned this Aug 8, 2026
@robobun

robobun commented Aug 13, 2026

Copy link
Copy Markdown
Collaborator

Triage of the smaller open console PRs against this one (built f07b71e and checked each behaviour).

Closed as covered here:

Since this also fixes the assert prefix, #19953 and #31714 could be added to the fixes list.

Left open because they cover things this PR does not, each noted to rebase once this lands:

One small thing noticed while checking: the native assert prefix is written before format2's group indent, so inside a console.group it comes out as Assertion failed: msg.

@robobun

robobun commented Aug 13, 2026

Copy link
Copy Markdown
Collaborator

Triage of the stdio half of the Supersedes list (the console half was handled above).

Closed as covered here: #35956 (the force-sync O_NONBLOCK undo; the function is deleted here), #35949 (Bun.write(Bun.stdout) order on a file-backed stdout), #34347 (worker console rebinding), #33508 (writeFast removal / Writable accounting), #35064 (EPIPE as 'error' per call), #36226 (diagnostics_channel console channels).

Left open: #33560. alii reviewed it on Aug 13 as ready to merge on its own, and it is the same fd_write_all_quiet hunk as here, so whichever lands first the other is a trivial rebase.

Things in the closed PRs that this branch does not have, in case any are worth lifting:

Not superseded, left alone: #35953 and #36025 fix the WriteFile::do_write stale-EAGAIN spin in blob/write_file.rs, which #35956 also patched and this PR does not touch. It still reproduces on main through Bun.write(Bun.file(fd), 1 MiB) on an O_NONBLOCK FIFO (never resolves, a pool thread at 100%), and a Blob source to a non-blocking stdout pipe takes that general path here too.

Also still open and, from reading, covered by this branch but not in the Supersedes list, so not closed: #33474, #31538 and #33484 (all fall out of the writeFast removal; #33508 listed them as superseded) and #31180 (two Bun.file(1).writer() calls now share one sink).

@robobun

robobun commented Aug 14, 2026

Copy link
Copy Markdown
Collaborator

#38498 fixed the same two copy_file.rs sites this branch patches (do_copy_file_range on EAGAIN and read_write_loop_capped). It came from await Bun.write(Bun.stdout, Bun.stdin) rejecting with EAGAIN from splice once fd 1 is non-blocking (always the case inside a Worker, on the main thread after any process.stdout use), and from Bun.write(Bun.stdout, Bun.file(path)) rejecting with EAGAIN from sendfile when the reader is slower than the copy. Closed in favour of this branch, which also converts copy_file_using_read_write_loop, the one site #38498 left out. Two things from it that may be worth lifting:

  • test/js/bun/io/bun-write-nonblocking-stdio.test.ts on farm/f62de375/bun-write-copyfile-eagain: 8 POSIX tests for the copy loops. The stdin idiom on the main thread and in a Worker, a pipe source and a regular-file source into a stdout the child has filled up first, and a FIFO source opened with O_NONBLOCK, the last three each for the kernel-copy loop and (via BUN_CONFIG_DISABLE_COPY_FILE_RANGE=1) the read/write loop. They assert that the copy completes, not how, so they should hold for the fallback-on-EAGAIN here as they are; against 1.4.0 they fail 60 of 60 runs with EAGAIN from splice, sendfile, read and write respectively. The Bun.write(Bun.stdout, ...) tests on this branch use string sources, which take WriteFile, so the CopyFile hunks have no test at the moment.
  • wait_until is poll(2) on macOS as well. Going by the XNU analysis in Bun.file(fifo).bytes(): wait for a named pipe's EOF with select(2) on macOS #37823 and Bun.write: reject with EPIPE when a FIFO's reader goes away mid-write #37852, poll on a named FIFO (mkfifo, unlike a pipe(2) pipe) is not woken by the other end closing, so read_retrying on a non-blocking FIFO whose writer goes away would wait forever where it rejects with EAGAIN today. The select$DARWIN_EXTSN wait those two PRs add (Bun.write(file, file): wait for a non-blocking fd instead of rejecting with EAGAIN #38498 used it for both directions) does wake up; the FIFO test above is the one that would show the difference on the darwin lanes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

2 participants