Stabilize rewatch scheduling and integration tests (#8667)

* Fix deterministic rewatch scheduling

Signed-off-by: Christoph Knittel <ck@cca.io>

* Stabilize rewatch integration tests

Signed-off-by: Christoph Knittel <ck@cca.io>

* Preserve blocked dependents across full rewatch rebuilds

Signed-off-by: Christoph Knittel <ck@cca.io>

* Abort warning persistence test if watcher stays running

Signed-off-by: Christoph Knittel <ck@cca.io>

* Abort watcher tests when shutdown times out

Signed-off-by: Christoph Knittel <ck@cca.io>

---------

Signed-off-by: Christoph Knittel <ck@cca.io>

Author: cknitt

CHANGELOG.md

@@ -20,6 +20,8 @@
 
 #### :bug: Bug fix
 
+- Make rewatch compile independent modules after an unrelated failure and recompile blocked dependents when a changed interface survives a failed implementation, including across full watcher rebuilds. https://github.com/rescript-lang/rescript/pull/8667
+
 #### :memo: Documentation
 
 #### :nail_care: Polish

rewatch/AGENTS.md

@@ -217,5 +217,5 @@ When clippy suggests refactoring that could impact performance, consider the tra
 ## CI Gotchas
 
 - **`sleep` is fragile** — Prefer polling (e.g., `wait_for_file`) over fixed sleeps. CI runners are slower than local machines.
-- **`exit_watcher` is async** — It only signals the watcher to stop (removes the lock file), it doesn't wait for the process to exit. Avoid triggering config-change events before exiting, as the watcher may start a concurrent rebuild.
+- **Wait for watcher shutdown with `exit_watcher`** — It removes the lock file and waits for the recorded watcher process to exit. Check its return status before continuing when later mutations could race with the watcher.
 - **`sed -i` differs across platforms** — macOS requires `sed -i '' ...`, Linux does not. Use the `replace` / `normalize_paths` helpers from `rewatch/tests/utils.sh` instead of raw `sed`.

rewatch/src/build/compile.rs

@@ -440,7 +440,11 @@ pub fn compile(
     rayon::in_place_scope(|scope| {
         let mut in_flight: usize = 0;
         loop {
-            while in_flight < capacity && !has_errors {
+            // Keep dispatching work that was already independent of any
+            // failed module. Otherwise diagnostics vary with worker count:
+            // a warning-producing module may or may not have started before
+            // an unrelated error finishes.
+            while in_flight < capacity {
                 let Some(work) = ready_heap.pop() else { break };
                 let module_name = work.module_name.clone();
                 let is_dirty = dirty_set.contains(&module_name);
@@ -462,10 +466,6 @@ pub fn compile(
             }
 
             if in_flight == 0 {
-                if !ready_heap.is_empty() {
-                    // Errors suppressed new spawns; nothing left to drain.
-                    break;
-                }
                 if completed.len() < compile_universe_count && !has_errors {
                     stalled = true;
                 }
@@ -475,7 +475,8 @@ pub fn compile(
             let Ok(msg) = rx.recv() else { break };
             in_flight -= 1;
 
-            if msg.result.is_err() || msg.interface_result.as_ref().is_some_and(|r| r.is_err()) {
+            let failed = msg.result.is_err() || msg.interface_result.as_ref().is_some_and(|r| r.is_err());
+            if failed {
                 has_errors = true;
             }
 
@@ -493,16 +494,21 @@ pub fn compile(
                 if !compile_universe.contains(dep) {
                     continue;
                 }
+                // A successful interface can replace the CMI even when its
+                // implementation fails. Keep dependents dirty for recovery,
+                // but do not release them until the module succeeds.
                 if !is_clean {
                     dirty_set.insert(dep.clone());
                 }
-                let count = pending_deps.get_mut(dep).unwrap();
-                *count -= 1;
-                if *count == 0 && !completed.contains(dep) {
-                    ready_heap.push(WorkUnit {
-                        priority: priorities[dep],
-                        module_name: dep.clone(),
-                    });
+                if !failed {
+                    let count = pending_deps.get_mut(dep).unwrap();
+                    *count -= 1;
+                    if *count == 0 && !completed.contains(dep) {
+                        ready_heap.push(WorkUnit {
+                            priority: priorities[dep],
+                            module_name: dep.clone(),
+                        });
+                    }
                 }
             }
         }
@@ -523,9 +529,9 @@ pub fn compile(
     let mut num_compiled_modules = 0;
 
     // Persist propagated dirtiness back onto build_state. Modules that were
-    // marked dirty (because a predecessor's cmi changed) but never scheduled
-    // — e.g. the first compile error aborted further dispatch — must keep
-    // compile_dirty = true so the next incremental build recompiles them.
+    // marked dirty (because a predecessor's cmi changed) but blocked by a
+    // failed prerequisite must keep compile_dirty = true so the next
+    // incremental build recompiles them.
     // Successful recompiles in the result loop below override this back to
     // false for the modules that actually ran.
     for name in &dirty_set {

rewatch/src/watcher.rs

@@ -172,7 +172,7 @@ fn unregister_watches(watcher: &mut RecommendedWatcher, watch_paths: &[(PathBuf,
     }
 }
 
-fn carry_forward_compile_warnings(previous: &BuildCommandState, next: &mut BuildCommandState) {
+fn carry_forward_compile_state(previous: &BuildCommandState, next: &mut BuildCommandState) {
     for (module_name, next_module) in next.build_state.modules.iter_mut() {
         let Some(previous_module) = previous.build_state.modules.get(module_name) else {
             continue;
@@ -184,6 +184,12 @@ fn carry_forward_compile_warnings(previous: &BuildCommandState, next: &mut Build
         match (&previous_module.source_type, &mut next_module.source_type) {
             (SourceType::SourceFile(previous_source), SourceType::SourceFile(next_source)) => {
                 if previous_source.implementation.path == next_source.implementation.path {
+                    // A changed CMI can leave a dependent blocked behind a failed
+                    // implementation. Asset timestamps alone cannot recover this
+                    // dirtiness when a full watcher rebuild recreates the state.
+                    if previous_module.compile_dirty {
+                        next_module.compile_dirty = true;
+                    }
                     next_source.implementation.compile_warnings =
                         previous_source.implementation.compile_warnings.clone();
 
@@ -501,9 +507,8 @@ async fn async_watch(
                     .expect("Could not initialize build");
 
                     // Full rebuilds can be triggered by editor atomic saves that surface as rename events.
-                    // Preserve warning state for unchanged modules so their warnings are re-emitted after the
-                    // fresh build state replaces the previous one.
-                    carry_forward_compile_warnings(&build_state, &mut next_build_state);
+                    // Preserve warnings and blocked dirty modules when fresh state replaces the previous one.
+                    carry_forward_compile_state(&build_state, &mut next_build_state);
                     build_state = next_build_state;
 
                     // Re-register watches based on the new build state
@@ -804,7 +809,7 @@ mod tests {
         );
         let mut next = test_build_state("ModuleA", test_module("src/ModuleA.res", None, None, None));
 
-        carry_forward_compile_warnings(&previous, &mut next);
+        carry_forward_compile_state(&previous, &mut next);
 
         let module = next.get_module("ModuleA").expect("module should exist");
         let SourceType::SourceFile(source_file) = &module.source_type else {
@@ -826,7 +831,7 @@ mod tests {
         );
         let mut next = test_build_state("ModuleA", test_module("src/ModuleARenamed.res", None, None, None));
 
-        carry_forward_compile_warnings(&previous, &mut next);
+        carry_forward_compile_state(&previous, &mut next);
 
         let module = next.get_module("ModuleA").expect("module should exist");
         let SourceType::SourceFile(source_file) = &module.source_type else {
@@ -853,7 +858,7 @@ mod tests {
             test_module("src/ModuleA.res", None, Some("src/ModuleA.resi"), None),
         );
 
-        carry_forward_compile_warnings(&previous, &mut next);
+        carry_forward_compile_state(&previous, &mut next);
 
         let module = next.get_module("ModuleA").expect("module should exist");
         let SourceType::SourceFile(source_file) = &module.source_type else {
@@ -864,4 +869,24 @@ mod tests {
         assert_eq!(interface.compile_warnings.as_deref(), Some("warning: interface"));
         assert_eq!(interface.compile_state, CompileState::Warning);
     }
+
+    #[test]
+    fn carries_forward_blocked_dirtiness_only_for_matching_sources() {
+        let mut previous = test_build_state("ModuleA", test_module("src/ModuleA.res", None, None, None));
+        previous
+            .build_state
+            .modules
+            .get_mut("ModuleA")
+            .unwrap()
+            .compile_dirty = true;
+
+        let mut same_source = test_build_state("ModuleA", test_module("src/ModuleA.res", None, None, None));
+        carry_forward_compile_state(&previous, &mut same_source);
+        assert!(same_source.get_module("ModuleA").unwrap().compile_dirty);
+
+        let mut different_source =
+            test_build_state("ModuleA", test_module("src/Other.res", None, None, None));
+        carry_forward_compile_state(&previous, &mut different_source);
+        assert!(!different_source.get_module("ModuleA").unwrap().compile_dirty);
+    }
 }

rewatch/tests/clean/01-clean-single-project.sh

@@ -42,7 +42,7 @@ other_project_compiled_files=$(find packages/new-namespace -type f -name '*.mjs'
 if [ "$other_project_compiled_files" -gt 0 ];
 then
   success "Didn't clean other project files"
-  git restore .
+  git restore --worktree -- .
 else
   error "Expected files from new-namespace not to be cleaned"
   exit 1

rewatch/tests/clean/02-clean-dev-dependencies.sh

@@ -31,7 +31,7 @@ project_compiled_files=$(find packages/pure-dev -type f -name '*.mjs' | wc -l |
 if [ "$project_compiled_files" -eq 0 ];
 then
   success "pure-dev cleaned"
-  git restore .
+  git restore --worktree -- .
 else
   error "Expected 0 .mjs files in pure-dev after clean, got $project_compiled_files"
   printf "%s\n" "$error_output"

rewatch/tests/clean/03-clean-node-modules.sh

@@ -31,7 +31,7 @@ compiler_assets=$(find node_modules/rescript-nodejs/lib/ocaml -type f -name '*.*
 if [ $compiler_assets -eq 0 ];
 then
   success "compiler assets from node_modules cleaned"
-  git restore .
+  git restore --worktree -- .
 else
   error "Expected 0 files in node_modules/rescript-nodejs/lib/ocaml after clean, got $compiler_assets"
   printf "%s\n" "$error_output"

rewatch/tests/clean/04-clean-rebuild-no-compiler-update.sh

@@ -18,24 +18,25 @@ else
   exit 1
 fi
 
-# Rebuild with snapshot output
-snapshot_file=../tests/snapshots/clean-rebuild.txt
-rewatch build &> $snapshot_file
+# Rebuild with captured output. This test checks one lifecycle message; writing
+# into a tracked snapshot made the fixture dirty whenever work counts changed.
+output_file=$(mktemp "${TMPDIR:-/tmp}/rewatch-clean-rebuild.XXXXXX")
+trap 'rm -f "$output_file"' EXIT
+rewatch build &> "$output_file"
 build_status=$?
-normalize_paths $snapshot_file
 if [ $build_status -eq 0 ];
 then
   success "Repo Built"
 else
   error "Error Building Repo"
-  cat $snapshot_file >&2
+  cat "$output_file" >&2
   exit 1
 fi
 
 # Verify the undesired message is NOT present
-if grep -q "Cleaned previous build due to compiler update" $snapshot_file; then
+if grep -q "Cleaned previous build due to compiler update" "$output_file"; then
   error "Unexpected compiler-update clean message present in rebuild logs"
-  cat $snapshot_file >&2
+  cat "$output_file" >&2
   exit 1
 else
   success "No compiler-update clean message present after explicit clean"

rewatch/tests/compile/08-remove-file.sh

@@ -11,7 +11,7 @@ rewatch build &> /dev/null
 rm packages/dep02/src/Dep02.res
 rewatch build &> ../tests/snapshots/remove-file.txt
 normalize_paths ../tests/snapshots/remove-file.txt
-git checkout -- packages/dep02/src/Dep02.res
+git restore --worktree -- packages/dep02/src/Dep02.res
 
 rewatch build &> /dev/null
 

rewatch/tests/compile/09-dependency-cycle.sh

@@ -11,7 +11,7 @@ rewatch build &> /dev/null
 echo 'Dep01.log()' >> packages/new-namespace/src/NS_alias.res
 rewatch build &> ../tests/snapshots/dependency-cycle.txt
 normalize_paths ../tests/snapshots/dependency-cycle.txt
-git checkout -- packages/new-namespace/src/NS_alias.res
+git restore --worktree -- packages/new-namespace/src/NS_alias.res
 
 rewatch build &> /dev/null