Repository navigation
Conversation
fs.writev(fd, buffers, fn, cb) and fs.readv(fd, buffers, fn, cb) took the function in the position slot as the callback and never called cb. The result went to the wrong function, and util.promisify(fs.writev)(fd, buffers, fn) never resolved. Both wrappers now use node's line, `callback ||= position`. The native parser already reads a function in the position slot as the current position. Both functions report an invalid fd or buffers before an invalid callback, as node does. Only a call with an invalid callback runs that check.
|
Status
|
|
Updated 5:31 AM PT - Oct 22nd, 2026
❌ @robobun, your commit 04f04fe has 1 failures in
🧪 To try this PR locally: bunx bun-pr 46460That installs a local version of the PR into your bun-46460 --bun |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
Walkthrough
ChangesFilesystem callbacks
Suggested reviewers: Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to
|
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 note, I checked the one behavior change the refactor introduces: position is now forwarded to fs.writevCb/fs.readvCb even when it holds a function. The native parser (FdVectorIo::from_js in src/runtime/node/node_fs.rs, via i52::offset_from_js -> JSValue::get_number) yields None for any non-number, so a function in that slot is "no position" as before. I also confirmed the array form $ERR_INVALID_ARG_TYPE("buffers", ["ArrayBufferView[]"], ...) renders Node's "must be an ArrayBufferView[]" text (src/jsc/bindings/ErrorCode.cpp list rendering), matching the new assertions.
Extended reasoning...
The diff replaces the position/callback reshuffle in fs.writev and fs.readv with Node's callback ||= position, adds a helper that reproduces Node's fd-then-buffers-then-cb check order on the invalid-callback path, and adds a 30-test block covering last-argument-wins, falsy fallbacks, error ordering, promisify and EBADF delivery. It touches no security-sensitive surface. The native parser already ignores a non-number position and the error-code formatting matches Node, so the remaining concerns were ruled out from the code.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟣
src/js/node/fs.ts— pre-existing nit: users of util.promisify(fs.writev) still get a result keyed buffers, so Node-style code reading result.buffer sees undefined. src/js/node/fs.ts:659 registers ["bytesWritten", "buffers"] while Node's lib/fs.js v26.3.0 L953-L957 registers ['bytesWritten', 'buffer'] for writev (readv keeps buffers). Fix: change the writev entry at fs.ts:659 to ["bytesWritten", "buffer"] and turn the new it.failing at test/js/node/fs/fs.test.ts:2527 into a passing test, leaving readv at fs.ts:657 as buffers. The PR adds an it.failing for this instead of the one-line fix it is next to.Why this was flagged
A caller does const { bytesWritten, buffer } = await util.promisify(fs.writev)(fd, bufs), the shape Node documents and returns. In Bun, defineCustomPromisifyArgs(writev, ["bytesWritten", "buffers"]) at src/js/node/fs.ts:659 makes util.promisify build the object with key buffers, so buffer is undefined. The base branch behaves the same; the PR touches writev and adds it.failing("util.promisify(fs.writev) resolves { bytesWritten, buffer } like node") at test/js/node/fs/fs.test.ts:2527 that certifies the gap rather than fixing it. Node v26.3.0 lib/fs.js L953-L957 (cited by the test comment) uses 'buffer' for writev and 'buffers' for readv, so only the writev entry needs to change.
Verification: pre-existing (nit). Trigger: any caller doing
const { bytesWritten, buffer } = await util.promisify(fs.writev)(fd, bufs)(Node's key name). Mechanism: src/js/node/fs.ts:659defineCustomPromisifyArgs(writev, ["bytesWritten", "buffers"]);so promisify builds{ bytesWritten, buffers }andresult.bufferis undefined. The base branch has the identical line, and the diff does not touch it, so merging makes nothing worse.
Problem
fs.writev(fd, bufs, fn, cb)andfs.readv(fd, bufs, fn, cb)callfnand nevercb. Node callscb.util.promisify(fs.writev)(fd, bufs, fn)resolves on Node and stays pending on Bun.if (typeof position === "function") callback = positioninsrc/js/node/fs.ts. It overwrites a callback that the caller gave. Node hascallback ||= position(lib/fs.js:755).Fix
callback ||= position. The native parser already reads a function in the position slot as the current position.fdorbuffersbefore an invalid callback, as Node does. Only an invalid callback reaches that check.test/js/node/fs/fs.test.ts, one new block (30 tests, 13 fail on main). Other suites: Notes.node:testcopy of the rows. Two belong to node:fs: take a function in the options slot as the default options #46369.Background
readFile,writeFile,write). This PR adds two.appendFileandrealpath.native, leave the function in the options slot. They need node:fs: take a function in the options slot as the default options #46369 first.Downsides
fs.writev(fd, bufs, fn, "x")calledfn. It now throwsERR_INVALID_ARG_TYPEbefore any I/O, as Node does..textdoes not change. A valid call runs 5 fewer bytecodes.fs.writeand other fd functions still check the callback first. Notes lists what stays different from Node.Notes
Repro. Node v26.3.0, a release build of main 9634520, and a release build of this change.
Node v26.3.0:
main 9634520:
This branch prints Node's lines, except for line 3:
resolves bytesWritten,buffers(see "Not changed here").The promisify case. It is a parity gap in these two functions, not a general hang.
util.promisifyputs its callback last, sopromisify(fs.writev)(fd, bufs, fn)isfs.writev(fd, bufs, fn, cb). In a slot-first function such asfs.statorfs.readdir, the same shape stays pending on Node too. This PR does not change those.Native binding.
fs.writevCbtakes the callback as argument 0 (split_callbackinsrc/runtime/node/node_fs_binding.rs), so the choice betweenpositionandcallbackhas to happen in the wrapper, where Node makes it.Check order.
callback ||= positionalone would movefs.writev(-1, bufs, fn, "str")from Node'sfderror to thecberror.throwInvalidVectorCallbackkeeps Node's order (fd, buffers, callback) forwritevandreadv. It also repairsfs.writev(-1, bufs), which reportedcbon main. The helper is for these two functions only.fs.write(-1, buf, fn, "str")still reportscbwhere Node reportsfd, and so do other functions: in a probe of 22 doubly invalid calls, 10 differ from Node on main (access,chmod,copyFile,open,rm,mkdir,truncate,ftruncate,write,fstat). Node checks the callback at a different point in each function, so no single place fixes the class. Open #44476 reordersfs.readandfs.write.Not changed here. Each row is the same on main and on this branch.
fs.realpath.native(p, fn, cb)cbfnfs.appendFile(p, d, fn, "x")ERR_INVALID_ARG_TYPEfnutil.promisify(fs.writev)(fd, bufs){ bytesWritten, buffer }{ bytesWritten, buffers }it.failingin this PR marks itfs.readv(fd, [], cb)cb(EINVAL)cb(null, 0, [])fs.writev(fd, "x", cb)The "buffers" argument must be an ArrayBufferView[]. Received type string ('x')Expected ArrayBufferView[]fs.writev(2147483647, bufs, cb)err.syscalliswriteerr.syscalliswritevreadvfs.write(-1, buf, fn, "str")ERR_OUT_OF_RANGEforfdERR_INVALID_ARG_TYPEforcbfs.read(fd, buf, 0, 1, 0, "str")ERR_INVALID_ARG_TYPETypeErrorfs.fstat(fd, "str")ERR_INVALID_ARG_TYPETypeErrorappendFileandrealpath.nativeneed #46369 first becausecallback ||= optionsleaves the function in the options slot, and the native parser reads option properties off it. On main,fs.appendFile(p, d, fn, cb)with aflushproperty onfnthrowsERR_INVALID_ARG_TYPEforflush. Withcallback ||= optionsalone,fs.appendFile(p, d, cb)would do the same for a debounced callback. #46369 makes the parsers take a function as the default options. Its open review threads already ask for thecallback || optionsform inrealpath.native.History and precedent. The two wrappers came with #3419, with
if (typeof position === "function") { callback = position; position = null; }. Node's lines arelib/fs.js:755(readv) and:936(writev). #12412 wrotecallback ||= optionsintoreadFileandwriteFile. #39552 made the same kind of change innode:dns. #46021 took the error path of these two wrappers (callback(err, 0, buffers)) from #41440.Measurements. Release builds (LTO) of main 9634520 and of this change on top of it, same tree and build directory. The branch is now rebased onto 7a74841, which does not change
src/js/node/fs.ts.ensureCallback(BUN_JSC_dumpGeneratedBytecodes=1): 30 to 25 with a position, 32 to 27 without. The wrapper tests the callback itself and callswrapFsCallbackdirectly, soensureCallback(8 bytecodes) no longer runs.heapStatsafter 1,000 pending calls): 2Functionand 2JSLexicalEnvironment, 0Promise, before and after.llvm-size -A):.bun_builtins2,712,743 to 2,713,237 (+494 B),.text81,932,069 unchanged.bloatyis not installed in my container.writevandreadvcall shapes (position and callback of each type): 10 on main, 0 on this branch.Tests. The block has 30 tests. On main 13 fail. The other 17 are controls:
readFile,writeFileandwritealready take the last argument, slot-first functions (realpath,readdir,stat,mkdir) keep calling the function in the slot, a falsy last argument falls back to the function before it, and oneit.failingmarks thepromisifykey. Each expected value is what Node v26.3.0 gives for the same call. I reverted each of the 10 clauses of the change in turn (the two||=lines,||=to??=, each check of the helper, their order): each revert fails at least one test.Also run on a debug ASAN build of this branch: the vendored
test-fs-*files for read, write, readv, writev and streams (97 files, all pass),dir.test.ts,promises.test.js,fs-promises-writeFile-async-iterator.test.ts,util-promisify.test.js,trace-events.test.ts(all pass) andAsyncLocalStorage.test.ts(62 pass, and one test, a loop of 100,000 iterations with no fs call, times out on the debug build in my container).The whole of
fs.test.tson the release build: 652 pass, 1 fails. The failure is "keeps a non-throwing callback in the same place in the event loop". It fails in 7 of 8 runs on a release build of main too in my container: it races a thread-poolfs.statagainstsetImmediate.Node's own tests do not pass two functions, so no vendored test covers this change.
Not run. Windows and macOS. The change is JS only and has no platform branch.
no test proof · iteration 0 · 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