node:fs: call void operation callbacks with (null) alone, as node does by robobun · Pull Request #38085 · oven-sh/bun

robobun · 2026-08-13T07:10:20Z

Problem

The callback form of every void node:fs operation calls back with two arguments on success, (null, undefined); node calls back with one, (null):

$ bun  -e "require('fs').chmod('f', 0o644, function(){ console.log(arguments.length, Array.from(arguments)) })"
2 [ null, undefined ]
$ node -e "require('fs').chmod('f', 0o644, function(){ console.log(arguments.length, Array.from(arguments)) })"
1 [ null ]

Affects the 22 operations wired through nullcallback(): appendFile, chmod, chown, copyFile, fchmod, fchown, fdatasync, fsync, ftruncate, futimes, lchmod, lchown, link, lutimes, mkdir, rename, rm, rmdir, truncate, unlink, utimes, writeFile.
Cause: nullcallback() (src/js/node/fs.ts:50) returns callback.bind(undefined, null), and callers pass that straight to .then(), e.g. fs.chmod(path, mode).then(nullcallback(callback), callback) (fs.ts:141). The promise machinery invokes the bound function with the resolved value, so the user callback receives (null, undefined).
fs.cp has the opposite mismatch: bun calls back with (null), node with (null, undefined).
Observable through arguments.length and rest parameters, e.g. multi-argument promisify wrappers (pify's multiArgs) resolve to [undefined] in bun and [] in node.

Fix

nullcallback() now binds callOnceWithNull (the existing helper used by Dir.prototype.close) with the callback, so the fulfillment value is dropped and the callback receives exactly (null). This is what the comment above nullcallback always claimed it was equivalent to; the callOnceWithNull/callOnceWithNullThen declarations move up next to it.
mkdir gets its own fulfillment handler: a recursive mkdir resolves with the first directory it created, and node passes that along as a second argument, so it is forwarded when defined and dropped otherwise. That is the same rule as node's FSReqCallback::Resolve (see Background); the other 21 operations always resolve with undefined, so they only need callOnceWithNull.
cp switches to callOnceWithNullThen, so it calls back with (null, undefined) like node, whose fs.cp is util.callbackify(cpFn) (lib/fs.js#L1098 in v26.3.0). Matching the observable behavior rather than the other operations' shape is deliberate; a code comment says why.
Why this is correct: it matches node. For every operation touched here, node v26.3.0 and this branch now produce identical callback argument lists (probe output in the details below).
fs.access and fs.symlink hand the native promise's value to the callback directly and are handled in node:fs: accessSync() returns undefined, promises.access() resolves to undefined, symlink callback gets null #38065; they are left alone here. access appears in the new table only because it already matches.
Verified:
- test/js/node/fs/fs.test.ts, new describe("a void operation's callback receives the same arguments as in node"): one row per operation asserting toStrictEqual([null]) (toEqual would accept the extra undefined), plus recursive mkdir still passing [null, firstDirCreated] and cp passing [null, undefined]. 24 of the 27 cases fail on the released binary (1.4.0), all pass with the fix.
- Full test/js/node/fs/fs.test.ts: 537 pass, 0 fail.
- test/js/node/fs/{cp,cp-symlink-target,dir,fs-mkdir}.test.ts, promises.test.js: 123 pass, 0 fail.
- Ported node tests for the affected operations (test-fs-mkdir*, rm, rmdir*, chmod*, utimes*, lchown*, lchmod, link, cp-async-, append-file, write-file*, truncate*, copyfile*, rename*, unlink*, fsync*, chown*, futimes*, opendir*) and test-trace-events-fs-{async,sync}: all pass.

Background

The callback form of node:fs in bun is a thin layer over the native promise-returning binding: binding.op(...).then(onSuccess, callback). onSuccess is always invoked with the promise's resolved value, so whatever function is passed there decides what the user callback sees.
Function.prototype.bind prepends the bound arguments and still forwards the call-time arguments. callback.bind(undefined, null) therefore calls callback(null, resolvedValue); binding a helper that takes the callback as its bound argument and ignores the rest is what yields callback(null).
In node, async fs operations complete through FSReqCallback::Resolve (src/node_file.cc#L736-L741 in v26.3.0): it calls the callback with just null when the result is undefined, and with (null, result) otherwise. Recursive mkdir is the only operation touched here whose result can be defined.
fs.cp is the exception because node implements it in JS via util.callbackify, which always calls cb(null, ret), even when ret is undefined.
fs: run node:fs callbacks from the event loop, not a promise reaction #33366 replaces this promise layer with native callbacks and would subsume this change; this PR is the narrow fix for the argument shape on the current code.

Probe: callback argument lists, node v26.3.0 vs this branch

Each operation is run on a fresh file/directory and the callback's arguments are printed. The two runtimes now print identical output for all of these; the bun 1.4.0 column is what the released binary prints.

op                           node v26.3.0 / this branch        bun 1.4.0
access                       1 [null]                          1 [null]
appendFile                   1 [null]                          2 [null, undefined]
chmod                        1 [null]                          2 [null, undefined]
chown                        1 [null]                          2 [null, undefined]
close                        1 [null]                          1 [null]
copyFile                     1 [null]                          2 [null, undefined]
cp                           2 [null, undefined]               1 [null]
fchmod                       1 [null]                          2 [null, undefined]
fchown                       1 [null]                          2 [null, undefined]
fdatasync                    1 [null]                          2 [null, undefined]
fsync                        1 [null]                          2 [null, undefined]
ftruncate                    1 [null]                          2 [null, undefined]
futimes                      1 [null]                          2 [null, undefined]
lchown                       1 [null]                          2 [null, undefined]
link                         1 [null]                          2 [null, undefined]
lutimes                      1 [null]                          2 [null, undefined]
mkdir                        1 [null]                          2 [null, undefined]
mkdir (recursive, created)   2 [null, "<first dir created>"]   2 [null, "<first dir created>"]
mkdir (recursive, existing)  1 [null]                          2 [null, undefined]
rename                       1 [null]                          2 [null, undefined]
rm                           1 [null]                          2 [null, undefined]
rm (recursive)               1 [null]                          2 [null, undefined]
rmdir                        1 [null]                          2 [null, undefined]
truncate                     1 [null]                          2 [null, undefined]
unlink                       1 [null]                          2 [null, undefined]
utimes                       1 [null]                          2 [null, undefined]
writeFile                    1 [null]                          2 [null, undefined]

Unchanged and already matching node: dir.read ((null, dirent) / (null, null)), dir.close ((null)), and the value-returning operations ((null, value)).

nullcallback() bound the user callback to null and handed the result to .then(), so the promise's resolved value arrived as a second argument and every void operation (chmod, utimes, unlink, writeFile, ...) called back with (null, undefined) where node calls back with (null). Bind a helper that drops the value instead. mkdir keeps forwarding the first created directory of a recursive mkdir, which node also passes along. cp now calls back with (null, undefined), matching node's util.callbackify-based implementation.

coderabbitai · 2026-08-13T07:10:28Z

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 26 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: c8c460e2-ee5b-4462-928b-3d72e1f8e5ee

📥 Commits

Reviewing files that changed from the base of the PR and between b7a0431 and 659bc00.

📒 Files selected for processing (2)

src/js/node/fs.ts
test/js/node/fs/fs.test.ts

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

robobun · 2026-08-13T07:10:30Z

^{Updated 2:35 PM PT - Aug 13th, 2026}

✅ @robobun, your commit 659bc008581d08a453e97f1435f50a982c9f5a91 passed in Build #94793! 🎉

🧪 To try this PR locally:

bunx bun-pr 38085

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

bun-38085 --bun

robobun · 2026-08-13T07:11:32Z

Reproduced on bun 1.4.0 against node v26.3.0: every void operation wired through nullcallback() (chmod, utimes, unlink, writeFile, rm, ... 22 in total) calls back with (null, undefined) where node calls back with (null), and fs.cp calls back with (null) where node calls back with (null, undefined). The new table in test/js/node/fs/fs.test.ts fails 24 of its 27 cases on the released binary and passes with this branch; the node-vs-bun probe output is in the PR description.

Related PRs: fs.access and fs.symlink have the separate bug of passing the native promise's value straight through; that is fixed in #38065 (different lines, not duplicated here). #33366 is the larger native-callback rewrite that would remove these call sites altogether; this PR is the narrow fix on the current code.

claude

LGTM — narrow, well-tested Node compat fix.

What was reviewed:

nullcallback() now binds callOnceWithNull so the promise's resolved value is dropped and callbacks receive exactly (null); traced every caller and confirmed the error path is unchanged.
mkdir's new handler correctly forwards firstDirCreated only when defined (matches FSReqCallback::Resolve); cp switching to callOnceWithNullThen yields (null, undefined) per node's util.callbackify.
Moving callOnceWithNull/callOnceWithNullThen is a no-op (hoisted function declarations); existing Dir#read/Dir#close call sites still resolve them.
Tests: toStrictEqual([null]) distinguishes arity, chown -1 -1 has precedent in this file, fs.lchmod is guarded, and path.toNamespacedPath handles the Windows mkdir result.

Extended reasoning...

Overview

This PR fixes a Node.js compatibility gap in src/js/node/fs.ts: the callback form of 22 void fs operations was passing (null, undefined) on success where Node passes (null) alone. The root cause was nullcallback() returning callback.bind(undefined, null), which .then() then invokes with the resolved value as an extra trailing argument. The fix binds the existing callOnceWithNull helper instead, so the fulfillment value is discarded. mkdir gets its own handler (recursive mkdir may resolve with a path, which Node forwards), and cp flips to callOnceWithNullThen because Node implements it via util.callbackify and calls back with (null, undefined). The callOnceWithNull/callOnceWithNullThen declarations move next to nullcallback; being hoisted function declarations, this is purely organizational and their existing call sites in Dir#read/Dir#close are unaffected.

Security risks

None. This changes only the number of arguments passed to user-supplied callbacks on the success path of local filesystem operations. No new inputs are parsed, no validation is weakened, and the error path (.then(..., callback)) is untouched.

Level of scrutiny

Low-to-moderate. The source change is ~15 lines of pure-JS glue in the Node compat layer, mechanically derived from Node's documented FSReqCallback::Resolve semantics (with source links). The only observable difference is arguments.length / rest-param length in success callbacks — code reading arguments[1] still sees undefined. I traced each modified path: nullcallback callers all go through ensureCallback first so the wrapped callback is what gets bound; the mkdir closure captures the already-wrapped callback; cp was already using callOnceWithNull.bind(null, callback) so switching to callOnceWithNullThen is a one-token swap.

Other factors

Test coverage is thorough: an it.each table exercises every affected operation (including fd-based ones with proper try/finally cleanup) asserting toStrictEqual([null]), plus dedicated cases for recursive mkdir returning the created path and cp's [null, undefined]. All needed imports (fs, path, tempDir, openSync, closeSync, mkdirSync, join) are already present in the test file. Cross-platform concerns are handled: chown/lchown/fchown use -1, -1 (already precedented at line ~5445 without a Windows skip), lchmod is added conditionally, and the recursive-mkdir assertion uses path.toNamespacedPath for Windows. The PR description documents verification against Node v26.3.0 for every operation and full/related test-file passes. No prior human review comments to address.

github-actions · 2026-08-13T07:24:22Z

This PR may be a duplicate of:

fs: run node:fs callbacks from the event loop, not a promise reaction #33366 - Rewrites the same nullcallback/promise-reaction layer in src/js/node/fs.ts to native *Cb callbacks and claims the same fixes (void ops call back with one argument and null instead of undefined, recursive mkdir passes the created path), so it subsumes this change.
node:fs: accessSync() returns undefined, promises.access() resolves to undefined, symlink callback gets null #38065 - Same file, same bug and same mechanism (binding callOnceWithNull so the promise fulfillment value never reaches the callback, asserted with toStrictEqual([null])), applied to access/symlink instead of the 22 nullcallback ops.

🤖 Generated with Claude Code

robobun · 2026-08-13T07:26:35Z

Neither of the flagged PRs covers this change:

node:fs: accessSync() returns undefined, promises.access() resolves to undefined, symlink callback gets null #38065 fixes fs.access and fs.symlink, the two wrappers that hand the native promise's value straight to the callback. It does not touch nullcallback(), mkdir or cp, which is everything this PR changes. The two PRs edit different lines of src/js/node/fs.ts and apply cleanly in either order (that PR's new access/symlink lines call the same callOnceWithNull helper this PR moves to the top of the file, still a hoisted declaration).
fs: run node:fs callbacks from the event loop, not a promise reaction #33366 is a rewrite of the callback layer onto native callbacks to fix nextTick ordering; the one-argument shape falls out of that as a side effect. It has had merge conflicts since July, and this PR is the few-line fix for the argument shape on the current code. If fs: run node:fs callbacks from the event loop, not a promise reaction #33366 lands later it removes these call sites anyway; the new test table still applies to it.

Use the module's captured Function.prototype.bind for cp, as nullcallback does, so a user replacing Function.prototype.bind cannot intercept the internal handler. Also cover chmod, mkdir and cp in the callback-throw table, one for each way a void operation now reaches its callback.

github-actions · 2026-08-13T18:14:09Z

+// The resolved value must not reach the callback: node's void operations call
+// back with `null` alone (FSReqCallback::Resolve drops an undefined result),
+// https://github.com/nodejs/node/blob/v26.3.0/src/node_file.cc#L736-L741


If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code

github-actions · 2026-08-13T18:14:10Z

+      // Only a recursive mkdir that created something resolves with a path;
+      // node passes it along and otherwise calls back with `null` alone.


If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code

github-actions · 2026-08-13T18:14:10Z

+  // Unlike the other void operations, node's fs.cp is util.callbackify(cpFn),
+  // so a successful copy calls back with (null, undefined):
+  // https://github.com/nodejs/node/blob/v26.3.0/lib/fs.js#L1098


If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code

claude

LGTM — narrow, well-tested Node.js compat fix.

What was reviewed:

nullcallback now binds callOnceWithNull so the promise's resolved value is dropped and void ops call back with exactly (null); guardCallback forwards arity via arguments.length switch so the count survives the wrapper.
mkdir's inline handler and cp's callOnceWithNullThen bind both go through the already-guarded callback, and the added rows in the uncaughtException table cover all three new success-path shapes.
callOnceWithNull/callOnceWithNullThen are hoisted function declarations, so moving them above Dir is safe; FunctionPrototypeBind is read at call time, no TDZ.
Test table uses toStrictEqual (distinguishes [null] from [null, undefined]), guards lchmod on availability, uses -1,-1 for chown so it runs unprivileged, and toNamespacedPath for the recursive-mkdir assertion on Windows.

The three comment-cop flags fire on 2–3-line comments that cite the upstream node source for why mkdir/cp deliberately differ from the other void ops — that's the reference-implementation citation the repo's own review guidance asks for, not a workaround justification, so I'm treating them as false positives.

Extended reasoning...

Overview

Changes nullcallback() in src/js/node/fs.ts from callback.bind(undefined, null) (which forwarded the promise's resolved value as a second argument) to callOnceWithNull.bind(undefined, callback) (which drops it), so the 21 void node:fs operations call back with exactly (null) on success like Node. mkdir gets an inline handler that forwards firstDirCreated only when defined (recursive mode), and cp switches to callOnceWithNullThen so it calls back with (null, undefined) matching node's util.callbackify(cpFn). The two helpers move from below cp to next to nullcallback; both are hoisted function declarations so Dir.prototype.read/close still resolve them. The test file gains a 25-row toStrictEqual([null]) table plus dedicated cases for recursive mkdir and cp, and three new rows in the existing uncaughtException table covering each new success-path shape.

Security risks

None. Success-path callback arity only; no input parsing, no error-path changes, no auth/crypto/permissions.

Level of scrutiny

Low-to-medium. ~15 lines of net source change in the JS shim layer over the native binding; the mechanism (.bind prepends bound args and forwards call-time args) is well understood and the fix is the minimal reshape. Verified guardCallback (internal/shared.ts:163) forwards the exact argument count via a switch on arguments.length, so callback(null) reaches the user callback as one argument and callback(null, undefined) as two — the arity fix is not undone by the guard wrapper. FunctionPrototypeBind is the captured primordial already used by the old nullcallback, read at call time so no TDZ. No CODEOWNERS cover the touched files.

Other factors

Tests are thorough: every affected op has a row, toStrictEqual is used specifically because toEqual would not catch the extra undefined, lchmod is gated on availability, chown/lchown/fchown use -1,-1 so they succeed unprivileged, fd-based cases close in finally, each case runs in a fresh using tempDir, and the recursive-mkdir assertion uses path.toNamespacedPath for Windows. The uncaughtException table gains chmod/mkdir/cp so all three new fulfillment shapes are proven to still route callback throws to uncaughtException via guardCallback. The PR description shows the new table fails 24/27 on the released binary and passes on this branch, and the full fs.test.ts plus adjacent cp/mkdir/dir suites and the ported node tests pass.

The only outstanding review activity is three automated comment-cop flags on the 2–3-line source comments citing node_file.cc/lib/fs.js for why mkdir and cp diverge from the other void ops. Those are reference-implementation citations (encouraged by the repo's node-compat review guidance), not workaround justifications — without them a future reader would "fix" cp to match the others. I'm treating them as false positives rather than blocking on them.

… alone nullcallback() bound null as the first argument of the user callback, so the promise machinery appended the resolution value and the 22 operations built on it called back with (null, undefined). Node calls back with (null): FSReqCallback::Resolve drops an undefined result. async's waterfall and seq hand the extra undefined to the next task as its first argument, so writeFile -> chmod -> unlink threw ERR_INVALID_ARG_TYPE. The helper now applies node's rule: (null) when the result is undefined, (null, result) otherwise, which is what a recursive mkdir needs. access and symlink use the helper and no longer inline its body. The test table is taken from #38085, with rows added for access and both symlink overloads.

robobun · 2026-09-12T06:36:41Z

#40916 now contains the nullcallback() fix and the voidOps test table from this PR (commit 8ed7483). CI passed on it (build #114573, 181 of 181 jobs). The shape of the fix differs. One handler follows FSReqCallback::Resolve: cb(null) when the result is undefined, cb(null, result) otherwise. So mkdir needs no handler of its own.

The fs.cp change in this PR is not in #40916. After #40916 merges, that change is the only part of this PR that is still needed.

…ction (#43728) ### Problem - Inside an async `node:fs` callback, a microtask the callback queues runs before a `process.nextTick()` it queues. Node runs the nextTick first: it prints `cb0 tick0 mt0 cb1 tick1 mt1`, and bun prints `cb0 tick0 mt0 cb1 mt1 tick1`. - The binding only returns promises, so `src/js/node/fs.ts` ran each callback from a promise reaction. A reaction is a microtask, so the callback's microtasks ran before the tick queue. ### Fix - The binding gets a third arm, `xCb(callback, ...args)`. The task keeps the callback in a `Strong` and calls it from the completion with `EventLoop::run_callback`, as `node:crypto` does. - `run_callback` then runs the tick queue and the microtasks, which is node's order. A callback API creates no promise, reaction or microtask. - `node:fs/promises` stays on the promise arm. Callback arguments do not change. - Verified: `test/js/node/fs/fs.test.ts` (49 new tests), node's `test-fs-*` files, Windows x64. ### Background - A checkpoint runs the tick queue (`process.nextTick`), then the microtask queue (promise reactions, `queueMicrotask`). Node calls an fs callback from the event loop, outside both. - `fs.promises` forwards every user argument, so a trailing function cannot select the callback arm. The arm needs its own entry points. ### Downsides - Seven callback APIs are JS promise chains with no binding operation (`cp`, `rm`, recursive `rmdir`, `Dir.read`, `Dir.close`, `glob`, `opendir`). They still use `process.nextTick`, so they report a `TickObject` to `async_hooks` and wait on a replaced `process.nextTick`. - Binary size: stripped Linux x64 release `bun` +53,248 bytes (+0.066%), `.text` +49,152, for 41 new host functions. - `fs.access` allocates one more closure per call (1 → 2), for a JS adapter that keeps its callback arguments identical. Others allocate fewer (Notes). <details><summary>Notes</summary> **Repro** ```js import fs from "node:fs"; const seq = []; let n = 0; function cb() { const i = n++; seq.push("cb" + i); process.nextTick(() => seq.push("tick" + i)); queueMicrotask(() => seq.push("mt" + i)); } for (let k = 0; k < 2; k++) fs.stat(".", cb); setTimeout(() => console.log(seq.join(" ")), 200); ``` The first callback of a process looks correct without the fix. The tick queue does not exist until the first `process.nextTick()` call. While it does not exist, the `onEachMicrotaskTick` hook (`ZigGlobalObject.cpp`, `checkIfNextTickWasCalledDuringMicrotask`) drains it right after the microtask that created it. **History of this PR** The first version kept the promise and passed the callback to `process.nextTick` from the reaction. A maintainer asked for a native callback arm instead ([review](#43728 (review))). The hop kept the whole promise path and added a tick. It also had two costs that the native arm removes for the operations of the binding, both measured on one `fs.stat` callback: | | released bun | hop | native arm | node v26.3.0 | |---|---|---|---|---| | `async_hooks` `init` events | `{}` | `{"TickObject":1}` | `{}` | `{"FSREQCALLBACK":1}` | | `process.nextTick` replaced before `node:fs` loads | callback runs | callback held | callback runs | callback runs | Bun reports no `FSREQCALLBACK` resource, before or after this PR. #33366 was an earlier native attempt, closed as stale without a review. **Shape of the callback arm** - `FsCompletion { Promise(JSPromiseStrong), Callback(Strong) }` replaces the promise in `AsyncFSTask`, the recursive readdir task and the Windows `UVFSRequest`. `NewAsyncCpTask` keeps its promise, because `fs.cp` is a JS promise chain. - The callback is stored with `with_async_context_if_needed` and called with `ContextId::NONE`, the pairing `run_callback` documents for a callback that carries its own `AsyncLocalStorage` frame. - An error found before the operation is scheduled (an aborted signal, a path that is too long) goes to the callback on the next tick. The callback goes in bare, because `Process::queueNextTick` asserts against an `AsyncContextFrame`. - The task releases its arguments before it calls back. They pin their buffers, and the callback now runs inside the completion, so a transfer of the buffer from the callback would copy it. Main and node detach it. The Windows `UVFSRequest` takes the completion out, frees the task, and then calls back. It takes `*mut Self` for that reason. - `FsCompletion::reject` builds the error value as `JSPromise::reject` does: it creates an out-of-memory error and does not take one, it returns a termination, and it unwraps a thrown `Exception` cell to its `Error`. - The entry points are named `xCb`, not `xCallback`. The class generator names the C++ host wrapper of a method `x` `Prototype__xCallback`, so a method named `xCallback` collides with it. - Ten operations keep a small JS adapter so that their argument lists stay identical: `access`, `close`, `exists`, `read`, `write` (both overloads), `readv`, `writev`, `symlink`. `fs.symlink` with a callback that is not callable stays on the promise arm, as before. - `internal/trace_events.ts` wraps `binding[method + "Cb"]` for each operation in its existing table, so `node.fs.async` spans still close. Without this, `test-trace-events-fs-async.js` gets no traces. **Size and performance** Two release builds from one tree and toolchain, each in its own build directory: main `bf80d21` and this PR. Linux x64. | | main | PR | delta | |---|---|---|---| | `bun` (stripped) | 80,770,592 | 80,823,840 | +53,248 (+0.066%) | | `bun-profile` | 169,212,560 | 169,315,968 | +103,408 (+0.061%) | | `.text` | 58,086,812 | 58,135,964 | +49,152 | | `.rodata` | 19,821,204 | 19,825,364 | +4,160 | | `.data`, `.data.rel.ro`, `.bss` | | | 0 | JS heap objects allocated per operation. This count is exact and has no timing in it: the script starts 1000 operations in one synchronous turn and reads `heapStats()` in the same turn. No operation can complete before control returns to the event loop, so each one is pending and roots what it allocated. Three runs of each binary give identical numbers. | per operation | `Promise` | `Function` | |---|---|---| | callback `fs.stat` | 2 → 0 | 2 → 1 | | callback `fs.readFile` | 2 → 0 | 2 → 1 | | callback `fs.read` | 2 → 0 | 3 → 2 | | callback `fs.access` | 2 → 0 | 1 → 2 | | `fs.promises.stat` | 3 → 3 | 0 → 0 | | `fs.promises.readFile` | 3 → 3 | 0 → 0 | On main each callback operation allocates two promises: the one the binding returns, and the one `.then()` derives. Eight operations keep a JS adapter so that their callback arguments stay identical. For `fs.access` the adapter is one more closure than main, which passed the callback to `.then()` directly. For `fs.read` it replaces two closures that main created. I measured only these two of the eight. Process CPU time per operation (user + system, µs), 200,000 operations, 256 in flight, 10 alternating runs per binary. This is statistical, not exact. The spread between runs of one binary is 2 to 4 µs, which is larger than the differences, and the distributions overlap. Read it as a direction. | | main median | PR median | main min | PR min | |---|---|---|---|---| | callback `fs.stat` | 12.36 | 10.41 | 10.27 | 8.46 | | callback `fs.read` | 10.47 | 9.50 | 8.67 | 8.64 | | `fs.promises.stat` | 11.51 | 11.88 | 8.57 | 9.79 | `fs.stat` is about 16% cheaper by the median and by the minimum. `fs.read` is not conclusive: 9% by the median, no difference by the minimum. `fs.promises.stat` is the control, because its code path and its allocations did not change. Its 3% difference shows the noise of the method, and it gives no evidence of a regression. I could not count CPU instructions on this machine. `valgrind` has no install candidate, and `perf_event_open` returns `EPERM`. **Not in this PR** - `dns.lookup` has the same order problem (`cb mt tick`). - Void operations call back with `(null, undefined)` where node passes `(null)`. Same on main, #38085 changes it. - `fs.symlink` calls back with `(undefined)` on success. Same on main, #40916 changes it. - `fs.fstat`, `fs.read` and the 4-argument `fs.symlink` do not check the callback synchronously as node does. Same on main. - `fs.readFile` with a signal that is already aborted calls back synchronously in node and asynchronously in bun. **Verification** - `test/js/node/fs/fs.test.ts`: the 45 order tests give 0 pass with main's `src/` and 45 pass with this PR. The 4 transfer tests guard a bug that an earlier commit of this PR had (6174ba5 fails them with `source: 8`). They pass on main. Whole file: 614 pass, 0 fail. - The same cases as a plain script pass on node v26.3.0. - 15 other files in `test/js/node/fs/`: 240 pass, 0 fail. `abort-signal-leak-read-write-file.test.ts` is a 300 s timeout under the ASAN debug build and uses only `fs.promises`. - Node's `test/parallel` files for fs, trace events, `util.promisify`, `util.callbackify`, http2 `respondWithFile`, `Utf8Stream`, `stream.pipeline`, readline and `FileHandle`: 413 files, all pass. `test-fs-cp-async-file-url.mjs` needs the working directory `test/js/node`. - `AsyncLocalStorage` stores reach the callback on 10 paths (native, adapter, error, recursive readdir, the early abort path, and the JS chains), the same as node. - `bun run rust:check-all`: 12 of 12 targets. - Windows x64 debug build: the order tests are 42 pass, 0 fail and the transfer tests 4 pass, on the final commit. An earlier run of the whole file was 598 pass, 2 fail. Both fail the same way on main without this PR on the same machine: `keeps a non-throwing callback in the same place in the event loop` (`setImmediate` runs before the `fs.stat` callback) and `readSync > works on large files` (5 s timeout). </details>  --- **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  --------- Co-authored-by: Alistair Smith <hi@alistair.sh>

robobun · 2026-09-23T18:50:50Z

Closing in favor of #40916.

#43728 (merged 2026-09-22, fe927c3) moved the node:fs callbacks to the native completion. It removed nullcallback() and callOnceWithNull from src/js/node/fs.ts. Every source hunk of this PR patches those helpers, or the mkdir and cp wrappers built on them. The diff no longer applies to main.

The defect is still on main. A build of main at 6d504dd prints 2 [ null, undefined ] for the chmod example in the description. Node v26.3.0 prints 1 [ null ]. #40916 carries the same fix and the voidOps test table from this PR. The rework of #40916 onto the native completion (FsCompletion::resolve in src/runtime/node/node_fs.rs) is in progress. Its status comment has the details.

One part of this PR is not covered there. fs.cp calls back with (null), and Node calls back with (null, undefined). That can be its own change.

github-actions Bot added the claude label Aug 13, 2026

claude Bot reviewed Aug 13, 2026

View reviewed changes

github-actions Bot reviewed Aug 13, 2026

View reviewed changes

claude Bot reviewed Aug 13, 2026

View reviewed changes

robobun mentioned this pull request Sep 11, 2026

node:fs: apply node's callback argument rule in the native completion, and make access give undefined #40916

Open

robobun mentioned this pull request Sep 21, 2026

node:fs: call callbacks from the native completion, not a promise reaction #43728

Merged

robobun closed this Sep 23, 2026

Navigation Menu

Repository navigation

node:fs: call void operation callbacks with (null) alone, as node does - #38085

node:fs: call void operation callbacks with (null) alone, as node does#38085
robobun wants to merge 2 commits into
mainfrom
farm/7e77c11b/fs-void-callback-args

robobun commented Aug 13, 2026

Uh oh!

coderabbitai Bot commented Aug 13, 2026 •

edited

Loading

Review limit reached

Uh oh!

robobun commented Aug 13, 2026 •

edited

Loading

Uh oh!

robobun commented Aug 13, 2026 •

edited

Loading

Uh oh!

claude Bot left a comment

Uh oh!

github-actions Bot commented Aug 13, 2026

Uh oh!

robobun commented Aug 13, 2026

Uh oh!

github-actions Bot Aug 13, 2026

Uh oh!

github-actions Bot Aug 13, 2026

Uh oh!

github-actions Bot Aug 13, 2026

Uh oh!

claude Bot left a comment

Uh oh!

robobun commented Sep 12, 2026

Uh oh!

robobun commented Sep 23, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

		// Only a recursive mkdir that created something resolves with a path;
		// node passes it along and otherwise calls back with `null` alone.

Conversation

robobun commented Aug 13, 2026

Problem

Fix

Background

Uh oh!

coderabbitai Bot commented Aug 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review limit reached

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

claude Bot left a comment

Choose a reason for hiding this comment

Overview

Security risks

Level of scrutiny

Other factors

Uh oh!

github-actions Bot commented Aug 13, 2026

Uh oh!

robobun commented Aug 13, 2026

Uh oh!

github-actions Bot Aug 13, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Aug 13, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Aug 13, 2026

Choose a reason for hiding this comment

Uh oh!

claude Bot left a comment

Choose a reason for hiding this comment

Overview

Security risks

Level of scrutiny

Other factors

Uh oh!

robobun commented Sep 12, 2026

Uh oh!

robobun commented Sep 23, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

coderabbitai Bot commented Aug 13, 2026 •

edited

Loading

robobun commented Aug 13, 2026 •

edited

Loading

robobun commented Aug 13, 2026 •

edited

Loading