Skip to content

node:fs: run appendFile through writeFile's write path and report the { flush: true } fsync error - #36485

Open
robobun wants to merge 1 commit into
mainfrom
farm/64e693d7/appendfile-flush
Open

robobun wants to merge 1 commit into
mainfrom
farm/64e693d7/appendfile-flush

Conversation

@robobun

@robobun robobun commented Jul 30, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

  • With { flush: true }, fs.appendFile never calls fsync(2). fs.writeFile calls it and drops the result: a failed sync (EIO) reports success. Node reports { code: 'EIO', syscall: 'fsync' }.
  • Cause: NodeFS::append_file (src/runtime/node/node_fs.rs:4700) had its own write loop and never read flush. write_file_with_path_buffer (:7490) did let _ = Syscall::fsync(fd).

Fix

  • append_file opens the file, then runs write_file_to_fd: the part of writeFile after the open. It reads flush and returns the fsync error.
  • fs.promises does not request the sync on a FileHandle (withoutFlush). Its iterable path validates flush and syncs.
  • Correct: on Linux, all 14 call forms match node v26.3.0 in fsync count and result. On main, 12 differ.
  • Verified: test/js/node/fs/fs.test.ts, the iterator and seccomp tests beside it. 15 tests fail on main. Self-reviewed: 20 concerns raised, 19 addressed (Notes).

Background

Downsides

  • Needs a maintainer's yes: handle.appendFile() and fsPromises.writeFile(handle) with flush stop syncing. Bun has synced them since v1.2.0, node never. Use await handle.sync().
  • Needs a maintainer's yes: a sync write of a string with explicit encoding: 'utf8' syncs. Node's fast path skips that fsync (fs: honor flush option in writeFileSync utf8 fast path nodejs/node#63887).
  • flush unset: 0 added syscalls, appendFileSync(path) +15 instructions, writeFileSync(path) -1. .text +256 bytes, builtin JS +375 bytes.
Notes

Origin. Found by an audit of { flush: true } against node v26.3.0. There is no user report. The suite could not see the defect: the upstream "performs flush" cases are commented out in test/js/node/test/parallel/test-fs-append-file-flush.js and test-fs-write-file-flush.js, because they spy on fs.fsyncSync and Bun syncs in native code.

The 14 call forms. fsync(2) calls / result when fsync fails with EIO. An LD_PRELOAD shim counts the calls and injects the error. Linux x64, release builds.

form, with { flush: true } node v26.3.0 main 13a98b0 this PR
writeFileSync(path) 1 / EIO 1 / ok 1 / EIO
writeFileSync(fd) 1 / EIO 1 / ok 1 / EIO
appendFileSync(path) 1 / EIO 0 / ok 1 / EIO
appendFileSync(fd) 1 / EIO 0 / ok 1 / EIO
writeFile(path, cb) 1 / EIO 1 / ok 1 / EIO
writeFile(fd, cb) 1 / EIO 1 / ok 1 / EIO
appendFile(path, cb) 1 / EIO 0 / ok 1 / EIO
appendFile(fd, cb) 1 / EIO 0 / ok 1 / EIO
fsPromises.writeFile(path) 1 / EIO 1 / ok 1 / EIO
fsPromises.appendFile(path) 1 / EIO 0 / ok 1 / EIO
fsPromises.writeFile(handle) 0 / ok 1 / ok 0 / ok
fsPromises.appendFile(handle) 0 / ok 0 / ok 0 / ok
handle.writeFile() 0 / ok 0 / ok 0 / ok
handle.appendFile() 0 / ok 1 / ok 0 / ok

Measurements. Main 13a98b0 against this change on the same commit, release builds from one toolchain, Linux x64.

  • fsync(2) count and result against node v26.3.0 under the fsync shim, with and without EIO: 0 of 14 rows differ (main: 12).
  • /dev/null, no shim: 14 of 14 rows equal node (10 x EINVAL from fsync, 4 x ok).
  • fsPromises.writeFile(handle) / handle.appendFile() with { flush: true }: 1 to 0 fsync(2) per call.
  • appendFile syscalls with flush unset: +0 in each form (path 1 byte, path 3 MiB, fd 'a' 3 MiB, fd 'w' 3 MiB, path with { flag: 'w' }). writeFile forms: +0 (gdb catch syscall).
  • appendFileSync(path,'x'), flush unset: +15 instructions, +5 conditional branches per call (1358 to 1373). appendFileSync(fd,'x'): +13 / +4 (674 to 687). writeFileSync(path,'x'): -1 / 0 (1430 to 1429). Counted with gdb single-step from the entry of the host function to its return. write_file_to_fd is #[inline(always)]: without it writeFileSync pays +14 for the call.
  • fsPromises.writeFile / appendFile(path), flush unset: +0 bytecodes on the executed path, +0 calls, +0 objects. FileHandle target with an options argument: +1 read of options.flush, +1 call (withoutFlush), +0 objects. With flush: true on a FileHandle: 1 object and 4 more option reads.
  • Release binary .text: +256 bytes. fs.promises.js: +375 bytes. Host functions: +0 (size, wc, nm).
  • Windows: bun run rust:check-all passes for all 12 targets. Nothing ran on Windows or macOS: no such machine here. CI has to show the Windows arm. On Linux, two processes x 10,000 appended 64-byte lines lose 0 lines on main and 0 with this change.
  • fs.appendFile(p, 'x', { signal }, cb); ac.abort() x 1000, calls that still wrote: 1000 on main, 692 to 801 with this change. fs.writeFile on main: 753. Node: 0. This is a race between the abort and the job, so it is a count and not a test.

Two rulings for a maintainer.

  1. FileHandle targets stop syncing. Bun has issued this fsync for fsPromises.writeFile(handle) and handle.appendFile() since v1.2.0 (node fs compat pr #2 #16422), and dropped its error. Node returns for a FileHandle before its flush step, and docs: update options in filehandle.appendFile() nodejs/node#56972 removed flush from the documentation of filehandle.appendFile(). With the error reported, the other choice is that these forms reject on a handle that cannot sync (/dev/null, a pipe), where node resolves. Code that needs the sync calls await handle.sync(). This needs a line in the release notes.
  2. Node's sync fast path is not ported. In node v26.3.0, writeFileSync / appendFileSync with string data and an explicit encoding: 'utf8' return before the fsync. fs: honor flush option in writeFileSync utf8 fast path nodejs/node#63887 (open) makes that path sync. Bun syncs for each encoding. Main already issues this fsync in writeFileSync. New here: its error is reported, and appendFileSync issues it too. Example that now throws: fs.writeFileSync(1, out, { encoding: 'utf8', flush: true }) when fd 1 is a pipe or a tty.

Also decided here: in fs.promises only a FileHandle is exempt. A raw numeric fd is a Bun extension (node rejects a number there). It syncs and reports, as the callback form does.

What else changes.

  • appendFile (callback and promise) checks the signal after its open, as writeFile does. Before, it wrote the data and then reported AbortError.
  • The sync forms never hold signal. fs.writeFileSync(path, data, { signal: AbortSignal.abort() }) and the readFileSync form threw EINTR: interrupted system call, access. writeFileSync also left an empty file. Node's sync forms never read options.signal.
  • appendFile stops on a zero-length write.
  • FileHandle#writeFile validates flush: { flush: 'yes' } rejects with ERR_INVALID_ARG_TYPE, as in node.
  • Iterable data in fsPromises.writeFile: a Buffer or URL path is opened with the flag. Before, Bun.file(path) wrote it in place: "seed seed seed" overwritten with ["a", "b"] read "abed seed seed". The resize after the write is removed. Its byte count was too high for many chunks: 500 chunks of 10 bytes gave a file of 9090 bytes. Each truncating flag already truncates in the open. A falsy value that the iterable throws (throw null) now rejects.
  • Not changed: with flush unset, appendFile makes the same syscalls as on main in each form. The shared body skips the preallocation and the resize for appendFile, and on Windows it writes through libuv as before.

Not done here.

Related PRs.

Self-review. 20 concerns, 19 addressed. The largest: an fs.fsync cleanup that turned fs-stat-seccomp-linux.test.ts red is removed. appendFile keeps its syscalls and its Windows write call. The options copy for a FileHandle reads each option from the caller's object, so a getter keeps its receiver. The abort check stays after the open. Each divergence from node has a link in the code and a test. Not done: a Windows-only test on a descriptor that is open for reading. No Windows machine is available to find its error code. The test with a descriptor that is not open runs on Windows.

Tests.

  • test/js/node/fs/fs.test.ts: the 14 forms on /dev/null and /dev/full (Linux: fsync fails with EINVAL on the first, the write fails with ENOSPC on the second, so no shim and no compiler), a descriptor that is not open (each platform), the 14 forms on a temp file (each platform), empty data and an explicit flag, explicit utf8, a raw fd in fs.promises, no descriptor leak over 100 failed syncs, flush validation on the four FileHandle forms, inherited and private-getter options, the sync forms with an aborted signal, an abort during the open of a FIFO, and appendFile with a 3 MiB buffer. 9 fail on main.
  • test/js/node/fs/fs-promises-writeFile-async-iterator.test.ts: iterable data to each kind of path, a raw fd and a FileHandle, a failed write, validation before the open, exact bytes for many chunks, falsy rejections. 4 fail on main.
  • test/js/node/fs/fs-stat-seccomp-linux.test.ts: { flush: true } with an fsync errno outside the table reports EUNKNOWN, as fs.fsyncSync does. 2 fail on main.
  • Also run: promises.test.js, fs-leak.test.js, fs-path-length.test.ts, test/bundler/bun-build-compile-sourcemap.test.ts, and the vendored node tests test-fs-append-file*.js, test-fs-write-file*.js, test-fs-write-stream-flush.js, test-fs-fsync.js, test-fs-readfile*.js, test-fs-promises-writefile*.js, test-fs-promises-file-handle-{append-file,writeFile,write,sync}.js. tsc -p src/js/tsconfig.json reports 0 errors.
  • On the release build, all of fs.test.ts passes. On the debug build in this container, four readdir ... x 100 cases of fs.test.ts exceed their time limit with and without this change: Array.prototype.sort of 6,734 strings takes 2.5 s on that build.

Earlier revision. The first diff of this PR (84b2818) patched the two native sites with a flush_file_buffers helper in NodeFS and tested them with an LD_PRELOAD shim. Rebased onto main it matched node on 11 of the 14 forms and made three FileHandle forms reject. Its Windows arm also no longer compiled: #40860 removed windows::get_last_errno.


no test proof · iteration 1 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/node/fs/fs.test.ts

@coderabbitai

coderabbitai Bot commented Jul 30, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository: oven-sh/bun/.coderabbit.yaml
  • Review profile: ASSERTIVE
  • Plan: Essentials
  • Run ID: 97562323-3a17-474d-98a7-f5c659e4cc31
📥 Commits

Reviewing files that changed from the base of the PR and between 721f14f and 7619d2c.

📒 Files selected for processing (6)
  • src/js/node/fs.promises.ts
  • src/runtime/node/node_fs.rs
  • src/sys/windows/mod.rs
  • test/js/node/fs/fs-promises-writeFile-async-iterator.test.ts
  • test/js/node/fs/fs-stat-seccomp-linux.test.ts
  • test/js/node/fs/fs.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 8 remain after this review.


Walkthrough

Filesystem write APIs now share append-write handling and report requested flush failures. Promise writes support flush options for FileHandle and async-iterable targets. Tests cover write results, fsync errors, descriptor handling, and abort signals.

Changes

Filesystem writes and flushing

Layer / File(s) Summary
Promise write options and FileHandle forwarding
src/js/node/fs.promises.ts
Promise write wrappers adjust flush options for FileHandle targets. FileHandle.writeFile accepts and forwards flush.
Async-iterable write and flush flow
src/js/node/fs.promises.ts
Async-iterable writes validate flush, handle path targets, preserve thrown values, and call fsync after successful writes when requested. The helper no longer truncates to a counted byte total.
Native append and flush handling
src/runtime/node/node_fs.rs, src/sys/windows/mod.rs
NodeFS uses a shared write helper for append and path-buffer writes. Append mode skips preallocation and resizing. Requested Windows and POSIX flush errors are propagated. Sync calls validate abort signals without retaining them.
Async-iterable write validation
test/js/node/fs/fs-promises-writeFile-async-iterator.test.ts
Tests cover async-iterable flush results, write and iterable errors, target paths, append behavior, byte counts, and option validation.
Filesystem flush, append, and abort coverage
test/js/node/fs/fs.test.ts, test/js/node/fs/fs-stat-seccomp-linux.test.ts
Tests cover flush behavior across APIs, append writes, fsync failures, descriptor cleanup, option validation, and abort signals.

Suggested reviewers: jarred-sumner, dylan-conway

Priority: ➖ Normal

Merge Risk: ⚪ Minimal · up to 7619d

No merge-blocking issue is established; the change is mergeable after normal checks.

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
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.
Title check ✅ Passed The title clearly identifies the main change: routing appendFile through the write path and reporting { flush: true } fsync errors.
Description check ✅ Passed The description explains the problem, fix, trade-offs, and verification results. It provides substantial detail about behavior, tests, and known limitations, covering the template’s required informati…

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/node/node_fs.rs`:
- Around line 4779-4802: Update the write loop in append_file to detect when
Syscall::write returns zero bytes and break before slicing data again. Preserve
the existing behavior for positive writes and subsequent optional flushing,
while ensuring non-empty input cannot cause an infinite loop.
🪄 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: 9d210b77-6767-48fb-8363-31b04f66d788

📥 Commits

Reviewing files that changed from the base of the PR and between bbe3f6a and 1906fbb.

📒 Files selected for processing (2)
  • src/runtime/node/node_fs.rs
  • test/js/node/fs/fs.test.ts

Comment thread src/runtime/node/node_fs.rs Outdated
@github-actions

Copy link
Copy Markdown
Contributor

This PR may be a duplicate of:

  1. fs: honor the flag option in appendFile / appendFileSync #33559 - Also fixes ignored options in append_file() by routing through write_file; a superset that would also cover the { flush: true } fix in this PR

🤖 Generated with Claude Code

Comment thread src/runtime/node/node_fs.rs Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
src/runtime/node/node_fs.rs (2)

4781-4784: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Preserve the path on open errors.

The path branch propagates Syscall::open with ?, losing the target path in the error metadata. Match neighboring filesystem code and attach path_.slice() with err.with_path(...).

🤖 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 `@src/runtime/node/node_fs.rs` around lines 4781 - 4784, Update the
PathOrFileDescriptor::Path branch to handle Syscall::open errors explicitly
instead of propagating with ?. Preserve the existing successful file-descriptor
and scopeguard behavior, but attach path_.slice() to the error via
err.with_path(...) before returning it.

4795-4803: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Propagate flush errors and short writes. Both FlushFileBuffers and Syscall::fsync(fd) are ignored, so { flush: true } can return success without making the data durable. Also, a 0-byte write here breaks the loop and reports success after a partial append.

🤖 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 `@src/runtime/node/node_fs.rs` around lines 4795 - 4803, Update the flush
handling in the surrounding write method to propagate errors from both Windows
FlushFileBuffers and non-Windows Syscall::fsync instead of discarding their
results. Also revise the write loop so a zero-byte write is treated as an error
rather than successful completion, preserving correct reporting for partial
appends.

Source: Coding guidelines

🤖 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/node/node_fs.rs`:
- Around line 4790-4794: Update the write loop around the written check so a
zero-byte write with remaining data returns a concrete write error instead of
breaking and ultimately returning success. Preserve the existing data slicing
for positive writes and continue returning success only after all data has been
written.

---

Outside diff comments:
In `@src/runtime/node/node_fs.rs`:
- Around line 4781-4784: Update the PathOrFileDescriptor::Path branch to handle
Syscall::open errors explicitly instead of propagating with ?. Preserve the
existing successful file-descriptor and scopeguard behavior, but attach
path_.slice() to the error via err.with_path(...) before returning it.
- Around line 4795-4803: Update the flush handling in the surrounding write
method to propagate errors from both Windows FlushFileBuffers and non-Windows
Syscall::fsync instead of discarding their results. Also revise the write loop
so a zero-byte write is treated as an error rather than successful completion,
preserving correct reporting for partial appends.
🪄 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: 6c823d42-3a53-47c6-98dd-ea0e58e821ca

📥 Commits

Reviewing files that changed from the base of the PR and between 1906fbb and 1b989fb.

📒 Files selected for processing (1)
  • src/runtime/node/node_fs.rs

Comment thread src/runtime/node/node_fs.rs Outdated
Comment thread src/runtime/node/node_fs.rs Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

♻️ Duplicate comments (1)
src/runtime/node/node_fs.rs (1)

4787-4799: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Incomplete write still reported as success in append_file.

When Syscall::write returns 0 for a non-empty data, the loop now correctly breaks instead of spinning forever, but the function still falls through to the optional flush and returns Ok(()) even though data may still contain unwritten bytes. This silently truncates the appended content while telling the caller the append succeeded — this was already flagged in a prior review round (unresolved) as "Do not report success after an incomplete write."

🛡️ Proposed fix
         while !data.is_empty() {
             let written = Syscall::write(fd, data)?;
             if written == 0 {
-                break;
+                return Err(sys::Error::from_code(E::EIO, sys::Tag::write).with_fd(fd));
             }
             data = &data[written..];
         }
🤖 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 `@src/runtime/node/node_fs.rs` around lines 4787 - 4799, Update append_file’s
write loop so a zero-byte Syscall::write result with remaining data returns an
error instead of proceeding to flush and returning Ok(()). Preserve successful
flushing and completion only when all data has been written.
🤖 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.

Duplicate comments:
In `@src/runtime/node/node_fs.rs`:
- Around line 4787-4799: Update append_file’s write loop so a zero-byte
Syscall::write result with remaining data returns an error instead of proceeding
to flush and returning Ok(()). Preserve successful flushing and completion only
when all data has been written.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 4eee4ef4-ba6b-47c1-904c-5cb1051296df

📥 Commits

Reviewing files that changed from the base of the PR and between 1b989fb and 4c43de3.

📒 Files selected for processing (2)
  • src/runtime/node/node_fs.rs
  • test/js/node/fs/fs.test.ts

@robobun

robobun commented Jul 30, 2026 •

Copy link
Copy Markdown
Collaborator Author

Status. The branch was rebuilt on main and force-pushed as 7619d2c. The old diff did not apply to main and did not compile for Windows.

How to reproduce on main (Linux):

const fs = require("node:fs");
fs.writeFileSync("/dev/null", "x", { flush: true });  // bun: returns. node v26.3.0: EINVAL from fsync
fs.appendFileSync("/dev/null", "x", { flush: true }); // bun: returns. node v26.3.0: EINVAL from fsync

fsync(2) fails with EINVAL on /dev/null, so that error shows that a call synced and reported it. With an LD_PRELOAD shim that counts fsync and makes it fail with EIO, 12 of the 14 call forms in the PR body differ from node on main. On this branch, 0 differ.

The PR body has the design, the measurements, and two decisions that need a maintainer.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — the fsync error-propagation nit from my earlier pass is addressed via flush_file_buffers, and both call sites now surface EIO correctly.

What was reviewed:

  • append_file fd/path unification: close guard is held through the flush; fd arm still doesn't close the caller's fd.
  • flush_file_buffers: POSIX path propagates Syscall::fsync; Windows path uses the same from_code(get_last_errno(), Tag::fsync).with_fd(fd) shape as sibling syscall wrappers.
  • write_file_with_path_buffer: flush now runs after the held write_err is returned, so a write failure still surfaces first.
  • LD_PRELOAD tests: hermetic (skipIf on missing cc), drain both pipes, chain existing LD_PRELOAD, assert exact per-section fsync counts and {code:'EIO', syscall:'fsync'}.
Extended reasoning...

Overview

This PR makes fs.appendFile / fs.appendFileSync honor { flush: true } (Node ≥21 semantics) and, as a same-class fix, makes both append_file and write_file_with_path_buffer propagate fsync/FlushFileBuffers failures instead of binding them to let _. The two match arms in append_file are unified so a single write loop + optional flush runs on one fd, with the scopeguard close held until after the flush. The flush is factored into NodeFS::flush_file_buffers, which returns Maybe<()> and is called with ? at both sites. Two Linux-only tests compile an LD_PRELOAD fsync interposer to (a) count fsyncs per API form and (b) inject EIO and assert it surfaces as { code: 'EIO', syscall: 'fsync' }.

Security risks

None. No untrusted-input parsing, no auth/crypto, no new syscalls beyond what writeFile({flush:true}) already issued. The Windows error path uses the established sys::Error::from_code(windows::get_last_errno(), Tag::fsync).with_fd(fd) pattern from src/sys/lib.rs.

Level of scrutiny

Medium — this is a small, mechanical Node-compat fix in node_fs.rs. The only behavior change beyond the headline is that writeFileSync({flush:true}) now throws on fsync failure instead of silently returning success, which is the documented Node behavior and is covered by the new EIO test. The append_file refactor preserves the prior fd/path semantics: user-supplied fds are not closed (guard is None), path-opened fds are closed on every exit including the new ? on flush.

Other factors

All prior review threads are resolved: CodeRabbit's zero-byte-write guard was added (matching write_file_with_path_buffer), my earlier nit about discarding the fsync result was addressed by the flush_file_buffers helper + ?, and the comment-cop flag was resolved by dropping the doc comment. The PR author reports rust:check-all passes on all targets and the full fs.test.ts suite plus the four upstream Node test-fs-*-flush / test-fs-append-file* tests remain green. The related #33559 is noted as a superset with merge conflicts; this is the minimal targeted fix.

@robobun

robobun commented Aug 16, 2026

Copy link
Copy Markdown
Collaborator Author

Cross-reference from #39192 (the JS iterable path of flush), in case it is useful while this PR changes the native fsync handling: node's fsPromises.writeFile / appendFile validate flush for a FileHandle target but never sync it (early return before handleFdSync), and only sync paths they opened themselves. Probe on node v26.3.0 with a /dev/null handle, which cannot be fsynced: fh.writeFile("x", { flush: true }), fh.appendFile("x", { flush: true }) and fsp.writeFile(fh, ["x"], { flush: true }) all resolve, while fsp.writeFile("/dev/null", "x", { flush: true }) rejects with EINVAL from fsync. #39192 follows that for iterable data. With this PR's native error propagation, the fd/handle cases on the native path (handle.appendFile("x", { flush: true }), and handle.writeFile once #39192 forwards the option) would start rejecting on such descriptors, so the handle case may be worth deciding explicitly here; the callback fs.writeFile(fd, ...) form does sync user fds in node, so the distinction is promises-FileHandle specific.

… { flush: true } fsync error

appendFile had its own write loop and never read `flush`, so
`{ flush: true }` did not call fsync(2). writeFile called it and dropped
the result, so a failed sync reported success.

- NodeFS::append_file opens the file as before and then runs
  write_file_to_fd, the part of write_file_with_path_buffer after the
  open. That body reads `flush` and returns the fsync error. For
  appendFile it skips the preallocation and the resize, and on Windows
  it writes through libuv, as before.
- On Windows the sync is bun_sys::windows::flush_file_buffers.
- The parsers of readFile and writeFile keep `signal` only for the async
  forms. The sync forms never read it, as in node. appendFile now checks
  the signal after its open, as writeFile does.
- fs.promises does not request the sync on a FileHandle, as in node.
  FileHandle#writeFile validates `flush`.
- The iterable path of fs.promises.writeFile validates `flush`, opens
  each kind of path with the flag, syncs, drops the resize after the
  write, and rejects with a falsy value that the iterable throws.
@robobun
robobun force-pushed the farm/64e693d7/appendfile-flush branch from 84b2818 to 7619d2c Compare October 7, 2026 07:37
@robobun robobun changed the title fs: honor { flush: true } in appendFile / appendFileSync node:fs: run appendFile through writeFile's write path and report the { flush: true } fsync error Oct 7, 2026

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.

The fsync result is now propagated at both sites (Syscall::fsync(..)? on POSIX, windows::flush_file_buffers(fd)? on Windows), which covers what I raised on the previous push. Beyond the inline finding, I also checked: stripping flush for a FileHandle in fs.promises.writeFile/appendFile matches Node, which returns before its fsync step for a handle; the write_file_to_fd refactor keeps the close-on-path scopeguard and skips preallocate and the O_TRUNC resize for append, so the old appendFile semantics are preserved there; and gating options.signal on will_be_async only drops the already-aborted check for the sync variants, as in Node.

Extended reasoning...

The change moves the sync appendFile write loop into a shared write_file_to_fd helper, propagates the { flush: true } fsync/FlushFileBuffers error instead of discarding it, strips flush for FileHandle callers in fs.promises, and makes the sync fs variants ignore options.signal. It touches no security-sensitive surface. A confirmed inline finding on the shared write loop's amt == 0 exit remains, so this is a ruled-out note rather than an approval.

Additional findings (outside the current diff — GitHub can't attach inline comments there):

  • 🟡 src/runtime/node/node_fs.rs — Callers of appendFileSync / appendFile on an fd can now get success with part of the data never written, where the base branch's loop never returned until every byte was written. The shared loop at src/runtime/node/node_fs.rs:7466-7468 breaks on amt == 0 with write_err still None, and write_file_to_fd then runs the flush and returns Ok(()). appendFile previously had its own loop at base node_fs.rs:4709-4712 that only exited when data was empty. Fix: when a write returns 0 for a non-empty buffer, set write_err to a short-write error (ENOSPC/EIO-style with Tag::write) instead of breaking silently, for both the append and writeFile callers.

    Why this was flagged

    Trigger: fs.appendFileSync(fd, data) or fs.appendFile(fd, data, cb) where the descriptor's write returns 0 before the buffer is drained; a 0 return for a non-empty buffer reaches the if amt == 0 { break; } at src/runtime/node/node_fs.rs:7466. write_err stays None, so the check at node_fs.rs:7494 does not fire; the function proceeds to the optional fsync and returns Ok(()) at node_fs.rs:7508 with buf still non-empty. The caller is told the append succeeded and, with { flush: true }, that it is durable, while the tail of the data was never written. On the base branch append_file's loop (base node_fs.rs:4709-4712 and 4719-4722) kept calling write until the slice was empty, so it never reported success with bytes left over. appendFile accepts any fd, including sockets, pipes and device files where zero-length write completions are possible, and the consequence is silent data loss rather than an error. Remedy: turn a 0-byte write of a non-empty buffer into a write error.

    Verification: Triggers only when the descriptor's write() returns 0 for a non-empty buffer; on that input the base appendFile spun in its own loop, while the merged code returns success with data missing. In src/runtime/node/node_fs.rs the loop at 7448-7471 does if amt == 0 { break; } with write_err left None, so the check at 7494-7496 does not fire and the function returns Ok(()) at 7508.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants