Fix rewatch losing watches when an overlapping source folder is removed

changes/rewatch-overlapping-unwatch.fix.md

@@ -0,0 +1 @@
+- Rewatch: keep watching a source folder on Linux when an overlapping recursive source folder is removed from `rescript.json`. https://github.com/rescript-lang/rescript/pull/8839

rewatch/src/watcher.rs

@@ -216,12 +216,32 @@ fn register_watches(
     registered
 }
 
-/// Brings the registered watches from `current` to `next`, touching only the paths that changed.
+/// Whether `update_watches` registers only the new paths, leaving unchanged watches alone, rather
+/// than registering every path again (see `update_watches` for why this differs per platform).
+const REGISTER_ONLY_NEW_WATCHES: bool = cfg!(target_os = "macos");
+
+/// Brings the registered watches from `current` to `next`, which happens on every full rebuild:
+/// after a `rescript.json` change, but also when a source file is added, removed or renamed.
+///
+/// Stale watches are unwatched first: those whose path is no longer in `next`, and those whose
+/// directory was replaced since it was registered. What happens to the other watches depends on
+/// how the platform's watcher backend (from the `notify` crate) treats overlapping watches:
+///
+/// - **macOS (FSEvents):** every `watch`/`unwatch` call restarts the event stream, and events that
+///   happen during the restart are lost. Each registered path is watched independently, so
+///   unwatching one path never affects another. Only the new paths are registered, so a full
+///   rebuild that keeps the same paths doesn't restart the stream at all.
+///
+/// - **Linux (inotify) and other platforms:** notify keeps one watch per directory, shared by
+///   every registered path that reaches it: overlapping source folders, other spellings of a
+///   folder (`..`, symlinks) and directory symlinks inside a recursive folder. Unwatching a stale
+///   path can therefore also drop a watch that is still needed (#8825). So every path in `next` is
+///   registered again after the unwatches. Registering a directory that is already watched only
+///   updates its existing watch, so watches that weren't dropped keep reporting events without a
+///   gap, and those that were dropped are restored.
 ///
-/// Some backends restart their event stream on every `watch`/`unwatch` call (FSEvents on macOS
-/// does), and events that happen during the restart are lost. Leaving unchanged watches alone
-/// avoids those gaps entirely in the common case where a full rebuild keeps the same paths.
-/// Watches whose directory was replaced since registration are registered again.
+///   Windows takes this path too. It has no stable directory identity, so every watch counts as
+///   stale there and is unwatched and registered again on each full rebuild.
 ///
 /// Returns the watches that are registered afterwards, which excludes paths that failed to register.
 fn update_watches(
@@ -236,6 +256,9 @@ fn update_watches(
     for watch in stale {
         let _ = watcher.unwatch(&watch.path);
     }
+    if !REGISTER_ONLY_NEW_WATCHES {
+        return register_watches(watcher, next);
+    }
     let added: WatchPaths = next
         .iter()
         .filter(|(path, mode)| {
@@ -999,4 +1022,143 @@ mod tests {
         std::fs::create_dir(&dir).expect("dir should be recreated");
         assert!(!watch.is_still_valid_for(&dir, RecursiveMode::Recursive));
     }
+
+    /// Creates the symlinks `(link, target)` of `links`, registers `before` with a real watcher,
+    /// updates the watches to `after`, and checks that a file written to `write_dir` afterwards is
+    /// reported. Paths are relative to a temp dir, and `""` is the temp dir itself.
+    #[cfg(target_os = "linux")]
+    fn assert_update_keeps_watching(
+        before: &[(&str, RecursiveMode)],
+        after: &[(&str, RecursiveMode)],
+        write_dir: &str,
+        links: &[(&str, &str)],
+    ) {
+        use std::sync::mpsc;
+
+        let temp_dir = tempfile::TempDir::new().expect("temp dir should be created");
+        let root = temp_dir.path();
+        for (link, target) in links {
+            std::fs::create_dir_all(root.join(target)).expect("dir should be created");
+            if let Some(parent) = root.join(link).parent() {
+                std::fs::create_dir_all(parent).expect("dir should be created");
+            }
+            std::os::unix::fs::symlink(root.join(target), root.join(link))
+                .expect("symlink should be created");
+        }
+        let to_watch_paths = |paths: &[(&str, RecursiveMode)]| -> WatchPaths {
+            paths
+                .iter()
+                .map(|(path, mode)| (root.join(path), *mode))
+                .collect()
+        };
+        for (path, _) in to_watch_paths(before).iter().chain(&to_watch_paths(after)) {
+            std::fs::create_dir_all(path).expect("dir should be created");
+        }
+        std::fs::create_dir_all(root.join(write_dir)).expect("dir should be created");
+
+        let (tx, rx) = mpsc::channel();
+        let mut watcher = RecommendedWatcher::new(tx, Config::default()).expect("watcher should start");
+        let current = register_watches(&mut watcher, &to_watch_paths(before));
+        let registered = update_watches(&mut watcher, current, &to_watch_paths(after));
+        assert_eq!(registered.len(), after.len());
+
+        let file = root.join(write_dir).join("Demo.res");
+        std::fs::write(&file, "let x = 2").expect("file should be written");
+        // Events name the file through whichever spelling of its directory notify registered
+        let resolved_file = std::fs::canonicalize(&file).expect("file should resolve");
+        let is_file = |path: &PathBuf| std::fs::canonicalize(path).ok().as_ref() == Some(&resolved_file);
+        let deadline = Instant::now() + Duration::from_secs(5);
+        loop {
+            let remaining = deadline.saturating_duration_since(Instant::now());
+            match rx.recv_timeout(remaining) {
+                Ok(Ok(event)) if event.paths.iter().any(is_file) => return,
+                Ok(_) => continue,
+                Err(_) => panic!(
+                    "no event for {} after updating watches from {before:?} to {after:?}",
+                    file.display()
+                ),
+            }
+        }
+    }
+
+    /// inotify shares one watch per directory between overlapping paths, so unwatching one path
+    /// can drop another (#8825). Watches that remain configured must keep reporting events.
+    #[cfg(target_os = "linux")]
+    #[test]
+    fn keeps_watching_after_an_overlapping_watch_is_removed() {
+        use RecursiveMode::{NonRecursive, Recursive};
+
+        // A full rebuild that keeps the same paths, overlapping ones included
+        assert_update_keeps_watching(
+            &[
+                ("", NonRecursive),
+                ("src", Recursive),
+                ("src/retained", NonRecursive),
+            ],
+            &[
+                ("", NonRecursive),
+                ("src", Recursive),
+                ("src/retained", NonRecursive),
+            ],
+            "src/retained",
+            &[],
+        );
+        // A folder kept below a removed recursive folder
+        assert_update_keeps_watching(
+            &[
+                ("", NonRecursive),
+                ("src", Recursive),
+                ("src/retained", NonRecursive),
+            ],
+            &[("", NonRecursive), ("src/retained", NonRecursive)],
+            "src/retained",
+            &[],
+        );
+        assert_update_keeps_watching(
+            &[("src", Recursive), ("src/retained", Recursive)],
+            &[("src/retained", Recursive)],
+            "src/retained/nested",
+            &[],
+        );
+        // A folder removed below a kept recursive folder
+        assert_update_keeps_watching(
+            &[("src", Recursive), ("src/retained", NonRecursive)],
+            &[("src", Recursive)],
+            "src/retained",
+            &[],
+        );
+        // A folder that stops being recursive
+        assert_update_keeps_watching(
+            &[("", Recursive), ("src", NonRecursive)],
+            &[("", NonRecursive), ("src", NonRecursive)],
+            "src",
+            &[],
+        );
+        // Other spellings of a folder below the removed one
+        assert_update_keeps_watching(
+            &[("other/../src/retained", NonRecursive), ("src", Recursive)],
+            &[("other/../src/retained", NonRecursive)],
+            "src/retained",
+            &[],
+        );
+        assert_update_keeps_watching(
+            &[("link", NonRecursive), ("src", Recursive)],
+            &[("link", NonRecursive)],
+            "src/retained",
+            &[("link", "src/retained")],
+        );
+        // A recursive folder reaching a separately watched one through a symlink
+        assert_update_keeps_watching(
+            &[("shared", NonRecursive), ("src", Recursive)],
+            &[("src", Recursive)],
+            "shared",
+            &[("src/link", "shared")],
+        );
+        assert_update_keeps_watching(
+            &[("shared", NonRecursive), ("src", Recursive)],
+            &[("shared", NonRecursive)],
+            "shared",
+            &[("src/link", "shared")],
+        );
+    }
 }