console: retry on EAGAIN instead of dropping output when stdout is nonblocking

[robobun]

Problem

Once process.stdout or process.stderr has been touched (reading .isTTY, any write, etc.), console.log / console.error to a pipe silently drop output when the pipe is momentarily full. Exit code stays 0 and nothing is reported.

void process.stdout.isTTY;
console.log(Buffer.alloc(1 << 20, "A").toString());
// piped to `wc -c`: node prints 1048577, bun prints one pipe buffer (8192 to 65536) and exits 0.

The same thing happens to a bun parent that never touched its own stream, if a child it spawned with inherited stdio touched its process.stdout: the O_NONBLOCK flag lives on the shared open file description. bun test --parallel builds this topology itself and loses worker output.

Cause

Materializing process.stdout creates the FileSink on a dup of fd 1 and puts O_NONBLOCK on the shared description (open_for_writing). The native console writer behind console.* is a separate writer on the same fd; its write loop, fd_write_all_quiet in src/sys/lib.rs, treated the resulting EAGAIN like any other error and returned, discarding the unwritten tail.

Fix

fd_write_all_quiet now retries on EINTR and, on EAGAIN, waits for POLLOUT and retries; a poll failure or any other write error still ends the loop as before. This restores the blocking-write semantics console.* has when the fd is blocking, independent of who flipped the flag (this process, a bun child, or any other program sharing the description).

process.stdout.write() itself is unchanged: it keeps returning false and emitting drain on a full pipe, as on main and in node. An earlier revision of this PR also made ForceFileSinkToBeSynchronousForProcessObjectStdio clear O_NONBLOCK; that turned process.stdout.write() to pipes into a blocking call and has been dropped (see alii's review). A test now pins the async contract.

The flag itself is therefore still set on fd 1 / fd 2 after process.stdout / process.stderr is materialized. This PR makes bun's own console writer correct under it; a non-bun child that inherits the fd and does plain write(2) (alii's head -c example) is unchanged and out of scope here, since clearing the flag is what regressed write().

Tests

test/js/node/process/process-stdio.test.ts, POSIX only. Each loss test writes a single 1 MiB line, which exceeds any pipe buffer, so the unfixed binary fails deterministically with no reader delay:

test	unfixed	fixed
console.log after touching process.stdout, into wc -c	8193	1048577
console.error after touching process.stderr, into wc -c	8192	1048577
parent console.log after a bun child with inherited stdout touched its stream	8192	1048577
process.stdout.write(1 MiB) to a pipe returns false and emits drain	passes	passes (fails with the dropped FileSink change)

Verified separately (not in CI, timing dependent): the bun test --parallel=4 fixture from the report goes from losing 29 to 71 percent of worker console.log lines to losing none.

[coderabbitai]

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

The change derives synchronous process-stdio descriptors from IOWriter, retries interrupted or blocked writes, and adds Windows-skipped coverage for delayed pipes, large output, inherited stdout, and raw nonblocking descriptors.

Changes

Process stdio reliability

Layer / File(s)	Summary
Synchronous descriptor and write retry path src/runtime/webcore/FileSink.rs, src/sys/lib.rs	Synchronous stdio derives its descriptor from IOWriter; quiet writes retry EINTR and wait for POLL_OUT after EAGAIN.
Windows nonblocking stdio coverage test/js/node/process/process-stdio.test.ts	Tests cover delayed pipe readers, 1 MiB lines, inherited stdout shared with a Bun child, and raw O_NONBLOCK output.

Possibly related PRs

• oven-sh/bun#35993: Modifies FileSink.rs and sys/lib.rs for synchronous and nonblocking descriptor writes.
• oven-sh/bun#36066: Extends the stdio fix across FileSink, fd_write_all_quiet, and process-stdio tests.

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title clearly and concisely describes the primary fix for dropped console output when stdout is nonblocking.
Description check	✅ Passed	The description explains the problem, cause, fix, scope, and verification results, although it uses different headings from the template.

---

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

[robobun]

^{Updated 11:57 PM PT - Aug 12th, 2026}

❌ @robobun, your commit 7d5bb80 has 1 failures in Build #93803 (All Failures):

• test/js/bun/http/proxy-stress-protocol.test.ts - code 1 on 🐧 13 x64-asan

---

🧪   To try this PR locally:

bunx bun-pr 33560

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

bun-33560 --bun

[github-actions]

Found 1 issue this PR may fix:

1. process.stdout.write outputs nothing, while console.log works #9573 - process.stdout.write outputs nothing in CI environments, consistent with the O_NONBLOCK/EAGAIN output-drop bug this PR fixes

If this is helpful, copy the block below into the PR description to auto-close this issue on merge.

Fixes #9573

🤖 Generated with Claude Code

[github-actions]

This PR may be a duplicate of:

1. console: surface EPIPE from console.log on process.stdout #30635 - Both fix silent data loss in fd_write_all_quiet (src/sys/lib.rs) caused by process.stdout setting O_NONBLOCK on fd 1; console: surface EPIPE from console.log on process.stdout #30635 surfaces EPIPE while console: retry on EAGAIN instead of dropping output when stdout is nonblocking #33560 retries on EAGAIN, and both modify the same function area for the same root cause

🤖 Generated with Claude Code

[robobun]

On the two bot suggestions:

• Not a duplicate of console: surface EPIPE from console.log on process.stdout #30635. That PR surfaces EPIPE from console.log; this one fixes silent loss on EAGAIN. Both touch fd_write_all_quiet, but they handle different errno paths and are complementary.
• Not adding Fixes #9573. That issue is the opposite direction (console.log works, process.stdout.write outputs nothing), which is the node-compat stream path, not the native console writer this PR changes. Didn't want to auto-close an issue this doesn't verifiably fix.

[claude]

Thanks for addressing the exit-code nit. I don't see any bugs, but since this changes the blocking semantics of the writer behind every console.log (now blocks the JS thread on poll when fd 1/2 is nonblocking and the reader stalls), it's worth a human confirming that trade-off and layer choice.

Extended reasoning...

Overview

This PR modifies fd_write_all_quiet in src/sys/lib.rs — the low-level write loop backing the native console.log / Output writer (QuietWriter / SysQuietWriterAdapter) and ScopedLogger. On Unix it now retries on EINTR and blocks on poll(POLLOUT, -1) on EAGAIN before retrying, instead of returning false and dropping the unwritten tail. A regression test in test/js/node/process/process-stdio.test.ts reproduces the slow-pipe scenario and asserts all 1500 lines arrive. Since my prior inline comment, commit f57553a dropped the misleading pipeline exit-code assertion; that thread is resolved.

Security risks

None identified. No untrusted input parsing, no auth/crypto/permissions surface. The change only affects how stdout/stderr writes handle transient EAGAIN on the process's own fds.

Level of scrutiny

Medium-high. The diff is ~15 lines and the mechanism (poll-then-retry) is textbook, but fd_write_all_quiet sits under every console.log in every bun process. The change trades "silently drop output" for "block the JS thread until the pipe drains," which is almost certainly the right call (matches Node, and silent loss is worse than a stall), but it is a real behavioral change on a hot path. There's also a reasonable architectural question — whether the fix belongs here vs. preventing the native writer's fd from being flipped to O_NONBLOCK in the first place, or unifying the two writers — that a maintainer should ratify.

Other factors

• All callers of fd_write_all_quiet are local to this file (the quiet-writer adapter and vtable), so blast radius is contained to console/logger output.
• poll returning on POLLERR/POLLHUP (broken pipe) is handled correctly: the next write gets EPIPE and hits the fall-through Err(_) => return false arm; a signal-interrupted poll just loops back to write → EAGAIN → poll again without busy-spinning.
• Windows is unchanged (#[cfg(unix)]-gated) and the test is skipped there.
• The bug-hunting system found no issues this round; my only prior nit was addressed.
• A related open PR (#30635) touches the same function for EPIPE; the author explained they're complementary, but a human may want to coordinate the two.

[robobun]

On the blocking trade-off and layer choice, for a maintainer to weigh in:

• Blocking on a stalled reader is the pre-existing behavior. Before process.stdout is materialized, fd 1 is blocking, so console.log to a full pipe already parks the JS thread until it drains. This change restores that same semantics after the fd gets flipped to O_NONBLOCK, rather than inventing a new stall. Node behaves the same way (synchronous blocking writes to a pipe stdout).

• Why fix here and not at the O_NONBLOCK flip: the fd is a dup sharing one open file description, so the flag is visible on both the stream's fd and fd 1 by design, and materializing process.stdout intentionally makes the stream nonblocking (matching Node). Keeping the native console writer tolerant of a nonblocking fd is the narrow fix; it also covers stderr and any other consumer of fd_write_all_quiet. Unifying the two writers is a larger change worth doing separately.

• Complementary to console: surface EPIPE from console.log on process.stdout #30635 (EPIPE on the fall-through Err arm), not conflicting.

[robobun]

CI status on 7d5bb80 (build 93803): 195 passed, 1 failed.

The failure is test/js/bun/http/proxy-stress-protocol.test.ts on :debian: 13 x64-asan (ECONNRESET on one early-reply-during-upload case out of 102; the file makes no console or stdio writes, and it was already in the flaky list on build 85953). Reported for main-break triage. test/js/node/process/process-stdio.test.ts passed on every lane. Remaining annotations are the usual flaky set, all passed on retry.

[claude]

No issues found — the EAGAIN→poll(POLLOUT) retry restores the same blocking semantics fd 1 already had before process.stdout flipped it, and the fall-through Err(_) arm still ends the loop on EPIPE/POLLHUP. Deferring to a maintainer since the author explicitly flagged the layer choice for sign-off and #30635 touches the same function.

What was reviewed:

• posix::poll is already EINTR-retried; discarding its result is fine because the next write() surfaces EPIPE if the read end closed
• happy-path cost is zero — the new arms only fire on errno the old code silently dropped
• both tests deterministically overfill the 64 KiB pipe; the sleep 0.4 is the reader delay that creates the condition, not a wait-for-condition
• Windows is cfg'd/skipped; EAGAIN==EWOULDBLOCK on all supported unix targets

Extended reasoning...

Overview

Adds two #[cfg(unix)] match arms to fd_write_all_quiet in src/sys/lib.rs: retry on EINTR, and on EAGAIN block via poll(POLLOUT, -1) before retrying. This is the write loop behind the native console writer (SysQuietWriterAdapter / quiet_writer_write_all). Two new regression tests in test/js/node/process/process-stdio.test.ts reproduce the O_NONBLOCK-on-fd-1 shape via a shell pipeline with a delayed reader.

Security risks

None. No untrusted input reaches the new code — it only reacts to errno from write(2) on stdout/stderr. No new allocation, no user-controlled sizes.

Level of scrutiny

Medium-high. The diff is 13 lines of native code plus tests, but it sits directly under every console.log in the runtime. The behavioral change (block instead of drop) is defensible — it's what a blocking fd 1 already does and what Node does — but it is a design choice: the alternative fixes (don't set O_NONBLOCK on the shared OFD, or unify the two writers so console goes through the buffered stream) live at different layers. The author explicitly surfaced this trade-off in the thread and asked for maintainer input, which is the right call.

Other factors

• I confirmed posix::poll (src/sys/lib.rs:8229) is EINTR-retried internally, so the discarded return is safe; if the read end closes, poll returns with POLLERR/POLLHUP and the subsequent write() yields EPIPE → Err(_) => return false.
• My earlier minor comment (pipeline exit status is cat's, not bun's) was addressed in 9cca594 — the vacuous exitCode assertion is gone and the delivered-count is the load-bearing check.
• The comment-cop bot flagged the doc comment length; the author trimmed it to a two-line inline note in 735e326, which reads as a legitimate cross-crate hazard note rather than a workaround justification.
• #30635 modifies the same function's fall-through Err arm for EPIPE. The two changes are complementary but a maintainer should coordinate the merge order.
• Tests use /bin/sh, sleep, cat, wc — all POSIX-standard, and the block is skipIf(isWindows). The sleep 0.4 is not the flaky wait-for-condition anti-pattern; it is the slow-reader mechanism itself, and the assertion is on delivered byte/line count, not timing.

[claude]

No issues found, but this changes core console-write semantics (indefinite poll(POLLOUT, -1) on EAGAIN) and you already flagged the blocking/layer trade-off for a maintainer — leaving that call to a human, especially with #30635 touching the same function.

Checked: posix::poll already retries EINTR internally, so the discarded result can't busy-spin on signals; a POLLERR/POLLHUP wake surfaces via the next write() hitting the fall-through Err arm. EAGAIN == EWOULDBLOCK on all supported Unix targets, so the single errno match is sufficient. Callers are all the console/quiet-writer path — no other consumers of fd_write_all_quiet. Tests use the harness idioms (tempDir, Buffer.alloc over .repeat) and the sleep 0.4 is the deliberate slow-reader setup, not a condition wait.

Extended reasoning...

Overview

Two files: src/sys/lib.rs adds #[cfg(unix)] EINTR-retry and EAGAIN→poll(POLLOUT, -1) arms to the fd_write_all_quiet loop (13 lines); test/js/node/process/process-stdio.test.ts adds two POSIX-only regression tests reproducing the slow-pipe data loss after process.stdout materialization flips fd 1 to O_NONBLOCK.

Security risks

None. No untrusted input parsing; the fd being polled is the process's own stdout/stderr. The only new hazard is a liveness one (indefinite block on a stalled reader), which the author correctly notes is the pre-existing behavior when fd 1 is blocking — this restores it rather than introducing it.

Level of scrutiny

Medium-high. fd_write_all_quiet backs every console.log via SysQuietWriterAdapter / quiet_writer_write_all, so a mistake here affects all console output. The fix itself is small and idiomatic (poll-then-retry is the standard write-all pattern for a possibly-nonblocking fd), but the choice to block indefinitely rather than, say, flip the fd back to blocking or route through the node stream, is a design call the author explicitly deferred to a maintainer in the thread. #30635 also modifies this function's error arm, so merge coordination is a human concern.

Other factors

• I verified posix::poll (src/sys/lib.rs:8229) has its own EINTR loop, so let _ = on its result won't degrade to a busy-spin on signals; on any other poll failure the next write re-hits EAGAIN and re-polls, which is at worst a tight loop on a pathological poll errno that shouldn't occur on a valid fd.
• If poll wakes with POLLERR/POLLHUP/POLLNVAL in revents, the next write(2) returns the real error (EPIPE/EBADF) and hits Err(_) => return false, so the loop terminates.
• Only EAGAIN is matched; EWOULDBLOCK is the same value on Linux/macOS/BSD, so no gap there.
• Windows is unchanged (#[cfg(unix)]) and the tests skip Windows — appropriate since O_NONBLOCK on a shared open file description is the Unix-specific mechanism.
• My earlier nit (pipeline exit code being cat's, not bun's) was addressed in 9cca594; all inline threads are resolved.
• Tests follow harness conventions: tempDir, bunEnv/bunExe, Buffer.alloc(n, fill) over .repeat, await using for spawn cleanup, kept outside the concurrent block with a stated reason.

[Jarred-Sumner]

For pipes and sockets, couldn't we instead make it use sendmsg with MSG_WAIT or whatever that flag is, when the output source type is a socket/pipe (we stat on start already)? Then we don't need to poll and it will still block.

[robobun]

Unfortunately that doesn't pan out for two reasons:

• send/sendmsg only work on sockets. On a pipe they return ENOTSOCK (verified on Linux, same on macOS/BSD), and the motivating case here is bun app | consumer, i.e. a pipe.
• There is no MSG_* flag on the send side that forces blocking on an O_NONBLOCK fd. MSG_DONTWAIT goes the other direction (force nonblocking on a blocking fd), and MSG_WAITALL is receive-only. So even on a socket, send(fd, buf, len, 0) still returns EAGAIN when the send buffer is full.

The per-call options I'm aware of on POSIX are:

1. poll(POLLOUT) then retry (what this PR does). One extra syscall only on the EAGAIN path, zero cost on the happy path.
2. fcntl-clear O_NONBLOCK, write, restore. That's three syscalls instead of two, and it toggles the shared open file description, so the process.stdout FileSink briefly sees a blocking fd and another thread touching fd 1 races the window.
3. Linux-only: pwritev2(fd, iov, 1, -1, RWF_SYNC) does not override O_NONBLOCK either; there's no RWF_* that does.

Happy to switch if there's a primitive I'm missing, but as far as I can tell poll-then-retry is the portable way to block a write on a fd that someone else has flipped to nonblocking.

[coderabbitai]

Actionable comments posted: 3

🤖 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/runtime/webcore/FileSink.rs`:
- Around line 242-247: Update the `FileSink` writer closure to call the
`IOWriter::fd()` method instead of the nonexistent `get_fd()` method, preserving
the existing `force_sync` assignment and nonblocking update logic.

In `@src/sys/lib.rs`:
- Around line 9413-9425: Update the EAGAIN branch in the write helper to handle
the result from posix::poll: retry on EINTR, but return false for other poll
errors so repeated EAGAIN results cannot spin. Also document that this blocking
behavior applies to generic File::quiet_writer() callers, or restrict the
behavior to the intended descriptors.

In `@test/js/node/process/process-stdio.test.ts`:
- Around line 167-246: Mark the new independent tests in the “console.log after
process.stdout is materialized on a pipe” suite as concurrent, preferably by
applying the existing test harness’s concurrent describe convention. Keep the
suite outside the earlier stdin concurrent block and preserve each test’s
current setup and assertions.

🪄 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: 9939b7e1-58d8-445f-bc6e-764e8ec23cef

📥 Commits

Reviewing files that changed from the base of the PR and between 0316b6f and a8f370c.

📒 Files selected for processing (3)

• src/runtime/webcore/FileSink.rs
• src/sys/lib.rs
• test/js/node/process/process-stdio.test.ts

[coderabbitai]

Caution

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

⚠️ Outside diff range comments (3)

test/js/node/process/process-stdio.test.ts (3)

182-193: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift

Propagate and assert the Bun producer’s exit status.

Each test only observes the downstream cat/wc result, so a Bun child can fail after producing enough output and still make the test pass. Use an explicit status channel or a reader design that preserves the producer status, then assert it after draining stdout.

As per coding guidelines: subprocess tests must assert the complete observable result, not only downstream output.

Also applies to: 203-213, 231-236

🤖 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 `@test/js/node/process/process-stdio.test.ts` around lines 182 - 193, Update
the subprocess tests around the Bun-spawned producer to preserve and expose the
producer’s exit status instead of observing only downstream cat/wc output. Drain
stdout fully, then assert the Bun child exits successfully through an explicit
status channel or equivalent reader design; apply the same change to the related
test cases.

Source: Coding guidelines

---

224-228: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Verify that the inherited-stdout child actually ran.

If spawnSync fails or exits nonzero, the parent still writes all 20,000 lines through its original blocking descriptor, allowing this regression test to pass without exercising the intended shared-state path.

Proposed fix

-        spawnSync(process.execPath, ["-e", 'process.stdout.write("")'],
-                  { stdio: ["ignore", "inherit", "ignore"] });
+        const child = spawnSync(process.execPath, ["-e", 'process.stdout.write("")'],
+                                { stdio: ["ignore", "inherit", "ignore"] });
+        if (child.error || child.status !== 0) {
+          throw new Error(`inherited-stdout child failed: ${child.error ?? child.status}`);
+        }

As per coding guidelines: tests must assert setup preconditions before validating the final behavior.

🤖 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 `@test/js/node/process/process-stdio.test.ts` around lines 224 - 228, Update
the setup in the process-stdio test around spawnSync to capture its result and
assert that the inherited-stdout child completed successfully before writing the
20,000 lines. Keep the existing child command and stdio configuration, and
ensure the precondition assertion fails when spawnSync errors or returns a
nonzero status.

Source: Coding guidelines

---

194-196: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Assert the emitted records, not only an aggregate count.

A missing line can be offset by a duplicate valid line in the first test, and malformed content can retain the same newline count in the parent test. Capture the output and compare each expected line in order.

As per coding guidelines: every assertion must prove the strongest relevant invariant.

Also applies to: 237-238

🤖 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 `@test/js/node/process/process-stdio.test.ts` around lines 194 - 196, Update
the assertions around stdout processing in the affected tests to compare the
complete emitted records against the expected lines in order, rather than
counting regex-matching lines or relying on newline totals. Preserve the
existing output format and use the captured stdout from the relevant process
execution to validate both record content and ordering.

Source: Coding guidelines

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

Outside diff comments:
In `@test/js/node/process/process-stdio.test.ts`:
- Around line 182-193: Update the subprocess tests around the Bun-spawned
producer to preserve and expose the producer’s exit status instead of observing
only downstream cat/wc output. Drain stdout fully, then assert the Bun child
exits successfully through an explicit status channel or equivalent reader
design; apply the same change to the related test cases.
- Around line 224-228: Update the setup in the process-stdio test around
spawnSync to capture its result and assert that the inherited-stdout child
completed successfully before writing the 20,000 lines. Keep the existing child
command and stdio configuration, and ensure the precondition assertion fails
when spawnSync errors or returns a nonzero status.
- Around line 194-196: Update the assertions around stdout processing in the
affected tests to compare the complete emitted records against the expected
lines in order, rather than counting regex-matching lines or relying on newline
totals. Preserve the existing output format and use the captured stdout from the
relevant process execution to validate both record content and ordering.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 496ac8f6-040b-4327-80f2-358ce909c866

📥 Commits

Reviewing files that changed from the base of the PR and between a8f370c and 2a82767.

📒 Files selected for processing (1)

• test/js/node/process/process-stdio.test.ts

[robobun]

On the three outside-diff test comments:

• Producer exit status (182-193): already discussed and resolved in the earlier review thread. The shell pipeline's exit is cat's/wc's by POSIX semantics, and set -o pipefail isn't available under /bin/sh. The delivered-count assertion is the load-bearing check: the bug is data loss, so a count of N proves every write reached the reader. Restructuring to spawn bun directly would remove the separate late-starting reader that makes the pipe fill deterministic.
• Inherited-stdout child precondition (224-228): taken in fdaeb6b. The fixture now throws if spawnSync errors or exits nonzero, so the regression guard can't be bypassed by a failed grandchild. Re-verified: fixed binary 3/3 pass, stock binary 3/3 fail (324/20000 on the child-inherit case).
• Per-record assertion (194-196): the bug under test is data loss, not duplication or reordering; a for (let i=0; ...) console.log(...) loop cannot emit duplicates, and the 1 MiB case asserts an exact byte count via wc -c. The count is the strongest invariant for "no bytes dropped". Comparing 20,000 lines in order would turn a ~1s assertion into a multi-MB string diff without tightening what the test proves.

[coderabbitai]

Caution

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

⚠️ Outside diff range comments (3)

test/js/node/process/process-stdio.test.ts (3)

232-233: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Document the intentional one-second reader delay.

The test uses sleep 1 outside a polling loop, but the surrounding comment does not explain that this lets the pipe fill and forces the parent through the nonblocking retry path.

As per coding guidelines: “comment sleeps of 50ms or more outside polling loops.”

🤖 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 `@test/js/node/process/process-stdio.test.ts` around lines 232 - 233, Add a
concise comment near the `sleep 1` command in the `Bun.spawn` setup explaining
that the intentional one-second reader delay lets the pipe fill and forces the
parent through the nonblocking retry path. Preserve the existing test behavior
and command structure.

Source: Coding guidelines

---

190-190: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Spread bunEnv for each new spawn.

Use { ...bunEnv } at these sites to follow the test-harness convention and keep each subprocess environment independently owned.

As per coding guidelines: “Follow existing harness conventions: spread bunEnv.”

Proposed change

-      env: bunEnv,
+      env: { ...bunEnv },

Also applies to: 210-210, 234-234

🤖 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 `@test/js/node/process/process-stdio.test.ts` at line 190, Update each affected
spawn configuration in the process-stdio tests to pass a shallow copy of bunEnv
using the existing spread convention, including the sites around lines 190, 210,
and 234, so every subprocess receives an independently owned environment object.

Source: Coding guidelines

---

195-196: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Assert the complete payload, not only a loose line count.

/^O\d+ x+$/ accepts truncated lines, and filtering can hide missing, duplicate, or unexpected data. Compare the full output with the expected 1,500-line sequence.

As per coding guidelines: “Every test assertion must be able to fail and assert the strongest invariant.”

Proposed change

-    const delivered = stdout.split("\n").filter(l => /^O\d+ x+$/.test(l)).length;
-    expect(delivered).toBe(N);
+    expect(stdout.split("\n")).toEqual([
+      ...Array.from({ length: N }, (_, i) => `O${i} ${pad}`),
+      "",
+    ]);

🤖 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 `@test/js/node/process/process-stdio.test.ts` around lines 195 - 196,
Strengthen the assertion in the process-stdio test by comparing the complete
stdout payload against the expected 1,500-line sequence, rather than filtering
lines and counting matches with the loose /^O\d+ x+$/ pattern. Preserve the
expected ordering, line contents, and exact line count so missing, duplicate,
truncated, or unexpected output causes the assertion to fail.

Source: Coding guidelines

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

Outside diff comments:
In `@test/js/node/process/process-stdio.test.ts`:
- Around line 232-233: Add a concise comment near the `sleep 1` command in the
`Bun.spawn` setup explaining that the intentional one-second reader delay lets
the pipe fill and forces the parent through the nonblocking retry path. Preserve
the existing test behavior and command structure.
- Line 190: Update each affected spawn configuration in the process-stdio tests
to pass a shallow copy of bunEnv using the existing spread convention, including
the sites around lines 190, 210, and 234, so every subprocess receives an
independently owned environment object.
- Around line 195-196: Strengthen the assertion in the process-stdio test by
comparing the complete stdout payload against the expected 1,500-line sequence,
rather than filtering lines and counting matches with the loose /^O\d+ x+$/
pattern. Preserve the expected ordering, line contents, and exact line count so
missing, duplicate, truncated, or unexpected output causes the assertion to
fail.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 111d44d4-cbb7-4d1e-ad6c-292440fae740

📥 Commits

Reviewing files that changed from the base of the PR and between 2023d1f and fdaeb6b.

📒 Files selected for processing (1)

• test/js/node/process/process-stdio.test.ts

[coderabbitai]

Actionable comments posted: 1

Caution

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

⚠️ Outside diff range comments (1)

test/js/node/process/process-stdio.test.ts (1)

222-230: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

Use an ESM import in the .mjs fixture.

parent.mjs is an ESM file and does not test CommonJS loading. Replace require("node:child_process") with a top-level import.

🤖 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 `@test/js/node/process/process-stdio.test.ts` around lines 222 - 230, Update
the parent.mjs fixture to use a top-level ESM import for child_process instead
of require("node:child_process"), while preserving the existing spawnSync
behavior and test flow.

Source: Coding guidelines

🤖 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 242-258: Update the test “console.log survives a raw O_NONBLOCK on
fd 1 (isolates the writer)” to delay the pipe reader before it drains output,
following the existing slow-reader test pattern. Ensure the delayed `wc -c`
setup forces the writer to encounter EAGAIN so the assertion exercises
fd_write_all_quiet’s retry path rather than merely validating normal pipe
writes.

---

Outside diff comments:
In `@test/js/node/process/process-stdio.test.ts`:
- Around line 222-230: Update the parent.mjs fixture to use a top-level ESM
import for child_process instead of require("node:child_process"), while
preserving the existing spawnSync behavior and test flow.

🪄 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: 1baf95f4-eb1d-4f66-a889-fcbeaa96858b

📥 Commits

Reviewing files that changed from the base of the PR and between fdaeb6b and 73ff61e.

📒 Files selected for processing (1)

• test/js/node/process/process-stdio.test.ts

[claude]

My earlier isolating-test concern is addressed in 73ff61e; no further issues found this pass. Deferring to Jarred on the poll-vs-alternative call he raised — the response (sendmsg is socket-only; no send-side flag overrides O_NONBLOCK) reads correct to me, but this changes the writer behind every console.log and deserves his sign-off.

What was reviewed:

• fd_write_all_quiet: posix::poll already loops on EINTR, so Err → return false prevents an EAGAIN busy-spin; EPIPE/other errors still fall through the final arm.
• writer.get_fd() resolves to StreamingWriter::get_fd (src/io/PipeWriter.rs:699), not the unrelated shell IOWriter.
• The delayed reader robobun said was in ebc63ae isn't on the branch (HEAD is 73ff61e), but the isolating test still pins the sys fix as-is: 1 MiB into a 64 KiB pipe hits EAGAIN before wc can drain, and A-only was verified failing at 73ff61e.

Extended reasoning...

Overview

Two small native changes plus four POSIX-only regression tests. Bun__ForceFileSinkToBeSynchronousForProcessObjectStdio now reads the fd from writer.get_fd() (where FileSink::setup actually stores it) instead of the never-populated this.fd, so update_nonblocking(fd, false) runs and clears the O_NONBLOCK that open_for_writing set on the shared open file description. Separately, fd_write_all_quiet in src/sys/lib.rs gains EINTR retry and a poll(POLLOUT) wait on EAGAIN so the native console writer never silently drops a partial write when fd 1/2 has been flipped nonblocking by anything (including a non-bun child on the shared OFD).

Security risks

None identified. No untrusted-input parsing, no auth/crypto, no new external surface. The new poll call is on an fd the process already owns; the only behavioral change is that a write that would previously have been truncated now blocks until writable, which restores the pre-materialization semantics of a blocking fd 1.

Level of scrutiny

High. fd_write_all_quiet sits under SysQuietWriterAdapter, which backs bun_core::Output — i.e. every console.* and scoped-log write on POSIX. A mistake here (busy-spin, deadlock, dropped bytes) would be user-visible everywhere. The FileSink change is narrower (only the process.stdout/stderr force-sync path) but still touches process-wide fd flags on a shared open file description. Both are cfg-gated to unix.

Other factors

• Jarred asked whether sendmsg/MSG_WAIT could avoid the poll. The reply is technically sound — sendmsg returns ENOTSOCK on pipes and there is no send-side flag that overrides O_NONBLOCK — but he hasn't acknowledged it, so the design choice is still awaiting maintainer confirmation.
• My previous review flagged that neither fix had an isolating test; 73ff61e adds one that leaves O_NONBLOCK set via Bun.file(1).writer() (bypasses ForceFileSinkToBeSynchronous) and was verified to fail with only the FileSink hunk applied. That thread is resolved.
• robobun told CodeRabbit the isolating test's reader was delayed in commit ebc63ae, but that commit is not on the branch (HEAD is 73ff61e, and git log --all has no ebc63ae). Verifiers examined this and concluded it does not weaken the test: a single 1 MiB write into a 64 KiB pipe returns a partial count then EAGAIN before wc -c can drain regardless, and the A-only failure was demonstrated at 73ff61e without the sleep. Not a blocker, but worth noting the thread's stated resolution doesn't match the branch.
• Tests use /bin/sh pipelines with fixed sleeps (0.4s / 1s) to force backpressure. That is the observable condition here (there is no in-process signal for "pipe filled"), and the suite is deliberately kept out of the file's describe.concurrent block after robobun reproduced it flaking the earlier stdin tests.

Given the maintainer engagement on the approach and the breadth of the code path, this should get a human sign-off rather than a bot approval.

[robobun]

On the review note that ebc63ae isn't on the branch: it is. git ls-remote and the PR head both show ebc63ae4b4e0a67cab63a215d30ded8db1c689b6; the review ran against a snapshot fetched before that push landed. The delayed reader for the isolating test is present at HEAD.

[robobun]

Verified a third face of this bug against this branch: bun test --parallel silently loses worker output with no slow reader and no user setup (workers write to socketpairs drained by a busy coordinator, and a test file touching process.stdout/process.stderr flips the worker's fd via the same leaked O_NONBLOCK).

Fixture: 4 files x 100k console.log lines plus one file doing 20480 x 1 KiB process.stdout.write and 5120 x 1 KiB process.stderr.write, run with --parallel=4, output redirected to files.

	stock 1.4.0	this branch (3/3 runs)
worker console.log lines	71378 / 100000	100000 / 100000
process.stdout.write lines	9907 / 20480	20480 / 20480
process.stderr.write lines	1659 / 5120	5120 / 5120

Serial bun test is complete on both, unchanged.

This also settles whether the bulk process.stdout.write loss was a separate FileSink-queue bug: it is the same root. The leaked O_NONBLOCK made the FileSink's force-sync write loop fail on EAGAIN the same way the console writer did, and clearing the flag in ForceFileSinkToBeSynchronousForProcessObjectStdio fixes it. Not adding the fixture as a CI test: the loss depends on coordinator scheduling, so it is not deterministic enough for the gate, and the four existing tests already pin both src hunks individually.

[alii]

Requesting changes. The lib.rs retry is right and is the only thing the four new tests exercise. The FileSink.rs change is a separate behavior change: process.stdout.write() to a pipe or socket now blocks the JS thread instead of returning false and emitting drain, and the body does not say so.

• FileSink.rs:247: say what it changes and pin it in a test, or land the lib.rs half on its own.
• Nothing fails without the FileSink change. What it actually fixes is what non-bun children inherit; test that.
• Tests 1 and 3 stop discriminating when bun starts slower than the reader sleep; use the single 1 MiB write shape test 2 already uses.

[alii]

Looks ready to merge from this side. 7d5bb80 drops the FileSink change, so the PR is now just the fd_write_all_quiet retry plus tests; the three loss tests each deliver one pipe buffer (65536 here) against an unfixed debug build and 1048577 under node with no reader delay, the inherited-fd one goes back to 1048577 once the child stops touching its stdout, and the new async test returns false plus drain on main and node with both a fast and a 0.5s-late reader. Not rebuilt with the patch, so the fixed-side numbers in the body are unverified here; the lib.rs hunk still applies cleanly to current main even though the surrounding file has moved.

[robobun]

Thanks. On the fixed-side numbers: CI is the verification for those. The three loss tests assert exactly 1048577 and the async test asserts false plus drain, and process-stdio.test.ts passed on every lane that ran it in build 93803 against 7d5bb80 (the one red lane there is an unrelated proxy race, already reported).

One housekeeping note: the earlier Changes Requested review is still the recorded state on the PR, so it will need to be re-reviewed or dismissed before the merge button unlocks. Happy to rebase if you'd rather CI run against current main first; the hunk applies cleanly as you say, so I've left the branch alone to avoid churning reviews.

[robobun]

Leaving this one open while closing the other PRs that #37128 lists as superseded: it was reviewed above as ready to merge on its own, and #37128 carries the same fd_write_all_quiet change (EINTR retry, poll(POLLOUT) on EAGAIN), so whichever lands first, the other rebases over this 15-line hunk.