Conversation
Collaborator
Author
|
Updated 9:05 PM PT - May 14th, 2026
❌ @robobun, your commit ad30454 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 30635That installs a local version of the PR into your bun-30635 --bun |
Contributor
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR adds stdio write error detection and forwarding: console stdout/stderr write failures are captured, converted to JS errors, and forwarded to the corresponding process stream (destroy(err) / 'error' listener) at most once per stream. ChangesStdio Write Error Handling
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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.zig`:
- Around line 119-136: The code currently clears backing.err and sets emitted
before converting the system error to a JS error and calling
Bun__ConsoleObject__onStdioWriteError, which can drop the original write_err if
toJS() or the callback returns early; fix by deferring mutation of backing.err
and emitted until after sys_err.toJS(global) and
Bun__ConsoleObject__onStdioWriteError succeed — i.e., compute system_errno and
sys_err, call sys_err.toJS(global) and
Bun__ConsoleObject__onStdioWriteError(global, fd, js_err), and only then set
backing.err = null and emitted.* = true (or handle rollback on failure) so the
pending write_err is preserved if forwarding fails.
- Around line 101-107: The write-error handoff in messageWithTypeAndLevel_ calls
maybeEmitStdioWriteError after releasing stdout_mutex/stderr_mutex, allowing
races on console.writer_backing/console.error_writer_backing and
stdout_write_error_emitted/stderr_write_error_emitted; fix by performing the
check-and-clear (the maybeEmitStdioWriteError invocation or its internal
check/clear logic) while holding the corresponding mutex (stdout_mutex or
stderr_mutex) so the handoff is synchronized, or alternatively protect
backing.err and the *_write_error_emitted flags with atomics or a dedicated lock
and update maybeEmitStdioWriteError to use those atomics/lock to avoid races
(adjust both the branch for is_stderr and the same code paths at lines 112-123).
🪄 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: 57badac8-44aa-438f-9810-6b52c5e51567
📒 Files selected for processing (3)
src/jsc/ConsoleObject.zigsrc/jsc/bindings/BunProcess.cpptest/regression/issue/07251.test.ts
Contributor
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/js/node/process/process-stdio.test.ts`:
- Around line 170-180: The test currently only writes err.code back to the
sibling stream in the process.${stream}.on('error') handler; update the handler
inside the script function to serialize and write the full error object
(including at least err.code, err.errno and err.syscall) to the sibling stream
(e.g., as JSON) so the test asserts the complete error shape rather than just
code; apply the same change to the other occurrence referenced (lines ~204-229)
to ensure both stdout/stderr error paths return the full payload.
🪄 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: 9e3b01a0-1856-4b5f-b341-1e92f0316a33
📒 Files selected for processing (2)
src/jsc/ConsoleObject.zigtest/js/node/process/process-stdio.test.ts
Collaborator
Author
CI status — diff is green, remaining failures are fleet-wide flakesThis PR's own changes pass on every lane. Build 54384 surfaced one real issue (EAGAIN surfaced as a stream error on release x64) — fixed in dfff7e5 + 43a3550 (skip EAGAIN/EINTR in the transient-errno filter). All subsequent review feedback addressed through ad30454. Remaining red lanes on build 54457 are unrelated to
Already re-rolled once. All review threads (15 total) addressed and resolved. Ready for a maintainer to merge. |
When stdout is piped to a process that closes (e.g. `bun script.js | head`),
Bun's native console.log writer gets EPIPE from write(2) but swallowed it with
`catch {}`. Node.js routes console.log through process.stdout.write(), so the
write error surfaces as an 'error' event on process.stdout and user handlers
fire. In Bun the process just looped forever.
After a console.log/console.error write fails, forward the error to
process.stdout/process.stderr via stream.destroy(err), with a
once('error', noop) added first (matching Node's createWriteErrorHandler)
so the common `| head` case without a listener still exits cleanly. Only
done once per stream so a tight sync loop doesn't queue unbounded destroys.
Fixes #7251
So a transient failure in toJS() (pending exception) doesn't permanently suppress the emit; a later failed write will retry.
…test.ts - Skip the JS emit path when a JS exception is already pending so the C++ handler's exception scopes can't swallow the user's original throw. The pipe stays broken so the next console.* call retries. - Move the #7251 tests into test/js/node/process/process-stdio.test.ts (this is a Node-compat gap, not a regression) and make them concurrent.
ban-words.test.ts flags global.hasException() as incompatible with strict exception checks. We already know an exception is pending when messageWithTypeAndLevel_ returns an error (voidFromJSError leaves it), so just clear backing.err and return from the catch block — no need to query the VM.
Matches Node's createWriteErrorHandler and Bun's own user-created Console
path in src/js/builtins/ConsoleObject.ts — only add the safety noop when
no one else is listening, so process.stdout.listeners('error') doesn't
briefly show an extra anonymous function when the user has their own
handler.
If a Proxy get trap on process.stdout throws during the listenerCount/once lookup, JSObject::get() returns an empty JSValue after the exception is cleared. On JSVALUE64 empty passes isCell(), so asCell()->isCallable() would dereference null. Use else-if so the callable check is skipped when the lookup threw, matching the destroy() lookup below.
…ntry + ConsoleObject emit)
Collaborator
Author
|
Rebased onto main (post-Rust-rewrite) and ported:
|
robobun
force-pushed
the
farm/33010463/console-epipe-process-stdout
branch
from
May 14, 2026 18:46
9771619 to
84d1138
Compare
CI on release x64 intermittently saw {code:'EAGAIN', errno:-11} instead
of EPIPE from the #7251 test. EAGAIN is a transient 'would block' when
the pipe buffer is momentarily full and the fd happens to be non-blocking
before the reader closes — Node.js's stream layer retries these and never
surfaces them as 'error' events. Skip them here too; the next console.*
call will either succeed (buffer drained) or hit the real EPIPE once the
reader actually closes.
Applied to both the Rust implementation (active) and the legacy Zig
reference for consistency.
Bun__ConsoleObject__onStdioWriteError runs arbitrary JS synchronously (lazy process.stdout getter, listenerCount, once → 'newListener', destroy → _destroy), any of which may re-enter message_with_type_and_level and call vm_console_mut again. Passing &mut writer_backing / &Cell<bool> as function arguments keeps them protected (Stacked Borrows) across that FFI call, violating vm_console_mut's documented single-borrow invariant. Split into take_stdio_write_error (extracts errno + latches emitted with the &mut scoped to that call) and emit_stdio_write_error (takes only the i32 errno, no ConsoleObject borrows) so nothing derived from ConsoleObject is live across the re-entrant boundary.
On macOS bun_sys::write() uses check_once! (no EINTR retry), so a signal interrupting a console write can record EINTR into the sticky err field. EINTR is the same transient-retry class as EAGAIN — Node.js/libuv retry it internally and never surface it as a stream 'error'. Without this filter it would latch emitted=true and permanently consume the once-per-stream slot so a later real EPIPE never surfaces.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/jsc/ConsoleObject.rs`:
- Around line 436-465: The helper take_stdio_write_error currently both clears
the sticky errno (backing.take_err()) and sets the one-shot latch
(stdout_write_error_emitted/stderr_write_error_emitted), causing a lost error
path when called from message_with_type_and_level just to clear errno; change
the API so clearing vs emitting are separate: either add a parameter (e.g.
set_emitted: bool) to take_stdio_write_error or create a new helper (e.g.
clear_stdio_write_error_no_latch) that only calls backing.take_err() and returns
the error without setting emitted, then update message_with_type_and_level (and
other callers that only need to clear errno) to call the no-latch variant while
leaving existing call sites that intend to emit to use the latch-setting
behavior; ensure EAGAIN/EINTR handling remains the same and emitted.set(true)
only happens when you intend to forward the error to JS.
🪄 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: ca9e0351-0364-4768-a1ea-07ee9f703e50
📒 Files selected for processing (2)
src/jsc/ConsoleObject.rssrc/jsc/ConsoleObject.zig
take_stdio_write_error() sets emitted=true as a side effect before returning Some(errno). Using it with `let _ =` on the error path to just clear the sticky errno also latches emitted without ever calling emit_stdio_write_error, so the next console.* that hits EPIPE returns None at the emitted.get() gate and the #7251 hang recurs. Inline backing.take_err() on the error path (clears errno only, no latch) to restore parity with the Zig sibling at ConsoleObject.zig:108.
- Revert ConsoleObject.zig to main: per src/CLAUDE.md, .zig siblings are uncompiled porting references for original semantics — 'Never add new behavior to a .zig file.' The .rs is the only implementation that ships. - BunProcess.cpp: comment says 'Called from ConsoleObject.zig' but the live caller is ConsoleObject.rs. - ConsoleObject.rs: no-arg console.warn()/console.error() (len==0 && Log) was hardcoding the stdout writer regardless of level, so the EPIPE detection's is_stderr predicate checked the wrong backing. Use the level-selected writer so the newline goes to stderr (matching Node.js) and detection and body agree.
Collaborator
Author
|
Superseded by #35064, which takes the same errno-recording approach for the |
Collaborator
Author
|
Superseded by #35064, which covers this fix plus the |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #7251.
Repro
bun run bug.js | headBefore: prints 10 lines then hangs forever.
After: prints 10 lines, then
EPIPE emitted, exits 0 — same as Node.js.Cause
Node.js's
console.logwrites throughprocess.stdout.write(), so awrite(2)failure (EPIPE when the read end closes) surfaces as an'error'event onprocess.stdoutvia the stream's normal error machinery.Bun's
console.loguses a separate native writer (ConsoleObject.zig) that writes directly to fd 1 and swallows every error withcatch {}. Theprocess.stdoutstream is never told anything went wrong, so a user'error'listener never fires and asetImmediateloop spins forever writing to a dead pipe.Fix
After each
console.log/console.errorcall, check the writer adapter's recorded error. If a write failed, build abun.sys.Errorfrom the errno and hand it toprocess.stdout/process.stderrviastream.destroy(err)so the'error'event fires on nextTick. Aonce('error', noop)is added first (matching Node'screateWriteErrorHandler) so the common| headcase without a user listener still completes quietly instead of turning into an uncaught exception. This is done at most once per stream so a tight sync loop (for (...) console.log(...)) doesn't queue unboundeddestroy()calls. The emit path is skipped if a JS exception is already pending so the C++ handler's exception scopes can't swallow a user's throw.process.stdout.write()already handled EPIPE correctly; only theconsole.*path was affected.Verification
Error object shape matches Node:
{"code":"EPIPE","syscall":"write","errno":-32}New tests in
test/js/node/process/process-stdio.test.tsfail on 1.3.13 (hang/timeout) and pass with this fix. Existing tests intest/regression/issue/1632.test.ts,test/js/web/console/, andtest/js/bun/console/still pass.Tests are POSIX-only (
describe.skipIf(isWindows)) since EPIPE-on-broken-pipe is a POSIX behavior; Windows stdio uses a different mechanism.