fix(rspack_watcher): keep mtime baseline persistent across watch() cycles (#14077)

Author: stormslowly

crates/rspack_watcher/src/lib.rs

@@ -180,11 +180,20 @@ impl FsWatcher {
   ) -> Result<()> {
     self.path_manager.update(files, directories, missing)?;
 
-    // Record baseline mtimes for all registered files BEFORE starting the disk
-    // watcher. This must be synchronous to avoid a race where FSEvents delivers
-    // stale events before the baseline is populated (causing has_mtime_changed
-    // to find no baseline and let the stale event through).
-    self.record_file_mtimes();
+    // Record baseline mtimes for files added in this watch() cycle, BEFORE
+    // starting the disk watcher. Synchronous so a stale FSEvent delivered
+    // immediately after subscription still finds a baseline to compare
+    // against.
+    //
+    // Only newly-added files are stat'd here. Files carried across cycles
+    // already have a baseline that `has_mtime_changed` advanced atomically
+    // on the previous event; re-stat'ing them on every rewatch would
+    // (a) cost O(N) syscalls per rebuild on large projects, and
+    // (b) re-introduce the race fixed by this PR — a user write that
+    //     landed in the gap between aggregate and rewatch would have its
+    //     post-write mtime become the new baseline, and the FSEvent still
+    //     in flight for that write would then be suppressed as a duplicate.
+    self.record_initial_file_mtimes();
 
     self.scanner.scan(start_time);
 
@@ -194,15 +203,18 @@ impl FsWatcher {
     Ok(())
   }
 
-  fn record_file_mtimes(&self) {
+  fn record_initial_file_mtimes(&self) {
     let accessor = self.path_manager.access();
-    let paths: Vec<_> = accessor.files().0.iter().map(|p| p.clone()).collect();
+    // `.1` is `files.added` — the diff set populated by `update()` and
+    // cleared by `reset()`. In steady state (no add/remove churn) this
+    // is empty and the loop is a no-op.
+    let paths: Vec<_> = accessor.files().1.iter().map(|p| p.clone()).collect();
     for path in paths {
       if let Ok(mtime) = path
         .metadata()
         .and_then(|m| m.modified().or_else(|_| m.created()))
       {
-        self.path_manager.set_file_mtime(path, mtime);
+        self.path_manager.set_file_mtime_if_absent(path, mtime);
       }
     }
   }

crates/rspack_watcher/src/paths.rs

@@ -204,16 +204,41 @@ impl PathManager {
     }
   }
 
-  /// Reset the state of the `PathManager`, clearing all tracked paths.
+  /// Reset the per-`watch()`-call diff state (added / removed sets) without
+  /// touching the long-lived mtime baselines.
+  ///
+  /// `file_mtimes` is intentionally NOT cleared here: the mtime baselines
+  /// are tied to the lifetime of the registered files, not to the lifetime
+  /// of a single `FsWatcher::watch()` invocation. Each call to `watch()`
+  /// from rspack's rebuild cycle (aggregate → pause → rebuild → rewatch)
+  /// must NOT re-snapshot the baseline, otherwise the snapshot can capture
+  /// a post-user-write mtime and then `has_mtime_changed` silently
+  /// suppresses the very FSEvent that the user is waiting for. Stale
+  /// entries for files that have actually been unregistered are pruned by
+  /// `update()` via `remove_file_mtime`.
   pub fn reset(&self) {
     self.files.reset();
     self.directories.reset();
     self.missing.reset();
-    self.file_mtimes.clear();
   }
 
-  pub fn set_file_mtime(&self, path: ArcPath, mtime: SystemTime) {
-    self.file_mtimes.insert(path, mtime);
+  /// Record an initial baseline mtime for `path`, but only if no baseline
+  /// already exists. This is the "incremental" form used by
+  /// `FsWatcher::wait_for_event`: on the first `watch()` call we record
+  /// mtimes for all newly-registered files; on subsequent `watch()` calls
+  /// we MUST skip files that already have a baseline, otherwise we risk
+  /// snapshotting a mtime that the user has just bumped via `writeFile`.
+  ///
+  /// Use `has_mtime_changed` (which atomically reads-and-updates the
+  /// baseline) to advance the recorded mtime in response to real events.
+  pub fn set_file_mtime_if_absent(&self, path: ArcPath, mtime: SystemTime) {
+    self.file_mtimes.entry(path).or_insert(mtime);
+  }
+
+  /// Drop the baseline for a path that is no longer being watched, so the
+  /// map does not grow unboundedly across watch cycles.
+  pub fn remove_file_mtime(&self, path: &ArcPath) {
+    self.file_mtimes.remove(path);
   }
 
   /// Check if a file's mtime has changed from the stored baseline.
@@ -260,6 +285,15 @@ impl PathManager {
     PathUpdater::from(directories).update(&self.directories, &self.ignored)?;
     PathUpdater::from(missing).update(&self.missing, &self.ignored)?;
 
+    // Prune mtime baselines for files no longer being watched so the map
+    // does not grow unboundedly across `watch()` cycles. `reset()` has
+    // already cleared `self.files.removed` at the start of this `watch()`
+    // call, so what we see here is only this cycle's removals.
+    let removed_files: Vec<ArcPath> = self.files.removed.iter().map(|p| p.clone()).collect();
+    for path in &removed_files {
+      self.remove_file_mtime(path);
+    }
+
     Ok(())
   }
 
@@ -384,4 +418,90 @@ mod tests {
       assert!(all_paths.iter().any(|p| p.ends_with(path)));
     }
   }
+
+  /// Regression for the FSEvents stale-event race: simulate two consecutive
+  /// `FsWatcher::watch()` cycles with a real file write landing between
+  /// them (the slow-runner case where the FSEvent is in the kernel queue
+  /// but not yet delivered when the second cycle starts). The baseline
+  /// for the already-registered file must survive the second `reset()`,
+  /// so the delayed change event isn't suppressed as stale.
+  #[test]
+  fn test_baseline_persists_across_consecutive_watch_cycles() {
+    use std::{thread::sleep, time::Duration};
+
+    use tempfile::NamedTempFile;
+
+    let tempfile = NamedTempFile::new().expect("create temp file");
+    let path = ArcPath::from(tempfile.path());
+
+    let pm = PathManager::default();
+    pm.update(
+      (std::iter::once(path.clone()), std::iter::empty()),
+      (std::iter::empty(), std::iter::empty()),
+      (std::iter::empty(), std::iter::empty()),
+    )
+    .expect("register file");
+
+    // T0 — first `watch()` cycle records the baseline.
+    let initial_mtime = tempfile
+      .path()
+      .metadata()
+      .and_then(|m| m.modified())
+      .expect("read initial mtime");
+    pm.set_file_mtime_if_absent(path.clone(), initial_mtime);
+
+    // T3 — rspack starts a rebuild; `reset()` runs ahead of the next `watch()`.
+    pm.reset();
+
+    // Invariant: `file_mtimes` must survive `reset()`. Without this the
+    // baseline gets wiped on every watch cycle and the next bullet point
+    // can no longer hold.
+    assert_eq!(
+      pm.file_mtimes.get(&path).map(|v| *v),
+      Some(initial_mtime),
+      "file_mtimes must persist across reset()",
+    );
+
+    // T4 — a real write lands while no watcher is attached. The sleep
+    // covers 1s-resolution filesystems (HFS+, FAT); modern APFS / ext4 /
+    // NTFS would not need it but this regression must hold everywhere.
+    sleep(Duration::from_millis(1100));
+    std::fs::write(tempfile.path(), b"v2").expect("rewrite tempfile");
+    let post_write_mtime = tempfile
+      .path()
+      .metadata()
+      .and_then(|m| m.modified())
+      .expect("read post-write mtime");
+    assert_ne!(
+      post_write_mtime, initial_mtime,
+      "test sanity: file mtime must advance after the write",
+    );
+
+    // T5 — second `watch()` cycle reaches `record_initial_file_mtimes`,
+    // which delegates here. For an already-baselined path it must be a
+    // no-op so the post-write mtime does NOT overwrite the original.
+    pm.set_file_mtime_if_absent(path.clone(), post_write_mtime);
+    assert_eq!(
+      pm.file_mtimes.get(&path).map(|v| *v),
+      Some(initial_mtime),
+      "set_file_mtime_if_absent must not overwrite an existing baseline",
+    );
+
+    // T6 — the delayed FSEvent finally reaches `Trigger::on_event`, which
+    // calls `has_mtime_changed`. Current disk mtime now differs from the
+    // preserved baseline, so the event must NOT be suppressed.
+    assert!(
+      pm.has_mtime_changed(&path),
+      "delayed change event must not be suppressed as stale",
+    );
+
+    // `has_mtime_changed` also atomically advances the baseline so a
+    // subsequent duplicate FSEvent (e.g. re-delivery during rewatch)
+    // can still be filtered correctly.
+    assert_eq!(
+      pm.file_mtimes.get(&path).map(|v| *v),
+      Some(post_write_mtime),
+      "has_mtime_changed should advance the baseline on a real change",
+    );
+  }
 }