Repository navigation
Conversation
The FileSink's flags() method was ignoring the truncate option, which defaults to true. This caused existing files to not be truncated when writing via file.writer(), so if the new content was shorter than the old content, the tail of the old content would remain. Closes #25968 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
WalkthroughFixes FileSink truncation behavior by conditionally applying the TRUNC flag based on the truncate setting. Previously, the flag combination was fixed and didn't account for the truncate option. Includes regression tests validating the fix for file overwrite scenarios. Changes
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @test/regression/issue/25968.test.ts:
- Around line 30-37: The spawned inline script calls writer.end() without
awaiting, which can let the read occur before the write completes; update the
script so the end is awaited (await writer.end()) after using file.writer() and
before calling file.text(), ensuring the write completes before reading the file
contents.
- Around line 13-15: The test has a race: writer.end() can return a promise and
must be awaited to ensure all buffered data is flushed before reading; update
the test to await the writer.end() call (e.g., await writer.end()) so that
subsequent file.text() reads the fully written contents, and ensure the
surrounding test function is async if not already.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/bun.js/webcore/FileSink.zigtest/regression/issue/25968.test.ts
🧰 Additional context used
📓 Path-based instructions (6)
**/*.test.ts?(x)
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.test.ts?(x): Never usebun testdirectly - always usebun bd testto run tests with debug build changes
For single-file tests, prefer-eflag overtempDir
For multi-file tests, prefertempDirandBun.spawnover single-file tests
UsenormalizeBunSnapshotto normalize snapshot output of tests
Never write tests that check for 'panic', 'uncaught exception', or similar strings in test output
UsetempDirfromharnessto create temporary directories - do not usetmpdirSyncorfs.mkdtempSync
When spawning processes in tests, expect stdout before expecting exit code for more useful error messages on test failure
Do not write flaky tests - do not usesetTimeoutin tests; instead await the condition to be met
Verify tests fail withUSE_SYSTEM_BUN=1 bun test <file>and pass withbun bd test <file>- tests are invalid if they pass with USE_SYSTEM_BUN=1
Test files must end with.test.tsor.test.tsx
Avoid shell commands likefindorgrepin tests - use Bun's Glob and built-in tools instead
Files:
test/regression/issue/25968.test.ts
test/regression/issue/*.test.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Place regression tests for specific GitHub issues in
test/regression/issue/${issueNumber}.test.tswith real issue numbers only
Files:
test/regression/issue/25968.test.ts
test/**/*.test.ts?(x)
📄 CodeRabbit inference engine (CLAUDE.md)
Always use
port: 0in tests - do not hardcode ports or use custom random port number functions
Files:
test/regression/issue/25968.test.ts
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}
📄 CodeRabbit inference engine (test/CLAUDE.md)
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}: Usebun bd test <...test file>to run tests with compiled code changes. Do not usebun testas it will not include your changes.
Usebun:testfor files ending in*.test.{ts,js,jsx,tsx,mjs,cjs}. For test files without .test extension in test/js/node/test/{parallel,sequential}/*.js, usebun bd <file>instead ofbun bd test <file>since they expect exit code 0.
Do not set a timeout on tests. Bun already has timeouts built-in.
Files:
test/regression/issue/25968.test.ts
**/*.zig
📄 CodeRabbit inference engine (CLAUDE.md)
In Zig code, be careful with allocators and use defer for cleanup
Files:
src/bun.js/webcore/FileSink.zig
src/**/*.zig
📄 CodeRabbit inference engine (src/CLAUDE.md)
src/**/*.zig: Use the#prefix for private fields in Zig structs, e.g.,struct { #foo: u32 };
Use Decl literals in Zig, e.g.,const decl: Decl = .{ .binding = 0, .value = 0 };
Place@importstatements at the bottom of the file in Zig (auto formatter will handle positioning)
Never use@import()inline inside functions in Zig; always place imports at the bottom of the file or containing struct
Files:
src/bun.js/webcore/FileSink.zig
🧠 Learnings (16)
📚 Learning: 2025-11-24T18:36:59.706Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/test/v8/v8.test.ts : Add corresponding test cases to test/v8/v8.test.ts using checkSameOutput() function to compare Node.js and Bun output
Applied to files:
test/regression/issue/25968.test.ts
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to **/*.test.ts?(x) : Verify tests fail with `USE_SYSTEM_BUN=1 bun test <file>` and pass with `bun bd test <file>` - tests are invalid if they pass with USE_SYSTEM_BUN=1
Applied to files:
test/regression/issue/25968.test.ts
📚 Learning: 2026-01-05T23:04:01.518Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2026-01-05T23:04:01.518Z
Learning: Applies to test/**/*-fixture.ts : Test files that spawn Bun processes should end in `*-fixture.ts` to identify them as test fixtures rather than tests themselves.
Applied to files:
test/regression/issue/25968.test.ts
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to **/*.test.ts?(x) : For multi-file tests, prefer `tempDir` and `Bun.spawn` over single-file tests
Applied to files:
test/regression/issue/25968.test.ts
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to **/*.test.ts?(x) : Use `normalizeBunSnapshot` to normalize snapshot output of tests
Applied to files:
test/regression/issue/25968.test.ts
📚 Learning: 2025-10-26T01:32:04.844Z
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 24082
File: test/cli/test/coverage.test.ts:60-112
Timestamp: 2025-10-26T01:32:04.844Z
Learning: In the Bun repository test files (test/cli/test/*.test.ts), when spawning Bun CLI commands with Bun.spawnSync for testing, prefer using stdio: ["inherit", "inherit", "inherit"] to inherit stdio streams rather than piping them.
Applied to files:
test/regression/issue/25968.test.ts
📚 Learning: 2026-01-05T23:04:01.518Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2026-01-05T23:04:01.518Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Use `bun:test` for files ending in `*.test.{ts,js,jsx,tsx,mjs,cjs}`. For test files without .test extension in test/js/node/test/{parallel,sequential}/*.js, use `bun bd <file>` instead of `bun bd test <file>` since they expect exit code 0.
Applied to files:
test/regression/issue/25968.test.ts
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to test/regression/issue/*.test.ts : Place regression tests for specific GitHub issues in `test/regression/issue/${issueNumber}.test.ts` with real issue numbers only
Applied to files:
test/regression/issue/25968.test.ts
📚 Learning: 2026-01-05T23:04:01.518Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2026-01-05T23:04:01.518Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Use `bun bd test <...test file>` to run tests with compiled code changes. Do not use `bun test` as it will not include your changes.
Applied to files:
test/regression/issue/25968.test.ts
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to **/*.test.ts?(x) : When spawning processes in tests, expect stdout before expecting exit code for more useful error messages on test failure
Applied to files:
test/regression/issue/25968.test.ts
📚 Learning: 2025-11-06T00:58:23.965Z
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 24417
File: test/js/bun/spawn/spawn.test.ts:903-918
Timestamp: 2025-11-06T00:58:23.965Z
Learning: In Bun test files, `await using` with spawn() is appropriate for long-running processes that need guaranteed cleanup on scope exit or when explicitly testing disposal behavior. For short-lived processes that exit naturally (e.g., console.log scripts), the pattern `const proc = spawn(...); await proc.exited;` is standard and more common, as evidenced by 24 instances vs 4 `await using` instances in test/js/bun/spawn/spawn.test.ts.
Applied to files:
test/regression/issue/25968.test.ts
📚 Learning: 2025-11-08T04:06:33.198Z
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 24491
File: test/js/bun/transpiler/declare-global.test.ts:17-17
Timestamp: 2025-11-08T04:06:33.198Z
Learning: In Bun test files, `await using` with Bun.spawn() is the preferred pattern for spawned processes regardless of whether they are short-lived or long-running. Do not suggest replacing `await using proc = Bun.spawn(...)` with `const proc = Bun.spawn(...); await proc.exited;`.
Applied to files:
test/regression/issue/25968.test.ts
📚 Learning: 2026-01-05T23:04:01.518Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2026-01-05T23:04:01.518Z
Learning: When spawning Bun processes in tests, use `bunExe` and `bunEnv` from `harness` to ensure the same build of Bun is used and debug logging is silenced.
Applied to files:
test/regression/issue/25968.test.ts
📚 Learning: 2026-01-05T23:04:01.518Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2026-01-05T23:04:01.518Z
Learning: Use `-e` flag for single-file tests when spawning Bun processes with `Bun.spawn()`.
Applied to files:
test/regression/issue/25968.test.ts
📚 Learning: 2025-10-16T02:17:35.237Z
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 23710
File: src/analytics.zig:15-21
Timestamp: 2025-10-16T02:17:35.237Z
Learning: In src/analytics.zig and similar files using bun.EnvVar boolean environment variables: the new EnvVar API for boolean flags (e.g., bun.EnvVar.do_not_track.get(), bun.EnvVar.ci.get()) is designed to parse and return boolean values from environment variables, not just check for their presence. This is an intentional design change from the previous presence-based checks using bun.getenvZ().
Applied to files:
src/bun.js/webcore/FileSink.zig
📚 Learning: 2026-01-05T16:32:07.551Z
Learnt from: alii
Repo: oven-sh/bun PR: 25474
File: src/bun.js/event_loop/Sigusr1Handler.zig:0-0
Timestamp: 2026-01-05T16:32:07.551Z
Learning: In Zig codebases (e.g., Bun), treat std.posix.sigaction as returning void and do not perform runtime error handling for its failure. The Zig standard library views sigaction failures as programmer errors (unreachable) because they only occur with invalid signals like SIGKILL/SIGSTOP. Apply this pattern across Zig files that call sigaction (e.g., crash_handler.zig, main.zig, filter_run.zig, process.zig) and ensure failures are not handled as recoverable errors; prefer reaching an explicit unreachable/compile-time assumption when such failures are detected.
Applied to files:
src/bun.js/webcore/FileSink.zig
🧬 Code graph analysis (1)
test/regression/issue/25968.test.ts (1)
test/harness.ts (1)
tempDir(277-284)
🔇 Additional comments (2)
src/bun.js/webcore/FileSink.zig (1)
56-62: LGTM! The fix correctly addresses the truncation issue.The implementation properly builds the flags incrementally, starting with the base flags (
NONBLOCK | CLOEXEC | CREAT | WRONLY) and conditionally addingTRUNCwhenthis.truncateis true. Sincetruncatedefaults totrue(line 52), this ensures existing file content is removed by default when usingfile.writer(), which matches the expected behavior reported in issue #25968.test/regression/issue/25968.test.ts (1)
45-49: LGTM! Good test structure following coding guidelines.The test correctly uses
Promise.allto await all outputs, and the assertion order (checking stdout at line 48 before exit code at line 49) follows the guideline for better error messages on test failure.
| const writer = file.writer(); | ||
| writer.write("Short"); | ||
| writer.end(); |
There was a problem hiding this comment.
Potential race condition: writer.end() should be awaited.
The writer.end() call may not complete synchronously—if there's pending data, it returns a promise. Without awaiting, file.text() on line 18 might read the file before the write is fully flushed, potentially causing flaky tests.
Proposed fix
// Write shorter content using writer
const writer = file.writer();
writer.write("Short");
- writer.end();
+ await writer.end();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const writer = file.writer(); | |
| writer.write("Short"); | |
| writer.end(); | |
| const writer = file.writer(); | |
| writer.write("Short"); | |
| await writer.end(); |
🤖 Prompt for AI Agents
In @test/regression/issue/25968.test.ts around lines 13 - 15, The test has a
race: writer.end() can return a promise and must be awaited to ensure all
buffered data is flushed before reading; update the test to await the
writer.end() call (e.g., await writer.end()) so that subsequent file.text()
reads the fully written contents, and ensure the surrounding test function is
async if not already.
| ` | ||
| const file = Bun.file("${filePath.replace(/\\/g, "\\\\")}"); | ||
| await Bun.write(file, "Long content that is longer"); | ||
| const writer = file.writer(); | ||
| writer.write("Short"); | ||
| writer.end(); | ||
| console.log(await file.text()); | ||
| `, |
There was a problem hiding this comment.
Same issue in spawned script: writer.end() should be awaited.
The inline script also calls writer.end() without awaiting. To ensure the write is complete before reading, add await:
Proposed fix
const file = Bun.file("${filePath.replace(/\\/g, "\\\\")}");
await Bun.write(file, "Long content that is longer");
const writer = file.writer();
writer.write("Short");
-writer.end();
+await writer.end();
console.log(await file.text());📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ` | |
| const file = Bun.file("${filePath.replace(/\\/g, "\\\\")}"); | |
| await Bun.write(file, "Long content that is longer"); | |
| const writer = file.writer(); | |
| writer.write("Short"); | |
| writer.end(); | |
| console.log(await file.text()); | |
| `, | |
| ` | |
| const file = Bun.file("${filePath.replace(/\\/g, "\\\\")}"); | |
| await Bun.write(file, "Long content that is longer"); | |
| const writer = file.writer(); | |
| writer.write("Short"); | |
| await writer.end(); | |
| console.log(await file.text()); | |
| `, |
🤖 Prompt for AI Agents
In @test/regression/issue/25968.test.ts around lines 30 - 37, The spawned inline
script calls writer.end() without awaiting, which can let the read occur before
the write completes; update the script so the end is awaited (await
writer.end()) after using file.writer() and before calling file.text(), ensuring
the write completes before reading the file contents.
|
Closing this PR because it has been inactive for more than 90 days. |
1 participant
Summary
FileSink.Options.flags()method to respect thetruncateoption which defaults totruetruncateis true, theO_TRUNCflag is now included when opening the file, ensuring existing content is removedCloses #25968
Test plan
test/regression/issue/25968.test.tsShortcontentinstead ofShort)🤖 Generated with Claude Code