FileSink: return the chunk's byte count when write() triggers a buffer drain

[robobun]

Fixes #12194.

What

On POSIX, Bun.file(...).writer().write(chunk) normally returns the byte length of chunk. When the sink's internal buffer crosses the auto-flush threshold (4096 bytes, 16384 on Apple Silicon) it instead returned the total bytes drained, which includes every previously-buffered chunk. A caller that sums the returns to track bytes written double-counts everything buffered so far.

Repro

import fs from "node:fs";
const fd = fs.openSync("/tmp/x", "w");
const sink = Bun.file(fd).writer();
let total = 0;
for (let i = 0; i < 60; i++) {
  const r = sink.write(Buffer.alloc(81, "x").toString());
  total += r;
  if (r !== 81) console.log(`i=${i}: rc=${r}`);   // i=50: rc=4131
}
await sink.end();
console.log(total, fs.statSync("/tmp/x").size);    // 8910 4860

Every call writes 81 bytes. The 51st call (buffer crosses 4096) returns 4131 = 51 * 81 instead of 81, so the summed total is almost twice the file size. The bytes on disk are correct; only the return value is wrong.

Cause

PosixStreamingWriter::write takes two paths. When the combined size stays under CHUNK_SIZE it appends and returns WriteResult::Wrote(chunk_len). When it crosses the threshold with data already buffered it appends, then try_write_newly_buffered_data drains the whole buffer and returned WriteResult::Wrote(total_drained). FileSink::to_result maps Wrote(n) straight to the JS return value, so the caller sees the total.

This is the synchronous counterpart of #33538, which fixed the async (Pending) arm via FileSink::bytes_accepted. #33532 had this fix but was closed as superseded by #33538; that PR did not touch the Wrote/Done arms, so the synchronous path was still wrong.

Fix

try_write_newly_buffered_data now takes the caller's chunk_len and returns it on the fully-drained Wrote arm. The Done arm returns the chunk-relative portion that reached the fd before EOF. parent_on_write still receives the total drained (unchanged), and Pending is unchanged: its caller-facing value is already re-derived from buffered_len() in FileSink::bytes_accepted. The three callers (write, write_latin1/write_utf16 via maybe_write_newly_buffered_data) pass the encoded chunk length they already have.

Windows is unaffected: WindowsStreamingWriter::write returns Pending(0) for async file writes and routes through bytes_accepted.

Verification

New test in filesink.test.ts writes a small ASCII string, a large Uint8Array, a Latin-1 string and a UTF-16 string in a pattern that triggers three buffer-drain boundaries, and asserts each return equals Buffer.byteLength(chunk) and the sum matches the final file size.

• USE_SYSTEM_BUN=1 bun test fails with 66041/40010/20001 instead of 65536/40000/20000
• bun bd test filesink.test.ts: 51 pass
• bun bd test spawn-streaming-stdin.test.ts, fs-promises-writeFile-async-iterator.test.ts, process-stdio.test.ts: pass
• bun run rust:check-all: 10 ok, 0 failed

---

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

[coderabbitai]

Warning

Review limit reached

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

Next review available in: 45 seconds

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: 846bd03a-d0b0-4608-8d78-2964bdf78403

📥 Commits

Reviewing files that changed from the base of the PR and between 44f6469 and bdde7de.

📒 Files selected for processing (2)

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

---

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

[robobun]

^{Updated 5:22 PM PT - Jul 26th, 2026}

❌ @robobun, your commit bdde7de has 2 failures in Build #82808 (All Failures):

• test/js/node/worker_threads/worker_threads.test.ts - SIGABRT on 🐧 13 x64-asan
• 📦 Binary size — 12 over 0.50 MB

target	this build	canary: main #79916
		size	Δ
❌ bun-darwin-aarch64	58.13 MB	57.58 MB	+564.9 KB
❌ bun-darwin-x64	63.48 MB	62.95 MB	+544.5 KB
❌ bun-linux-aarch64	70.98 MB	70.42 MB	+576.0 KB
❌ bun-linux-x64	72.47 MB	71.95 MB	+528.0 KB
❌ bun-linux-aarch64-musl	64.88 MB	64.32 MB	+576.0 KB
❌ bun-linux-x64-musl	66.98 MB	66.45 MB	+544.0 KB
❌ bun-linux-aarch64-android	78.47 MB	77.97 MB	+512.0 KB
❌ bun-linux-x64-android	80.62 MB	80.10 MB	+529.2 KB
❌ bun-freebsd-x64	83.07 MB	82.56 MB	+528.0 KB
❌ bun-freebsd-aarch64	84.84 MB	84.31 MB	+544.0 KB
❌ bun-windows-x64	80.26 MB	79.70 MB	+570.5 KB
❌ bun-windows-aarch64	70.86 MB	70.34 MB	+533.0 KB

Add [skip size check] to the commit message if this increase is intentional.

---

🧪   To try this PR locally:

bunx bun-pr 35999

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

bun-35999 --bun

[claude]

LGTM — focused return-value accounting fix; bytes-on-disk unchanged.

What was reviewed:

• Traced all three callers (write, write_latin1/write_utf16 via maybe_write_newly_buffered_data) — each passes the encoded chunk length it already computed.
• Confirmed parent_on_write still receives the total drained amount, so FileSink::written accounting is unchanged; only the JS-facing return via to_result is affected.
• Checked the other PosixStreamingWriter consumer (Terminal.rs) — it ignores per-arm counts and returns input_len directly, so unaffected.
• Done arm ordering: old_buffered is captured before outgoing.reset(), and only locals are used after parent_on_write — no reentrancy hazard.

Extended reasoning...

Overview

This PR fixes #12194: on POSIX, FileSink::write() returned the total bytes drained from the internal buffer (including previously-buffered chunks) when a write triggered the auto-flush threshold, instead of the current chunk's byte length. Callers summing return values to track bytes written would double-count.

The fix threads the caller's chunk_len into try_write_newly_buffered_data and returns it on the fully-drained Wrote arm; the Done arm now returns the chunk-relative portion (amt - old_buffered). parent_on_write still receives the full drained total, so FileSink's internal written counter and on_write bookkeeping are unchanged. Pending is untouched — its caller-facing value is already re-derived from buffered_len() in FileSink::bytes_accepted (fixed in #33538).

Security risks

None. This is return-value accounting only; the actual bytes written to the fd are unchanged.

Level of scrutiny

Medium — this is core I/O plumbing, but the change is narrowly scoped to one private helper's return value on two match arms. I verified:

• All three call sites pass the correct chunk length (encoded byte count for latin1/utf16, buf.len() for raw bytes).
• The only other PosixStreamingWriter parent besides FileSink is Terminal, which explicitly ignores per-arm counts (Terminal.rs:1517-1522) and returns input_len directly — its comment even documents the pre-fix behavior this PR corrects.
• The condition rewrite (amt == size → amt < size else fully-drained) is equivalent since try_write cannot report more than outgoing.slice().len().
• old_buffered is computed before reset() and before parent_on_write (which may re-enter), and the return uses only locals afterward.

Other factors

• The new test covers all three encoding paths (raw bytes, Latin-1, UTF-16) with three separate buffer-drain crossings, and asserts both per-call return values and that the sum equals statSync().size. It's correctly skipped on Windows where file writes always return a promise.
• PR description documents USE_SYSTEM_BUN=1 failure (66041/40010/20001 vs expected 65536/40000/20000) and passing runs of filesink.test.ts, spawn-streaming-stdin, fs-promises-writeFile-async-iterator, process-stdio, and rust:check-all.
• CI status shows only "Failed to create agent" infrastructure errors, not test failures.

[claude]

LGTM — narrow return-value fix in one private helper, all call sites updated, thorough test.

What was reviewed:

• Traced all three callers of try_write_newly_buffered_data (write, write_latin1/write_utf16 via maybe_write_newly_buffered_data) — each passes the encoded chunk length it already computed.
• Checked the other PosixStreamingWriter consumer (Terminal.rs) — it ignores the per-arm count and returns input_len directly, so unaffected.
• Verified parent_on_write still receives the total drained amt, so FileSink's written-bytes accounting is unchanged; old_buffered in the Done arm is captured before reset()/parent_on_write, so re-entrancy is not an issue.
• Test hits all three encoding paths across the auto-flush boundary and cross-checks against the on-disk file size.

Extended reasoning...

Overview

Fixes a return-value bug in PosixStreamingWriter::try_write_newly_buffered_data (src/io/PipeWriter.rs): when a write() call pushes the internal buffer past the auto-flush threshold and the drain succeeds synchronously, the function previously returned the total bytes drained (including previously-buffered chunks) instead of the caller's chunk length. FileSink::to_result maps Wrote(n) straight to the JS return, so sink.write(chunk) over-reported. The fix threads chunk_len into the helper and returns it on the fully-drained Wrote arm; the Done arm returns the chunk-relative portion (amt - old_buffered, saturating). Pending is unchanged — its caller-facing value is already re-derived in FileSink::bytes_accepted. Net change is ~20 lines in one private function plus its three call sites, and one new test.

Security risks

None. This adjusts an integer return value that surfaces to JS as the byte count from FileSink.write(). No allocation sizes, buffer bounds, or fd handling are affected. parent_on_write continues to receive the actual drained amt, so internal accounting is untouched.

Level of scrutiny

Low-to-medium. The function is private and has exactly three call sites, all in the same file and all updated. The other PosixStreamingWriterParent consumer, Terminal, ignores the WriteResult count entirely (it matches only on Err vs not-Err and returns input_len) — its own comment already documents this exact over-reporting hazard as the reason. Windows uses a separate writer and routes through Pending/bytes_accepted, so is unaffected as the PR states. The Wrote-arm restructure (if amt < size { … return Pending } else { … return Wrote(chunk_len) }) is behaviour-equivalent to the old if amt == size shape modulo the returned count, since amt is bounded by outgoing.slice().len().

Other factors

• The Done arm captures old_buffered = outgoing.size() - chunk_len before reset() and before parent_on_write (which the file's own comment notes may re-enter write()), so the computation is sound. The debug_assert!(outgoing.size() >= chunk_len) holds because the chunk was just appended and try_write takes &self.
• The new test is placed in the existing filesink.test.ts, uses tempDir, is skipIf(!isPosix) with a stated reason, exercises the raw-bytes / latin1 / utf16 paths across three flush boundaries, asserts exact per-write returns, and cross-checks the sum against fs.statSync().size. The PR verified it fails under USE_SYSTEM_BUN=1 and passes under bun bd, and rust:check-all is green.
• The comment-cop bot flagged the doc comment twice (both threads resolved); the final three-line form documents the parameter contract and matches the surrounding file's style.

[robobun]

CI status for bdde7de (build 82808): 192/196 jobs passed, filesink.test.ts green on every lane. The two red lanes are unrelated to this diff:

• binary-size: comparing against a stale baseline (main #79916); every recent PR hits the same +500 KB delta (e.g. #82800). This change is ~15 lines in one private helper and does not affect codegen size.
• debian 13 x64-asan: worker_threads.test.ts SIGABRT on ExceptionScope::assertNoException during worker terminate, which is the pre-existing issue ASAN CI: ExceptionScope::assertNoException during worker terminate (worker-transfer-terminate-stress, separate from #34095) #34690. This diff touches only the return value of PosixStreamingWriter::try_write_newly_buffered_data and does not reach the worker termination path.

All other failures were retried and passed ([flaky]). Ready for review.

[robobun]

Re-checked against current main (165dc9f) since #12194 was closed: this PR is still needed. #12194 was closed for the backpressured (Pending) arm fixed in #33538; the synchronous drain arm this PR covers is unchanged on main (try_write_newly_buffered_data still returns the total drained and FileSink::to_result maps Wrote(n) straight through). The repro above still prints i=50: rc=4131 for an 81 byte write (summed returns 8910, file size 4860), and the new test in filesink.test.ts still fails on a plain main build with 66041/80010/20001 instead of 65536/80000/20000.