node_fs_watcher: batch events as BoundedArray<Event, 8>

[robobun]

What

FSWatchTaskPosix (src/runtime/node/node_fs_watcher.rs) is the batch of fs.watch events the watcher thread collects and posts to the JS thread. It stored the batch as a hand-rolled fixed-capacity vector: a count: u8 plus entries: [MaybeUninit<Entry>; 8], where Entry paired each Event with a needs_free: bool. append wrote entries[count], run read each slot with assume_init_ref, clean_entries read needs_free with assume_init_ref and conditionally called assume_init_drop, enqueue swapped the raw array out with mem::replace and reset count, append rebuilt the whole struct with *self = Self { .. } after flushing a full batch, and a manual Default impl built the uninitialised array.

// before
pub struct FSWatchTaskPosix {
    ctx: Option<bun_ptr::ParentRef<FSWatcher>>,
    count: u8,
    entries: [MaybeUninit<Entry>; 8],
    concurrent_task: ConcurrentTask,
}
pub struct Entry { event: Event, needs_free: bool }
pub(crate) fn append(&mut self, event: Event, needs_free: bool)

// after
#[derive(Default)]
pub struct FSWatchTaskPosix {
    ctx: Option<bun_ptr::ParentRef<FSWatcher>>,
    entries: BoundedArray<Event, 8>,
    concurrent_task: ConcurrentTask,
}
pub(crate) fn append(&mut self, event: Event)

append is now enqueue() when full followed by append_assume_capacity; run iterates as_slice() with the same match; enqueue moves the batch into the heap task with mem::take and drops a batch nobody will run with entries.clear() or by dropping the Box; deinit (still the dispatcher's entry point, same signature) just reclaims the Box and keeps its debug assertion. Entry, needs_free, clean_entries, the *self = Self { .. } reset in append and the manual Default impl are deleted. The two append callers (on_path_update_posix, append_abort) drop their needs_free argument. Removes 3 unsafe blocks; dispatch.rs and the Windows task are unchanged. One file, 20 insertions, 77 deletions.

Why

needs_free carried no information: the only caller passing false appended Event::Abort, a payload-less variant whose drop is a no-op, while Rename/Change own a Box<[u8]> and Error owns a bun_sys::Error, so Event's own drop glue already frees exactly what clean_entries freed. With BoundedArray the "which prefix of the array is initialised" invariant lives in one type instead of in every method, and a batch left in the embedded current_task or in an unposted heap task is freed by drop glue instead of by remembering to call clean_entries. It is zero-cost: the representation is still [MaybeUninit<_>; 8] plus a length, append_assume_capacity is the same slot write and increment, clear() is a drop_in_place over the initialised prefix where the per-entry needs_free branch used to be, and because a slot is now a 48-byte Event instead of a 56-byte Entry the task shrinks from 496 to 432 bytes on x86_64 Linux (the usize length takes the padded slot the u8 count occupied).

Part of a series of small type-system hardening changes; each PR stands alone.

Verification

cargo check and cargo clippy are clean for the touched crates. Debug build succeeds. test/js/node/watch/fs.watch.test.ts, fs.watch.rewrite.test.ts, fs.watch.close-exit.test.ts, fs.watch.deadlock.test.ts: 50 pass, 6 skip, 0 fail (56 tests across 4 files).

[coderabbitai]

Warning

Review limit reached

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

Next review available in: 15 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: d7e5058c-9188-4d60-a60e-c431b63e7042

📥 Commits

Reviewing files that changed from the base of the PR and between da3851e and d8a0cdd.

📒 Files selected for processing (1)

• src/runtime/node/node_fs_watcher.rs

---

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

[claude]

LGTM — replaces the hand-rolled [MaybeUninit<Entry>; 8] + count: u8 with the in-tree BoundedArray<Event, 8> and drops the redundant needs_free flag.

What was reviewed:

• Verified BoundedArray semantics in src/bun_core/bounded_array.rs: Default (empty), Drop → clear() (drops initialised prefix), append_assume_capacity, as_slice, and is_empty via Deref<[T]> — all match what the diff relies on.
• Confirmed needs_free carried no information: the only false caller was Event::Abort (payload-less, no-op drop); all other variants own their payloads and free correctly via Event's drop glue.
• Checked enqueue() leaves entries empty on every path (early return / mem::take / clear()), so append_assume_capacity after a full-batch flush is sound.
• run() still borrows entries; deinit() reclaims the Box and BoundedArray::Drop frees them — same lifecycle as before, minus the manual clean_entries.

Extended reasoning...

Overview

Single-file refactor of FSWatchTaskPosix in src/runtime/node/node_fs_watcher.rs (POSIX-only, #[cfg(not(windows))]). Replaces a hand-rolled fixed-capacity vector (count: u8 + [MaybeUninit<Entry>; 8] + manual assume_init_ref/assume_init_drop + manual Default) with bun_collections::BoundedArray<Event, 8>. Deletes the Entry wrapper struct and its needs_free: bool, clean_entries(), the *self = Self { .. } reset in append(), the manual Default impl, and 3 unsafe blocks. Net -57 lines. The two append callers drop the needs_free argument; no other files change.

Security risks

None. This is an internal data-structure swap in the fs.watch event batching path. No user input parsing, no boundary changes, no allocation sizing driven by external data. The change removes unsafe code (assume_init_ref/assume_init_drop) in favour of a type whose invariants are maintained centrally.

Level of scrutiny

Medium — this touches memory-owning code on a cross-thread path (watcher thread → JS thread), so I verified the actual BoundedArray implementation rather than trusting the PR description. Key checks: bun_collections::BoundedArray re-exports bun_core::bounded_array::BoundedArray unchanged; it stores [MaybeUninit<T>; N] + len: usize, has Default (len=0), has Drop that calls clear() which drop_in_places the initialised prefix; append_assume_capacity writes the slot then increments (debug_assert!(len < N)); mem::take moves the array out and leaves an empty default. All match what the new code relies on.

The needs_free deletion is provably safe: on POSIX, EventPathString = Box<[u8]>, so Event::Rename/Change own their payload by construction; Event::Error owns a bun_sys::Error (whose path: Box<[u8]> drops normally); Abort/Close/NoFilename are payload-less or Copy. The only caller that passed needs_free=false was append_abort with Event::Abort, whose drop is a no-op — so unconditional drop via BoundedArray::clear() is behaviourally identical to the old per-entry needs_free branch.

The enqueue() postcondition ("leaves entries empty on every path") holds: empty→early return; ref_task() true→mem::take empties self.entries; refused post→drop(Box) frees the moved-out batch; ref_task() false→clear(). So append()'s append_assume_capacity after a full-batch enqueue() always sees len=0. The old *self = Self{..} reset was redundant with what enqueue() already did.

Other factors

• #[derive(Default)] on FSWatchTaskPosix is valid: Option (None), BoundedArray (explicit impl), ConcurrentTask (already had ::default() at both old call sites).
• The debug_assert! in deinit losing its #[cfg(debug_assertions)] wrapper is a no-op — debug_assert! already compiles out in release.
• Minor improvement not called out in the PR: the embedded current_task now frees any un-flushed entries when FSWatcher finalizes (via BoundedArray::Drop), where the old [MaybeUninit; 8] would have leaked them. This is strictly better.
• The PR follows repo guidance directly: uses the in-tree helper instead of hand-rolling, deletes the dead code (Entry, clean_entries, manual Default) in the same PR.
• Existing test suite (fs.watch.test.ts + 3 siblings, 50 pass / 6 skip) exercises this path; no new tests needed for a representation-preserving refactor.