Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 25 additions & 0 deletions src/runtime/webcore/FileSink.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1134,6 +1134,31 @@ impl FileSink {
}
WriteResult::Err(err) => {
self.done.set(true);
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() });
}
Comment on lines +1137 to +1161

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 The sibling FileSink::end() (reached via sink.close()${name}__doClosejs_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 binds close${name}__doClose (line 496), which calls sink->detach() then ${name}__close (line 515) → Sink.rs::js_close (line 638) → this.end(None) (line 655) → JsSinkType::endFileSink::end().
  • controller.close() on the ReadableStream controller: ${controller}__close (line 378) reaches the same ${name}__close at line 403. This is the path assign_to_stream uses.

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 reaches on_close, which only fires signal.close(None) and clear_keep_alive_ref(this) — it never touches self.pending.
  • The writer's on_error is not invoked: flush() returned the error synchronously as a WriteResult::Err, not via the callback path.
  • on_auto_flush short-circuits on its first guard (if (*this).done.get() … return false) once done==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, so run_pending never fires.

The WritablePending slot is therefore left in state == Pending forever.

Step-by-step proof

  1. const p = sink.write(Buffer.alloc(4 * 1024 * 1024)) on a socket-pair fd → write_bytesto_result returns Writable::Pending(self.pending.as_ptr()); Writable::to_js calls WritablePending::promise() which sets pending.state = Pending and hands JS a Promise.
  2. Close the read end → the socket's peer is gone.
  3. sink.close()${name}__doClosejs_closeFileSink::end(None)writer.flush() returns WriteResult::Err(EPIPE) synchronously.
  4. Err arm: done.set(true); writer.end() (→ on_close: signal + keep-alive ref only); return Err(EPIPE).
  5. js_close throws the EPIPE to the close() caller — but p from step 1 is never settled. await p hangs 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.

self.writer.with_mut(|w| w.end());
sys::Result::Err(err)
}
Expand Down
47 changes: 47 additions & 0 deletions test/js/bun/util/filesink.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -273,6 +273,53 @@ it.skipIf(!isPosix)("a backpressured string write() resolves to its encoded byte
expect(received).toBe(size);
});

// end() called after a backpressured write() whose promise the caller
// discarded, with the reader already gone: end_from_js's flush() sees EPIPE
// synchronously. Throwing it would report the failure to end()'s caller and
// then let the auto-flush/error path reject the orphaned write() promise as an
// unhandledRejection; instead the error is delivered to that pending promise
// and end() returns the same promise, so the failure is reported exactly once.
it.skipIf(!isPosix)(
"end() after a discarded backpressured write() delivers EPIPE once, with no unhandled rejection",
async () => {
const [readFd, writeFd] = createSocketPair();
let readFdOpen = true;
const sink = Bun.file(writeFd).writer();
let unhandled: any = null;
function onUnhandled(e: unknown) {
unhandled = e;
}
process.on("unhandledRejection", onUnhandled);
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();
Comment on lines +293 to +309

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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

  1. sink.write(4MB) on a fresh AF_UNIX socketpair: PosixStreamingWriter::write fills the send buffer, hits the Pending arm, and calls parent_on_write(amt, Pending) synchronously. FileSink::on_write registers the auto-flusher and returns early at status == Pending && has_pending_data. to_result seeds pending.state = Pending, pending.result = Owned(consumed), and returns Writable::Pending(&self.pending) → JS gets promise P1, which the test discards.
  2. fs.closeSync(readFd).
  3. sink.end() runs synchronously before any microtask/deferred-task checkpoint (no await between the write and the end). end_from_jswriter.flush()drain_buffered_datatry_writesend() on a closed-peer socket → EPIPE with drained == 0 → returns WriteResult::Err(EPIPE) without calling on_error (PipeWriter.rs's drained == 0 path).
  4. 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_impl deinits the poll and synchronously invokes on_close, which only fires signal.close(None) (dead signal → no-op) and clear_keep_alive_ref. Nothing touches pending. sys::Result::Err propagates → js_end throws EPIPE.
  5. await sink.end() catches the synchronous throw → expect(caught?.code).toBe("EPIPE") passes ✓.
  6. The deferred auto-flush drains at the first microtask checkpoint. on_auto_flush sees done == true at its very first guard and returns false immediately — never touches pending. The poll was already deinit'd in step 4, so no on_poll/on_error callback ever fires either. P1 stays Pending forever.
  7. An unsettled promise is not an unhandledRejectionexpect(unhandled).toBeNull() passes ✓.
  8. finally block runs try { await sink.end() } catch {}. end_from_js sees done == true && pending.state == Pending && future is Promise → returns strong.value() = P1, still pending forever → the await hangs → 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.

} finally {
process.off("unhandledRejection", onUnhandled);
try {
await sink.end();
} catch {}
try {
fs.closeSync(writeFd);
} catch {}
if (readFdOpen) fs.closeSync(readFd);
}
},
);

// The deferred auto-flush microtask runs at the first microtask checkpoint
// after write() backpressures. If its flush() hit EPIPE, it discarded the
// error and then let `run_pending_later()` resolve the pending write() promise
Expand Down