node:fs: fix GC root leak for Buffer path arguments in async ops

[robobun]

Fixes #32191

Every async node:fs operation that received a Buffer/TypedArray/ArrayBuffer path argument leaked one GC root for that argument, permanently pinning the path buffer (and, for small Buffers, the shared Buffer pool chunk behind it).

Repro

import fs from "node:fs";
import { heapStats } from "bun:jsc";

for (let i = 0; i < 5000; i++) {
  await fs.promises.access(Buffer.from("nonexistent-" + i)).catch(() => {});
}
Bun.gc(true);
console.log(heapStats().objectTypeCounts.Uint8Array);

On 1.4.0-canary this reports one leaked Uint8Array per call (5000+), surviving Bun.gc(true). The sync equivalent does not leak.

Cause

Two gcProtects per call, one gcUnprotect:

1. PathLike::from_js_with_allocator (src/runtime/node/types.rs) called arguments.protect_eat() on the argument at parse time.
2. PathLike::to_thread_safe (src/jsc/node_path.rs) protected the same value again at task-schedule time.
3. At completion, ThreadSafe<A>'s drop runs PathLike::unprotect(), which unprotects once.
4. The protect_eat root was supposed to be released by Drop for ArgumentsSlice, but run_async (src/runtime/node/node_fs_binding.rs) wraps the slice in ManuallyDrop and intentionally never drops it on the success path.

Net +1 permanent root per call; gcProtect is refcounted so they accumulate. The sync path was balanced (parse protect released by the slice's drop at the end of the host call), matching the report. The Zig implementation shipped in 1.3.x had the same accounting, so this is not a port regression.

Fix

Remove the parse-time protect_eat root (the two arguments.protect_eat() to arguments.eat() changes in types.rs are the fix; the rest is the cleanup this enables). The root was redundant in both modes:

• During the host call (parse through task creation, and the whole sync dispatch) the argument is kept alive by the JS call frame.
• Anything that outlives the host call goes through to_thread_safe(), whose protect is released exactly once, on the JS thread, by ThreadSafe's drop. Audited every PathLike-parsing call site to confirm nothing retains a borrowed buffer past the host call without it: node:fs sync/async/cp/readdir, the Windows UVFSRequest path, fs.watch/fs.watchFile (copy the path bytes during the call), and Bun.file/Bun.write/S3 (call to_thread_safe before storing). This also matches how sync buffer arguments (fs.writeSync etc.) already work: they take no protect at all.

Consequential changes in the same commit:

• protect_eat now has zero callers and the ArgumentsSlice.protected bitset is never set, so that machinery (protect_eat, unprotect, the bitset field, the Drop impl, the unused init_async) is deleted, and the ManuallyDrop handling in run_async/cp/readdir, which existed only to suppress the bitset unprotect, becomes plain drops.
• The run_async early-error paths (pending exception after parse, pre-aborted AbortSignal) previously called args.unprotect() when only parse-time roots existed, over-unprotecting by one for every buffer whose protect is taken at schedule time (PathLike, Read, Write, writev/readv); an unbalanced gcUnprotect can release a root legitimately held elsewhere on the same value. They now release through drop(args.into_thread_safe()), the exact inverse of parse-time acquisitions and the same release path a completed task uses, which also keeps WriteFile.data's parse-time protect balanced.

Verification

New test in test/js/node/fs/fs-leak.test.js measures heapStats().objectTypeCounts deltas across 128 async calls each for: access with Buffer paths, access with ArrayBuffer paths, writeFile with a Buffer path, and pre-aborted writeFile with Buffer path and data. On the unfixed build the first three fail with leaked 128 objects over 128 calls; with the fix all deltas are ~0.

Also ran on the debug build: fs.test.ts, promises.test.js, cp.test.ts, readdirSync-recursive-error-leak.test.ts, fs.watch/fs.watchFile suites, bun-write.test.js, resolve.test.ts. Remaining failures there are pre-existing debug/ASAN timeouts (100k-iteration and x100-loop tests that exceed the 5s default), run-as-root environment issues (EACCES tests), and a fixture whose ASAN RSS cap keys on the binary being named bun-asan; each either reproduces on the unfixed binary or provably does not execute the changed arms (string paths).

Notes

• Textually overlaps node:fs: snapshot path buffers backed by resizable ArrayBuffers for async ops #32189 (resizable-ArrayBuffer path snapshotting) in types.rs; the fixes are independent, and whichever lands second needs a trivial rebase (the protect_eat arm of the snapshot branch collapses to plain eat).
• Pre-existing and not changed here: NewAsyncCpTask/AsyncReaddirRecursiveTask leak their task box (including protected args) if converting the result to JS throws at completion, unlike AsyncFSTask which destroys via scopeguard.

[robobun]

^{Updated 9:16 PM PT - Jun 12th, 2026}

❌ @robobun, your commit a6cffe8 has some failures in Build #62179 (All Failures)

---

🧪   To try this PR locally:

bunx bun-pr 32199

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

bun-32199 --bun

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 860f9bd9-34be-4947-8abb-eb3423776966

📥 Commits

Reviewing files that changed from the base of the PR and between a846f59 and a6cffe8.

📒 Files selected for processing (1)

• src/jsc/CallFrame.rs

---

Walkthrough

This PR removes per-argument protection from ArgumentsSlice, replaces protect_* parsing calls with eat()/next_eat(), moves async protection to to_thread_safe/into_thread_safe with drop-based cleanup, updates NodeFS async error paths to release resources via into_thread_safe(), and adds a subprocess regression test for Buffer/ArrayBuffer path leaks.

Changes

Async fs protection lifecycle refactor

Layer / File(s)	Summary
Remove protection tracking from ArgumentsSlice src/jsc/CallFrame.rs	Delete IntegerBitSet import, remove protected field, remove unprotect/protect_eat/protect_eat_next methods, drop Drop impl, update init/init_async and remaining_buf docs.
Shift protection contract to to_thread_safe and update parsing src/runtime/node/types.rs, src/jsc/node_path.rs, src/runtime/api/BunObject.rs, src/runtime/node/node_fs.rs	Replace protect_eat/protect_eat_next with eat()/next_eat() for typed-array/ArrayBuffer/string-like arms and resolve/FdVectorIo arg parsing; refine docs to state async protection happens in to_thread_safe.
Update NodeFS async operations for new lifecycle src/runtime/node/node_fs_binding.rs	Remove ManuallyDrop import and change run_async and bespoke async bindings (cp, readdir) to release parse-time resources on error/early-return via drop(args.into_thread_safe()) instead of manual unprotect/ManuallyDrop sequences; adjust related comments.
Add regression test for Buffer path argument leak test/js/node/fs/fs-leak.test.js	Import Bun harness utilities and add a subprocess-based test that measures heap object counts around async fs.promises operations with Buffer/ArrayBuffer path args (including aborted writes) and asserts no persistent GC-root leak via JSON delta thresholds.

Suggested reviewers:

• Jarred-Sumner

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately and concisely summarizes the main change: fixing a GC root leak for Buffer path arguments in async node:fs operations.
Description check	✅ Passed	The description comprehensively covers what the PR does, the root cause analysis, the fix, verification approach, and relevant notes, addressing all aspects of the template.
Linked Issues check	✅ Passed	The PR fully addresses the objectives from #32191: reproduces the leak mechanism, identifies the root cause (two gcProtects, one gcUnprotect), removes redundant parse-time protect, audits callers, and verifies with tests.
Out of Scope Changes check	✅ Passed	All changes are within scope: removing protect_eat machinery from ArgumentsSlice, updating call sites in types.rs/node_fs_binding.rs/BunObject.rs to use eat instead, and adding a regression test.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

---

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

[coderabbitai]

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/jsc/CallFrame.rs (1)

320-325: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win

Rename or remove protect_eat_next.

This helper no longer protects anything, so keeping the old name on a lifetime-sensitive API makes the new contract easy to misuse. next_eat already covers the current behavior; either delete this shim or rename it to reflect plain consumption.

As per coding guidelines, "Name things truthfully: a function must not match more inputs than its name claims."

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/jsc/CallFrame.rs` around lines 320 - 325, The method protect_eat_next on
CallFrame no longer provides protection and should be removed or renamed to
reflect plain consumption: either delete protect_eat_next and update all call
sites to call next_eat() directly, or rename it to a truthful name like
consume_next (or next_if_any) and keep its implementation, ensuring signatures
(returning Option<JSValue>) and uses remain correct; search for protect_eat_next
references and replace them with next_eat (or the new name) and run tests to
confirm no lifetime contract changes are introduced.

Source: Coding guidelines

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/runtime/node/types.rs`:
- Around line 1254-1258: The async buffer parsing paths are double-protecting GC
roots: StringOrBuffer::from_js_maybe_async_into and
StringOrBuffer::from_js_with_encoding_maybe_async_into call protect() at
parse-time and then to_thread_safe() calls protect() again while unprotect()
only drops one root, leaking; remove the parse-time protect() in those async
branches (or instead make unprotect() reliably release both roots) so the async
Buffer-backed path only acquires a single root via to_thread_safe(); update both
from_js_maybe_async_into and from_js_with_encoding_maybe_async_into (and the
similar site around the other mention) and run a grep for other StringOrBuffer
protect() usages to fix any sibling sites.

---

Outside diff comments:
In `@src/jsc/CallFrame.rs`:
- Around line 320-325: The method protect_eat_next on CallFrame no longer
provides protection and should be removed or renamed to reflect plain
consumption: either delete protect_eat_next and update all call sites to call
next_eat() directly, or rename it to a truthful name like consume_next (or
next_if_any) and keep its implementation, ensuring signatures (returning
Option<JSValue>) and uses remain correct; search for protect_eat_next references
and replace them with next_eat (or the new name) and run tests to confirm no
lifetime contract changes are introduced.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 92c03d87-50cb-416c-9d09-a52f692b8b5b

📥 Commits

Reviewing files that changed from the base of the PR and between ac312f0 and 8014736.

📒 Files selected for processing (5)

• src/jsc/CallFrame.rs
• src/jsc/node_path.rs
• src/runtime/node/node_fs_binding.rs
• src/runtime/node/types.rs
• test/js/node/fs/fs-leak.test.js

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/runtime/node/node_fs.rs (1)

2837-2845: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Add direct async readv/writev coverage for this next_eat() path.

This stops taking the parse-time root for ArrayBufferView[], but the new regression coverage only exercises path-like arguments. FdVectorIo goes through a separate VectorArrayBuffer pin/release path, so this sibling site still needs a focused fs.promises.readv/writev leak/lifetime test in the existing fs leak suite.

As per coding guidelines, "Fix the whole class in the same PR" and "Every behavioral change ships an automated test in the same PR."

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/runtime/node/node_fs.rs` around lines 2837 - 2845, The
VectorArrayBuffer::from_js call via arguments.next_eat() (the next_eat() path
that uses will_be_async) lacks direct async readv/writev coverage; add focused
fs leak-suite tests that call fs.promises.readv and fs.promises.writev using an
ArrayBufferView[] argument so the code path that roots/pins via
VectorArrayBuffer::from_js is exercised (not the FdVectorIo sibling path).
Ensure tests exercise the async branch (will_be_async=true), assert buffers are
released after completion, and integrate them into the existing fs leak
detection harness so any lifetime/leak regressions on
next_eat()/VectorArrayBuffer::from_js are caught.

Source: Coding guidelines

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@src/runtime/node/node_fs.rs`:
- Around line 2837-2845: The VectorArrayBuffer::from_js call via
arguments.next_eat() (the next_eat() path that uses will_be_async) lacks direct
async readv/writev coverage; add focused fs leak-suite tests that call
fs.promises.readv and fs.promises.writev using an ArrayBufferView[] argument so
the code path that roots/pins via VectorArrayBuffer::from_js is exercised (not
the FdVectorIo sibling path). Ensure tests exercise the async branch
(will_be_async=true), assert buffers are released after completion, and
integrate them into the existing fs leak detection harness so any lifetime/leak
regressions on next_eat()/VectorArrayBuffer::from_js are caught.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: c2b202ca-3e7d-4d3b-afc9-7057d51edc46

📥 Commits

Reviewing files that changed from the base of the PR and between 8014736 and 6f1100f.

📒 Files selected for processing (3)

• src/jsc/CallFrame.rs
• src/runtime/api/BunObject.rs
• src/runtime/node/node_fs.rs

💤 Files with no reviewable changes (1)

• src/jsc/CallFrame.rs

[robobun]

Re the outside-diff note on readv/writev: the protect_eat_next change was behavior-identical, it never protected (rooting for ArrayBufferView[] happens inside VectorArrayBuffer::from_js and is balanced by release() at completion). The new fixture segments confirm this: they pass on the unfixed build too. Added writev/readv segments to the leak test in 761e36b anyway, since the run_async early-path change does touch FdVectorIo's release lifecycle.

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@test/js/node/fs/fs-leak.test.js`:
- Around line 182-190: The test currently measures only per-element Uint8Array
roots for writev/readv (via measure("Uint8Array", ...) and stores results in
deltas.writevBuffers and deltas.readvBuffers) but does not observe the scheduled
array wrapper root; add a second measurement that observes the array wrapper
(e.g., call measure with the appropriate array/object root type such as "Array"
or the runtime-specific wrapper type) using the same writev/readv invocations
and store them (e.g., deltas.writevArrayRoots and deltas.readvArrayRoots), then
add assertions that both the element-buffer metrics
(deltas.writevBuffers/deltas.readvBuffers) AND the new array-root metrics are
"ok"; repeat the same addition for the other occurrence referenced (lines
~216-225) so both element and array-root lifecycles are validated for writev and
readv.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: baa8ae96-2908-48ec-9aa4-07466c54656f

📥 Commits

Reviewing files that changed from the base of the PR and between 6f1100f and 761e36b.

📒 Files selected for processing (1)

• test/js/node/fs/fs-leak.test.js

[claude]

No issues found and my earlier nit was addressed — but this rewrites the GC-root lifecycle contract for async fs args (removing the parse-time protect, the ArgumentsSlice Drop, and changing the early-error release path), which is memory-safety-critical enough that a human should sign off on the accounting.

Extended reasoning...

Overview

The PR fixes a per-call GC root leak for Buffer/TypedArray/ArrayBuffer path arguments in async node:fs operations. The core fix is two protect_eat() → eat() changes in PathLike::from_js_with_allocator. The bulk of the diff is consequential cleanup: deleting the now-unused ArgumentsSlice protection machinery (protected bitset, Drop impl, protect_eat/unprotect/init_async/protect_eat_next), removing the ManuallyDrop wrappers in run_async/cp/readdir, and changing the early-error release path from args.unprotect() to drop(args.into_thread_safe()). A subprocess regression test in fs-leak.test.js measures heap-type deltas across six segments.

Security risks

No injection/auth/data-exposure surface. The risk class here is memory safety: GC-root accounting for JS-backed buffers that cross to a worker thread. An under-protect would let a backing store be collected while a syscall reads it (UAF); an over-unprotect could release a root legitimately held elsewhere. The PR description audits every PathLike-parsing call site and the early-error path change is argued to be the exact inverse of parse-time acquisitions, but this is precisely the kind of invariant that benefits from a maintainer who knows the ThreadSafe<A>/Unprotect contract confirming the audit.

Level of scrutiny

High. This is core runtime code on a hot path, and the change is a contract rewrite (single-root via to_thread_safe only) rather than a localized patch. The early-error path change in run_async — into_thread_safe() taking the schedule-time protects and immediately dropping them to release both those and parse-time protects like WriteFile.data's — is subtle and depends on every FsArgument's to_thread_safe/unprotect pair being balanced for that idiom.

Other factors

All prior review threads (CodeRabbit's StringOrBuffer double-protect concern, my protect_eat_next vestige nit, the writev/readv array-root coverage ask) are resolved with follow-up commits and the author's responses demonstrate solid command of the accounting. The new test is well-constructed (warmup, exact-error gating, multi-type heap observation). The CI failures on musl targets are unrelated LTO link-layout errors. Still, per the approval guidelines this is squarely "complex change touching critical code paths," so deferring to a human reviewer.

[claude]

Both cleanup nits are addressed and I have nothing further on a6cffe8 — deferring to a human for the GC-root accounting itself, since the fix's correctness rests on the call-site audit (nothing retains a borrowed PathLike past the host call without to_thread_safe) and on the reworked run_async early-error release path, where an off-by-one in either direction is leak vs. UAF.

Extended reasoning...

Overview

The PR fixes #32191: every async node:fs op that received a Buffer/TypedArray/ArrayBuffer path leaked one permanent gcProtect root. The two-line core fix swaps arguments.protect_eat() → arguments.eat() in PathLike::from_js_with_allocator (src/runtime/node/types.rs). Everything else is the cleanup that enables: deleting protect_eat/protect_eat_next/unprotect/the protected bitset/Drop for ArgumentsSlice/init_async/the all field from ArgumentsSlice (src/jsc/CallFrame.rs), removing the now-unneeded ManuallyDrop wrapping in run_async/cp/readdir (src/runtime/node/node_fs_binding.rs), updating three next_eat() call sites (BunObject.rs, node_fs.rs), a doc-comment tweak in node_path.rs, and a new subprocess heap-delta regression test in fs-leak.test.js. Seven files, ~110 lines net removed from src/.

Security risks

No injection/auth/data-exposure surface. The risk profile is memory safety: gcProtect/gcUnprotect is refcounted, so under-protecting (or over-unprotecting) a JS value that backs a raw &[u8] borrowed across a thread hop is a use-after-free hazard. The PR explicitly changes the early-error branches (has_exception, pre-aborted signal) from args.unprotect(); drop(args) — which the description argues was already over-unprotecting by one for schedule-time-protected fields — to drop(args.into_thread_safe()), the same release path a completed task uses. That reasoning looks sound and is now well-commented, but it is exactly the kind of invariant a maintainer should confirm.

Level of scrutiny

High. This is hot-path runtime code (every async fs call) and the correctness argument is non-local: it depends on (a) the audit in the PR description that no PathLike consumer retains a borrowed buffer past the host call without going through to_thread_safe(), and (b) the per-args-type to_thread_safe/Unprotect pairings (e.g. WriteFile.data's parse-time protect being adopted rather than re-protected) staying balanced. The dead-code deletions are mechanically safe (compiler-checked); the lifecycle change is not.

Other factors

• All four review threads (two from me on protect_eat_next and the write-only all field; two from CodeRabbit on StringOrBuffer double-protect — refuted — and writev/readv array-root coverage) are resolved, with follow-up commits 6f1100f / 761e36b / a846f59 / a6cffe8.
• The bug-hunting system found nothing on the latest revision.
• New test covers Buffer path, ArrayBuffer path, writeFile Buffer path, pre-aborted writeFile (the changed early-error branch), and writev/readv element+array roots; the description states the first three fail on the unfixed build with delta == N.
• Description notes a textual overlap with #32189 and a pre-existing NewAsyncCpTask/AsyncReaddirRecursiveTask leak left out of scope.
• No CODEOWNERS entry for these paths.

[robobun]

CI status: the diff is green on every lane that ran tests (285 jobs passed). The one red job is darwin 14 aarch64 - test-bun, which timed out during VM boot before executing a single test (the job log ends at clone+boot ... from bun-ci-base then Received cancellation signal, with no test output). Buildkite auto-retried the other darwin-aarch64 lanes in this build and they passed; this instance was not auto-retried.

This is an agent/VM infrastructure timeout unrelated to the change, which is platform-agnostic Rust. Ready for review; a re-run of that lane should clear it.