Make watcher shutdown synchronous

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<PathBuf>`. 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<notify::RecommendedWatcher>,
+    debouncer_thread: Option<thread::JoinHandle<()>>,
+    shutdown: Arc<AtomicBool>,
+}
+
+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<Event>, out: Sender<PathBuf>, debounce: Duration) {
+fn debouncer_loop(
+    raw_rx: mpsc::Receiver<Event>,
+    out: Sender<PathBuf>,
+    debounce: Duration,
+    shutdown: &AtomicBool,
+) {
     let mut pending: HashMap<PathBuf, Instant> = HashMap::new();
 
     loop {
@@ -155,6 +195,12 @@ fn debouncer_loop(raw_rx: mpsc::Receiver<Event>, out: Sender<PathBuf>, 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<PathBuf> = 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<_>>(),
+            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(),

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<AtomicBool>` 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.

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