FileSink: flush buffered bytes in finalize before closing

[robobun]

Reproduction

import { readFileSync } from "node:fs";
for (let i = 0; i < 3; i++) {
  const w = Bun.file("/tmp/sink-" + i).writer();
  w.write("payload-" + i);
  // abandoned: no flush(), no end(), no reference kept
}
Bun.gc(true); await Bun.sleep(20); Bun.gc(true);
process.on("exit", () =>
  console.log([0,1,2].map(i => JSON.stringify(readFileSync("/tmp/sink-" + i, "utf8"))).join(" ")));
// before:  "" "" ""
// after:   "payload-0" "payload-1" "payload-2"

Without the Bun.gc() calls the same program flushes correctly at exit, so this is GC-timing-dependent data loss. The real-world shape is per-request or per-job log/append sinks created and dropped in a long-lived process: the last unflushed chunk vanishes whenever a collection lands before the deferred-microtask drain.

Cause

FileSink::write() buffers small writes (below CHUNK_SIZE) in the StreamingWriter's outgoing buffer and registers an AutoFlusher deferred microtask to drain it after the current microtask queue. If a GC sweep collects the JS wrapper before that deferred task runs, FileSink::finalize() drops the wrapper's ref, deinit() unregisters the deferred task and drops the writer, and close_without_reporting() closes the fd with the buffer still unwritten.

Fix

Add StreamingWriter::drain_without_reporting() and call it from the top of FileSink::finalize():

• POSIX: loops try_write() over the outgoing buffer (pure syscalls via get_fd()/get_file_type(); no Parent::on_* dispatch). Stops on the first retry/error, so a backpressured pollable fd does not block the sweep.
• Windows: no-op. Every non-SyncFile write already went through process_send(), which queued a uv_fs_write/uv_write and took a parent ref, so the native sink already outlives its JS wrapper until the completion fires.

This does not go through IOWriter::flush(): flush() can dispatch Parent::on_* via the stored *mut FileSink backref while finalize holds &mut self, which is the R-2 noalias re-entry the file's header and on_auto_flush doc warn against (and the POSIX error arm would reach run_pending() and touch a JSPromise during the sweep).

Verification

New test in test/js/bun/util/filesink.test.ts spawns a subprocess that creates three abandoned sinks with small buffered writes, forces two full GCs around a sleep, and asserts the file contents match the payloads.

• fails on main with ["", "", ""]
• passes with this change
• full filesink.test.ts suite (51 tests) passes

---

no test proof · iteration 1 · 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

[coderabbitai]

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 6 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 26aed97b-4ede-4885-af61-6b3284d4d536

📥 Commits

Reviewing files that changed from the base of the PR and between ae4b17d and f595858.

📒 Files selected for processing (3)

• src/io/PipeWriter.rs
• src/runtime/webcore/FileSink.rs
• test/js/bun/util/filesink.test.ts

---

_{Comment @coderabbitai help to get the list of available commands.}

[robobun]

Status: diff is green. filesink.test.ts passed on every test lane across builds #80124 and #80205; the new test fails on stock 1.4.0 with empty files.

Remaining CI red is unrelated infra:

• build #80205: darwin-aarch64-build-bun and linux-x64-asan-build-bun timed out (state: timed_out, exit -1); both passed in #80124 so this code compiles fine on those targets
• linux-x64-musl-build-bun and linux-aarch64-android-build-bun are "step failed outside runner", pre-existing on main
• test-repl-close.js EPIPE on Windows, process.memoryUsage.arrayBuffers on Windows aarch64, bun-security-scanner-matrix on Alpine: all flaky, passed on retry

Ready for review/merge.

[robobun]

^{Updated 11:36 PM PT - Jul 24th, 2026}

❌ @robobun, your commit f595858 has 1 failures in Build #80205 (All Failures):

• step failed outside runner - exit 1 on 🐧 x64-musl - build-bun
• step failed outside runner - exit 1 on 🐧 aarch64-android - build-bun

---

🧪   To try this PR locally:

bunx bun-pr 35528

That installs a local version of the PR into your bun-35528 executable, so you can run:

bun-35528 --bun

[claude]

The revised fix addresses my earlier R-2 concern — drain_without_reporting() on POSIX only touches try_write() (reads get_fd()/get_file_type() and does syscalls; no Parent::on_* dispatch), and the Windows no-op is justified since process_send() already holds a parent ref past the wrapper's finalize. I didn't find further issues, but this is a semantic change to GC-finalize behaviour in a file whose header is dense with aliasing/refcount invariants, so a maintainer sign-off is worth having.

What was reviewed:

• Traced POSIX drain_without_reporting() → try_write() → try_write_with_write_fn: no path reaches the *mut FileSink parent backref, so no R-2 re-entry while finalize holds &mut self.
• Verified Windows write_internal_u8 always reaches process_send() (or a prior one is in flight) for non-SyncFile sources, and process_send() refs the parent on the queue-success path (line ~2325), so the sink already outlives the wrapper — the no-op is sound.
• Checked write_to_blocking_pipe (used for FileType::Pipe) uses non-blocking write / 0-timeout poll, so the drain cannot stall the sweep.
• Confirmed the outgoing.wrote(drained) cursor advance means a surviving sink (keep-alive ref held) won't double-write when its poll later fires.

Extended reasoning...

Overview

Adds StreamingWriter::drain_without_reporting() (POSIX: best-effort try_write loop over outgoing; Windows: no-op) and calls it from the top of FileSink::finalize(), so buffered small writes are pushed to the fd when the JS wrapper is collected before the deferred auto-flush runs. Adds a subprocess-spawning regression test in test/js/bun/util/filesink.test.ts.

Prior review resolution

My earlier review flagged that the first iteration called IOWriter::flush() from finalize(&mut self), which re-enters FileSink via the writer's stored *mut FileSink backref (Windows process_send() → Parent::ref_/loop_; POSIX partial-write-then-error → Parent::on_error) — the exact R-2 noalias hazard the file documents. The revised approach avoids this: drain_without_reporting() reads only writer-local state (is_done, closed_without_reporting, force_sync, outgoing) and try_write() reads only get_fd()/get_file_type() from self.handle before doing raw syscalls. No Parent::on_* dispatch is reachable, so no backref access occurs while &mut self is live and no JSPromise can be resolved during the sweep.

Security risks

None — pure I/O path, no untrusted input parsing, no auth/crypto.

Level of scrutiny

High. FileSink.rs is one of the most delicate files in the runtime for aliasing/provenance (the R-2 header, the borrow = ptr macro note, and on_auto_flush's doc all exist because prior iterations here were UB). finalize runs during the GC lazy sweep and its own header forbids touching live JS cells. Adding I/O to a finalizer is a semantic change (best-effort flush-on-GC) that a maintainer should confirm is the intended contract — though the fact that Windows already exhibits this behaviour (via the parent ref) and process-exit already flushes both point to yes.

Other factors

• The test follows harness conventions (tempDir, bunEnv, concurrent pipe drain, stderr asserted before exitCode) and mirrors the existing "under GC pressure" subprocess pattern in the same file.
• The Err(_) => 0 arm in drain_without_reporting() drops the partially-written offset that try_write accumulated internally before the error, but since the error path leaves the fd broken (subsequent writes will also fail) and finalize is best-effort, no double-write can reach the fd.
• On Windows the test is not gated; it depends on the uv_fs_write completion landing during await Bun.sleep(20) — should be fine, but worth watching in CI.
• comment-cop threads are resolved with the author's justification for keeping the trimmed "why not flush()" comments; I agree those are load-bearing.

[claude]

The R-2 backref re-entry from the first revision is fixed — drain_without_reporting() on POSIX only reads get_fd()/get_file_type() and loops sys::write, with no Parent::on_* dispatch. Deferring for a human to confirm the Windows no-op invariant (that every non-SyncFile write leaves a process_send() ref outstanding whenever has_pending_data() is true), since this is GC-finalize + cross-platform refcounting.

Checked: try_write → try_write_with_write_fn touches only handle and the fd (no parent backref); write_to_blocking_pipe polls first / uses RWF_NOWAIT so a blocking-pipe fd won't stall the sweep; WindowsStreamingWriter::write_internal_u8 calls process_send() on the first buffered write, and the completion chain re-queues while outgoing is non-empty. Test drains pipes concurrently, uses tempDir, and asserts stderr/exitCode.

Extended reasoning...

Overview

Adds PosixStreamingWriter::drain_without_reporting() (best-effort try_write loop over outgoing, no parent-callback dispatch) and a no-op Windows counterpart, called from the top of FileSink::finalize() so buffered bytes reach the fd when the JS wrapper is GC'd before the deferred auto-flush runs. One new subprocess test in filesink.test.ts.

Prior review resolved

The first revision called IOWriter::flush() from finalize(&mut self), which I flagged as R-2 noalias UB (backref re-entry into Parent::on_* while &mut self is live, plus run_pending() touching a JSPromise during lazy sweep). Commit 642ea68 replaced it with drain_without_reporting(). I traced the POSIX call graph: try_write → get_file_type() (poll-flag read) / get_fd() → try_write_with_write_fn → sys::write/send_non_block/write_to_blocking_pipe. None of these touch self.parent. outgoing.{slice,wrote,is_empty,reset} are pure StreamBuffer ops. So no re-entry into FileSink while &mut self is held — the concern is addressed.

Also checked that write_to_blocking_pipe polls with is_writable() (or uses RWF_NOWAIT on Linux) rather than blocking, so a backpressured blocking-pipe fd won't stall the GC sweep; it maps to Pending(n) and drain_without_reporting stops there.

Security risks

None. Best-effort draining of already-buffered user bytes to an already-open fd; errors are dropped (nothing to report to — the wrapper is dead).

Level of scrutiny

High. This runs during JSC lazy sweep and interacts with the FileSink intrusive-refcount / *mut FileSink backref machinery that this file's R-2 header and the impl_streaming_writer_parent!(borrow = ptr) note document at length. The Windows no-op rests on the invariant that whenever has_pending_data() is true, a process_send()-taken parent ref is already outstanding — I read write_internal_u8 and it holds for the first-write case (empty outgoing → process_send()) and the chained case (had_buffered_data → prior process_send() still in flight), but a maintainer familiar with the Windows completion chain should confirm.

Other factors

All prior threads are resolved. Bug-hunting pass found nothing on the current revision. Test follows harness conventions (tempDir, bunEnv, concurrent pipe drain, stderr-before-exitCode). CI build #80124 is in progress.

[robobun]

Closing as part of a cleanup of stale pull requests. This PR has had no new commits since 2026-07-25, it conflicts with main, and its last CI run failed. This is not a judgment on the fix itself. If the problem still reproduces on a current build, reopen this PR after a rebase or open a new one against main.