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

@@ -194,7 +194,14 @@ fn register_watches(
     watch_paths: &[(PathBuf, RecursiveMode)],
 ) -> Vec<RegisteredWatch> {
     let mut registered = Vec::with_capacity(watch_paths.len());
-    for (path, mode) in watch_paths {
+    // Register non-recursive paths first. notify's inotify backend keeps one
+    // recursive flag per directory, set by the last registration that reaches
+    // it, and only watches a folder created later if its parent's flag is
+    // recursive. A non-recursive path inside a recursive one (an overlapping
+    // source folder) must not clear the flag that the recursive one set.
+    let mut ordered: Vec<&(PathBuf, RecursiveMode)> = watch_paths.iter().collect();
+    ordered.sort_by_key(|(_, mode)| *mode == RecursiveMode::Recursive);
+    for (path, mode) in ordered {
         let mode_str = if *mode == RecursiveMode::Recursive {
             "recursive"
         } else {
@@ -216,12 +223,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.
 ///
-/// 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.
+/// - **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.
+///
+///   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 +263,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)| {

rewatch/tests/suite.sh

@@ -150,6 +150,7 @@ fi
 ./watch/05-watch-ignores-non-source.sh &&
 ./watch/06-watch-missing-source-folder.sh &&
 ./watch/07-changed-interface-after-failed-implementation.sh &&
+./watch/08-watch-overlapping-source-removed.sh &&
 
 # Lock tests
 ./lock/01-lock-when-watching.sh &&

rewatch/tests/watch/08-watch-overlapping-source-removed.sh

@@ -0,0 +1,135 @@
+#!/bin/bash
+cd $(dirname $0)
+source "../utils.sh"
+cd ../../testrepo
+
+bold "Test: Watcher keeps watching a folder when an overlapping source folder is removed"
+
+error_output=$(rewatch clean 2>&1)
+if [ $? -eq 0 ];
+then
+  success "Repo Cleaned"
+else
+  error "Error Cleaning Repo"
+  printf "%s\n" "$error_output" >&2
+  exit 1
+fi
+
+DEP01_CONFIG=packages/dep01/rescript.json
+RETAINED=packages/dep01/src/retained
+
+cleanup() {
+  git restore --worktree -- "$DEP01_CONFIG"
+  rm -rf "$RETAINED"
+  rm -f rewatch.log
+}
+
+# src/retained is watched both through the recursive src folder and as a
+# source folder of its own
+mkdir -p "$RETAINED"
+echo 'let value = 1' > "$RETAINED/Retained.res"
+node -e "
+  const fs = require('fs');
+  const config = JSON.parse(fs.readFileSync('$DEP01_CONFIG', 'utf8'));
+  config.sources = [{dir: 'src', subdirs: true}, 'src/retained'];
+  fs.writeFileSync('$DEP01_CONFIG', JSON.stringify(config, null, 2) + '\n');
+"
+
+retained_output() {
+  find "$RETAINED" -name 'Retained.*js' | head -n 1
+}
+
+rewatch_bg watch > rewatch.log 2>&1 &
+success "Watcher Started"
+
+wait_for_retained_value() {
+  local value=$1
+  local timeout=30
+  while [ "$timeout" -gt 0 ]; do
+    output=$(retained_output)
+    if [ -n "$output" ] && grep -q "$value" "$output"; then
+      return 0
+    fi
+    sleep 1
+    timeout=$((timeout - 1))
+  done
+  return 1
+}
+
+if ! wait_for_retained_value "= 1"; then
+  error "Initial build did not compile the retained folder"
+  cat rewatch.log
+  exit_watcher
+  cleanup
+  exit 1
+fi
+success "Initial build completed"
+
+# A folder created inside src/retained belongs to the recursive src folder, so
+# the watch of src/retained must stay recursive although src/retained is also
+# watched on its own
+mkdir "$RETAINED/nested"
+echo 'let value = 1' > "$RETAINED/nested/Nested.res"
+timeout=30
+until [ -n "$(find "$RETAINED/nested" -name 'Nested.*js')" ] || [ "$timeout" -le 0 ]; do
+  sleep 1
+  timeout=$((timeout - 1))
+done
+if [ -n "$(find "$RETAINED/nested" -name 'Nested.*js')" ]; then
+  success "A folder created inside the retained folder is watched"
+else
+  error "A folder created inside the retained folder is not watched"
+  cat rewatch.log
+  exit_watcher
+  cleanup
+  exit 1
+fi
+
+completed_builds=$(grep -c "Finished .*compilation" rewatch.log 2>/dev/null || true)
+
+# Remove the recursive src folder while keeping src itself (non-recursive)
+# and src/retained
+node -e "
+  const fs = require('fs');
+  const config = JSON.parse(fs.readFileSync('$DEP01_CONFIG', 'utf8'));
+  config.sources = ['src', 'src/retained'];
+  fs.writeFileSync('$DEP01_CONFIG', JSON.stringify(config, null, 2) + '\n');
+"
+if ! wait_for_pattern_count rewatch.log "Finished .*compilation" \
+  $((completed_builds + 1)) 30; then
+  error "No rebuild after the configuration change"
+  cat rewatch.log
+  exit_watcher
+  cleanup
+  exit 1
+fi
+
+echo 'let value = 2' > "$RETAINED/Retained.res"
+if wait_for_retained_value "= 2"; then
+  success "Edit in the retained folder was compiled"
+else
+  error "Edit in the retained folder was not picked up"
+  cat rewatch.log
+  exit_watcher
+  cleanup
+  exit 1
+fi
+
+if ! exit_watcher; then
+  cleanup
+  exit 1
+fi
+cleanup
+
+# Rebuild to regenerate the outputs removed by `rewatch clean`
+rewatch build > /dev/null 2>&1
+
+if git diff --exit-code . > /dev/null 2>&1 && [ -z "$(git ls-files --others --exclude-standard .)" ];
+then
+  success "No leftover changes"
+else
+  error "Leftover changes detected"
+  git diff .
+  git ls-files --others --exclude-standard .
+  exit 1
+fi