From d3591fd08996c27a3767b25b80d22ba8831f2c11 Mon Sep 17 00:00:00 2001 From: Christoph Knittel Date: Fri, 16 Oct 2026 16:33:14 +0000 Subject: [PATCH] Keep watching a source folder when an overlapping one is removed Rust rewatch (`rescript-rust`) watches each source folder separately, and source folders may overlap. On a full rebuild it unwatched the paths that went away and registered only the new ones. With inotify on Linux, notify keeps one watch per directory, shared by every registered path that reaches it, so unwatching a removed recursive folder also dropped a folder that is still configured, and edits there were no longer compiled. Outside macOS, every configured path is now registered again after the stale ones are unwatched; registering an already watched directory only updates its watch, so unchanged watches have no gap. macOS keeps registering only the new paths, because FSEvents restarts its event stream on every watch change and its watches don't depend on each other. Watches are also registered non-recursive first. notify keeps one recursive flag per directory, set by the last registration that reaches it, so a non-recursive source folder inside a recursive one could clear the flag, and folders created there later weren't watched. Example (text): rescript.json of a package, while `rescript-rust watch` runs: "sources": [{"dir": "src", "subdirs": true}, "src/retained"] changed to "sources": ["src", "src/retained"] then src/retained/Retained.res is edited. Before the change, a file is also created in a new folder src/retained/nested. Before (text): The edit isn't compiled (Linux). The new folder's file is missed in most runs. After (text): Both are compiled. Source: rewatch/tests/watch/08-watch-overlapping-source-removed.sh, run against master's rescript-rust (the edit fails 2 of 2), this branch without the registration order (the new folder fails 7 of 8) and this branch (passes 8 of 8). Changelog entry: - 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 Review: Is OCaml rewatch (`rescript`) affected? No. It resolves every watched folder to its canonical directory, keeps one libuv handle per directory, and on a refresh closes only the directories no longer wanted. The new test runs against both executables in the shared suite and passes for `rescript` on master already. Review: Why not keep the incremental update everywhere? Watches can share inotify state through overlapping folders, `..` and symlinks, including directory symlinks inside a recursive folder; working out every way an unwatch can drop another watch took several review rounds. Registering every path again needs no such reasoning and costs no gap on inotify. Review: Can an edit be missed while the watches are updated? Only after a config change that removes a recursive folder overlapping a kept one: the kept folder's watch is dropped by the unwatch and restored when every path is registered again, so an edit in between is picked up only with the next change. Ordinary full rebuilds unwatch nothing. Closing that window would need a rescan after registration, which isn't worth it for the Rust fallback. Review: Why does macOS keep the incremental update? #8730 introduced it because FSEvents restarts its event stream on every watch or unwatch call and loses the events in between. FSEvents watches are independent paths, so an unwatch never drops another watch there. Area: rewatch tests Changelog: fix Output: unchanged Tests: +rewatch/tests/watch/08-watch-overlapping-source-removed.sh ~rewatch/tests/suite.sh PR: #8839 Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01BC5F5QPeQh5cHBjGBnvJ2T Signed-off-by: Christoph Knittel --- changes/rewatch-overlapping-unwatch.fix.md | 1 + rewatch/src/watcher.rs | 42 +++++- rewatch/tests/suite.sh | 1 + .../08-watch-overlapping-source-removed.sh | 135 ++++++++++++++++++ 4 files changed, 173 insertions(+), 6 deletions(-) create mode 100644 changes/rewatch-overlapping-unwatch.fix.md create mode 100755 rewatch/tests/watch/08-watch-overlapping-source-removed.sh diff --git a/changes/rewatch-overlapping-unwatch.fix.md b/changes/rewatch-overlapping-unwatch.fix.md new file mode 100644 index 00000000000..61454ff2115 --- /dev/null +++ b/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 diff --git a/rewatch/src/watcher.rs b/rewatch/src/watcher.rs index d27e416a4e3..4e58c1dff60 100644 --- a/rewatch/src/watcher.rs +++ b/rewatch/src/watcher.rs @@ -194,7 +194,14 @@ fn register_watches( watch_paths: &[(PathBuf, RecursiveMode)], ) -> Vec { 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)| { diff --git a/rewatch/tests/suite.sh b/rewatch/tests/suite.sh index 5f8d194f7ab..84e84e1803f 100755 --- a/rewatch/tests/suite.sh +++ b/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 && diff --git a/rewatch/tests/watch/08-watch-overlapping-source-removed.sh b/rewatch/tests/watch/08-watch-overlapping-source-removed.sh new file mode 100755 index 00000000000..e8c8066b57d --- /dev/null +++ b/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