Repository navigation
Conversation
After reopen(), fileOpened emits 'ready' synchronously. A listener on
'ready' that calls write() starts an async fs.write and typically
registers once('drain', ...) to read the file once that write lands.
fileOpened then unconditionally scheduled process.nextTick(emit('drain'))
whenever reopening was true, even though #writing had just been set by
the ready-handler's write(). That stale drain raced the threadpool
worker running the write() syscall; on a loaded box the nextTick would
win and the drain listener observed an empty file.
This is the test-fastutf8stream-reopen.js flake seen on 32/400 recent CI
builds (mostly linux aarch64). Node's upstream has the same check but
doesn't flake because its threadpool worker almost always finishes the
write() before the nextTick runs.
Only schedule the post-reopen drain when no write is in flight. #release
emits drain once the pending write completes, which is what callers
actually want.
WalkthroughChangesUtf8Stream reopen behavior
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 7:06 PM PT - Jul 25th, 2026
⏳ @robobun, your commit f364382 is still building in |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline nit, I traced the other reopen paths: block 5 of test-fastutf8stream-reopen.js (reopen → once('drain') with no write in 'ready') still fires because #writing stays false; sync: true is unaffected since 'ready' is deferred to nextTick there, so #writing is still false at the guard; and the minLength > 0 buffered-below-threshold case keeps its prior behavior.
Extended reasoning...
Traced the added !this.#writing guard against the other consumers of the post-reopen 'drain': block 5 of the flaking Node parallel test, sync-mode reopen, and the buffered-below-minLength path all reach the guard with #writing === false and behave identically to before. The only behavioral change is suppressing the premature nextTick 'drain' when a write was started synchronously inside the 'ready' handler, which #release then emits once the write lands.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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/fs/fast-utf8-stream.test.ts`:
- Around line 8-126: Mark the independent filesystem-based tests in the
“fs.Utf8Stream reopen” suite as concurrent using the test framework’s concurrent
test API. Update both test declarations while preserving their existing setup,
assertions, and cleanup behavior.
🪄 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: 6893e21f-c72b-4103-9e5e-86601ad537c3
📒 Files selected for processing (2)
src/js/internal/streams/fast-utf8-stream.tstest/js/node/fs/fast-utf8-stream.test.ts
The #flushPending disjunct had the same post-'ready' hazard as the reopening arm: with minLength > 0, a flush(cb) while opening sets #flushPending, and if a write() inside the sync 'ready' emit starts an fs.write, fileOpened would call #actualWrite a second time on the same #writingBuf. Hoist a single !#writing guard above both arms so neither can fire while a write is already in flight; #release drives the state machine once the in-flight write completes. Adds a deterministic test that counts fs.write dispatches across a reopen with a pending flush.
There was a problem hiding this comment.
LGTM — both earlier findings are addressed (the writing assertion is now captured outside the callback, and the !this.#writing guard is hoisted over both arms with a dedicated test).
What was reviewed:
- Traced
fileOpenedafter the hoist: when#writingis set insideemit('ready'),#releaseon that write drives both the'drain'emit and the pending-flush path, so nothing is dropped. - Confirmed the
!#writingno-write-in-ready path still schedules the post-reopen'drain'(test 2 covers this; block 5 of the upstream node test unaffected). - Walked test 3's sequencing (
minLength: 4,flush()while opening,write()in'ready') — on main it double-dispatches, with the guardwritesis["hello\n"]; the append-mode file-content assertion is correct for a same-path reopen.
Extended reasoning...
Overview
Two files: a 7-line control-flow change in src/js/internal/streams/fast-utf8-stream.ts (#openFile's fileOpened callback) hoisting a single !this.#writing guard above the post-'ready' dispatch, and a new test/js/node/fs/fast-utf8-stream.test.ts with three deterministic tests using the constructor's fs override hook to control write timing.
The fix addresses a real CI flake (32/400 builds) in test-fastutf8stream-reopen.js where a process.nextTick(() => emit('drain')) scheduled after reopen raced the threadpool worker executing a write() started synchronously inside the 'ready' handler. The hoisted guard also fixes the sibling || this.#flushPending arm, which could double-dispatch #actualWrite() under minLength > 0 — that was raised in my earlier review and folded into this PR with its own test.
Security risks
None. This is internal event-ordering in a Node-compat stream class; no untrusted input parsing, no auth/crypto/permissions surface.
Level of scrutiny
Moderate. The source change is tiny and mechanical, but it alters the state machine of a shared stream class, so I traced every case where the old code took an action and the new code doesn't:
#writing && #flushPending: old code called#actualWrite()(the double-dispatch bug); new code skips, and#releaseon the in-flight write reaches the drain branch which fires#callFlushCallbackOnDrain'sonDrain— flush is still serviced.#writing && reopening: old code scheduled a premature'drain'; new code skips, and#releaseemits'drain'after the write lands (#reopeningwas already cleared tofalseearlier infileOpened, so#releasereaches the else/drain branch).!#writingcases are byte-identical to before.
Since #writing is set to false immediately before emit('ready'), the only way it's true at the guard is via a 'ready' listener calling write(), which always leaves a #release pending — so the skipped work is never lost.
Other factors
- All prior review threads (my two, comment-cop, coderabbit) are resolved by follow-up commits d92c8b0, 7f7ff6b, a607d2f, 7e523f0.
- Tests follow harness conventions:
tempDirwithusing,describe.concurrent(independent temp dirs), no sleeps — the race is made deterministic via thefs.writeoverride holding the callback until after aprocess.nextTickhas drained. Assertions are outside event callbacks per REVIEW.md. - No pre-existing bun:test file for
Utf8Stream(only ported node parallel tests), so the new file location is correct. - PR description confirms all 14
test-fastutf8stream-*.jsupstream tests still pass and the flaking test passed 30/30.
There was a problem hiding this comment.
LGTM. The hoisted !this.#writing guard is the right fix for both arms, and both prior review threads (assertion-in-callback, #flushPending double-dispatch) are addressed. The remaining inline nit about using stream for fd cleanup on assertion failure is worth applying but not blocking.
Extended reasoning...
Overview
Six-line change in src/js/internal/streams/fast-utf8-stream.ts (fileOpened): the post-emit('ready') dispatch previously read if ((!this.#writing && this.#len > this.#minLength) || this.#flushPending) { #actualWrite() } else if (reopening) { nextTick(emit('drain')) }. Neither the #flushPending disjunct nor the reopening arm accounted for a write() inside the synchronous 'ready' listener having already set #writing = true. The PR hoists a single if (!this.#writing) above both arms. When a write is in flight, #release already drives the rest of the state machine (continues buffered writes, services #flushPending via the 'drain' listener registered in #callFlushCallbackOnDrain, emits 'drain'). When #writing is false the transformed condition is identical to the original, so the no-write-in-'ready' path is unchanged. In sync mode 'ready' is deferred via process.nextTick, so #writing is always false at this point and behavior is unchanged there too.
A new test file test/js/node/fs/fast-utf8-stream.test.ts adds three deterministic tests using the fs override hook: (1) holds the post-reopen fs.write past nextTick and asserts 'drain' only fires after the write callback; (2) asserts reopen with no write in 'ready' still emits 'drain'; (3) counts fs.write dispatches with minLength: 4 and a flush() pending across reopen. The evidence block shows tests 1 and 3 fail on main and all three pass with the fix.
Security risks
None. Internal stream state-machine ordering; no untrusted input, no auth/crypto/permissions.
Level of scrutiny
Moderate — this is a Node-compat internal stream (Utf8Stream, ported from SonicBoom) used for logging-style workloads. The change is small, the root cause is well explained, and I traced each arm: with #writing true after 'ready', skipping both arms is safe because #release (already the sole driver once a write is in flight) handles #flushPending via the registered 'drain' listener and emits 'drain' itself; #reopening was already cleared to false before emit('ready'), so #release correctly takes the normal drain path rather than re-reopening. I also checked end()/destroy()/reopen() called from inside 'ready' — each either sets #destroyed (caught by the earlier guard) or starts a write/open that #release/the next fileOpened will complete.
Other factors
Both of my earlier inline comments were addressed (d92c8b0 moved the assertion out of the event callback; 7e523f0 hoisted the guard over the #flushPending arm and added the third test). The comment-cop and CodeRabbit nits were also addressed. No CODEOWNERS entry for these paths. The one remaining nit this run — stream.end() cleanup placed after the assertions instead of using stream — is a test-hygiene point that only matters on assertion failure; it doesn't affect the fix's correctness and can be a one-token follow-up.
There was a problem hiding this comment.
LGTM — the hoisted !this.#writing guard is the minimal correct fix for both arms of the post-'ready' dispatch, and all prior review feedback has been addressed.
What was reviewed:
- Traced
#releaseto confirm it drives 'drain' emission and#flushPendingservicing when the guard skips both arms — no lost drain, no lost flush callback. - Verified the no-write-in-'ready' path is unchanged (
#writingstays false → nextTick drain still fires; covered by test 2). - Checked the divergence from upstream Node's identical unguarded code — it's a genuine race in the state machine, documented in the PR body and the test's describe block; not a Bun-layer workaround.
Extended reasoning...
Overview
The PR changes 6 lines in src/js/internal/streams/fast-utf8-stream.ts (fileOpened's post-emit('ready') dispatch) and adds a new test file with three deterministic tests. The production change hoists a single !this.#writing guard above both the #actualWrite() arm and the process.nextTick(() => emit('drain')) arm, so neither fires when a write() inside the synchronous 'ready' handler has already started an async fs.write. #release (the write completion callback) already handles both cases correctly: it emits 'drain' when the buffer drains, and the once('drain', onDrain) registered by #callFlushCallbackOnDrain services the pending flush.
Security risks
None. This is internal state-machine sequencing in a Node-compat stream class; no untrusted input parsing, no auth/crypto/permissions surface.
Level of scrutiny
Moderate. It's a Node-compat internal (fs.Utf8Stream, ported from Node/SonicBoom), so behavioral changes matter — but the change strictly narrows when a redundant 'drain'/#actualWrite() fires, and the retained path (#release) already produces the correct events. The PR description documents that upstream Node has the identical unguarded code; this is a real race (not a Bun-specific symptom), so the "suspect a different layer" rule doesn't apply. The file is not in CODEOWNERS.
Other factors
All five prior review threads are resolved: my three nits (assertion inside event callback → captured to local; sibling #flushPending arm → guard hoisted over both arms with a dedicated test; fd cleanup on assertion failure → using stream), comment-cop (explanatory comment dropped; rationale in commit message + test describe block), and coderabbit (describe.concurrent). The PR evidence shows both new failure modes reproduce on main (ASAN + release) and pass with the fix; the existing 14 test-fastutf8stream-*.js upstream tests still pass. Tests use tempDir, using, the fs override hook for determinism (no sleeps), and assert exact event ordering and file contents.
|
CI on f364382 (build 81896): the new The remaining red is unrelated to this diff:
This change is a 6-line hoist in a bundled JS module plus a test file; neither touches worker threads or JSC exception handling. Ready for merge. |
|
Still the top non-threshold flake: |
|
Superseded by #37200, which carries the same guard (drain only when no write is in flight after reopen) with a deterministic regression test. Closing in favor of that PR. |
1 participant
test/js/node/test/parallel/test-fastutf8stream-reopen.jshas been flaking across CI (32 of the last 400 builds, 190 per-lane occurrences, heaviest on linux 25.04 x64/aarch64 and alpine 3.23):Cause
After
reopen(),#openFile'sfileOpenedcallback emits'ready'synchronously. A'ready'listener that callswrite()starts an asyncfs.write(setting#writing = true) and then typically registersonce('drain', ...)to read the file once the write lands.fileOpenedthen continues:Neither arm accounts for
#writinghaving just been set inside the synchronousemit('ready'):else if (reopening)arm schedulesprocess.nextTick(() => emit('drain'))unconditionally. That nextTick races the threadpool worker executing thewrite()syscall; when the worker is delayed (loaded CI box), the nextTick wins, the'drain'listener fires early, andreadFile(dest)observes an empty file. This is the CI flake.|| this.#flushPendingdisjunct calls#actualWrite()even though one is already in flight. WithminLength > 0and aflush(cb)issued while the reopen'sfs.openis pending,fileOpeneddispatches a secondfs.writeof the same#writingBuf, so the data is appended twice and#releasefires twice.Node's upstream
lib/internal/streams/fast-utf8-stream.jshas the identical code; it happens not to flake in Node's CI because libuv's threadpool worker almost always completes the smallwrite()before the main thread drains the nextTick queue.Fix
Hoist a single
!this.#writingguard above both arms. When a write is in flight,#releasedrives the state machine (emitting'drain', continuing buffered writes, and servicing#flushPending) once that write completes. The no-write-in-'ready'path (block 5 of the upstream test) is unchanged:#writingstays false and the post-reopen'drain'still fires.Verification
test/js/node/fs/fast-utf8-stream.test.tsadds three deterministic tests via thefsoverride hook:fs.writeuntil after nextTick and asserts'drain'only fires once the write callback is delivered (fails on main withevents: ["ready", "drain"]before the callback);'drain'when nothing is written in'ready';fs.writedispatches across a reopen withminLength: 4and a pendingflush()(fails on main with["hello\n", "hello\n"]).[review] gate passed · iteration 1 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 1
evidence per changed file