node:fs: validate options.signal in readFile, writeFile and watch like node

[robobun]

Problem

• fs.writeFileSync(p, "x", { signal: 1 }) throws The "signal" argument must be of type AbortSignal. Received type number (1); node v26.3.0 throws The "options.signal" property must be an instance of AbortSignal. Received type number (1). The code (ERR_INVALID_ARG_TYPE) already matches, the text differs in the name and in of type vs an instance of. Same for readFile/readFileSync, writeFile/appendFile and their sync forms, fs.promises.readFile/writeFile/appendFile, and FileHandle#readFile.
• fs.watch(p, { signal: 1 }) throws signal is not of type AbortSignal (right code, none of node's wording).
• The same three sites read the option with get_truthy, so { signal: null } and { signal: "" } are silently accepted and the operation runs; node rejects them with the same error (Received null, Received type string ('')), since only undefined means "no signal". This is the one user-observable behavior change in the PR.
• Cause: args::ReadFile::from_js and args::WriteFile::from_js_with_default_flag in src/runtime/node/node_fs.rs (lines 4077 and 4186 on main) each brand-check the value themselves and call throw_invalid_argument_type_value(b"signal", b"AbortSignal", ..); Arguments::from_js in src/runtime/node/node_fs_watcher.rs:704 does the same with a hand-written message. Node runs one function for all of them, validateAbortSignal(options.signal, "options.signal") from getOptions() (lib/internal/fs/utils.js#L340-L342, lib/internal/validators.js#L444-L451).
• The Rust ERR_INVALID_ARG_TYPE renderers (JSGlobalObject::throw_invalid_argument_type_value*) always print "<name>" argument, so even with the right name they could not produce node's property. The C++ renderer (Message::addParameter in ErrorCode.cpp) already applies node's rule; the Rust one never did.
• No issue reports this; it was found by diffing bun against node while fixing the neighbouring flush message. The upstream fs tests in the tree only assert the error code here, so they pass before and after; the tests below are what pins the text.

Fix

• validators.rs: new validate_abort_signal(ctx, value, name) -> JsResult<AbortSignalRef>, the port of node's validator: returns a ref to the native signal, otherwise throws ERR_INVALID_ARG_TYPE rendered as an instance of AbortSignal through throw_invalid_argument_type_value2. This is now the only place that decides what counts as a signal for the native fs code and how the rejection reads; node:fs: accept AbortSignal-shaped objects in readFile/writeFile/watch #35275 (accepting AbortSignal-shaped objects) would be a change to this one function instead of to three sites.
• The three sites read the option with get (undefined or missing means absent, anything else is validated, mirroring node's options.signal !== undefined) and call the helper; the per-site brand checks and messages are deleted. fs.watch's Arguments now holds the returned AbortSignalRef instead of a &AbortSignal borrowed from the JS value, and FSWatcher::init clones it; this removes the ensure_still_alive dance and an unsafe adopt, and the refcount stays balanced (2 live AbortSignal wrappers after GC for 10,000 watchers created with signals, same as bun 1.4.0; the fixture is in the details below).
• JSGlobalObject.rs: the three Rust renderers format the name through InvalidArgTypeName, node's rule ("x" argument, "a.b" property, a name already ending in argument verbatim; lib/internal/errors.js#L1407-L1414), the same rule the C++ renderer applies. Side effect: the 15 other Rust call sites that pass a dotted name (listed in the details below; every other caller passes a plain name and is unchanged) now also say property. For the node APIs among them that is node's text (fs.rmSync(p, { maxRetries: "a" }), { retryDelay: "a" }, new SocketAddress({ address: 1 }), { flowlabel: "x" } diff clean against node); no test in the tree pinned the old "<dotted>" argument wording. node: render the Rust validators' ERR_INVALID_ARG_TYPE messages like Node #38663 carries the same renderer change for a different set of callers; the text here is taken from it verbatim, and a trial merge in both orders is conflict-free and ends up with a single InvalidArgTypeName. It is duplicated rather than stacked so that neither PR has to wait for the other.
• Why this is correct: for node:fs node's output is the contract, and with this change a 40-call matrix (10 bad values through readFileSync and writeFileSync, { signal: 1 } through every sync/callback/promises/FileHandle entry point, fs.watch with 1, {}, 0, null, plus valid and undefined signals) is byte-identical to node v26.3.0, except for two entry points that never reach this code and are fixed elsewhere: FileHandle#appendFile/#writeFile rebuild their options in JS (fs.promises: forward options.signal from the FileHandle wrappers and validate it for iterable data #39272) and fs.promises.watch validates in JS through the C++ renderer (node errors: render a single class name in ERR_INVALID_ARG_TYPE as "an instance of" like node #38694 fixes its of type). Rejecting null/"" happens before any file is opened (the test checks the output file is not created).
• Deliberately not in this PR: node's getOptions() also validates options.signal for functions that then ignore it (readdir, readlink, realpath, mkdtemp, ...); this PR only changes the three sites that use the signal. Accepting AbortSignal-shaped objects stays with node:fs: accept AbortSignal-shaped objects in readFile/writeFile/watch #35275, which edits the same three sites on main; whichever of the two lands second has a mechanical rebase (with this PR the change becomes the helper's accept condition).
• Tests (fail on the current release, pass with this change):
  • test/js/node/fs/promises.test.js, options.signal validation matches node: exact message for ten non-signal values through readFileSync; { signal: null } through all 13 entry points that reach the binding (readFileSync, readFile, promises.readFile by path and by handle, FileHandle#readFile, writeFileSync, writeFile, promises.writeFile by path and by handle, appendFileSync, appendFile, promises.appendFile by path and by handle); undefined and a real signal still accepted; no I/O performed.
  • test/js/node/watch/fs.watch.test.ts: fs.watch with 1, null, "", {} and an AbortController; undefined and a real signal still create a watcher.
  • Also run with the change: the rest of promises.test.js, fs.watch.test.ts, fs.watch.close-exit.test.ts, fs.test.ts, socketaddress.spec.ts, parse-args.test.mjs, scrypt.test.ts, the path tests, fetch-compress.test.ts, the abort-signal leak fixture for readFile/writeFile, and the upstream test-fs-readfile, test-fs-promises-readfile, test-fs-promises-writefile, test-fs-promises-file-handle-readFile/-writeFile, test-fs-write-file, test-fs-writefile-with-fd, test-fs-watch, test-fs-watch-abort-signal, test-fs-watch-recursive-promise, test-fs-promises-watch, test-fs-rm, test-socketaddress, test-blocklist, test-http2-client-request-options-errors. cargo fmt --check is clean.

Background

• ERR_INVALID_ARG_TYPE(name, expected, actual) in node renders The <subject> must be <expected>. Received <actual>. The subject is "name" argument, or "name" property when the name contains a . (an option path such as options.signal). A class name as expected renders as an instance of X; a primitive type name as of type x.
• validateAbortSignal(signal, name) is node's validator for signal options: undefined passes, anything else that is not an object with an aborted property throws ERR_INVALID_ARG_TYPE(name, "AbortSignal", signal). getOptions() in node's fs layer calls it with options.signal, which is why all the entry points above share one message. The native port accepts only a real AbortSignal, because that is the only kind the native abort paths can subscribe to; the comment on the helper says so.
• Bun has two ERR_INVALID_ARG_TYPE renderers: the C++ one in ErrorCode.cpp, used by C++ and by the JS builtins ($ERR_INVALID_ARG_TYPE, validateAbortSignal), and the Rust helpers on JSGlobalObject, used by natively parsed options such as these. throw_invalid_argument_type_value prints of type <typename>; throw_invalid_argument_type_value2 takes the whole phrase after must be, which is how a class-name expectation is rendered from Rust. determine_specific_type (shared with C++) renders the received value.
• AbortSignalRef is the intrusive smart pointer over the C++ AbortSignal (ref() on clone, unref() on drop); AbortSignal::ref_from_js downcasts a JS value and takes one. JSValue::get returns None for a missing or undefined property; get_truthy additionally drops null and "", which is the filter that let those values through.
• src/runtime/node/util/validators.rs is the Rust port of node's lib/internal/validators.js; the new helper sits with the other validate_* functions there.

bun 1.4.0 vs node v26.3.0 for a few of the calls (after this change every bun line equals the node line)

readFileSync(p, { signal: 1 })
  bun:  The "signal" argument must be of type AbortSignal. Received type number (1)
  node: The "options.signal" property must be an instance of AbortSignal. Received type number (1)
readFileSync(p, { signal: null })
  bun:  (reads the file)
  node: The "options.signal" property must be an instance of AbortSignal. Received null
writeFileSync(p, "x", { signal: "" })
  bun:  (writes the file)
  node: The "options.signal" property must be an instance of AbortSignal. Received type string ('')
fs.promises.appendFile(p, "x", { signal: {} })
  bun:  The "signal" argument must be of type AbortSignal. Received an instance of Object
  node: The "options.signal" property must be an instance of AbortSignal. Received an instance of Object
fs.watch(p, { signal: 1 })
  bun:  signal is not of type AbortSignal
  node: The "options.signal" property must be an instance of AbortSignal. Received type number (1)
fs.watch(p, { signal: null })
  bun:  (creates the watcher)
  node: The "options.signal" property must be an instance of AbortSignal. Received null
fs.rmSync(p, { maxRetries: "a" })            (renderer side effect)
  bun:  The "options.maxRetries" argument must be of type number. Received type string ('a')
  node: The "options.maxRetries" property must be of type number. Received type string ('a')

Every Rust call site whose text changes from "argument" to "property"

Found by listing all callers of the three renderers and keeping those whose name contains a . (the callers that pass a name through a variable, in validators.rs, csrf_jsc.rs, s3/credentials_jsc.rs, ffi_body.rs and validate_object, all receive plain names such as callback, expiresIn, options; the only dotted names reaching them are the two fs.rm ones).

call site	name	API
node_fs.rs via validate_integer (2)	options.maxRetries, options.retryDelay	fs.rm/rmSync/promises.rm (matches node)
socket/SocketAddress.rs (3)	options.address (x2), options.flowlabel	net.SocketAddress (matches node)
api/bun/h2_frame_parser.rs (6)	options.silent, options.endStream, options.exclusive, options.parent, options.weight, options.signal	native http2 request options (node's JS validators say property for these)
node/node_cluster_binding.rs (1)	handle.fd	cluster IPC handle
socket/socket_body.rs (2)	rule.syscall, rule.action	internal socket fault injection
webcore/fetch/compress_body.rs (1)	compress.encoding	fetch compress option

Refcount check for the fs.watch ownership change

2,000 iterations of: a watcher with a live signal closed explicitly, one created with a pre-aborted signal, one aborted after creation, one call rejected with { signal: {} }, and one whose options object returns a fresh signal from a getter. After Bun.gc(true), heapStats().objectTypeCounts.AbortSignal is 2 on this branch and 2 on bun 1.4.0 (debug/ASAN build for this branch).

Earlier revision

The first push kept the brand check at each of the three sites and only shared the error (throw_err_invalid_abort_signal); the watcher still borrowed the signal from the JS value. A review pass pointed out that this left three copies of the accept condition, so the second commit moved check and error into validate_abort_signal and made the watcher own its ref. Tests are unchanged between the two revisions.

[coderabbitai]

Warning

Review limit reached

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

Next review available in: 30 minutes

Limit details: You’ve used all 5 included reviews currently available under your plan.

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: 72142be5-36d9-4250-a55c-43ab27a38230

📥 Commits

Reviewing files that changed from the base of the PR and between aec33f5 and 9863825.

📒 Files selected for processing (6)

• src/jsc/JSGlobalObject.rs
• src/runtime/node/node_fs.rs
• src/runtime/node/node_fs_watcher.rs
• src/runtime/node/util/validators.rs
• test/js/node/fs/promises.test.js
• test/js/node/watch/fs.watch.test.ts

---

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

[robobun]

Status: ready for review (current head: 9863825).

Reproduced on main (a42889a, debug build) and on the released bun 1.4.0 with a script that runs readFileSync/writeFileSync over ten non-signal values, { signal: 1 } through every sync/callback/promises/FileHandle entry point, and fs.watch with 1, {}, 0, null, then diffs the output against node v26.3.0. Before: every line differs ("signal" argument ... of type AbortSignal, signal is not of type AbortSignal, and null/"" silently accepted). With this branch the diff is empty apart from the two entry points owned by #39272 (FileHandle#appendFile) and #38694 (fs.promises.watch).

Since the first push: the brand check and the error moved into one validators::validate_abort_signal used by all three sites, and fs.watch now owns a ref to the signal instead of borrowing it (refcount probe in the PR body). Tests are unchanged: test/js/node/fs/promises.test.js (options.signal validation matches node) and the new case in test/js/node/watch/fs.watch.test.ts fail on the release build and pass with this change.

[claude]

LGTM — focused Node.js compat fix for options.signal validation in fs readFile/writeFile/watch, with the Rust ERR_INVALID_ARG_TYPE renderer brought in line with Node's argument/property rule.

What was reviewed:

• Verified JSValue::get returns None for undefined/missing (JSValue.rs:1136), so the get_truthy → get swap correctly rejects only defined non-signals while leaving undefined as "no signal".
• Traced the InvalidArgTypeName renderer's blast radius: dotted-name callers in SocketAddress, h2_frame_parser, node_cluster, node_fs (maxRetries/retryDelay), fetch compress_body, and socket_body all move from argument → property; grep found no test pinning the old wording, and the change matches both Node's rule and the existing C++ ErrorCode.cpp renderer.
• Confirmed validators is already in scope in node_fs.rs and strings::contains_char exists in bun_core.
• Tests cover a 10-value matrix, 13 entry points, positive cases (undefined + real signal), and assert no I/O on rejection.

Extended reasoning...

Overview

This PR aligns options.signal validation in node:fs (readFile/writeFile/appendFile in all sync/callback/promises/FileHandle forms, plus fs.watch) with Node v26.3.0. Three call sites in node_fs.rs and node_fs_watcher.rs switch from get_truthy to get (so null/"" are now rejected like Node does) and route through a new validators::throw_err_invalid_abort_signal helper. To render Node's exact text ("options.signal" property must be an instance of AbortSignal), the three Rust ERR_INVALID_ARG_TYPE renderers in JSGlobalObject.rs gain an InvalidArgTypeName formatter implementing Node's rule (dotted name → property, argument suffix → verbatim, else → argument).

Security risks

None. This is error-message wording plus tightening of an option validator (rejecting more inputs, not accepting more). No auth, crypto, path handling, or resource acquisition changes.

Level of scrutiny

Medium. The three call-site changes are trivially correct once JSValue::get's semantics are confirmed (done — it returns None for undefined and missing properties, so undefined still means "no signal"). The shared-renderer change in JSGlobalObject.rs has broader reach: it changes error text for every Rust caller passing a dotted argname. I enumerated those callers (~15 sites across SocketAddress, h2_frame_parser, node_cluster, node_fs rmSync options, fetch compress.encoding, and socket rule.*). For the node-compat callers this is strictly a move toward Node's text, and it matches the rule the C++ ErrorCode.cpp renderer already applies. For the two Bun-native callers the wording change is still more accurate. Grep confirms no test in the tree asserts the old "<dotted>" argument phrasing, and the PR description lists the relevant node parallel tests (test-socketaddress, test-http2-client-request-options-errors, test-fs-rm, fetch-compress.test.ts) as passing.

Other factors

• The PR description is unusually thorough: it cites Node source lines, documents the renderer side-effect and the trial-merge with the overlapping #38663 (which carries the identical InvalidArgTypeName change), and lists every test file re-run.
• Test coverage is strong: a 10-value type matrix through readFileSync, a 13-entry-point matrix with { signal: null }, positive cases for undefined and a real signal, an assertion that the output file is not created (validation precedes I/O), and an equivalent fs.watch matrix that closes created watchers.
• The behavior change (rejecting { signal: null } / { signal: "" }) is intentional Node alignment and happens before any file is opened.
• No caller passes a name already ending in argument, so that branch of InvalidArgTypeName is currently dead but harmless (it exists for parity with Node and the C++ renderer).
• The bug hunting system found no issues.

[robobun]

On the one observation in the review (no Rust caller currently passes a name ending in argument, so that branch of InvalidArgTypeName is unused today): keeping it is deliberate. It is part of node's rule for this message and of the C++ addParameter this mirrors, and the renderer text is the same as #38663's so the two keep merging cleanly. Nothing else to change from that review.

[robobun]

^{Updated 1:04 AM PT - Aug 16th, 2026}

✅ @robobun, your commit 9863825baa5130d9b9634169012a70ae07da3ea4 passed in Build #99290! 🎉

---

🧪   To try this PR locally:

bunx bun-pr 39274

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

bun-39274 --bun

[claude]

I reviewed this PR and didn't find any bugs. Because it changes AbortSignal refcount ownership in fs.watch and widens the ERR_INVALID_ARG_TYPE renderer's argument/property rule to every Rust caller, a human look would still be worthwhile.

What was reviewed:

• AbortSignalRef refcount balance across Arguments::from_js → FSWatcher::init (clone) → detach() (replace-None drop), including the path_watcher::watch error path — balanced, and one unsafe adopt removed.
• JSValue::get returns None for both missing and undefined, so only undefined bypasses validation (matches node's options.signal !== undefined).
• InvalidArgTypeName uses strings::contains_char and mirrors Bun::Message::addParameter; existing non-dotted callers keep the old "x" argument text.

Extended reasoning...

Overview

This PR aligns options.signal validation in fs.readFile/writeFile/appendFile (all forms) and fs.watch with Node.js. It adds validators::validate_abort_signal returning an AbortSignalRef, switches the three call sites from get_truthy to get so null/""/0 are validated (and rejected) like node does, and adds an InvalidArgTypeName formatter to the three Rust throw_invalid_argument_type_value* renderers so dotted names read "options.signal" property instead of "options.signal" argument. fs.watch's Arguments struct now holds Option<AbortSignalRef> instead of a borrowed &AbortSignal, and FSWatcher::init clones it — removing an unsafe { AbortSignalRef::adopt(s.ref_()) } and the ensure_still_alive dance. Two test files gain a value-matrix and entry-point-matrix pinning node v26.3.0's exact error text.

Security risks

None identified. This is input validation for a documented option; the change makes validation stricter (rejecting more values), not laxer. No path handling, no privilege boundaries, no untrusted-data parsing beyond a JS brand check.

Level of scrutiny

Medium-high. Two aspects push this beyond "mechanical":

1. Refcount ownership change: fs.watch now takes an owned AbortSignalRef in Arguments and clones it into the FSWatcher. I traced the refcount: ref_from_js (+1) → stored in args.signal → .clone() into FSWatcher.signal (+1) → args dropped (−1) → detach()'s replace(None) (−1). The error path in init (when path_watcher::watch fails) calls finalize → detach before returning, so the cloned ref is released there too. This is balanced and matches the author's heapStats probe, but it's a change to intrusive-refcount lifetime in a GC-interacting struct — the kind of thing REVIEW.md calls out for maintainer sanity-checking.
2. Repo-wide renderer change: InvalidArgTypeName applies to every Rust caller of the three throw_invalid_argument_type_value* helpers. The PR body says the affected dotted-name callers were spot-checked against node and no test pinned the old text, and the change mirrors what ErrorCode.cpp already does — but the blast radius is wider than the three fs sites.

Other factors

CI (Build #99007) is green. Tests are thorough (10-value matrix through readFileSync, 13-entry-point matrix with {signal: null}, plus the negative check that no I/O happened). The comment-cop bot flagged three doc comments; the author responded to each with justification (node-compat guidance requires citing the upstream source for ported rules; the _value2 doc is two lines) and trimmed one. Those responses look reasonable to me but a maintainer may want to weigh in on whether the 5-line InvalidArgTypeName doc should be trimmed further. There's also stated overlap with #38663 (same renderer hunk) and #35275 (same three call sites) that a maintainer coordinating those PRs would want to be aware of.