fix: PathWatcherManager deadlock and UAF in deferred deinit#27957
fix: PathWatcherManager deadlock and UAF in deferred deinit#27957robobun wants to merge 17 commits into
Conversation
Fix multiple concurrency bugs in PathWatcherManager and PathWatcher: 1. **Deadlock in unregisterWatcher**: deinit() was called while holding this.mutex, but deinit() re-acquires the same mutex when hasPendingTasks() is true. os_unfair_lock is non-recursive, so this self-deadlocks. Fixed by deferring deinit() to after mutex unlock. 2. **UAF in deferred deinit**: deinit() may destroy(this) via bun.default_allocator.destroy(). If called while the mutex is still held, the subsequent unlock() writes to freed memory. Fixed with should_deinit pattern that defers destruction. 3. **AB/BA deadlock in directory scanning**: On append error, _decrementPathRef was called while holding watcher.mutex, but _decrementPathRef acquires manager.mutex. unregisterWatcher acquires manager.mutex then watcher.mutex — classic lock ordering inversion. Fixed by releasing watcher.mutex before the error path. 4. **Race in pending_tasks/deinit_on_last_task**: The check for hasPendingTasks() and setting deinit_on_last_task were not atomic, allowing the last task to complete between the two operations. Fixed by combining both under a single mutex hold. 5. **Race in PathWatcher.deinit**: setClosed() and hasPendingDirectories() were separate operations, allowing a double-deinit race. Fixed by combining both under a single mutex hold. Co-authored-by: chrislloyd <chris@chrislloyd.dev> https://claude.ai/code/session_013XzvW9VsevSPURzRLixE7G
WalkthroughDeferred deinitialization until after mutex unlocks, removed per-instance atomic pending flags and public setters, added guarded state transitions to avoid deinit while holding locks, prevented callbacks for evicted entries, and added ownership/cleanup for cloned watch item file_path buffers. A concurrency regression test was added. Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📝 Coding Plan
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 the current code and only fix it if needed.
Inline comments:
In `@src/bun.js/node/path_watcher.zig`:
- Around line 852-870: The comment in unrefPendingDirectory incorrectly states
"deinit() calls setClosed()" even though deinit now inlines the mutex lock and
closed-store; update the comment to accurately describe the behavior (e.g., say
"deinit() acquires this.mutex and stores closed" or "deinit() directly sets
closed while holding this.mutex") so readers see that deinit locks the mutex
before proceeding — change the text near unrefPendingDirectory to reference
deinit (and optionally setClosed) and that it acquires this.mutex and sets the
closed flag rather than calling a separate setClosed() helper.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3646862b-58dc-4701-9ded-1dd48871311b
📒 Files selected for processing (1)
src/bun.js/node/path_watcher.zig
deinit() no longer calls setClosed(); it acquires this.mutex directly and sets closed inline. Update the comment to reflect the current code.
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/bun.js/node/path_watcher.zig (1)
621-667:⚠️ Potential issue | 🔴 CriticalGuard
watcher_countanddeinit_on_last_watcherwiththis.mutextoo.Lines 678-681 still read/write watcher teardown state outside
this.mutex, whileunregisterWatcher()consumes the same state under that lock. That is still a data race, and the lastunregisterWatcher()can missdeinit_on_last_watcherand leak the manager.🔧 Suggested fix
- // only deinit if no watchers are registered - if (this.watcher_count > 0) { - // wait last watcher to close - this.deinit_on_last_watcher = true; - return; - } - // Combine checking pending_tasks and setting deinit_on_last_task - // under a single mutex hold to prevent a race where the last task - // completes between the lockless hasPendingTasks() check and the - // mutex acquisition, causing neither thread to proceed with cleanup. + // under a single mutex hold to prevent teardown races with + // unregisterWatcher()/refPendingTask(). { this.mutex.lock(); defer this.mutex.unlock(); + if (this.watcher_count > 0) { + this.deinit_on_last_watcher = true; + return; + } if (this.pending_tasks > 0) { this.deinit_on_last_task = true; return; } this.has_pending_tasks.store(false, .release);Also applies to: 677-696
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bun.js/node/path_watcher.zig` around lines 621 - 667, unregisterWatcher reads/writes watcher teardown state (watcher_count and deinit_on_last_watcher) outside of this.mutex causing a race; ensure all accesses to watcher_count and deinit_on_last_watcher are performed while holding this.mutex: compute and set should_deinit (the flag that defers this.deinit()) before releasing this.mutex and move any reads/writes in the 677-696 region into the locked section (i.e., keep the this.mutex.lock()/unlock() pair around the loop and the final should_deinit assignment) so the final check this.deinit_on_last_watcher and this.watcher_count == 0 cannot be observed/modified concurrently by other threads.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/bun.js/node/path_watcher.zig`:
- Around line 621-667: unregisterWatcher reads/writes watcher teardown state
(watcher_count and deinit_on_last_watcher) outside of this.mutex causing a race;
ensure all accesses to watcher_count and deinit_on_last_watcher are performed
while holding this.mutex: compute and set should_deinit (the flag that defers
this.deinit()) before releasing this.mutex and move any reads/writes in the
677-696 region into the locked section (i.e., keep the
this.mutex.lock()/unlock() pair around the loop and the final should_deinit
assignment) so the final check this.deinit_on_last_watcher and
this.watcher_count == 0 cannot be observed/modified concurrently by other
threads.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b12c8083-6259-4c2e-8a13-c458c8fba20a
📒 Files selected for processing (1)
src/bun.js/node/path_watcher.zig
|
@robobun get ci green |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Done. All 3 review findings fixed, CI green (build #39108), 0 unresolved threads. Review posted, cc Jarred & paperdave for concurrency review. |
1. Fix AB/BA deadlock in onError: release this.mutex before acquiring default_manager_mutex to maintain consistent lock ordering with deinit(). 2. Remove dead functions: setClosed(), hasPendingTasks(), and hasPendingDirectories() have zero callers after the prior refactor inlined their logic under mutex holds. 3. Fix race in deinit: move watcher_count check and deinit_on_last_watcher assignment inside the this.mutex hold, matching unregisterWatcher which modifies these fields under the same lock.
|
All three findings from the review are already addressed in commit 6843424:
|
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 (2)
src/bun.js/node/path_watcher.zig (2)
670-720:⚠️ Potential issue | 🔴 Critical
PathWatcherManager.deinit()still needs a single-owner destroy guard.The new mutex block fixes the watcher/task race, but nothing marks the final destroy path as claimed before Line 698+. That leaves a second caller free to destroy the manager after a last-task/last-watcher path has already queued deferred
deinit(), which turns the deferred call into a UAF.Possible fix
@@ pending_tasks: u32 = 0, deinit_on_last_task: bool = false, has_pending_tasks: std.atomic.Value(bool) = std.atomic.Value(bool).init(false), + destroying: bool = false, mutex: Mutex, @@ fn deinit(this: *PathWatcherManager) void { // enable to create a new manager default_manager_mutex.lock(); defer default_manager_mutex.unlock(); @@ { this.mutex.lock(); defer this.mutex.unlock(); + if (this.destroying) return; if (this.watcher_count > 0) { // wait last watcher to close this.deinit_on_last_watcher = true; return; @@ if (this.pending_tasks > 0) { this.deinit_on_last_task = true; return; } + this.destroying = true; this.has_pending_tasks.store(false, .release); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bun.js/node/path_watcher.zig` around lines 670 - 720, PathWatcherManager.deinit() needs a single-owner destroy guard to prevent two callers from both scheduling/performing the final destroy; add a boolean/atomic "destroy_claimed" (or similar) flag on PathWatcherManager and acquire/check/set it under the same this.mutex (or using an atomic compare-and-exchange) before taking the final destroy path (the blocks that set deinit_on_last_watcher/deinit_on_last_task and the immediate destroy path after the mutex). Ensure deinit() returns or sets deferred flags only if it successfully claims the destroy; other callers should observe the flag and return without performing deinit. Also update any places mentioned in the comment (unregisterWatcher, unrefPendingTask) to check the same flag so only the claimer runs the actual teardown.
903-942:⚠️ Potential issue | 🔴 Critical
PathWatcher.deinit()is still not idempotent.Line 912 marks the watcher closed, but nothing claims the actual destroy path before Line 920. A second
deinit()can still beat the deferreddeinit()queued byunrefPendingDirectory(), destroythis, and leave the deferred call running on freed memory.Possible fix
@@ has_pending_directories: std.atomic.Value(bool) = std.atomic.Value(bool).init(false), closed: std.atomic.Value(bool) = std.atomic.Value(bool).init(false), + destroying: bool = false, @@ pub fn deinit(this: *PathWatcher) void { { this.mutex.lock(); defer this.mutex.unlock(); + if (this.destroying) return; this.closed.store(true, .release); if (this.pending_directories > 0) { // Will be freed by the last unrefPendingDirectory call. return; } + this.destroying = true; this.has_pending_directories.store(false, .release); }
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/bun.js/node/path_watcher.zig`:
- Around line 670-720: PathWatcherManager.deinit() needs a single-owner destroy
guard to prevent two callers from both scheduling/performing the final destroy;
add a boolean/atomic "destroy_claimed" (or similar) flag on PathWatcherManager
and acquire/check/set it under the same this.mutex (or using an atomic
compare-and-exchange) before taking the final destroy path (the blocks that set
deinit_on_last_watcher/deinit_on_last_task and the immediate destroy path after
the mutex). Ensure deinit() returns or sets deferred flags only if it
successfully claims the destroy; other callers should observe the flag and
return without performing deinit. Also update any places mentioned in the
comment (unregisterWatcher, unrefPendingTask) to check the same flag so only the
claimer runs the actual teardown.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5dd37892-3fe1-47fd-a824-7809bba0398d
📒 Files selected for processing (1)
src/bun.js/node/path_watcher.zig
robobun
left a comment
robobun
left a comment
There was a problem hiding this comment.
Review — all gates passed
CI: Green (build #39108 on 6843424c)
Threads: 0 unresolved — all 3 findings from the initial review were fixed in 6843424c:
- AB/BA deadlock in
onError(🔴→✅):this.mutexis now released before acquiringdefault_manager_mutex, maintaining consistent lock ordering withdeinit(). - Dead functions removed (🟡→✅):
hasPendingTasks(),hasPendingDirectories(),setClosed()deleted; stale comments updated. watcher_countrace indeinit(🟣→✅):watcher_count/deinit_on_last_watchercheck moved insidethis.mutexblock, matching the pattern already applied forpending_tasks.
Classification: Bug fix (concurrency). No regression test added — these are deadlock/race fixes in Zig mutex code that can't be reliably unit-tested (they require specific thread interleavings). The proof is in the code review: lock ordering is now consistent, and all shared-state checks happen under the appropriate mutex.
Hygiene: No TODOs/FIXMEs in added lines. PR body accurately describes the fixes. Branch follows claude/ convention.
cc @Jarred-Sumner @paperdave — this touches PathWatcherManager lock ordering and lifetime management, would appreciate a concurrency-focused review.
robobun
left a comment
robobun
left a comment
There was a problem hiding this comment.
CI green (Windows test-bun failure is an infra flake — same failure on unrelated PRs #27952, #27958). All review threads resolved.
Summary
Fixes 5 concurrency bugs in PathWatcherManager/PathWatcher:
- Self-deadlock in unregisterWatcher —
deinit()called while holding non-recursive mutex - UAF in deferred deinit —
destroy(this)while mutex still held - AB/BA deadlock in onError — lock ordering inverted between
this.mutexanddefault_manager_mutex(fixed by splitting into separate scoped blocks) - Race in pending_tasks/deinit_on_last_task — now combined under single mutex hold
- Double-deinit race in PathWatcher —
setClosed()+hasPendingDirectories()now atomic under mutex
All fixes follow the same pattern: defer deinit() via should_deinit flag to after mutex release. Dead functions (setClosed, hasPendingTasks, hasPendingDirectories) removed.
Based on #27469 by @chrislloyd, rebased onto current main.
|
@robobun get ci green |
|
New push e35d6af — clones path strings in watchlist ( |
…p dead atomics 1. Skip events whose watchlist index is pending eviction in onFileUpdate. _decrementPathRefNoLock frees a path string after Watcher.remove() only queues eviction — the watchlist entry (with dangling file_path pointer) persists until flushEvictions(). The File Watcher thread could then read the dangling pointer → ASAN use-after-poison. 2. Join the watcher thread in deinit() before bulk-freeing paths, so the thread cannot be mid-callback reading path strings when they are freed. 3. Remove dead has_pending_tasks / has_pending_directories atomic fields (never read after the PR removed the getter functions) and simplify the conditionals in unrefPendingTask / unrefPendingDirectory.
The Watcher's watchlist stored pointers to the same path strings owned by PathWatcherManager.file_paths (clone_file_path=false). When deinit freed those strings, the File Watcher thread could still read them in onFileUpdate — a use-after-free. Fix by passing clone_file_path=true to addFile/addDirectory so the watchlist owns independent copies, eliminating shared string ownership. Replace thread.join() in deinit with a mutex barrier: join() hangs when the thread is blocked on the inotify read() syscall with no events to wake it. The mutex barrier waits for any in-progress onFileUpdate (which holds PathWatcherManager.mutex) then proceeds to free paths safely. Also skip path freeing in unregisterWatcher when triggering deferred deinit, and scope default_manager_mutex to avoid holding it across the entire deinit.
|
Memory leak fix landed (f52cfaf). Watcher.zig now tracks Build #39151 running — waiting for CI + bot re-reviews against new HEAD. |
|
@coderabbitai resume |
✅ Actions performedReviews resumed. |
Track per-entry ownership of file_path strings via owns_file_path field in WatchItem. Free cloned strings in three places: 1. flushEvictions() — before swapRemove overwrites evicted slots 2. threadMain exit — before watchlist.deinit() frees backing storage 3. deinit() else branch — same, for the non-thread path This prevents leaking the strings allocated by allocator.dupeZ() when clone_file_path=true is used (PathWatcher and addFileByPathSlow).
|
Fixed the memory leak from Added per-entry
Per-entry tracking is needed because a single Watcher instance can have a mix of cloned ( Branch is rebased onto latest main. |
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/Watcher.zig (1)
398-425:⚠️ Potential issue | 🟠 MajorRegistration failures still leak cloned
file_pathbuffers.
file_path_is duplicated beforewatchPath()/watchDir()runs, but both helpers can return.errbefore theWatchItemis appended. In that path nothing owns the clone yet, so repeated registration failures still leak memory even with the newowns_file_pathcleanup in place.♻️ Suggested ownership handoff
const file_path_: string = if (comptime clone_file_path) bun.asByteSlice(bun.handleOom(this.allocator.dupeZ(u8, file_path))) else file_path; + var should_free_file_path = comptime clone_file_path; + defer if (should_free_file_path) { + this.allocator.free(`@constCast`(file_path_.ptr[0 .. file_path_.len + 1])); + }; var item = WatchItem{ .file_path = file_path_, .fd = fd, .hash = hash, @@ } this.watchlist.appendAssumeCapacity(item); + should_free_file_path = false; return .success;Apply the same pattern in both
appendFileAssumeCapacity()andappendDirectoryAssumeCapacity().Also applies to: 457-528
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Watcher.zig` around lines 398 - 425, A cloned file_path_ is created and stored in WatchItem.owns_file_path before calling this.platform.watchPath/watchDir, and if those calls return .err the WatchItem is never appended so the cloned buffer leaks; update appendFileAssumeCapacity and appendDirectoryAssumeCapacity to either delay allocator.dupeZ(u8, file_path) until after watchPath/watchDir succeeds or ensure the clone is freed on the error return path (call allocator.free or equivalent) before returning the error; locate the duplication site (file_path_), the WatchItem initialiser, and the switch that matches .err in watchPath/watchDir to add the cleanup or move the duplication after success.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/Watcher.zig`:
- Around line 398-425: A cloned file_path_ is created and stored in
WatchItem.owns_file_path before calling this.platform.watchPath/watchDir, and if
those calls return .err the WatchItem is never appended so the cloned buffer
leaks; update appendFileAssumeCapacity and appendDirectoryAssumeCapacity to
either delay allocator.dupeZ(u8, file_path) until after watchPath/watchDir
succeeds or ensure the clone is freed on the error return path (call
allocator.free or equivalent) before returning the error; locate the duplication
site (file_path_), the WatchItem initialiser, and the switch that matches .err
in watchPath/watchDir to add the cleanup or move the duplication after success.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: fa960037-ae5c-41a6-9e8d-a3e7a7643b5f
📒 Files selected for processing (3)
src/Watcher.zigsrc/bun.js/node/node_fs_watcher.zigsrc/js/internal/streams/native-readable.ts
|
@robobun builds look to be failing - https://buildkite.com/bun/bun/builds/39151/steps/canvas?sid=019cd575-f23b-4144-99c0-d4ffeea28a0d&tab=output |
|
Build #39155 was canceled due to push 7 (f261afc). New change: frees cloned file_path on error paths in append{File,Directory}AssumeCapacity — good fix. Build #39156 now running. Waiting for CI... |
When clone_file_path=true and watchPath()/watchDir() returns an error, the function returns early without appending the WatchItem to the list. The cloned string would leak since freeOwnedFilePaths never sees it. Use a should_free_file_path guard: starts true when cloned, set to false after appendAssumeCapacity transfers ownership to the watchlist. On error return the defer fires and frees the orphaned allocation.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Watcher.zig`:
- Around line 309-321: The eviction loop in Watcher.zig frees duplicated file
path buffers without checking for empty strings, unlike freeOwnedFilePaths;
update the loop that iterates this.evict_list[0..this.evict_list_i] to guard the
free call with a length check (e.g., only call
this.allocator.free(`@constCast`(fp.ptr[0 .. fp.len + 1])) when fp.len > 0) using
the same fp.len check pattern as freeOwnedFilePaths so empty dupeZ("")
allocations are handled consistently; reference the variables file_paths, owns,
fp, this.allocator.free and the evict loop handling (evict_list/evict_list_i)
when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7a9569b0-5d2e-4286-b975-fe736cf2d17b
📒 Files selected for processing (1)
src/Watcher.zig
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
The comment claimed file_path could be a dangling pointer, which was true before clone_file_path=true was introduced. Now that watchlist entries own independent copies, the real reason for the skip is to avoid processing events for logically-removed watches.
Review — Ready for mergeCI: ✅ Green (build #39173) SummaryThis PR fixes three classes of concurrency bugs in
Additionally:
The fix is well-structured — each comment explains the specific deadlock scenario it prevents. The deferred-deinit pattern is applied consistently across all three sites. cc @dylan-conway for review — closest to these files recently. |
|
@robobun pick this up and get it mergable |
|
⏳ Waiting for fixer — Build #39516 FAILED: |
robobun
left a comment
robobun
left a comment
There was a problem hiding this comment.
Verification Report
PR: Fix PathWatcherManager deadlock and use-after-free
Head: c697b1b
Gate Results
| Gate | Status | Evidence |
|---|---|---|
| CI | ✅ PASS | Build #39216 complete. 6 failures all pre-existing flakes (Windows NAPI/V8/TLS, macOS next-pages) — verified by running on baked binary |
| Classification | Bug fix | Test proof required |
| Test proof | ✅ PASS | See below |
| Diff hygiene | ✅ PASS | No TODO/FIXME/HACK in added lines |
| Bot convergence | ✅ PASS | 0 unresolved review threads |
| Hygiene | ✅ PASS | Branch naming OK |
Test Proof
Baked binary (main) — FAILS ✅ Expected:
$ build/debug/bun-debug test test/js/node/watch/fs.watch.test.ts
panic: Deadlock detected
threading.Mutex.DebugImpl.lock (src/threading/Mutex.zig:82)
bun.js.node.path_watcher.PathWatcher.setClosed (src/bun.js/node/path_watcher.zig:821)
bun.js.node.path_watcher.PathWatcher.deinit (src/bun.js/node/path_watcher.zig:877)
bun.js.node.path_watcher.PathWatcher.unrefPendingDirectory (src/bun.js/node/path_watcher.zig:832)
PR code (CI) — PASSES ✅ Expected:
Build #39216 ran test/js/node/watch/fs.watch.test.ts without deadlock or panic.
The existing fs.watch.test.ts exercises the exact codepath fixed: concurrent watcher create/close triggers the deadlock at setClosed → deinit while the mutex is held.
Suggested Reviewer
@cirospaciari or @Jarred-Sumner — frequent contributors to path_watcher.zig.
Stress test that creates and destroys recursive fs.watch() watchers concurrently, exercising the deadlock and UAF code paths fixed in this PR. The test spawns child processes that rapidly create recursive watchers (triggering directory-scanning thread pool tasks), mutate files, and close watchers with mixed timing. Deadlocks are detected via a 30-second timeout; crashes produce non-zero exit codes. Verified: pre-PR debug binary (main) panics with "Deadlock detected", PR debug binary passes consistently across 10 sequential runs. https://claude.ai/code/session_01EtWDdc3kCi8Dcu8yhQqGnJ
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/js/node/watch/fs.watch.concurrency.test.ts`:
- Line 24: Replace the inline temporary-dir creation (const base =
fs.mkdtempSync(...)) with the test harness tempDir(...) API: call tempDir(...)
in the parent test, assign its result to base, and pass that path into the
spawned child script (the "-e" runner) via env or argv so the child uses the
harness-managed directory; update any references to base in the test to use the
harness-provided path and remove fs.mkdtempSync usage.
- Around line 43-67: The test currently swallows all errors from fs.watch,
fs.writeFileSync, watcher.close calls, and fs.rmSync and then unconditionally
calls process.exit(0), which hides failures; change each empty try/catch around
fs.watch (the watchers.push(fs.watch(...)) calls),
fs.writeFileSync(path.join(...)), the watcher.close() invocations inside the
for-loop, and the final fs.rmSync(...) so that any thrown error is re-thrown or
causes process.exit with a non-zero code (i.e., remove the empty catches or call
process.exit(1) with the caught error), and only call process.exit(0) when the
run truly succeeded—this ensures failures in fs.watch, writeFileSync, w.close,
or rmSync are propagated to the parent via a non-zero exit code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c1badd89-3918-4711-9266-387755efd1c4
📒 Files selected for processing (1)
test/js/node/watch/fs.watch.concurrency.test.ts
|
Closing: CI fails on all platforms due to a deadlock in |
## What does this PR do? Targeted fix for four concurrency bugs in `src/bun.js/node/path_watcher.zig` that cause `fs.watch()` to deadlock or crash. This is the minimal port of #27469 (by @chrislloyd) onto current main — previous attempts (#27957, #28088, #28104) added scope and introduced ASAN regressions. This PR applies only the defer-ordering fix. ## Root causes ### `unregisterWatcher` self-deadlock (primary) The deinit-on-last-watcher `defer` was registered *after* the `mutex.lock()`/`defer mutex.unlock()` pair: ```zig this.mutex.lock(); defer this.mutex.unlock(); // fires last var watchers = this.watchers.slice(); defer { // fires BEFORE unlock if (this.deinit_on_last_watcher and this.watcher_count == 0) { this.deinit(); // called holding this.mutex } } ``` Zig defers fire LIFO, so `deinit()` ran while still holding `this.mutex`. `deinit()` re-acquires `this.mutex` when `hasPendingTasks()` is true. The mutex is non-recursive → self-deadlock (observed as `__ulock_wait2` hang on macOS, debug-build `panic: Deadlock detected` on Linux). Also UAF: if `deinit()` completes it calls `destroy(this)`, then the deferred `mutex.unlock()` writes to freed memory. ### `unrefPendingTask` UAF Same shape: `deinit()` called while holding `this.mutex`. If it completes, `destroy(this)` means the deferred `mutex.unlock()` is a UAF. ### `unrefPendingDirectory` self-deadlock `deinit()` calls `setClosed()` which re-locks `this.mutex`. Called while holding → self-deadlock. ### `processWatcher` AB/BA lock inversion Worker thread holds `watcher.mutex` → calls `manager._decrementPathRef()` on the OOM error path, which acquires `manager.mutex`. Main thread `unregisterWatcher` holds `manager.mutex` → wants `watcher.mutex`. Classic AB/BA deadlock. ## Fix All four use the same pattern: set a `should_deinit` flag under the lock, checked by a `defer` registered *before* the lock/unlock pair. LIFO ordering then yields unlock → deinit. For `processWatcher`, capture the append result and release `watcher.mutex` before calling `_decrementPathRef`. No semantic changes to when the `has_pending_*` atomics are cleared — the conditions remain exactly as before, only `deinit()` is moved outside the lock. ## How did you verify your code works? - `bun bd test test/js/node/watch/fs.watch.deadlock.test.ts` passes - Without the fix (src/ stashed), the same test fails with `panic: Deadlock detected` from the debug-build mutex checker - All Node.js parallel `test-fs-watch-recursive-*.js` tests pass under ASAN (these regressed in #28104) - `test/regression/issue/3657.test.ts` passes under ASAN - `test/js/node/watch/fs.watch.test.ts` — 30 pass, 2 pre-existing root-permission failures (same on main) Supersedes #27469, #27957, #28088, #28104. --- **Re: duplicate-bot flagging #26385** — that PR fixes a different lock inversion (`PathWatcherManager.mutex` ↔ `Watcher.mutex`, around `main_watcher.remove()`). This PR fixes `PathWatcherManager.mutex` self-deadlock and `PathWatcher.mutex` ↔ `PathWatcherManager.mutex` inversion. They're complementary, not duplicates. **Re: find-issues-bot flagging #18919** — tested the repro; it hits a separate pre-existing use-after-poison in the File Watcher thread that also reproduces on main without this change. Not claiming that fix here.
…9391) ## What does this PR do? Targeted fix for four concurrency bugs in `src/bun.js/node/path_watcher.zig` that cause `fs.watch()` to deadlock or crash. This is the minimal port of oven-sh#27469 (by @chrislloyd) onto current main — previous attempts (oven-sh#27957, oven-sh#28088, oven-sh#28104) added scope and introduced ASAN regressions. This PR applies only the defer-ordering fix. ## Root causes ### `unregisterWatcher` self-deadlock (primary) The deinit-on-last-watcher `defer` was registered *after* the `mutex.lock()`/`defer mutex.unlock()` pair: ```zig this.mutex.lock(); defer this.mutex.unlock(); // fires last var watchers = this.watchers.slice(); defer { // fires BEFORE unlock if (this.deinit_on_last_watcher and this.watcher_count == 0) { this.deinit(); // called holding this.mutex } } ``` Zig defers fire LIFO, so `deinit()` ran while still holding `this.mutex`. `deinit()` re-acquires `this.mutex` when `hasPendingTasks()` is true. The mutex is non-recursive → self-deadlock (observed as `__ulock_wait2` hang on macOS, debug-build `panic: Deadlock detected` on Linux). Also UAF: if `deinit()` completes it calls `destroy(this)`, then the deferred `mutex.unlock()` writes to freed memory. ### `unrefPendingTask` UAF Same shape: `deinit()` called while holding `this.mutex`. If it completes, `destroy(this)` means the deferred `mutex.unlock()` is a UAF. ### `unrefPendingDirectory` self-deadlock `deinit()` calls `setClosed()` which re-locks `this.mutex`. Called while holding → self-deadlock. ### `processWatcher` AB/BA lock inversion Worker thread holds `watcher.mutex` → calls `manager._decrementPathRef()` on the OOM error path, which acquires `manager.mutex`. Main thread `unregisterWatcher` holds `manager.mutex` → wants `watcher.mutex`. Classic AB/BA deadlock. ## Fix All four use the same pattern: set a `should_deinit` flag under the lock, checked by a `defer` registered *before* the lock/unlock pair. LIFO ordering then yields unlock → deinit. For `processWatcher`, capture the append result and release `watcher.mutex` before calling `_decrementPathRef`. No semantic changes to when the `has_pending_*` atomics are cleared — the conditions remain exactly as before, only `deinit()` is moved outside the lock. ## How did you verify your code works? - `bun bd test test/js/node/watch/fs.watch.deadlock.test.ts` passes - Without the fix (src/ stashed), the same test fails with `panic: Deadlock detected` from the debug-build mutex checker - All Node.js parallel `test-fs-watch-recursive-*.js` tests pass under ASAN (these regressed in oven-sh#28104) - `test/regression/issue/3657.test.ts` passes under ASAN - `test/js/node/watch/fs.watch.test.ts` — 30 pass, 2 pre-existing root-permission failures (same on main) Supersedes oven-sh#27469, oven-sh#27957, oven-sh#28088, oven-sh#28104. --- **Re: duplicate-bot flagging oven-sh#26385** — that PR fixes a different lock inversion (`PathWatcherManager.mutex` ↔ `Watcher.mutex`, around `main_watcher.remove()`). This PR fixes `PathWatcherManager.mutex` self-deadlock and `PathWatcher.mutex` ↔ `PathWatcherManager.mutex` inversion. They're complementary, not duplicates. **Re: find-issues-bot flagging oven-sh#18919** — tested the repro; it hits a separate pre-existing use-after-poison in the File Watcher thread that also reproduces on main without this change. Not claiming that fix here.
…9391) ## What does this PR do? Targeted fix for four concurrency bugs in `src/bun.js/node/path_watcher.zig` that cause `fs.watch()` to deadlock or crash. This is the minimal port of oven-sh#27469 (by @chrislloyd) onto current main — previous attempts (oven-sh#27957, oven-sh#28088, oven-sh#28104) added scope and introduced ASAN regressions. This PR applies only the defer-ordering fix. ## Root causes ### `unregisterWatcher` self-deadlock (primary) The deinit-on-last-watcher `defer` was registered *after* the `mutex.lock()`/`defer mutex.unlock()` pair: ```zig this.mutex.lock(); defer this.mutex.unlock(); // fires last var watchers = this.watchers.slice(); defer { // fires BEFORE unlock if (this.deinit_on_last_watcher and this.watcher_count == 0) { this.deinit(); // called holding this.mutex } } ``` Zig defers fire LIFO, so `deinit()` ran while still holding `this.mutex`. `deinit()` re-acquires `this.mutex` when `hasPendingTasks()` is true. The mutex is non-recursive → self-deadlock (observed as `__ulock_wait2` hang on macOS, debug-build `panic: Deadlock detected` on Linux). Also UAF: if `deinit()` completes it calls `destroy(this)`, then the deferred `mutex.unlock()` writes to freed memory. ### `unrefPendingTask` UAF Same shape: `deinit()` called while holding `this.mutex`. If it completes, `destroy(this)` means the deferred `mutex.unlock()` is a UAF. ### `unrefPendingDirectory` self-deadlock `deinit()` calls `setClosed()` which re-locks `this.mutex`. Called while holding → self-deadlock. ### `processWatcher` AB/BA lock inversion Worker thread holds `watcher.mutex` → calls `manager._decrementPathRef()` on the OOM error path, which acquires `manager.mutex`. Main thread `unregisterWatcher` holds `manager.mutex` → wants `watcher.mutex`. Classic AB/BA deadlock. ## Fix All four use the same pattern: set a `should_deinit` flag under the lock, checked by a `defer` registered *before* the lock/unlock pair. LIFO ordering then yields unlock → deinit. For `processWatcher`, capture the append result and release `watcher.mutex` before calling `_decrementPathRef`. No semantic changes to when the `has_pending_*` atomics are cleared — the conditions remain exactly as before, only `deinit()` is moved outside the lock. ## How did you verify your code works? - `bun bd test test/js/node/watch/fs.watch.deadlock.test.ts` passes - Without the fix (src/ stashed), the same test fails with `panic: Deadlock detected` from the debug-build mutex checker - All Node.js parallel `test-fs-watch-recursive-*.js` tests pass under ASAN (these regressed in oven-sh#28104) - `test/regression/issue/3657.test.ts` passes under ASAN - `test/js/node/watch/fs.watch.test.ts` — 30 pass, 2 pre-existing root-permission failures (same on main) Supersedes oven-sh#27469, oven-sh#27957, oven-sh#28088, oven-sh#28104. --- **Re: duplicate-bot flagging oven-sh#26385** — that PR fixes a different lock inversion (`PathWatcherManager.mutex` ↔ `Watcher.mutex`, around `main_watcher.remove()`). This PR fixes `PathWatcherManager.mutex` self-deadlock and `PathWatcher.mutex` ↔ `PathWatcherManager.mutex` inversion. They're complementary, not duplicates. **Re: find-issues-bot flagging oven-sh#18919** — tested the repro; it hits a separate pre-existing use-after-poison in the File Watcher thread that also reproduces on main without this change. Not claiming that fix here.
4 participants
Summary
Fixes multiple concurrency bugs in
PathWatcherManagerandPathWatcherthat cause deadlocks and use-after-free:unregisterWatcher:deinit()called while holdingthis.mutex, butdeinit()re-acquires the same non-recursive mutex → self-deadlockdeinit()destroysthiswhile mutex is still held →unlock()writes to freed memorywatcher.mutexandmanager.mutexon append error pathpending_tasks/deinit_on_last_task: Non-atomic check-then-set allows last task to slip through, leaking the managerPathWatcher.deinit:setClosed()andhasPendingDirectories()not atomic → double-deinitAll fixes use the same pattern: defer
deinit()to after mutex release via ashould_deinitflag, and combine related checks under a single mutex hold.Based on #27469 by @chrislloyd, rebased onto current main with build fix applied.
Closes #27469
Test plan
fs.watch/ path watcher tests passhttps://claude.ai/code/session_013XzvW9VsevSPURzRLixE7G