Repository navigation
Conversation
`Watcher::shutdown` published `running = false` and returned, but the watcher thread only re-checks that flag *between* waits and was parked indefinitely waiting for filesystem events. For a stopped DevServer none ever arrive, so the thread — and the `Box<Watcher>` it owns and is responsible for freeing — survived for the life of the process. Creating and dropping six dev servers left six live `File Watcher` threads. Each backend gets an explicit wakeup: - Linux: an eventfd, waited on alongside the inotify fd via `ppoll`. `read` returns empty when it fires, so `watch_loop` re-checks `running` and exits. The eventfd is never drained, so there is no lost-wakeup window and `wake` is safe to call repeatedly from any thread. If `eventfd(2)` fails the fd is left invalid and `read` waits on the inotify fd alone — uninterruptible, but still correct, so watcher startup cannot fail on this. `wake` also releases the `watch_count` futex, which is where the thread parks when nothing is watched yet. - kqueue: `EVFILT_USER` with `NOTE_TRIGGER`, registered and fired in one `kevent` call. The ident cannot collide with a real registration; those use `EVFILT_VNODE` and key on file descriptors. - Windows: `PostQueuedCompletionStatus` with a null `OVERLAPPED`, which the existing wait loop already treats as a stop signal. Adds the missing extern declaration to `bun_sys::windows::kernel32`. The load-bearing ordering detail: `wake` is called *inside* `Watcher.mutex` and `thread_main` acquires the same lock before deallocating. Waking after unlocking would read `platform.wake_fd` out of freed memory — `running = false` is already published at that point, so the thread is free to exit and drop the box — and write eight bytes into whatever descriptor number that garbage happened to name. Note this is deliberately not an epoll change. For a single inotify fd, epoll measures *slower* than a blocking read (+6.7% draining 8000 events); the reason to wait rather than read is interruptibility, not throughput, so `ppoll` on two fds is the right primitive. Making the thread actually exit reached two `close()` loops that were previously unreachable. Both closed every watchlist fd unconditionally, but platforms that do not need a descriptor per watch store `Fd::INVALID`, which `bun_sys::close` asserts against — this surfaced as `panic: assertion failed: fd != Fd::INVALID` in `thread_main`. Both now skip invalid descriptors, matching what `flush_evictions` already did. The deinitialization fixture asserts no `File Watcher` thread outlives its dev server. Verified by stashing this change, rebuilding, and re-running: the test fails (six threads leaked) without it and passes with it. Linux: test/bake/deinitialization, test/cli/watch, test/cli/hot — 23 pass, 0 fail.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
WalkthroughChangesThe watcher shutdown path now wakes blocking platform waits on Linux, BSD/macOS, and Windows, coordinates cleanup under a mutex, closes only valid descriptors, and verifies Linux watcher threads terminate during deinitialization. Watcher shutdown
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/watcher/WindowsWatcher.rs (1)
382-402: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winReset
self.iocpandself.watcher.dir_handleinstop().
wake()depends on these fields beingINVALID_HANDLE_VALUEonce the handles are closed, butstop()only closes them. Sinceshutdown()can callplatform.wake()afterthread_mainenters theErrbranch and callsplatform.stop(), this needs the same ownership cleanup pattern asINotifyWatcher::stop()andKEventWatcher::stop(): reset both fields tow::INVALID_HANDLE_VALUEafter closing so later wake paths do not use stale closes/handles.🤖 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/watcher/WindowsWatcher.rs` around lines 382 - 402, Update WindowsWatcher::stop to set both self.watcher.dir_handle and self.iocp to w::INVALID_HANDLE_VALUE immediately after closing them, matching the cleanup pattern used by the other watcher implementations so later wake calls cannot use stale handles.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.
Inline comments:
In `@src/watcher/Watcher.rs`:
- Around line 283-292: Update thread_main so me.platform.stop() executes once
for every non-error lifecycle exit, including graceful wakeups on Linux, macOS,
and Windows. Move the stop call out of the Err-specific match branch and place
it before descriptor cleanup or deallocation, while preserving the existing
error handling and synchronization behavior.
---
Outside diff comments:
In `@src/watcher/WindowsWatcher.rs`:
- Around line 382-402: Update WindowsWatcher::stop to set both
self.watcher.dir_handle and self.iocp to w::INVALID_HANDLE_VALUE immediately
after closing them, matching the cleanup pattern used by the other watcher
implementations so later wake calls cannot use stale handles.
🪄 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 Plus
Run ID: c2bf0670-e299-4027-a0f6-0587e3f723fb
📒 Files selected for processing (6)
src/sys/windows/mod.rssrc/watcher/INotifyWatcher.rssrc/watcher/KEventWatcher.rssrc/watcher/Watcher.rssrc/watcher/WindowsWatcher.rstest/bake/fixtures/deinitialization/test.ts
| // Must be inside the lock. `running = false` is published above, so | ||
| // the watcher thread may now exit and free the `Box<Watcher>`; | ||
| // `thread_main` takes this same lock before freeing, so holding it | ||
| // here is what keeps `me` alive across the wake. | ||
| me.platform.wake(); | ||
| me.mutex.unlock(); | ||
| } else { | ||
| if close_descriptors && me.running.load() { | ||
| let fds = me.watchlist.items_fd(); | ||
| for &fd in fds { | ||
| for &fd in fds.iter().filter(|fd| fd.is_valid()) { |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n "fn watch_loop_cycle" -A 40 src/watcher/
rg -n "impl Drop for (INotifyWatcher|KEventWatcher|WindowsWatcher|Watcher)" -A 15 src/watcher/
rg -n "platform\.stop\(\)|platform::stop" -n src/watcher/Repository: oven-sh/bun
Length of output: 9570
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Watcher.rs relevant sections =="
sed -n '240,370p' src/watcher/Watcher.rs | cat -n | sed 's/^/Watcher.rs:/'
echo
echo "== platform trait definitions/usages =="
rg -n "trait .*Platform|struct .*Watcher|enum .*Platform|fn stop|fn wake|fn next|fn read" src/watcher -g '*.rs' | sed -n '1,220p'
echo
echo "== watch_loop cycle dispatch =="
rg -n "watch_loop_cycle|watchloop_handle|is_empty|read\\?|next\\?|Inotify|KEvent|Windows" src/watcher/Watcher.rs src/watcher/INotifyWatcher.rs src/watcher/KEventWatcher.rs src/watcher/WindowsWatcher.rs src/watcher/platform.rs 2>/dev/null || trueRepository: oven-sh/bun
Length of output: 12308
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== KEventWatcher stop/wake =="
sed -n '1,100p' src/watcher/KEventWatcher.rs | cat -n
echo
echo "== INotifyWatcher relevant stop/wake/read/watch_loop =="
sed -n '200,280p' src/watcher/INotifyWatcher.rs | cat -n
sed -n '280,305p' src/watcher/INotifyWatcher.rs | cat -n
sed -n '440,515p' src/watcher/INotifyWatcher.rs | cat -n
echo
echo "== WindowsWatcher stop/wake next relevant =="
sed -n '300,420p' src/watcher/WindowsWatcher.rs | cat -nRepository: oven-sh/bun
Length of output: 18496
Close platform resources on the graceful shutdown path.
thread_main calls me.platform.stop() only in the Err arm, but the graceful shutdown wake returns Ok(()) on Linux and macOS, so the inotify/eventfd or kqueue fd are never closed by stop(). They only close later via close_descriptors, and the Windows null-OVERLAPPED path reaches stop() only by accident.
Move me.platform.stop() out of the match so it runs once for every non-error lifecycle exit, before descriptor cleanup/deallocation.
🤖 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/watcher/Watcher.rs` around lines 283 - 292, Update thread_main so
me.platform.stop() executes once for every non-error lifecycle exit, including
graceful wakeups on Linux, macOS, and Windows. Move the stop call out of the
Err-specific match branch and place it before descriptor cleanup or
deallocation, while preserving the existing error handling and synchronization
behavior.
Source: Coding guidelines
|
Closing as a duplicate of #36253, which predates this by a day and is a strict superset. I opened this without checking for existing open PRs on the same subsystem — my mistake. For anyone comparing the two, #36253 is the better change and this adds nothing on top of it:
The The one thing here that is not in #36253 is the thread-count assertion in |
1 participant
What does this PR do?
Watcher::shutdownpublishedrunning = falseand returned, but the watcher thread only re-checks that flag between waits and was parked indefinitely waiting for filesystem events. For a stoppedDevServernone ever arrive, so the thread — and theBox<Watcher>it owns and is responsible for freeing — survived for the life of the process.Creating and dropping six dev servers left six live
File Watcherthreads.Each backend gets an explicit wakeup:
ppoll.readreturns empty when it fires, sowatch_loopre-checksrunningand exits. The eventfd is never drained, so there is no lost-wakeup window andwakeis safe to call repeatedly from any thread. Ifeventfd(2)fails the fd is left invalid andreadwaits on the inotify fd alone — uninterruptible but still correct, so watcher startup cannot fail on this.wakealso releases thewatch_countfutex, which is where the thread parks when nothing is watched yet.EVFILT_USERwithNOTE_TRIGGER, registered and fired in onekeventcall. The ident cannot collide with a real registration; those useEVFILT_VNODEand key on file descriptors.PostQueuedCompletionStatuswith a nullOVERLAPPED, which the existing wait loop already treats as a stop signal. Adds the missing extern declaration tobun_sys::windows::kernel32.The load-bearing ordering detail:
wakeis called insideWatcher.mutex, andthread_mainacquires the same lock before deallocating. Waking after unlocking would readplatform.wake_fdout of freed memory —running = falseis already published at that point, so the thread is free to exit and drop the box — and write eight bytes into whatever descriptor number that garbage happened to name. I hit exactly that while developing this.Note this is deliberately not an epoll change. For a single inotify fd, epoll measures slower than a blocking read (+6.7% draining 8000 events); the reason to wait rather than read is interruptibility, not throughput, so
ppollon two fds is the right primitive.Why the
Fd::INVALIDguards are in this diffMaking the thread actually exit reached two
close()loops that were previously unreachable. Both closed every watchlist fd unconditionally, but platforms that do not need a descriptor per watch storeFd::INVALID, whichbun_sys::closeasserts against — this surfaced aspanic: assertion failed: fd != Fd::INVALIDinthread_main. Both now skip invalid descriptors, matching whatflush_evictionsalready did. They are required by this change rather than a drive-by.How did you verify your code works?
test/bake/fixtures/deinitializationnow asserts noFile Watcherthread outlives its dev server, by counting threads by name in/proc/self/task(Linux-only; the check no-ops elsewhere).Verified it catches the regression: with this change stashed and the binary rebuilt, the test fails — six threads leaked. With it, zero.
Linux (x86_64):
test/bake/deinitialization,test/cli/watch,test/cli/hot— 23 pass, 0 fail.macOS (arm64, 26.5.2): the
EVFILT_USERmechanism was confirmed with a standalone probe — a thread blocked inkevent()with no timeout is woken byEVFILT_USER+NOTE_TRIGGER. The thread-count assertion itself is Linux-only, so the kqueue wakeup has no end-to-end test here; that is the weakest part of this PR.Windows is compile-checked only (
cargo check --target x86_64-pc-windows-msvc); I have no Windows host, so CI is the gate for that hunk.