fs.Utf8Stream: don't emit post-reopen 'drain' while a write is in flight

[robobun]

test/js/node/test/parallel/test-fastutf8stream-reopen.js has been flaky on roughly 30% of recent CI builds across all platforms (ubuntu, alpine, debian, mac, windows), failing with:

AssertionError: Expected values to be strictly equal:
+ ''
- 'after reopen\n'

Repro

Deterministic, by deferring the post-reopen fs.write callback past nextTick:

const { Utf8Stream } = require("node:fs");
const fs = require("node:fs");

let afterReopen = false;
const slowFs = {
  write(fd, data, enc, cb) {
    if (afterReopen) setImmediate(() => fs.write(fd, data, enc, cb));
    else fs.write(fd, data, enc, cb);
  },
};
const stream = new Utf8Stream({ dest: "/tmp/a.log", sync: false, fs: slowFs });
stream.write("hello\n");
stream.once("drain", () => {
  fs.renameSync("/tmp/a.log", "/tmp/a.log-moved");
  stream.reopen();
  stream.once("ready", () => {
    afterReopen = true;
    stream.write("after reopen\n");
    stream.once("drain", () => {
      console.log("writing:", stream.writing);              // true
      console.log(fs.readFileSync("/tmp/a.log", "utf8"));   // ""
    });
  });
});

Cause

#openFile's fileOpened callback, on a successful reopen in async mode, does:

this.#writing = false;
this.emit("ready");           // synchronous
if ((!this.#writing && this.#len > this.#minLength) || this.#flushPending) {
  this.#actualWrite();
} else if (reopening) {
  process.nextTick(() => this.emit("drain"));
}

If a 'ready' listener calls write(), #actualWrite starts an async fs.write and sets #writing=true. The first branch's !this.#writing is now false, so it falls through to the reopening branch and schedules a nextTick 'drain'. That drain fires while the write is still in flight; a once('drain') listener that reads the file observes it empty.

The vendored test relies on Node's libuv threadpool completing the physical write(2) before nextTick fires (even though the callback runs later). Under CI threadpool contention, Bun's fs.write hasn't reached disk yet when nextTick runs, exposing the race. Node v26.3.0 has the identical logic and fails the same deterministic repro above.

Fix

Guard the post-reopen drain on !this.#writing. When a write is in flight, #release emits 'drain' once it completes.

Verification

$ bun bd test test/js/node/fs/utf8stream-reopen-drain.test.ts
(pass) fs.Utf8Stream: reopen does not emit 'drain' while a write started from 'ready' is pending

Fail-before (src/ stashed): AssertionError: 'drain' fired while a write is in flight. All 14 test-fastutf8stream-*.js vendored tests pass with the fix.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: b3c954f1-ae41-41bf-960c-bc9d8bff41cb

📥 Commits

Reviewing files that changed from the base of the PR and between e61c15e and ad5377c.

📒 Files selected for processing (2)

• src/js/internal/streams/fast-utf8-stream.ts
• test/js/node/fs/utf8stream-reopen-drain.test.ts

---

Walkthrough

Changes

Utf8Stream reopen drain handling

Layer / File(s)	Summary
Guard drain during reopened writes src/js/internal/streams/fast-utf8-stream.ts, test/js/node/fs/utf8stream-reopen-drain.test.ts	The reopen path avoids scheduling drain while an asynchronous write is active. A deterministic test verifies event ordering, process results, and file contents after reopening.

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly matches the main fix: suppressing post-reopen 'drain' while a write is in flight.
Description check	✅ Passed	The PR description covers the change and verification, and includes cause/fix details even though it doesn't use the template headings verbatim.
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.}

[github-actions]

This PR may be a duplicate of:

1. fs.Utf8Stream: suppress post-reopen 'drain' while a write is in flight #35824 - Fixes the same post-reopen 'drain' race condition in Utf8Stream.fileOpened by guarding on !this.#writing

🤖 Generated with Claude Code

[robobun]

Duplicate of #35824, which has the same fix (slightly different structuring) and a more thorough in-process test. Closing in favor of that PR.

[claude]

LGTM — targeted one-guard deflake with a deterministic regression test; the inline nit is pre-existing and non-blocking.

What was reviewed:

• Traced fileOpened → sync emit('ready') → listener write() → #actualWrite sets #writing=true → fall-through to the reopening arm; the added !this.#writing guard is the correct minimal gate, and #release still emits 'drain' on completion so no drain is lost.
• Confirmed the no-write-started case (reopening && !#writing) still schedules the nextTick drain as before.
• Test: hermetic (tempDir, subprocess, bunEnv), deterministic via setImmediate fs shim (no sleeps), pipes drained concurrently, asserts stream.writing === false at drain plus exact file contents.

Extended reasoning...

Overview

Single-line change to src/js/internal/streams/fast-utf8-stream.ts: adds && !this.#writing to the else if (reopening) branch in fileOpened, so the post-reopen nextTick 'drain' is not scheduled when a 'ready' listener has already synchronously started an async fs.write. Paired with a new deterministic regression test that forces the race via a setImmediate-delayed fs.write shim.

Security risks

None. Internal event-ordering fix in the Utf8Stream reopen path; no new inputs, no auth/crypto/permissions surface.

Level of scrutiny

Low-to-moderate. It's a one-token guard on a well-understood state machine, motivated by a 30% CI flake with a clear mechanism traced in the PR description. I walked the state transitions: after emit('ready') runs listeners synchronously, either (a) a listener started a write → #writing=true → new guard skips the premature drain and #release emits it on completion, or (b) no write started → #writing still false → drain schedules exactly as before. No behavior is lost. The PR notes upstream Node has the identical logic and fails the same deterministic repro, so this is a genuine fix rather than papering over a Bun-specific timing quirk.

Other factors

The bug hunter flagged one nit: the sibling || this.#flushPending clause on the same conditional has the analogous hazard. It's pre-existing (matches upstream), requires a materially narrower trigger (minLength > 0 + pending flush(cb) across reopen + a 'ready' listener writing ≥ minLength), and doesn't affect the CI flake this PR targets — reasonable to land as-is and hoist the guard in a follow-up. The test follows repo conventions (harness tempDir/bunEnv/bunExe, concurrent pipe drain, stderr/stdout asserted before exitCode, no sleeps or hardcoded ports), and the PR reports all 14 vendored test-fastutf8stream-*.js tests pass.

[claude]

🟡 The sibling || this.#flushPending clause on this same conditional has the identical hazard and remains unguarded — if a flush is pending across a reopen and a 'ready' listener starts an async write, fileOpened calls #actualWrite() a second time on the in-flight #writingBuf, issuing two concurrent fs.write calls (duplicate bytes + double #release). Hoisting !this.#writing && around the whole disjunction fixes both arms. This is pre-existing (matches upstream Node) and needs a narrower trigger (minLength > 0 + flush(cb) pending across a reopen), so it's a nit, not a blocker.

Extended reasoning...

What the bug is

The PR correctly identifies that code in fileOpened running after the synchronous emit('ready') cannot assume #writing is still false, because a 'ready' listener may have called write() and started an async fs.write. It guards the else if (reopening) branch accordingly. However, the immediately preceding branch on the same line has the identical hazard on its || this.#flushPending clause, and that clause is not gated on !this.#writing:

if ((!this.#writing && this.#len > this.#minLength) || this.#flushPending) {
  this.#actualWrite();
} else if (reopening && !this.#writing) {   // <- PR adds guard here

If #flushPending is true when fileOpened runs and a 'ready' listener has just started an async write (#writing = true), the first branch is taken via || this.#flushPending and #actualWrite() is called again. #actualWriteUtf8 does this.#writingBuf ||= this.#bufs.shift() || "" — since #writingBuf is already the non-empty in-flight payload, the ||= keeps it and #fsWrite() issues a second concurrent fs.write(fd, sameBuf). Both completions invoke the shared boundRelease.

Step-by-step proof (async mode, minLength > 0)

1. new Utf8Stream({ dest, sync: false, minLength: 8 }); write("abc") buffers (3 < 8, no write).
2. flush(cb) → #flushUtf8: minLength > 0, so #callFlushCallbackOnDrain(cb) sets #flushPending = true and registers once('drain', onDrain); then #actualWrite() starts an async fs.write (#writing = true).
3. reopen() → #reopening = true; returns early because #writing.
4. The fs.write from step 2 completes → #release sees #reopening, takes the reopen branch without emitting 'drain' (so onDrain never fires and #flushPending stays true), and calls reopen() → #openFile.
5. fs.open completes → fileOpened: clears #reopening/#opening/#writing, then synchronously emit('ready').
6. User's once('ready') listener calls write("after reopen\n") (13 ≥ 8) → #writeUtf8 hits !this.#writing && this.#len >= this.#minLength → #actualWrite() → #writing = true, #writingBuf = "after reopen\n", async fs.write in flight.
7. Control returns to fileOpened. (!this.#writing && …) is false, but || this.#flushPending is true → #actualWrite() runs again → #writingBuf ||= … keeps "after reopen\n" → second #fsWrite() on the same fd with the same payload.

Why existing code doesn't prevent it

• #flushPending is set only in #callFlushCallbackOnDrain and cleared only by its 'drain'/'error' listeners. #release's #reopening branch does not emit 'drain', and #openFile clears #asyncDrainScheduled but not #flushPending, so the flag survives into fileOpened.
• #actualWriteUtf8's ||= is designed for the partial-write retry path; it has no way to know a write is already in flight, so it happily re-issues.

Impact

Two concurrent fs.write calls on the same fd in append mode → the payload is written to disk twice. Both completions call boundRelease, so #release runs twice: the first drains #writingBuf and (with #len now 0) emits 'drain'; the second runs against already-cleared state, likely emitting a spurious extra 'drain' and desynchronizing #len bookkeeping.

Fix

Same fix as the PR's, applied to the sibling clause — hoist the guard around the whole disjunction:

if (!this.#writing && (this.#len > this.#minLength || this.#flushPending)) {
  this.#actualWrite();
} else if (reopening && !this.#writing) {

When #writing is already true, #release on the in-flight write will pick up the pending flush via its normal len > this.#minLength / drain path.

Severity

Nit. This is pre-existing (predates the PR and matches upstream Node's fast-utf8-stream.js identically), and the trigger is materially narrower than the bug the PR fixes: it needs minLength > 0, a flush(cb) pending across a reopen, and a 'ready' listener that writes ≥ minLength bytes. The observed CI flake is fully addressed by the PR as-is. Flagging per REVIEW.md's "fix the whole class in the same PR — parallel switch arms" since it's the same hazard on the same conditional the PR edits, and the one-token hoist covers both — but it shouldn't block merging this deflake.