GeezerVison

Review requested:

• @nodejs/loaders

force pushing to add the missing : "Signed-off-by:" in the commit message

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.05%. Comparing base (761e49e) to head (4d39a40).
⚠️ Report is 12 commits behind head on main.

@@            Coverage Diff             @@
##             main   #64682      +/-   ##
==========================================
- Coverage   90.06%   90.05%   -0.02%     
==========================================
  Files         769      769              
  Lines      261316   261341      +25     
  Branches    49625    49627       +2     
==========================================
- Hits       235367   235344      -23     
- Misses      16996    17035      +39     
- Partials     8953     8962       +9     

This was the behaviour in days of yore (#36638) so this is effectively a reversion of that change, hence the failing tests.

One would expect that the impact of the scenario described here is far more common than the need to modify the module tree on-the-fly after a failed require(), which was the motivating case for that change. However, this PR does reintroduce the edge case where the following will fail on both loads, as opposed to the current behaviour where the second require() picks up the new module:

const jsonModule = path.resolve('./test.json')

try {
  const m = require(jsonModule)
  console.debug(m)
} catch (e) {
  console.error(e)
}

fs.writeFileSync(jsonModule, '{}\n')

try {
  const m = require(jsonModule)
  console.debug(m)
} catch (e) {
  console.error(e)
}

FWIW, there aren't any module benchmarks which really capture the scenario of repeated attempts at resolving the same module specifier(s) and/or having to walk several levels up the directory tree, which would probably help make the case for this PR. The original change was accepted in the context of a neutral impact on the module benchmark suite at the time.

the behavior change here is that a path missing on first probe is now invisible for the rest of the require tree. before this, negative results were never cached, so a file created mid-tree (codegen that writes then requires, some transpile-on-demand setups) would be picked up on a later require in the same tree — now it serves the cached miss. the test actually locks that new behavior in. is that regression considered acceptable, or should negative caching be gated so only the "clearly hot" probes (tryExtensions / node_modules walk) cache, not arbitrary require targets?

Thanks both! I've reworked the PR.

On the regression: All the CI failures were the same test, parallel/test-module-cache, i.e. exactly the behaviour #36642 locked in. I've scoped the negative caching instead of applying it everywhere, which is close to what @Sanjays2402 suggested:

• Negative results are cached only for speculative probes: paths that resolution guesses at rather than paths the user named: the extension candidates in tryExtensions, and the node_modules ancestor directories walked for bare specifiers.
• A cached negative is also only read back by a speculative probe, so a stat of a user-named path always re-stats.

So create-then-require in the same tree still works, and test-module-cache.js passes unmodified. The negative cache only covers paths the user never asked for, where create-mid-tree isn't a meaningful pattern.

On the benchmark: Added benchmark/module/module-resolve-misses.js. A module nested depth levels down requires deps distinct bare specifiers: each walk re-probes the same missing node_modules ancestors.

(higher is better; 10 runs each)

Failed statx calls, same workload at depth 12: 3000 → 612 (−80%), and the saving scales with depth, as the mechanism predicts.

@Renegade334 on the test.json example you posted: with this version the second require() picks up the new file, since that's a user-named path.

Let me know!

Also, now that the previous tests are passing, I'm not sure this is a breaking change anymore. Should we remove the semver-major tag? Thanks!

FWIW, I get a modest but significant performance improvement on the new benchmark, albeit tested on a disk that's not under load, but only at the higher depth.

                                                        confidence improvement accuracy (*)   (**)  (***)
module/module-resolve-misses.js n=30 deps=200 depth=12        ***      3.05 %       ±1.33% ±1.77% ±2.30%
module/module-resolve-misses.js n=30 deps=200 depth=4                 -0.67 %       ±1.31% ±1.74% ±2.27%

Thanks @Renegade334
I've noticed you added a new label about benchmark CI, is it something I need to do on my end? or does the benchmark/module/module-resolve-misses.js file I provided is enough?
Let me know! Thanks

@jasnell let me know if you have more comments or if you're happy with the changes I've made after your review

cc @nodejs/loaders for review.

I'll leave for someone else to make a verdict on semver, dynamic loaders like PnP do some fairly eclectic things on-the-fly, and at this point I'm pretty sure anything that touches module resolution has the potential to xkcd1172 something 😆

Benchmarking CI https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1895/