Conversation
WalkthroughChangesThe change records native stdio write errors, forwards them from console output to JavaScript streams, updates stream destruction behavior, and adds EPIPE coverage for piped stdout. Stdio EPIPE propagation
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 11:05 PM PT - Aug 1st, 2026
❌ @robobun, your commit bcecace has 1 failures in
UnsupportedThe requested configuration is currently not supported. Please check the documentation for supported configurations.51199a26-909b-4f32-bdf0-a453313dfad9
UnsupportedThe requested configuration is currently not supported. Please check the documentation for supported configurations.8adf9fbc-0a54-4759-a662-8c00088488c8
UnsupportedThe requested configuration is currently not supported. Please check the documentation for supported configurations.5138fb2c-f3c5-4c8e-a0ea-175230a32924
🧪 To try this PR locally: bunx bun-pr 35064That installs a local version of the PR into your bun-35064 --bun |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
|
Re #17381: that panic ( |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
There was a problem hiding this comment.
Beyond the inline nits, I also checked: the drain_to_fd refactor's &mut self + slice-of-self.buf shape — buf is a raw *mut u8 so the slice isn't borrowed through &self and drain_to_fd only touches self.fd/self.err, no aliasing; and the new C++ DECLARE_TOP_EXCEPTION_SCOPE + tryClearException() sequence — it matches the existing pattern used throughout BunProcess.cpp for top-level event dispatch.
Extended reasoning...
This is a substantive cross-language Node-compat change (Rust writer plumbing, new C++ FFI entry, built-in JS stream _destroy/errorEmitted semantics) — not a candidate for auto-approval. The two inline findings are nits; noting the two additional concerns I examined and ruled out so a later pass doesn't re-derive them.
There was a problem hiding this comment.
Both earlier findings are addressed in 95d5463 and nothing new turned up this pass, but this is a cross-cutting Node-compat behavior change (native console write path → C++ → process.stdout.destroy(), plus flipping autoDestroy on non-TTY stdio and reworking _destroy/_undestroy latching) that changes observable stream state — worth a human look.
Checked this run and ruled out: onStdioWriteError eagerly materializing lazy process.stdout (only reached on a recorded write errno, not per-log); the new EPIPE tests awaiting only 'close' without wiring 'error' (child errors surface via stderr JSON + exit code, so a hang isn't possible); fd_write_all_quiet becoming dead after drain_to_fd (still used by quiet_writer_write_all).
Extended reasoning...
Overview
Eight files across four layers: SysQuietWriterAdapter in src/sys/lib.rs gains a sticky err: i32 field and drain_to_fd (replacing the discarded-result fd_write_all_quiet calls); bun_core::output exposes take_err(); ConsoleObject.rs adds forward_write_error() called after message_with_type_and_level, count, and timeLog; BunProcess.cpp adds Bun__ConsoleObject__onStdioWriteError which resolves process.stdout/stderr, checks listenerCount('error'), and calls .destroy(err); ProcessObjectInternals.ts flips _writableState.autoDestroy to true for all non-TTY stdio (previously only file-backed) and reworks _destroy to preserve errorEmitted across _undestroy() when no listener is attached. Tests cover the four listener×API combinations plus the #7251 loop, and process-stdout-write-after-end.test.ts was updated because autoDestroy:true changes post-end() writableEnded/writable state.
Security risks
None identified. No untrusted-input parsing, no auth/crypto/permission surface. The C++ path calls user-overridable listenerCount/destroy on process.stdout, but exceptions are caught and cleared under a top-level scope, and it only runs after a real write errno on fd 1/2.
Level of scrutiny
Medium-high. console.log is one of the hottest user-facing paths in the runtime; the added forward_write_error runs on every call (though it's a cheap take_err() → None → return in the common case, and the &mut on the console backing is scoped before any re-entrant FFI). More importantly, changing autoDestroy and the _destroy/_undestroy/errorEmitted interaction on process.stdout/stderr is a Node-compat semantics change with observable effects on stream state (writableEnded, writable) — the kind of thing that can ripple into userland stream libraries in ways the test matrix here may not cover. The updated process-stdout-write-after-end.test.ts expectations (now writableEnded: false, writable: true post-end) illustrate exactly that ripple.
Other factors
- Both prior review findings (missing
forward_write_errorincount/timeLog; undrained stdout pipe in the #7251 test) were addressed in 95d5463 and verified in the current diff. - The PR description lists a broad set of still-passing Node parallel tests (
test-process-external-stdio-close*,test-stdio-undestroy,test-console-*-write-error, etc.), which is the right regression surface. - The
errorEmittedlatch keyed onlistenerCount('error') === 0at_destroytime is a Bun-specific divergence from Node (justified in the comment by Bun not terminating on uncaught) — a maintainer should confirm that trade-off is acceptable. - New tests are
skipIf(isWindows); Windows EPIPE semantics remain uncovered.
95d5463 to
7b14cc3
Compare
|
Rebased onto main (was 262 commits behind) and fixed the Also switched the new EPIPE Re-verified locally on 7b14cc3:
Closed #30635 as superseded. |
…ror' on process.stdout When stdout's reader has gone away (piped to a consumer that exited), console.log and process.stdout.write disagreed on whether the program failed and neither matched Node: - console.log wrote natively to fd 1 via a quiet writer that swallowed every errno, so an attached process.stdout 'error' listener was never called and the process exited 0 with all output lost. - process.stdout.write went through errorOrDestroy with the pipe stream's autoDestroy left at false (autoClose:false), so emitErrorNT latched on errorEmitted after the first emit and later writes never surfaced an error. Node's pipe stdio is a net.Socket (autoDestroy:true) whose _destroy is overridden to _undestroy(), so each failed write goes errorOrDestroy -> destroy -> _undestroy -> emit 'error' on nextTick and the stream stays writable. Node's console.log writes through process.stdout.write(), so the same path carries its failures. Fix: - Set autoDestroy:true on non-TTY stdio write streams so errorOrDestroy takes the destroy path, and preserve errorEmitted across _undestroy() when no 'error' listener is attached so a write loop on a dead pipe doesn't re-emit forever (Node terminates on the first uncaught; Bun doesn't, so the latch lives here instead). - Record the native console writer's first write errno in the QuietWriterAdapter and, after each console.* call, forward it to process.stdout/stderr via stream.destroy(err) when an 'error' listener is attached. With no listener the failure is dropped (Node's createWriteErrorHandler swallows it; test-process-external-stdio-close). Fixes #7251 for the with-listener case.
…count/timeLog write errors - process-stdout-write-after-end.test.ts asserted writableEnded:true/ writable:false for piped stdio after end(), but Node's pipe stdio (net.Socket, autoDestroy:true) runs finish -> destroy -> _undestroy, which resets those. The write-after-end contract itself (ret:false, ERR_STREAM_WRITE_AFTER_END, bytes not delivered) is enforced at write() time before that reset and is unchanged. - Bun__ConsoleObject__count / timeLog write through the same adapter backings; forward their recorded errno too so console.count() on a dead pipe reaches a process.stdout 'error' listener and doesn't leave a sticky errno for the next console.log. - Rewrote the new EPIPE tests to spawn one child directly instead of two nested ones so the debug-build concurrency load is lower.
…sts sequentially - ConsoleObject.rs: move the SAFETY comment onto the line directly above the unsafe block so clippy::undocumented_unsafe_blocks accepts it (it was above the enclosing let-expression, which the lint doesn't follow). - process-stdio.test.ts: drop describe.concurrent on the new EPIPE block. The existing process-stdio describe.concurrent already runs nine debug+ASAN children at once; five more in parallel pushed the timer-paced stdin tests past their 5s default timeout.
7b14cc3 to
268946d
Compare
…og doesn't reify the lazy stream
There was a problem hiding this comment.
No issues found this pass — the earlier feedback (console.count/timeLog wiring, undrained pipe in the test, lazy stdout reification via getDirect) is all addressed. Deferring to a human for the final call: this changes stdio stream semantics for every Bun program (moves autoDestroy:true to all non-TTY stdio, adds a Bun-specific errorEmitted latch across _undestroy()), inverts the expected writableEnded/writable values in an existing test, and adds a new C++→JS re-entry point on the console write path.
Checked: the sticky-errno flow (recorded in drain_to_fd, cleared unconditionally in forward_write_error before the pending-exception guard); getDirect probe skips lazy stream construction; DECLARE_TOP_EXCEPTION_SCOPE + tryClearException on every JS call in the new C++ hook; EAGAIN/EINTR filtered out so transient errnos don't emit 'error'.
Extended reasoning...
Overview
Wires EPIPE (and other write errnos) from Bun's native console.* fd writes back to process.stdout/stderr as 'error' events, and fixes process.stdout.write on a dead pipe to emit 'error' per write instead of latching after the first. Touches: SysQuietWriterAdapter (sticky errno field + drain_to_fd), the OutputSink link interface, ConsoleObject.rs (forward_write_error called from message_with_type_and_level, count, timeLog), a new C++ entry Bun__ConsoleObject__onStdioWriteError in BunProcess.cpp, and the getStdioWriteStream builtin (autoDestroy:true for all non-TTY stdio + an errorEmitted-preservation branch in _destroy).
Security risks
None identified. No untrusted-input parsing; the errno flows from Bun's own syscall wrapper.
Level of scrutiny
Medium-high. ProcessObjectInternals.ts runs for every Bun process that touches process.stdout/stderr; the autoDestroy change was previously scoped to file-backed stdio and now applies to pipes/sockets too, and process-stdout-write-after-end.test.ts had its expected writableEnded/writable values inverted to match. The _destroy override adds a deliberate Bun-vs-Node divergence (preserving errorEmitted when no listener is attached) justified by Bun not exiting on uncaughtException. The new C++ hook re-enters JS from inside a console.* host call and clears any exception it raises. These are all defensible per the PR body's Node comparison, but they're design decisions on a hot, user-visible path.
Other factors
All three of my earlier inline findings were addressed in follow-up commits (95d5463, 268946d, bd3f63b). The bug hunter's one candidate this run — a transient EINTR/EAGAIN masking a later real EPIPE within one console.* call — was ruled out: forward_write_error filters EAGAIN/EINTR after take_err() has already cleared the sticky slot, so the next write's EPIPE is recorded fresh. Test coverage looks solid (four listener×API combinations plus the #7251 loop, event-driven with no sleeps), and the PR body lists a broad set of node-parallel tests re-verified. CI build #86890 is still in progress.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/jsc/ConsoleObject.rs`:
- Around line 404-405: Remove the duplicated stream-selection predicate from the
outer forwarding logic and centralize the decision in the message routing flow.
Update message_with_type_and_level_ to return or expose the selected
destination, or extract a shared helper used by both callers, so the adapter
selection remains identical whenever routing changes.
In `@test/js/node/process/process-stdio.test.ts`:
- Around line 207-211: Update the report-parsing logic around lines and
JSON.parse to assert that the JSON report line exists before parsing, and assert
the expected child output while preserving stderr and exitCode in failures. Keep
the existing result and exit validation unchanged once the report is confirmed.
🪄 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: fb60e2b6-2814-498b-a064-8852e678aecd
📒 Files selected for processing (8)
src/bun_core/lib.rssrc/bun_core/output.rssrc/js/builtins/ProcessObjectInternals.tssrc/jsc/ConsoleObject.rssrc/jsc/bindings/BunProcess.cppsrc/sys/lib.rstest/js/node/process/process-stdio.test.tstest/js/node/process/process-stdout-write-after-end.test.ts
…hen report line is missing
…E in the same console.* call
…eLog break not return; cover stderr in EPIPE tests
|
CI status: the diff is green on every lane that ran it. Build 87508 (bcecace): 181 jobs passed, 14 still scheduled (agent queue backlog), 1 canceled. The only hard failure is the Build 87011 (27ad547, the last fully-completed build): only hard failure was bcecace over 27ad547 only adds the |
There was a problem hiding this comment.
No further findings after bcecace — the four items from my last round (hasProcessObject guard, empty-Log stderr writer, timeLog break, stderr test coverage) are all addressed. Deferring to a human because this changes user-visible process.stdout/stderr stream semantics (autoDestroy flip, _destroy→_undestroy with errorEmitted latching) and inverts the writableEnded/writable expectations in an existing test.
What was reviewed:
forward_write_errorexception handling:take_err()clears before the pending-exception check, and the C++ side uses a top exception scope withtryClearExceptionon every call — no exception leaks back intoconsole.*.getDirect+hasProcessObject()guard: verified neitherprocessnor the lazy stdout/stderr stream is reified when the user never touched them.drain_to_fdsticky-errno: EAGAIN/EINTR skipped at record time;adapter_write_all's buffered-slice reconstruction matches the removedbuffered()helper's invariants._destroyoverride: checked thaterrorEmittedis snapshotted before_undestroy()and only re-latched when no listener is attached, so the with-listener path still fires per write.
Extended reasoning...
Overview
The PR wires EPIPE (and other write errnos) from Bun's native console.* fd writes back to process.stdout/stderr as 'error' events, and fixes process.stdout.write's one-shot error latching so repeated writes to a dead pipe each surface an error. It spans four layers: SysQuietWriterAdapter gains a sticky errno slot (src/sys/lib.rs), bun_core::output exposes take_err(), ConsoleObject.rs calls forward_write_error after each console write, a new C++ entry point Bun__ConsoleObject__onStdioWriteError in BunProcess.cpp resolves the stream and calls .destroy(err), and ProcessObjectInternals.ts flips autoDestroy on non-TTY stdio and preserves errorEmitted across _undestroy() when unlistened.
Security risks
None identified. The new C++ path only reads own properties via getDirect and calls listenerCount/destroy on the process-owned stream object; it doesn't parse untrusted input or touch auth/crypto. Exception scopes are declared and cleared on every fallible call.
Level of scrutiny
High. This is Node-compat behavior for process.stdout/stderr — a hot, user-visible surface where subtle stream-state changes (autoDestroy, writableEnded, errorEmitted) ripple into every program that pipes output. The change also inverts assertions in an existing test (process-stdout-write-after-end.test.ts: writableEnded: true→false, writable: false→true), which per REVIEW.md warrants explicit human sign-off that the new values match Node rather than weakening a guard. The Rust↔C++↔JS re-entrancy (forward_write_error → destroy() → _destroy → _undestroy) is the kind of path where a maintainer should confirm the design.
Other factors
The PR has been through several review rounds; all four of my prior findings and the earlier CodeRabbit/robobun feedback are resolved in bcecace. The console.timeEnd sibling was intentionally excluded with a stated reason (it writes through Output::Source, not the ConsoleObject adapter), which satisfies the "say so in the PR" rule. Test coverage now spans stdout×stderr × console×write × with/without listener plus the #7251 loop; the PR description lists the Node parallel tests that still pass. Given the cross-layer scope and the existing-test expectation flip, this needs a human reviewer's approval rather than mine.
|
Closing: #37128 reworks the stdio sinks and lists this PR as superseded (a failed If something here turns out not to be covered once #37128 lands, this can be reopened. |
…mpat bun >=1.4 surfaces EPIPE from writes into a closed stdio/IPC channel as a fatal error instead of swallowing it (oven-sh/bun#35064). Install a process-wide broken-pipe guard in bootstrap (uncaughtException, unhandledRejection, stdout/stderr 'error') so the TUI/server/worker legitimately racing channel teardown keep running as they did on bun <=1.3; genuine uncaught errors still terminate. Also harden the LLM worker writer (write/flush/end) to treat EPIPE/EBADF as no-ops at the source.
First release line built and verified with bun 1.4.0 (.157): TTFD ~25% faster than 1.3.14 in interleaved Ghostty A/B, build time down. Requires the EPIPE guard from b071364 (oven-sh/bun#35064 surfaces EPIPE from writes into torn-down stdio channels as fatal errors). bun.lock normalized by the 1.4 installer (pruned unused optional coverage/legacy entries).
bun >=1.4 prints some internal runtime errors (e.g. EPIPE surfaced from torn-down stdio channels, oven-sh/bun#35064) directly to the child's stderr, bypassing process-level handlers — so they leaked into the user's terminal. Pump the TUI server child's stderr into <data>/opencode/log/tui-server.log instead of inheriting the tty.
Once stdout's reader is gone (piped to a consumer that exited),
console.logandprocess.stdout.writedisagreed on whether the program failed, and neither matched Node.Repro
WHICH=log[stdout error EPIPE]×5, exit 0[stdout error EPIPE]×5, exit 0WHICH=write[stdout error EPIPE]×5, exit 0[stdout error EPIPE]×1, exit 0[stdout error EPIPE]×5, exit 0Cause
console.log writes natively to fd 1 via
SysQuietWriterAdapter(src/sys/lib.rs), whosewrite_all/flushalways returnOk(())and drop the errno.process.stdoutnever hears about it.process.stdout.write on a pipe is a
fs.WriteStreamwithautoClose:false, which leaves_writableState.autoDestroyatfalse.writeFast's rejection handler callserrorOrDestroy, which withautoDestroy:falsegoes straight toemitErrorNT; that latches onerrorEmittedafter the first emit, so writes 2..N never surface. Node's pipe stdio is anet.Socket(autoDestroy:true) whose_destroyis overridden to_undestroy(), so each failed write goeserrorOrDestroy -> destroy -> _undestroy -> emit 'error'and the stream stays writable.Fix
ProcessObjectInternals.ts: setautoDestroy:trueon non-TTY stdio write streams soerrorOrDestroytakes the destroy path. In the_destroyoverride, preserveerrorEmittedacross_undestroy()when no'error'listener is attached so a write loop on a dead pipe emits at most one uncaught (Node terminates on the first; Bun doesn't, so the latch lives here).SysQuietWriterAdapter: record the first write errno in a sticky field; expose viaQuietWriterAdapter::take_err().ConsoleObject.rs/BunProcess.cpp: after eachconsole.*call, if an errno was recorded (and isn'tEAGAIN/EINTR), hand it toprocess.stdout/stderrviastream.destroy(err)so'error'fires on nextTick. When no'error'listener is attached the failure is dropped rather than becoming an uncaught exception, matching Node'screateWriteErrorHandlerandtest-process-external-stdio-close.Error shape matches Node:
{"code":"EPIPE","syscall":"write","errno":-32}Verification
New tests in
test/js/node/process/process-stdio.test.tscover all four listener × write-API combinations plus the #7251| headloop with a listener that exits. All fail on main (except theconsole.log-without-listener regression guard, which matches main's silent behavior) and pass here.Still passing:
test/regression/issue/1632.test.ts,test/js/web/console/,test/js/bun/console/,test/js/node/tty.test.ts,test/js/node/child_process/child-process-stdio.test.js,test/js/node/fs/fs.test.ts -t WriteStream, and node'stest-process-external-stdio-close{,-spawn},test-console-log-stdio-broken-dest,test-stdio-{closed,undestroy,pipe-access,pipe-stderr},test-stdout-stderr-{reading,write},test-console-{async,sync}-write-error,test-child-process-stdout-flush{,-exit}.Supersedes #30635 (same surface for the console path, plus the
autoDestroyfix for the write path).Fixes #7251 for the with-listener case.
console.timeEndis intentionally excluded: it writes through the process-wideOutputsource adapters rather than theConsoleObjectbackingsforward_write_errorreads, and Node routes it to stdout (Bun routes it to stderr, a pre-existing divergence). The errno those adapters record is an inerti32that nothing reads.The setImmediate loop with no
'error'listener still hangs (pre-existing BununcaughtExceptionsemantics, unchanged by this PR), but no longer spams stderr.no test proof · iteration 4 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/node/process/process-stdio.test.ts