FileSink: mark sink done when end() fails during flush by robobun · Pull Request #31135 · oven-sh/bun

robobun · 2026-05-20T16:04:15Z

When FileSink::end()/end_from_js() hit a write error while flushing buffered data, they closed the writer's handle via writer.close() but left both FileSink.done and writer.is_done unset. A subsequent write()/flush() (including the deferred auto-flusher microtask, or after start({}) reset done) would then attempt to write to Fd::INVALID, tripping a debug assertion (fd != Fd::INVALID in sys::Error::with_fd, or the rustix fd validity assert depending on the write path taken).

Fix

In the WriteResult::Err arm of FileSink::end and FileSink::end_from_js, set self.done = true and call writer.end() (which sets is_done before closing) instead of writer.close(). This matches the other arms which all leave the writer in a terminal state.
Defensively, PosixPipeWriter::try_write_with_write_fn now returns Done(0) when the handle has no fd instead of issuing a syscall on fd -1.

Repro

const w = Bun.file(path).writer();
w.start({ fd: readOnlyFd }); // dup a read-only fd into the writer
w.write("x");
try { await w.end(); } catch {} // flush -> write() fails -> handle closed
w.write("y");                  // previously: writes to Fd::INVALID
w.flush();                      // debug assertion

Found by Fuzzilli.

When FileSink::end()/end_from_js() hit a write error while flushing, they closed the writer's handle via writer.close() but left both FileSink.done and writer.is_done false. A subsequent write()/flush() (including the deferred auto-flusher) would then call sys::write with Fd::INVALID, tripping a debug assertion. Use writer.end() instead of writer.close() in the error arm so writer.is_done is set, and set FileSink.done as well. Also guard try_write_with_write_fn against an invalid fd so any other path that reaches it with a closed handle returns Done(0) instead of issuing a syscall on fd -1.

coderabbitai · 2026-05-20T16:07:13Z

Walkthrough

This PR modifies error handling in the file sink writer. FileSink now marks itself done and transitions the writer via end() instead of closing it when flush() fails. A defensive guard in PipeWriter prevents writing to invalid file descriptors. A new test validates that the sink gracefully handles failed end() operations without crashing.

Changes

FileSink writer error handling and defensive guards

Layer / File(s)	Summary
FileSink error path refactoring `src/runtime/webcore/FileSink.rs`	`FileSink::end()` and `FileSink::end_from_js()` now set `done=true` and call `writer.end()` instead of `writer.close()` when handling `WriteResult::Err` from flush operations.
PipeWriter invalid FD guard `src/io/PipeWriter.rs`	`PosixPipeWriter::try_write_with_write_fn` short-circuits and returns `WriteResult::Done(0)` when `get_fd()` returns `Fd::INVALID`.
Error scenario test coverage `test/js/bun/util/filesink.test.ts`	New POSIX-only test reproduces a failed `writer.end()` during flush and verifies that subsequent `write()`, `start()`, `flush()`, and `end()` calls do not crash.

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely summarizes the main change: marking FileSink done when end() fails during flush, which directly addresses the core bug fix.
Description check	✅ Passed	The description includes both required sections (What does this PR do and How did you verify your code works) with comprehensive explanations, code examples, and context about the issue.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

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

coderabbitai

Actionable comments posted: 2

🤖 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`:
- Line 283: Replace the use of tmpdirSync() assigned to the variable dir with
the test harness tempDir fixture: import { tempDir } from 'harness' (if not
already imported) and replace const dir = tmpdirSync(); with const dir = await
tempDir(); (or const dir = tempDir(); depending on the fixture API) so the test
uses the harness-managed temporary directory and automatic cleanup instead of
fs/tmpdirSync. Ensure any code that expects a string path still works with the
fixture return value.
- Around line 304-308: The test currently uses a synchronous assertion for
writer.flush() which only catches sync throws; change it to await the promise
and assert it resolves (e.g., use await
expect(writer.flush()).resolves.toBeUndefined() or await writer.flush() inside
try/catch) so async rejections fail the test, and similarly replace the
Promise.resolve(writer.end()).catch(() => {}) pattern with an awaited assertion
or try/catch (await writer.end() or await
expect(writer.end()).resolves.toBeUndefined()) to surface async errors from
writer.end().

🪄 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: 163542fc-71c5-4537-8856-b22fec0c323d

📥 Commits

Reviewing files that changed from the base of the PR and between a43a01b and 917756a.

📒 Files selected for processing (3)

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

coderabbitai · 2026-05-20T16:07:16Z

 });
+
+it.skipIf(!isPosix)("writing after end() fails during flush does not crash", async () => {
+  const dir = tmpdirSync();


⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Use tempDir fixture instead of tmpdirSync in this new test.

This new test introduces tmpdirSync(), which is against the repo test harness rule and weakens cleanup guarantees.

Suggested change

- const dir = tmpdirSync(); - const target = join(dir, "ro.txt"); + using dir = tempDir("filesink-end-fail-flush", {}); + const target = join(dir, "ro.txt");

As per coding guidelines: "**/*.test.{ts,tsx}: Use tempDir from 'harness' to create temporary directories - do not use tmpdirSync or fs.mkdtempSync."

📝 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.

Suggested change

const dir = tmpdirSync();

using dir = tempDir("filesink-end-fail-flush", {});

const target = join(dir, "ro.txt");

🤖 Prompt for 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. In `@test/js/bun/util/filesink.test.ts` at line 283, Replace the use of tmpdirSync() assigned to the variable dir with the test harness tempDir fixture: import { tempDir } from 'harness' (if not already imported) and replace const dir = tmpdirSync(); with const dir = await tempDir(); (or const dir = tempDir(); depending on the fixture API) so the test uses the harness-managed temporary directory and automatic cleanup instead of fs/tmpdirSync. Ensure any code that expects a string path still works with the fixture return value.

coderabbitai · 2026-05-20T16:07:16Z

+  expect(() => writer.write("y")).not.toThrow();
+  expect(() => writer.start({})).not.toThrow();
+  expect(() => writer.write("z")).not.toThrow();
+  expect(() => writer.flush()).not.toThrow();
+  await Promise.resolve(writer.end()).catch(() => {});


⚠️ Potential issue | 🟠 Major | ⚡ Quick win

flush() assertion is sync-only and can miss async failures.

expect(() => writer.flush()).not.toThrow() only validates synchronous throw. If flush() rejects asynchronously, this test can produce false positives or unhandled rejection noise.

Suggested change

- expect(() => writer.flush()).not.toThrow(); - await Promise.resolve(writer.end()).catch(() => {}); + await expect(writer.flush()).resolves.toBeUndefined(); + await Promise.resolve(writer.end()).catch(() => {});

🤖 Prompt for 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. In `@test/js/bun/util/filesink.test.ts` around lines 304 - 308, The test currently uses a synchronous assertion for writer.flush() which only catches sync throws; change it to await the promise and assert it resolves (e.g., use await expect(writer.flush()).resolves.toBeUndefined() or await writer.flush() inside try/catch) so async rejections fail the test, and similarly replace the Promise.resolve(writer.end()).catch(() => {}) pattern with an awaited assertion or try/catch (await writer.end() or await expect(writer.end()).resolves.toBeUndefined()) to surface async errors from writer.end().

github-actions · 2026-05-20T16:07:27Z

Found 2 issues this PR may fix:

await writer.end() crashes with ENOSPC instead of throwing a catchable error #24032 - await writer.end() crashes with ENOSPC instead of throwing a catchable error — this PR fixes the exact crash path where end() hits a write error during flush and leaves the sink in an inconsistent state
The app crashes when writing chunks because there isn't enough disk space #25706 - App crashes when writing chunks due to insufficient disk space — same root cause: write errors during flush crash the process instead of being handled gracefully

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

Fixes #24032
Fixes #25706

🤖 Generated with Claude Code

robobun · 2026-05-20T16:08:30Z

^{Updated 10:32 AM PT - May 20th, 2026}

✅ @robobun, your commit 917756aa88a77632b67242398102e646e632193e passed in Build #56418! 🎉

🧪 To try this PR locally:

bunx bun-pr 31135

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

bun-31135 --bun

claude

LGTM — focused fix aligning the error arm with the other terminal arms, plus a defensive fd guard and regression test.

Extended reasoning...

Overview

This PR fixes a fuzzer-found debug assertion where FileSink::end()/end_from_js() hitting a write error during flush would call writer.close() without setting self.done or writer.is_done. A subsequent write()/flush() (or start({}) resetting done) would then attempt I/O on Fd::INVALID. The fix is two lines per error arm (set done = true, call writer.end() instead of writer.close()), a 3-line defensive Fd::INVALID check in PosixPipeWriter::try_write_with_write_fn, and a regression test.

Security risks

None. This is a state-machine consistency fix in the file-sink error path; no auth, parsing, or trust-boundary changes. The defensive guard returns Done(0) rather than issuing a syscall on fd -1, which is strictly safer.

Level of scrutiny

Low-to-medium. The change is mechanical: writer.end() is exactly is_done = true; close() (PipeWriter.rs:1070-1077), so the only behavioral delta vs. the old close() call is that is_done gets set — which is precisely what the other WriteResult arms (Done/Wrote) already do via writer.end(). Setting self.done = true mirrors the Pending arm. The on_close re-entry path is unchanged since end() still calls close().

Other factors

The defensive Fd::INVALID → Done(0) guard matches the existing is_done || closed_without_reporting → Done(0) early-returns in write/flush/write_latin1/write_utf16, so it's consistent with established semantics. The new POSIX-only test exercises the exact repro (read-only fd → flush error → write/start/flush after) and verifies no crash. No outstanding reviewer comments; no prior reviews on the timeline.

github-actions Bot added the claude label May 20, 2026

coderabbitai Bot reviewed May 20, 2026

View reviewed changes

claude Bot reviewed May 20, 2026

View reviewed changes

Jarred-Sumner merged commit 3eb0fda into main May 20, 2026
79 checks passed

Jarred-Sumner deleted the farm/75717719/filesink-write-after-end-error branch May 20, 2026 23:56

coderabbitai Bot mentioned this pull request May 21, 2026

sys: drop with_fd debug assertion on Fd::INVALID #31165

Open

claude Bot mentioned this pull request May 23, 2026

fix(bun:test): don't assert when formatting a Set/Map with a non-numeric size property #31292

Closed

This was referenced Jun 2, 2026

Pipe S3 downloads into local files natively #31683

Open

Truncate existing destination files in path-opened FileSinks #31686

Closed

This was referenced Jun 25, 2026

Remove stale signal.is_dead() assertion in pipe_readable_stream_to_blob #32657

Merged

FileSink: return 0 from write() and undefined from flush() after end() #32857

Open

coderabbitai Bot mentioned this pull request Jul 7, 2026

s3: S3File.writer().end(error) aborts the upload instead of committing #33681

Closed

Navigation Menu

FileSink: mark sink done when end() fails during flush - #31135

FileSink: mark sink done when end() fails during flush#31135
Jarred-Sumner merged 1 commit into
mainfrom
farm/75717719/filesink-write-after-end-error

robobun commented May 20, 2026

Uh oh!

coderabbitai Bot commented May 20, 2026

Uh oh!

coderabbitai Bot left a comment

Uh oh!

coderabbitai Bot May 20, 2026

Uh oh!

coderabbitai Bot May 20, 2026

Uh oh!

github-actions Bot commented May 20, 2026

Uh oh!

robobun commented May 20, 2026 •

edited

Loading

Uh oh!

claude Bot left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	const dir = tmpdirSync();
	using dir = tempDir("filesink-end-fail-flush", {});
	const target = join(dir, "ro.txt");

Conversation

robobun commented May 20, 2026

Fix

Repro

Uh oh!

coderabbitai Bot commented May 20, 2026

Walkthrough

Changes

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot May 20, 2026

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot May 20, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented May 20, 2026

Uh oh!

robobun commented May 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

claude Bot left a comment

Choose a reason for hiding this comment

Overview

Security risks

Level of scrutiny

Other factors

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

robobun commented May 20, 2026 •

edited

Loading