FileSink: reject the pending write() when the deferred auto-flush hits EPIPE - #35278
Merged
Merged
Conversation
When a backpressured write() registers the deferred auto-flush microtask and that flush's drain hits a write error (typically EPIPE because the reader closed while the chunk was still buffered), on_auto_flush matched WriteResult::Err(_) and discarded the error before calling run_pending_later(). The pending promise still held the Owned(consumed) result to_result seeded it with, so the awaited write() resolved as if the full chunk had been delivered, and a follow-up end() returned 0. Latch the error into pending.result, move the sink to its terminal state (done + writer.end()), and unregister the auto-flusher. Deterministic repro uses a socketpair: write 4MB (backpressures), close the read fd before the first await so the auto-flush drain sees EPIPE.
Contributor
|
Warning Review limit reached
Next review available in: 4 seconds Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the 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 configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
Contributor
|
Found 3 issues this PR may fix:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
- assert sink.write("x") === true so the done/writer.end() half of the
Err arm is observable (write() returns 1 if the sink stays writable)
- close readFd in the finally when the pre-close assertion throws
- use try/await for end() cleanup so a synchronous throw is caught
Contributor
There was a problem hiding this comment.
LGTM — the earlier readFd cleanup nit is addressed in ce16462, and no new issues found.
What was reviewed:
- Traced
PosixStreamingWriter::flush()'sErrpath (PipeWriter.rs:982-986): confirmed it resetsoutgoingand returns without calling_on_error, so nothing else latches the error intopending.result. - Verified
writer.end()after a flush error only reacheson_close(noton_write), so the newly-latchedWritable::Errcannot be overwritten beforerun_pending_later()runs; the pre-existing_guardkeepsthisalive acrosson_close→clear_keep_alive_ref. - New
Errarm mirrorsend_from_js/end'sErrarms (done=true,writer.end()) and the existingon_errorhandler (gate onPendingState::Pending,run_pending_later()). - Test: hermetic fd cleanup via
readFdOpenflag; asserts specificEPIPEcode and terminal-state behavior; POSIX-gated.
Extended reasoning...
Overview
Splits the WriteResult::Err(_) | WriteResult::Done(_) arm in FileSink::on_auto_flush so that a write error from the deferred auto-flush is latched into pending.result (rejecting the awaited write() promise) and the sink is moved to its terminal state, instead of being silently discarded and letting the pending promise resolve with the stale Owned(consumed) seeded by to_result. Adds a POSIX-gated regression test in filesink.test.ts using createSocketPair() to force backpressure then close the reader before the first await.
Security risks
None. This is error-path plumbing in a streaming file writer; no auth, crypto, parsing of untrusted input, or permission surface.
Level of scrutiny
Elevated — this is unsafe native code with intrusive refcounting, JsCell interior mutability, and re-entrant writer callbacks (on_close/on_write via the parent backref). I traced the new arm's control flow through PipeWriter.rs:
flush()onErrresetsoutgoingand returns without touchingis_doneor calling_on_error(lines 982-986), confirming the PR's root-cause claim.writer.end()setsis_done = trueand callsclose(), which dispatchesParent::on_close— noton_write— so the freshly-latchedWritable::Err(err)cannot be clobbered byon_write'sOwnedAndDone(consumed)overwrite.on_close→clear_keep_alive_refmay deref, but the_guard = FileSinkRef::new_ref(this)taken before the match holds an extra ref, sothissurvives to the trailingrun_pending_later()/auto_flusher.registered.set(false).- The
writer.with_mut(|w| w.end())re-entry shape is identical to the existingend/end_from_js/on_attached_process_exitcall sites;on_closedoes not borrowwriter.
Other factors
- The change is ~18 native lines and is a strict superset of the old
Errbehavior (still doesupdate_ref(false)+run_pending_later()), plus the terminal-state transition copied fromend_from_js'sErrarm. - PR description shows the test failing on the unfixed build (
Received: undefinedforcaught?.code) and passing after;spawn.test.ts -t stdin,spawn-streaming-stdin,shell/epipe, andrust:check-allreported green. - My prior review nit (potential
readFdleak on early assertion failure) was addressed in ce16462 with areadFdOpenflag guarding thefinallyclose — avoids double-closing a recycled fd number, which is better than the blindtry { closeSync } catch {}I originally suggested. - The existing "does not leak native FileSink when a pending write fails (EPIPE)" test in the same file exercises the same scenario for native-leak coverage, so the keep-alive ref release path through
writer.end()→on_close→clear_keep_alive_refis already leak-tested.
Contributor
There was a problem hiding this comment.
No issues found, but this touches the FileSink refcount/re-entrancy state machine (unsafe native code), so worth a human look at the new writer.end() call inside the deferred-microtask context.
What was reviewed:
- The new
Errarm mirrorsend_from_js's Err handling (done = true, latch error intopending.result,writer.end()) and is guarded by the existingFileSinkRef::new_refsoon_closere-entry can't freethismid-body. - Confirmed
run_pending_later()(notrun_pending()) is used, consistent withon_error's deferred-queue path. - The earlier
readFdcleanup nit was addressed in ce16462 via thereadFdOpenflag.
Extended reasoning...
Overview
Splits the WriteResult::Err(_) | WriteResult::Done(_) match arm in FileSink::on_auto_flush so that a flush error (EPIPE, ENOSPC, etc.) is latched into pending.result before run_pending_later() fires — previously the error was discarded and the pending write() promise resolved with the seeded Owned(consumed) value as if the full chunk had been delivered. Adds a POSIX-only regression test using createSocketPair that backpressures a 4 MB write, closes the reader before the first await, and asserts the write rejects with EPIPE and the sink reaches its terminal state.
Security risks
None. This is error-propagation plumbing on an already-owned fd; no new input parsing, no auth/crypto surface.
Level of scrutiny
High. FileSink.rs is ~1300 lines of unsafe Rust with intrusive refcounting, *mut Self backrefs, and extensive re-entrancy commentary. The new arm adds a writer.end() call inside the deferred-microtask context (is_inside_deferred_task_queue == true), which is a slightly different call site than the existing end/end_from_js Err arms it mirrors (those run from JS host-fn context). The _guard = FileSinkRef::new_ref(this) taken before flush() should keep this alive across any on_close re-entry that writer.end() triggers, and run_pending_later() correctly defers promise resolution rather than draining synchronously — but a maintainer familiar with the PipeWriter close/end sequencing should confirm nothing about calling writer.end() from inside the AutoFlusher tick violates an invariant.
Other factors
- The change is small (~18 lines Rust) and follows the established pattern from
end_from_js'sWriteResult::Errarm exactly. - The regression test is well-constructed: it pins the terminal-state contract (
sink.write("x") === trueafter the error,end()returns < size), and the PR description shows it fails on the unfixed build. - My earlier nit about
readFdcleanup on assertion failure was addressed in ce16462 with areadFdOpenflag (avoids double-closing a recycled fd number). - The bug-hunting system found no issues this round.
Given the file's complexity and the memory-safety weight REVIEW.md places on this category, I'm deferring rather than approving.
Collaborator
Author
|
Updated 10:40 AM PT - Jul 23rd, 2026
✅ @robobun, your commit d55e75c161c91650aca683f028353d1addee85a2 passed in 🧪 To try this PR locally: bunx bun-pr 35278That installs a local version of the PR into your bun-35278 --bun |
Collaborator
Author
|
CI status: the diff itself is green. Remaining red is unrelated to this change:
Ready for review/merge. |
This was referenced Jul 23, 2026
cirospaciari
added a commit
that referenced
this pull request
Jul 24, 2026
…ad of double-reporting (#35344) ## What broke Since #35278 landed, `test/js/bun/spawn/spawn.test.ts` fails deterministically on the x64 Linux lanes (ubuntu 25.04 / debian 13 / alpine 3.23) — including on main's own build [78927](https://buildkite.com/bun/bun/builds/78927): - `gcTick > spawn > stdin.end() rejects with EPIPE when the child exits before consuming the write` fails with an **unhandled** `EPIPE: broken pipe, write` (~13ms, 100% reproducible with the build-78927 release binary; repro: `bun test test/js/bun/spawn/spawn.test.ts -t 'rejects with EPIPE when the child exits'`) - `with BUN_FEATURE_FLAG_FORCE_WAITER_THREAD` fails because its inner full-file re-run exits 1 on the same unhandled rejection (`expect(result.exitCode).toBe(0)` at spawn.test.ts:625) The test's own assertion actually passes — `await proc.stdin.end()` does reject with EPIPE and the test catches it. What fails the test is a **second** delivery of the same error, as an unhandledRejection on the `write()` promise the test deliberately discarded. ## Root cause `src/runtime/webcore/FileSink.rs`, `end_from_js` (`WriteResult::Err` arm, previously line ~1136). On release-build timing the child exits **before** `end()` runs, so `end_from_js`'s own `flush()` sees the EPIPE first and threw it synchronously — while the backpressured `write()`'s promise was still sitting in the pending slot. #35278's auto-flush Err arm (correctly) no longer swallows that error, so it then rejected the orphaned promise with nobody holding it. Debug/slower builds don't hit this because `end()` runs before the child exits, takes the `Pending` arm, and shares the write promise — one promise, one rejection, handled by the `await`. Before #35278 the orphaned promise silently resolved as a full success — the lie that PR removed. This completes it: the error goes to exactly one place. ## Fix In `end_from_js`'s `Err` arm, when a backpressured write's promise is outstanding: latch the error into the pending slot and return **that same promise** (exactly like the `Pending` arm already does), instead of throwing synchronously. The latch and promise grab happen before `writer.end()`, whose teardown can re-enter `on_error`/`run_pending` synchronously. When no write is pending, the synchronous throw is unchanged. ## Verification - New regression test in `filesink.test.ts` (discarded backpressured `write()` + reader closed + same-tick `end()`): fails on the unfixed build (orphaned pending promise), passes with the fix — deterministic on any build type, no release timing needed. - The exact CI failure reproduces locally with the release binary downloaded from main build 78927 (both cases, 100%), and mechanically cannot recur: the write promise and the `end()` return value are now the same object on this path. - #35278's own regression test (`a backpressured write() rejects with EPIPE when the reader closes before the deferred flush`) still passes — its fix is preserved, not reverted. - Green on the debug build: `filesink.test.ts` 48/0 (includes both regression tests), `spawn.test.ts` EPIPE case 5/5, `spawn-stdin-readable-stream` 28/0, `spawn-stdin-pipe-fd-leak` 2/0, `spawn-streaming-stdin` 1/0, `shell/epipe` 2/0. - A local release-build run of the full `spawn.test.ts` is in flight; CI covers the release lanes either way. Found while triaging CI on #34598, which inherited the failure through a main merge — this class currently reds every branch that merges main. <!-- robobun:evidence:begin --> --- **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/bun/util/filesink.test.ts <!-- robobun:evidence:end --> --------- Co-authored-by: robobun <117481402+robobun@users.noreply.github.com>
Jarred-Sumner
pushed a commit
that referenced
this pull request
Jul 24, 2026
…lose()/end() flush arm (#35365) Closes out the bug class #35278 and #35344 started: `FileSink::end()` (the `js_close` path behind `sink.close()`) and `FileSink::end_from_js()`'s remaining `Done`/`Wrote` arms both orphan a backpressured `write()`'s promise. ## Repro ```js import { createSocketPair } from "bun:internal-for-testing"; import fs from "node:fs"; const [readFd, writeFd] = createSocketPair(); const sink = Bun.file(writeFd).writer(); const writePromise = sink.write(Buffer.alloc(4 * 1024 * 1024, 0x61)); // backpressures fs.closeSync(readFd); // reader gone before the first await try { sink.close(); } catch {} // throws EPIPE synchronously on main await writePromise; // never settles on main ``` The same hang happens on the success path: write a backpressuring chunk, drain the reader synchronously with `fs.readSync`, then `sink.end()` (or `sink.close()`). `flush()` pushes the remaining buffer through in one shot and returns `Done`/`Wrote`, the arm calls `writer.end()` and returns, and the write's promise is left pending forever. ## Cause All three synchronous arms (`Err`/`Done`/`Wrote`) of `FileSink::end()`, and the `Done`/`Wrote` arms of `FileSink::end_from_js()`, tear the writer down via `writer.end()` and return without touching `self.pending` or scheduling `run_pending`. `writer.end()` re-enters `on_close` synchronously, which fires `signal.close(None)` and releases the keep-alive ref but never touches the pending slot; `IOWriter::flush()` doesn't route through `parent_on_write` for its drain; `on_auto_flush` short-circuits on `done==true` or `!has_pending_data()`. Nothing ever schedules `run_pending`, so the backpressured `write()`'s promise stays pending forever. On `end()`'s Err arm `js_close` additionally threw the EPIPE at the `close()` caller. #35344 fixed `end_from_js()`'s Err arm; #35278 fixed `on_auto_flush`. Both left `end()` entirely and `end_from_js()`'s Done/Wrote arms unchanged. ## Fix In both `end()` and `end_from_js()`, when a backpressured write's promise is outstanding: - **Err arm** (both): latch the error into the pending slot, schedule `run_pending_later()`, and hand the caller that promise (for `end_from_js`) / return `Ok(())` so `js_close` doesn't also throw (for `end()`). #35344 already did this for `end_from_js()`; `end()` now matches. - **Done/Wrote arms** (both): `pending.result` already holds `Owned(consumed)` from `to_result`; schedule `run_pending_later()` to deliver it. `end_from_js()` additionally returns the promise (like its Err/Pending arms) instead of a bare byte count. - **Pending arm** (both): unchanged; the async drain fires `on_write`, which already settles the slot. `end()` returns `sys::Result<()>` so it can't hand the promise back the way `end_from_js` does, but routing the outcome to the promise the caller is already meant to be awaiting keeps the one-delivery invariant #35344 established. The other caller of `FileSink::end()` (`subprocess::Writable::close`) discards its result, so the `Ok(())` doesn't change it, and its pending stdin write now settles where it previously hung. When nothing is pending, `end()`'s Err-arm throw is unchanged. ## Verification ``` $ git checkout main -- src/ && bun bd test test/js/bun/util/filesink.test.ts \ -t 'close.. after a backpressured|reader drained returns' (fail) close() after a backpressured write() with the reader gone ... Expected: "EPIPE" Received: "close-threw" (fail) end() after a backpressured write() with the reader drained ... Expected: Promise { <pending> } Received: 87936 $ git checkout HEAD -- src/ && bun bd test test/js/bun/util/filesink.test.ts 50 pass 0 fail ``` `spawn.test.ts -t "EPIPE|stdin"`, `spawn-streaming-stdin.test.ts`, `spawn-stdin-readable-stream.test.ts`, `shell/epipe.test.ts`, and `rust:check-all` are green. ## Test notes - The `sink.close()` EPIPE test runs in a subprocess with `detect_leaks=0` in its env: `sink.close()` on a Blob-created FileSink leaks the native FileSink on main (`${name}__doClose` nulls `m_sinkPtr` before `${name}__close`, so `~JSFileSink` skips `${name}__finalize` and the wrapper's +1 ref is never released). That leak is pre-existing and tracked separately; no test on main exercises `sink.close()` on a Blob writer. - The drained-`Done`/`Wrote` test is Linux-only: reaching that arm with one `flush()` needs the AF_UNIX send buffer to hold the whole remainder after one read cycle (Linux default ~200KB; macOS ~8KB, where `flush()` returns `Pending` and the promise was already settled via `on_write`, so there is nothing to regress). Flagged by a review comment on closed #35351 (duplicate of merged #35344). <!-- robobun:evidence:begin --> --- **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/bun/util/filesink.test.ts <!-- robobun:evidence:end -->
Jarred-Sumner
pushed a commit
that referenced
this pull request
Jul 27, 2026
…lose()/end() flush arm (#35365) Closes out the bug class #35278 and #35344 started: `FileSink::end()` (the `js_close` path behind `sink.close()`) and `FileSink::end_from_js()`'s remaining `Done`/`Wrote` arms both orphan a backpressured `write()`'s promise. ## Repro ```js import { createSocketPair } from "bun:internal-for-testing"; import fs from "node:fs"; const [readFd, writeFd] = createSocketPair(); const sink = Bun.file(writeFd).writer(); const writePromise = sink.write(Buffer.alloc(4 * 1024 * 1024, 0x61)); // backpressures fs.closeSync(readFd); // reader gone before the first await try { sink.close(); } catch {} // throws EPIPE synchronously on main await writePromise; // never settles on main ``` The same hang happens on the success path: write a backpressuring chunk, drain the reader synchronously with `fs.readSync`, then `sink.end()` (or `sink.close()`). `flush()` pushes the remaining buffer through in one shot and returns `Done`/`Wrote`, the arm calls `writer.end()` and returns, and the write's promise is left pending forever. ## Cause All three synchronous arms (`Err`/`Done`/`Wrote`) of `FileSink::end()`, and the `Done`/`Wrote` arms of `FileSink::end_from_js()`, tear the writer down via `writer.end()` and return without touching `self.pending` or scheduling `run_pending`. `writer.end()` re-enters `on_close` synchronously, which fires `signal.close(None)` and releases the keep-alive ref but never touches the pending slot; `IOWriter::flush()` doesn't route through `parent_on_write` for its drain; `on_auto_flush` short-circuits on `done==true` or `!has_pending_data()`. Nothing ever schedules `run_pending`, so the backpressured `write()`'s promise stays pending forever. On `end()`'s Err arm `js_close` additionally threw the EPIPE at the `close()` caller. #35344 fixed `end_from_js()`'s Err arm; #35278 fixed `on_auto_flush`. Both left `end()` entirely and `end_from_js()`'s Done/Wrote arms unchanged. ## Fix In both `end()` and `end_from_js()`, when a backpressured write's promise is outstanding: - **Err arm** (both): latch the error into the pending slot, schedule `run_pending_later()`, and hand the caller that promise (for `end_from_js`) / return `Ok(())` so `js_close` doesn't also throw (for `end()`). #35344 already did this for `end_from_js()`; `end()` now matches. - **Done/Wrote arms** (both): `pending.result` already holds `Owned(consumed)` from `to_result`; schedule `run_pending_later()` to deliver it. `end_from_js()` additionally returns the promise (like its Err/Pending arms) instead of a bare byte count. - **Pending arm** (both): unchanged; the async drain fires `on_write`, which already settles the slot. `end()` returns `sys::Result<()>` so it can't hand the promise back the way `end_from_js` does, but routing the outcome to the promise the caller is already meant to be awaiting keeps the one-delivery invariant #35344 established. The other caller of `FileSink::end()` (`subprocess::Writable::close`) discards its result, so the `Ok(())` doesn't change it, and its pending stdin write now settles where it previously hung. When nothing is pending, `end()`'s Err-arm throw is unchanged. ## Verification ``` $ git checkout main -- src/ && bun bd test test/js/bun/util/filesink.test.ts \ -t 'close.. after a backpressured|reader drained returns' (fail) close() after a backpressured write() with the reader gone ... Expected: "EPIPE" Received: "close-threw" (fail) end() after a backpressured write() with the reader drained ... Expected: Promise { <pending> } Received: 87936 $ git checkout HEAD -- src/ && bun bd test test/js/bun/util/filesink.test.ts 50 pass 0 fail ``` `spawn.test.ts -t "EPIPE|stdin"`, `spawn-streaming-stdin.test.ts`, `spawn-stdin-readable-stream.test.ts`, `shell/epipe.test.ts`, and `rust:check-all` are green. ## Test notes - The `sink.close()` EPIPE test runs in a subprocess with `detect_leaks=0` in its env: `sink.close()` on a Blob-created FileSink leaks the native FileSink on main (`${name}__doClose` nulls `m_sinkPtr` before `${name}__close`, so `~JSFileSink` skips `${name}__finalize` and the wrapper's +1 ref is never released). That leak is pre-existing and tracked separately; no test on main exercises `sink.close()` on a Blob writer. - The drained-`Done`/`Wrote` test is Linux-only: reaching that arm with one `flush()` needs the AF_UNIX send buffer to hold the whole remainder after one read cycle (Linux default ~200KB; macOS ~8KB, where `flush()` returns `Pending` and the promise was already settled via `on_write`, so there is nothing to regress). Flagged by a review comment on closed #35351 (duplicate of merged #35344). <!-- robobun:evidence:begin --> --- **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/bun/util/filesink.test.ts <!-- robobun:evidence:end -->
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When a
FileSink.write()backpressures it registers a deferred auto-flush microtask. If that microtask'sflush()drains to a write error (typicallyEPIPEbecause the reader closed while most of the chunk was still sitting in the sink's buffer),on_auto_flushmatchedWriteResult::Err(_), discarded the error, and calledrun_pending_later(). The pending promise still held theOwned(consumed)resultto_resulthad seeded, so the awaitedwrite()resolved as if every buffered byte had reached the reader, and a follow-upawait end()returned0.Repro
The user-facing shape (found while hardening the existing
stdin.end() rejects with EPIPEtest inspawn.test.ts) is aBun.spawnchild that reads a little and exits while the parent is doingawait proc.stdin.write(16MB)thenawait proc.stdin.end(): on the unfixed build roughly half of the runs resolve both promises with no error even though the child only ever consumed a few KB.Cause
PosixStreamingWriter::flush()returnsWriteResult::Errwithout routing through_on_error(it resetsoutgoingand hands the error back), so nothing stores the error inpending.result. Every otherFileSinkcaller offlush()(end,end_from_js,flush_from_js) propagates theErritself;on_auto_flushwas the one place that threw it away.Fix
Split
ErrfromDoneinon_auto_flush: onErr, setpending.result = Writable::Err(err)(if the slot is still pending), setdone = trueandwriter.end()so the sink reaches its terminal state (mirrorsend_from_js'sErrarm), thenrun_pending_later()and unregister the auto-flusher.Verification
spawn.test.ts -t stdin,spawn-streaming-stdin.test.ts,shell/epipe.test.ts, andrust:check-allare green.no test proof · iteration 1 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/bun/util/filesink.test.ts