From 83346c1f303030bceb6fe569dae2bf13d61f99f5 Mon Sep 17 00:00:00 2001 From: Frank O'Hara Date: Sat, 8 Aug 2026 11:59:59 -0600 Subject: [PATCH] Make watcher shutdown synchronous Drop signalled teardown but detached the debouncer thread, so events could still arrive after drop. Signal a shutdown flag, drop the watcher, then join. --- crates/lash-core/src/watcher.rs | 114 ++++++++++++++++++++++++++++---- devlog.md | 33 +++++++++ tasks/tasks.live-updates.md | 5 +- 3 files changed, 138 insertions(+), 14 deletions(-) diff --git a/crates/lash-core/src/watcher.rs b/crates/lash-core/src/watcher.rs index 3d0306f..ccba3bb 100644 --- a/crates/lash-core/src/watcher.rs +++ b/crates/lash-core/src/watcher.rs @@ -7,7 +7,9 @@ //! The watcher runs on its own thread and forwards events to a caller-owned //! `mpsc::Sender`. Returning a handle that owns the underlying //! `notify::RecommendedWatcher` makes shutdown trivial: drop the handle and -//! the watcher (and its thread) tear down. +//! the watcher (and its thread) tear down. The handle's `Drop` joins the +//! debouncer thread, so shutdown is complete — not merely started — by the +//! time `drop` returns. //! //! Filtering & debouncing happen inline in the watcher thread: //! - non-`.md` paths are dropped @@ -18,7 +20,9 @@ use std::collections::HashMap; use std::path::{Path, PathBuf}; +use std::sync::atomic::{AtomicBool, Ordering}; use std::sync::mpsc::{self, Sender}; +use std::sync::Arc; use std::thread; use std::time::{Duration, Instant}; @@ -36,12 +40,39 @@ const IGNORED_DIRS: &[&str] = &[".git", "target", ".lash", "node_modules"]; /// Handle to a running watcher. Drop this to stop the watcher thread. /// -/// We hold both the `notify` watcher (whose drop stops backend events) and -/// the debouncer thread's join handle (which the watcher's death will cause -/// to exit cleanly as its input channel is hung up). +/// Shutdown is synchronous: once `drop` returns, the debouncer thread has +/// exited and no further paths can be sent to the caller's channel. Callers +/// can therefore drop the handle and immediately assume the channel is inert. +/// +/// The watcher and thread fields are `Option` only so `Drop` can take them +/// and enforce the order below; they are always `Some` while the handle is +/// alive. pub struct FileWatcherHandle { - _watcher: notify::RecommendedWatcher, - _debouncer_thread: thread::JoinHandle<()>, + watcher: Option, + debouncer_thread: Option>, + shutdown: Arc, +} + +impl Drop for FileWatcherHandle { + fn drop(&mut self) { + // Signal before anything else. Dropping the watcher is not enough on + // its own: the backend can outlive its own drop briefly (FSEvents on + // macOS does), during which the debouncer may reach the flush deadline + // for an already-pending path and emit it — after the caller believed + // the watcher was gone. The flag closes that window; the debouncer + // checks it before every emit. + self.shutdown.store(true, Ordering::Release); + + // Then drop the watcher, which destroys the `notify` closure holding + // the debouncer's input sender. That disconnect is what lets the loop + // return, so this must happen before the join or it would deadlock. + drop(self.watcher.take()); + + if let Some(thread) = self.debouncer_thread.take() { + // A panicking debouncer must not turn into a panic-in-drop. + let _ = thread.join(); + } + } } /// Start a watcher rooted at `root`. Each Markdown file change (after @@ -93,9 +124,12 @@ pub fn start_with_debounce( io_error: Some(e.to_string()), })?; + let shutdown = Arc::new(AtomicBool::new(false)); + let thread_shutdown = Arc::clone(&shutdown); + let debouncer_thread = thread::Builder::new() .name("lash-file-watcher-debouncer".into()) - .spawn(move || debouncer_loop(raw_rx, out, debounce)) + .spawn(move || debouncer_loop(raw_rx, out, debounce, &thread_shutdown)) .map_err(|e| LashError::IO { code: codes::E_IO_READ_ERROR, message: format!("failed to spawn watcher debouncer thread: {e}"), @@ -104,8 +138,9 @@ pub fn start_with_debounce( })?; Ok(FileWatcherHandle { - _watcher: watcher, - _debouncer_thread: debouncer_thread, + watcher: Some(watcher), + debouncer_thread: Some(debouncer_thread), + shutdown, }) } @@ -128,7 +163,12 @@ fn is_relevant(path: &Path) -> bool { /// even if no further events arrive — without that, the very last event in a /// burst would sit in the pending map forever. #[allow(clippy::needless_pass_by_value)] -fn debouncer_loop(raw_rx: mpsc::Receiver, out: Sender, debounce: Duration) { +fn debouncer_loop( + raw_rx: mpsc::Receiver, + out: Sender, + debounce: Duration, + shutdown: &AtomicBool, +) { let mut pending: HashMap = HashMap::new(); loop { @@ -155,6 +195,12 @@ fn debouncer_loop(raw_rx: mpsc::Receiver, out: Sender, debounce: Err(mpsc::RecvTimeoutError::Disconnected) => return, } + // The handle has been dropped: abandon anything still pending rather + // than racing the teardown to emit it. + if shutdown.load(Ordering::Acquire) { + return; + } + let now = Instant::now(); let mut to_emit: Vec = Vec::new(); pending.retain(|path, &mut deadline| { @@ -209,6 +255,43 @@ mod tests { out } + fn md_event(path: &Path) -> Event { + Event::new(EventKind::Modify(notify::event::ModifyKind::Any)).add_path(path.to_path_buf()) + } + + /// The shutdown flag must win over a path that is already due to flush. + /// A zero debounce makes the path due the instant it is recorded, which is + /// the race the flag exists to close — the backend can keep feeding events + /// for a short while after the handle is dropped, and without the flag the + /// debouncer would emit them. + #[test] + fn shutdown_flag_suppresses_due_emissions() { + let path = PathBuf::from("/tmp/proj/tasks.md"); + + // Control: no shutdown signalled, so the due path is emitted. + let (raw_tx, raw_rx) = mpsc::channel(); + let (out_tx, out_rx) = mpsc::channel(); + raw_tx.send(md_event(&path)).unwrap(); + drop(raw_tx); + debouncer_loop(raw_rx, out_tx, Duration::ZERO, &AtomicBool::new(false)); + assert_eq!( + out_rx.iter().collect::>(), + vec![path.clone()], + "control: a due path should be emitted when not shutting down" + ); + + // Same input, shutdown already signalled: the path is abandoned. + let (raw_tx, raw_rx) = mpsc::channel(); + let (out_tx, out_rx) = mpsc::channel(); + raw_tx.send(md_event(&path)).unwrap(); + drop(raw_tx); + debouncer_loop(raw_rx, out_tx, Duration::ZERO, &AtomicBool::new(true)); + assert!( + out_rx.iter().next().is_none(), + "no path may be emitted once shutdown is signalled" + ); + } + #[test] fn is_relevant_accepts_md_outside_ignored() { assert!(is_relevant(Path::new("/tmp/foo/tasks.md"))); @@ -303,11 +386,16 @@ mod tests { let (tx, rx) = mpsc::channel(); let handle = start_with_debounce(root, tx, Duration::from_millis(50)).unwrap(); std::thread::sleep(Duration::from_millis(80)); + + // No settling sleep after this point on purpose: `drop` joins the + // debouncer thread, so the channel must already be inert when it + // returns. A sleep here would hide a regression back to detached + // shutdown, which failed intermittently on loaded CI runners. drop(handle); - // Allow watcher to fully tear down. - std::thread::sleep(Duration::from_millis(50)); - std::fs::write(&path, "after drop\n").unwrap(); + for i in 0..10 { + std::fs::write(&path, format!("after drop {i}\n")).unwrap(); + } let events = drain_events(&rx, Duration::from_millis(200)); assert!( events.is_empty(), diff --git a/devlog.md b/devlog.md index 266f0d2..5a221ac 100644 --- a/devlog.md +++ b/devlog.md @@ -2386,3 +2386,36 @@ formula no-ops; missing artifact and missing-formula-in-plan both fail loudly. **2. Empty tap repo.** `actions/checkout` cannot check out a repository with no commits (actions/checkout#1477, #746), so the tap needs at least one commit before the first release runs. + +--- + +## Watcher shutdown race (`dropping_handle_stops_events`) + +`Test (macos-latest, stable)` failed once on PR #30 with +`no events expected after handle is dropped; got [".../tasks.md"]`, then passed +on re-run with no code change. The diff at the time was workflow YAML only, so +the test — not the change — was at fault. + +`FileWatcherHandle` held `_debouncer_thread: JoinHandle<()>`, and dropping a +`JoinHandle` detaches rather than joins. So `drop(handle)` merely *started* +teardown; the test's fixed 50 ms sleep was the only thing standing between that +and the following write, with the debounce window also 50 ms — right on the +boundary. + +Joining the thread alone is not sufficient. The `notify` backend can outlive its +own drop briefly (FSEvents does), so the debouncer can reach the flush deadline +for an already-pending path and emit it before it ever observes the disconnect; +a join would just wait through that emission. The fix is an +`Arc` shutdown flag that `Drop` sets *before* dropping the watcher, +checked by the loop before every emit. Drop order is load-bearing and commented: +signal, drop the watcher (which disconnects the debouncer's input), then join. +Joining before dropping the watcher would deadlock. + +Testing: the sleep-based test could not be made to fail locally even under CPU +contention (40/40 passes), so it is a poor regression guard. Replaced the guard +with `shutdown_flag_suppresses_due_emissions`, which drives `debouncer_loop` +directly with a zero debounce — the path is due the instant it is recorded — and +asserts both directions: emitted when not shutting down, abandoned when it is. +No sleeps, no filesystem, runs in 0.00s, and verified to FAIL when the guard is +removed. `dropping_handle_stops_events` also lost its post-drop settling sleep, +since the whole point is that none is needed. diff --git a/tasks/tasks.live-updates.md b/tasks/tasks.live-updates.md index 84b7115..fbbb524 100644 --- a/tasks/tasks.live-updates.md +++ b/tasks/tasks.live-updates.md @@ -121,7 +121,10 @@ paths. Forwards collapsed events as `PathBuf` on an `mpsc::Sender`. - [x] `watcher::start(project_root, tx)` spawns thread, returns handle - [x] Debounce window 150ms (configurable via `start_with_debounce` for tests); coalesce per-path - [x] Ignore-list: `.git/`, `target/`, `.lash/`, `node_modules/`, and any path not ending in `.md` -- [x] Graceful shutdown on handle drop (verified by test) +- [x] Graceful shutdown on handle drop: `Drop` signals a shutdown flag, drops + the watcher, then joins the debouncer, so no path can be emitted once + `drop` returns (verified by `shutdown_flag_suppresses_due_emissions`; + the original sleep-based test was an unreliable guard and flaked on CI) - [x] Test: edit a fixture file, assert a small number of events surface after debounce - [x] Test: non-`.md` writes produce 0 events