fs.watch(macOS): make FSEventsLoop Sync and retain the CFRunLoop across shutdown - #33303
Conversation
|
Updated 3:58 PM PT - Jul 4th, 2026
✅ @autofix-ci[bot], your commit 82165c8ac403ed955aead18a2c5510fe4e12211a passed in 🧪 To try this PR locally: bunx bun-pr 33303That installs a local version of the PR into your bun-33303 --bun |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
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:
WalkthroughRefactors macOS FSEvents and PathWatcher initialization to use leaked ChangesFSEvents singleton refactor
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| // through the signal and exit code (stderr is included for diagnostics). | ||
| expect({ | ||
| stdout, | ||
| signalCode: proc.signalCode, | ||
| exitCode, | ||
| crash: stderr.includes("panic") || stderr.includes("Segmentation fault"), | ||
| }).toEqual({ |
There was a problem hiding this comment.
🟡 The crash: stderr.includes("panic") || stderr.includes("Segmentation fault") field is the pattern CLAUDE.md prohibits ("NEVER write tests that check for no 'panic' … in the test output"). signalCode: null and exitCode: 0 are the load-bearing assertions here — a real segfault surfaces through those — so crash adds no coverage and would false-positive if a benign debug/ASAN stderr line ever contained the substring "panic". Drop the crash field; keep stderr in the asserted object purely for diagnostic output on failure.
Extended reasoning...
What this is
Root CLAUDE.md §Writing Tests states verbatim:
NEVER write tests that check for no "panic" or "uncaught exception" or similar in the test output. These tests will never fail in CI.
The new test's assertion object at fs.watch.close-exit.test.ts:90-96 includes:
expect({ stdout, signalCode: proc.signalCode, exitCode, crash: stderr.includes("panic") || stderr.includes("Segmentation fault") }).toEqual({
stdout: "",
signalCode: null,
exitCode: 0,
crash: false,
});The crash: stderr.includes("panic") || stderr.includes("Segmentation fault") → crash: false half is exactly the prohibited shape.
Why it's redundant
Walk through what happens on the regression this test guards:
- Unpatched build segfaults inside CoreFoundation at
+0xC. - Bun's crash handler catches SIGSEGV, prints the crash report to stderr, then re-raises the signal / exits non-zero.
- The subprocess terminates with either
proc.signalCode === "SIGSEGV"(re-raised) orexitCode !== 0(crash-handler exit). expect({ …, signalCode: null, exitCode: 0, … }).toEqual(…)fails on those fields.
So signalCode: null and exitCode: 0 are already the load-bearing assertions. The crash field never fires independently of them — there is no path where Bun writes "panic" or "Segmentation fault" to stderr and then exits cleanly with code 0 and no signal.
Why it's a (small) liability
The test's own comment at line 88 acknowledges "Debug/ASAN builds may write benign warnings to stderr" — that's why the assertion doesn't require stderr === "". But substring-matching that same benign stderr for "panic" contradicts the acknowledgment: a debug log line like "installed panic handler" or an ASAN diagnostic that happens to contain the substring would flip crash to true and fail a passing build. The rule exists precisely because stderr content on debug/ASAN CI lanes is not stable enough to assert on.
Fix
Drop the crash field from both sides of the toEqual. If you want stderr visible in the failure diff for diagnostics, include it as a raw value without asserting on its content:
expect({ stdout, stderr, signalCode: proc.signalCode, exitCode }).toEqual({
stdout: "",
stderr, // echoed for diagnostics; not asserted
signalCode: null,
exitCode: 0,
});or simply omit stderr entirely since { stdout, signalCode, exitCode } already fully characterizes "did not crash".
Severity
Nit. The rule's intent — preventing tests whose only assertion is a stderr grep that never fails — is not violated here, because signalCode/exitCode do catch the regression. This is a redundant assertion matching a documented anti-pattern with a marginal false-positive risk, not a correctness defect; it doesn't need to block merge.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 382-385: The FSEventsLoopState::state helper is triggering Clippy
for intentionally returning &mut from &self and for missing an inline unsafe
justification. Add a targeted allow for clippy::mut_from_ref on state(), and
change the unsafe block in state() to include a // SAFETY: comment directly on
the block so clippy::undocumented_unsafe_blocks is satisfied.
In `@test/js/node/watch/fs.watch.close-exit.test.ts`:
- Around line 4-28: The comments in the fs.watch regression test are too long
and repeat the full root-cause analysis instead of briefly describing
setup/action/assertion. Trim the file-level comment and the inline comments in
the embedded script to a short reference to the related PR/commit plus 1-2 lines
on what the test exercises, and remove the mechanism details about pointer
laundering, noalias, and aliasing from the test body. Keep the wording concise
in the test file, especially around the
watch/trigger/close-in-callback/process.exit flow in the embedded script.
🪄 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: ce0489d0-72d1-4d46-b214-b1e79dfedfaf
📒 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
| // Regression test for pointer-provenance UB in the fs.watch backends | ||
| // introduced by the Rust port. | ||
| // | ||
| // `FSEventsLoop::init()` spawned the CoreFoundation thread by laundering | ||
| // `*mut FSEventsLoop` through `usize` (`this as usize` → `addr as *mut _`) to | ||
| // satisfy `Send` on the closure. That strips provenance: the CF thread's | ||
| // writes to `self.loop_` become disconnected from the JS thread's reads. | ||
| // Compounding this, `cf_thread_loop` took `&mut self` and held it across | ||
| // `CFRunLoopRun()`, so the JS thread's `&mut FSEventsLoop` in | ||
| // `register_watcher`/`unregister_watcher`/`Drop` aliased it — two live | ||
| // `&mut` to one allocation is UB regardless of synchronization. Under | ||
| // `noalias` LLVM is free to treat the CF thread's | ||
| // `self.loop_ = CFRunLoopGetCurrent()` as invisible, so the JS thread's | ||
| // `enqueue_task_concurrent` reads a stale `NULL` and calls | ||
| // `CFRunLoopWakeUp(NULL)`, faulting inside CoreFoundation at +0xC. | ||
| // | ||
| // The same `usize` round-trip existed in the Linux inotify and FreeBSD | ||
| // kqueue reader-thread spawns; fixed together. | ||
| // | ||
| // Field report: test/js/node/async_hooks/async-context/async-context-fs-watch.js | ||
| // crashed on macOS aarch64 release with "Segmentation fault at address 0xC". | ||
| // | ||
| // This test hammers the exact sequence from that report — watch → trigger → | ||
| // close-in-callback → process.exit — across many subprocesses so the optimizer | ||
| // has plenty of chances to exploit the UB. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Trim the extensive root-cause commentary.
The file-level comment (Lines 4-28) and several inline comments inside the embedded script restate the full UB root-cause analysis (pointer laundering, noalias, aliasing &mut, etc.) rather than staying with setup/action/assertion framing. As per coding guidelines, comments should be kept concise ("Keep code comments to 3 lines max... If the code needs more explanation than that, it belongs in docs"), and per a retrieved learning, inline comments in test bodies should not restate bug context — that belongs in the PR description/commit message.
Condense to a short pointer (e.g., link to the PR/commit) plus 1-2 lines on what the test exercises; drop the mechanism narrative from the test body.
Also applies to: 46-53, 55-66, 74-77
🤖 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 `@test/js/node/watch/fs.watch.close-exit.test.ts` around lines 4 - 28, The
comments in the fs.watch regression test are too long and repeat the full
root-cause analysis instead of briefly describing setup/action/assertion. Trim
the file-level comment and the inline comments in the embedded script to a short
reference to the related PR/commit plus 1-2 lines on what the test exercises,
and remove the mechanism details about pointer laundering, noalias, and aliasing
from the test body. Keep the wording concise in the test file, especially around
the watch/trigger/close-in-callback/process.exit flow in the embedded script.
Sources: Coding guidelines, Learnings
|
CI: the only real failure was |
346dc82 to
e1c02b6
Compare
| debug_assert!(!loop_.is_null()); | ||
| // SAFETY: retained in `cf_thread_loop`; sole owner after join. | ||
| unsafe { (cf.release)(loop_) }; | ||
|
|
||
| let signal_source = self.signal_source.swap(ptr::null_mut(), Ordering::Relaxed); | ||
| debug_assert!(!signal_source.is_null()); | ||
| // SAFETY: signal_source is a valid CF object until released here | ||
| unsafe { (cf.release)(self.signal_source) }; | ||
| self.signal_source = ptr::null_mut(); | ||
| unsafe { (cf.release)(signal_source) }; | ||
|
|
There was a problem hiding this comment.
🟡 The doc comment says a still-running Worker calling watch() during shutdown() "now sees a shut-down loop instead", but that isn't benign: shutdown() swaps loop_/signal_source to null before taking self.mutex, and enqueue_task_concurrent() passes those loads straight to CFRunLoopSourceSignal/CFRunLoopWakeUp with no null check — so a Worker's register_watcher/unregister_watcher interleaving here still faults at CFRuntimeBase+0xC via NULL. Not a regression (the old code freed the whole struct → strictly-worse UAF), but since the process is exiting anyway you could just skip the swap-to-null + CFRelease and leak the ~200 bytes of CF handles (the FSEventsLoop itself is already leaked), which makes the comment's claim actually true.
Extended reasoning...
What the bug is
shutdown() at fs_events.rs:978-987 swaps self.loop_ and self.signal_source to null and CFReleases them without holding self.mutex (the lock is taken only afterward at line 991). Meanwhile enqueue_task_concurrent() at fs_events.rs:626-631 loads both atomics and passes them directly to CFRunLoopSourceSignal / CFRunLoopWakeUp with no null check. Neither CF function tolerates NULL — both immediately lock the argument's CF spinlock at CFRuntimeBase+0xC, which is exactly the fault signature this PR is fixing for the single-threaded case.
The shutdown() doc comment at lines 954-956 explicitly names the scenario — "A still-running Worker calling watch() in that window raced the old code's full free of the loop; it now sees a shut-down loop instead" — as if seeing a shut-down loop were safe. It isn't: touching that shut-down loop via register_watcher/unregister_watcher still crashes.
The code path that triggers it
watch()'s fast path (fs_events.rs:1074-1078) does an unlocked FSEVENTS_DEFAULT_LOOP.get() and calls FSEventsWatcher::init directly — it does not take FSEVENTS_DEFAULT_LOOP_MUTEX. Only close_and_wait() takes that mutex, so it provides no exclusion between a Worker's watch() and the main thread's shutdown(). The only shared lock is self.mutex, and shutdown() swaps the CF handles to null before acquiring it.
On macOS, Bun__onExit is registered via atexit() (c-bindings.cpp:708), and libc runs atexit handlers on the calling thread while other pthreads keep running — Workers are not terminated first. So a Worker thread can be actively inside fs.watch() while the main thread runs shutdown().
Why existing code doesn't prevent it
FSEVENTS_DEFAULT_LOOP_MUTEX: only held byclose_and_wait()andwatch()'s slow path; the fast path bypasses it.self.mutex:register_watcher/unregister_watcherhold it, butshutdown()swaps the CF handles at lines 978-986 before taking it at line 991, so there is no serialization across the swaps.enqueue_task_concurrent's comment at lines 619-625 says "every enqueue sees a non-null run loop" — true only for enqueues ordered beforeshutdown()'s swaps; a concurrent Worker enqueue is not.
Step-by-step proof
- Main thread:
process.exit()→ atexit →Bun__onExit→close_and_wait()takesFSEVENTS_DEFAULT_LOOP_MUTEX→shutdown(). shutdown():thread.take()returnsSome, enqueues_stop,thread.join()returns. CF thread is gone; last_scheduleclearedhas_scheduled_watchers = false.shutdown()line 978:self.loop_.swap(null, Relaxed)→CFRelease. Line 983:self.signal_source.swap(null, Relaxed)→CFRelease.self.mutexis not held yet.- Worker thread (concurrently):
fs.watch()→ fast pathFSEVENTS_DEFAULT_LOOP.get()→Some(loop_)(OnceLock is never cleared) →FSEventsWatcher::init→register_watcher→ takesself.mutex(uncontended — main thread hasn't reached line 991 yet). - Worker:
state.has_scheduled_watchersisfalse→ sets ittrue→enqueue_task_concurrent(Task::new(self, _schedule)). - Worker:
self.signal_source.load(Relaxed)→ NULL;self.loop_.load(Acquire)→ NULL. - Worker:
CFRunLoopSourceSignal(NULL)→__CFRunLoopSourceLock(NULL)→ deref at offset 0xC → SIGSEGV. (Or if it survives that,CFRunLoopWakeUp(NULL)faults identically.)
The same interleaving applies to a Worker dropping an FSEventsWatcher whose loop_ Cell hasn't yet been nulled — the swaps happen before shutdown()'s trailing state.watchers loop that nulls those Cells. There's also a UAF variant if the Worker loads before the swap and calls CF after CFRelease.
Impact
macOS-only crash-at-exit when a Worker is actively calling fs.watch() (or dropping a watcher) during process.exit(). Extremely narrow window (post-join(), pre-self.mutex) and the process is already terminating. Not a regression — the pre-PR code freed the entire FSEventsLoop allocation via heap::take, so the same race was a use-after-free of the whole struct, strictly worse than a deterministic NULL deref. This PR is a net improvement; the doc comment just overclaims.
How to fix
Simplest: don't swap-to-null / CFRelease in shutdown() at all. The process is exiting; the FSEventsLoop struct is already leaked; ~200 bytes of CF handles are noise. Then a racing Worker's enqueue_task_concurrent sees valid handles: CFRunLoopSourceSignal on a source removed from all run loops just sets its pending bit, and per this PR's own cf_thread_loop comment, "CFRunLoopWakeUp on a stopped-but-alive loop is a documented no-op" — so the doc comment's claim ("now sees a shut-down loop instead") becomes actually true. Alternatively, add an early-return null check in enqueue_task_concurrent after the loads.
e1c02b6 to
b08f0fb
Compare
|
Addressed review: removed the explanatory comment blocks (kept one-line |
There was a problem hiding this comment.
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)
427-473: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winClean up the loop/source on init failure.
thisis leaked before the fallible CF source creation and thread spawn complete. IfCFRunLoopSourceCreatereturns null, the leakedFSEventsLoopis never reclaimed; ifspawnfails, both the leaked loop andsignal_sourceare left behind. Keep the allocation guarded until the singleton is fully published, and releasesignal_sourceon the spawn-error path.As per coding guidelines, “Pair every acquisition with its release at the acquisition site” and re-audit ownership whenever a fallible call or early return is added.
🤖 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 427 - 473, The init path in FSEventsLoop::init leaks ownership before all fallible setup succeeds. Keep the Box-backed FSEventsLoop guarded until CFRunLoopSourceCreate and thread::Builder::spawn both succeed, and on either early return make sure the allocated loop is reclaimed. Also release the created signal_source if spawn fails so the init failure path pairs every acquisition with a corresponding cleanup.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/fs_events.rs`:
- Around line 427-473: The init path in FSEventsLoop::init leaks ownership
before all fallible setup succeeds. Keep the Box-backed FSEventsLoop guarded
until CFRunLoopSourceCreate and thread::Builder::spawn both succeed, and on
either early return make sure the allocated loop is reclaimed. Also release the
created signal_source if spawn fails so the init failure path pairs every
acquisition with a corresponding cleanup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b29e8473-fc43-42f1-95c4-de75586e2f57
📒 Files selected for processing (4)
src/runtime/node/fs_events.rssrc/runtime/node/path_watcher.rstest/internal/dead-code-escape-limits.jsontest/js/node/watch/fs.watch.close-exit.test.ts
b08f0fb to
6cc84b1
Compare
|
Both findings addressed: every fallible init step after the singleton is leaked now reclaims on failure — |
There was a problem hiding this comment.
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)
819-858: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winHold
self.mutexwhile releasing the CF handles inshutdown()
enqueue_task_concurrent()readsloop_/signal_sourcewithout locking, whileregister_watcher()/unregister_watcher()can still call it underself.mutex. Move the mutex acquisition above theswap()/CFRelease()pair so watcher churn can’t race a freed run loop.Proposed fix
let cf = CoreFoundation::get(); + let _guard = self.mutex.lock_guard(); let loop_ = self.loop_.swap(ptr::null_mut(), Ordering::Relaxed); debug_assert!(!loop_.is_null()); // SAFETY: retained in `cf_thread_loop`; sole owner after join. unsafe { (cf.release)(loop_) }; let signal_source = self.signal_source.swap(ptr::null_mut(), Ordering::Relaxed); debug_assert!(!signal_source.is_null()); // SAFETY: signal_source is a valid CF object until released here unsafe { (cf.release)(signal_source) }; - - let _guard = self.mutex.lock_guard();🤖 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 819 - 858, `FSEventsLoop::shutdown` releases `loop_` and `signal_source` without holding `self.mutex`, which can race with `enqueue_task_concurrent()` being called from `register_watcher()` / `unregister_watcher()`. Move the mutex guard to cover the `swap()` and `cf.release` sequence, then keep the existing watcher cleanup under the same lock so no concurrent task submission can observe freed CF handles.
🤖 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/fs_events.rs`:
- Around line 819-858: `FSEventsLoop::shutdown` releases `loop_` and
`signal_source` without holding `self.mutex`, which can race with
`enqueue_task_concurrent()` being called from `register_watcher()` /
`unregister_watcher()`. Move the mutex guard to cover the `swap()` and
`cf.release` sequence, then keep the existing watcher cleanup under the same
lock so no concurrent task submission can observe freed CF handles.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2f87525e-3cfa-410f-aab2-fa2fd973d468
📒 Files selected for processing (4)
src/runtime/node/fs_events.rssrc/runtime/node/path_watcher.rstest/internal/dead-code-escape-limits.jsontest/js/node/watch/fs.watch.close-exit.test.ts
6cc84b1 to
c9c54da
Compare
|
Good catch — the reclaim arms were reconstructing a |
There was a problem hiding this comment.
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)
764-863: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winGuard post-shutdown reuse of the FSEvents singleton
shutdown()tears down the CF handles but leavesFSEVENTS_DEFAULT_LOOPpublished. Any later exit callback can still callwatch()and get a dead loop, which will enqueue work against nullloop_/signal_source. Mark the loop shut down and reject reuse inwatch().🤖 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 764 - 863, `FSEventsLoop::shutdown()` tears down the CoreFoundation resources but leaves the singleton reachable, so later callbacks can still route into a dead loop. Update the `FSEventsLoop::watch()` path to detect a shut-down loop and refuse to schedule new watchers, and have `shutdown()` mark the loop as permanently shut down before releasing `loop_` and `signal_source`. Use the existing `FSEventsLoop`, `shutdown()`, and `watch()` symbols to wire the guard so post-shutdown reuse is rejected cleanly.
🤖 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/fs_events.rs`:
- Around line 764-863: `FSEventsLoop::shutdown()` tears down the CoreFoundation
resources but leaves the singleton reachable, so later callbacks can still route
into a dead loop. Update the `FSEventsLoop::watch()` path to detect a shut-down
loop and refuse to schedule new watchers, and have `shutdown()` mark the loop as
permanently shut down before releasing `loop_` and `signal_source`. Use the
existing `FSEventsLoop`, `shutdown()`, and `watch()` symbols to wire the guard
so post-shutdown reuse is rejected cleanly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a0cab05d-0cef-462a-960a-125048b267d2
📒 Files selected for processing (4)
src/runtime/node/fs_events.rssrc/runtime/node/path_watcher.rstest/internal/dead-code-escape-limits.jsontest/js/node/watch/fs.watch.close-exit.test.ts
c9c54da to
2d48f94
Compare
…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.
2d48f94 to
dd28d32
Compare
|
Also removed the file's remaining dead-code escapes rather than inventorying them: they only existed because |
| // ──────────────────────────────────────────────────────────────────────────────── | ||
| // Windows stub | ||
| // ──────────────────────────────────────────────────────────────────────────────── |
There was a problem hiding this comment.
🟡 The WindowsStub struct + impl were deleted in the final commit, but the three-line "// Windows stub" section-divider banner that introduced them is left behind as the last three lines of the file with nothing beneath it. Trivial cleanup: delete lines 1560-1562. (Related but lower-priority: now that the whole module is #[cfg(not(windows))] at src/runtime/node.rs:96, the internal #[cfg(not(windows))] gates on ChangeEvent's fields/impl and PathWatcher::emit are also redundant.)
Extended reasoning...
What the bug is
The final commit on this PR (per the 2026-07-04T22:04 timeline entry: "the WindowsStub backend + both #[allow(dead_code)]s are deleted — the file is out of the dead-code inventory entirely") removed the WindowsStub struct and its impl block from the end of path_watcher.rs, and gated the whole module #[cfg(not(windows))] at its declaration in src/runtime/node.rs:96. However, the three-line section-divider comment that introduced WindowsStub was left in place. The file now ends with:
// ────────────────────────────────────────────────────────────────────────────────
// Windows stub
// ────────────────────────────────────────────────────────────────────────────────and nothing else — a section header labeling an empty section.
Why it's in scope for this PR
Root CLAUDE.md §"Code style & idioms reviewers enforce" says "Delete dead code in the same PR that makes it dead", and src/CLAUDE.md says "NEVER add comments to deleted code blocks." The section header became dead in this PR's own final commit — pre-PR it labeled a real #[cfg(windows)] struct WindowsStub + impl that made the file compile on Windows (because win_watcher.rs borrowed EventType from here). Now that EventType moved to node_fs_watcher.rs as WatchEventKind and the module is cfg-gated at the parent, the stub is gone and its header is a dangling comment. This is the same class of cleanup as the earlier resolved review threads on this PR (the fs_events.rs deinit-note reversion at line 183, the path_watcher.rs TrivialNew doc at line 287, and the "see deinit note below" cross-reference at line 155).
Step-by-step proof
- Pre-PR
path_watcher.rsended with the "Windows stub" section header at lines 1593-1595 followed by#[cfg(windows)] struct WindowsStub {}and itsimplblock (16 lines). - This PR's diff hunk at
@@ -1593,19 +1560,3 @@shows the struct + impl as removed lines (-prefix) while the three section-header lines remain as unchanged context. - Post-PR the file ends at line 1562 — verified against the preloaded file content: the last three lines are the divider/label/divider, with no code beneath them.
src/runtime/node.rs:96now reads#[cfg(not(windows))]onpub mod path_watcher;, so no Windows code can exist in this file — the section the header names is not just empty but structurally impossible.- The one refutation on this finding was purely procedural ("duplicate of bug_005, consolidate there"); the synthesis agent has already merged bug_002 and bug_005 into this single report, so that objection is moot.
Secondary observation (lower priority)
With the module now gated #[cfg(not(windows))] at the parent, the many internal #[cfg(not(windows))] guards throughout the file are also redundant: ChangeEvent's three fields (lines 222-227) and its impl block (line 230), PathWatcher::emit (line 259), PathWatcher::flush, and the use bun_wyhash::hash / use bun_core::ZBox imports. These were only needed while the file had to compile on Windows for the sake of EventType; they now gate against a target the file is never built for. This is the same "delete dead code in the PR that makes it dead" principle, but it's a broader mechanical sweep than the three-line header — reasonable to defer if the author prefers.
Impact & fix
Comment-only; zero runtime effect. Delete lines 1560-1562. Filed as nit — nothing breaks if this merges as-is; it's the same class of leftover the author has already cleaned up three times on this PR.
2 participants
Fixes a real, CI-observed crash:
panic(main thread): Segmentation fault at address 0xCinfs.watchon macOS, hit on thedarwin-14 x64test lane runningtest/js/node/async_hooks/async-context/async-context-fs-watch.js(watcher closed from inside its own callback, thenprocess.exit). The fault address isCFRuntimeBase's info word at offset 12 — a freed/nullCFRunLoopRef.Two root causes, both in
src/runtime/node/fs_events.rs:&mutacross threads (Rust-port regression).cf_thread_loop(&mut self)held anoalias&mut FSEventsLoopfor the CF thread's whole life while the JS thread formed its own&mutinregister_watcher/unregister_watcher/Drop.FSEventsLoopis now properlySync:loop_/signal_sourceareAtomicPtr, watcher/stream state lives in anUnsafeCellguarded by the existing mutex, every method takes&self,init()publishes a leaked&'staticthrough aOnceLock, and an explicitshutdown(&'static self)(called fromclose_and_wait()at exit) replacesDrop.CFRunLoopGetCurrent()follows CF's Get rule; when the CF thread exits, pthread TSD frees its run loop, so the JS thread's trailingCFRunLoopWakeUp(loop)after signaling the_stopsource could touch a freed object. The CF thread nowCFRetains its run loop andshutdown()CFReleases it afterthread.join().Also included: the Linux/FreeBSD
PathWatcherManagerinit now does its fallible syscall first and then leaks the singleton (&'staticeverywhere, no unsafe in the reader-thread handoff, and the EMFILE retry path no longer leaks), and a regression stress test (test/js/node/watch/fs.watch.close-exit.test.ts: watch → close inside the callback →process.exit, across 40 subprocesses), which reproduces the crash on macOS x64 CI without the fix.This change was originally authored on
farm/50928778/fs-watch-provenance(May 22) and never merged; this is that commit ported to currentmain(twocfg_attr(allow(dead_code))annotations for targets whose caller is cfg'd out, a hardened test assertion that does not require an empty stderr, and tightened SAFETY comments).Verification: full debug build;
test/js/node/watch/57/57 including the new stress test;AsyncLocalStorage-tracking.test.ts×3;bun run rust:check-allgreen on all targets (the changed code is macOS-gated, so cross-target checks matter here).