Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 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
26 changes: 20 additions & 6 deletions src/runtime/webcore/FileSink.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1044,16 +1044,35 @@
return sys::Result::Ok(());
}

// A backpressured `write()` may have left its promise in `self.pending`;
// `writer.end()` only re-enters `on_close`, which never touches it, so
// every synchronous arm that tears the writer down here must settle it
// (mirrors `on_auto_flush` / `end_from_js`). `js_close` can't hand the
// promise back, so the outcome is delivered via `run_pending` and the
// call returns `Ok`.
let has_pending = self.pending.get().state == streams::PendingState::Pending;

// SAFETY(JsCell): `IOWriter::flush` is pure I/O; any callback re-entry
// goes via the stored `*mut FileSink` backref, not this borrow.
match self.writer.with_mut(|w| w.flush()) {
WriteResult::Done(written) => {
WriteResult::Done(written) | WriteResult::Wrote(written) => {
self.written.set(self.written.get() + written as usize); // @truncate
self.writer.with_mut(|w| w.end());
if has_pending {
// `to_result` already seeded `Owned(consumed)`; just deliver it.
self.run_pending_later();
}
sys::Result::Ok(())
}

Check failure on line 1066 in src/runtime/webcore/FileSink.rs

View check run for this annotation

Claude / Claude Code Review

end_from_js's Done/Wrote arms leave a backpressured write()'s promise unsettled

`end_from_js()`'s `Done` and `Wrote` arms have the exact same bug this PR fixes in `end()`: they call `writer.end()` and return a plain `js_number` without checking `self.pending` or scheduling `run_pending`, so a backpressured `write()`'s promise is stranded forever. The PR's own `drainedArm` test with `sink.end()` in place of `sink.close()` triggers it, and the new comment here claims the fix "mirrors … `end_from_js`" — but `end_from_js` only mirrors the `Err` arm. Per REVIEW.md's fix-the-whol
Comment thread
robobun marked this conversation as resolved.
WriteResult::Err(e) => {
self.done.set(true);
if has_pending {
self.pending
.with_mut(|p| p.result = streams::Writable::Err(e));
self.writer.with_mut(|w| w.end());
self.run_pending_later();
return sys::Result::Ok(());
}
self.writer.with_mut(|w| w.end());
sys::Result::Err(e)
}
Expand All @@ -1066,11 +1085,6 @@
self.done.set(true);
sys::Result::Ok(())
}
WriteResult::Wrote(written) => {
self.written.set(self.written.get() + written as usize); // @truncate
self.writer.with_mut(|w| w.end());
sys::Result::Ok(())
}
}
}

Expand Down
78 changes: 78 additions & 0 deletions test/js/bun/util/filesink.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -318,6 +318,84 @@
},
);

// Sibling of the end() test above for sink.close() (js_close -> FileSink::end()).
// end()'s Err/Done/Wrote arms set done=true / tore down the writer without
// ever scheduling run_pending, so a backpressured write()'s promise was left
// pending forever. close() can't hand the promise back, so the outcome goes to
// it via run_pending and close() returns undefined.
//
// Runs in a subprocess because sink.close() on a Blob-created FileSink
// currently leaks the native FileSink (doClose detaches m_sinkPtr so the
// wrapper's +1 never reaches finalize); running it in-process would abort the
// whole file under detect_leaks=1. That leak is pre-existing on main and
// tracked separately.
it.skipIf(!isPosix)(
"close() after a backpressured write() settles the write's promise (Err + drained-Done arms)",
async () => {
const src = `
const { createSocketPair } = require("bun:internal-for-testing");
const fs = require("node:fs");

async function errArm() {
const [readFd, writeFd] = createSocketPair();
const sink = Bun.file(writeFd).writer();
const p = sink.write(Buffer.alloc(4 * 1024 * 1024, 0x61));
if (!(p instanceof Promise)) return "err-arm:not-backpressured";
fs.closeSync(readFd);
let threw = false;
try { sink.close(); } catch { threw = true; }
if (threw) return "err-arm:close-threw";
try { await p; return "err-arm:resolved"; }
catch (e) { return "err-arm:" + (e?.code ?? "unknown"); }
}

async function drainedArm() {
const [readFd, writeFd] = createSocketPair();
const sink = Bun.file(writeFd).writer();
const size = 300 * 1024;
const p = sink.write(Buffer.alloc(size, 0x61));
if (!(p instanceof Promise)) return "drained-arm:not-backpressured";
const buf = Buffer.alloc(64 * 1024);
while (true) { try { if (!fs.readSync(readFd, buf)) break; } catch { break; } }
sink.close();
// Drain any remainder close()'s flush pushed to the socket.
while (true) { try { if (!fs.readSync(readFd, buf)) break; } catch { break; } }
let settled = "timeout";
await Promise.race([
p.then(v => { settled = String(v); }, e => { settled = "rejected:" + (e?.code ?? e); }),
// run_pending is an enqueued task, not a timing race; the fixed build
// settles within one tick. The bound only exists for the fail-before.
new Promise(r => setTimeout(r, 500)),
]);
try { fs.closeSync(readFd); } catch {}
try { fs.closeSync(writeFd); } catch {}
return "drained-arm:" + settled + ":" + size;
}

Check failure on line 373 in test/js/bun/util/filesink.test.ts

View check run for this annotation

Claude / Claude Code Review

drainedArm sub-test is platform-dependent and will fail on macOS CI

The `drainedArm` sub-test assumes `close()`'s flush drains the whole remaining buffer in one shot, but that depends on the platform's AF_UNIX buffer size relative to the 300KB payload. On macOS (`net.local.stream.sendspace` defaults to 8192) `flush()` returns `Pending` — not `Done`/`Wrote` — so `run_pending_later()` is never scheduled, nobody drains `readFd` during the 500ms await, and `settled` stays `"timeout"` → the assertion fails on macOS CI. Either drop the drained-arm sub-test (the errArm
Comment thread
robobun marked this conversation as resolved.
Outdated

console.log(await errArm());
console.log(await drainedArm());
`;
await using proc = Bun.spawn({
cmd: [bunExe(), "-e", src],
env: {
...bunEnv,
// Pre-existing leak in sink.close() (see comment above); don't let the
// child's LSAN abort hide the actual assertion we're testing.
ASAN_OPTIONS: "allow_user_segv_handler=1:disable_coredump=0:detect_leaks=0",
},
stderr: "pipe",
});
const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
expect(stderr).toBe("");
const lines = stdout.trim().split("\n");
expect(lines[0]).toBe("err-arm:EPIPE");
// The drained arm resolves with the bytes write() accepted.
const [, settled, size] = lines[1].split(":");
expect(settled).toBe(size);
expect(exitCode).toBe(0);
},
);

// 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
Loading