FileSink: settle the pending write()'s promise in every synchronous close()/end() flush arm - #35365
Conversation
…ails FileSink::end() (the js_close path behind sink.close() on the FileSink prototype and the piped-ReadableStream controller's close()) had the same WriteResult::Err arm that #35344 fixed in end_from_js(): it set done=true, tore down the writer, and returned Err without touching the pending slot. A backpressured write()'s promise was left pending forever while close() threw EPIPE at its caller. Mirror end_from_js(): when a backpressured write()'s promise is outstanding, latch the error into it, schedule run_pending, and return Ok so js_close doesn't also throw. When nothing is pending the Err return (and close()'s throw) is unchanged.
|
Warning Review limit reached
Next review available in: 4 minutes 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 |
|
Updated 2:10 AM PT - Jul 24th, 2026
✅ @robobun, your commit ca307169dd3052120b139ceff03ad39d327356e5 passed in 🧪 To try this PR locally: bunx bun-pr 35365That installs a local version of the PR into your bun-35365 --bun |
|
Found 2 issues this PR may fix:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find any bugs. The change mirrors the already-merged end_from_js / on_auto_flush Err arms exactly, and I verified both callers of FileSink::end() (js_close and subprocess::Writable::close) are fine with the Ok(()) return. Deferring only because this is native FileSink teardown code and changes close()'s observable contract (throw → route to pending promise), which is worth a human confirming.
Checked: latch-before-writer.end() ordering matches the sibling paths; self stays live through run_pending_later() (keep-alive ref taken in to_result, wrapper's +1 still held during js_close); the non-pending path is unchanged so close() still throws when no write is outstanding; test fails on unfixed build with the thrown EPIPE and mirrors the sibling end() test.
Extended reasoning...
Overview
Adds a pending-state guard to FileSink::end()'s WriteResult::Err arm (src/runtime/webcore/FileSink.rs:1055): when a backpressured write()'s promise is outstanding, latch the error into self.pending, call writer.end(), schedule run_pending_later(), and return Ok(()) so js_close doesn't also throw. When nothing is pending, behavior is unchanged. Adds a POSIX-only regression test in filesink.test.ts that is a direct sibling of the existing end()-after-backpressure-with-reader-gone test.
Security risks
None. No untrusted input parsing, no new allocation, no new refcount ops. The only new state mutation is writing streams::Writable::Err(e) into an existing JsCell slot.
Level of scrutiny
Medium-high. FileSink teardown is refcount- and re-entrancy-sensitive (the file is dense with SAFETY/provenance commentary), and the change alters the user-visible behavior of sink.close() when a write is pending — it now returns undefined and rejects the outstanding write promise asynchronously instead of throwing EPIPE synchronously. The change itself is small and follows two already-merged sibling fixes (#35344 end_from_js, #35278 on_auto_flush) line-for-line, so the pattern is proven, but a maintainer should confirm the API-contract shift is desired.
Other factors
- Verified callers:
js_close(Sink.rs:655) mapsOk(())→JSValue::UNDEFINED;subprocess::Writable::close(Writable.rs:534) discards the result withlet _ =, soOk(())doesn't change it — and it now gets its pending promise settled where previously it was orphaned. - Lifetime: when
pending.state == Pending,to_resultalready setmust_be_kept_alive_until_eofand took a +1.writer.end()→on_close→clear_keep_alive_refreleases that +1, but the caller (js_closevia the C++ wrapper'sm_sinkPtr, or the subprocessWritable::Pipeslot) still holds its own ref, soselfis live for the subsequentself.run_pending_later(). This is the identical sequence already shipped inend_from_js's Err arm. - Test mirrors the sibling
end()test precisely (socket pair, 4 MB backpressure, close reader, assertclose()returns undefined, await write promise → EPIPE), with the same cleanup pattern. PR description shows it fails on the unfixed build with the thrown EPIPE.
…to a subprocess The Err arm wasn't the whole bug: end()'s Done and Wrote arms (reached when close()'s flush drains the buffer synchronously because the reader pulled between write() and close()) call writer.end() without scheduling run_pending either, so the write()'s promise hangs the same way. writer.end() only re-enters on_close, which never touches the pending slot; flush() doesn't route through on_write. Schedule run_pending_later() in those arms too (pending.result already holds Owned(consumed) from to_result). The test now runs in a subprocess with detect_leaks=0 in its env: sink.close() on a Blob-created FileSink strands the JS wrapper's +1 ref on main (doClose nulls m_sinkPtr so ~JSFileSink skips finalize), which the x64-asan lane flags. That leak is pre-existing and tracked separately.
robobun
changed the title
…rm-fragile drained close() sub-test end_from_js()'s Done/Wrote arms had the identical orphan (#35344 only fixed its Err arm). When a backpressured write()'s promise is outstanding they now hand that promise back (like the Err/Pending arms already do) and schedule run_pending to resolve it, instead of returning a bare byte count while the promise hangs. The close() subprocess test's drained-arm check depended on Linux's ~200KB AF_UNIX send buffer letting close()'s flush drain the remainder in one go; on macOS (~8KB) flush() returns Pending and the test would have timed out. Drop it and cover the Done/Wrote path via an in-process sink.end() test instead, which is Linux-only for the same reason (on macOS the Pending arm was already correct) and doesn't hit the pre-existing close() leak.
robobun
changed the title
There was a problem hiding this comment.
Beyond the inline nit: 159dbd5 addressed both of my earlier comments — end_from_js()'s Done/Wrote arms now hand back the pending write's promise and schedule run_pending_later(), and the platform-fragile drained close() sub-test was replaced with a Linux-gated in-process sink.end() test. I also checked the detect_leaks=0 override in the new subprocess test — it scopes to the pre-existing sink.close() wrapper-detach leak documented in the test comment, not to anything this fix introduces.
Extended reasoning...
Re-reviewed after 159dbd5. The two substantive issues from the prior run are resolved: end_from_js()'s Done/Wrote arms now mirror the Err/Pending arms (return the outstanding promise, schedule run_pending_later()), and the drained-arm coverage moved to an in-process Linux-only sink.end() test where the AF_UNIX buffer size makes the Done/Wrote path deterministic. In end(), has_pending is read before flush() and in end_from_js() after — checked that IOWriter::flush() never re-enters on_write/run_pending, so the ordering difference doesn't matter. Also confirmed writer.end() in the Done/Wrote arms only re-enters on_close (which never touches self.pending), so grabbing the promise after writer.end() there is safe, unlike the Err arm's before-ordering. The remaining finding is a dead-flag nit; leaving the merge decision to a maintainer given this is FileSink lifecycle/refcount code.
There was a problem hiding this comment.
All three prior findings (macOS drained-arm flake, unfixed end_from_js Done/Wrote arms, dead readFdOpen flag) are addressed and the bug hunter found nothing new this round. Deferring to a human because this is native lifecycle code with a user-visible behavior change to sink.close() (no longer throws when a backpressured write is pending — the error routes to the write's promise instead), and the new subprocess test sets detect_leaks=0 to work around a pre-existing doClose leak that a maintainer should be aware of.
What was reviewed:
end()'s new Err/Done/Wrote arms andend_from_js()'s new Done/Wrote arms — ordering ofwriter.end()vsrun_pending_later()matches the merged #35344 Err-arm shape;on_closenever touchesself.pending, so grabbing the promise afterwriter.end()is safe.- Both callers of
FileSink::end()(js_closeat Sink.rs:655,subprocess::Writable::closeat Writable.rs:534) — the Ok(()) return when pending is harmless for both. has_pendinghoisting before/afterflush()—flush()is pure I/O per the SAFETY comment and doesn't touchpending.state.
Extended reasoning...
Overview
Completes the FileSink pending-promise bug class started by #35278 and #35344. Touches src/runtime/webcore/FileSink.rs (two functions: end() and end_from_js()) and adds two tests to test/js/bun/util/filesink.test.ts. In end(), all three synchronous flush arms (Err/Done/Wrote) now schedule run_pending_later() when a backpressured write's promise is outstanding, and the Err arm returns Ok(()) instead of propagating the error (which js_close would have thrown, orphaning the promise). In end_from_js(), the Done/Wrote arms now return the outstanding promise (like the Err/Pending arms already did) and schedule run_pending_later().
Security risks
None. No untrusted input parsing, no auth/crypto, no allocation sizing from external data. The unsafe blocks are the same JsCell get_mut() promise-grab pattern already merged in #35344's Err arm, with matching SAFETY comments.
Level of scrutiny
High — native Rust with unsafe, JsCell interior mutability, intrusive refcounting, and re-entrant writer.end() → on_close → clear_keep_alive_ref teardown. I traced the ordering: writer.end() re-enters on_close, which fires signal.close(None) and releases the keep-alive ref but never touches self.pending, so the post-writer.end() promise grab and run_pending_later() are safe. run_pending_later() is idempotent (gated on has). The wrapper's +1 keeps the FileSink alive across the call (same as pre-existing end() Done arm). Both callers of FileSink::end() were checked: js_close returns undefined on Ok(()); subprocess::Writable::close discards the result.
Other factors
This PR went through three prior review rounds where I flagged (1) a macOS-specific test flake in the drained-arm sub-test, (2) the end_from_js Done/Wrote arms being left unfixed, and (3) a dead readFdOpen flag — all addressed in 159dbd5 and ca30716. Two things a maintainer should sign off on: the sink.close() behavior change (silently returning undefined instead of throwing EPIPE when a write is pending — arguably correct since the error now reaches the promise, but it's observable), and the ASAN_OPTIONS: detect_leaks=0 override in the new subprocess test, which papers over a pre-existing doClose/m_sinkPtr leak the PR description says is tracked separately. The Done/Wrote test is Linux-only, which is justified (macOS AF_UNIX buffers are ~8KB so flush() returns Pending there and the arm is unreachable), but leaves that arm without CI coverage on macOS.
|
Build 79397: 194/196 lanes pass. |
…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 -->
3 participants
Closes out the bug class #35278 and #35344 started:
FileSink::end()(thejs_closepath behindsink.close()) andFileSink::end_from_js()'s remainingDone/Wrotearms both orphan a backpressuredwrite()'s promise.Repro
The same hang happens on the success path: write a backpressuring chunk, drain the reader synchronously with
fs.readSync, thensink.end()(orsink.close()).flush()pushes the remaining buffer through in one shot and returnsDone/Wrote, the arm callswriter.end()and returns, and the write's promise is left pending forever.Cause
All three synchronous arms (
Err/Done/Wrote) ofFileSink::end(), and theDone/Wrotearms ofFileSink::end_from_js(), tear the writer down viawriter.end()and return without touchingself.pendingor schedulingrun_pending.writer.end()re-enterson_closesynchronously, which firessignal.close(None)and releases the keep-alive ref but never touches the pending slot;IOWriter::flush()doesn't route throughparent_on_writefor its drain;on_auto_flushshort-circuits ondone==trueor!has_pending_data(). Nothing ever schedulesrun_pending, so the backpressuredwrite()'s promise stays pending forever. Onend()'s Err armjs_closeadditionally threw the EPIPE at theclose()caller.#35344 fixed
end_from_js()'s Err arm; #35278 fixedon_auto_flush. Both leftend()entirely andend_from_js()'s Done/Wrote arms unchanged.Fix
In both
end()andend_from_js(), when a backpressured write's promise is outstanding:run_pending_later(), and hand the caller that promise (forend_from_js) / returnOk(())sojs_closedoesn't also throw (forend()). FileSink: deliver a failed end() to the pending write's promise instead of double-reporting #35344 already did this forend_from_js();end()now matches.pending.resultalready holdsOwned(consumed)fromto_result; schedulerun_pending_later()to deliver it.end_from_js()additionally returns the promise (like its Err/Pending arms) instead of a bare byte count.on_write, which already settles the slot.end()returnssys::Result<()>so it can't hand the promise back the wayend_from_jsdoes, 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 ofFileSink::end()(subprocess::Writable::close) discards its result, so theOk(())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
spawn.test.ts -t "EPIPE|stdin",spawn-streaming-stdin.test.ts,spawn-stdin-readable-stream.test.ts,shell/epipe.test.ts, andrust:check-allare green.Test notes
sink.close()EPIPE test runs in a subprocess withdetect_leaks=0in its env:sink.close()on a Blob-created FileSink leaks the native FileSink on main (${name}__doClosenullsm_sinkPtrbefore${name}__close, so~JSFileSinkskips${name}__finalizeand the wrapper's +1 ref is never released). That leak is pre-existing and tracked separately; no test on main exercisessink.close()on a Blob writer.Done/Wrotetest is Linux-only: reaching that arm with oneflush()needs the AF_UNIX send buffer to hold the whole remainder after one read cycle (Linux default ~200KB; macOS ~8KB, whereflush()returnsPendingand the promise was already settled viaon_write, so there is nothing to regress).Flagged by a review comment on closed #35351 (duplicate of merged #35344).
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