FileSink: deliver deferred-flush EPIPE to the pending write() promise - #35351
FileSink: deliver deferred-flush EPIPE to the pending write() promise#35351cirospaciari wants to merge 2 commits into
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.
WalkthroughChangesFileSink now routes end-time errors through an existing backpressured write promise, finalizes the writer, and schedules pending resolution. A POSIX test covers discarded writes, reader closure, EPIPE propagation, and unhandled rejection behavior. FileSink pending write error flow
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![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/bun/util/filesink.test.ts`:
- Around line 295-321: Strengthen the regression test around the original
sink.write call by capturing its returned promise and explicitly awaiting it,
asserting that it rejects with EPIPE. Keep the sink.end assertion for its
intended behavior, but remove reliance on the cleanup await hanging or on
unhandledRejection to distinguish the regression; ensure the captured write
promise is settled before teardown.
🪄 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: e023e61a-0a93-4039-ae6d-db9467e96e77
📒 Files selected for processing (2)
src/runtime/webcore/FileSink.rstest/js/bun/util/filesink.test.ts
| sink.write(Buffer.alloc(4 * 1024 * 1024, 0x61)); | ||
| fs.closeSync(readFd); | ||
| readFdOpen = false; | ||
|
|
||
| let caught: any; | ||
| try { | ||
| await sink.end(); | ||
| } catch (e) { | ||
| caught = e; | ||
| } | ||
| expect(caught?.code).toBe("EPIPE"); | ||
|
|
||
| // Bounded window for a stray second rejection to surface. | ||
| for (let i = 0; i < 10; i++) await Bun.sleep(1); | ||
| expect(unhandled).toBeNull(); | ||
| } finally { | ||
| process.off("unhandledRejection", onUnhandled); | ||
| try { | ||
| await sink.end(); | ||
| } catch {} | ||
| try { | ||
| fs.closeSync(writeFd); | ||
| } catch {} | ||
| if (readFdOpen) fs.closeSync(readFd); | ||
| } | ||
| }, | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Test's real regression signal is a hang, not the stated assertions.
caught?.code === "EPIPE" also passes on the pre-fix build (old end_from_js threw EPIPE synchronously too), and unhandled stays null either way (pre-fix, the orphaned write() promise is left merely unsettled, not rejected — so no unhandledRejection fires in either case). The only thing that actually distinguishes fixed vs. unfixed here is that the redundant await sink.end() in finally hangs forever pre-fix, because end_from_js's done-early-return path returns that same still-Pending promise. Relying on an incidental timeout in cleanup code to catch a regression isn't a falsifiable, intention-revealing assertion.
Capture the original write() promise and assert directly that it settles with EPIPE:
🧪 Proposed fix
- sink.write(Buffer.alloc(4 * 1024 * 1024, 0x61));
+ const writeP = sink.write(Buffer.alloc(4 * 1024 * 1024, 0x61));
fs.closeSync(readFd);
readFdOpen = false;
let caught: any;
try {
await sink.end();
} catch (e) {
caught = e;
}
expect(caught?.code).toBe("EPIPE");
- // Bounded window for a stray second rejection to surface.
- for (let i = 0; i < 10; i++) await Bun.sleep(1);
- expect(unhandled).toBeNull();
+ // The discarded write() promise must settle with EPIPE, not hang forever.
+ const writeOutcome = await Promise.race([
+ writeP.then(() => "resolved", (e: any) => e?.code ?? e),
+ Bun.sleep(2000).then(() => "timeout"),
+ ]);
+ expect(writeOutcome).toBe("EPIPE");
+ expect(unhandled).toBeNull();Based on learnings, Tests must prove they fail for the intended reason, including tracing fixtures through guards and fast paths, verifying environment knobs are read, asserting setup preconditions, and ensuring removing each fix clause breaks a test. and Every test assertion must be able to fail and assert the strongest invariant; avoid un-awaited expectations, unreachable assertions, conditional assertions, bare toThrow(), weak containment checks, and stale snapshots.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| sink.write(Buffer.alloc(4 * 1024 * 1024, 0x61)); | |
| fs.closeSync(readFd); | |
| readFdOpen = false; | |
| let caught: any; | |
| try { | |
| await sink.end(); | |
| } catch (e) { | |
| caught = e; | |
| } | |
| expect(caught?.code).toBe("EPIPE"); | |
| // Bounded window for a stray second rejection to surface. | |
| for (let i = 0; i < 10; i++) await Bun.sleep(1); | |
| expect(unhandled).toBeNull(); | |
| } finally { | |
| process.off("unhandledRejection", onUnhandled); | |
| try { | |
| await sink.end(); | |
| } catch {} | |
| try { | |
| fs.closeSync(writeFd); | |
| } catch {} | |
| if (readFdOpen) fs.closeSync(readFd); | |
| } | |
| }, | |
| ); | |
| const writeP = sink.write(Buffer.alloc(4 * 1024 * 1024, 0x61)); | |
| fs.closeSync(readFd); | |
| readFdOpen = false; | |
| let caught: any; | |
| try { | |
| await sink.end(); | |
| } catch (e) { | |
| caught = e; | |
| } | |
| expect(caught?.code).toBe("EPIPE"); | |
| // The discarded write() promise must settle with EPIPE, not hang forever. | |
| const writeOutcome = await Promise.race([ | |
| writeP.then(() => "resolved", (e: any) => e?.code ?? e), | |
| Bun.sleep(2000).then(() => "timeout"), | |
| ]); | |
| expect(writeOutcome).toBe("EPIPE"); | |
| expect(unhandled).toBeNull(); | |
| } finally { | |
| process.off("unhandledRejection", onUnhandled); | |
| try { | |
| await sink.end(); | |
| } catch {} | |
| try { | |
| fs.closeSync(writeFd); | |
| } catch {} | |
| if (readFdOpen) fs.closeSync(readFd); | |
| } | |
| }, | |
| ); |
🧰 Tools
🪛 GitHub Check: Claude Code Review
[failure] 295-314: Regression test's unhandledRejection assertion is vacuous; only fails via cleanup timeout
The new regression test's stated assertion (expect(unhandled).toBeNull()) passes on the unfixed build too — the socketpair scenario never produces a second rejection, so the test only distinguishes fixed/unfixed via a timeout on the redundant-looking await sink.end() in the finally block. Hold the write's promise and explicitly assert it settles (e.g. await Promise.race([writeP.catch(e => e), Bun.sleep(100).then(() => 'hung')]) with a code check), so the load-bearing check isn't a cleanu
🤖 Prompt for 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.
In `@test/js/bun/util/filesink.test.ts` around lines 295 - 321, Strengthen the
regression test around the original sink.write call by capturing its returned
promise and explicitly awaiting it, asserting that it rejects with EPIPE. Keep
the sink.end assertion for its intended behavior, but remove reliance on the
cleanup await hanging or on unhandledRejection to distinguish the regression;
ensure the captured write promise is settled before teardown.
Sources: Coding guidelines, Linters/SAST tools
|
Updated 7:43 PM PT - Jul 23rd, 2026
@cirospaciari, your commit 600381d is building: |
|
Yes, this is the same fix as #35344. Both PRs were opened from the same branch ( The conflict is the squash merge colliding with the branch's own original commits. After rebasing, the only surviving delta is in There's no remaining |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| if self.pending.get().state == streams::PendingState::Pending { | ||
| // A backpressured write() left its promise outstanding. | ||
| // Throwing here would report the failure to the caller and | ||
| // then let the auto-flush/error path reject that promise a | ||
| // second time — with nobody holding it when the caller | ||
| // discarded write()'s return value, that second delivery | ||
| // surfaces as an unhandledRejection. Deliver the error to | ||
| // the pending promise instead and hand the caller the same | ||
| // promise (exactly like the Pending arm), so the failure is | ||
| // reported once, to whichever await is watching. The latch | ||
| // and promise grab happen before `writer.end()`: its | ||
| // teardown can re-enter `on_error`/`run_pending` | ||
| // synchronously, and the slot must already hold the error | ||
| // and this caller's promise when that runs. | ||
| self.pending | ||
| .with_mut(|p| p.result = streams::Writable::Err(err)); | ||
| // SAFETY: JsCell — `WritablePending::promise` allocates a | ||
| // JSPromise (may GC) but does not invoke any FileSink | ||
| // host-fn synchronously. | ||
| let promise_result = unsafe { self.pending.get_mut() }.promise(global_this); | ||
| self.writer.with_mut(|w| w.end()); | ||
| self.run_pending_later(); | ||
| // SAFETY: `WritablePending::promise()` never returns null. | ||
| return sys::Result::Ok(unsafe { (*promise_result).to_js() }); | ||
| } |
There was a problem hiding this comment.
🔴 The sibling FileSink::end() (reached via sink.close() → ${name}__doClose → js_close, and via ${controller}__close on the piped-stream path) has the identical WriteResult::Err arm and is left unfixed: it sets done=true, calls writer.end(), and returns — never latching pending.result or scheduling run_pending_later(). A backpressured write()'s promise is therefore still orphaned forever when close() (rather than end()) hits synchronous EPIPE. REVIEW.md requires covering the sync/async twin in the same PR; the same pending.result = Writable::Err(e) + run_pending_later() latch (as already applied in on_auto_flush's Err arm) belongs in end()'s Err arm too.
Extended reasoning...
What the bug is
This PR fixes end_from_js()'s WriteResult::Err arm so that when flush() returns EPIPE synchronously and a backpressured write() has left pending.state == Pending, the error is latched into self.pending and delivered to the outstanding promise. The twin method FileSink::end() has the byte-identical Err-arm shape and is left untouched:
WriteResult::Err(e) => {
self.done.set(true);
self.writer.with_mut(|w| w.end());
sys::Result::Err(e)
}It sets done=true, tears down the writer, and returns the error to the caller — but never touches self.pending or schedules run_pending.
The code path that triggers it
FileSink::end() is JS-reachable through two routes in generate-jssink.ts:
sink.close()on the prototype: line 1178 bindsclose→${name}__doClose(line 496), which callssink->detach()then${name}__close(line 515) →Sink.rs::js_close(line 638) →this.end(None)(line 655) →JsSinkType::end→FileSink::end().controller.close()on the ReadableStream controller:${controller}__close(line 378) reaches the same${name}__closeat line 403. This is the pathassign_to_streamuses.
FileSink does not override get_pending_error (default returns None, Sink.rs:343), so js_close proceeds straight to end().
Why nothing else settles the promise
After end()'s Err arm runs:
writer.end()'s teardown reacheson_close, which only firessignal.close(None)andclear_keep_alive_ref(this)— it never touchesself.pending.- The writer's
on_erroris not invoked:flush()returned the error synchronously as aWriteResult::Err, not via the callback path. on_auto_flushshort-circuits on its first guard (if (*this).done.get() … return false) oncedone==true, without reaching the Err-arm latch that FileSink: reject the pending write() when the deferred auto-flush hits EPIPE #35278 added.run_pending_later()is never scheduled, sorun_pendingnever fires.
The WritablePending slot is therefore left in state == Pending forever.
Step-by-step proof
const p = sink.write(Buffer.alloc(4 * 1024 * 1024))on a socket-pair fd →write_bytes→to_resultreturnsWritable::Pending(self.pending.as_ptr());Writable::to_jscallsWritablePending::promise()which setspending.state = Pendingand hands JS a Promise.- Close the read end → the socket's peer is gone.
sink.close()→${name}__doClose→js_close→FileSink::end(None)→writer.flush()returnsWriteResult::Err(EPIPE)synchronously.- Err arm:
done.set(true);writer.end()(→on_close: signal + keep-alive ref only);return Err(EPIPE). js_closethrows the EPIPE to theclose()caller — butpfrom step 1 is never settled.await phangs forever, and the FileSink (plus its 4 MB buffer) is pinned by the promise's strong ref for the life of the process.
This is the exact scenario the PR's new test exercises, with sink.close() substituted for sink.end().
Impact and fix
REVIEW.md ("Error handling → Every error/abort/timeout path actively completes the operation. Settle every pending promise slot — an unsettled promise pins objects and hangs callers forever") and "Correctness → Fix the whole class in the same PR — sync/async twins … same-class sites are ONE concern, not scope creep" both apply directly. end() and end_from_js() are the canonical twins here; leaving one of the two unfixed re-exposes the same hang/leak on a public prototype method and on the piped-ReadableStream close path.
The fix mirrors what on_auto_flush's Err arm already does (since #35278) — end() returns sys::Result<()> so it can't hand back the promise, but it can settle it:
WriteResult::Err(e) => {
self.done.set(true);
if self.pending.get().state == streams::PendingState::Pending {
self.pending.with_mut(|p| p.result = streams::Writable::Err(e));
}
self.writer.with_mut(|w| w.end());
self.run_pending_later();
sys::Result::Err(e)
}(or hoist the shared latch+schedule into a helper both Err arms call). A sibling test using sink.close() instead of sink.end() would cover it.
| try { | ||
| // Discarded on purpose: the write's promise must not surface on its own. | ||
| sink.write(Buffer.alloc(4 * 1024 * 1024, 0x61)); | ||
| fs.closeSync(readFd); | ||
| readFdOpen = false; | ||
|
|
||
| let caught: any; | ||
| try { | ||
| await sink.end(); | ||
| } catch (e) { | ||
| caught = e; | ||
| } | ||
| expect(caught?.code).toBe("EPIPE"); | ||
|
|
||
| // Bounded window for a stray second rejection to surface. | ||
| for (let i = 0; i < 10; i++) await Bun.sleep(1); | ||
| expect(unhandled).toBeNull(); |
There was a problem hiding this comment.
🟡 Both assertions in this test — expect(caught?.code).toBe("EPIPE") and expect(unhandled).toBeNull() — already pass on pre-fix code (773be9d); the test only fails there because the finally block's defensive await sink.end() returns the never-settled write() promise and times out. To make the assertions load-bearing on the actual fix, hold the write() promise and assert it settles rejected with EPIPE (e.g. const wp = sink.write(...); …; await expect(wp).rejects.toMatchObject({code:'EPIPE'})), or assert a second sink.end() resolves to a number.
Extended reasoning...
What the finding is
The new test claims to prove that end() after a discarded backpressured write() delivers EPIPE exactly once (to the pending promise) with no unhandled rejection. But tracing the pre-fix code (773be9d) through this exact scenario shows both load-bearing assertions already pass — the test only distinguishes pre-fix from post-fix via an incidental cleanup hang in the finally block.
Step-by-step trace on 773be9d
sink.write(4MB)on a fresh AF_UNIX socketpair:PosixStreamingWriter::writefills the send buffer, hits the Pending arm, and callsparent_on_write(amt, Pending)synchronously.FileSink::on_writeregisters the auto-flusher and returns early atstatus == Pending && has_pending_data.to_resultseedspending.state = Pending,pending.result = Owned(consumed), and returnsWritable::Pending(&self.pending)→ JS gets promise P1, which the test discards.fs.closeSync(readFd).sink.end()runs synchronously before any microtask/deferred-task checkpoint (noawaitbetween the write and the end).end_from_js→writer.flush()→drain_buffered_data→try_write→send()on a closed-peer socket →EPIPEwithdrained == 0→ returnsWriteResult::Err(EPIPE)without callingon_error(PipeWriter.rs'sdrained == 0path).- Pre-fix Err arm (773be9d, lines 1135-1138):
WriteResult::Err(err) => { self.done.set(true); self.writer.with_mut(|w| w.end()); sys::Result::Err(err) }
writer.end()→close()→PollOrFd::close_impldeinits the poll and synchronously invokeson_close, which only firessignal.close(None)(dead signal → no-op) andclear_keep_alive_ref. Nothing touchespending.sys::Result::Errpropagates →js_endthrows EPIPE. await sink.end()catches the synchronous throw →expect(caught?.code).toBe("EPIPE")passes ✓.- The deferred auto-flush drains at the first microtask checkpoint.
on_auto_flushseesdone == trueat its very first guard and returnsfalseimmediately — never touchespending. The poll was already deinit'd in step 4, so noon_poll/on_errorcallback ever fires either. P1 stays Pending forever. - An unsettled promise is not an
unhandledRejection→expect(unhandled).toBeNull()passes ✓. finallyblock runstry { await sink.end() } catch {}.end_from_jsseesdone == true && pending.state == Pending && future is Promise→ returnsstrong.value()= P1, still pending forever → theawaithangs → test times out.
So both assertions the test title/comment claim to check pass on pre-fix; the test only fails there via a defensive cleanup line wrapped in try/catch. Delete or reorder that finally-block await sink.end() and the test passes on both builds.
Why the test's premise doesn't hold here
The comment above the test says throwing from end() "would … then let the auto-flush/error path reject the orphaned write() promise as an unhandledRejection". That is true for the Bun.spawn on_attached_process_exit path (which explicitly latches Writable::Err(EPIPE) into pending and calls run_pending), and it's the mechanism this PR's Rust change guards against. But in the socketpair scenario the test actually constructs, pre-fix code never reaches any path that rejects P1 — on_auto_flush bails on done, on_close doesn't run pending, and drain_buffered_data's drained==0 Err path doesn't call on_error. So the unhandledRejection assertion is vacuous in both directions.
REVIEW.md rules violated
- "Prove the test fails for the RIGHT reason … a test that passes both ways is worse than no test" — the assertions pass both ways; only cleanup ordering distinguishes.
- "Every assertion must be able to fail … Hunt vacuous patterns" —
expect(unhandled).toBeNull()cannot fail on either build for this scenario.
How to fix
Make the assertion target the invariant the Rust change actually restores — that the pending write() promise settles (rejected with EPIPE) instead of being orphaned:
const writePromise = sink.write(Buffer.alloc(4 * 1024 * 1024, 0x61));
fs.closeSync(readFd);
readFdOpen = false;
let caught: any;
try { await sink.end(); } catch (e) { caught = e; }
expect(caught?.code).toBe("EPIPE");
// Load-bearing: on 773be9df this promise never settles → test times out here,
// which IS the invariant under test (not a cleanup accident).
await expect(writePromise).rejects.toMatchObject({ code: "EPIPE" });Alternatively (or additionally), assert that a second sink.end() resolves to a number, proving the pending slot was drained rather than left in Pending state.
Severity
nit — the test does still fail on pre-fix (via timeout), so USE_SYSTEM_BUN=1 catches it, and the PR description names test/js/bun/spawn/spawn.test.ts as the primary regression guard for the fix. Merging as-is doesn't leave the Rust change untested. But the assertions are vacuous for what they claim, and the test is fragile: any refactor that removes or reorders the defensive finally-block await sink.end() (which is wrapped in try/catch — clearly best-effort cleanup) turns this into a test that passes on the buggy code.
|
On the two bot reviews that landed after close:
|
…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 -->
…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 -->
2 participants
What this does
Fixes a regression from #35278: when the deferred auto-flush hits EPIPE, the pending
write()promise was left unsettled and the error surfaced as an uncaught exception instead.stdin.end()-style flows that expect the write to reject with EPIPE crashed the process.What changed
FileSink.rs: when the flush path fails afterend(), the stored pending-write promise now receives the rejection (instead of the error escaping to the uncaught handler). The FileSink: reject the pending write() when the deferred auto-flush hits EPIPE #35278 fix's intent is preserved — the error is still delivered, never swallowed — it just lands on the promise that callers actually hold.How we know it works
test/js/bun/spawn/spawn.test.ts"stdin.end() rejects with EPIPE when the child exits before consuming the write" — fails on current main, passes with this change.BUN_FEATURE_FLAG_FORCE_WAITER_THREADfull-file re-run passes.test/js/bun/util/filesink.test.tscovering the deferred-flush EPIPE rejection both with and without a pending write.Found while investigating CI on #34598 — the failure reproduces identically on main's tip (build 78927).