Skip to content

node:fs: call back before return for an already-aborted signal in readFile, writeFile and appendFile - #47489

Open
robobun wants to merge 1 commit into
mainfrom
robobun/fab9fac1/fs-file-abort-before-return
Open

robobun wants to merge 1 commit into
mainfrom
robobun/fab9fac1/fs-file-abort-before-return

Conversation

@robobun

@robobun robobun commented Oct 30, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

  • fs.readFile, fs.writeFile and fs.appendFile call back on the next tick when options.signal is already aborted. Node v26.12.1 calls back before the call returns (checkAborted), so a throw from the callback reaches the caller.
  • If the process exits first, the callback never runs.
  • Cause: reject_before_schedule (src/runtime/node/node_fs_binding.rs:136) queues every early error with process.nextTick.

Fix

  • For an aborted signal the binding returns the AbortError and queues nothing. fs.ts calls the program's callback with it.
  • A path that is too long and fs.readFile(fd) stay on the next tick, as in node.
  • Verified: test/js/node/fs/fs-file-signal.test.ts (51 cases, main fails 37), trace-events.test.ts, one --aot program, fs.test.ts, 357 node fs tests.
  • Self-reviewed: 40 concerns raised, 40 addressed.

Background

  • The binding holds the callback inside guardCallback, a wrapper that turns a throw into an uncaughtException. So fs.ts calls the callback itself.
  • bun build --compile --aot reads each binding's result type from node_fs_binding.bind.ts. The three now return undefined or an object.
  • Considered a check in fs.ts: each call reads options.signal twice. Considered a native call of an unwrapped callback: every fs completion then needs a native uncaught report.

Downsides

  • The callback runs inside the call, and a throw from it reaches the caller. Both are node's behaviour.
  • Each callback call of the three tests the result: +43 instructions with the JIT off, +13 with it. fs.readFile grows from 74 to 91 bytecode bytes (DFG inline limit: 80).
  • Not fixed: with an aborted signal, an invalid path or flag still throws. Node calls back.
Notes

Where this comes from. No Bun user reported it. A comparison with node v26.12.1 found it. #43728 and #47310 both list it as not done. #47310 makes fs.stat, fs.lstat and fs.fstat call back before they return, with a helper in fs.ts. One maintainer decision on before-return delivery applies to both PRs.

Repro.

const fs = require("node:fs");
for (const [name, call] of Object.entries({
  readFile: cb => fs.readFile(__filename, { signal: AbortSignal.abort() }, cb),
  writeFile: cb => fs.writeFile(__filename + ".tmp", "x", { signal: AbortSignal.abort() }, cb),
  appendFile: cb => fs.appendFile(__filename + ".tmp", "x", { signal: AbortSignal.abort() }, cb),
})) {
  let returned = false;
  call(err => console.log(name, err.name, returned ? "after the call returned" : "before the call returned"));
  returned = true;
}
output for each of the three
node v26.12.1, v26.3.0 AbortError before the call returned
main f7607a3d35 AbortError after the call returned
this PR AbortError before the call returned

fs.writeFile(p, "x", { signal: AbortSignal.abort() }, () => process.exit(3)); process.exit(0) exits with 3 in node and here, and with 0 on main. A call made inside an 'exit' listener behaves the same way: main never runs the callback.

What changes. A 46-row probe compares each call form with node v26.12.1 (what is thrown, what the callback gets, and when). 34 rows differ on main and 18 here. The 16 rows that change are the 12 timing rows and the 4 rows where the callback throws. fs.writeFile(fd) and fs.appendFile(fd) are among them: node checks the signal of an fd in writeAll.

What stays different from node. Each row differs on main too, except the second one.

what where
With an aborted signal, an invalid path or flag throws. Node checks the signal first and calls back (11 probe rows, and the promise forms) The check order is the statement order of args::ReadFile::from_js and args::WriteFile::from_js_with_default_flag. #46789 replaces that parse API, and #46575 and #43563 change the same order
A flag getter of fs.readFile that aborts the signal: the callback runs before the call returns. Node reads flag after its check, so it calls back later. Main calls back on the next tick. One case has expectFailure for it same
fs.readFile(fd, { signal }): AbortError on the next tick. Node defers this form and then loses the abort: it calls back with (null, <empty Buffer>) kept on purpose
fs.stat, fs.lstat, fs.fstat #47310
fs.exists with an invalid path: node calls back before it returns, and a throw reaches the caller. Bun calls back later, and the throw is an uncaughtException no PR
A signal that is aborted while a failing call is in flight: Bun reports the AbortError, node the error of the operation no PR
{ signal: null } and objects that look like a signal #39274
Node records no node.fs.async trace span for an aborted call. Bun records a complete one, as on main not changed

The first shape of this change. It was withdrawn after the self-review, before a push. It called the program's callback from native code, without guardCallback, and it sent every fs completion through a new native call that reports a throw inside the callback's async context. It also kept the error of a path or a flag during the parse, so that the AbortError could win. The review found three changes in one: the kept error is built on ArgumentsSlice, which #46789 deletes, it changed fs.promises.readFile for a flag getter that aborts the signal, and the native report is a new primitive with one caller. That shape measured 1,798 instructions and 3 JS cells less per fs.readFile callback call (JIT off). That is the cost of guardCallback, and removing it is a separate change.

Cost. Release builds of main f7607a3d35 and of this branch, from one tree, Linux x64.

  • Instructions of the first thread in user mode per fs.readFile(f, cb) with no signal (perf_event_open, slope between 20,000 and 60,000 calls, least of 5, two rounds): JIT off 10,249.8 and 10,252.4 on main, 10,294.9 and 10,294.4 here. With an options object: 9,562.7 and 9,560.8, 9,605.3 and 9,604.2 here. Default JIT: 7,170.6 and 7,178.2, 7,194.8 and 7,187.1 here. With options: 6,391.1 and 6,391.3, 6,403.6 and 6,404.5 here.
  • JS cells per call: the same as main (heapStats, 2,000 calls).
  • Bytecode: readFile 74 to 91 bytes (16 to 20 instructions), writeFile 77 to 94, appendFile 83 to 100. The call-inline limit is 80 for the DFG and 170 for the FTL.
  • Binary: 138,270,840 to 138,266,744 bytes, .text 4,352 bytes less. reject_before_schedule is now #[cold] and out of line. It was inlined into each of the 40 instantiations of run_async.
  • Host functions: +0. Table of built-ins: the result type of three rows.
  • require("node:fs") at start: 16,520,636 to 16,522,864 instructions (+0.013%). hello.js differs by 0.05% between the two binaries. The generated node/fs.js is 142 bytes larger.
  • A callback that makes the call again on an AbortError now nests. With no end it stops at a RangeError at depth 12,864 (release) and 10,917 (debug with ASAN). Node v26.12.1 stops at 1,901. On main such a loop never nests.

Tests.

  • fs-file-signal.test.ts uses node:test and node:assert, so the same file runs under node: 51 cases, 51 pass on v26.12.1. Its last case runs it there when that node is installed. On main 37 of its cases fail.
  • It covers the path, Buffer, URL and fd forms, a path that does not exist or is too long, this, the argument count, the cause, the AsyncLocalStorage store, the order against a tick and a microtask, util.promisify, a nested call, a process that exits, a throwing callback, arguments that still throw, options that abort the signal while they are read, and what stays after the return.
  • trace-events.test.ts: the new case fails on main, and also when wrapFsCallbackMethod does not end the span (4 begin events and no end event for one call).
  • test/aot/parallel/native-callbacks/callbacks-fs-signal-aborted-before-the-call.ts must print under --compile --aot what it prints under plain bun. On a build without the result types in node_fs_binding.bind.ts the compiled program never calls the readFile and writeFile callbacks, and this program fails.
  • Also run with this build: fs.test.ts (631 pass), promises.test.js, fs-path-length.test.ts, the new cases with BUN_JSC_validateExceptionChecks=1, 357 of node's tests (test-fs-*, trace events, util.promisify, util.callbackify, FileHandle, stream.pipeline), test/internal/builtins-table/, test/aot/builtins/, and bun src/codegen/check-types-of-modules.ts.
  • bun run rust:check-all for x86_64-pc-windows-msvc and aarch64-apple-darwin: both pass. Before the push, no test ran on Windows or macOS. In CI the three new tests pass on Windows x64, Windows aarch64, macOS x64 and macOS aarch64.

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-file-signal.test.ts

@robobun
robobun requested a review from alii as a code owner October 30, 2026 18:27
@robobun

robobun commented Oct 30, 2026 •

Copy link
Copy Markdown
Collaborator Author

Status

Reproduced on main (f7607a3d35) and on node v26.12.1 with the script in the Notes of the description:

  • main prints AbortError after the call returned for fs.readFile, fs.writeFile and fs.appendFile.
  • node v26.12.1 and this branch print AbortError before the call returned.

test/js/node/fs/fs-file-signal.test.ts fails 37 of its 52 cases on main and passes on this branch. The same file passes under node v26.12.1.

CI (build 135864) has finished: 183 jobs passed and 5 failed.

  • Each of the 5 failed jobs fails on one test only: test/aot/engine/ir-6.test.ts (pipeline/gc/custom-accessor-reached-computed-key.ir:220). It fails the same way on main (build 135526), and this PR does not touch it.
  • Main is still at f7607a3d35, the base of this branch, so a new run cannot pass yet.
  • No job reports a failure of the tests of this PR. I read the logs of the platforms that I cannot run locally: fs-file-signal.test.ts has 52 pass on Windows 2019 x64, Windows 11 aarch64, macOS aarch64 and macOS x64. That count includes the case that runs the file under node. trace-events.test.ts and the --aot program pass on the same four platforms.
  • The other failures in the build passed on a retry.

PR: #47489

@robobun

robobun commented Oct 30, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 12:45 PM PT - Oct 30th, 2026

❌ @robobun, your commit e710ab5 has 1 failures in Build #135864 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 47489

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

bun-47489 --bun

@coderabbitai

coderabbitai Bot commented Oct 30, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review.

Recent review info
Run configuration
Commits

Reviewing files that changed from the base of the PR and between f7607a3 and e710ab5.

Files selected for processing: 10
  • src/codegen/builtins/host/runtime-dependencies.builtins.ts
  • src/js/internal/trace_events.ts
  • src/js/node/fs.ts
  • src/js/private.d.ts
  • src/runtime/node/node_fs.rs
  • src/runtime/node/node_fs_binding.bind.ts
  • src/runtime/node/node_fs_binding.rs
  • test/aot/parallel/native-callbacks/callbacks-fs-signal-aborted-before-the-call.ts
  • test/js/node/fs/fs-file-signal.test.ts
  • test/js/node/trace_events/trace-events.test.ts

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

Walkthrough

Already-aborted signals can return errors from selected filesystem bindings. The callback wrappers now handle those errors, and the bindings select immediate or next-tick callback delivery based on the parsed arguments. Tests cover callback behavior, promise APIs, and trace events.

Changes

Filesystem abort callback flow

Layer / File(s) Summary
Early error delivery
src/runtime/node/node_fs.rs, src/runtime/node/node_fs_binding.rs, src/runtime/node/node_fs_binding.bind.ts, src/js/private.d.ts, src/codegen/builtins/host/runtime-dependencies.builtins.ts
The binding carries an EarlyDelivery mode with rejected async arguments. Callback calls either return the error before invoking the callback or invoke the callback on the next tick. ReadFile selects delivery based on whether its argument is a file descriptor or path. Callback declarations and documentation describe returned errors for appendFile, readFile, and writeFile.
JavaScript wrapper and trace handling
src/js/node/fs.ts, src/js/internal/trace_events.ts
The three filesystem wrappers invoke the callback when the binding returns an error. The trace wrapper ends async events when the binding returns a non-undefined value.
Abort behavior tests
test/aot/parallel/native-callbacks/callbacks-fs-signal-aborted-before-the-call.ts, test/js/node/fs/fs-file-signal.test.ts, test/js/node/trace_events/trace-events.test.ts
Tests cover already-aborted and live signals, callback timing and context, validation, promise behavior, file-descriptor reads, and matching filesystem trace span begin and end counts.

Suggested reviewers: jarred-sumner, dylan-conway, cirospaciari

Merge Risk: Minimal · up to e710a

The already-aborted callback behavior has matching wrapper handling and targeted tests. No concrete merge-blocking issue remains; merge after normal checks pass.

Pre-merge checks: Passed 4
Check name Status Explanation
Title check Passed The title clearly and specifically summarizes the main change: synchronous callback delivery for already-aborted signals in readFile, writeFile, and appendFile.
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Description check Passed The description follows the repository template with both required sections, "What does this PR do?" (titled Problem, Fix, Background, Downsides) and "How did you verify your code works?" (titled Veri…
Full details: Description check

Explanation

The description follows the repository template with both required sections, "What does this PR do?" (titled Problem, Fix, Background, Downsides) and "How did you verify your code works?" (titled Verified, Tests, Cost). It states the problem, the fix, the trade-offs, and a detailed verification plan with concrete test counts and a reproduction script. Gaps: the verification section says no tests ran on Windows or macOS before the push, and the robobun evidence note flags that the platform-specific test file has not been run locally. CI covers those platforms, so this is a minor gap, not a missing section.

  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code review found no issues

No high-confidence issues detected in this change.

This branch has not been deployed

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant