Conversation
When stdout/stderr is a pipe, constructing process.stdout (or any FileSink over fd 1/2) sets O_NONBLOCK on the fd so the event loop can drive it. O_NONBLOCK is a property of the open file *description*, which fd inheritance shares with the parent shell and every sibling in a pipeline, so after bun exits later blocking writers in the same pipeline (head, cat, tar, the shell itself) hit EAGAIN and truncate at the pipe buffer. bun_restore_stdio() already restored termios for TTY fds at exit; this extends it to also restore the O_NONBLOCK bit of F_SETFL for all three stdio fds, guarded by a (st_dev, st_ino) identity check so a fd the user reopened is left alone. Matches Node.js ResetStdio() in src/node.cc.
WalkthroughThis PR extends Bun's stdio restoration to preserve the original O_NONBLOCK flag state on stdin/stdout/stderr file descriptors, not just termios settings. A startup snapshot captures F_GETFL flags and file identity per fd; restoration checks identity before reapplying O_NONBLOCK. A Linux-only test validates the behavior across multiple scenarios. ChangesStdio O_NONBLOCK restoration
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 7:38 PM PT - Jul 10th, 2026
❌ @robobun, your commit 4178b7d has 1 failures in 🧪 To try this PR locally: bunx bun-pr 33827That installs a local version of the PR into your bun-33827 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Address review: the SIGINT/SIGTERM handler that runs bun_restore_stdio() was only installed when a stdio fd was a TTY, so fully-piped processes (CI, docker without -t, systemd) still leaked O_NONBLOCK on signal death. Install it whenever we have either a termios or fcntl-flags snapshot to restore, which is effectively always on POSIX. Node.js registers its ResetStdio handlers unconditionally. Also: match probe lines per-line (/m) instead of a fully-anchored regex so benign ASAN/runtime stderr between them does not break the test, and add a SIGTERM-death case covering the widened guard.
There was a problem hiding this comment.
Both earlier findings are addressed in 16b4f0d — no outstanding defects. Deferring to a human because this touches process init/exit and now installs the SIGINT/SIGTERM onExitSignal handler in effectively all runs (not just TTY), which is the right call but is a lifecycle-wide behavior change worth a maintainer's sign-off.
Extended reasoning...
Overview
Adds an O_NONBLOCK snapshot/restore for fds 0/1/2 in bun_initialize_process() / bun_restore_stdio() (src/jsc/bindings/c-bindings.cpp), mirroring Node's ResetStdio(), so bun no longer leaves the shared open file description non-blocking for downstream pipeline writers. Adds a Linux-only /proc/self/fdinfo-based regression test covering five stdout/stderr triggers, a SIGTERM-in-headless case, and a "don't clear a flag we didn't set" case.
Since my previous review, 16b4f0d widened the sigaction(SIGTERM/SIGINT, onExitSignal) guard to anyTTYs || anyFlagsSnapshotted (with a new SIGTERM test case) and switched the test's probe parsing to per-line /m matches. Both of my earlier concerns are resolved and the current bug-hunt pass found nothing.
Security risks
None identified. The new code only reads/writes F_GETFL/F_SETFL on fds 0–2 and compares dev/ino snapshots; all calls used in the signal path (fstat, fcntl) are async-signal-safe. No user-controlled input reaches this path.
Level of scrutiny
Medium-high. The logic is small and closely tracks Node's reference implementation, but it lives in bun_initialize_process() / bun_restore_stdio(), which run on every process start and every exit path (normal exit, signal death, crash handler). The follow-up commit means onExitSignal is now registered for SIGINT/SIGTERM essentially unconditionally (any fd where fstat+F_GETFL succeed) rather than only when a TTY is present — that's the correct fix for the headless case I flagged, and it matches Node, but it is a process-lifecycle behavior change (default SIGINT/SIGTERM disposition is now handler→re-raise in piped/headless runs) that I'd want a maintainer to explicitly OK rather than auto-approve.
Other factors
- The restore is careful: only the
O_NONBLOCKbit is rewritten, other flag changes are preserved, and fds reopened to a different file are skipped via dev/ino identity check. - Test is Linux-only (relies on
/proc/self/fdinfo); the C++ path is POSIX-generic and untested on macOS/FreeBSD beyond compilation. PR description notes "no test proof · deferring to CI" and build #70909 was still in progress at review time. - The widened handler should compose fine with user
process.on('SIGINT', ...)(which installs its ownsigactionlater), but that interaction is exactly the kind of thing a maintainer familiar withBunProcess.cppsignal plumbing should glance at.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/bindings/c-bindings.cpp`:
- Around line 710-715: The signal-exit path in onExitSignal still resets the
handler with signal(sig, SIG_DFL) even though SA_RESETHAND already restores the
default disposition; remove that redundant call and keep only the raise(sig)
re-delivery so the default termination path remains intact. Make the change in
the signal-handling logic around onExitSignal and preserve the existing stdio
restore behavior triggered from the anyTTYs/anyFlagsSnapshotted check.
In `@test/js/node/process/process-stdio-nonblock.test.ts`:
- Around line 1-7: Move this Linux-only stdio case into process-stdio.test.ts so
all stdio coverage stays under the same describe.concurrent.skipIf(!isLinux)
block. Keep the existing test logic and symbols like bunEnv, bunExe, isLinux,
and the stdio leak assertion, but place this scenario alongside the other stdio
tests instead of in a separate file.
🪄 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: c5f4bc4b-1319-4bae-a45f-cc6a9c2885c7
📒 Files selected for processing (2)
src/jsc/bindings/c-bindings.cpptest/js/node/process/process-stdio-nonblock.test.ts
There was a problem hiding this comment.
Both earlier concerns are addressed and I don't see further issues, but since this now installs the SIGINT/SIGTERM onExitSignal handler in effectively every bun process (not just TTY sessions), it's worth a human sanity check on the signal-handling scope change.
Extended reasoning...
Overview
This PR fixes cross-process state corruption where bun leaves O_NONBLOCK set on inherited stdout/stderr at exit, breaking subsequent blocking writers in the same shell pipeline. It touches src/jsc/bindings/c-bindings.cpp (bun_initialize_process() and bun_restore_stdio()) and adds a Linux-only test file.
The two issues I raised on the first revision were both addressed in 16b4f0d: the onExitSignal install guard was widened to anyTTYs || anyFlagsSnapshotted with a new SIGTERM test case, and the test's probe parsing switched to per-line /m matches so ASAN/debug stderr noise can't break it. Both CodeRabbit threads were also resolved (one withdrawn as out-of-scope pre-existing code, one withdrawn after the author explained the separate-file rationale). The bug-hunting pass on the current revision found nothing.
Security risks
None identified. The change reads/writes fcntl flags and fstat identity on the process's own fds 0/1/2; there's no untrusted input, no path handling, and no privilege boundary crossed. All syscalls used in the signal-handler path (fstat, fcntl, tcsetattr, pthread_sigmask, signal, raise) are POSIX async-signal-safe.
Level of scrutiny
Medium-high. bun_initialize_process() runs once in every bun invocation, and the guard change means onExitSignal is now installed as the SIGINT/SIGTERM handler in effectively all processes (since anyFlagsSnapshotted is true whenever any of fds 0/1/2 exist, which is always after the /dev/null substitution). Previously it was only installed when a TTY was present. The handler re-raises with default disposition via SA_RESETHAND + signal(SIG_DFL) + raise(), so observable exit semantics should be unchanged, and Node.js registers its equivalent unconditionally — but widening signal-handler installation to every process is the kind of change a maintainer should eyeball.
Other factors
The implementation closely mirrors Node.js ResetStdio() (identity check via st_dev/st_ino, restoring only the O_NONBLOCK bit rather than the whole flags word, EINTR retry loops). Test coverage is solid: five trigger paths for normal exit, a SIGTERM-death case in a fully non-TTY environment (which fails without the widened guard), and a negative case verifying a pre-existing O_NONBLOCK is preserved. CI build #70941 was still in progress at review time.
|
CI status across four runs (#70905, #70909, #70941, #71702):
This change is POSIX-only ( |
Jarred-Sumner
left a comment
Jarred-Sumner
left a comment
There was a problem hiding this comment.
We already fstat these on start. Can we not do that twice?
Per review: keep startup cost to just fcntl(F_GETFL) per stdio fd. The (st_dev, st_ino) identity check guarded against user code reopening fd 0/1/2 between startup and exit; dropping it means the restore could touch a different file in that narrow case, but only the O_NONBLOCK bit is written and only when it differs from the startup value.
|
@Jarred-Sumner pushed 4178b7d dropping the fstat identity check; the snapshot is now just For context on the "already fstat on start" part: I went looking but couldn't find an existing startup fstat of fds 0/1/2; the ones in |
There was a problem hiding this comment.
No issues in the latest revision — both earlier findings (signal-handler install gate, probe regex anchoring) are addressed. Leaving final sign-off to a human since this touches every-invocation process init/exit and effectively makes the SIGINT/SIGTERM onExitSignal install unconditional on POSIX, and there's an open question back to the maintainer on the dropped fstat identity check.
What was reviewed:
- New
fcntlrestore loop inbun_restore_stdio: only touches theO_NONBLOCKbit, EINTR-retried, async-signal-safe for theonExitSignalpath. - Snapshot in
bun_initialize_processruns after the/dev/nullbackfill so fds 0-2 are always valid;-1sentinel skips restore ifF_GETFLfailed. - SIGTERM test case: FIFO handshake avoids a sleep race;
bunexit 143asserts signal death, so it fails without the widened guard.
Extended reasoning...
Overview
The PR modifies src/jsc/bindings/c-bindings.cpp in two functions that run on every POSIX bun invocation: bun_initialize_process() snapshots fcntl(fd, F_GETFL) for fds 0/1/2, and bun_restore_stdio() restores the O_NONBLOCK bit to that snapshot at exit and on SIGINT/SIGTERM. The guard around the sigaction(SIGTERM/SIGINT, onExitSignal) install is widened from anyTTYs to anyTTYs || anyFlagsSnapshotted, which in practice makes it unconditional (the snapshot loop runs after the /dev/null backfill, so F_GETFL succeeds on all three fds). A new Linux-only test file exercises five triggers plus a SIGTERM case and a pre-set-O_NONBLOCK case via /proc/self/fdinfo.
The latest commit (4178b7d) simplified the earlier revision per maintainer feedback by dropping the fstat identity check that guarded against restoring onto a reopened fd; the author left an open question about whether that's the intended tradeoff.
Security risks
None identified. No user-controlled input reaches the new code — it's three fcntl(F_GETFL) calls at startup and up to three F_GETFL/F_SETFL pairs at exit on fixed fds 0/1/2. The restore only flips the O_NONBLOCK bit and preserves all other flags via (cur & ~O_NONBLOCK) | want_nonblock.
Level of scrutiny
High. bun_initialize_process and bun_restore_stdio run for every bun process on POSIX, and bun_restore_stdio is invoked from async-signal context via onExitSignal. The widened sigaction install changes default SIGINT/SIGTERM behavior for fully-piped/headless invocations (previously default disposition, now routed through onExitSignal → restore → signal(SIG_DFL) → raise). The new loop is async-signal-safe (fcntl is on the POSIX list), matches Node's ResetStdio(), and the SA_RESETHAND + explicit re-raise pattern is unchanged from before this PR — but this is exactly the kind of runs-everywhere code where a maintainer should confirm the behavioral change is wanted.
Other factors
Both of my earlier inline findings were addressed in 16b4f0d. All CodeRabbit threads are resolved/withdrawn. CI on the pre-4178b7db revision was green on Linux lanes for the new test with unrelated failures elsewhere; build #71702 for the latest commit is still running per the status comment. A maintainer has already engaged and the author's last comment poses an open question about the dropped identity check, so approving ahead of that exchange would be premature.
|
#43868 lists this PR as superseded. I checked that branch (6bd759f). It clears
This PR stays open until a maintainer decides to fold the change into #43868 or to land it separately. Details: #43868 (comment) |
This branch has not been deployed
2 participants
What does this PR do?
Fixes cross-process state corruption where bun leaves
O_NONBLOCKset on its inherited stdout/stderr open file description at exit, breaking every later blocking writer in the same shell pipeline.Repro
{ bun -e 'void process.stdout.isTTY'; head -c 300000 /dev/zero; } | { sleep 0.4; wc -c; }300000, head exits 0.head: error writing 'standard output': Resource temporarily unavailable, wc reports65536(pipe capacity)./proc/self/fdinfoon the pipe write-end showsflags: 01before bun andflags: 04001after (O_NONBLOCK added).Cause
When stdout/stderr is a pipe, constructing
process.stdout/process.stderr(or anyFileSinkover fd 1/2) reachesopen_for_writing(src/io/openForWriting.rs:174), which dups the fd and callsset_nonblockingon it so the event loop can drive writes.dup()shares the open file description, soO_NONBLOCKapplies to the parent shell's fd too.bun_restore_stdio()already ran at exit but only restoredtermioson TTY fds; it never touchedF_SETFLand skipped non-TTY fds entirely. So after any bun process in a pipeline so much as touchedprocess.stdout, subsequent coreutils in that pipeline write-error and truncate at 64 KiB.Related: #1016 is bun's own in-process EAGAIN drop while running; this is the flag escaping the process.
Related to #43868. That PR clears
O_NONBLOCKon the descriptors a spawned child inherits and waits onEAGAINin the native console writer. It does not restore the flag at exit: the repro above still fails on its head (6bd759f), and this PR's test file fails 6 of 7 there. The change here applies cleanly on top of it, and then both test files pass.Fix
bun_initialize_process()now snapshotsfcntl(fd, F_GETFL)for fds 0/1/2 at startup.bun_restore_stdio()restores theO_NONBLOCKbit (only that bit, so intentional changes to other flags are preserved) whenever it differs from the startup value. The SIGINT/SIGTERMonExitSignalhandler is now installed whenever there is state to restore, not only when a stdio fd is a TTY, so fully-piped processes that die by signal also restore. Matches Node.jsResetStdio()insrc/node.cc.Verification
test/js/node/process/process-stdio-nonblock.test.tsreads the pipe's/proc/self/fdinfoflags before and after a bun subprocess touches stdout/stderr via five triggers (process.stdout,process.stderr,Bun.file(1).writer(),Bun.stdout.writer()), asserting the flags are unchanged. A SIGTERM-death case covers signal exit in a fully non-TTY environment, and a seventh case pre-setsO_NONBLOCKbefore bun runs and asserts bun does not clear a flag it did not set.Before this change the first six cases fail (flags
02->04002); after, all seven pass. Linux-only test (uses/proc); the code path is POSIX-generic.no test proof · iteration 5 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/node/process/process-stdio-nonblock.test.ts