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

[dylan-conway]

What does this PR do?

Fixes the test/js/node/test/parallel/test-fastutf8stream-reopen.js flake (15 of the last ~80 main builds, every Linux distro + macOS):

AssertionError: Expected values to be strictly equal:
+ ''
- 'after reopen\n'
      at test-fastutf8stream-reopen.js:52:22   (also :94:22)

Root cause is in Utf8Stream (src/js/internal/streams/fast-utf8-stream.ts), not the test. After reopen() the fileOpened callback emits 'ready' and then unconditionally does process.nextTick(() => this.emit('drain')). The test's 'ready' listener calls stream.write('after reopen\n'), which starts an async fs.write and sets #writing = true — so the scheduled 'drain' fires on the next tick while that write is still in flight. The test's once('drain') takes it as "buffer flushed", readFile()s the destination, and races the pending write. (When the write does complete, #release emits a second, genuine 'drain'.)

You can see the double drain directly — this prints writing = true on the first drain in both Bun (before this change) and Node:

stream.once('drain', () => {
  renameSync(dest, dest + '-moved');
  stream.reopen();
  stream.once('ready', () => {
    stream.write('after reopen\n');
    stream.once('drain', () => console.log('drain; writing =', stream.writing));   // true
  });
});

Node carries the same code but its libuv threadpool is a strict FIFO, so the earlier-queued write practically always lands before the later readFile. Bun's pool makes no such promise, and under CI load the read wins occasionally.

Fix: the reopen tick only emits 'drain' if no write is in flight; otherwise the write's completion emits it, as it already does everywhere else in the class. Sync mode is unaffected (a sync write in 'ready' has finished by the time the tick runs).

How did you verify your code works?

• New deterministic test in test/js/node/fs/fs.test.ts that holds the fs.write callback (via the fs override option) across the reopen and asserts no 'drain' is emitted until the write is released. Fails on the previous build (drainedWhileHeld === true), passes now.
• test-fastutf8stream-*.js (all 14) pass with the debug build; -reopen.js 40/40 under 8-way parallel stress.

---

no test proof · iteration 0 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/node/fs/fs.test.ts

[dylan-conway]

@robobun adopt

[robobun]

Adopted. Verifying the fix locally (debug build + the new test and the fastutf8stream suite), will handle CI and review feedback from here.

[coderabbitai]

Walkthrough

Changes

Utf8Stream reopen drain ordering

Layer / File(s)	Summary
Reopen drain guard and regression coverage src/js/internal/streams/fast-utf8-stream.ts, test/js/node/fs/fs.test.ts	The reopen path defers drain until no write has started. The regression test covers delayed writes, event ordering, output, errors, and closure.

Possibly related PRs

• oven-sh/bun#36412: Implements and tests the same Utf8Stream post-reopen drain fix.

Suggested reviewers: cirospaciari, jarred-sumner, robobun

🚥 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 identifies the Utf8Stream reopen issue and the prevention of an incorrect drain event during an in-flight write.
Description check	✅ Passed	The description explains the root cause, fix, verification steps, test coverage, and platform-specific test limitation.

---

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

[claude]

LGTM — minimal, well-reasoned guard on the reopen 'drain' tick; the inline nit is test-hygiene only and doesn't affect the fix or the test's ability to catch the regression.

Extended reasoning...

Overview

The production change is three lines in src/js/internal/streams/fast-utf8-stream.ts: the process.nextTick(() => this.emit('drain')) scheduled in fileOpened after a reopen now checks if (!this.#writing) before emitting. The PR description traces the mechanism precisely — a 'ready' listener that calls write() starts an async fs.write and sets #writing = true, so the previously-unconditional 'drain' on the next tick lied about buffer state and raced the pending write in test-fastutf8stream-reopen.js. With the guard, the write's own completion in #release emits the (only) drain, matching every other drain site in the class.

The accompanying test in test/js/node/fs/fs.test.ts deterministically reproduces the race by injecting an fs.write override that captures the callback, asserts no 'drain' fires while the write is held, then releases it and asserts the genuine drain arrives with correct file contents.

Security risks

None. This is event-emission ordering in a JS-side stream helper; no untrusted input parsing, no auth/crypto/permissions surface.

Level of scrutiny

Low-to-moderate. The source change is a single guarded conditional inside an existing nextTick callback, and the guard mirrors the invariant already used throughout the class (#release is the canonical drain emitter when #writing transitions to false). I traced the surrounding branch: this else if (reopening) arm is only reached when #len <= #minLength and #writing was false immediately after emit('ready') returned — so if a listener started a write, #writing is now true and the tick correctly defers to #release; if no write was started, behavior is unchanged. Sync mode is unaffected as claimed: 'ready' is itself deferred via nextTick in sync mode, and a sync write completes before the guard is evaluated, so #writing is false and the emit proceeds exactly as before.

Other factors

• The one inline finding is a nit: expect(stream.writing).toBe(true) inside the 'ready' callback isn't wrapped in try/catch → reject like the other assertions in the same test. If it ever fails it hangs to timeout instead of showing the diff. It doesn't affect the test's correctness today (with minLength = 0 and the held write stub, writing is deterministically true there) and is trivially fixable.
• This diverges slightly from upstream Node/SonicBoom, which carries the unconditional emit — the PR explains why Node doesn't hit it (libuv threadpool FIFO) and why Bun does. The divergence is a strict tightening (fewer, not more, drain events) so Node-compat risk is minimal.
• Verified in the description: all 14 test-fastutf8stream-*.js pass and the flaky one survives 40× under 8-way stress.

[claude]

🟡 This expect(stream.writing).toBe(true) runs inside the 'ready' event callback (emitted synchronously from the fs.open callback) without a try/catch, so if it ever fails the throw becomes an uncaughtException and the test hangs to timeout instead of rejecting done with the assertion diff. Wrap it like the other in-callback expects in this test: try { expect(stream.writing).toBe(true); } catch (e) { return reject(e); }.

Extended reasoning...

What the bug is

At test/js/node/fs/fs.test.ts:6134, expect(stream.writing).toBe(true) is called directly inside the stream.once('ready', ...) callback with no try/catch → reject wrapper. In async mode (sync: false), 'ready' is emitted synchronously from fileOpened, which is the callback passed to this.#fs.open(file, flags, mode, fileOpened). A thrown assertion therefore propagates through emit('ready') → fileOpened → the fs.open callback context and surfaces as an uncaughtException — it never reaches the awaited done promise.

Code path that triggers it

1. stream.reopen() → #openFile(file) → this.#fs.open(file, flags, mode, fileOpened) (the test does not override fs.open, so this is the real node-style async callback).
2. fileOpened runs and calls this.emit('ready') synchronously.
3. The test's 'ready' listener runs; expect(stream.writing).toBe(true) throws.
4. EventEmitter#emit re-throws synchronously into fileOpened, which re-throws into fs.open's callback dispatch → uncaughtException (exactly the mechanism the describe block immediately above this test demonstrates).
5. done is never settled; the test hangs until the runner's timeout.

Why existing code doesn't prevent it

stream.on('error', reject) only catches events the stream itself emits — it does not intercept a synchronous throw from a user listener. And the test's own pattern already acknowledges this: the expects at lines 6143–6147 and 6150–6154 are wrapped in try { ... } catch (e) { return reject(e); }. Line 6134 is the one place the pattern was missed, so the test is internally inconsistent.

REVIEW.md is explicit on this: "Wire EVERY failure event … to reject the awaited promise — never throw inside event callbacks."

Impact

If a future regression makes stream.writing false at this point (e.g. #actualWriteUtf8 stops setting #writing = true, or write() short-circuits), the test will hang to timeout with a bare uncaughtException in stderr instead of failing fast with the Jest diff Expected: true / Received: false. Correctness of the test as shipped is unaffected — with minLength = 0 and the held fs.write stub, write('after reopen\n') synchronously reaches #actualWriteUtf8 which sets #writing = true, so the assertion currently always passes.

Step-by-step proof

Assume stream.writing were somehow false:

• expect(false).toBe(true) throws a JestAssertionError.
• emit('ready') has no try/catch around listener invocation, so it propagates.
• fileOpened has no try/catch around this.emit('ready'), so it propagates.
• fs.open's callback trampoline re-raises listener throws as uncaughtException (see the preceding describe("a throw from a node-style callback is an uncaughtException") at line ~6070+).
• resolve/reject are never called → await done hangs → test times out.

Fix

Wrap it the same way the other assertions in this test already are:

stream.write("after reopen\n");
try {
  expect(stream.writing).toBe(true);
} catch (e) {
  return reject(e);
}