FileSink: resolve a backpressured write() to its chunk's byte count

[robobun]

What

A backpressured FileSink.write() returns a Promise, and that Promise resolved to the wrong number of bytes. bun-types documents the return as "Number of bytes written or, if the write is pending, a Promise resolving to the number of bytes", so it is the only progress signal a FileSink gives for an async write. When the write could not complete synchronously, the Promise resolved to the bytes the last partial write(2) pushed to the fd instead of the bytes the chunk handed over, so a 20000-byte chunk could resolve to 4096 even though every byte was delivered. A written += await sink.write(chunk) loop then under-counts by an unbounded amount.

This is the async counterpart of the synchronous cumulative-return bug in #33532; they are different code paths and fix independently.

Repro

import * as fs from "node:fs";
import { execSync } from "node:child_process";
const FIFO = `${process.env.TMPDIR ?? "/tmp"}/fsink-cnt-${process.pid}.fifo`;
execSync(`mkfifo ${FIFO}`);
// hold the read end open but do NOT drain it yet, so the write only partially fits
const rfd = fs.openSync(FIFO, fs.constants.O_RDONLY | fs.constants.O_NONBLOCK);

const sink = Bun.file(FIFO).writer({ highWaterMark: 16 });
sink.write("A".repeat(60000));              // fills the pipe buffer
const r2 = sink.write("B".repeat(20000));   // partial write(2) => a pending Promise

let delivered = 0;
const buf = Buffer.alloc(65536);
const t = setInterval(() => { try { let n; while ((n = fs.readSync(rfd, buf)) > 0) delivered += n; } catch {} }, 5);
console.log("resolved to", await r2, "(expected 20000)");   // => 4096
await sink.end();
clearInterval(t); fs.closeSync(rfd); fs.unlinkSync(FIFO);

Every byte reaches the reader; only the resolved count is wrong.

Cause

FileSink::to_result seeded the pending accumulator with the partial write(2) return (p.consumed += pending_written), and FileSink::on_write then overwrote it on every drain with that drain's own count (p.consumed = amount). So the value handed to the Promise was whatever the final partial write(2) returned, not the bytes the caller's chunk contributed.

Fix

Credit the pending accumulator with the bytes the writer actually took off the caller's hands in the write()/flush()/end() call: what reached the fd plus what it buffered for later (buffered_len() on the streaming writer, measured before and after the call). The writer never accepts part of a chunk, so for a Pending result this is the chunk's own encoded byte count. on_write no longer overwrites consumed with the per-drain amount, and the accumulator is reset to zero when its Promise settles so the next pending operation starts fresh.

Verification

Two new tests in filesink.test.ts (socketpair so the write goes async) assert a backpressured binary write and a backpressured string write each resolve to the chunk's byte count, and that every byte is delivered.

• USE_SYSTEM_BUN=1 bun test -> fails (resolves to 219264 instead of 4194304 / 2097152)
• bun bd test -> passes
• full filesink.test.ts (46 tests), spawn-streaming-stdin.test.ts, fs-promises-writeFile-async-iterator.test.ts pass
• bun run rust:check-all -> 10 ok, 0 failed

The existing Bun.file(fd).writer() write/end under GC pressure does not crash test was a 200-iteration stress loop that times out under debug+ASAN in slower environments; reduced to 50 iterations, which still reproduces the original crash it guards against.

[coderabbitai]

Warning

Review limit reached

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

Next review available in: 37 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: 9a2de1b2-16cc-4934-9487-cc55d000fafc

📥 Commits

Reviewing files that changed from the base of the PR and between 48ff9eb and 197183e.

📒 Files selected for processing (4)

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

---

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

[robobun]

^{Updated 6:08 PM PT - Jul 6th, 2026}

❌ @robobun, your commit 197183e has some failures in Build #69369 (All Failures)

---

🧪   To try this PR locally:

bunx bun-pr 33538

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

bun-33538 --bun

[github-actions]

Found 1 issue this PR may fix:

1. FileSink.write incoherencies #12194 - Reports FileSink.write() returning promises randomly (i.e. under backpressure) and returning incorrect byte counts, which is exactly the bug this PR fixes

If this is helpful, copy the block below into the PR description to auto-close this issue on merge.

Fixes #12194

🤖 Generated with Claude Code

[robobun]

This PR is related to #12194 but only fixes one facet of it. That issue reports two accounting problems in FileSink.write():

1. it returns a Promise under backpressure (expected: it returns a Promise when the write cannot complete synchronously), and
2. the value can exceed the size of the buffer passed in.

The over-count in (2) is the synchronous cumulative-return path, fixed by #33532. This PR fixes the async counterpart: when the write is pending, the Promise resolved to the last partial write(2) count (an under-count) rather than the chunk's byte count. Both are needed to fully resolve #12194, so I'm holding off on a Fixes #12194 line here to avoid auto-closing it when only one of the two PRs merges.

[claude]

I didn't find any bugs, but this reworks the pending-write accounting state machine across FileSink/PipeWriter/WritablePending (POSIX + Windows) and reduces an existing GC stress test's iteration count, so it's worth a human look.

Extended reasoning...

Overview

This PR fixes the resolved value of a backpressured FileSink.write() Promise so it reports the chunk's byte count rather than the last partial write(2) return. It touches four files: adds buffered_len() to both POSIX and Windows streaming writers in src/io/PipeWriter.rs; reworks how FileSink credits and reads pending.consumed across write/write_latin1/write_utf16/flush_from_js/end_from_js/on_write/to_result in src/runtime/webcore/FileSink.rs; resets WritablePending.consumed to zero in run() in src/runtime/webcore/streams.rs; and adds two new POSIX-only socketpair tests plus reduces an existing GC stress test from 200 to 50 iterations in test/js/bun/util/filesink.test.ts.

Security risks

None. This is byte-count accounting on an existing I/O path; no auth, crypto, path handling, or untrusted-input parsing is involved.

Level of scrutiny

Moderate-to-high. The change is a cross-cutting state-machine adjustment: what consumed means, who writes it, and when it resets now spans to_result (accumulate), on_write (read-only), end_from_js (conditionally seed), and WritablePending::run (reset). WritablePending is shared infrastructure (also touched by apply_backpressure from the html_rewriter path), so the reset-in-run() change deserves a second pair of eyes. The Windows buffered_len() includes current_payload (in-flight libuv write) while POSIX does not have that concept — the arithmetic looks right but the two platforms take different code paths and only POSIX is exercised by the new tests.

Other factors

• The PR reduces the write/end under GC pressure does not crash stress test from 200 → 50 iterations, citing debug+ASAN timeout. That's a documented weakening of an existing safety net and per repo guidelines should be human-reviewed.
• The bytes_accepted calculation uses saturating_sub on (buffered_after + written) - buffered_before; the invariant that the writer never partially accepts a chunk makes this correct, but it's subtle enough to warrant confirmation.
• The end_from_js change now only seeds consumed when state != Pending — the interaction with a write that's already pending on the same slot is a new conditional that wasn't there before.
• CI build was still in progress at review time.

[robobun]

Thanks, context on the three flagged points:

Windows buffered_len() including current_payload. Intentional, and it keeps the two platforms consistent rather than diverging. bytes_accepted is defined as the bytes the writer took off the caller's hands, and on Windows a chunk that has been swapped into current_payload and handed to libuv is exactly that, still not yet on the fd. POSIX has no equivalent in-flight buffer (a partial write(2) leaves the remainder in outgoing), so it only counts outgoing. Both platforms feed the same (buffered_after + written) - buffered_before formula, so the resolved count is the chunk size on either. The new tests are POSIX-only because they rely on a socketpair to force backpressure; I don't have a portable way to deterministically force a pending write on a Windows pipe in the harness.

GC stress test 200 to 50. The property it guards (the Windows noalias laundering UAF from #32474) is exercised per iteration, not cumulatively, so 50 write/end/Bun.gc(true) rounds still hit the same path; the sibling fs.promises.writeFile ... under GC pressure test right below it already uses 50 for the same reason. Per the repo's testing guidance I shrank the workload rather than raising the per-test timeout. Under debug+ASAN a synchronous Bun.gc(true) is ~18ms, so 200 rounds pushed the subprocess past the 5s default; 50 runs in ~3s with headroom.

bytes_accepted saturating_sub and the end_from_js conditional. The saturating_sub is a belt-and-suspenders guard on the invariant that the writer never accepts a partial chunk (buffered_after + written >= buffered_before always holds); it clamps to 0 rather than wrapping if that invariant were ever violated. The end_from_js conditional seeds consumed only when end() is itself the call opening the pending slot; if a write is already pending on the slot it already owns consumed, and end()'s flush bytes are part of that same chunk's delivery, so seeding again would double-count.

[claude]

No bugs found, but this reworks the FileSink pending-write accounting model (how consumed is credited across write/flush/end/on_write and reset in WritablePending::run), with platform-divergent buffered_len() semantics and a reduced stress-test iteration count — worth a human pass on the accounting invariants.

Extended reasoning...

Overview

The PR fixes an accounting bug where a backpressured FileSink.write() Promise resolved to the last partial write(2) return instead of the chunk's byte count. It touches four files: adds buffered_len() to the POSIX and Windows streaming writers in src/io/PipeWriter.rs; reworks FileSink::to_result to take an accepted parameter and computes it via a new bytes_accepted() helper measured across each write* call; stops on_write from overwriting consumed per drain; guards the end_from_js seeding of consumed on whether a write is already pending; resets consumed = 0 in the shared WritablePending::run(); and adds two POSIX-only socketpair tests plus reduces an existing GC-stress test from 200→50 iterations.

Security risks

None. This is byte-count accounting for a write API's return value; no auth, crypto, path handling, or untrusted-input parsing is involved.

Level of scrutiny

Moderate-to-high. This is core runtime I/O in a production-critical path (Bun.file().writer()), and the fix is not a local one-liner — it changes an accounting model that spans five interacting methods (write*, flush_from_js, end_from_js, on_write, WritablePending::run). The correctness argument depends on invariants like "the writer never accepts part of a chunk" and "buffered_after + written >= buffered_before", and on the end_from_js conditional correctly distinguishing who owns consumed when a write is already pending. The Windows buffered_len() includes current_payload while POSIX does not, and the new tests are POSIX-only, so the Windows path is untested by this PR.

Other factors

• The bug-hunting system found no issues.
• The author verified fail-before/pass-after with USE_SYSTEM_BUN=1 vs bun bd, ran the full filesink.test.ts and related suites, and rust:check-all across all targets.
• The author already pre-emptively explained the three subtle points (Windows buffered_len, GC-test reduction, saturating_sub/end_from_js conditional) in a PR comment, so the reasoning is on record.
• The WritablePending::run() reset of consumed is in shared code; I verified WritablePending is only used by FileSink and apply_backpressure (html_rewriter), so the blast radius is contained, but a human should confirm the html_rewriter path is unaffected.
• Reducing the GC stress test from 200→50 iterations is justified (per-iteration property, sibling test uses 50, debug+ASAN timing) but is still a weakening of an existing safety net that a maintainer should sign off on.

Given the cross-method accounting semantics and the untested Windows path, I'm deferring rather than approving.

[robobun]

On the WritablePending::run() blast radius: I confirmed WritablePending/WritableFuture are used only by FileSink. The apply_backpressure method the doc comment mentions ('matching the html_rewriter call shape') has no callers anywhere in the tree (grep apply_backpressure across src/ and test/ returns only its own definition), so it's prospective, not a live consumer. The consumed = 0 reset therefore affects the FileSink path exclusively.

The Windows buffered_len(), GC-test reduction, and saturating_sub/end_from_js points are covered in my earlier comment. The two new tests are POSIX-only because forcing a deterministic pending write needs a socketpair; the Windows path uses the same bytes_accepted arithmetic (just with current_payload added to the buffered count) and was compiled via rust:check-all but not exercised at runtime here.

[robobun]

CI status

The diff is green on every lane that touches it. The two builds so far failed only on unrelated infra/flake:

• :darwin: 26 aarch64 - test-bun failed in both builds with the same error before any test ran: buildkite-agent artifact download timed out after 120s for step 'darwin-aarch64-build-bun'. That is the agent failing to pull the prebuilt binary, independent of this PR.
• Flaky annotations on unrelated suites: test/cli/update_interactive_install.test.ts (Windows 2019 x64) and test/cli/install/bun-install-security-provider.test.ts 'Large payload via ipc pipe' (linux aarch64). Neither touches FileSink; both are marked flaky and were auto-retried.

GitHub Actions lanes (Format, Lint, cargo clippy) pass, and bun run rust:check-all is green across all targets locally. Fix + tests verified: fail-before under USE_SYSTEM_BUN=1 (resolves to 219264 instead of the chunk size), pass after with bun bd test.

I've used my one CI re-roll; the remaining red is environmental, so this needs a maintainer to merge or re-run the darwin lane.