node: carry the stat watcher refs as OwnedRef values

[robobun]

Stacked on #37665 (the base of this PR is that branch). The first version of this PR replaced the two hand-rolled ref guards in src/runtime/node/node_fs_stat_watcher.rs with ScopedRef; review asked whether there was something cleaner than guards, and this version is the answer: the refs themselves become values.

What

fs.watchFile keeps each StatWatcher alive through several intrusive refs: the scheduler's queue holds one per watcher, the initial-stat work-pool task holds one, and every hop posted from the pool to the JS thread carries one. Until now all of them were ref_() calls paired with deref() calls (or guards adopting them) at the other end, seven pairs spread over the file. Now each of them is an OwnedRef<StatWatcher> held by the thing that owns it:

• The queue. UnboundedQueue<StatWatcher> is wrapped in a private WatcherQueue whose push takes an OwnedRef (the one into_raw for the queue) and whose pop_batch yields OwnedRefs back (the one from_raw, with the invariant stated there). WatcherBatch drains itself on drop, so a batch that is not consumed to the end releases the rest instead of leaking it; append takes the queue's ref by value; work_pool_callback is a for watcher in batch loop that continues past closed watchers (dropping the queue's ref is what removes them) and pushes the value back otherwise; shutdown_for_exit closes and drops. The two hand-written ThreadSafeRefCount::<StatWatcher>::deref calls and the ref_() in append are gone.
• The initial stat task. InitialStatTask.watcher is an OwnedRef, taken with the one OwnedRef::acquire in init() where the freshly created watcher is known live, so create_and_schedule is a safe fn; on the closed path run_owned simply returns (the field drops before the embedded_work_finished guard, the order the explicit deref had), otherwise the value moves into the hop.
• Hops. post_to_js_thread(watcher: OwnedRef<Self>, hop) moves the ref into the posted task (the one into_raw in that direction) and take_hop_ref is the one from_raw in the other; run_hop calls it and dispatches the three continuations with the value, which therefore take &OwnedRef<Self> (the two initial-stat ones clone it to hand the queue its ref) or plain &self, and the hop's ref is released when run_hop returns instead of at the end of each continuation, the next statement on the same thread. Taskable::release_unrun, which releases a hop the VM tears down unrun, is drop(take_hop_ref(this)). restat posts watcher.clone(); the as_ctx_ptr helper it needed is gone.
• shutdown_for_exit adopts the RareData slot's ref on the scheduler as an OwnedRef local and lets it drop at the end, where Self::deref used to be, so that wrapper is deleted too; StatWatcher::finalize uses the existing bun_ptr::finalize_js_box idiom, which let the StatWatcher::ref_ / deref wrappers go as well.

Two things are deliberately unchanged: the scheduler's own ref across its work-pool hop still uses ScopedRef::adopt (the task node is intrusive, so there is no value to carry the ref; parking one in a field would need a lock or an ordering argument plus an unsafe acquire on the other side, which is not cleaner), and each watcher's RefPtr to the scheduler (converting it would move that release from finalize to the destructor, a behaviour change). Every ref is still taken and released at the same points and on the same threads as before, including the order relative to embedded_work_finished(); the file goes from 34 unsafe blocks to 27, net minus 99 lines.

Why

Which holder owns which ref is now written in the types: a popped watcher, a task's field and a hop's value each carry their ref, a release is a drop, and the raw hand-offs the intrusive queue and the task queue force are confined to one push/pop pair and one post/take pair, each stating its invariant once. OwnedRef is a NonNull, so the queue nodes, the task and the posted pointer are exactly the words they were, and each drop is the same atomic decrement the removed deref was.

Part of a series of small type-system hardening changes.

Verification

cargo check and cargo clippy are clean for bun_runtime; the source-tree lints in test/internal/source-lints pass (82 pass). Debug build succeeds. bun bd test test/js/node/watch/fs.watchFile.test.ts test/js/node/watch/fs.watch.test.ts: 53 pass, 8 skip, 0 fail (re-run on the final revision); the Node test suite's test-fs-watchfile.js, test-fs-watchfile-bigint.js and test-fs-watchfile-ref-unref.js, run under the debug build the way the Node test runner runs them, all exit 0.

cargo check --target x86_64-pc-windows-msvc also passes for bun_runtime on this branch, so no platform-gated caller of the removed hand-written ref/deref entry points remains.

[coderabbitai]

Walkthrough

Changes

The stat watcher replaces local reference guards with bun_ptr::ScopedRef for scheduler work-pool and watcher references used by asynchronous callbacks.

Stat watcher reference ownership

Layer / File(s)	Summary
Scheduler reference adoption src/runtime/node/node_fs_stat_watcher.rs	The scheduler work-pool callback uses ScopedRef for reference ownership.
Watcher callback adoption src/runtime/node/node_fs_stat_watcher.rs	Initial-stat success, initial-stat error, and changed-stat callbacks use ScopedRef for watcher references.

Suggested reviewers: dylan-conway, jarred-sumner

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
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 explains the ownership changes and provides detailed verification results, although it uses headings different from the repository template.
Title check	✅ Passed	The title clearly describes carrying stat watcher references as OwnedRef values, which is a substantive part of the described changes.

---

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

[claude]

LGTM — mechanical swap of two file-local RAII guards for the shared ScopedRef.

What was reviewed:

• Confirmed ScopedRef::adopt stores NonNull without bumping and its Drop calls T::rc_deref, which the #[derive(ThreadSafeRefCounted)] bridge routes to ThreadSafeRefCount::<T>::deref — byte-identical to the deleted Drop impls.
• Checked the added non-null precondition (NonNull::new_unchecked vs the old *mut): all 4 call sites already NonNull::new(this).expect(..) on the next line, so this is provably non-null.
• The explicit drop(_ref_guard) ordering in work_pool_callback before embedded_work_finished is preserved.

Extended reasoning...

Overview

This PR deletes two private RAII types (SchedulerRefGuard, WatcherRefGuard) from src/runtime/node/node_fs_stat_watcher.rs and replaces their 4 uses with bun_ptr::ScopedRef::<T>::adopt. Net +7/-61 lines in one file. Both host types already #[derive(bun_ptr::ThreadSafeRefCounted)], and I verified in src/bun_core_macros/lib.rs that this derive emits an AnyRefCounted impl with DestructorCtx = () and rc_deref_with_context forwarding to ThreadSafeRefCount::<Self>::deref — exactly what the deleted Drop bodies called. ScopedRef::adopt (src/ptr/ref_count.rs:992) does not bump the refcount, matching the deleted adopt constructors.

Security risks

None. This is a pure refactor of an internal RAII type with no user-facing surface, no new unsafe (net -4 unsafe items), and no change to refcount semantics or ordering.

Level of scrutiny

Low-to-medium. Refcount code is memory-safety-sensitive, so I traced the semantics rather than trusting the description: ScopedRef<T> requires T: AnyRefCounted with DestructorCtx: Default, both satisfied by the derive; adopt uses NonNull::new_unchecked (adding a non-null precondition the old *mut field lacked), but every call site immediately follows with NonNull::new(this).expect(...) for ParentRef, so non-null is already an established invariant. The one site with an explicit drop(_ref_guard) before embedded_work_finished still drops at the same point since ScopedRef implements Drop the same way.

Other factors

The PR description is precise and matches the code. This follows REVIEW.md's "grep for the in-tree helper before hand-writing anything" — ScopedRef is already used across ~20 files in the runtime for this exact pattern. fs.watchFile.test.ts passes per the description. No prior reviews or outstanding comments on the timeline.

[robobun]

^{Updated 4:05 AM PT - Aug 12th, 2026}

❌ @robobun, your commit 4accd62 has some failures in Build #92872 (All Failures)

---

🧪   To try this PR locally:

bunx bun-pr 37591

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

bun-37591 --bun

[Jarred-Sumner]

this is marginally better but is there an even cleaner thing we can do here?

[robobun]

The cleaner version is to not adopt anything in the callbacks: the ref taken when the work-pool task / restat is scheduled should travel as an owned value on the task itself, released when the task is dropped after the callback runs, so the four callbacks stop knowing about refcounts at all. That needs the same owning ref type I am adding for #37618; I will restructure this PR on top of it rather than keep the guards.

[robobun]

Reworked in 5a2628f, now stacked on #37665 (OwnedRef). Instead of guards adopting refs that were taken by hand elsewhere, the refs are values held by their owners: the scheduler's queue pushes and pops OwnedRef<StatWatcher> (one push/pop pair does the raw conversion the intrusive queue needs), InitialStatTask holds its ref as a field, and a hop posted to the JS thread carries one (one post/take pair). Releases are drops, the two hand-written deref calls on the queue and the ref_/deref wrappers are gone, and the hand-offs are stated once per direction. The scheduler's own ref across its work-pool task still uses ScopedRef::adopt, since the intrusive task node has no value to carry it and parking it in a field is not cleaner; the description has the details. Same refs taken and released at the same points and on the same threads as before; unsafe blocks 34 to 27, net minus 107 lines.

[claude]

Beyond the inline nit, I traced the refcount balance on every rewritten path (queue push/pop, post_to_js_thread → take_hop_ref/release_unrun, InitialStatTask::run_owned's Box partial-move, shutdown_for_exit, finalize_js_box) and each ref is released exactly once; the this.loop_handle read after into_raw() in post_to_js_thread matches the pre-PR shape, and WatcherBatch::size_hint is sound (BatchIterator::next decrements batch.count). Given this is a cross-thread refcount-ownership rewrite, a human pass is still worthwhile — note the PR description is stale (describes the earlier +7/-61 ScopedRef swap, not the current OwnedRef restructure).

Extended reasoning...

This is a memory-safety refactor of fs.watchFile's stat-watcher that replaces manual ThreadSafeRefCount::ref_/deref pairing with typed OwnedRef<T> values carried through the queue, work-pool tasks, and JS-thread hops. I verified against src/ptr/ref_count.rs that OwnedRef::acquire bumps, from_raw/into_raw transfer without touching the count, Clone bumps, and Drop releases; and against src/threading/unbounded_queue.rs that BatchIterator decrements count per yield so the ExactSizeIterator impl is correct. The one finding (no Drop on WatcherBatch) is defensive only — both call sites fully drain and the in-loop panics are unreachable — and is not a regression from the pre-PR raw-loop shape. Deferring rather than approving because this touches the most-blocked category (cross-thread intrusive-refcount lifetime across ~15 rewritten functions) and the PR body no longer matches the diff.

[claude]

Re-reviewed after the Drop for WatcherBatch addition — that closes the one gap I flagged, and this pass found nothing further. Given this restructures seven cross-thread refcount hand-offs (queue push/pop, initial-stat task, pool→JS hops, RareData slot) with load-bearing drop ordering in run_owned and post_to_js_thread, a human look would still be worthwhile.

What was reviewed:

• Each OwnedRef acquire/drop traced against the removed ref_()/deref() pair — same points, same threads, including the _finished-before-watcher ordering in run_owned.
• post_to_js_thread's this.loop_handle read after into_raw() — every caller still holds a second ref (queue's or JS wrapper's), same as the pre-PR &self shape.
• WatcherBatch::size_hint reads batch.count, which BatchIterator::next decrements, so len() and the new Drop drain agree.
• finalize → finalize_js_box matches the old body's leak-then-shared-borrow-then-deref sequence.

Extended reasoning...

Overview

Single-file refactor of src/runtime/node/node_fs_stat_watcher.rs (net −107 lines, 34→27 unsafe blocks). Replaces seven hand-paired ref_()/deref() sites with typed OwnedRef<StatWatcher> values carried by their owners: a private WatcherQueue newtype whose push/pop convert to/from raw for the intrusive queue, InitialStatTask.watcher as an owned field, pool→JS hops that move an OwnedRef into the posted task and reconstitute it in take_hop_ref, and shutdown_for_exit adopting the RareData slot's ref. Deletes the two hand-rolled SchedulerRefGuard/WatcherRefGuard types, the StatWatcher::ref_/deref and StatWatcherScheduler::deref wrappers, as_ctx_ptr, and release_hop. Stacked on #37665 which introduces OwnedRef.

Security risks

None identified. No user-facing surface change, no parsing of untrusted input, no auth/crypto. The risk class here is memory safety (UAF/leak from an unbalanced intrusive refcount across the work-pool ↔ JS-thread boundary), not security.

Level of scrutiny

High. This is exactly the category REVIEW.md flags as most-blocked: intrusive refcounts across threads with multiple terminal paths (closed-early, VM teardown, shutdown, success/error hops). Each hand-off's release point moved from an explicit deref() call to a value's drop site, and drop ordering is load-bearing in at least two places (run_owned declares watcher after the _finished guard so the closed-path drop precedes embedded_work_finished(); run_hop releases the hop's ref on return rather than inside each continuation). I traced each new drop against the removed deref() and found them equivalent, but a maintainer familiar with the scheduler's lifetime should confirm.

Other factors

• My prior 🟡 nit (missing Drop on WatcherBatch) was addressed in the current revision — drop now drains un-yielded nodes via self.by_ref().for_each(drop), and size_hint correctly tracks BatchIterator's decrementing batch.count.
• finalize now uses the shared bun_ptr::finalize_js_box idiom; checked it matches the removed body (leak Box → run closure with &T → rc_deref).
• post_to_js_thread reads this.loop_handle through a non-owning this_ptr() after into_raw() — verified every call site (restat via queue's ref, run_owned via JS wrapper's Strong ref) still holds a second ref across the post, same as the pre-PR &self receiver did.
• The one unresolved comment-cop nag on line 127 targets the new two-line Drop doc comment; the author already resolved 18 identical bot fires on the previous revision with the same rationale (ownership documentation, not a workaround justification).
• fs.watchFile.test.ts / fs.watch.test.ts pass per the description; no new test is expected for a behaviour-preserving type-system refactor.

[robobun]

CI status for this revision (build 92872): every lane passed except the two "darwin 26 aarch64" test jobs, which expired in the queue without running (single machine on that tier, long backlog); the same lanes passed on the base PR #37665 (build 92785). The test failures listed on the build all passed on retry. Not retriggering, since a new build would only join the same queue; the two jobs can be retried once the lane drains.

[robobun]

Heads-up: #37857 converts StatWatcher::post_to_js_thread / restat on main to take the watcher's pointer and to clone loop_handle out before posting (the hop can release the watcher's last ref before post_task returns, so the post should not go through this.loop_handle inside the watcher). Whichever of the two lands second needs a small rebase in that function; in this PR's shape that is let handle = watcher.loop_handle.clone(); before into_raw(), then handle.post_task(task).