Skip to content

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

Merged
stormslowly merged 4 commits into
mainfrom
fix/fsevents-mtime-race
May 26, 2026
Merged

fix(rspack_watcher): keep mtime baseline persistent across watch() cycles#14077
stormslowly merged 4 commits into
mainfrom
fix/fsevents-mtime-race

Conversation

@stormslowly

@stormslowly stormslowly commented May 18, 2026

Copy link
Copy Markdown
Contributor

Why

On slow macOS runners, FsWatcher (used by experiments.nativeWatcher) silently drops a real user edit between rebuild cycles. Symptom: No test files need re-run with stale output. Observed deterministically in rstest #1261 on macos-14 + node 24.

Root cause: PathManager::reset() wiped file_mtimes and wait_for_event() re-stat'd every registered file on every FsWatcher::watch() call. If the user's write landed between aggregate and rewatch — possible because FSEvents delivery is async — the re-stat captured the post-write mtime as the new baseline. The in-flight FSEvent then arrived to find current == baseline and was suppressed as a duplicate.

What

Tie mtime baselines to the file's lifetime, not to a single watch() call (watchpack model).

  • reset() no longer clears file_mtimes; only the per-cycle added/removed diff is reset.
  • update() prunes mtime entries for files unregistered this cycle so the map cannot grow unboundedly.
  • record_initial_file_mtimes iterates files.added (this cycle's new files) instead of files.all. Steady-state rewatch now issues 0 stats instead of O(N).

Test plan

  • New unit test test_baseline_persists_across_consecutive_watch_cycles covers reset → post-write → delayed-event-not-suppressed.
  • rstest #1261

…cles

The previous mtime gate was correct in concept but had a fatal race in
its lifecycle handling. `PathManager::reset()` cleared `file_mtimes`
and `wait_for_event()` re-snapshotted every registered file's current
mtime as the new baseline -- on EVERY `FsWatcher::watch()` call, which
rspack invokes on every rebuild cycle (aggregate -> pause -> rebuild
-> rewatch). On a slow runner (observed on GHA macos-14 + node 24)
FSEvents queues the user-triggered Change event, rspack re-enters
watch() in the gap, and the snapshot captures the post-write mtime.
When the queued FSEvent finally lands, `has_mtime_changed` sees
`current_mtime == baseline` and silently suppresses the event,
producing rstest's "No test files need re-run" failure on PR #1261.

Adopt the watchpack model: mtime baselines are lifetime-tied to
registered files, not to individual watch() calls.

  * `PathManager::reset()` no longer clears `file_mtimes`; only the
    per-cycle added/removed diff sets are reset.
  * `PathManager::update()` now removes mtime entries for files that
    have been unregistered this cycle, so the map cannot grow
    unboundedly across rebuilds.
  * `set_file_mtime` is replaced with `set_file_mtime_if_absent`
    (entry-or-insert), and `wait_for_event` calls
    `record_initial_file_mtimes` which only stats files that have no
    baseline yet. Existing baselines are advanced atomically by
    `has_mtime_changed` itself when a real change is observed.

This keeps the original "filter stale FSEvent replays" semantics for
the first watch() call after registration -- exactly when stale
replays from the per-path ring buffer can fire -- while making
subsequent watch() calls a no-op for already-tracked files, so an
in-flight user write can never be captured as a baseline.

Verified locally on macos-14 node24 against the failing watch e2e from
rstest PR #1261.
@github-actions

github-actions Bot commented May 18, 2026

Copy link
Copy Markdown
Contributor

📦 Binary Size-limit

Comparing ef6e212 to chore(deps): update patch npm dependencies (#14132) by renovate[bot]

🙈 Size remains the same at 61.75MB

@github-actions

github-actions Bot commented May 18, 2026

Copy link
Copy Markdown
Contributor

Rsdoctor Bundle Diff Analysis

Found 5 projects in monorepo, 5 projects with changes.

📊 Quick Summary
Project Total Size Change
popular-libs 1.7 MB -
react-10k 5.7 MB -
react-5k 2.7 MB -
react-1k 827.2 KB -
ui-components 4.8 MB -
📋 Detailed Reports (Click to expand)

📁 popular-libs

Path: ../build-tools-performance/cases/popular-libs/dist/rsdoctor-data.json

⚠️ No baseline data found - Unable to perform comparison analysis

Metric Current Baseline Change
📊 Total Size 1.7 MB - -
📄 JavaScript 1.7 MB - -
🎨 CSS 0 B - -
🌐 HTML 289.0 B - -
📁 Other Assets 0 B - -

📁 react-10k

Path: ../build-tools-performance/cases/react-10k/dist/rsdoctor-data.json

⚠️ No baseline data found - Unable to perform comparison analysis

Metric Current Baseline Change
📊 Total Size 5.7 MB - -
📄 JavaScript 5.7 MB - -
🎨 CSS 21.0 B - -
🌐 HTML 328.0 B - -
📁 Other Assets 0 B - -

📁 react-5k

Path: ../build-tools-performance/cases/react-5k/dist/rsdoctor-data.json

⚠️ No baseline data found - Unable to perform comparison analysis

Metric Current Baseline Change
📊 Total Size 2.7 MB - -
📄 JavaScript 2.7 MB - -
🎨 CSS 21.0 B - -
🌐 HTML 328.0 B - -
📁 Other Assets 0 B - -

📁 react-1k

Path: ../build-tools-performance/cases/react-1k/dist/rsdoctor-data.json

⚠️ No baseline data found - Unable to perform comparison analysis

Metric Current Baseline Change
📊 Total Size 827.2 KB - -
📄 JavaScript 826.9 KB - -
🎨 CSS 0 B - -
🌐 HTML 328.0 B - -
📁 Other Assets 0 B - -

📁 ui-components

Path: ../build-tools-performance/cases/ui-components/dist/rsdoctor-data.json

⚠️ No baseline data found - Unable to perform comparison analysis

Metric Current Baseline Change
📊 Total Size 4.8 MB - -
📄 JavaScript 4.7 MB - -
🎨 CSS 107.0 KB - -
🌐 HTML 328.0 B - -
📁 Other Assets 0 B - -

Generated by Rsdoctor GitHub Action

@codspeed-hq

codspeed-hq Bot commented May 18, 2026

Copy link
Copy Markdown
Contributor

Merging this PR will not alter performance

✅ 37 untouched benchmarks
⏩ 25 skipped benchmarks1


Comparing fix/fsevents-mtime-race (ef6e212) with main (c93016a)2

Open in CodSpeed

Footnotes

  1. 25 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

  2. No successful run was found on main (4501c21) during the generation of this report, so c93016a was used instead as the comparison base. There might be some changes unrelated to this pull request in this report.

@stormslowly stormslowly marked this pull request as ready for review May 25, 2026 01:18
Copilot AI review requested due to automatic review settings May 25, 2026 01:18

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adjusts rspack_watcher’s mtime baseline lifecycle so registered files keep their baseline across repeated FsWatcher::watch() calls, preventing delayed macOS FSEvents from being incorrectly suppressed after rebuild/rewatch cycles.

Changes:

  • Keeps file_mtimes across PathManager::reset() and prunes entries only when files are removed.
  • Replaces unconditional mtime insertion with set_file_mtime_if_absent.
  • Renames and documents initial mtime recording in FsWatcher.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
crates/rspack_watcher/src/paths.rs Makes mtime baselines persistent across watch cycles and cleans them up for removed files.
crates/rspack_watcher/src/lib.rs Updates initial mtime recording to preserve existing baselines across rewatch cycles.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread crates/rspack_watcher/src/lib.rs
Comment thread crates/rspack_watcher/src/lib.rs
@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented May 25, 2026

Copy link
Copy Markdown

Deploying rspack with  Cloudflare Pages  Cloudflare Pages

Latest commit: fad4f00
Status: ✅  Deploy successful!
Preview URL: https://d7d6bb68.rspack-v2.pages.dev
Branch Preview URL: https://fix-fsevents-mtime-race.rspack-v2.pages.dev

View logs

…ycles

Regression for the FSEvents stale-event race fixed in this PR. Walks
the lifecycle a single `PathManager` sees during rspack's
aggregate -> pause -> rebuild -> rewatch loop, with a real file write
landing between the two `watch()` calls -- the slow-runner case where
the FSEvent is queued in the kernel but not yet delivered when the
second cycle starts.

Asserts the three invariants the fix introduces:

  1. `file_mtimes` survives `reset()`.
  2. `set_file_mtime_if_absent` is a no-op for an already-baselined
     path, so the post-write mtime can't overwrite the original.
  3. The delayed change event is therefore not suppressed by
     `has_mtime_changed`, and the baseline advances atomically.
…ile_mtimes

Iterate `files.added` instead of `files.all` so the loop becomes a no-op
in steady state (no add/remove churn this cycle) instead of issuing
O(N) `metadata()` syscalls on every rebuild rewatch.

The previous behavior was correct (`set_file_mtime_if_absent` discarded
the post-write mtime via entry-or-insert) but expensive on large
projects, and contradicted the PR's stated invariant "only stat files
that do NOT already have a baseline".

Side effect: this also strengthens the race fix. Post-write mtimes
no longer reach `set_file_mtime_if_absent` for persisted files at all,
so the FSEvents stale-event race remains closed even if the or-insert
contract is accidentally weakened in the future.

Addresses review feedback on #14077.
stormslowly added a commit to stormslowly/rstest that referenced this pull request May 25, 2026
Picks up the final fix for the FSEvents mtime baseline race + the
Option 2 perf follow-up (record_initial_file_mtimes now only stats
files added in this watch cycle, not the full registered set).

Tracks web-infra-dev/rspack#14077.
@stormslowly stormslowly requested a review from hardfist May 25, 2026 07:44
@stormslowly stormslowly merged commit 72a8c48 into main May 26, 2026
59 checks passed
@stormslowly stormslowly deleted the fix/fsevents-mtime-race branch May 26, 2026 03:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants