Repository navigation
Conversation
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.
|
Updated 4:52 PM PT - Oct 1st, 2026
✅ @robobun, your commit c37bac708909b831c473442f8a023e9899503377 passed in 🧪 To try this PR locally: bunx bun-pr 44330That installs a local version of the PR into your bun-44330 --bun |
|
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
LD_PRELOAD=$PWD/shim.so bun -e '
const fs = require("fs");
const fd = fs.openSync("x.28.close-fault", "w");
fs.writeSync(fd, "data");
fs.closeSync(fd);
console.log("closeSync returned");'
The stack: #44329 (one close in |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Findings marked 🟡 are optional suggestions and need no follow-up push.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🔴
src/js/node/http2.ts— http2 clients downloading a file from a server whose file system reports an error at close of a read-only descriptor (FUSE flush) now receive RST_STREAM INTERNAL_ERROR after the whole body; on the base branch the response ended cleanly. On the success path the read stream has autoClose at src/js/node/http2.ts:3116, its close error goes to onFileStreamError at src/js/node/http2.ts:3143, and that calls this.close(NGHTTP2_INTERNAL_ERROR) whenever the stream is not yet closed. Fix: a close error on an fd that was only read must not reset the http2 stream once the body has been handed over; treat the error from the read stream's fd close like tryClose does (ignore or log), or skip onFileStreamError after fileStream 'end'. [also at: src/js/node/http2.ts:3001 - HTTP/2 clients served a whole file from a FUSE or NFS mount can now receive RST_STREAM INTERNAL_ERROR after the complete body, where the base finished the stream cleanly. respondWithFile's read stream is created with autoClose (src/js/node/http2.ts:3115) and its final close now surfaces EIO/ENOSPC via src/sys/lib.rs:1960, so the read stream emits 'error' and onFileStreamError at src/js/node/http2.ts:3143 calls this.close(NGHTTP2_INTERNAL_ERROR) on a stream that already sent END_STREAM.]Why this was flagged
Trigger: stream.respondWithFile(path) succeeds and the read fd's close(2) returns a non-EINTR/EINPROGRESS errno. Mechanism: src/sys/lib.rs:1960 now surfaces the errno; fs.createReadStream(null, { fd, autoClose: true }) at src/js/node/http2.ts:3111-3120 closes the fd in _destroy after 'end' and emits 'error' when close fails; src/js/node/http2.ts:3143 routes that to onFileStreamError (3151-3153), which sends RST_STREAM INTERNAL_ERROR if this.closed is still false. Whether the stream is already closed when the async close callback lands is a race between nghttp2 processing END_STREAM and the fs callback, so some fraction of full transfers get a reset. Base behaviour: close_error returned None for every errno but EBADF, so no 'error' event and a clean end. Remedy: do not reset the http2 stream for a close error of an fd that was only read.
Verification: src/sys/lib.rs:1958-1960 now returns every other errno from
close_error. That is wired toonFileStreamError(http2.ts:3143), which doesif (!this.destroyed && !this.closed) this.close(NGHTTP2_INTERNAL_ERROR)(http2.ts:3151-3153). On the base branchclose_errorsurfaced only EBADF, so this fd close never errored and the response ended cleanly. -
🟣
src/js/node/wasi.ts— Programs holding two WASI instances with different bindings get the file operations of the instance constructed last applied to both, and the new test leaves a throwing closeSync installed module-wide. src/js/node/wasi.ts:413 declares a single module-levellet fs, and src/js/node/wasi.ts:603 reassigns it in every constructor. The new wasi tests install a bindings.fs whose closeSync throws ENOSPC; after they run, any later fd_close/fd_renumber on an instance created earlier in the same process, or any WASI instance created before another one, uses the wrong fs. Fix: store the bindings' fs on the instance (this.fs) and read it in the wasiImport closures instead of a module global.Why this was flagged
src/js/node/wasi.ts:413
let fs;is module scope; src/js/node/wasi.ts:603fs = bindings.fs;runs in each constructor. All wasiImport closures (e.g. fd_close at src/js/node/wasi.ts:867, fd_renumber at 1173) read the module-level fs, not this.bindings.fs. Trigger: two WASI instances alive with different bindings (the second constructor rebinds fs for the first), or the new test in test/js/bun/wasm/wasi.test.js (bindingsWithFailingClose) followed by any use of an earlier instance in the same bun test process. The dismissing finders relied on each new WASI resetting fs before use; that holds only for the instance constructed last. The PR touches exactly the two functions (fd_close, fd_renumber) that now depend on closeSync throwing per-binding, and adds tests that make the module-level binding throw, so the shared global is now load-bearing for correctness. Base branch has the same global, so the root design is pre-existing, but the new tests are the first in-tree code to install a throwing fs there. Remedy: keep fs per instance.Verification: pre-existing. /home/claude/bun/src/js/node/wasi.ts:413
let fs;is module scope and :603fs = bindings.fs;rebinds it in every constructor; fd_close (:867) and fd_renumber (:1173) read that module-levelfs. So two live WASI instances with differentbindings.fsshare the last one's. Not introduced by the PR: b6666c4 has identical lines 413 and 603; the diff only reordersFD_MAPvsfs.closeSync.
| const fromEntry = CHECK_FD(from, BigInt(0)); | ||
| const toEntry = CHECK_FD(to, BigInt(0)); | ||
| fs.closeSync(fromEntry.real); | ||
| this.FD_MAP.set(from, toEntry); | ||
| this.FD_MAP.delete(to); | ||
| fs.closeSync(fromEntry.real); |
There was a problem hiding this comment.
🟣 pre-existing, not blocking: pre-existing: WASI guests calling fd_renumber(from, to) get the two descriptors swapped the wrong way, so the file they meant to keep is closed and to becomes EBADF. The reordered body at src/js/node/wasi.ts:1171-1173 still maps from to the old to entry, deletes to, and closes fromEntry.real, which is the inverse of WASI/uvwasi semantics (close to's file, move from's entry to to, delete from). The new test at test/js/bun/wasm/wasi.test.js:305 asserts this inverted mapping and so certifies it. Fix: close toEntry.real, set FD_MAP.set(to, fromEntry), delete from (no-op when from === to), and update the test to expect to holding the former from entry.
A small fix can ride a push you are already making; otherwise a short reply is enough.
Why this was flagged
A guest calls fd_renumber(5, 3) through wasiImport, expecting fd 3 to afterwards refer to the file that fd 5 held and fd 5 to be released (wasi_snapshot_preview1 fd_renumber; uvwasi_fd_table_renumber(dst=to, src=from) closes dst's file and copies src into dst). In src/js/node/wasi.ts:1169-1173 the code instead keeps entry 5 pointing at the old fd 3 file, deletes entry 3, and closes fromEntry.real, the file the guest wanted to keep. The guest's next read or write on fd 3 returns WASI_EBADF, and any further use of fd 5 hits the file that should have been discarded, while the real host descriptor of the kept file is already closed. The base branch behaves the same way (only the closeSync call moved), so this is pre-existing; the new test test/js/bun/wasm/wasi.test.js:298-306 pins the inverted result ({ from: secondEntry, to: false }), so it locks the bug in rather than guarding against it.
Verification: pre-existing — the base branch already has the inverted mapping; this PR reorders the body and adds a test that pins the inverted behavior. Triggering condition: any WASI guest calling fd_renumber(from, to). At src/js/node/wasi.ts:1168-1175 from now points at the old to resource, to is gone, and the real fd behind from is closed, so the next fd_read on to returns WASI_EBADF (8).
There was a problem hiding this comment.
Correct, and it is older than this PR. uvwasi closes to and moves from into it (fd_table.c#L441-L456). wasi.ts does the reverse.
This PR only moves the map update before the close, so I left the direction for a change of its own. The test no longer pins it: it now asserts that no guest number maps to the descriptor that the call closed. The Notes list the inversion under "Found, not changed here".
| Ok(()) | Err(libc::EINTR | libc::EINPROGRESS) => None, | ||
| Err(errno) => Some(Error::from_code_int(errno, Tag::close).with_fd(fd)), |
There was a problem hiding this comment.
🟣 pre-existing, not blocking: Apps that call fs.close(fd) without a callback now get an unhandled rejection instead of Node's uncaught exception when the close fails. src/sys/lib.rs:1960 makes ENOSPC/EDQUOT/EIO reach fs.close, and the no-callback arm at src/js/node/fs.ts:94 does fs.close(fd).then(() => {}), so the error surfaces only as an unhandled rejection. In Bun's default mode that prints, sets exit code 1 and keeps running; uncaughtException handlers never see it. Fix: route the no-callback case through fs.closeCb with a guarded default callback that rethrows the error, so every fs-reported close error takes the uncaughtException path like Node. Pre-existing for EBADF; this PR widens it to environment errors.
A small fix can ride a push you are already making; otherwise a short reply is enough.
Why this was flagged
User code opens a file on NFS/SMB/FUSE, writes, and calls fs.close(fd) with no callback, which is common fire-and-forget usage. After src/sys/lib.rs:1959-1960 the close's ENOSPC/EDQUOT/EIO is returned as an error where the base returned None. src/js/node/fs.ts:89-97 handles the missing callback with fs.close(fd).then(() => {}), so the rejection has no handler. src/jsc/VirtualMachine.rs:4392-4398 and 4450-4451 show the default Mode::Bun path: Bun__handleUnhandledRejection only consults unhandledRejection listeners, never uncaughtException, then the default handler prints the error and bumps unhandled_error_counter so the process later exits 1 while continuing to run. Node's close(fd, callback = defaultCloseCallback) throws the error from the FS callback, so it goes to process.on('uncaughtException') handlers or crashes. On the base branch only EBADF (a programming bug) could reach this arm; after the PR a file-system condition in production does.
Verification: src/sys/lib.rs:1957-1961 now returns Some(Error) for every errno except EINTR/EINPROGRESS (base returned it only for EBADF). src/js/node/fs.ts:89-97 (untouched by the PR) handles the missing-callback case with fs.close(fd).then(() => {}) — no rejection handler, so the error becomes an unhandled rejection. The process continues; uncaughtException listeners never see it.
There was a problem hiding this comment.
Correct. Node throws the error from defaultCloseCallback (fs.js#L488-L505). Bun's arm for no callback has been a promise since #10229, so EBADF already takes this path.
To move it to the callback arm changes what an unhandledRejection handler sees for EBADF today. That is a change of its own, so it is not in this PR. The Notes list it under "Found, not changed 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 a113f11.
|
…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.
|
c921d2b answers a finding that the review posted on #44331 (#44331 (comment)). The code is from this PR, so the change is here.
|
| try { | ||
| await fs.close(fdOrPath); |
There was a problem hiding this comment.
🟣 pre-existing, not blocking: pre-existing: users writing to SMB (or a FUSE daemon that reports a flush error once) with fs.promises.writeFile(path, iterable) still get a resolved promise after a lost write, which Node rejects with ENOSPC. writeFileAsyncIteratorInner writes through Bun.file(fd).writer(), a dup of the fd (src/io/openForWriting.rs:34), and its close drops the errno; cifs clears the error on the first close, so fs.close at src/js/node/fs.promises.ts:1668 sees 0. Fix: carry the sink's close error (or write through the original fd so one close reports), covering FileHandle.writeFile(iterable) too. The shim faults every close, so the test cannot see this.
A small fix can ride a push you are already making; otherwise a short reply is enough.
Why this was flagged
A user calls fs.promises.writeFile(path, asyncIterable) on a cifs mount that is out of space; write(2) succeeds and the error is reported at close. writeFileAsyncIteratorInner (src/js/node/fs.promises.ts:1574) creates Bun.file(fd).writer(), which open_for_writing turns into dup_with_flags(fd, 0) (src/io/openForWriting.rs:34). await writer.end() (src/js/node/fs.promises.ts:1600) closes that dup on the work pool; Closer::run calls Fd::close() (src/io/lib.rs:2225), which discards the errno. cifs_flush test_and_clear_bit()s AS_ENOSPC/AS_EIO, so only the first close of the open file description gets ENOSPC. When the dup's close runs first the fs.close at src/js/node/fs.promises.ts:1668 returns 0 and the new catch at 1669-1672 never runs; writeFile resolves. The base branch also resolves here, so this is pre-existing, but the PR's test (test/js/node/fs/fs-close-error.test.ts:67, 202) asserts "ENOSPC close" for this path, which passes only because fs-close-error-shim.c:57-62 fails every close of a matching file, including the dup.
Verification: pre-existing. Trigger: fs.promises.writeFile(path, asyncIterable) on a cifs mount, when the write error surfaces at close. The sink writes to a dup (src/io/openForWriting.rs:34); Closer::run drops the errno (src/io/lib.rs:2224); cifs_flush clears the error, so fs.close at fs.promises.ts:1668 returns 0 and the promise resolves after a lost write. The shim fails every close, so the dup also fails.
There was a problem hiding this comment.
Yes. The writer of writeFileAsyncIteratorInner is a FileSink on a duplicate of the descriptor, and its close drops the result. On NFS and SMB that first close gets the error, so the fs.close after it returns 0.
This PR does not change that. Downsides lists FileSink as not fixed, and the Notes have this case under "Not covered". To carry the error of the sink changes FileSink for every user of Bun.file().writer(), so it needs a change of its own.
The comment of the fixture now says that the shim fails both closes, and that on NFS and SMB only the first one gets the error.
… 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.
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 (4):
- Unresolved: 4 minor or pre-existing.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟡
src/js/node/fs.promises.ts— nit: a FileHandle whose sync autoClose fails now never emits 'close', although its fd is already -1 and the async close() path emits it on every outcome. In [kCloseSync] at src/js/node/fs.promises.ts:1502-1503 closeSync throws before this.emit("close") runs, which this PR makes reachable for ENOSPC/EDQUOT/EIO instead of EBADF only. Fix: emit 'close' before rethrowing (try/finally around closeSync) so listeners such as streams.ts:174/418 and user handle.on('close') see the handle closed on the error path too, matching close() at line 743.Why this was flagged
A user calls handle.writer({ autoClose: true }) or handle.pull({ autoClose: true }) on a mount whose close(2) reports ENOSPC (or EIO via FUSE flush for the read side). endSync() at src/js/node/fs.promises.ts:1453, fail() via teardown at 1466 ([Symbol.dispose] at 1484) or the pull cleanup at 1026 call handlekCloseSync. At 1500-1502 kFd is set to -1 and the registry entry is unregistered, then closeSync throws, so this.emit("close") at 1503 is skipped. The async close() at 731-743 emits 'close' unconditionally before returning the (possibly rejecting) promise, and Node's FileHandle.close emits it the same way. Listeners on the handle's 'close' event (user code, or the fd.on("close", ...) hooks at src/js/internal/fs/streams.ts:174 and 418) never fire although the descriptor is gone. On the base branch close_error only returned EBADF, so this path was a programming-error case; after src/sys/lib.rs:1959-1960 it is reached by a file-system condition in production.
Verification: nit. Trigger: a FileHandle whose fd is auto-closed synchronously on a mount where close(2) returns ENOSPC/EDQUOT/EIO, which this PR newly surfaces (on the base only EBADF could throw here). [kCloseSync] at src/js/node/fs.promises.ts:1494-1504 calls
closeSync(fd)(:1502) with no try/finally, sothis.emit("close")at :1503 is skipped when closeSync throws.
|
On the Bun's |
…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.
| PathOrFileDescriptor::Path(path_) => { | ||
| let path = path_.slice_z(&mut self.sync_error_buf); | ||
| let fd = Syscall::open(path, args.flag.as_int(), args.mode)?; | ||
| let _close = scopeguard::guard(fd, |fd| fd.close()); | ||
| let close = scopeguard::guard(fd, |fd| fd.close()); | ||
| while !data.is_empty() { | ||
| let written = Syscall::write(fd, data)?; | ||
| data = &data[written..]; | ||
| } | ||
| Ok(()) | ||
| Self::close_written(scopeguard::ScopeGuard::into_inner(close)) | ||
| } |
There was a problem hiding this comment.
🟣 pre-existing, not blocking: pre-existing: users passing flush: true to fs.appendFile / appendFileSync / promises.appendFile never get an fsync, so the data is not forced to disk and no fsync error can be reported. NodeFS::append_file at src/runtime/node/node_fs.rs:4700-4726 parses flush (args::WriteFile::from_js, src/runtime/node/node_fs.rs:4253 and 4281) but never reads args.flush. Fix: when args.flush is set, call fsync (FlushFileBuffers on Windows) after the write loop in both the Fd and Path arms and return its error with syscall fsync, matching Node where appendFileSync delegates to writeFileSync and writeAll runs fs.fsync; test/js/node/test/parallel/test-fs-append-file-flush.js:26 comments out the 'performs flush' cases on the claim that "fsync is called in native code", which is not true for appendFile.
A small fix can ride a push you are already making; otherwise a short reply is enough.
Why this was flagged
A user calls fs.appendFileSync(path, data, { flush: true }) (or the callback / promise forms) expecting the bytes to be on disk when the call returns. The option is parsed into args.flush by args::WriteFile::from_js at src/runtime/node/node_fs.rs:4253-4262 and 4281. NodeFS::append_file at src/runtime/node/node_fs.rs:4700-4726 writes in a loop and goes straight to close_written (4723) or Ok(()) (4713); nothing reads args.flush and no fsync is issued. A power loss or a file system that reports the write error only at fsync therefore loses the appended data with no error, where Node's appendFileSync calls writeFileSync which runs fs.fsyncSync(fd) and surfaces its error. The base branch behaves the same, so this is pre-existing, but it sits in the function this PR rewrites to surface deferred write errors. The vendored Node test test/js/node/test/parallel/test-fs-append-file-flush.js:26-38 disables the 'performs flush' assertions with a comment saying fsync is called in native code, so nothing in the suite catches the missing call.
Verification: Trigger: any caller passing { flush: true } to fs.appendFile / fs.appendFileSync. NodeFS::append_file (src/runtime/node/node_fs.rs:4700-4726) only writes in a loop and then returns Ok(()) or Self::close_written(...); args.flush is never read. The base branch has the identical omission. Data appended with flush:true is not forced to storage and no fsync error can be reported.
There was a problem hiding this comment.
Correct, and also older than this PR: append_file never reads args.flush.
It belongs with the fsync result of writeFile (the other thread): one change for flush: true, which can also turn on the cases that test-fs-append-file-flush.js has commented out. The Notes list it under "Found, not changed here".
| } | ||
| } |
There was a problem hiding this comment.
🟣 pre-existing, not blocking: pre-existing: users passing flush: true to fs.writeFile / writeFileSync / promises.writeFile on NFS or SMB still get silent success after a lost write, which Node reports. The fsync result is discarded at src/runtime/node/node_fs.rs:7512 (and FlushFileBuffers at 7508), and on these file systems the deferred ENOSPC/EDQUOT/EIO is reported once, so fsync consumes it and the new close_written at 7518 then sees a clean close. Fix: propagate a failed fsync as the call's error (syscall fsync, as Node's writeFileSync does via fsyncSync and writeAll via the fsync callback) on both the posix and Windows arms, before the close; close_written still runs afterwards so a later close error is not lost either.
A small fix can ride a push you are already making; otherwise a short reply is enough.
Why this was flagged
A user calls fs.writeFileSync(path, data, { flush: true }) on an NFS or SMB mount that is out of space or quota; write(2) succeeds and the error is deferred. write_file_with_path_buffer at src/runtime/node/node_fs.rs:7505-7514 runs let _ = Syscall::fsync(fd); (7512) or let _ = FlushFileBuffers(...) (7508) and discards the ENOSPC it returns. NFS's nfs_file_fsync reports the error and advances the errseq, and nfs_file_flush at close only reports errors raised since its own sample, so the close at src/runtime/node/node_fs.rs:7518 (close_written) returns 0. The call returns Ok(()) and the user is told the data is durable although it is not on disk. Node's writeFileSync calls fs.fsyncSync(fd) inside the try and throws ENOSPC with syscall fsync. The base branch behaves the same, so this is pre-existing, but the PR's purpose of surfacing deferred write errors from writeFile leaves exactly the durability-opt-in path still silent. close_written cannot catch it because the file system has already reported and cleared the error at fsync.
Verification: pre-existing. Trigger: fs.writeFileSync / fs.writeFile / fs.promises.writeFile with { flush: true } on a file system that reports a deferred write error (NFS/SMB). src/runtime/node/node_fs.rs lines 7505-7514 run let _ = Syscall::fsync(fd); (7512) and discard the result; the fsync call consumes the deferred error, so close_written (7516-7518) returns 0 and the call returns Ok(()).
There was a problem hiding this comment.
Correct, and it is older than this PR. With flush: true the fsync takes the deferred error first, so the close after it sees none.
It needs a change of its own: the result of fsync on POSIX and of FlushFileBuffers on Windows, node's error shape for it (syscall fsync), and a way to make fsync fail in a test. The Notes list it under "Found, not changed here", with the appendFile case from the other thread.
|
c37bac7: A review of the whole stack asked for this. Without it the stack fixed |
This branch has not been deployed
1 participant
Problem
fs.close*,FileHandle.close, write streams,fs.writeFile*andfs.appendFile*report success whenclose(2)fails. NFS, SMB and FUSE can report a write error first at close. Node throwsENOSPC: no space left on device, close.close_errorinsrc/sys/lib.rsreturns EBADF and drops every other errno.Fix
close_errorreturns every errno except EINTR and EINPROGRESS, as libuv does.writeFileandappendFileby path return the error of their close.Fd::close()still drops the result, and asserts for EBADF only. Three users offs.closechange too (Notes).test/js/node/fs/fs-close-error.test.ts,test/js/bun/wasm/wasi.test.js. 3 of the 4 new tests fail on sys: one single-shot POSIX close behind close_allowing_standard_io and bun_sys::close #44329. The fourth guards http2.Background
NodeFS::close,writeFileandappendFileread the result of a close. Of theBun.*APIs, only the external sourcemap ofBun.buildwithcompilechanges.LD_PRELOADshim does the real close, then returns the error.fs.closewrong.src/install/npm.rs). If that must stay, node:fs: copyFile and cp fail and remove the destination when close(2) of the destination fails #44331 gets its own close function and this PR closes.Downsides
text, 552 B of it built-in JS. Instructions of a call that succeeds:closeSync154 (no change),writeFileSync237 to 235,appendFileSync162 to 164.Bun.writeandFileSinkdrop the result of their close, andflush: truethat offsync(Notes).Notes
Stack. PR 2 of 3. #44329 is the groundwork. #44331 uses this in
fs.copyFileandfs.cp. The order to land: #44329, then this PR. #44331 is a draft until this PR and its base #41798 are on main.Self-review. It ran on #44331 as the head of the stack, and its list is in the Notes there. Its concern for this PR:
writeFileandappendFileby path still returned success where node throwsENOSPC close. c37bac7 adds them.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. Bun and libuv both close throughsyscall(SYS_close, fd)on Linux, so one shim works for both runtimes.fs.closeSynccloseclosefs.close(callback)FileHandle.closefs.createWriteStreamcloseevent, no errorerrorevent with ENOSPCerrorevent with ENOSPCfs.writeFileSync,fs.writeFile,fs.promises.writeFileby pathclose, file keptfs.appendFileSync,fs.appendFile,fs.promises.appendFileby pathclose, file keptfs.closeSync, errno EINTR or EINPROGRESSfs.closeSyncgives the same result for EDQUOT and EIO. In every row the descriptor is closed, andcloseis issued once.Who reads a close result. Counted with
grepat the head of this PR:close_allowing_standard_ioandclose_allowing_bad_file_descriptor: 17 call sites outsidesrc/sys/fd.rs. Two read the result:NodeFS::closeand the newNodeFS::close_written, whichwriteFileandappendFilecall (src/runtime/node/node_fs.rs). Three return it to callers that drop it:Entry::close_fdin the resolver,Accessor::closein the glob walker, andsys_uv::close, which is Windows only. The other 12 drop it.NodeFS::write_file_with_path_bufferhas one caller outsidenode:fs:Bun.buildwithcompilewrites its external sourcemap through it (js_bundle_completion_task.rs). A close that fails there now printsfailed to write sourcemap fileand drops that output, as any other write error does.bun_sys::close: 37 qualified call sites, alllet _ =. Insrc/sys, 51 more: 50 drop it andFile::closereturns it. No caller ofFile::closeoutside unit tests reads it.fs.closeorfs.closeSync:fs.promisesFileHandlesets its fd to -1 before the close.internal/fs/streamsandUtf8Streamemiterror, as node does.worker_threads,netand theFileHandlefinalizer call it insidetry.Dirfromopendirholds no descriptor (its handle is the constant 1), so it closes nothing.FileHandleiterator APIs (pull,writer,readableWebStream) close in the same places as node's.node:wasi,node:http2andfs.promises.writeFilewith an iterable change in this PR.fs.close(fd)with no callback: a failed close is an unhandled rejection. In node it is an uncaught exception.The users that change.
node:wasifd_closeandfd_renumbercalledcloseSyncbefore they updatedFD_MAP. A close that throws left the entry in the map, and the guest's nextfd_closeclosed that number again. The number can belong to another file by then. uvwasi keeps the entry when the close fails (uvwasi.c#L813-L816). Bun removes it, because the descriptor is released.tryClosecallsfs.close(fd)with no callback. Itstrydoes not catch the rejection, so a failed close ended the process. Node asserts here (fs.close(fd, assert.ifError)), which also ends the process. Bun now ignores the error: the descriptor was only read.fs.createReadStreamwithautoClose. A failed close made that stream emiterror, andonFileStreamErrorreset the http2 stream withNGHTTP2_INTERNAL_ERRORwhen it was still open. WithwaitForTrailersthat happens after the whole body. The handler now ignores an error that comes after the read stream'send: only the close can fail then. Node destroys the stream with that error (core.js#L2685-L2691). Bun ignores it for the same reason as intryClose.fs.promises.writeFile(path, iterable)opens and closes the file itself, and itsawait fs.close()had no guard. 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'shandleFdClose(promises.js#L1075-L1086). For an abort the AggregateError has the codeABORT_ERRand both errors. An abort still shadows the error of the iterable, as before. When only the close fails, the call rejects with the close error.writeFileandappendFileby path (sync, callback and promise) open the file, write it and close it inNodeFS::write_file_with_path_bufferandNodeFS::append_file. Both closed throughFd::close(). After a write that succeeded they now return the error of the close. As in node, the error hascode,errnoandsyscallonly, and the file stays. When the write fails too, bun reports the write error in every form, as before. Node's sync forms report the close error then, and its async forms an AggregateError with the code of the write error (seen withulimit -fand the shim).Machine code. Release builds of #44329 and of this PR (c37bac7), same toolchain:
size:text80,666,326 B to 80,667,134 B.sizecounts the embedded built-in JS astext: that is 552 B. ThewriteFileandappendFilecommit is 256 B:write_file_with_path_buffergrows by 109 B andappend_fileby 89 B (nm -S).nm -Sshowed 6 functions that grew, by 5 to 7 B each. They are the ones that return the result:close_allowing_standard_io,close_allowing_bad_file_descriptor,bun_sys::close,File::close, the globAccessor::close, and thecloseSynchost function.objdump -d, addresses normalised: 279 of the 284 functions with an inlinedsyscall(SYS_close)are identical. Everyfd.close()that drops its result compiles to the same code as before.gdb, single-step through one call, base and head: thecloseSynchost function runs 154 instructions and 18 conditional jumps in both, with 1 syscall.write_file_with_path_buffer(writeFileSyncof 4 KB by path) runs 237 and 235 instructions, 35 and 36 conditional jumps, 5 calls and 4 syscalls in both (openat,write,ftruncate,close).append_fileruns 162 and 164 instructions, 25 conditional jumps, 4 calls and 3 syscalls in both.bundle-modules.ts --debug=OFF):node/http2.js168,934 B to 169,010 B,node/fs.promises.js43,556 B to 44,032 B.node/wasi.jsdoes not change size. The new code runs only when a close fails.Tests.
fs-close-error.test.tshas 2 tests. Each runs one bun process under the shim and compares its report with onetoEqual. On a build of sys: one single-shot POSIX close behind close_allowing_standard_io and bun_sys::close #44329 thenode:fstest fails: the calls return where they must throw.node:fstest has a row for each of the sixwriteFileandappendFileforms by path: the error is{ code: "ENOSPC", syscall: "close" }with nopathand nofd, and the file has its 4,096 bytes.node:fstest also coverswriteFilewith an iterable: only the close fails, the iterable and the close fail, an abort after the open and the close fail, an abort in the iterable and the close fail. Without thefs.promises.tschange the close error replaces the first error./cancelledtakes thetryClosepath./sentsends the file withwaitForTrailers, so the http2 stream is still open when the close of the file fails. Without thehttp2.tschange the client getsrstCode2 for/sent.wasi.test.jshas 2 new tests. Both fail without thewasi.tschange. Thefd_renumbertest asserts that no guest number maps to the descriptor that the call closed. It does not depend on the direction of the move.fs-stat-seccomp-linux.test.ts: they need a C fixture and glibc. Say so if they belong infs.test.ts.fs-close-error.test.ts,wasi.test.js,fs-promises-writeFile-async-iterator.test.ts,fs.test.ts(615 pass, 8 skip),test/bundler/bun-build-compile-sourcemap.test.ts(9 pass), and 15 of node'stest-fs-write-file*,test-fs-append-file*,test-fs-promises-writefile*,test-fs-promises-file-handle-writeFile,test-fs-promises-file-handle-append-fileandtest-fs-promisesfiles. On a113f11: 20 of node'stest-http2-respond-*,test-http2-generic-streams-sendfileandtest-http2-error-orderfiles.tsc -p src/js/tsconfig.jsonandoxlintpass.fs.test.ts,cp.test.ts,promises.test.js,dir.test.ts,fs-promises-writeFile-async-iterator.test.ts,fs-open-cloexec.test.ts, and 64 of node'stest-fs-close*,test-fs-promises-file-handle-*,test-fs-write-stream*,test-fs-read-stream-*andtest-http2-respond-*files. The host was under heavy load, and 6 tests exceeded their timeout in the full runs. 5 of them pass when run alone. The sixth,readdirSync ... x 100, has a 10 s limit and took 12.6 s.Not covered.
fs.promises.writeFile(path, asyncIterable)closes the file twice: aFileSinkon a duplicate of the descriptor, thenfs.close. On NFS and SMB the first close takes the error and drops it. The test shim fails both closes, so the tests do not show this.bun run rust:check-allpassed for all 12 targets at the first commit of this PR. The macOS and FreeBSD arms are compiled, not run. On Windowsuv_fs_closealready returns its error, sowriteFileandappendFilereport it there too. CI builds and tests those targets.fs.truncateby path has no close in bun: it callstruncate(2). Node opens the file for it, so node reportsENOSPC closethere under the shim.Found, not changed here.
writeFilewithflush: truedrops the result offsync(FlushFileBufferson Windows). On NFS and SMB that call takes the deferred error, so the close after it reports none.appendFiledoes not readflushat all. Node reports the error offsync.node:wasifd_renumber(from, to)moves the wrong way: it closesfromand gives its number to the file ofto. uvwasi closestoand movesfrominto it (fd_table.c#L441-L456). This is older than this PR, which only moves the map update before the close.wasi.tskeeps thefsof its bindings in one module-level variable, so two instances with different bindings share the last one.fs.close(fd)with no callback reports a failed close as an unhandled rejection. Node throws it fromdefaultCloseCallback(fs.js#L488-L505). EBADF already takes this path.Related. #38630 adds a debug ledger to both close entry points. After #44329 there is one place for it.