fix(io): Prevent hang in sliced BunFile stream on large files

[mattermoran]

Fixes #18192
Fixes #21175

Properly closes the stream when max_size is reached.

Previously just checking buf.len == 0 was not enough as that could trigger another read but then it would hit the this.total_readed >= max_size guard and return without closing the stream leading to a hang.

Rather than checking buf.len == 0 it's better to this.total_readed >= max_size to check if we already at the end.

The first if was also modified to handle edge case with 0 length slice as that would hang as well.

[coderabbitai]

Walkthrough

Adjusts FileReader chunk-read control flow to set closure flags when a max_size is reached and ensures closure checks run after consuming a buffer. Adds two regression tests exercising streamed slices of Bun.file() to reproduce/verify fixes for hanging/freeze scenarios.

Changes

Cohort / File(s)	Summary
FileReader closure semantics fix src/bun.js/webcore/FileReader.zig	In onReadChunk, propagate closure semantics earlier by setting close and hasMore when max_size is reached and change post-consumption checks to use total_readed >= max_size, ensuring finalization runs even if buf isn't fully consumed.
Regression tests for sliced file streaming test/regression/issue/18192.test.ts, test/regression/issue/21175.test.ts	Add tests creating 1 MiB files and verifying Bun.file().slice(...).stream() does not hang: tiny, zero-length, middle-slice scenarios, and async-iterator consumption for a 16,384-byte slice.

Suggested reviewers

• cirospaciari

Pre-merge checks

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and specifically describes the main change: preventing a hang in sliced BunFile stream operations on large files, which directly matches the core fix in FileReader.zig.
Description check	✅ Passed	The description adequately explains the fix, root cause, and how the code was modified, addressing both linked issues. It follows the template structure implicitly with what-and-how information.
Linked Issues check	✅ Passed	The code changes directly fix the issues by modifying stream closure logic in FileReader.zig to detect end-of-slice via total_readed >= max_size, and regression tests verify both issue scenarios work without hanging.
Out of Scope Changes check	✅ Passed	All changes are scoped to the linked issues: FileReader.zig fixes the stream hang logic, and two regression tests validate the specific scenarios from issues #18192 and #21175.

---

📜 Recent 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.

📥 Commits

Reviewing files that changed from the base of the PR and between 5f093416613ed735d408cb2fe973701ca670f1eb and 05fd100.

📒 Files selected for processing (3)

• src/bun.js/webcore/FileReader.zig
• test/regression/issue/18192.test.ts
• test/regression/issue/21175.test.ts

🧰 Additional context used

📓 Path-based instructions (7)

src/**/*.zig

📄 CodeRabbit inference engine (src/CLAUDE.md)

src/**/*.zig: Private fields in Zig are fully supported using the # prefix: struct { #foo: u32 };
Use decl literals in Zig for declaration initialization: const decl: Decl = .{ .binding = 0, .value = 0 };
Prefer @import at the bottom of the file (auto formatter will move them automatically)

Files:

• src/bun.js/webcore/FileReader.zig

**/*.zig

📄 CodeRabbit inference engine (CLAUDE.md)

In Zig code, be careful with allocators and use defer for cleanup

Files:

• src/bun.js/webcore/FileReader.zig

test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}

📄 CodeRabbit inference engine (test/CLAUDE.md)

test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}: Use bun:test with files that end in *.test.{ts,js,jsx,tsx,mjs,cjs}
Do not write flaky tests. Never wait for time to pass in tests; always wait for the condition to be met instead of using an arbitrary amount of time
Never use hardcoded port numbers in tests. Always use port: 0 to get a random port
Prefer concurrent tests over sequential tests using test.concurrent or describe.concurrent when multiple tests spawn processes or write files, unless it's very difficult to make them concurrent
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
Use -e flag for single-file tests when spawning Bun processes
Use tempDir() from harness to create temporary directories with files for multi-file tests instead of creating files manually
Prefer async/await over callbacks in tests
When callbacks must be used and it's just a single callback, use Promise.withResolvers to create a promise that can be resolved or rejected from a callback
Do not set a timeout on tests. Bun already has timeouts
Use Buffer.alloc(count, fill).toString() instead of 'A'.repeat(count) to create repetitive strings in tests, as ''.repeat is very slow in debug JavaScriptCore builds
Use describe blocks for grouping related tests
Always use await using or using to ensure proper resource cleanup in tests for APIs like Bun.listen, Bun.connect, Bun.spawn, Bun.serve, etc
Always check exit codes and test error scenarios in error tests
Use describe.each() for parameterized tests
Use toMatchSnapshot() for snapshot testing
Use beforeAll(), afterEach(), beforeEach() for setup/teardown in tests
Track resources (servers, clients) in arrays for cleanup in afterEach()

Files:

• test/regression/issue/18192.test.ts
• test/regression/issue/21175.test.ts

test/regression/issue/**/*.test.ts

📄 CodeRabbit inference engine (test/CLAUDE.md)

Regression tests for specific issues go in /test/regression/issue/${issueNumber}.test.ts. Do not put tests without issue numbers in the regression directory

Files:

• test/regression/issue/18192.test.ts
• test/regression/issue/21175.test.ts

**/*.test.ts?(x)

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.test.ts?(x): Never use bun test directly - always use bun bd test to run tests with debug build changes
For single-file tests, prefer -e flag over tempDir
For multi-file tests, prefer tempDir and Bun.spawn over single-file tests
Use normalizeBunSnapshot to normalize snapshot output of tests
Never write tests that check for 'panic', 'uncaught exception', or similar strings in test output
Use tempDir from harness to create temporary directories - do not use tmpdirSync or fs.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 use setTimeout in tests; instead await the condition to be met
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
Test files must end with .test.ts or .test.tsx
Avoid shell commands like find or grep in tests - use Bun's Glob and built-in tools instead

Files:

• test/regression/issue/18192.test.ts
• test/regression/issue/21175.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.ts with real issue numbers only

Files:

• test/regression/issue/18192.test.ts
• test/regression/issue/21175.test.ts

test/**/*.test.ts?(x)

📄 CodeRabbit inference engine (CLAUDE.md)

Always use port: 0 in tests - do not hardcode ports or use custom random port number functions

Files:

• test/regression/issue/18192.test.ts
• test/regression/issue/21175.test.ts

🧠 Learnings (20)

📓 Common learnings

Learnt from: RiskyMH
Repo: oven-sh/bun PR: 22606
File: src/glob/GlobWalker.zig:449-452
Timestamp: 2025-09-12T18:16:50.754Z
Learning: For Bun codebase: prefer using `std.fs.path.sep` over manual platform separator detection, and use `bun.strings.lastIndexOfChar` instead of `std.mem.lastIndexOfScalar` for string operations.

📚 Learning: 2025-10-17T20:50:58.644Z

Learnt from: taylordotfish
Repo: oven-sh/bun PR: 23755
File: src/bun.js/api/bun/socket/Handlers.zig:154-159
Timestamp: 2025-10-17T20:50:58.644Z
Learning: In Bun socket configuration error messages (src/bun.js/api/bun/socket/Handlers.zig), use the user-facing JavaScript names "data" and "drain" instead of internal field names "onData" and "onWritable", as these are the names users see in the API according to SocketConfig.bindv2.ts.

Applied to files:

• src/bun.js/webcore/FileReader.zig

📚 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/18192.test.ts
• test/regression/issue/21175.test.ts

📚 Learning: 2025-11-24T18:37:30.259Z

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:30.259Z
Learning: Applies to test/regression/issue/**/*.test.ts : Regression tests for specific issues go in `/test/regression/issue/${issueNumber}.test.ts`. Do not put tests without issue numbers in the regression directory

Applied to files:

• test/regression/issue/18192.test.ts
• test/regression/issue/21175.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/18192.test.ts
• test/regression/issue/21175.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/18192.test.ts
• test/regression/issue/21175.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/18192.test.ts
• test/regression/issue/21175.test.ts

📚 Learning: 2025-11-24T18:37:30.259Z

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:30.259Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Use `bun:test` with files that end in `*.test.{ts,js,jsx,tsx,mjs,cjs}`

Applied to files:

• test/regression/issue/18192.test.ts
• test/regression/issue/21175.test.ts

📚 Learning: 2025-11-24T18:37:30.259Z

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:30.259Z
Learning: Applies to test/**/*-fixture.ts : Test files that spawn Bun processes should end in `*-fixture.ts` to identify them as test fixtures and not tests themselves

Applied to files:

• test/regression/issue/18192.test.ts
• test/regression/issue/21175.test.ts

📚 Learning: 2025-11-24T18:37:30.259Z

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:30.259Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Use `Buffer.alloc(count, fill).toString()` instead of `'A'.repeat(count)` to create repetitive strings in tests, as ''.repeat is very slow in debug JavaScriptCore builds

Applied to files:

• test/regression/issue/18192.test.ts
• test/regression/issue/21175.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/18192.test.ts
• test/regression/issue/21175.test.ts

📚 Learning: 2025-11-24T18:37:30.259Z

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:30.259Z
Learning: Applies to test/js/node/test/{parallel,sequential}/*.js : For test/js/node/test/{parallel,sequential}/*.js files without a .test extension, use `bun bd <file>` instead of `bun bd test <file>` since these expect exit code 0 and don't use bun's test runner

Applied to files:

• test/regression/issue/18192.test.ts
• test/regression/issue/21175.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/18192.test.ts
• test/regression/issue/21175.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/18192.test.ts
• test/regression/issue/21175.test.ts

📚 Learning: 2025-11-24T18:37:30.259Z

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:30.259Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Always use `await using` or `using` to ensure proper resource cleanup in tests for APIs like Bun.listen, Bun.connect, Bun.spawn, Bun.serve, etc

Applied to files:

• test/regression/issue/18192.test.ts
• test/regression/issue/21175.test.ts

📚 Learning: 2025-11-24T18:37:30.259Z

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:30.259Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : 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/18192.test.ts
• test/regression/issue/21175.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/18192.test.ts
• test/regression/issue/21175.test.ts

📚 Learning: 2025-10-19T02:44:46.354Z

Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: packages/bun-otel/context-propagation.test.ts:1-1
Timestamp: 2025-10-19T02:44:46.354Z
Learning: In the Bun repository, standalone packages under packages/ (e.g., bun-vscode, bun-inspector-protocol, bun-plugin-yaml, bun-plugin-svelte, bun-debug-adapter-protocol, bun-otel) co-locate their tests with package source code using *.test.ts files. This follows standard npm/monorepo patterns. The test/ directory hierarchy (test/js/bun/, test/cli/, test/js/node/) is reserved for testing Bun's core runtime APIs and built-in functionality, not standalone packages.

Applied to files:

• test/regression/issue/18192.test.ts

📚 Learning: 2025-10-01T22:13:08.081Z

Learnt from: taylordotfish
Repo: oven-sh/bun PR: 23169
File: src/codegen/bindgenv2/internal/base.ts:120-125
Timestamp: 2025-10-01T22:13:08.081Z
Learning: Iterator.some() and other Iterator helper methods (map, filter, etc.) are supported in Bun and modern Node.js (22+) runtimes and can be safely used on string iterators and other iterator objects.

Applied to files:

• test/regression/issue/21175.test.ts

📚 Learning: 2025-10-04T21:17:53.040Z

Learnt from: markovejnovic
Repo: oven-sh/bun PR: 23253
File: test/js/valkey/valkey.failing-subscriber-no-ipc.ts:40-59
Timestamp: 2025-10-04T21:17:53.040Z
Learning: In Bun runtime, the global `console` object is an AsyncIterable that yields lines from stdin. The pattern `for await (const line of console)` is valid and documented in Bun for reading input line-by-line from the child process stdin.

Applied to files:

• test/regression/issue/21175.test.ts

🧬 Code graph analysis (1)

test/regression/issue/21175.test.ts (2)

test/js/bun/sqlite/sqlite.test.js (1)

• tmpbase (9-9)

test/harness.ts (1)

• tempDir (277-284)

🔇 Additional comments (4)

test/regression/issue/21175.test.ts (1)

1-24: LGTM! Well-structured regression test.

The test correctly reproduces the issue from #21175 where streaming a sliced file via async iterator would hang. The test structure follows all coding guidelines: uses bun:test, tempDir() with using for cleanup, and properly validates the expected behavior.

test/regression/issue/18192.test.ts (1)

1-41: LGTM! Comprehensive regression test with excellent edge case coverage.

The test effectively validates the fix for issue #18192 by testing three critical scenarios: a tiny 1-byte slice, a zero-length slice (important edge case mentioned in the PR description), and a slice from the middle of the file. All scenarios use a 1MB file to ensure the bug is triggered (issue appeared around 640KB threshold). The test properly validates both length and content.

src/bun.js/webcore/FileReader.zig (2)

323-327: LGTM! Defensive early exit prevents reading beyond slice boundary.

This early check handles the case where total_readed has already reached max_size before processing the current buffer. Importantly, this also handles the zero-length slice edge case (when max_size = 0), preventing the hang described in the issues.

---

334-337: LGTM! Key fix for the hanging issue.

The condition change from checking buf.len == 0 to this.total_readed >= max_size is the core fix. The previous logic would wait for an empty buffer to detect end-of-stream, which could trigger another read that would hit the guard at line 323 and return without closing the stream, causing a hang. Now the code correctly detects when the slice's max_size has been reached and properly closes the stream.

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

[mattermoran]

found a similar issue that I will address as well first #21175

[mattermoran]

confirmed this fixes #21175 as well

[robobun]

Closing as stale: this PR predates the Rust rewrite. Every src/ file it modifies has since been removed or relocated on main (Zig sources deleted; src/bun.js/ reorganized into src/jsc/), so it can no longer merge.

If the underlying change is still wanted, it will need to be redone against the current Rust/C++ tree. Apologies for the churn, and thank you for the contribution.