fs: report write-after-end on the WriteStream fast path

[robobun]

Repro

process.stdout.on("error", e => process.stderr.write("error-event:" + e.code + "\n"));
process.stdout.write("kept\n");
process.stdout.end();
const ret = process.stdout.write("dropped\n", err => {
  process.stderr.write("cb:" + (err && err.code) + "\n");
});
process.stderr.write("ret:" + ret + "\n");

	stdout	stderr
node v26	kept	ret:false, cb:ERR_STREAM_WRITE_AFTER_END, error-event:ERR_STREAM_WRITE_AFTER_END
bun	kept, dropped	ret:true, cb:null

Bun writes the chunk that was submitted after end(), returns true, and calls the write callback with no error. Same shape on a child process's stdin:

const cp = spawn("cat", [], { stdio: ["pipe", "ignore", "ignore"] });
cp.stdin.end();
cp.stdin.write("after-end", err => console.log("cb:", err?.code)); // node: ERR_STREAM_WRITE_AFTER_END, bun: undefined

Cause

writeFast() in src/js/internal/fs/streams.ts is installed as WriteStream.prototype.write whenever the FileSink fast path is enabled. That covers process.stdout, process.stderr (via ProcessObjectInternals and node:tty) and a child process's stdin (via writableFromFileSink).

Writable.prototype.write is where the ERR_STREAM_WRITE_AFTER_END / ERR_STREAM_DESTROYED checks live. writeFast() replaces it and goes straight to the sink without looking at the stream state, so neither check ran. For process.stdout the sink also still had an open fd, so the bytes really were written.

Fix

When the stream is already ending or destroyed, hand the write back to Writable.prototype.write, which drops the chunk and reports the error through the write callback and an 'error' event. Everything else keeps the fast path.

Verification

test/js/node/child_process/child-process-stdio.test.js:

• write after end() returns false and the callback gets ERR_STREAM_WRITE_AFTER_END; the bytes never reach the child
• a callback-less write after end() emits 'error' on child.stdin
• write after destroy() reports ERR_STREAM_DESTROYED

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

• process.stdout write after end() is dropped, returns false, and reports through both the callback and 'error'

All four fail on 1.4.0 and pass with this change. test/js/node/{fs,child_process,process,tty,stream,console} and the test-file-write-stream* / test-child-process-stdio node harness tests show no new failures.

Out of scope: a write to the stdin of a child that has already exited is still accepted silently (node reports ERR_STREAM_DESTROYED). Nothing tells the JS stream that the subprocess closed the pipe, so that needs separate plumbing. The Bun.spawn / FileSink side of the same behavior is #32857.

[coderabbitai]

Warning

Review limit reached

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

Next review available in: 1 minute

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: 4fdf822e-b6b2-41a0-b85a-daac6021ceac

📥 Commits

Reviewing files that changed from the base of the PR and between 48ff9eb and 811d93b.

📒 Files selected for processing (3)

• src/js/internal/fs/streams.ts
• test/js/node/child_process/child-process-stdio.test.js
• test/js/node/process/process-stdio.test.ts

Walkthrough

This PR adds a guard in the fast-path write function to defer to Node's standard Writable.prototype.write when a stream is already ended or destroyed, and adds tests for child.stdin and process.stdout verifying write-after-end and write-after-destroy error behavior.

Changes

Fast-path write guard and validation tests

Layer / File(s)	Summary
writeFast early guard src/js/internal/fs/streams.ts	Adds a check for writableEnded/destroyed in writeFast that reroutes to writablePrototypeWrite, preserving Node's built-in write-after-end/destroyed error handling.
child.stdin write-after-end/destroy tests test/js/node/child_process/child-process-stdio.test.js	Adds a describe("child.stdin") suite testing writes after end() (with and without callback) and after destroy(), asserting ERR_STREAM_WRITE_AFTER_END/ERR_STREAM_DESTROYED codes, write() returning false, and dropped bytes not reaching the child.
process.stdout write-after-end test test/js/node/process/process-stdio.test.ts	Adds a spawned-process test writing to stdout, ending it, then writing again, asserting dropped output, callback/error-event reporting of ERR_STREAM_WRITE_AFTER_END, and successful process exit.

Sequence Diagram(s)

sequenceDiagram
  participant Caller
  participant writeFast
  participant WritablePrototypeWrite as Writable.prototype.write

  Caller->>writeFast: write(chunk, encoding, callback)
  alt stream ended or destroyed
    writeFast->>WritablePrototypeWrite: forward write(chunk, encoding, callback)
    WritablePrototypeWrite-->>Caller: emit error (ERR_STREAM_WRITE_AFTER_END / ERR_STREAM_DESTROYED), return false
  else stream active
    writeFast->>writeFast: continue fast-path sink handling
  end

Loading

Related Issues: None referenced in the provided summary.

Related PRs: None referenced in the provided summary.

Suggested labels: bug, streams, node-compat, needs-tests

Suggested reviewers: None identifiable from the provided information.

🐰 A stream that's ended should not sneak past,
No fast-path write when the door's shut fast,
Errors now surface, callbacks now ring,
child.stdin and stdout, tested each thing,
Hop along, safely, the writes now last.

---

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

[robobun]

^{Updated 8:22 AM PT - Jul 6th, 2026}

@robobun, your commit 811d93b is building: #69088

[robobun]

The review run failed with an internal error (RST_STREAM), so no findings landed. Retrying.

@coderabbitai review

[coderabbitai]

@robobun Got it, kicking off the review now.

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[github-actions]

This PR may be a duplicate of:

1. stdio: route process.stdout/stderr writes through Writable's buffer accounting #33508 - Supersedes this PR's approach by removing writeFast() entirely and routing all stdio writes through Writable's standard buffer accounting, which inherently fixes write-after-end/destroy error reporting along with four other writeFast bugs

🤖 Generated with Claude Code

[claude]

LGTM — minimal guard that delegates write-after-end/destroy back to the standard Writable path; the inline nit is just a stale test comment.

Extended reasoning...

Overview

This PR adds a 4-line guard to writeFast() in src/js/internal/fs/streams.ts: when writableEnded || destroyed, delegate the write back to Writable.prototype.write so the standard ERR_STREAM_WRITE_AFTER_END / ERR_STREAM_DESTROYED reporting runs instead of silently writing through to the FileSink. The rest of the diff is four new tests covering child.stdin (write-after-end with and without callback, write-after-destroy) and process.stdout (write-after-end via subprocess).

Security risks

None. This is Node-compat error-reporting behavior for writable streams; no untrusted input parsing, auth, crypto, or filesystem-path handling is touched. The change strictly narrows what the fast path accepts (rejecting writes after end/destroy) rather than accepting anything new.

Level of scrutiny

Moderate — writeFast is on the hot path for process.stdout/stderr and child-process stdin, so a mistake here would be very visible. But the runtime change is tiny and conservative: it reuses the exact delegation pattern already present one line above (writablePrototypeWrite.$call for the monkey-patch-defense case), and only fires when the stream is already ended/destroyed — a state where the previous behavior was clearly wrong (bytes written after end()). The two added property reads (writableEnded, destroyed) are standard Writable prototype getters backed by _writableState bitflags, so per-write overhead is negligible. writableEnded maps to state.ending, which is exactly what Writable.prototype.write checks for ERR_STREAM_WRITE_AFTER_END, so the guard and the delegated check agree.

Other factors

• The PR description includes a clear repro table vs Node v26 and confirms all four new tests fail on 1.4.0 and pass with the change, plus no new failures across the fs/child_process/process/tty/stream/console suites and the relevant node-harness tests.
• The one issue the bug hunt surfaced is a nit: a test comment at child-process-stdio.test.js:136 says the stream will emit 'error', but the assertion below correctly expects errorEvents to be empty (because autoDestroy has already run by the time the post-finish write happens, so errorOrDestroy short-circuits). The assertion is right; only the comment wording is stale. This does not affect test validity or runtime behavior and is trivially fixable.
• No prior reviews or outstanding comments on the PR.

Given the minimal, pattern-following runtime change and solid test coverage, this is safe to approve; the comment nit can be addressed in a follow-up or before merge without blocking.

[claude]

🟡 The comment says "Surfacing the error also destroys the stream, which emits 'error'" — but the assertion below is expect(errorEvents).toEqual([]), which asserts the opposite. The assertion is correct (after await finished.promise autoDestroy has already run, so errorOrDestroy short-circuits and no 'error' fires); the comment should be reworded to say the handler is registered so an unexpected 'error' won't crash the test, and that none is expected here.

Extended reasoning...

What the bug is

The comment on line 136 of test/js/node/child_process/child-process-stdio.test.js reads:

// Surfacing the error also destroys the stream, which emits 'error'.

This directly implies that the write-after-end error will cause an 'error' event to be emitted on child.stdin, and that errorEvents will capture it. But sixteen lines later the test asserts:

expect(errorEvents).toEqual([]);

The comment and the assertion contradict each other. A reader cannot tell which one reflects the intended behavior.

Which one is correct

The assertion is correct; the comment is wrong. Step-by-step:

1. child.stdin is a WriteStream created via writableFromFileSink, which uses the default autoClose → autoDestroy: true.
2. child.stdin.end("kept\n") runs. When the writable finishes, finish() in src/js/internal/streams/writable.ts emits 'finish' and then — synchronously in the same tick, because autoDestroy is set — calls stream.destroy(), which sets kDestroyed.
3. await finished.promise resumes on the microtask after 'finish' fired, so by the time the test continues, _writableState already has kDestroyed set.
4. child.stdin.write("dropped\n", cb) now hits the new guard in writeFast (this.writableEnded || this.destroyed) and delegates to Writable.prototype.write. That path constructs ERR_STREAM_WRITE_AFTER_END, schedules process.nextTick(cb, err), and calls errorOrDestroy(stream, err, true).
5. errorOrDestroy (in src/js/internal/streams/destroy.ts) checks w.destroyed first and returns early — so no 'error' event is emitted.

Result: the callback receives ERR_STREAM_WRITE_AFTER_END, but errorEvents stays []. This matches Node's behavior for this exact sequence, and the assertion toEqual([]) is empirically right.

Why the comment is misleading

The comment describes the other scenario in this PR — the process-stdio test and the second child.stdin test — where write() is called immediately after end() without awaiting 'finish'. In that case the stream is not yet destroyed, errorOrDestroy proceeds, and both the callback and an 'error' event fire. That is what "Surfacing the error also destroys the stream, which emits 'error'" describes. But this test deliberately awaits 'finish' first, so it exercises the already-destroyed branch instead.

Impact

This is a documentation-only issue in test code. The test is functionally correct and not flaky; the fix in streams.ts is unaffected. The only cost is reader confusion: someone diffing the comment against the [] assertion will assume one of them is a mistake and won't know which.

Suggested fix

Reword the comment to match what the test actually verifies, e.g.:

// Register an 'error' handler so an unexpected emission doesn't crash the test.
// None is expected here: autoDestroy has already run by the time we write, so
// errorOrDestroy short-circuits and only the callback receives the error.
const errorEvents = [];
child.stdin.on("error", err => errorEvents.push(err.code));

(or simply drop the comment — the toEqual([]) assertion is self-explanatory once the misleading claim is gone).

[robobun]

Superseded by #33508, closing.

That PR deletes writeFast() entirely and routes stdio writes back through Writable's buffer accounting. Since Writable.prototype.write is where the ERR_STREAM_WRITE_AFTER_END / ERR_STREAM_DESTROYED checks live, it fixes this bug as a side effect, along with four others off the same bypass (writableLength/writableNeedDrain stuck at zero, cork() buffering nothing, write(chunk, encoding) ignoring the encoding, out-of-order write callbacks). It was opened before this one; I just did not find it when I checked for duplicates.

I verified the supersession rather than assuming it: the four tests from this PR pass unmodified against #33508's implementation, and I have pushed them onto that branch in ceb1c9b1 so the behavior stays pinned. #33508 had no coverage for it.

@claude thanks, the stale comment was right and is fixed in the moved copy: the handler is registered so an unexpected emission fails the assertion rather than crashing the test, and none is expected because autoDestroy has already run by then.

If the full writeFast removal turns out to be more churn than wanted on that hot path, the minimal 7-line guard is still on farm/1c11169c/writestream-fastpath-write-after-end and can be reopened.