fs.watch(macOS): make FSEventsLoop Sync and retain the CFRunLoop across shutdown - #30758
fs.watch(macOS): make FSEventsLoop Sync and retain the CFRunLoop across shutdown#30758robobun wants to merge 5 commits into
Conversation
Status — diff green on all macOS lanes, ready for mergeRebased over main (now includes #31783) at tip Fix for the Latest push ( Build 60234 (current tip, complete): 270 passed / 16 failed — every failure is unrelated flake
Prior macOS validation56886 (same functional code, pre-cfg-gate) 285/286, only a known gRPC flake; 55268 all six macOS shards green; 54695 the stress test caught the lifetime race on darwin 14 x64 (pre-CFRetain); 54561 Sync refactor green. None of the changed areas ( Also in this PR
|
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughConvert macOS FSEvents loop to a leak-backed &'static singleton with UnsafeCell-held mutable state and atomic handles; reshape PathWatcher init to return/leak &'static manager captured by spawned threads; add a concurrent regression test for fs.watch close-then-exit behavior. ChangesFSEvents loop refactor (macOS)
PathWatcher static init and thread capture
fs.watch close-exit regression test
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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/watch/fs.watch.close-exit.test.ts`:
- Around line 55-60: The fallback timeout currently calls process.exit(0) which
masks a missed watch callback; change the fallback in the setTimeout block (the
watcher.close() / process.exit call) to fail loudly instead of exiting with 0 —
e.g., make the timeout invoke process.exit(1) or throw an Error with a
descriptive message so the test fails if the watch callback path never ran;
ensure the watcher.close() call remains but the final process exit indicates
failure rather than success.
🪄 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: 6d92e6d1-e07b-45e3-b0e8-2b636e35ea3f
📒 Files selected for processing (3)
src/runtime/node/fs_events.rssrc/runtime/node/path_watcher.rstest/js/node/watch/fs.watch.close-exit.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
robobun
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
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/fs_events.rs (1)
955-985:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRelease the final stream/path CF objects during shutdown.
shutdown()stops the thread and releasessignal_source, but it never tears downstate.fsevent_stream,state.paths, orstate.cf_paths. The last active stream therefore leaks unless_schedule()happened to run a cleanup pass first.Suggested fix
let _guard = self.mutex.lock_guard(); // SAFETY: holding `mutex` — see `FSEventsLoop::state`. let state = unsafe { self.state() }; + let cs = CoreServices::get(); + + if !state.fsevent_stream.is_null() { + unsafe { + (cs.fs_event_stream_stop)(state.fsevent_stream); + (cs.fs_event_stream_invalidate)(state.fsevent_stream); + (cs.fs_event_stream_release)(state.fsevent_stream); + } + state.fsevent_stream = ptr::null_mut(); + } + + if let Some(paths) = state.paths.take() { + for s in paths.iter() { + if !s.is_null() { + unsafe { (cf.release)(*s) }; + } + } + } + + if !state.cf_paths.is_null() { + let cf_paths = state.cf_paths; + state.cf_paths = ptr::null_mut(); + unsafe { (cf.release)(cf_paths) }; + } + if state.watcher_count > 0 { while let Some(watcher) = state.watchers.pop() { if let Some(w) = watcher { bun_ptr::BackRef::from(w).loop_.set(None); }🤖 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/fs_events.rs` around lines 955 - 985, The shutdown() path currently joins the thread and releases signal_source but does not free the CoreFoundation objects held in the FSEventsLoop state; update shutdown() (after acquiring the mutex and reading state via FSEventsLoop::state) to explicitly release state.fsevent_stream, iterate and release any CF objects in state.cf_paths and clear state.paths, and then set state.fsevent_stream and state.cf_paths to null/empty so the last active stream does not leak; use CoreFoundation::get().release(...) to drop each CF object and ensure any Rust-owned Vecs (state.paths) are cleared/consumed while holding the mutex so invariants are preserved (similar to how signal_source is released).
🤖 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/fs_events.rs`:
- Around line 1057-1061: The fast-path in watch() must observe shutdown state so
it doesn't use a loop that's being torn down; add an atomic idempotent
"closing/closed" flag (e.g., FSEVENTS_CLOSING) that is set under
close_and_wait() before teardown and cleared/kept set after teardown, then
update the fast path that reads FSEVENTS_DEFAULT_LOOP (and currently skips
FSEVENTS_DEFAULT_LOOP_MUTEX) to check that flag and refuse/reinitialize the fast
path when closing is true; ensure close_and_wait() sets the flag early and that
any code paths (enqueue_task_concurrent(), signal_source usage, loop_ use)
verify the flag and either return an error or fall back to the slow path so
teardown cannot race with new watchers, and make the flag transitions idempotent
so multiple close_and_wait() calls are safe.
In `@src/runtime/node/path_watcher.rs`:
- Around line 146-151: Create the PathWatcherManager on the heap but do not call
bun_core::heap::release() until Platform::init succeeds: allocate let mut
manager = Box::new(PathWatcherManager::default()), call
Platform::init(&*manager) (update Platform::init's signature to accept
&PathWatcherManager if it currently requires &'static), and only after
Platform::init returns Ok(...) call heap::release(manager) (or
Box::leak(manager)) to publish the singleton; this preserves RAII so a failed
Platform::init (inotify/kqueue/thread spawn) drops the Box instead of leaking.
---
Outside diff comments:
In `@src/runtime/node/fs_events.rs`:
- Around line 955-985: The shutdown() path currently joins the thread and
releases signal_source but does not free the CoreFoundation objects held in the
FSEventsLoop state; update shutdown() (after acquiring the mutex and reading
state via FSEventsLoop::state) to explicitly release state.fsevent_stream,
iterate and release any CF objects in state.cf_paths and clear state.paths, and
then set state.fsevent_stream and state.cf_paths to null/empty so the last
active stream does not leak; use CoreFoundation::get().release(...) to drop each
CF object and ensure any Rust-owned Vecs (state.paths) are cleared/consumed
while holding the mutex so invariants are preserved (similar to how
signal_source is released).
🪄 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: f8628fec-1530-4b4b-a271-1d1ff4deb83a
📒 Files selected for processing (3)
src/runtime/node/fs_events.rssrc/runtime/node/path_watcher.rstest/js/node/watch/fs.watch.close-exit.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/runtime/node/path_watcher.rs (1)
685-697:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftDon't leak the singleton on
spawn()failure.
PathWatcherManager::leak()still happens beforestd::thread::Builder::spawn()in both backends.spawn()can fail for thread/resource limits as well as memory pressure, so repeatedfs.watch()retries can still leak one manager per call here. These branches also collapse the underlying failure toENOMEM, which hides the actual OS error.Keep the manager owned until the reader thread is successfully created, then leak/publish it; on failure, close the fd and translate
err.raw_os_error()instead of hard-codingENOMEM.Possible shape
- let manager = PathWatcherManager::leak(); + let manager = Box::new(PathWatcherManager::default()); manager.platform_fd.set(Fd::from_native(rc)); - match std::thread::Builder::new().spawn(move || Linux::thread_main(manager)) { + let manager_ptr = core::ptr::NonNull::from(manager.as_ref()); + match std::thread::Builder::new().spawn(move || unsafe { + Linux::thread_main(manager_ptr.as_ref()) + }) { Ok(handle) => drop(handle), - Err(_) => { + Err(err) => { manager.platform_fd.get().close(); - return Err(sys::Error::from_code(E::ENOMEM, Tag::watch)); + return Err(sys::Error::from_code_int( + err.raw_os_error().unwrap_or(E::ENOMEM as i32), + Tag::watch, + )); } } - Ok(manager) + Ok(Box::leak(manager))Apply the same pattern in
Kqueue::init().Does Rust `std::thread::Builder::spawn` only fail on OOM, or can it also fail with OS resource-limit errors such as `EAGAIN` / `WouldBlock`? What error kinds or raw OS errors should callers expect?Also applies to: 1231-1243
🤖 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/path_watcher.rs` around lines 685 - 697, Currently PathWatcherManager::leak() is called before spawning the reader thread (in the Linux branch calling Linux::thread_main), which causes a leak if Builder::spawn() fails and also masks the real OS error by returning ENOMEM; change the flow to create the manager as an owned value (do NOT call PathWatcherManager::leak() yet), set platform_fd on it, attempt std::thread::Builder::new().spawn(move || Linux::thread_main(manager_ref_or_owned)), and only call PathWatcherManager::leak() (publish/leak the singleton) after spawn() succeeds; on spawn failure close the manager.platform_fd.get().close() and return Err(sys::Error::from_code(err.raw_os_error().map_or(E::UNKNOWN, |c| c), Tag::watch)) (i.e., translate the actual raw OS error instead of hard-coding ENOMEM); apply the same pattern to Kqueue::init().
🤖 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/fs_events.rs`:
- Around line 633-639: The CF wakeup call can race with shutdown() nulling
loop_, so before calling cf.run_loop_wake_up ensure the loaded loop_ value is
non-null (e.g. check loop_ != 0 / not null) and only call
cf.run_loop_wake_up(loop_) when that check passes; still call
cf.run_loop_source_signal(signal_source) as before. Update the unsafe block
around cf.run_loop_source_signal and cf.run_loop_wake_up to perform this
defensive null check on loop_ (referencing loop_, signal_source,
cf.run_loop_wake_up, cf.run_loop_source_signal, and
enqueue_task_concurrent/shutdown()) so run_loop_wake_up is never invoked with a
null handle.
---
Duplicate comments:
In `@src/runtime/node/path_watcher.rs`:
- Around line 685-697: Currently PathWatcherManager::leak() is called before
spawning the reader thread (in the Linux branch calling Linux::thread_main),
which causes a leak if Builder::spawn() fails and also masks the real OS error
by returning ENOMEM; change the flow to create the manager as an owned value (do
NOT call PathWatcherManager::leak() yet), set platform_fd on it, attempt
std::thread::Builder::new().spawn(move ||
Linux::thread_main(manager_ref_or_owned)), and only call
PathWatcherManager::leak() (publish/leak the singleton) after spawn() succeeds;
on spawn failure close the manager.platform_fd.get().close() and return
Err(sys::Error::from_code(err.raw_os_error().map_or(E::UNKNOWN, |c| c),
Tag::watch)) (i.e., translate the actual raw OS error instead of hard-coding
ENOMEM); apply the same pattern to Kqueue::init().
🪄 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: dbc09d96-203d-4999-84d7-1f91a49e0a25
📒 Files selected for processing (2)
src/runtime/node/fs_events.rssrc/runtime/node/path_watcher.rs
62d5734 to
480b349
Compare
robobun
changed the title
Workaround so the async_hooks PR can be built + verified while #30758 is pending. The Zig-era \`define\` parser auto-quoted non-JSON raw values as strings; the Rust-port parser rejects the leading \`*\` in the minified CSS. The consumer is \`declare const OVERLAY_CSS: string\`, so JSON.stringify is the intended shape regardless. trimEnd() keeps the embedded text byte-identical to the previous codegen path. Identical to the bake-codegen hunk in #30758; drop whichever lands second.
Workaround so the async_hooks PR can be built + verified while #30758 is pending. The Zig-era \`define\` parser auto-quoted non-JSON raw values as strings; the Rust-port parser rejects the leading \`*\` in the minified CSS. The consumer is \`declare const OVERLAY_CSS: string\`, so JSON.stringify is the intended shape regardless. trimEnd() keeps the embedded text byte-identical to the previous codegen path. Identical to the bake-codegen hunk in #30758; drop whichever lands second.
4b3e28b to
f52827a
Compare
There was a problem hiding this comment.
No issues found in the current revision and all prior review feedback has been addressed, but this is a ~400-line unsafe-concurrency refactor (unsafe impl Sync, hand-rolled Acquire/Release on AtomicPtr, CFRetain/CFRelease lifetime across pthread-TSD teardown, UnsafeCell-under-mutex discipline) — worth a human pass on the Sync model and shutdown ordering before merge.
Extended reasoning...
Overview
This PR restructures the macOS fs.watch backend (src/runtime/node/fs_events.rs, ~400 lines of diff) to fix a segfault at 0xC caused by two issues: (1) aliasing UB from &mut FSEventsLoop held across CFRunLoopRun() on the CF thread while the JS thread forms its own &mut, and (2) a use-after-free of the CFRunLoop via pthread-TSD destruction racing CFRunLoopWakeUp. The fix converts FSEventsLoop to a leaked &'static singleton with unsafe impl Sync, moves mutable state into UnsafeCell<FSEventsLoopState> under the existing mutex, makes signal_source/loop_ into AtomicPtr with explicit Acquire/Release, CFRetains the run loop and releases it in a new shutdown(&'static self) after thread.join(), and replaces the Drop impl. path_watcher.rs gets the analogous (smaller) fix for Linux/FreeBSD reader-thread spawns. There's also a new 40-subprocess stress test and a defensive 2-line bake-codegen.ts change.
Security risks
No injection/auth/data-exposure surface. The risk class here is memory safety: unsafe impl Sync with a per-field SAFETY justification, raw-pointer recovery in CF callbacks, manual memory ordering, and CF object lifetime across thread exit. A mistake in any of these invariants is a UAF or data race in a runtime hot path. The reasoning in the PR is detailed and internally consistent (sem handshake orders loop_ store before all enqueues; thread field is JS-thread-only; state only touched under mutex; CFRetain keeps the run loop alive past TSD teardown), and CI including the new stress test is green on all macOS lanes that ran.
Level of scrutiny
High. This is production-critical runtime code (fs.watch on macOS) with hand-written unsafe impl Sync, cross-thread atomics, and FFI lifetime management. The design choices — leak-as-&'static + OnceLock (never reset), shutdown() instead of Drop, the "watch()-after-shutdown is unreachable because Bun__onExit runs after VM stops scheduling" invariant, and the Stacked-Borrows provenance reasoning for Task::new — are all sound as argued but are exactly the kind of architectural decisions a maintainer should sign off on, not a bot.
Other factors
The PR has been through multiple review rounds (CodeRabbit + my own inline comments on argv indexing, Builder::spawn, SAFETY-comment accuracy, and leak-comment wording); every comment has been addressed and resolved. The bug-hunting system found nothing in the current revision. The author's status comment reports the diff green across 284 CI shards. None of that changes the fact that this is a substantial unsafe-Rust concurrency redesign rather than a mechanical fix.
f52827a to
b8206a9
Compare
There was a problem hiding this comment.
All prior review feedback has been addressed and the bug-hunter found nothing on the rebased commits, but this is a hand-rolled unsafe impl Sync + AtomicPtr/UnsafeCell concurrency refactor with CF object lifetime management across two threads — worth a maintainer's eyes before merge.
Extended reasoning...
Overview
This PR refactors the macOS FSEventsLoop (~330 lines net in fs_events.rs) from a &mut self-everywhere design to a properly Sync shared-&'static singleton: AtomicPtr<c_void> for the cross-thread CF handles, UnsafeCell<FSEventsLoopState> under the existing mutex for watcher/stream bookkeeping, shutdown(&self) replacing Drop, and CFRetain/CFRelease on the run loop so it outlives the CF thread's pthread-TSD destructor. It also reshapes PathWatcherManager init on Linux/FreeBSD so the reader-thread closure captures &'static PathWatcherManager directly (no more usize pointer laundering), with the fallible syscall hoisted before the leak. A new 40-subprocess stress test and a small bake-codegen.ts define-quoting fix round it out.
Security risks
None identified. No user-controlled input reaches new unsafe blocks; the changes are internal threading/lifetime mechanics for fs.watch(). No auth, crypto, network, or permission surfaces are touched.
Level of scrutiny
High. The core of this PR is an unsafe impl Sync for FSEventsLoop whose soundness rests on hand-stated per-field invariants (which threads touch which UnsafeCell/AtomicPtr fields, under which lock, in which order), plus Acquire/Release memory ordering on AtomicPtr paired with a semaphore handshake, plus CF Get-rule lifetime reasoning for CFRunLoopGetCurrent(). The reasoning is thorough and well-commented, and the stress test caught the second (lifetime) half of the bug in CI — but this is exactly the class of change where a maintainer familiar with the FSEvents/CFRunLoop code paths and the project's unsafe-Rust conventions should sign off.
Other factors
- All five of my prior inline comments and all four CodeRabbit findings have been addressed and resolved by the author with point-by-point fixes.
- The branch was rebased over #31116 (workspace clippy) with non-trivial resolutions documented in the PR description (e.g.,
mut_from_refdeny → inlinedUnsafeCellaccess at each site,UnboundedQueue::pushnowNonNull<T>); these resolutions look correct against the current diff. - CI is reported green on all macOS lanes including the original crash platform.
- The
bake-codegen.tsand test-file changes are straightforward; the substance is entirely in the two.rsfiles.
I'm not approving because this is neither simple nor mechanical — it's a correctness-critical concurrency redesign with unsafe impl Sync, and the guidelines explicitly call out complex changes touching critical code paths as requiring human review.
b8206a9 to
7cc882a
Compare
There was a problem hiding this comment.
No bugs found and all earlier nits are addressed, but the core of this PR is an unsafe impl Sync over UnsafeCell state plus cross-thread CFRunLoop retain/release ordering — worth a human pass over the SAFETY justifications in fs_events.rs before merge.
Extended reasoning...
Overview
This PR refactors the macOS fs.watch backend (src/runtime/node/fs_events.rs, ~400 lines reshaped) to fix a segfault at 0xC caused by two issues: aliased &mut FSEventsLoop across the CF thread's CFRunLoopRun() (Rust-port UB), and a CFRunLoop use-after-free during shutdown (present in the Zig original). The fix converts FSEventsLoop to a leaked &'static singleton with AtomicPtr scalars + UnsafeCell<FSEventsLoopState> under a mutex, adds unsafe impl Sync, replaces Drop with shutdown(&self), and CFRetains the run loop so it outlives the CF thread's pthread-TSD destructor. It also reshapes PathWatcherManager init on Linux/FreeBSD to capture &'static in the reader-thread closure (removing usize pointer laundering), adds a 40-subprocess stress test, and includes a small defensive JSON.stringify fix in bake-codegen.ts.
Security risks
None identified. No auth, crypto, network, or user-input parsing is touched. The changes are internal threading/lifetime mechanics for filesystem watching.
Level of scrutiny
High. The PR hand-writes unsafe impl Sync for FSEventsLoop with per-field SAFETY justifications, reasons about Acquire/Release ordering across a semaphore handshake, manages CoreFoundation refcounts across thread boundaries, and relies on Stacked Borrows provenance arguments for why a single &'static reborrow is sound. The reasoning in the comments is detailed and looks correct to me, and the new stress test caught the lifetime half on CI — but this is exactly the class of change (hand-rolled Sync over UnsafeCell, FFI lifetime management at process exit) where a second human reading of the invariants is valuable. This is production code on the fs.watch hot path for every macOS user.
Other factors
- All five of my earlier inline comments (argv index bug in the test,
Builder::spawnvs panic, two SAFETY-comment inaccuracies, and a stale field doc) were addressed and resolved. - CodeRabbit's concerns (retry-leak in
PathWatcherManager, idempotent shutdown) were also addressed. - CI is green on all macOS lanes per the robobun status comment, including the originally-crashing test and the new stress test.
- The
bake-codegen.tschange is unrelated to the crash fix but is a small, defensive, byte-identical-output change. - The branch was rebased over the #31116 workspace clippy sweep with documented non-trivial resolutions (e.g. inlining
UnsafeCellaccess to satisfymut_from_refdeny).
There was a problem hiding this comment.
No further findings from me — all prior nits addressed. That said, this is a ~400-line unsafe concurrency refactor (manual unsafe impl Sync, UnsafeCell under a hand-rolled mutex, AtomicPtr ordering, CFRunLoop retain/release across thread join), so I'd like a human to sign off on the soundness argument rather than auto-approving.
Extended reasoning...
Overview
This PR rewrites the macOS FSEventsLoop (src/runtime/node/fs_events.rs, ~400 net lines changed) to fix a segfault caused by two distinct issues: (1) aliased &mut FSEventsLoop held across CFRunLoopRun() on the CF thread while the JS thread forms its own &mut (UB under Stacked Borrows / noalias), and (2) a CFRunLoop lifetime race where CFRunLoopWakeUp could be called on a freed run loop after the CF thread's pthread-TSD destructor ran. The fix converts FSEventsLoop to a leaked &'static singleton with AtomicPtr cross-thread scalars, UnsafeCell<FSEventsLoopState> under mutex, unsafe impl Sync, and CFRetain/CFRelease bracketing the run loop across thread.join(). It also reshapes PathWatcherManager::Platform::init (Linux/FreeBSD/macOS/Windows) to do the fallible syscall before leaking and capture &'static directly in the reader-thread closure, cfg-gates the fs_events module to macOS, adds a 40-subprocess stress test, and applies a defensive JSON.stringify to a bake-codegen define value.
Security risks
None identified. No auth/crypto/permissions surface; the change is internal threading/FFI plumbing for fs.watch. The bake-codegen.ts change is a build-time codegen script wrapping a CSS string in JSON.stringify — defensive, output-neutral per the PR description.
Level of scrutiny
High. This is exactly the class of change that should not be auto-approved:
- Hand-written
unsafe impl Sync for FSEventsLoopwith a multi-bullet SAFETY justification whose correctness depends on every access site honoring the "touchstate⇒ holdmutex" invariant and the "threadis JS-thread-only" invariant. - Manual
AtomicPtrwith explicit Acquire/Release ordering paired with a semaphore handshake — the soundness argument spansinit(),cf_thread_loop(),enqueue_task_concurrent(), andshutdown(). - CoreFoundation FFI lifetime management (
CFRetain/CFRelease) reasoning about pthread TSD destructor ordering vs. cross-threadCFRunLoopWakeUp. shutdown()replacesDropspecifically because&mut selfwould alias the CF thread's&'static— a subtle Stacked Borrows argument.- Touches four platform backends (Linux inotify, FreeBSD kqueue, macOS FSEvents, Windows stub) in
path_watcher.rs.
The reasoning in the code comments and PR description is exceptionally thorough, and CI is green on all six macOS shards (including the new stress test that caught the lifetime half on darwin 14 x64). But the correctness of unsafe impl Sync + manual memory ordering in a runtime-critical path is the kind of thing a human Rust reviewer should validate.
Other factors
- All five of my prior inline comments (argv index bug,
thread::spawnpanic, two SAFETY-comment inaccuracies, staleloop_doc) were addressed by the author and resolved. - All CodeRabbit findings were addressed or convincingly rebutted; all threads resolved.
- No outstanding reviewer comments.
- The bug-hunting system found nothing in the current revision.
- The PR has been rebased over a workspace clippy sweep with documented non-trivial resolutions.
Deferring solely on complexity/scope, not on any identified defect.
367af90 to
5910806
Compare
There was a problem hiding this comment.
All prior review feedback has been addressed and the bug hunter found nothing on this revision, but this is a ~400-line refactor of cross-thread unsafe Rust (hand-justified unsafe impl Sync, UnsafeCell-under-mutex state, CFRetain/CFRelease lifecycle across a joined thread) — worth a human pass over the SAFETY invariants before merge.
Extended reasoning...
Overview
This PR fixes a macOS segfault in fs.watch by restructuring FSEventsLoop (src/runtime/node/fs_events.rs, ~400 lines changed) from a &mut self-everywhere design — which created aliased &mut UB across the CoreFoundation thread and the JS thread — into a properly Sync type: AtomicPtr for cross-thread scalars, UnsafeCell<FSEventsLoopState> guarded by an existing mutex, leaked as &'static into a OnceLock, with shutdown(&self) replacing Drop. It also adds CFRetain of the run loop so it outlives the CF thread's pthread-TSD destructor (closing a signal/wake race that existed in the Zig original too). path_watcher.rs gets the analogous fix for the Linux/FreeBSD reader-thread spawn (fallible syscall before leak; capture &'static directly instead of usize round-trip). node.rs cfg-gates the module to macOS. bake-codegen.ts gets a small defensive JSON.stringify wrap. A new 40-subprocess stress test is added.
Security risks
None identified. This is internal runtime concurrency plumbing for fs.watch; no auth, crypto, network, or user-input parsing is touched. The unsafe blocks are about memory-model soundness (aliasing, ordering, CF refcounts), not privilege boundaries.
Level of scrutiny
High. The correctness of this change rests entirely on hand-written SAFETY justifications for unsafe impl Sync for FSEventsLoop, the per-field access discipline (which thread touches what, under which lock, with which ordering), and the CFRetain/CFRelease pairing across cf_thread_loop ↔ shutdown() after thread.join(). These invariants are extensively documented in-line and look correct to me, but getting any one of them wrong reintroduces UB that only manifests as a rare crash under load on one platform. This is exactly the kind of change where a second human familiar with Rust's aliasing model and CoreFoundation should read the SAFETY comments against the code.
Other factors
- All five of my prior inline comments (one 🔴 on the test's
argvindex, four 🟡 nits on SAFETY/doc-comment accuracy andBuilder::spawnerror handling) were addressed by the author and are resolved. - All three CodeRabbit findings were addressed or withdrawn after discussion.
- CI is green on all six macOS shards per the author's status comment; the new stress test caught the lifetime half of the bug on darwin x64 before the CFRetain fix was added.
- The PR description is thorough and the rebase-over-clippy-sweep resolutions are documented.
- This does not fit the auto-approve criteria (simple/mechanical/obvious); deferring.
…ss shutdown Fixes a segfault at address 0xC in fs.watch on macOS (reported from test/js/node/async_hooks/async-context/async-context-fs-watch.js). Two root causes: 1. Visibility (Rust-port regression): cf_thread_loop(&mut self) held a noalias &mut FSEventsLoop across CFRunLoopRun() while the JS thread formed its own &mut in register_watcher/unregister_watcher/Drop — aliasing UB. FSEventsLoop is now properly Sync: loop_/signal_source are AtomicPtr, watcher/stream state lives in UnsafeCell under the existing mutex, every method takes &self, init() leaks a &'static FSEventsLoop into a OnceLock, and the CF-thread closure captures that reference directly. shutdown(&self) replaces Drop. 2. Lifetime (present in the Zig original too): CFRunLoopGetCurrent() follows the Get-rule; when the CF thread exits, pthread TSD frees its run loop, so the JS thread's trailing CFRunLoopWakeUp(loop) after CFRunLoopSourceSignal on the _stop enqueue could hit a freed pointer (fault at CFRuntimeBase._rc, +0xC). cf_thread_loop now CFRetains the run loop and shutdown() CFReleases it after thread.join(). Also: - PathWatcherManager (Linux/FreeBSD): Platform::init does its fallible syscall first, then leaks the manager and captures &'static PathWatcherManager in the reader-thread closure — no unsafe in the threading, and the EMFILE retry path no longer leaks. - bake-codegen.ts: JSON.stringify the OVERLAY_CSS define value. - New stress test test/js/node/watch/fs.watch.close-exit.test.ts (watch -> close-in-callback -> process.exit across 40 subprocesses); it caught the lifetime half on darwin 14 x64 in CI.
- inline the UnsafeCell state access at each call site (mut_from_ref forbids the &self -> &mut accessor; matches PathWatcherManager) - cfg(macos) shutdown(), cfg(not(windows)) PathWatcherManager::leak() (dead_code is deny now) - name the CF thread via Builder::name, matching main - drop the no-longer-needed # Safety on FSEventsWatcher::init (&'static param replaced the raw-pointer contract)
The FSEvents backend is only reachable from path_watcher's Darwin code (whose import was already cfg(target_os = "macos")), so declare the module itself behind the same cfg in node.rs. That removes every allow(dead_code) escape hatch, the cfg(not(unix)) dlsym stub, and the per-item macos gates (shutdown, close_and_wait body) that only existed because the file used to compile on every platform.
5910806 to
56e9ca5
Compare
The rebase over #31783 reverted the CoreFoundation/CoreServices no-deinit comments back to commented-out Zig blocks; restore main's wording, which also carries the leaked-for-process-lifetime invariant the Send SAFETY comment references.
Restore main's wording for the two reverted hunks (module doc, PathWatcher::new doc) and replace the remaining Zig-era identifiers in comments with the real Rust names (deinit -> FSEventsWatcher::drop / remove_watch, addWatch/removeWatch, onFSEvent, unregisterWatcher, walkAndAdd, addOne, getOrPut, getFdPath, onPathUpdatePosix, runExitCallbacks, Zig enum-literal syntax).
|
Superseded by #33303, which landed this work on Closing this PR as redundant. Thanks for picking it up. |
1 participant
Crash
on macOS aarch64 (and, under stress, x64) release. The fixture does
fs.watch(file, cb)→ trigger → insidecb:watcher.close()+process.exit(0).Cause — two halves
1. Visibility (Rust-port regression)
FSEventsLoopspawned the CoreFoundation thread by laundering*mut FSEventsLoopthroughusize, andcf_thread_loop(&mut self)then held anoalias&mut FSEventsLoopfor the entire CF-thread lifetime (acrossCFRunLoopRun()). The JS thread concurrently forms its own&mut FSEventsLoopinregister_watcher/unregister_watcher/Drop. Two live&mutto one allocation is UB regardless of synchronization; undernoaliasLLVM is free to treat the CF thread'sself.loop_ = CFRunLoopGetCurrent()write as invisible to the JS thread's read.The same
usizeround-trip existed in the Linux inotify and FreeBSD kqueue reader-thread spawns inpath_watcher.rs.2. Lifetime (present in the Zig original too)
CFRunLoopGetCurrent()follows CF's Get-rule — the caller doesn't own a reference. When the CF thread function returns, the pthread TSD destructor releases the thread's run loop.enqueue_task_concurrent()doesCFRunLoopSourceSignal(src); CFRunLoopWakeUp(loop); on the_stopenqueue inshutdown()the CF thread can drain_stopoff the signal alone, runCFRunLoopStop, return fromCFRunLoopRun(), and fully exit (freeing the CFRunLoop via TSD) before the JS thread reachesCFRunLoopWakeUp(loop)— which then faults atCFRuntimeBase._rc(+0xC).The new stress test caught (2) on darwin 14 x64 in CI after (1) alone was applied.
Fix
FSEventsLoop(macOS):loop_/signal_source→AtomicPtr<c_void>; watchers/stream state →UnsafeCell<FSEventsLoopState>under the existingmutex;unsafe impl Sync; every method takes&self;init()leaks a&'static FSEventsLoopinto aOnceLock; the CF-thread closure captures that reference directly (T: Sync⇒&'static T: Send)shutdown(&self)replacesDrop(a&mut selfdrop would alias the CF thread's&'static)cf_thread_loopCFRetains the run loop before publishing it;shutdown()CFReleases afterthread.join()— the run loop now outlives every JS-thread reader, andCFRunLoopWakeUpon a stopped-but-alive loop is a documented no-opTask::newtakes&'static T/fn(&T)so the stored ctx ptr is stable under Stacked BorrowsPathWatcherManager(Linux/FreeBSD):Platform::initdoes its fallible syscall first, then leaks the manager and captures&'static PathWatcherManagerin the reader-thread closure. Nounsafein the threading; retry path doesn't leak.bake-codegen.ts:JSON.stringifytheOVERLAY_CSSdefine value (defensive; #30679 now also fixes the lexer side; output byte-identical).Verification
cargo check -p bun_runtimeonx86_64-linux,aarch64-apple-darwin,x86_64-unknown-freebsd,aarch64-pc-windows-msvcbun bd test test/js/node/watch/— full fs.watch suitetest/js/node/watch/fs.watch.close-exit.test.ts— new stress test (watch → close-in-callback →process.exit) × 40 subprocesses; this is what caught the lifetime half on CIbun bd test test/js/node/async_hooks/AsyncLocalStorage-tracking.test.ts -t fs-watch— original crashing testRebase notes
Post-#31116 (workspace clippy):
Semaphore→bun_threading::Semaphore(main deleted the local one)UnboundedQueue::pushnow takesNonNull<T>&self → &mutstate()accessor (mut_from_refis deny); theUnsafeCellaccess is now inlined at each call site with a SAFETY comment, matchingPathWatcherManager#[cfg(not(windows))]onPathWatcherManager::leak()(dead_codeis deny); per review, the wholefs_eventsmodule is declared behind#[cfg(target_os = "macos")]innode.rsinstead of per-item attributesBuilder::name, matching main's conventionPost-#31783 (TODO(port)/Zig-comment sweep): the sweep rewrote comments in the pre-refactor
fs_events.rs/path_watcher.rs, which this PR replaces. Resolved by keeping this PR's code and applying the same comment convention to it: removed all Zig cross-references,PORT NOTE/PERF(port)tags, and trailingported from:lines from both files (reusing the sweep's exact replacement wording where the text overlapped). No code changes from the resolution; re-verified with cross-targetcargo check/clippyand the full fs.watch suite.