You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
node:fs: keep options.signal only for the async forms of readFile and writeFile - #47993
This is the parse gate of #36485 alone. That PR conflicts with main.
Problem
fs.readFileSync and fs.writeFileSync throw EINTR: interrupted system call, access when options.signal is already aborted. The write does not happen. Node v26.3.0 reads and writes.
The option parsers (src/runtime/node/node_fs.rs:4547, :4645) kept the signal for a sync call. The thread pool's abort checks (:7503, :7833) then returned their internal EINTR value.
Fix
signal_option replaces both copies of the parse. It type-checks every signal and keeps it only for an async call. A debug assertion in run_sync checks that.
Node does the same with a valid signal: getOptions type-checks it, and the sync forms never read it again.
Verified: test/js/node/fs/fs.test.ts (16 new cases, 12 fail on main), promises.test.js, the AbortSignal leak test, 36 upstream test-fs-* files. Self-reviewed: 9 concerns raised, 9 addressed.
Background
Node documents no signal for the sync forms, but @types/node declares it on writeFileSync and appendFileSync.
The abort checks in NodeFS run on the thread pool. AsyncFSTask::then turns their EINTR value into an AbortError.
Considered a Flavor::Async test at the four checks: it costs every async call a branch.
Downsides
Code that relied on the EINTR throw to skip the I/O now does the I/O.
An async call with a signal runs 7 to 8 more parser instructions. Without a signal: +0. Code: +86 bytes.
Still unlike Node: signal: null is accepted and a signal-shaped object is rejected (Notes).
Notes
Origin. Found by a differential run of node:fs against Node v26.3.0. No user issue reports it. It needs an already-aborted signal on a sync form: a live signal already behaves like Node, and a call without signal runs no changed line.
Repro.
importfsfrom"node:fs";importosfrom"node:os";importpathfrom"node:path";constdir=fs.mkdtempSync(path.join(os.tmpdir(),"sync-signal-"));constat=name=>path.join(dir,name);constsignal=AbortSignal.abort();construn=fn=>{try{returnfn()??"ok";}catch(e){return`${e.code}, syscall ${e.syscall}`;}};constfile=name=>(fs.existsSync(at(name)) ? JSON.stringify(fs.readFileSync(at(name),"utf8")) : "(no file)");fs.writeFileSync(at("read"),"hello");fs.writeFileSync(at("existing"),"hello");console.log("readFileSync ",run(()=>fs.readFileSync(at("read"),{encoding: "utf8", signal })));console.log("writeFileSync, new file ",run(()=>fs.writeFileSync(at("new"),"W",{ signal })),"| file:",file("new"));console.log("writeFileSync, existing ",run(()=>fs.writeFileSync(at("existing"),"W",{ signal })),"| file:",file("existing"));console.log("appendFileSync, new file",run(()=>fs.appendFileSync(at("append"),"A",{ signal })),"| file:",file("append"));console.log("readFileSync, directory ",run(()=>fs.readFileSync(dir,{ signal })));fs.rmSync(dir,{recursive: true,force: true});
$ node repro.mjs # v26.3.0
readFileSync hello
writeFileSync, new file ok | file: "W"
writeFileSync, existing ok | file: "W"
appendFileSync, new file ok | file: "A"
readFileSync, directory EISDIR, syscall read
$ bun repro.mjs # main b1007f1177
readFileSync EINTR, syscall access
writeFileSync, new file EINTR, syscall access | file: ""
writeFileSync, existing EINTR, syscall access | file: "hello"
appendFileSync, new file ok | file: "A"
readFileSync, directory EINTR, syscall access
$ bun repro.mjs # this PR
readFileSync hello
writeFileSync, new file ok | file: "W"
writeFileSync, existing ok | file: "W"
appendFileSync, new file ok | file: "A"
readFileSync, directory EISDIR, syscall read
The same holds for the fd forms, a Buffer, flag: "a", an empty file and a 1 MB file. Of 11 sync rows with an aborted signal, 9 differ from Node on main and 0 with this PR.
Why the new file stayed empty.write_file_with_path_buffer opens with O_CREAT and without O_TRUNC (it resizes after the write). The abort check comes after the open.
Tests. The 16 cases: a path and an fd for each of the three sync forms, flags w / a / r+ / wx, AbortSignal.any(), an options getter that aborts the controller (4 keys), a call from inside the abort listener, EISDIR and ENOENT, one options object shared with fs.promises (still AbortError, no file created), a value that is not an AbortSignal for all 9 forms (still ERR_INVALID_ARG_TYPE, no file created), and a heap count after 3000 sync calls. The appendFileSync, invalid-signal and heap cases pass on main too: they pin what must not change. The 15 cases that need no Bun API also pass under Node v26.3.0, run through a small stand-in for bun:test and harness.
A temporary change that kept the signal on a sync parse trips the new assertion in run_sync (panic: a sync parse keeps no signal).
Measurements. Linux x64. Release builds of main b1007f1 and of this PR from one toolchain, unless a line says debug.
Instructions per sync call without a signal (perf_event_open, user instructions, N = 2e6, three runs, spread at most 1.5). readFileSync(p, "utf8") / readFileSync(p, { encoding }) / writeFileSync(p, "hello") / appendFileSync(p, "x") / statSync(p): 1949 / 3310 / 1641 / 1539 / 1537 on main, 1948 / 3310 / 1641 / 1539 / 1538 on this PR.
Instructions per sync call with a live signal: read 3394 to 3389, write 4153 to 4147, append 4068 to 4064.
Instructions inside the option parser for an async call (gdb single-step from entry to return, 4 calls, all equal). With a signal: readFile 2394 to 2401, writeFile 4027 to 4035. Without a signal: 2318 and 3931 on both.
AbortSignal FFI calls per sync call with a live signal (gdb breakpoint hit counts, debug builds, 1000 calls): read / write / append 1 / 1 / 1 (main 6 / 6 / 5). Per fs.promises call: 8 / 8 / 7 (main 8 / 8 / 7). Refs equal unrefs and increments equal decrements in every mode, also when the parse fails after it read the signal.
Syscalls per writeFileSync(path) (gdb catch syscall, N = 100, three agreeing runs), openat / write / ftruncate / close: no signal 1 / 1 / 1 / 1, live signal 1 / 1 / 1 / 1, aborted signal 1 / 1 / 1 / 1 (main with an aborted signal: 1 / 0 / 0 / 1).
Binary: size text is 140,289,307 bytes on both. The two parser symbols are 1029 / 1650 bytes (main 979 / 1614). That is +86 bytes, inside the padding of the section.
Live AbortSignal objects after 3000 sync calls with 1000 fresh signals: 2 (the test allows 10). After 300,000 calls: 1.
Wall clock, bench/snippets/read-file.mjs and write-file.mjs (mitata, 10 interleaved rounds, median of the round minimums). Read: 983 / 1053 / 2477 / 9766 ns on main, 989 / 1046 / 2475 / 9762 ns on this PR. Write: 3839 / 3827 / 4339 / 9356 ns on main, 3813 / 3848 / 4292 / 9376 ns on this PR. The round minimums of one binary spread by 72 to 1627 ns. No difference outside that.
signal_option is #[inline(always)]. As a plain function LLVM kept it out of line. Then each parse of an options object cost 1 more instruction, and a parse with a signal 12 to 13 more.
strace, perf and bloaty are not installed on this machine. gdb, a perf_event_open counter, size and nm stand in.
The Flavor::Async alternative also changes the signature of write_file_with_path_buffer, which the bundler calls.
Not in this PR. These differences from Node are the same on main and on this branch.
options.signal of readFileSync / writeFileSync
Node v26.3.0
Bun
null, ""
throws ERR_INVALID_ARG_TYPE
does the I/O
{ aborted: true }, { aborted: false }, a Proxy of a signal, Object.create(AbortSignal.prototype)
does the I/O
throws ERR_INVALID_ARG_TYPE
a rejected value, for example 1
The "options.signal" property must be an instance of AbortSignal.
The "signal" argument must be of type AbortSignal.
fs.readFile(fd, { signal: aborted }, cb) calls back with (null, "") in Node and with an AbortError in Bun. Node loses the error for a caller-owned fd. Not changed.
Related PRs on the same lines.#36485 (its two parse hunks and its sync test can go when it rebases on this), #39274, #46575.
Test placement. The heap check is a child process in the new describe block. It is not a new row in abort-signal-leak-read-write-file-fixture.ts, because #47542 rewrites that fixture around promise-returning calls.
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
…nd writeFile
`fs.readFileSync` and `fs.writeFileSync` threw `EINTR` with
`syscall: "access"` when `options.signal` was already aborted, and the
write did not happen. The parsers of `readFile` and `writeFile` kept the
signal for a sync call too, and the abort checks that are there for a
pool thread then returned their internal EINTR value to the sync caller.
Node's sync forms type-check `options.signal` and never read it again.
The two parsers now share `signal_option`. It type-checks the signal for
every form and keeps it (with its pending activity) only for an async
parse. A sync call takes no ref on the signal.
Reproduced on main b1007f1 (release and debug builds) and on 1.4.3-canary 367d939, against Node v26.3.0. The script is in the Notes of the description. readFileSync and writeFileSync with { signal: AbortSignal.abort() } throw EINTR, syscall access. Node reads and writes.
With this branch every row of that script matches Node. The 16 new cases in test/js/node/fs/fs.test.ts pass. 12 of them fail on main.
CI, build #137894, finished: no lane reports a failure in test/js/node/fs/fs.test.ts. Its summary shows 0 failures on Linux (glibc with ASAN, musl), Windows x64, Windows aarch64 and macOS aarch64. The macOS x64 lanes passed too.
The build is red on 27 test/aot/* files and on test/bundler/compile-aot-size.test.ts. This diff does not touch them. The same tests fail on the final builds of the most recently merged PRs, and main has not changed since this branch was pushed. A new CI run cannot turn them green until main is fixed.
No actionable comments were generated in the recent review.
Walkthrough
Filesystem read and write argument parsing now validates abort signals for sync and async calls. Synchronous calls do not retain valid signals. Tests cover sync behavior, async rejection, invalid signal values, and signal retention.
Changes
Filesystem abort-signal handling
Layer / File(s)
Summary
Shared signal parsing and sync assertion src/runtime/node/node_fs.rs, src/runtime/node/node_fs_binding.rs
A shared helper validates signal values and retains valid signals for async calls. Sync parsing does not retain signals, and run_sync asserts that the parsed arguments contain none.
Tests cover synchronous reads, writes, and appends with aborted signals, including descriptor operations, option getters, abort-listener reentrancy, and filesystem errors.
API validation and signal retention tests test/js/node/fs/fs.test.ts
Tests verify promise rejection for aborted signals, invalid signal errors across sync, callback, and promise APIs, and limited signal retention after repeated sync calls.
Possibly related PRs
oven-sh/bun#47489: Both PRs cover aborted-signal behavior for filesystem read, write, and append calls. This PR concerns how callback calls handle an already-aborted signal.
Suggested reviewers: dylan-conway, alii
Merge Risk: Minimal · up to 64cf8
The change separates synchronous signal handling from asynchronous cancellation, with tests covering the affected APIs. No concrete merge-blocking risk is established; merge after normal checks pass.
Pre-merge checks: 4
Check name
Status
Explanation
Title check
The title clearly describes the main change to signal retention.
Description check
The description explains the change and reports verification. It does not use the template headings, but it provides the required information.
Linked Issues check
Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check
Check skipped because no linked issues were found for this pull request.
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.
Reviewing files that changed from the base of the PR and between b1305dd and 64cf8b7.
Files selected for processing: 3
src/runtime/node/node_fs.rs
src/runtime/node/node_fs_binding.rs
test/js/node/fs/fs.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.
The reason will be displayed to describe this comment to others. Learn more.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline finding, I also checked that the new signal_option helper is ref-balanced against the old path: AbortSignal::ref_from_js is exactly from_js + opaque_deref(..).ref_() (src/jsc/AbortSignal.rs:231), and the release side (the Drop impls and scopeguards at src/runtime/node/node_fs.rs:4529-4555 and :4600-4635) is unchanged, so the async path still unrefs once. A grep confirms these two parsers are the only fs argument parsers that read signal, so no sibling site was missed.
Extended reasoning...
The change replaces two inline signal parses in the ReadFile/WriteFile argument parsers with a shared helper that returns None for sync calls, adds a debug_assert in run_sync, and adds a large test block; it touches no security-sensitive surface. The one inline finding (null/falsy signal accepted via get_truthy) is pre-existing behavior the author discloses, so the human judgment is whether that gap should ship separately.
#48124 is stacked on this branch (head 64cf8b7). It replaces the body of signal_option with Node's accept rule. The sync rule and the debug_assert! in run_sync stay as they are here.
Merge this one first. If this branch is rebased, #48124 needs the same rebase.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
This is the parse gate of #36485 alone. That PR conflicts with main.
Problem
fs.readFileSyncandfs.writeFileSyncthrowEINTR: interrupted system call, accesswhenoptions.signalis already aborted. The write does not happen. Node v26.3.0 reads and writes.src/runtime/node/node_fs.rs:4547,:4645) kept the signal for a sync call. The thread pool's abort checks (:7503,:7833) then returned their internalEINTRvalue.Fix
signal_optionreplaces both copies of the parse. It type-checks every signal and keeps it only for an async call. A debug assertion inrun_syncchecks that.getOptionstype-checks it, and the sync forms never read it again.test/js/node/fs/fs.test.ts(16 new cases, 12 fail on main),promises.test.js, the AbortSignal leak test, 36 upstreamtest-fs-*files. Self-reviewed: 9 concerns raised, 9 addressed.Background
signalfor the sync forms, but@types/nodedeclares it onwriteFileSyncandappendFileSync.NodeFSrun on the thread pool.AsyncFSTask::thenturns theirEINTRvalue into anAbortError.Flavor::Asynctest at the four checks: it costs every async call a branch.Downsides
EINTRthrow to skip the I/O now does the I/O.signal: nullis accepted and a signal-shaped object is rejected (Notes).Notes
Origin. Found by a differential run of
node:fsagainst Node v26.3.0. No user issue reports it. It needs an already-aborted signal on a sync form: a live signal already behaves like Node, and a call withoutsignalruns no changed line.Repro.
The same holds for the fd forms, a Buffer,
flag: "a", an empty file and a 1 MB file. Of 11 sync rows with an aborted signal, 9 differ from Node on main and 0 with this PR.Why the new file stayed empty.
write_file_with_path_bufferopens withO_CREATand withoutO_TRUNC(it resizes after the write). The abort check comes after the open.Node's code.
getOptionstype-checks the option for every form (utils.js#L340-L342).readFileSync(fs.js#L429-L486) andwriteFileSync(fs.js#L2386-L2437) never read it.Tests. The 16 cases: a path and an fd for each of the three sync forms, flags
w/a/r+/wx,AbortSignal.any(), an options getter that aborts the controller (4 keys), a call from inside the abort listener,EISDIRandENOENT, one options object shared withfs.promises(stillAbortError, no file created), a value that is not anAbortSignalfor all 9 forms (stillERR_INVALID_ARG_TYPE, no file created), and a heap count after 3000 sync calls. TheappendFileSync, invalid-signal and heap cases pass on main too: they pin what must not change. The 15 cases that need no Bun API also pass under Node v26.3.0, run through a small stand-in forbun:testandharness.A temporary change that kept the signal on a sync parse trips the new assertion in
run_sync(panic: a sync parse keeps no signal).Measurements. Linux x64. Release builds of main b1007f1 and of this PR from one toolchain, unless a line says debug.
perf_event_open, user instructions, N = 2e6, three runs, spread at most 1.5).readFileSync(p, "utf8")/readFileSync(p, { encoding })/writeFileSync(p, "hello")/appendFileSync(p, "x")/statSync(p): 1949 / 3310 / 1641 / 1539 / 1537 on main, 1948 / 3310 / 1641 / 1539 / 1538 on this PR.readFile2394 to 2401,writeFile4027 to 4035. Without a signal: 2318 and 3931 on both.fs.promisescall: 8 / 8 / 7 (main 8 / 8 / 7). Refs equal unrefs and increments equal decrements in every mode, also when the parse fails after it read the signal.writeFileSync(path)(gdbcatch syscall, N = 100, three agreeing runs), openat / write / ftruncate / close: no signal 1 / 1 / 1 / 1, live signal 1 / 1 / 1 / 1, aborted signal 1 / 1 / 1 / 1 (main with an aborted signal: 1 / 0 / 0 / 1).sizetext is 140,289,307 bytes on both. The two parser symbols are 1029 / 1650 bytes (main 979 / 1614). That is +86 bytes, inside the padding of the section.AbortSignalobjects after 3000 sync calls with 1000 fresh signals: 2 (the test allows 10). After 300,000 calls: 1.bench/snippets/read-file.mjsandwrite-file.mjs(mitata, 10 interleaved rounds, median of the round minimums). Read: 983 / 1053 / 2477 / 9766 ns on main, 989 / 1046 / 2475 / 9762 ns on this PR. Write: 3839 / 3827 / 4339 / 9356 ns on main, 3813 / 3848 / 4292 / 9376 ns on this PR. The round minimums of one binary spread by 72 to 1627 ns. No difference outside that.signal_optionis#[inline(always)]. As a plain function LLVM kept it out of line. Then each parse of an options object cost 1 more instruction, and a parse with a signal 12 to 13 more.strace,perfandbloatyare not installed on this machine. gdb, aperf_event_opencounter,sizeandnmstand in.Flavor::Asyncalternative also changes the signature ofwrite_file_with_path_buffer, which the bundler calls.Not in this PR. These differences from Node are the same on main and on this branch.
options.signalofreadFileSync/writeFileSyncnull,""ERR_INVALID_ARG_TYPE{ aborted: true },{ aborted: false }, aProxyof a signal,Object.create(AbortSignal.prototype)ERR_INVALID_ARG_TYPE1The "options.signal" property must be an instance of AbortSignal.The "signal" argument must be of type AbortSignal.null/""rows and the message. node:fs: accept AbortSignal-shaped objects in readFile/writeFile/watch #35275 (closed, not merged) had the signal-shaped objects. After this PR the check is one function.fs.promises.appendFile: Node rejects and appends nothing, Bun rejects withAbortErrorand appends in 100 of 100.fs.promises.writeFileto an existing file: Node leaves it empty, Bun keeps the old content in 97 and writes all of the data in 3. node:fs: run appendFile through writeFile's write path and report the { flush: true } fsync error #36485 has an abort check forappendFile. fs.writeFile: honor AbortSignal between write chunks #35021 (closed, not merged) had one between the chunks ofwriteFile. No open PR has thewriteFilepart.FileHandle#appendFiledrops the signal: fs.promises: forward options.signal from the FileHandle wrappers and validate it for iterable data #39272.fs.readFile(fd, { signal: aborted }, cb)calls back with(null, "")in Node and with anAbortErrorin Bun. Node loses the error for a caller-owned fd. Not changed.Related PRs on the same lines. #36485 (its two parse hunks and its sync test can go when it rebases on this), #39274, #46575.
Test placement. The heap check is a child process in the new
describeblock. It is not a new row inabort-signal-leak-read-write-file-fixture.ts, because #47542 rewrites that fixture around promise-returning calls.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