Skip to content

fix(cache): replay missing_dependency tracking in cached_node_modules warm path#236

Open
stormslowly wants to merge 2 commits into
mainfrom
fix/cached-node-modules-missing-dependency
Open

fix(cache): replay missing_dependency tracking in cached_node_modules warm path#236
stormslowly wants to merge 2 commits into
mainfrom
fix/cached-node-modules-missing-dependency

Conversation

@stormslowly
Copy link
Copy Markdown
Collaborator

@stormslowly stormslowly commented May 24, 2026

Why

PR #224 added early-return cache-hit guards to four CachedPath hot accessors. The package_json warm path correctly replays ctx.add_file_dependency / ctx.add_missing_dependency on cache hits, but cached_node_modules was missed.

On the cold path, cached_node_modulesmodule_directoryis_dir calls ctx.add_missing_dependency(node_modules_path) when the directory doesn't exist. The early-return skipped this, so webpack/rspack file watchers wouldn't receive the signal to rebuild when a node_modules directory was later created.

What

  • In the None branch of the warm-cache early-return, call ctx.add_missing_dependency(self.path.join("node_modules")) — mirroring what package_json already does.
  • Add a regression test that resolves the same specifier twice (cold then warm) and asserts missing_dependencies is identical on both calls, with a spot-check against the two absent node_modules paths that enhanced-resolve explicitly tracks.

Follow-up

A related cold-path divergence from enhanced-resolve was uncovered while validating this fix: is_dir does not record a missing-dep when the path exists as a non-directory (e.g., a regular file named node_modules). The warm path here happens to handle that case correctly because the OnceLock<Option<CachedPath>> folds "absent" and "non-dir" into the same None. Tracking the cold-path fix separately in #239.

…warm hit

When node_modules doesn't exist, the cold path adds the directory to
ctx.missing_dependencies via module_directory -> is_dir. The warm cache
early-return skipped this tracking, so webpack/rspack watchers would
miss the signal to rebuild when a node_modules directory was later created.

Mirrors the same replay pattern already applied to package_json in PR #224.
Copilot AI review requested due to automatic review settings May 24, 2026 09:07
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

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 fixes ResolveContext::missing_dependencies tracking on warm-cache hits for cached_node_modules, ensuring watcher-integrations (webpack/rspack) still receive “missing node_modules directory” signals after the resolver cache is populated.

Changes:

  • Replay ctx.add_missing_dependency(.../node_modules) on cached_node_modules cache hits when the cached result is None.
  • Add a regression test asserting cold vs warm resolves produce identical missing_dependencies, including absent traversed node_modules directories.

Reviewed changes

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

File Description
src/cache.rs Replays missing-dependency tracking on cached_node_modules warm-cache hits.
src/tests/dependencies.rs Adds a regression test comparing cold vs warm missing_dependencies for absent node_modules dirs.

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

Comment thread src/cache.rs
Comment thread src/cache.rs
@codspeed-hq
Copy link
Copy Markdown

codspeed-hq Bot commented May 24, 2026

Merging this PR will not alter performance

✅ 12 untouched benchmarks


Comparing fix/cached-node-modules-missing-dependency (9cf97ca) with main (c7167a1)

Open in CodSpeed

@stormslowly
Copy link
Copy Markdown
Collaborator Author

@codspeedbot explain why this is faster

@codspeed-hq
Copy link
Copy Markdown

codspeed-hq Bot commented May 25, 2026

@codspeedbot explain why this is faster

Based on my thorough flamegraph analysis, here's the explanation for the user. Since this is a comment response (no code changes needed), I'll provide the answer directly:

Analysis Summary

After analyzing the CodSpeed flamegraphs for both the HEAD and BASE runs, comparing call trees, instruction counts, and the PR's code diff, here's what's happening:

The 4.13% improvement is not directly caused by this PR's correctness fix. It's primarily attributable to PR #237 (skip Box::pin on realpath cache hit) which was merged into main and then flowed into this branch via the merge commit.

Evidence from the flamegraphs

Metric Before merge with main After merge with main (HEAD)
Benchmark total 58.1 ms 56.9 ms
realpath::{{closure}} self time 1.4 ms (2.33%) 311 µs (0.55%)
_strip_prefix path operations 2.1 ms (3.64%) Not present on hot path
metadata calls 15 10

PR #237 restructured the realpath code path so that cache hits return immediately without constructing a Box::pin future or running path normalization (_strip_prefix, Components::next). This eliminated ~3.3 ms of overhead in the realpath sub-tree on the multi-threaded benchmark.

When comparing the two runs on this PR branch (before and after the merge with main), CodSpeed marks all 12 benchmarks as unchanged — confirming the improvement comes from the merge, not from this PR's add_missing_dependency fix.

What this PR actually does

This PR's change — calling ctx.add_missing_dependency(self.path.join("node_modules")) on the cached_node_modules warm path — is a correctness fix with negligible performance impact. It adds a single path join + set insertion to ensure webpack/rspack watchers correctly track absent node_modules directories across cached resolutions. The CodSpeed baseline (59.3 ms) still reflected pre-#237 performance from the recent main branch history, which is why the comparison showed an improvement.

Summary
  • Warming up the performance wizard...
  • Analyzing performance improvement — Flamegraph analysis complete — preparing explanation
  • Handling comment — Comment handling finished successfully

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.

2 participants