Conversation
|
Warning Review limit reached
Next review available in: 25 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the 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 configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
|
Updated 2:21 AM PT - Jul 7th, 2026
❌ @robobun, your commit 97b3f3a has some failures in 🧪 To try this PR locally: bunx bun-pr 33532That installs a local version of the PR into your bun-33532 --bun |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…33538) ## 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 ```js 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.
FileSink.write() is documented to return the number of bytes accepted from the current chunk. When a chunk is appended to a non-empty outgoing buffer and the combined size crosses CHUNK_SIZE, the buffer is drained and the streaming writer reported the total bytes drained (older buffered chunks included) instead of the chunk's own size. Any written += write(chunk) accounting loop was then wrong by the size of the previously buffered data. try_write_newly_buffered_data now takes the chunk length and returns it on the fully-drained path, so the count reflects the accepted chunk. Buffer draining is what flush()'s return value accounts for.
The fully-drained Done arm of try_write_newly_buffered_data returned the total bytes drained from the buffer (older chunks included), the same over-reporting shape the Wrote arm had. Return only the portion of the drained bytes belonging to the current chunk. The Pending arm returns a promise whose resolution value is accounted for separately.
b97c9a1 to
97b3f3a
Compare
|
CI status for 97b3f3a: 282 test jobs passed,
Neither touched the diff and no test that actually executed failed. The previous build (69314) hit the same artifact-download timeout on darwin aarch64 plus an unrelated This is ready for a maintainer to merge. |
1 participant
Fixes #12194
What
FileSink.write()is documented to return the number of bytes written for the chunk passed to it (bun-types: "@returns Number of bytes written"), andtest/js/bun/util/filesink.test.tsasserts this ("write result is not cumulative"). That test only covers fd-backed sinks with anawaitbetween writes, so it never exercises the buffered -> flush path.When a chunk is appended to a non-empty internal buffer and the combined size crosses the streaming writer's
CHUNK_SIZE(4096), the buffer is drained andwrite()returned the total number of bytes drained (older buffered chunks included) instead of the current chunk's size. Anywritten += sink.write(chunk)accounting loop was then wrong by the amount of previously buffered data.Repro
The data written to disk is byte-perfect; only the returned counts are wrong.
Cause
In
src/io/PipeWriter.rs,try_write_newly_buffered_datais reached after the new chunk has already been appended tooutgoing. On the fully-drained branch it returnedWriteResult::Wrote(amt)whereamtis the bytes drained from the whole buffer, not the bytes of the chunk that triggered the drain.FileSink::to_resultforwards that straight to JS.Fix
try_write_newly_buffered_datanow takes the chunk length and returnsWriteResult::Wrote(chunk_len)on the fully-drained path. Draining of older buffered bytes is whatflush()'s return value accounts for. The partial-write / pending / error paths are unchanged.Verification
New test in
filesink.test.ts("write result is not cumulative when the chunk flushes buffered bytes") covers bytes, latin1, and utf16 chunks that each flush previously buffered data, and asserts the summed returns equal the file size.USE_SYSTEM_BUN=1 bun test-> fails (returns 66041/40010/20001 instead of 65536/40000/20000)bun bd test-> passesfilesink.test.ts,terminal.test.ts,process-stdio.test.ts,spawn-streaming-stdin.test.tspass (otherStreamingWriterconsumers)bun run rust:check-all-> 10 ok, 0 failed