fs.watch: type FSWatcher's wrapper self-reference as JsRef instead of a bare JSValue#31848
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
… a bare JSValue FSWatcher stored its reference to its own JS wrapper as a bare Cell<JSValue>, an untyped encoded value with no expression of its GC contract. The wrapper's liveness is rooted by hasPendingActivity() (pending_activity_count starts at 1 and stays positive until close/detach), so the field is now a JsCell<JsRef> held weak on purpose — a Strong here would be a self-reference cycle that pins the wrapper forever. All readers (emit, emit_with_filename, emit_error, emit_abort, close, the js_this() accessor) now go through JsRef::try_get() and copy the value out before any re-entrant listener call, and detach() idempotently resets the ref to JsRef::empty(). Behavior is unchanged; the field is now typed for its GC role and every read is null-checked through one audited path. Tested with a new subprocess GC-stress test in fs.watch.test.ts that forces Bun.gc(true) between watcher creation, event delivery, abort-signal abort, re-entrant close-from-listener, double close, and close-event emission, asserting all events arrive and the process exits cleanly.
Jarred-Sumner
force-pushed
the
claude/complex-17-fs-watch-type-fswatcher-s-wrapper-self-r
branch
from
c1d5bf6 to
1123295
Compare
|
Warning Review limit reached
More reviews will be available in 10 minutes and 23 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
Comment |
There was a problem hiding this comment.
The Cell → JsCell swap looks mechanically 1:1 and behavior-preserving (JsRef::Weak is just a tagged JSValue), but since it re-expresses the GC rooting contract for FSWatcher's self-wrapper in native runtime code, it's worth a quick human look.
Extended reasoning...
Overview
This PR retypes FSWatcher.js_this from Cell<JSValue> to JsCell<JsRef> in src/runtime/node/node_fs_watcher.rs, with the ref held in Weak mode (which is literally enum { Weak(JSValue), ... } — the same payload as before, just tagged). All read sites move from if !js_this.is_empty() to if let Some(js_this) = ...try_get(), the sentinel moves from JSValue::ZERO to JsRef::empty() (= Weak(UNDEFINED)), detach() clears via JsRef::empty(), and the field goes from pub(super) to private (verified: no super-module reads it; win_watcher only touches encoding). The only external caller of js_this() is NodeFS::watch at node_fs.rs:8184, which reads it immediately after init_js has populated it, so the ZERO→UNDEFINED sentinel change for the cleared state is unobservable. A new subprocess GC-stress test is added to fs.watch.test.ts.
Security risks
None. No new inputs, no parsing, no auth/crypto/permissions surface. This is a storage-type refactor of an internal field.
Level of scrutiny
Medium. The transformation is mechanical and JsRef::Weak engages no additional GC machinery (it's a bare JSValue inside an enum), so the actual rooting story — wrapper kept alive by hasPendingActivity() while pending_activity_count > 0 — is unchanged. try_get() checks is_empty_or_undefined_or_null() rather than just is_empty(), which is strictly more conservative and equivalent here since the field is only ever a wrapper object or the cleared sentinel. JsCell::get() returns &JsRef and try_get() immediately copies out an Option<JSValue>, so no borrow is held across the re-entrant listener calls. That said, this is native JSC-GC-adjacent code where rooting mistakes are subtle, so a human familiar with the JsRef/hasPendingActivity pattern signing off seems prudent.
Other factors
- The bug-hunting system found nothing.
- My previous nit about the explicit
30_000test timeout was answered by the author (file-local precedent + bounded ~20s worst case under slow CI) and the thread is resolved; I'm satisfied with that. - No CODEOWNERS cover these paths.
- The new test is additive and follows the existing subprocess-fixture pattern in the same file.
Jarred-Sumner
left a comment
Jarred-Sumner
left a comment
There was a problem hiding this comment.
get().try_get()
Really? We're doing .get().get() and .get().try_get()? This is slop.
…uble-get call sites
|
Added forwarding accessors on |
Jarred-Sumner
deleted the
claude/complex-17-fs-watch-type-fswatcher-s-wrapper-self-r
branch
2 participants
FSWatcher stored its reference to its own JS wrapper as a bare Cell, an untyped encoded value with no expression of its GC contract. The wrapper's liveness is rooted by hasPendingActivity() (pending_activity_count starts at 1 and stays positive until close/detach), so the field is now a JsCell held weak on purpose — a Strong here would be a self-reference cycle that pins the wrapper forever. All readers (emit, emit_with_filename, emit_error, emit_abort, close, the js_this() accessor) now go through JsRef::try_get() and copy the value out before any re-entrant listener call, and detach() idempotently resets the ref to JsRef::empty(). Behavior is unchanged; the field is now typed for its GC role and every read is null-checked through one audited path. Tested with a new subprocess GC-stress test in fs.watch.test.ts that forces Bun.gc(true) between watcher creation, event delivery, abort-signal abort, re-entrant close-from-listener, double close, and close-event emission, asserting all events arrive and the process exits cleanly.
Verification
Implemented and verified on a unified integration branch: full debug build (linux-x64, ASAN), cargo check across the workspace, and the affected test files run against the debug build (failures cross-checked against main's build to exclude pre-existing issues). Each change was reviewed twice (compile/API correctness and GC/concurrency/semantics lenses) with findings repaired before landing.