Skip to content

stdio: reset writable state after end() on file-backed process.stdout/stderr - #33618

Merged
Jarred-Sumner merged 4 commits into
mainfrom
claude/fix-file-stdio-write-after-end
Jul 8, 2026
Merged

Jarred-Sumner merged 4 commits into
mainfrom
claude/fix-file-stdio-write-after-end

Conversation

@robobun

@robobun robobun commented Jul 7, 2026

Copy link
Copy Markdown
Collaborator

What

Regression from b280ca3 (#33557). When process.stdout / process.stderr is a regular file (bun app.js > out.txt, cron/CI logs, spawn(..., { stdio: [.., fileFd, ..] })) and the program calls process.stdout.end(), either directly or via stream.pipeline(src, process.stdout) which always ends its destination, every later process.stdout.write() is now rejected with ERR_STREAM_WRITE_AFTER_END, an 'error' event fires (a fatal uncaught exception when there is no 'error' listener on stdout), and the bytes never reach the file. Node delivers them; so did Bun 1.4.0 and every earlier main.

Repro

// parent spawns this with stdout redirected to a file fd
const so = process.stdout;
so.on("error", e => {});
so.write("A");
so.end("B");
setTimeout(() => {
  so.write("C");      // node + bun 1.4.0: succeeds, "C" lands in the file
  console.log("D");   // main: write("C") -> ERR_STREAM_WRITE_AFTER_END, file gets "ABD\n"
}, 50);
runtime writableEnded after cycle later write("C") file contents
node v26 false succeeds "ABCD\n"
bun 1.4.0 true succeeds (fast path bypassed state) "ABCD\n"
bun main true ERR_STREAM_WRITE_AFTER_END "ABD\n"
this PR false succeeds "ABCD\n"

Cause

#33557 correctly made the fast-path write() delegate to Writable.prototype.write when state.ending is set, so piped stdio honors the write-after-end contract. File-backed stdio goes through the same WriteStream fast path, and its state.ending also latches: the stream is created with autoClose: false, which fs.WriteStream maps to autoDestroy: false, so end() reaches 'finish' but never runs destroy(). Node's file-backed stdio is a SyncWriteStream with autoDestroy: true; end() there runs finish -> destroy -> dummyDestroy -> _undestroy(), which resets ending/ended/finished so the stream is writable again.

Fix

Set _writableState.autoDestroy = true on file-backed stdio (fdType === file) in getStdioWriteStream. The existing stdio _destroy override (cb(err); this._undestroy()) then runs after 'finish' and resets writable state, matching Node's SyncWriteStream. Pipe/socket stdio keeps autoDestroy: false, so the write-after-end rejection from #33557 is preserved there (Node's piped stdout also rejects, via EPIPE after net.Socket half-close).

This also fixes the long-standing divergence where process.stdout.writableEnded stayed true on file-backed stdio after end() (Node reports false), and makes 'close' fire like it does in Node.

Verification

test/js/node/process/process-stdout-write-after-end.test.ts gains file-backed cases for both stdout and stderr that redirect the fixture's target stream to a temp-file fd, assert the post-end write succeeds with {writableEnded: false, ret: true, cbErr: null, ev: []}, and assert the file received "ABCD\n". Both new cases fail on the released binary and on current main without this change; all four cases (piped + file) pass with it. The fixture also produces identical output under Node.

…/stderr

b280ca3 made the fast-path write() consult writable state so a piped
process.stdout rejects writes after end(). File-backed stdio went through
the same check: its writable state also latched ending/ended because the
stream is created with autoClose:false, which WriteStream maps to
autoDestroy:false, so the finish -> destroy -> _undestroy cycle that
Node's SyncWriteStream relies on to reset state never ran. A later
process.stdout.write() then failed with ERR_STREAM_WRITE_AFTER_END and
the bytes were dropped from the output file, which is the
pipeline(src, process.stdout) followed by more logging shape.

Enable autoDestroy on file-backed stdio so end() runs through the
existing stdio _destroy override (cb + _undestroy), matching Node's
SyncWriteStream: writableEnded returns to false and later writes
succeed. Pipe/socket stdio keeps autoDestroy:false so the
write-after-end rejection from b280ca3 is preserved there.
@robobun

robobun commented Jul 7, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 5:05 AM PT - Jul 7th, 2026

❌ @robobun, your commit adeb9c1 has some failures in Build #69724 (All Failures)


🧪   To try this PR locally:

bunx bun-pr 33618

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

bun-33618 --bun

@github-actions github-actions Bot added the claude label Jul 7, 2026
@github-actions

github-actions Bot commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

This PR may be a duplicate of:

  1. stdio: route process.stdout/stderr writes through Writable's buffer accounting #33508 - Superset fix that routes all process.stdout/stderr writes through Writable's buffer accounting, which also resolves the write-after-end regression from stdio: enforce write-after-end contract for piped process.stdout/stderr #33557 by deleting the writeFast bypass entirely

🤖 Generated with Claude Code

@robobun

robobun commented Jul 7, 2026

Copy link
Copy Markdown
Collaborator Author

Not a duplicate of #33508; they are complementary.

#33508 deletes the writeFast override in src/js/internal/fs/streams.ts so stdio writes go through Writable.prototype.write. It does not touch src/js/builtins/ProcessObjectInternals.ts, so file-backed stdio is still constructed with autoDestroy: false (derived from autoClose: false). After end() on a file-backed process.stdout, state.ending stays latched, and Writable.prototype.write (which #33508 now routes through unconditionally) rejects with ERR_STREAM_WRITE_AFTER_END. #33508's own write-after-end test uses stdout: "pipe", so it does not exercise the file path.

This PR changes where the stream is constructed, not how writes are routed: it enables autoDestroy on file-backed stdio so end() runs the existing _destroy override, which _undestroy()s the state back to writable. That is what Node's SyncWriteStream does, and it is needed whether writeFast exists or not. The two changes touch disjoint files and can land independently.

@coderabbitai

coderabbitai Bot commented Jul 7, 2026 •

Copy link
Copy Markdown
Contributor

Review Change Stack

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 2 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 97885a14-189e-4b15-b07a-f6aa91e19f13

📥 Commits

Reviewing files that changed from the base of the PR and between 7664448 and adeb9c1.

📒 Files selected for processing (1)
  • src/js/builtins/ProcessObjectInternals.ts

Walkthrough

Adds a test fixture that writes to stdout or stderr, ends the stream, and attempts a post-end write while reporting the outcome as JSON to the opposite stream. A new parameterized test verifies write-after-end delivery succeeds when the target is a regular file.

Changes

File-backed stdio write-after-end test

Layer / File(s) Summary
Test fixture for post-end write behavior
test/js/node/process/process-stdout-write-after-end-file-fixture.mjs
New fixture selects stdout or stderr, writes/ends data, waits for finish and drain ticks, attempts a post-end write with a callback, and emits a JSON report (writableEnded, writable, write() return, callback error, recorded errors) to the opposite stream.
Test case for file-backed streams
test/js/node/process/process-stdout-write-after-end.test.ts
Imports tempDir and fs, and adds a concurrent parameterized test per stream that redirects the target to a temp file, parses the JSON report from the piped stream, asserts successful post-end write delivery, and verifies file contents contain "ABCD\n".

Sequence Diagram(s)

sequenceDiagram
  participant Test
  participant Fixture
  participant Stream
  participant OtherStream
  Test->>Fixture: spawn with stream redirected to file
  Fixture->>Stream: write("A"), end("B")
  Stream-->>Fixture: "finish" event
  Fixture->>Stream: write("C") with callback
  Stream-->>Fixture: callback error (if any)
  Fixture->>Stream: console log "D"
  Fixture->>OtherStream: write JSON report
  Test->>OtherStream: parse JSON report
  Test->>Test: verify file contents "ABCD\n"
Loading

Estimated code review effort: 3/5

Related issues: None referenced.

Related PRs: None referenced.

Suggested labels: test, node.js

Suggested reviewers: None identified.

🐰 A stream that ends but writes once more,
to files it lands, through pipes it tore,
with A then B then C then D,
a JSON note for all to see.

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title is specific and matches the main change: resetting writable state for file-backed stdio after end().
Description check ✅ Passed It clearly explains the fix and verification, and covers both required sections despite using different headings than the template.
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.

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

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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-stdout-write-after-end.test.ts`:
- Around line 47-92: The subprocess/filesystem test in the process stdio
write-after-end suite should be made concurrent. Update the relevant `test.each`
cases, including the sibling file-backed variant in
`process-stdout-write-after-end.test.ts`, to `test.concurrent.each` so each
parameterized run can execute in parallel with its own `tempDir` and file
descriptor without shared state. Keep the existing `Bun.spawn`,
`fs.openSync`/`closeSync`, and `fs.readFileSync` flow unchanged, and ensure the
helper fixture references still point to the same
`process-stdout-write-after-end-file-fixture.mjs` logic.
🪄 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: 902df9ca-69ae-40c6-8417-8734570d1ad9

📥 Commits

Reviewing files that changed from the base of the PR and between 3f67971 and 92e8eef.

📒 Files selected for processing (3)
  • src/js/builtins/ProcessObjectInternals.ts
  • test/js/node/process/process-stdout-write-after-end-file-fixture.mjs
  • test/js/node/process/process-stdout-write-after-end.test.ts

Comment thread test/js/node/process/process-stdout-write-after-end.test.ts Outdated

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — targeted one-line regression fix with thorough tests.

Extended reasoning...

Overview

This PR fixes a regression from #33557 where process.stdout.end() on file-backed stdio (e.g. bun app.js > out.txt) permanently latches writableEnded, causing subsequent writes to fail with ERR_STREAM_WRITE_AFTER_END. The production change is a single else branch in getStdioWriteStream that sets _writableState.autoDestroy = true for file-backed stdio only, so the existing _destroy → _undestroy() override runs after 'finish' and resets writable state — mirroring Node's SyncWriteStream behavior. The rest of the diff is a new fixture and two new test.each cases covering stdout/stderr redirected to a file fd.

Security risks

None. This is a Node.js-compat behavioral fix to stdio stream state management. No parsing of untrusted input, no auth/crypto/permissions, no new external surface.

Level of scrutiny

Low-to-medium. The production change is 5 lines (one field assignment plus a comment) in a narrowly-scoped else branch that only fires when fdType === file, so pipe/socket stdio retains the write-after-end rejection from #33557 (verified by the existing piped test cases in the same file). The PR description traces the mechanism precisely (autoClose:false → autoDestroy:false → destroy never runs → _undestroy never resets ending) and cites the Node reference implementation. The fix reuses the already-present _destroy override rather than adding new machinery.

Other factors

Tests follow repo conventions well: tempDir/bunEnv/bunExe from harness, fd cleanup in finally, concurrent drain of report stream + proc.exited, exit-code asserted last, ASAN/debug noise on fd 2 handled via toContain while stdout uses exact toBe. The fixture awaits 'finish' + one setImmediate rather than a timeout. The PR states both new cases fail on the released binary and on current main, and the fixture produces identical output under Node. No prior reviewer comments to address.

robobun added 2 commits July 7, 2026 05:54
The previous commit inadvertently dropped the
src/js/builtins/ProcessObjectInternals.ts hunk while applying the
test.concurrent.each change.
Comment thread test/js/node/process/process-stdout-write-after-end.test.ts

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for restoring the source fix — the diff now looks correct to me, but since this changes stream lifecycle behavior on the core process.stdout/stderr path (now emits 'close' and runs destroy → _undestroy after every end() on file-backed stdio), it's worth a human sign-off.

Extended reasoning...

Overview

The PR is a one-line semantic change in src/js/builtins/ProcessObjectInternals.ts (getStdioWriteStream): when stdout/stderr is backed by a regular file (not TTY/pipe/socket), set _writableState.autoDestroy = true after constructing the fs.WriteStream. This wires end() into the existing _destroy override (which calls _undestroy() and emits 'close'), so writable state resets and post-end() writes succeed — matching Node's SyncWriteStream. It also adds a fixture and two parameterized file-backed test cases alongside the existing piped cases.

My earlier review flagged that commit 7664448 had accidentally dropped the source change; commit 2104091 restored it and the net diff now matches the PR description. No bugs were found by the bug-hunting pass on the restored diff.

Security risks

None. No untrusted input parsing, auth, crypto, or filesystem-path handling is touched. The change only flips an internal Writable state flag on a stream Bun itself constructs for fd 1/2.

Level of scrutiny

Moderate-to-high. The diff is tiny, but process.stdout/process.stderr is exercised by essentially every Bun program, and the file-backed branch is hit whenever output is redirected (> out.txt, cron/CI, spawn with a file fd). The change alters observable stream lifecycle: end() on file-backed stdio will now run destroy() → the stdio _destroy override → _undestroy(), and will start emitting 'close' where it previously did not. The PR argues this matches Node, and the reasoning traces cleanly through Node's SyncWriteStream (autoDestroy: true + dummyDestroy → _undestroy), but the interaction between autoDestroy, the custom _destroy/_final overrides, the `` sink, and emitClose is subtle enough that a maintainer familiar with the stdio fast-path (#33557, #33508) should confirm there's no unintended fallout for programs that call `end()` on file-backed stdio without expecting `'close'`.

Other factors

  • Tests are well-constructed: parameterized over stdout/stderr, assert exact JSON facts (writableEnded, writable, ret, cbErr, ev) and file contents, handle ASAN/debug noise on fd 2, clean up the fd in finally, and were verified to fail on both the released binary and current main.
  • The piped-stdio behavior from #33557 is preserved (tests for that still pass and autoDestroy remains false on the pipe/socket branch).
  • There is a related open PR (#33508) touching adjacent write-routing logic; the author's analysis that they're complementary looks correct, but coordination is a human call.

@robobun

robobun commented Jul 7, 2026

Copy link
Copy Markdown
Collaborator Author

The diff is ready. test/js/node/process/process-stdout-write-after-end.test.ts (all four cases, piped + file, stdout + stderr) passes on every lane in both CI runs.

CI is red on unrelated flakes that differ between the two runs:

  • build 69711: Windows install tests, napi, webview-chrome animation, bake dev-and-prod, sql-postgres-datetime, spawn-pipe-leak, net-mongodb-pattern-leak
  • build 69724: hot.test.ts sourcemap, net-connect-econnrefused timeout, install/security-provider, zlib leak, solc, napi_wrap, fetch-leak, postgres-multi-statement-fields, in-process-cron

None touch process.stdout/process.stderr, ProcessObjectInternals.ts, or fs.WriteStream. The two failure sets barely overlap, so these are transient. Needs a maintainer to merge past the flake.

@Jarred-Sumner
Jarred-Sumner merged commit 5cede9c into main Jul 8, 2026
75 of 77 checks passed
@Jarred-Sumner
Jarred-Sumner deleted the claude/fix-file-stdio-write-after-end branch July 8, 2026 06:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants