Repository navigation
node:fs: copyFile and cp fail and remove the destination when close(2) of the destination fails - #44331
node:fs: copyFile and cp fail and remove the destination when close(2) of the destination fails#44331robobun wants to merge 7 commits into
Conversation
…d bun_sys::close Both entry points had their own copy of the close call and of the rule for its result, one per platform: five copies in two files. They now share close_once (the call) and close_error (the rule). The rule does not change: only EBADF surfaces.
The POSIX close kept EBADF and dropped every other errno. A file system can report a write error (ENOSPC, EDQUOT, EIO) first at close(2): NFS, SMB and FUSE do. fs.close, fs.closeSync and FileHandle.close returned success for it. They now report the error, as node does. EINTR and EINPROGRESS stay a success, as in libuv. Fd::close() still drops the result, and its debug assert fires for EBADF only. node:wasi removes its entry for a descriptor before the close, and http2 respondWithFile ignores the result of its close.
…) of the destination fails A file system can report a write error (ENOSPC, EDQUOT, EIO) first at close(2): NFS, SMB and FUSE do. The copy paths dropped the result of the close of the destination, so copyFile and cp returned success for an incomplete copy. libuv's uv_fs_copyfile keeps the error and unlinks the destination. close_copy_dest now returns the result of the copy. After a good copy it closes through close_copied_dest: when that close fails, the copy fails with the close error as a copyfile error, and the destination is unlinked by the rule of a failed copy (only a regular file that is not the source). The FICLONE exits and the FreeBSD copyFile arm close through the same helper.
|
Status Reproduced on Linux x64 with a simulated close failure. My environment cannot mount NFS, SMB or FUSE, so a real mount is not tested. The shim in this PR ( cc -shared -fPIC -o shim.so test/js/node/fs/fs-close-error-shim.c -ldl
head -c 20000 /dev/urandom > src.bin
LD_PRELOAD=$PWD/shim.so bun -e 'require("fs").copyFileSync("src.bin", "copy.28.close-fault")'
ls -l copy.28.close-fault
The stack: #44329 (one close in This PR is a draft on purpose. Its base is the branch of #41798, so it must not merge before #41798 and #44330 are on main. Then it becomes one commit on main. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline findings, I also checked the two ScopeGuard::into_inner disarms in the Linux copy_file_range/sendfile arms of copy_file — each runs before close_copied_dest, so dest is closed exactly once on those exits — and the http2.ts tryClose change, where passing a noop callback to fs.close routes a close error to the callback instead of an unhandled promise rejection.
Extended reasoning...
The change unifies POSIX close into close_once/close_error in bun_sys, propagates close(2) errors through Fd::close_allowing_* and node:fs copyFile/cp (with cfg-gated copies for Linux, macOS and FreeBSD), and reorders FD_MAP updates in wasi.ts. It touches no auth, crypto or injection surface, but it does add an unlink of the destination path on close failure and alters syscall-wrapper semantics used across the runtime. Inline findings were posted, the diff is cross-platform and largely untestable on this host for macOS/FreeBSD arms, so a human look is still warranted; the note only records what else was ruled out.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟣
src/runtime/node/node_fs.rs— pre-existing: users of writeFile and appendFile on NFS, SMB or FUSE still get success and a short or missing file when close(2) reports ENOSPC, EDQUOT or EIO, while Node throws or rejects. This is the same class this PR fixes for copyFile: write_file_with_path_buffer closes its path-opened fd through a scopeguardfd.close()at src/runtime/node/node_fs.rs:7432-7438, which drops the close result; append_file does the same at node_fs.rs:4718. Fix: after a successful write, close through close_allowing_bad_file_descriptor and return the close error (syscall close) instead of Ok, for both sites, which serve writeFileSync, writeFile, promises.writeFile, appendFileSync, appendFile and promises.appendFile. [also at: src/runtime/node/node_fs.rs:4942 - pre-existing: users of fs.writeFileSync, fs.writeFile and fs.appendFile on NFS, SMB or FUSE still get success when close(2) of the file fails with ENOSPC, EDQUOT or EIO, while this PR makes copyFile and cp report it.]Why this was flagged
A user calls fs.writeFileSync(path, buffer) or fs.promises.writeFile(path, buffer) on an NFS, SMB or FUSE mount where the write is accepted and the error is reported first at close(2). write_file_with_path_buffer at src/runtime/node/node_fs.rs:7415-7431 opens the fd, and the scopeguard at node_fs.rs:7432-7438 closes it with
fd.close(), whose result Fd::close at src/sys/fd.rs:69-75 discards. The function returns Ok(()) at node_fs.rs:7543, so the user sees success and a file that may be short or missing. append_file at node_fs.rs:4718 has the same shape and returns Ok(()) at node_fs.rs:4723. Node's writeFileSync runs fs.closeSync(fd) in its finally block and throws the close error, and promises.writeFile rejects through handleFdClose. The base branch behaves the same way, so this is pre-existing; the PR series makes close errors observable in sys::close_error (src/sys/lib.rs:1956) but only rewires copyFile and cp to consume them.Verification: pre-existing (the base branch drops these close results by the same route; this PR fixes only the copyFile/cp destination close). The write error is reported first at close(2) (NFS, SMB, FUSE returning ENOSPC/EDQUOT/EIO).
write_file_with_path_bufferat src/runtime/node/node_fs.rs:7432-7439 closes the fd viafd.close()and returnsOk(())at line 7543 regardless of the close result.
| dest_fd: FD, | ||
| src_stat: &sys::Stat, | ||
| ) -> Maybe<ret::CopyFile> { | ||
| let close_err = dest_fd.close_allowing_bad_file_descriptor(None); |
There was a problem hiding this comment.
🟣 pre-existing, not blocking: Processes started with a closed stdio slot (e.g. bun script.js <&-) still get success from copyFile and cp when close(2) of the destination fails, and leak the descriptor, despite this PR's promise to report that error. node_fs.rs:4948 closes through close_allowing_bad_file_descriptor, and src/sys/fd.rs:81-83 returns None without calling close for any fd whose stdio_tag() is set, so a dest_fd of 0, 1 or 2 is never closed and never reports. Fix: close the copy destination through close_allowing_standard_io (the fd is owned by this function and was opened here), so a stdio-numbered descriptor is both released and reported like every other.
A small fix can ride a push you are already making; otherwise a short reply is enough.
Why this was flagged
A user runs bun with stdin closed (<&-, or from a supervisor that closes fd 0), so the next open(2) returns fd 0. In copy_file the Syscall::open(dest, ...) at node_fs.rs:5240 (Linux), 5134 (FreeBSD) or 5065 (macOS) yields dest_fd = 0. After the copy, node_fs.rs:4948 calls dest_fd.close_allowing_bad_file_descriptor(None); src/sys/fd.rs:81-83 checks self.stdio_tag().is_some() and returns None with a log line, never calling close(2). The match at node_fs.rs:4950 sees None and returns Ok, so a close that the file system would have failed with ENOSPC or EIO is never issued, the copy reports success, and fd 0 stays open for the life of the process. The base branch leaked the same fd through dest_fd.close() (the same skip), but the base did not claim to report close errors. Remedy: use close_allowing_standard_io for the dest descriptor that this function opened, as node_fs::close at node_fs.rs:4732 already does.
Verification: The corrected trigger is a script that closes fd 0/1/2 at runtime (e.g. fs.closeSync(0)) before calling copyFile/cp; bun script.js <&- does NOT produce this (c-bindings.cpp:735-741). node_fs.rs:4948 and src/sys/fd.rs:81-84 never pass a dest_fd numbered 0-2 to close(2); close_copied_dest returns Ok(()) (line 4951). Base dest_fd.close() (node_fs.rs:4927) has the same stdio skip, so merging makes nothing worse.
There was a problem hiding this comment.
This is #43844. Fd::close() skips fd 0, 1 and 2, so each descriptor that bun opens while one of them is closed stays open. #43845 moved the first group of descriptors off those numbers where bun_sys creates them. The issue lists each internal open and close pair as what is left.
A different close in this one caller does not fix that class, so this PR keeps the rule of Fd::close(). The Notes now list the case under "Not fixed here".
After the last byte of the file, the only error that the read stream can report is the close of the descriptor. The descriptor was only read, so the response is whole. Before this change that error reset the stream with NGHTTP2_INTERNAL_ERROR when the stream was still open. The wasi comment names what uvwasi does. Each test fixture now runs in its own test. The fd_renumber test does not depend on the direction of the move.
|
Review follow-up in 38478b6. It is the commit that #44330 got for its review (the http2 file response), with the copy tests of this PR in the new shape.
|
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Still open from earlier reviews (1):
- Unresolved: 1 minor or pre-existing.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟡
src/js/node/fs.promises.ts— Callers of fs.promises.writeFile with an async iterable on NFS, SMB or FUSE can now lose the write error or the AbortError when close(2) also fails. At src/js/node/fs.promises.ts:1663await fs.close(fdOrPath)is unguarded, so a close error thrown there skips the abort check and thethrow errorbelow it; the same at src/js/node/fs.promises.ts:1641 on the abort path. Fix: catch the close error on both sites and rethrow the earlier error or AbortError when one exists (Node aggregates the two via aggregateTwoErrors, keeping the write error's code), falling back to the close error only when the write succeeded.Why this was flagged
A user calls fs.promises.writeFile(path, asyncIterable) on a mount that reports write errors at close. A write fails or the signal aborts, then the close reports ENOSPC. writeFileAsyncIterator at src/js/node/fs.promises.ts:1649-1653 stores the write error in
error, then line 1663 awaits fs.close, which after this PR rejects with the close errno (src/sys/lib.rs:1961 now surfaces every errno except EINTR and EINPROGRESS, routed through node_fs.rs:4732). That rejection propagates out of writeFileAsyncIterator before the abort shadowing at line 1667 and thethrow errorat line 1672, so the caller sees only the close error. On the base branch, fs.close could only reject with EBADF, so this line never threw and the write error or AbortError always reached the caller. Node's handleFdClose rejects with aggregateTwoErrors(closeError, opError), which keeps the operation error's code. No guard exists: the try/catch at 1649 covers only the write, and 1663 is outside it.Verification: Trigger: fs.promises.writeFile(path, asyncIterable) where the write loop throws, or the signal aborts, and the close(2) fails. src/js/node/fs.promises.ts:1663
await fs.close(fdOrPath);is unguarded, so a rejecting close propagates out beforethrow errorat 1672. The original error is replaced by the close errno; on base the close error was swallowed. Not covered by the PR's tests.
|
Updated 5:38 PM PT - Oct 1st, 2026
❌ @robobun, your commit d27e7ba has 1 failures in
🧪 To try this PR locally: bunx bun-pr 44331That installs a local version of the PR into your bun-44331 --bun |
…ose fails too fs.promises.writeFile(path, iterable) closes the file itself. A close that failed replaced the error of the iterable, or the AbortError. The error that came first now keeps its place, in an AggregateError as in node's handleFdClose. An abort still shadows every other error.
|
Fixed. The code is from #44330, so the change is there (c921d2b) and in this branch as 38a9a64.
The |
There was a problem hiding this comment.
I reviewed the latest push (through the writeFile-iterable commit) and found no bugs, but since this widens what every unix close(2) reports and reshapes the copy exits on three platforms, a human look is still worthwhile.
What was reviewed:
- Every caller of
bun_sys::close/close_allowing_bad_file_descriptoroutside node_fs (watcher, resolver, glob, ipc, shell, Blob, install) discards the result, so the new non-EBADF errnos only surface throughnode:fsclose and the copy paths. - Each copy exit (FICLONE, copy_file_range, FreeBSD, the
cptwin) closes dest exactly once; the twoScopeGuard::into_innerdisarms happen only on the success exits, and the error exits still close through the guard. - The writeFile-iterable change matches Node's handleFdClose argument order (
aggregateTwoErrors(closeError, opError)), and the abort path still shadows a close error. - The macOS and FreeBSD arms of node_fs.rs could not be compiled in this environment; they were checked by reading only.
Extended reasoning...
The change makes posix_impl::close_once/close_error the single close path for unix in src/sys, so any errno except EINTR/EINPROGRESS now surfaces from bun_sys::close and close_allowing_bad_file_descriptor, and node_fs.rs routes every POSIX copy_file and cp destination close through close_copied_dest, which unlinks a regular non-source dest and fails the copy on a close error. JS changes cover fs.promises.writeFile with an iterable, http2 tryClose, and WASI fd_close/fd_renumber ordering; no auth, crypto, or injection surface is touched. Tests are an LD_PRELOAD shim plus four concurrent subprocess fixtures, gated on glibc and a C compiler. Deferring rather than approving because the diff spans cfg-gated code for macOS and FreeBSD that cannot be compiled here, changes the meaning of a core syscall wrapper used across the codebase, and makes a user-visible decision to unlink an existing destination on close failure.
Still open from earlier reviews (1):
- Unresolved: 1 minor or pre-existing.
… AbortError Node's handleFdClose aggregates the error of the close with the error of the operation, also when that error is the abort. An abort dropped the close error. It is now in the AggregateError, and the code stays ABORT_ERR.
…lose Both open a file, write it and close it. The close went through Fd::close(), which drops the result, so a write error that the file system reports first at close was lost. The error of the close is now the result of the call, as in node: syscall close, no path, and the file stays.
|
This PR is now a draft. I ran a review of the whole stack. Its verdict: keep the fix and its layer, and change how it lands.
The PR body has the list and the details. |
This branch has not been deployed
1 participant
Problem
fs.copyFile*andfs.cp*report success, and keep the file, whenclose(2)of the destination fails. NFS, SMB and FUSE can report a write error first at close. Node throwsENOSPC: no space left on device, copyfileand removes the destination.close_copy_destinsrc/runtime/node/node_fs.rs(node:fs: remove the destination when copyFile/cp fails partway #41798) ends withdest_fd.close(), which drops the result.Fix
copyfile. The destination is then unlinked by the rule of node:fs: remove the destination when copyFile/cp fails partway #41798: a regular file that is not the source.test/js/node/fs/fs-close-error.test.tsfail without thenode_fs.rschange.Background
close_copy_dest(node:fs: remove the destination when copyFile/cp fails partway #41798) trims or unlinks a destination. fs.writeFile, Bun.write, fs.copyFile: fail when the final ftruncate fails on a regular file #42618 adds anftruncatecheck there. The tail needs one function, and node:fs: remove the destination when copyFile/cp fails partway #41798 sets its signature.fsyncbefore the close: a blocking syscall for every copy, and the close result is still dropped.Downsides
copyFiledoes too, and itscpSynckeeps it. A maintainer must choose.copyFileSync) or 18 (cpSync): no new syscall, branch or allocation.textgrows by 1,024 B.Notes
Stack. The base of this PR is #41798, which adds
close_copy_dest. The commits of #44329 and #44330 are in this branch (0f2f8c7, 219635a, 38478b6, 38a9a64, 618c91e and d27e7ba). The change of this PR is 6772f97:node_fs.rs, the copy tests, and the FICLONE switch of the test shim. The order to land: #44329, then #44330, and #41798. Then this PR, as one commit on main.Self-review. It ran on this PR as the head of the stack. Its verdict: keep the fix and its layer, and change how it is cut.
writeFileandappendFileby path still returned success where node throwsENOSPC close. Done in sys, node:fs: close reports every errno except EINTR and EINPROGRESS #44330.close_copied_desthere, besideclose_copy_destof node:fs: remove the destination when copyFile/cp fails partway #41798 andfinish_copied_destof fs.writeFile, Bun.write, fs.copyFile: fail when the final ftruncate fails on a regular file #42618. Open.close_copy_destalready takes the result of the copy and returns it here. The FICLONE exits callclose_copied_destbecause node:fs: remove the destination when copyFile/cp fails partway #41798 leaves them without a trim. One function for every exit needs the signature that node:fs: remove the destination when copyFile/cp fails partway #41798 lands with, so that change comes with the rebase.src/sys/fd.rsas sys: one single-shot POSIX close behind close_allowing_standard_io and bun_sys::close #44329. Done: sys: one single-shot POSIX close behind close_allowing_standard_io and bun_sys::close #44329 says so.Reproduction. No NFS, SMB or FUSE mount exists in my environment, so the close failure is simulated. The shim
test/js/node/fs/fs-close-error-shim.cdoes the realcloseof each file named*.<errno>.close-faultthat is open for writing, and then returns that errno.copyFileSync,copyFile,promises.copyFilecopyfile, file removedcopyfile, file removedcpSync,cp,promises.cpof one filecopyfile, file removedcopyfile, file removedcopyFileSync, close returns EINTR or EINPROGRESSWhere this PR differs from node. Each row except the last node column is a test.
copyFileSync(p, p))ftruncate), and removes the fifocpSynconto a file that existed, andcpSyncof a directorycopyfile, file removedcp, file keptThe rule "only a regular file that is not the source" is the rule of #41798 for a copy that fails partway. In the last row node's
cpSynctakes another path in C++, which closes through libccloseand does not unlink. I saw that row with a shim that also interposesclose.Cost of a copy that succeeds. Release builds of this branch with and without the last commit, same toolchain:
gdb, every syscall of the JS thread during one call:copyFileSync8 before and after,cpSyncof one file 15 before and after. The lists are identical.gdb, single-step through the native function:copyFileSync1,857 to 1,878 instructions,cpSync(copy_single_file_sync) 281 to 299. Conditional jumps (270 and 28), calls (36 and 6) and allocator calls (0) do not change. The added instructions pass the result of the copy intoclose_copy_destand back.size:text80,661,782 B to 80,662,806 B.nm -S:copy_dest_close_failedis 408 B and cold,close_copied_destis 192 B,close_copy_destgrows by 245 B.newfstatatandunlink. With EINTR it issues none. The destination is closed once in both cases.What ran, and what did not.
fs-close-error.test.ts(4 pass),fs-promises-writeFile-async-iterator.test.tsandwasi.test.js. Each of the 4 tests runs one bun process under the shim and compares its report with onetoEqual.fs-copyfile-write-error.test.ts,cp.test.ts,cp-symlink-target.test.ts, thecopyFiletests offs.test.ts, and node's 79test-fs-copyfile*andtest-fs-cp-*files.node_fs.rsis the same in both.FICLONEioctl report success and copy nothing. An empty destination after a close that returns EINTR shows that the copy took that exit.bun run rust:check-allpasses for all 12 targets. The macOS arm (sources up to 128 KB) and the FreeBSD arms are compiled, not run.Deliberate differences from node.
fs.c#L1403-L1408).fchmodof the destination: bun drops its result, as before this PR. The data is written and the close reported no error. libuv fails the copy, except for EPERM on CIFS (fs.c#L1338-L1355).Not fixed here.
clonefile()andcopyfile(3)close their own descriptors.ftruncateruns between the data and the close, and its result is dropped. fs.writeFile, Bun.write, fs.copyFile: fail when the final ftruncate fails on a regular file #42618 is about that.CopyFileW) is unchanged.Fd::close()skips those numbers for every descriptor that bun opens (Descriptors that bun opens while fd 0, 1 or 2 is closed take that number and are never closed #43844).writeFileandappendFiledrop the result of their own close. sys, node:fs: close reports every errno except EINTR and EINPROGRESS #44330 lists them.