process: don't let a failing stderr write in the warning printer kill the process - #37347
process: don't let a failing stderr write in the warning printer kill the process#37347cirospaciari wants to merge 5 commits into
Conversation
|
Updated 5:43 PM PT - Aug 10th, 2026
✅ @robobun, your commit 01986ce27e31c2c49f7fbf47ea96d7c7f1c0f537 passed in 🧪 To try this PR locally: bunx bun-pr 37347That installs a local version of the PR into your bun-37347 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — small, focused regression fix that mirrors the existing kWriteToConsole port in ConsoleObject.ts line-for-line.
What was reviewed:
- Verified the try/once('error', noop)/finally + write-callback pattern matches
src/js/builtins/ConsoleObject.ts:445-528and Node'skWriteToConsole/createWriteErrorHandler. - Checked the reassigned-stderr-to-non-EventEmitter concern: same failure mode as ConsoleObject.ts and Node's own console; not a regression vs the pre-PR raw
.writecall. - Tests: sync-throw, EPIPE (deterministic via stdin gate +
stderr.cancel()), and no-listener-leak; all subprocess-isolated, pipes drained concurrently,it.concurrent.
Extended reasoning...
Overview
This PR wraps the warning printer's process.stderr.write in createOnWarning's writeOut (src/js/builtins/ProcessObjectInternals.ts) with the same error-swallowing pattern that ConsoleObject.ts already carries for kWriteToConsole: a temporary noop 'error' listener around the write, a catch that swallows everything except stack-overflow RangeError, and a write callback that re-arms the noop listener if an async error is coming and no user listener exists. This restores the pre-#31831 behavior where a warning printed into a dead pipe doesn't kill the process, matching Node v26.3.0. Three new subprocess tests in test/js/node/process/process.test.js cover sync throw, EPIPE, and listener cleanup.
Security risks
None. This is error-handling around a diagnostic write path; no input parsing, auth, crypto, or filesystem writes are involved.
Level of scrutiny
Low-to-medium. The change is ~20 lines in a builtin JS module and is a near-verbatim copy of the pattern at ConsoleObject.ts:445-468 + createWriteErrorHandler at line 512, which itself is a port of Node's lib/internal/console/constructor.js. The finally { stream.removeListener } and err !== null guard are identical. The stack-overflow rethrow condition matches. There is no novel logic to reason about — it's applying an already-reviewed pattern to a second call site that needs it.
Other factors
- The candidate concern about
removeListenerthrowing whenprocess.stderris reassigned to a non-EventEmitter was examined and ruled out: it's the same behavior as ConsoleObject.ts and Node's console, and the pre-PR code (process.stderr.write(...)) would already fail in that scenario, so no new failure mode is introduced. - Tests follow harness conventions:
it.concurrent,await usingon subprocesses,Promise.allon stdout/stderr/exited, combined-object assertions,{...bunEnv, NODE_NO_WARNINGS: undefined}. The EPIPE test is deterministic (child waits on stdin until parent has closed stderr's read end). - The PR description documents verification against Node v26.3.0 across several variants (throwing write, dead pipe, healthy pipe, redirect-warnings fallback, 200-warning burst, user's own error listener preserved) and confirms the vendored Node warning tests still pass.
- Stacked on #37344; only the top commit is reviewed here per the PR description, and the diff reflects that.
149f32b to
6ac8efd
Compare
|
@robobun adopt |
|
Adopted. Rebased onto main now that #37344 landed; head is 01986ce. Review notes folded in: |
… the process Node prints warnings through console.error, which swallows a synchronous write error and parks a one-shot noop 'error' listener so an asynchronous EPIPE from a pipe is not an unhandled error. Since #31831 switched the printer to a raw process.stderr.write, the first warning emitted into a pipe whose reader has gone away (bun x.js 2>&1 | head) exited the process with code 1, where both Node and earlier Bun releases carry on. Mirror console's kWriteToConsole (already ported in ConsoleObject.ts) inside the printer's stderr write. A user's own 'error' listener is left alone and still receives the error; a healthy stderr ends up with no extra listeners.
… the warning printer The error guard assumes an EventEmitter; a plain object assigned to process.stderr (common in test setups) has no once/removeListener, so the finally block threw and killed the process, which the previous raw write did not. Only arm and disarm the guard when the stream is an emitter.
The guard's write callback reads stream._writableState, so an EventEmitter stand-in for process.stderr whose write() invoked the callback threw from a later tick and took the process down. Gate the whole guard on _writableState so anything that is not a Writable gets the plain, callback-less write.
936cb2a to
39eca56
Compare
WalkthroughThe warning printer now guards Writable stderr writes, suppresses non-stack-overflow write failures, and prevents asynchronous stderr errors from terminating the process. Tests cover failing streams, broken pipes, custom stderr objects, and listener cleanup. ChangesWarning output handling
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/js/builtins/ProcessObjectInternals.ts`:
- Around line 668-689: Use a trusted Writable check in the warning stream
handling around the isWritable path before calling listenerCount, write, or
installing/removing error listeners; do not treat merely having _writableState
as sufficient, while preserving the existing fallback write behavior. Extend the
bare-object regression test in test/js/node/process/process.test.js:2284-2300
with _writableState: {} and assert that "alive" still prints.
🪄 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: c196439b-8d75-4ab8-b0d9-5c75e3b28bbf
📒 Files selected for processing (2)
src/js/builtins/ProcessObjectInternals.tstest/js/node/process/process.test.js
There was a problem hiding this comment.
LGTM — both earlier findings are now addressed: the gate switched from typeof stream.once === 'function' to stream._writableState !== undefined, so a bare { write } mock and an EventEmitter-with-write mock both take the plain-write branch (no callback, no removeListener in finally), and the callback's _writableState.errorEmitted read is only reached when _writableState was already confirmed present. Tests 4 and 5 in the new describe block pin each case.
Checked: the try/catch/finally + RangeError rethrow matches ConsoleObject.ts byte-for-byte; the finally remove is gated on the same isWritable flag that armed the listener, so it can't throw on a non-emitter; the healthy-stderr test pins that no listener leaks across warnings.
Extended reasoning...
Overview
Wraps the warning printer's process.stderr.write in the same try/once('error', noop)/finally guard that ConsoleObject.ts uses for console.error, so a warning printed into a dead pipe (EPIPE) or a throwing stderr.write doesn't take the process down. ~30 lines in src/js/builtins/ProcessObjectInternals.ts plus a 5-case describe block in test/js/node/process/process.test.js.
Prior review threads
Two earlier findings from this reviewer, both now addressed:
- Bare
{ write }mock threw infinally— fixed by gating the emitter calls; test 4 ("bare { write } object") covers it. Thread resolved. - EventEmitter mock's async callback dereferenced undefined
_writableState— fixed by tightening the gate fromtypeof stream.once === 'function'tostream._writableState !== undefined. An EventEmitter-only mock now takes the else branch and never receives a callback; test 5 ("EventEmitter mock whose write() calls back") is the exact repro from the finding and assertsbeforeExitfires withlistenerCount('error') === 0.
cirospaciari's noop-placement note was addressed in an earlier commit (moved to first line of createOnWarning; a shared noop would let interleaved console.error strip the parked listener).
Security risks
None. No untrusted input parsing, no privilege boundary; the change only affects whether a stderr write failure inside the built-in warning printer surfaces as uncaughtException.
Level of scrutiny
Medium. Built-in JS in a hot-ish path (every emitWarning), but the pattern is a verbatim port of the existing ConsoleObject.ts kWriteToConsole block (verified the RangeError rethrow string, listener add/remove ordering, and errorEmitted guard match). The novel bit — re-reading process.stderr per call and thus being exposed to non-Writable stand-ins — is exactly what the two prior review rounds shook out, and both edge cases now have tests.
Other factors
- Tests are subprocess-based, concurrent, drain both pipes, assert exact
{stdout, stderr, exitCode}. The EPIPE case is deterministic (parent closes stderr read end, then releases the child via stdin). - The
finallyand callback are both gated on the sameisWritableflag computed before the try, so no path can arm without disarming or vice versa. - PR description confirms behavior against node v26.3.0 for each test case and that the vendored
test-process-*warning*suite still passes.
…rinter's write guard A spread copy of the real stderr keeps _writableState as an own property but none of the prototype's listener methods, so the guard's finally threw on it. Check for removeListener alongside _writableState; such objects get the plain write.
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/js/builtins/ProcessObjectInternals.ts`:
- Line 669: Update the guarded condition in the relevant stream handling logic
to require _writableState plus removeListener, listenerCount, and once before
entering the guarded path. Add a regression case covering a replacement stream
with _writableState and write but missing either listenerCount or once,
preserving warning output instead of suppressing the resulting TypeError.
🪄 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: 17defcc9-6b33-4a51-8abb-58b75297b47c
📒 Files selected for processing (2)
src/js/builtins/ProcessObjectInternals.tstest/js/node/process/process.test.js
There was a problem hiding this comment.
LGTM — a contained port of the kWriteToConsole guard from ConsoleObject.ts into the warning printer, fixing a real regression where bun x.js 2>&1 | head dies on the first warning.
What was reviewed:
- Confirmed the try/once/finally block and the RangeError rethrow match
ConsoleObject.ts:450-468andcreateWriteErrorHandlerat:518-526line-for-line. - The
_writableState && typeof removeListener === 'function'gate now closes all three mock-stderr regressions raised in review (bare{write}, EventEmitter+callback, spread copy with_writableStatebut no prototype methods), each with a test. - Checked the six new subprocess tests: pipes drained concurrently, deterministic EPIPE ordering via stdin handshake, listener-count cleanup asserted.
Extended reasoning...
Overview
Wraps the warning printer's process.stderr.write in createOnWarning (src/js/builtins/ProcessObjectInternals.ts) with the same try/once('error', noop)/catch/finally guard that ConsoleObject.ts already uses for kWriteToConsole, so a synchronous throw or async EPIPE from stderr during a warning print no longer kills the process. Adds six subprocess tests in test/js/node/process/process.test.js.
Security risks
None. This is error-swallowing around a diagnostic write path; no auth, crypto, parsing, or untrusted-input handling is touched.
Level of scrutiny
Medium. Builtins code runs in a hostile environment per REVIEW.md, but the change is a near-verbatim copy of the proven ConsoleObject.ts pattern (verified against lines 450–468 and 518–526), scoped to a single ~30-line function. The one divergence — re-reading process.stderr per call, which exposes the guard to user mocks — is exactly what the three review iterations hardened against, and each raised case now has a dedicated regression test.
Other factors
- All four review threads (cirospaciari's noop-placement nit, my two inline findings on bare-
{write}and EventEmitter mocks, and CodeRabbit's_writableState-without-prototype case) are resolved, each with a corresponding commit and test. - The final
guardedpredicate requires both_writableStateandremoveListener, so thefinallyblock cannot throw and the callback's_writableState.errorEmittedread is safe on any object that passes the gate. - Tests follow harness conventions:
it.concurrent,await using,Promise.allon stdout/stderr/exited, combined-object assertions,{...bunEnv, NODE_NO_WARNINGS: undefined}. - PR description documents Node v26.3.0 parity for each case and confirms the two headline tests fail on current main.
2 participants
Follow-up to #37344 (landed; this branch is rebased onto main, so the diff is only the printer change).
What does this PR do?
Since #31831 the default warning printer writes with a raw
process.stderr.write. Node'swriteOutgoes throughconsole.error, whosekWriteToConsole(lib/internal/console/constructor.js) swallows a synchronous write error and parks a one-shot noop'error'listener so an asynchronous EPIPE is not an unhandled error. Without that, the first warning printed into a pipe whose reader is gone takes the process down:alivealivealiveA plain user
process.stderr.writeinto a dead pipe still exits 1, exactly as in Node: only the printer's own write is protected, which is what Node protects.The fix is the
try/once('error', noop)/finallyblock around the write increateOnWarning'swriteOut(src/js/builtins/ProcessObjectInternals.ts), a copy of thekWriteToConsole+createWriteErrorHandlerport thatConsoleObject.tsalready carries, including its stack-overflow rethrow. The--redirect-warningsfallback path goes through the same block. The guard is only armed whenprocess.stderrhas both things the guard uses:_writableState(read by the write callback) and the listener API (removeListeneris the call that would escape fromfinally). Every stderr Bun creates qualifies (tty, pipe and file are allWriteStreams). Anything else assigned toprocess.stderrby a test setup, a bare{ write }, an EventEmitter with awritemethod, or a spread copy of the real stream (which keeps_writableStatebut not the prototype's methods), gets the same callback-less write main gives it today; each of those shapes took the process down on an earlier revision of this branch.Checked against node v26.3.0, identical output in each case: throwing
stderr.write; dead-pipe stderr; 5 warnings to a healthy pipe and 3 to a file leaveprocess.stderr.listenerCount('error') === 0; a user's own'error'listener is left as the only listener and still receivesEPIPEwhile the process survives;--redirect-warningspointing at an unwritable path plus a dead stderr; 200 warnings into a dead pipe (listenerCountstill 0).How did you verify your code works?
describe("default warning printer survives a failing stderr")intest/js/node/process/process.test.js: the throwing-write and EPIPE cases exit 1 on the unfixed build (the EPIPE case closes the parent's read end before releasing the child via stdin, so it is deterministic; 15/15 runs pass here and it fails every time on the unfixed build); the third case pins that a healthy stderr ends up with no listener, which is what thefinallyand theerr !== nullguard are for; the remaining three (bare{ write }, EventEmitter, and spread-copy stand-ins forprocess.stderr) pass on main and each failed on an earlier revision of this branch (thrown fromfinally, from the write callback, and fromfinallyagain respectively). Both fixed cases also pass on bun 1.3.14, since the regression is newer than the release, soUSE_SYSTEM_BUN=1is not the failing oracle for them; main is. The existing warning tests in that file and the vendoredtest-process-warning*.js/test-process-warnings.mjs/test-process-emitwarning.js/test-env-var-no-warnings.js/test-process-redirect-warnings*.js/test-common-expect-warning.jsstill pass.no test proof · iteration 1 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/node/process/process.test.js