FileSink: deliver a failed end() to the pending write's promise instead of double-reporting - #35344
Merged
Merged
Conversation
…stead of double-reporting Since #35278, a failed deferred auto-flush rejects the pending write() promise. But when the reader disappears before end() runs — the common Bun.spawn shape where the child exits while a 16MB stdin write is still buffered — end_from_js's own flush() sees the write error first and threw it synchronously, leaving the backpressured write()'s promise outstanding. The auto-flush/error path then rejected that promise as well, and a caller that discarded write()'s return value (as spawn.test.ts's 'stdin.end() rejects with EPIPE' does) got an unhandledRejection for a failure it had already caught from end(). Before #35278 the orphaned promise silently resolved as a full success, which is the lie that fix removed - this completes it by routing the error to exactly one place. end_from_js's Err arm now latches the error into the pending slot and returns the same promise (exactly like its Pending arm), so the failure is reported once, to whichever await is watching. The latch happens before writer.end(), whose teardown can re-enter on_error/run_pending synchronously. Verification: new filesink.test.ts regression (discarded backpressured write + closed reader + same-tick end()) fails on the unfixed build and passes with the fix; spawn.test.ts's EPIPE and FORCE_WAITER_THREAD cases reproduce deterministically with the release binary from main's failing CI build 78927 and pass with the fix; filesink, spawn-stdin, streaming stdin, and shell epipe suites all green.
Collaborator
Contributor
|
Found 2 issues this PR may fix:
🤖 Generated with Claude Code |
Contributor
WalkthroughChangesFileSink pending error handling
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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/bun/util/filesink.test.ts`:
- Around line 282-322: Move the discarded backpressured write sequence from the
current test callback into a spawned Bun subprocess, using bunExe and the
child’s createSocketPair setup. Track unhandledRejection events only inside the
child, report the count/result to the parent, and assert the expected single
EPIPE from sink.end() with zero unhandled rejections. Remove the process-global
listener and related cleanup from the parent test while preserving the existing
POSIX guard and resource cleanup.
🪄 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: 724fd67d-f270-4de6-9c4a-80d4ab8dcb36
📒 Files selected for processing (2)
src/runtime/webcore/FileSink.rstest/js/bun/util/filesink.test.ts
Contributor
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
Member
Author
|
@robobun adopt |
Collaborator
|
✅ Adopted. Approved by dylan-conway. Tightened the regression test in 6cff521 (asserts |
dylan-conway
previously approved these changes
Jul 24, 2026
The previous test's unhandledRejection check never fired in the socketpair scenario (the orphaned promise simply hangs there; the unhandled rejection only happens via on_attached_process_exit in the spawn path), so fail-before was a 5s timeout on the second sink.end() in finally. Assert the actual invariant the fix establishes instead: end() returns the same Promise object write() did, and it rejects with EPIPE. Fails fast on main with a clear EPIPE throw at the sink.end() call site, passes with the fix, and no process-global listener needed.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
LGTM — the test rewrite in 6cff521 addresses my earlier concern: it now asserts the load-bearing invariant directly (end() returns the same Promise object write() did, rejects with EPIPE, follow-up end() returns a number), which fails fast on the unfixed build with a sync throw rather than via a cleanup-block timeout.
What was reviewed:
end_from_jsErr arm mirrors the existing Pending arm and #35278'son_auto_flushErr arm exactly — samepromise()grab, samerun_pending_later()scheduling;WritablePending::promise()returns the existing strong when one is already stored, soendResult === writePromiseholds.- Latch/grab ordered before
writer.end()per the comment;on_closere-entry doesn't touchpending, andrun()is idempotent (early-returns onstate != Pending), so a re-entrant settle plus the scheduled task can't double-reject. - No-pending-write path is byte-identical to before (still throws synchronously).
Extended reasoning...
Overview
Two files: ~25 lines added to src/runtime/webcore/FileSink.rs (end_from_js's WriteResult::Err arm) and a ~45-line POSIX regression test in test/js/bun/util/filesink.test.ts. The Rust change: when end()'s own flush() returns an error while a backpressured write()'s promise is still in the pending slot, latch the error into that slot and return the same promise instead of throwing synchronously — so the failure is delivered exactly once, to whichever await is watching. When no write is pending, behavior is unchanged.
Security risks
None. This is error-delivery routing in the FileSink write path; no new input parsing, no auth/crypto/permission surface.
Level of scrutiny
Medium-high — native Rust in a memory-safety-adjacent area (JsCell borrows, promise lifecycle, re-entrancy through writer.end()). But the change is small, precisely mirrors two existing patterns in the same file (the Pending arm's promise() grab + return, and on_auto_flush's Err arm from #35278 which does latch → writer.end() → run_pending_later()), and is fixing a deterministic CI-red-on-main regression. I verified WritablePending::promise() returns the existing strong promise when one is already stored (streams.rs:436), so the identity assertion in the test holds; and WritablePending::run() early-returns on state != Pending (streams.rs:462-465), so a re-entrant settle via writer.end() plus the scheduled run_pending_later() task is idempotent. on_close (the only synchronous re-entry from writer.end() on POSIX) does not touch pending, and the JS wrapper's +1 keeps the sink alive across clear_keep_alive_ref().
Other factors
My earlier inline comment (against 8212aad) flagged that the original test's headline assertion was vacuous on the unfixed build and the real discriminator was a cleanup-block timeout. The 6cff521 rewrite addressed this fully: the test now holds writePromise, asserts sink.end() === writePromise (fails fast on main with a sync EPIPE throw), awaits it and checks code === 'EPIPE', then asserts a follow-up sink.end() returns a number (pending slot settled). No process-global unhandledRejection listener, no sleep loop, no timeout dependency. CodeRabbit's isolation concern was also mooted by the same rewrite. dylan-conway approved, cirospaciari adopted, and build 79099 shows spawn.test.ts green on all x64 release lanes with only an unrelated aarch64 flake.
robobun
added a commit
that referenced
this pull request
Jul 24, 2026
…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.
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 -->
9 tasks
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
3 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.
What broke
Since #35278 landed,
test/js/bun/spawn/spawn.test.tsfails deterministically on the x64 Linux lanes (ubuntu 25.04 / debian 13 / alpine 3.23) — including on main's own build 78927:gcTick > spawn > stdin.end() rejects with EPIPE when the child exits before consuming the writefails with an unhandledEPIPE: 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_THREADfails 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 thewrite()promise the test deliberately discarded.Root cause
src/runtime/webcore/FileSink.rs,end_from_js(WriteResult::Errarm, previously line ~1136). On release-build timing the child exits beforeend()runs, soend_from_js's ownflush()sees the EPIPE first and threw it synchronously — while the backpressuredwrite()'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 becauseend()runs before the child exits, takes thePendingarm, and shares the write promise — one promise, one rejection, handled by theawait.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'sErrarm, when a backpressured write's promise is outstanding: latch the error into the pending slot and return that same promise (exactly like thePendingarm already does), instead of throwing synchronously. The latch and promise grab happen beforewriter.end(), whose teardown can re-enteron_error/run_pendingsynchronously. When no write is pending, the synchronous throw is unchanged.Verification
filesink.test.ts(discarded backpressuredwrite()+ reader closed + same-tickend()): fails on the unfixed build (orphaned pending promise), passes with the fix — deterministic on any build type, no release timing needed.end()return value are now the same object on this path.a backpressured write() rejects with EPIPE when the reader closes before the deferred flush) still passes — its fix is preserved, not reverted.filesink.test.ts48/0 (includes both regression tests),spawn.test.tsEPIPE case 5/5,spawn-stdin-readable-stream28/0,spawn-stdin-pipe-fd-leak2/0,spawn-streaming-stdin1/0,shell/epipe2/0.spawn.test.tsis 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.
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