Conversation
…ng files FileSink.Options has always carried truncate: true, but Options::flags() ignored it and opened path destinations with only O_WRONLY|O_CREAT. Any write that streams through a path-based FileSink — Bun.write(Bun.file(path), s3file) and Bun.file(path).writer() — left stale tail bytes behind when the destination was already larger than the data written. OR O_TRUNC into the open flags when truncate is set. Fd-based sinks are unaffected (they dup the fd and never consult flags), and Windows already truncates these opens via FILE_OVERWRITE_IF, so this brings POSIX to parity. Fixes #31682 Fixes #25968
|
Warning Review limit reached
More reviews will be available in 4 minutes and 29 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughFileSink now defers truncation for path-opened sinks via a new truncate_on_end flag and helper; Blob write paths set that flag for path-based opens on Windows. Tests verify truncation for direct writer use and S3 downloads into larger existing files. ChangesFile Truncation on Write
Possibly related PRs:
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
Updated 4:51 AM PT - Jun 2nd, 2026
❌ @robobun, your commit f9d9123 has 8 failures in
🧪 To try this PR locally: bunx bun-pr 31686That installs a local version of the PR into your bun-31686 --bun |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Re the duplicate flag: #25966 / #25967 are the same idea for the This PR applies the |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — minimal fix that makes the existing truncate: true default actually take effect; the inline nit about tmpdirSync is purely stylistic.
Extended reasoning...
Overview
The source change is 4 lines in src/runtime/webcore/FileSink.rs: Options::flags() now ORs in O_TRUNC when self.truncate is set, instead of ignoring the field with let _ = self;. The truncate field already defaulted to true and is never set to false anywhere in the codebase (verified by grep — every FileSinkOptions construction uses ..Default::default() without overriding it). The remaining diff is two regression tests: a new s3-download-truncate.test.ts exercising the original report via a fake S3 endpoint, and a 10-line case in filesink.test.ts for the direct Bun.file(path).writer() path.
Security risks
None. This adds O_TRUNC to an open that already uses O_WRONLY|O_CREAT — i.e., it only affects files the caller is already overwriting. No auth, crypto, permissions, or untrusted input handling is touched.
Level of scrutiny
Low-to-moderate. The fix is mechanically obvious once you see that truncate: true was the existing default but dead. I verified the PR's claim that fd-based sinks are unaffected: open_for_writing_result for the Fd arm calls dup_with_flags(*fd_, 0) and never consults input_flags, so Bun.stdout.writer(), Bun.file(fd).writer(), and subprocess stdin keep their current semantics. POSIX ignores O_TRUNC on FIFOs/terminals, and Windows already truncates via FILE_OVERWRITE_IF, so the blast radius is exactly the buggy case: path-opened regular files.
Other factors
The PR description is thorough, references two open issues (#31682, #25968) and a prior attempt (#25981) closed only for inactivity, and reports passing the relevant test suites. The new tests fail without the fix and pass with it. The only finding from the bug hunt is a style nit (tmpdirSync vs tempDir in the new S3 test file) which doesn't affect correctness and can be taken or left.
There was a problem hiding this comment.
LGTM — thanks for switching to tempDir.
Extended reasoning...
Overview
The src change is a 4-line edit to Options::flags() in src/runtime/webcore/FileSink.rs: when self.truncate is set (it always is — Default for Options sets truncate: true and no caller overrides it), OR in O_TRUNC to the open flags. Previously the field was dead code and path-opened FileSinks never truncated, leaving stale tail bytes when writing over a larger existing file. Two regression tests are added: a direct Bun.file(path).writer() case in filesink.test.ts and an end-to-end S3→file case with a fake Bun.serve endpoint.
Security risks
None. The change adds O_TRUNC to an open that already had O_WRONLY|O_CREAT — standard write-mode semantics, no privilege/permission surface touched. The new S3 test spins up a localhost server and a child process with proxy env vars stripped; nothing reaches the network.
Level of scrutiny
Low-to-moderate. The src change is mechanically trivial and the truncate field's intent was already encoded in the struct. I verified the PR's blast-radius claim: open_for_writing_result for the Fd arm calls dup_with_flags(*fd_, 0) and ignores input_flags entirely, so Bun.stdout.writer(), Bun.file(fd).writer(), and subprocess stdin are unaffected. POSIX ignores O_TRUNC on FIFOs/ttys. Windows already truncates via FILE_OVERWRITE_IF. This is a behavior change for Bun.file(path).writer(), but it fixes two reported bugs (#25968, #31682), matches Node's createWriteStream default, matches every other Bun.write variant, and three prior PRs (#25966/#25967/#25981) attempted the same fix against the old Zig tree.
Other factors
The PR description reports the full filesink, bun-write, streams, blob-write, and createWriteStream suites pass with the change. My only prior feedback — a style nit about tmpdirSync vs tempDir in the new test file — was addressed in b39192c and the thread is resolved. No CODEOWNERS cover the touched paths. The bug-hunting system found nothing.
On Windows these call sites open via bun_sys::open, which routes through libuv (uv_fs_open) with POSIX open semantics — not the NtCreateFile path whose WRONLY disposition overwrites. Without UV_FS_O_TRUNC the destination kept its stale tail on Windows, same as the POSIX bug fixed in the previous commit; the s3-download-truncate test caught it on the windows-x64 lane.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/js/bun/util/filesink.test.ts`:
- Around line 176-178: The test that creates target using tmpdirSync() should
use the harness-provided tempDir helper instead: import/use tempDir from
'harness' and replace tmpdirSync() with tempDir() when building the target path
in the "truncates an existing larger file when opened by path" test (the it(...)
block that defines target and writes the file); ensure the test follows the
established tempDir cleanup pattern rather than calling
tmpdirSync()/fs.mkdtempSync().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5f6cdbb4-0c8c-4986-bff6-57d5e00163b1
📒 Files selected for processing (4)
src/runtime/webcore/Blob.rssrc/runtime/webcore/FileSink.rstest/js/bun/s3/s3-download-truncate.test.tstest/js/bun/util/filesink.test.ts
There was a problem hiding this comment.
I didn't find any correctness issues and the fix looks right, but this changes default open semantics for Bun.file(path).writer() and the streaming Bun.write paths in core runtime code — worth a maintainer sign-off on the user-visible behavior change (and the late-added Windows Blob.rs opens).
Extended reasoning...
Overview
The PR adds O_TRUNC to three path-open sites so that path-backed FileSinks truncate a pre-existing larger destination file:
src/runtime/webcore/FileSink.rs:Options::flags()now honors the existingtruncate: truedefault (previously dead) by ORing inO_TRUNC. This is the POSIX path forBun.file(path).writer()and the S3→file streaming form ofBun.write.src/runtime/webcore/Blob.rs(two sites, both#[cfg(windows)]): addsO_TRUNCto the directbun_sys::opencalls used by the Windows streaming-write paths that bypassOptions::flags().- Two tests: a new S3 fake-endpoint test and a new
filesink.test.tscase, both verifying stale tail bytes are gone.
The diff is small and the reasoning in the PR description is solid: the truncate field already defaulted to true and was simply never read; every other Bun.write form already truncates; fd-based sinks and FIFO/tty are unaffected. My earlier nit (use tempDir instead of tmpdirSync in the new test file) was addressed in b39192c.
Security risks
None identified. This adds a standard open flag to write-only file opens; it does not touch auth, crypto, permissions, or untrusted input parsing.
Level of scrutiny
Medium. The mechanical change is trivial, but it lives in src/runtime/webcore/ and changes the default semantics of a public API (Bun.file(path).writer()) from "overwrite-in-place without truncation" to "truncate on open." That is almost certainly the intended behavior (it fixes #31682 and #25968, matches Node's createWriteStream default, and matches every other Bun.write variant), but it is still a user-visible behavior change in a core I/O primitive. Two prior community PRs (#25966 / #25967) attempted the same fix against the old Zig tree and were closed without merge, so a maintainer should confirm there's no lingering reason this was held back.
Other factors
- The Windows
Blob.rschanges were added in the most recent commit (b3819f6) and aren't covered by the PR description's "Windows already truncates viaFILE_OVERWRITE_IF" analysis — that analysis applies to theopen_for_writingpath inFileSink::setup, not these directbun_sys::opencalls. They look correct (and harmless if redundant), but a maintainer familiar with the Windowsbun_sys::open→NtCreateFilemapping should glance at them. - Good test coverage: both new tests fail without the src/ change and pass with it; the author also reports the broader filesink/bun-write/streams suites pass.
- No outstanding reviewer comments; CodeRabbit was rate-limited and did not review.
Jarred-Sumner
left a comment
Jarred-Sumner
left a comment
There was a problem hiding this comment.
Use ftruncate and do it at the end, O_TRUNC makes it slower when rewriting large files repeatedly.
…NC at open O_TRUNC frees every block of the destination up front, so rewriting a large file in place deallocates and reallocates the whole extent list each time. Instead, leave the open flags alone and ftruncate once when the sink ends, trimming the destination to exactly the bytes written. The byte count is the fd's current offset: path opens start at offset zero and the writer only writes sequentially (uv_fs_write uses offset -1 on Windows). Bytes still buffered at end() flush sequentially afterwards and re-extend the file, so the final size is unchanged by the ordering. lseek failing (pipe/tty/socket) skips the truncate, matching how O_TRUNC is ignored for those file types. The fd-provided sink paths never set the flag, so user fds are never truncated.
|
Reworked in c9238dd: the open flags are untouched again — path-opened sinks now |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/runtime/webcore/Blob.rs`:
- Around line 1929-1933: The Windows writer path currently forces
sink_mut.truncate_on_end = true for all path-backed writers; update the logic in
Blob.rs so truncate_on_end respects FileSinkOptions.truncate (the option parsed
by Start::from_js_with_tag::<{ streams::StartTag::FileSink }>()), i.e., only set
truncate_on_end when the file sink options indicate truncation and the path is
not a file descriptor (keep the existing PathOrFileDescriptor::Fd check); ensure
the Windows branch reads the same arg0/file sink options used on non-Windows so
Bun.file(path).writer({ truncate: false }) does not truncate on Windows.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d8c9b950-cde8-4c74-b8b1-0e6ec5630fc3
📒 Files selected for processing (2)
src/runtime/webcore/Blob.rssrc/runtime/webcore/FileSink.rs
…he writer
Truncating in the Pending arm of end()/endFromJS raced the in-flight
uv_fs_write on the libuv thread pool on Windows: if the worker's WriteFile
landed between the lseek and the ftruncate, its bytes were cut and nothing
re-extended the file. Move the truncate for the pending case into the
on_write completion callback, which runs once the post-end() drain finishes
(done, no buffered data, Drained/EndOfFile) while the fd is still open; the
synchronous Done/Wrote flush arms keep truncating inline since no write is
outstanding there.
Also read the fd from the writer's current source on both platforms instead
of self.fd on Windows: setup() never updates self.fd, so a sink re-started
with start({path}) would have truncated a stale — possibly user-owned —
descriptor.
|
CI status for merge consideration (build 59857 now complete): 71 checks pass, 3 fail, and none of the red touches this diff.
The FileSink/truncation tests pass everywhere they ran. Diff is green; the darwin red is runner provisioning + a known-flaky node-compat network test. |
|
Closing in favor of #31737 (per maintainer request), which fixes the same bug with This branch's variant — |
2 participants
What does this PR do?
Fixes #31682 —
Bun.write(Bun.file(path), s3file)did not truncate a larger pre-existing destination file: the first N bytes were overwritten but the stale tail survived. Also fixes #25968 —Bun.file(path).writer()had the same problem.Repro
Cause
Path destinations for
FileSink(the sink behindBun.file(path).writer()and the S3→file streaming form ofBun.write) were opened write-only and nothing ever truncated:FileSink.Optionshas always defaultedtruncate: true, but the field was dead on every platform. Every otherBun.writeform (bytes→file, file→file, Response→file) replaces the destination; only path-based FileSinks did not. The same dead field existed in the Zig implementation, so this was never correct — not a regression.Fix
Per review feedback, truncation is deferred to the end of the sink instead of
O_TRUNCat open, so rewriting a large file in place does not free and reallocate every block:FileSinkgets atruncate_on_endflag, set when the sink opened a path (withOptions.truncate, which nothing disables). Fd-provided sinks —Bun.stdout.writer(),Bun.file(fd).writer(), subprocess stdin — never set it, so user fds are never truncated.end()/endFromJS), it runsftruncate(fd, lseek(fd, 0, SEEK_CUR))once: path opens start at offset zero and the writer only writes sequentially (uv_fs_write(…, -1, …)on Windows), so the current offset is exactly the bytes written. Any bytes still buffered at that point flush sequentially afterwards and re-extend the file, leaving the final size exact. The fd's offset is used rather than thewrittencounter because the counter misses synchronous writes (it's whyBun.writeresolves with 0 on this path today).lseekfailing (FIFO/tty/socket) skips the truncate, matching howO_TRUNCis ignored for those file types.end()error arm) skip the truncate and reject as before.Verification
test/js/bun/s3/s3-download-truncate.test.ts(new): fake S3 endpoint viaBun.serve, 4 MiB pre-existing destination, 1 MiB object — asserts the file ends up exactly the object's bytes. The download runs in a child process withHTTP(S)_PROXYunset because the S3 client resolves the proxy from the environment at startup. (This test caught that Windows had the same bug — its direct opens route through libuv with POSIX open semantics.)test/js/bun/util/filesink.test.ts(new case):Bun.file(path).writer()over a larger existing file leaves only the written bytes.Both fail without the
src/change (stale bytes survive) and pass with it. Fullfilesink.test.ts(43),bun-write.test.js(35),streams.test.js(70),blob-write.test.ts,fs.test.ts -t createWriteStream,spawn-streaming-stdinandspawn-stdin-readable-streamsuites pass with the change on Linux;cargo checkpasses forx86_64-unknown-linux-gnuandx86_64-pc-windows-msvc.