Skip to content

fix(cache): fingerprint only whether files in node_modules/.bin exist - #824

Merged
wan9chi merged 1 commit into
claude/modules-yaml-issues-604e1ffrom
claude/node-modules-bin-existence
Oct 12, 2026
Merged

wan9chi merged 1 commit into
claude/modules-yaml-issues-604e1ffrom
claude/node-modules-bin-existence

Conversation

@wan9chi

@wan9chi wan9chi commented Oct 9, 2026

Copy link
Copy Markdown
Member

Motivation

pnpm's shims in node_modules/.bin embed the absolute workspace path, in NODE_PATH and in a trailing # cmd-shim-target= comment. Almost every task runs a dependency's executable through one, so installing with the workspace at another path, such as another clone or a CI checkout, made those tasks miss with 'node_modules/.bin/tsc' modified.

Changes

  • Files directly inside a node_modules/.bin directory are now fingerprinted only by whether they exist. A missing shim still differs from an existing one.
  • What a shim runs is still tracked through the package files the task reads. The shim's exec target is a relative path, and only pnpm reads cmd-shim-target.
  • This doesn't need the workspace root or the Windows path variants (C:\ws, C:/ws, /mnt/c/ws) that rewriting the absolute paths in place would. It also covers .cmd and .ps1 shims, pnpm's global virtual store, and npm/Yarn symlinks.
  • Trade-off: with pnpm's global virtual store, the dependency version in NODE_PATH was an incidental signal that a dependency changed, since reads in the store are outside the workspace and untracked. Tracking store reads properly is a separate problem.

🤖 Generated with Claude Code

@wan9chi
wan9chi added this pull request to stack #826 October 9, 2026 06:06
@wan9chi
wan9chi force-pushed the claude/node-modules-bin-existence branch from dc4a249 to e93f942 Compare October 9, 2026 06:08
@github-actions

github-actions Bot commented Oct 9, 2026 •

Copy link
Copy Markdown

fspy benchmark

linux

dynamic/launch             change  +4.75%  [-14.77% .. +24.37%]  overhead   +62.75%
dynamic/access             change  +0.26%  [ -5.47% .. +13.49%]  overhead    +8.46%
dynamic/access-relative    change  +0.44%  [-10.78% ..  +8.74%]  overhead   +45.26%
dynamic/access-contended   change  +3.07%  [ -3.41% ..  +8.35%]  overhead    +9.30%
static/launch              change  +1.95%  [-22.13% .. +37.09%]  overhead  +155.48%
static/access              change  +4.82%  [-17.50% .. +55.47%]  overhead +1489.50%
static/access-relative     change  +0.32%  [-21.54% .. +26.43%]  overhead +1473.56%
static/access-contended    change  +4.35%  [ -7.84% .. +18.26%]  overhead +3567.39%

macos

dynamic/launch             change  -0.72%  [ -3.75% ..  +2.71%]  overhead  +216.72%
dynamic/access             change  -0.36%  [ -2.27% ..  +1.95%]  overhead    +2.98%
dynamic/access-relative    change  +0.28%  [ -2.81% ..  +3.65%]  overhead  +265.78%
dynamic/access-contended   change  +0.37%  [ -3.55% ..  +3.29%]  overhead    +3.94%

windows

dynamic/launch             change  +0.27%  [ -6.19% ..  +5.06%]  overhead   +25.85%
dynamic/access             change  +0.00%  [ -9.56% ..  +4.68%]  overhead    +0.90%
dynamic/access-relative    change  +0.35%  [ -2.07% .. +20.57%]  overhead    +1.60%
dynamic/access-contended   change  +1.37%  [ -3.89% .. +23.31%]  overhead    +4.68%

@wan9chi
wan9chi force-pushed the claude/node-modules-bin-existence branch from e93f942 to 5421bbe Compare October 12, 2026 04:32
@wan9chi
wan9chi force-pushed the claude/node-modules-bin-existence branch from 5421bbe to 39ce61e Compare October 12, 2026 05:26
wan9chi added a commit that referenced this pull request Oct 12, 2026
## Motivation


`vitest_browser_cache::vitest_browser_caches_inputs_and_restores_outputs`
fails intermittently on Windows runners in the `Test (windows-ignored)`
job:

```
Error: listen EACCES: permission denied ::1:63315
```

63315 is the fixed default port for Vitest browser mode's server.
Windows reserves dynamic port ranges (Hyper-V/WinNAT excluded port
ranges), and listening on a reserved port fails with `EACCES`. Vite only
tries another port on `EADDRINUSE`, so the run fails, `dist/result.json`
is never written, and the snapshot diverges. It depends on which runner
the job lands on: it failed on #824 and #825 and passed on other runs of
the same code.

## Changes

The fixture's browser server now listens on port 0, so the OS chooses a
free port. Vite supports `server.listen(0)` directly, but the config
can't request it the normal way:

- `test.api: { port: 0 }`: Vitest only copies `api.port` when it is
truthy, so `0` falls back to 63315.
- Setting `test.api` at all, even with a concrete port, adds an `API
started at http://localhost:<port>/` banner line that would change the
snapshot. `test.browser.api` prints a deprecation warning in Vitest 5.

Instead, a small `enforce: 'post'` plugin sets `test.api.port = 0` after
Vitest resolves `test.api`. The banner stays unchanged, and the snapshot
is identical.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
pnpm's bin shims embed the absolute workspace path in `NODE_PATH` and a
`cmd-shim-target` comment, so any task that runs a dependency's executable
missed after installing with the workspace at another path. What a shim
runs is tracked through the package files the task reads, so the content
of files directly inside `node_modules/.bin` is no longer hashed.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@wan9chi
wan9chi force-pushed the claude/node-modules-bin-existence branch from 39ce61e to ec0eba1 Compare October 12, 2026 05:33
@wan9chi
wan9chi merged commit d7bde1a into main Oct 12, 2026
20 checks passed
@wan9chi
wan9chi deleted the claude/node-modules-bin-existence branch October 12, 2026 06:31
wan9chi added a commit that referenced this pull request Oct 13, 2026
#850)

## Motivation

#823 hashes pnpm's `node_modules/.modules.yaml` without `prunedAt`,
`storeDir`, `virtualStoreDir` and `linkedSkills`. What's left protects
little and still causes needless misses:

- Every cached `vite build` and `vitest` task reads the file because
Vite reads it while resolving its config
([`packages/vite/src/node/server/index.ts`](https://github.com/vitejs/vite/blob/dce9fe69d37e957b62f3b5df61a761f7f2f949f5/packages/vite/src/node/server/index.ts#L1332-L1361)).
Vite reads only `virtualStoreDir`, to add pnpm's global virtual store to
the dev server's `server.fs.allow`, inside a try/catch for non-pnpm
projects. `virtualStoreDir` is already left out of the hash, so the hash
protects nothing Vite uses, and for `vite build` the file doesn't affect
the output.
- The other fields describe `node_modules`, which tasks already read. In
pnpm 11 they are `hoistedDependencies`, `hoistPattern`,
`publicHoistPattern`, `hoistedLocations`, `nodeLinker`, `included`,
`skipped`, `injectedDeps`, `layoutVersion`, `virtualStoreDirMaxLength`,
`pendingBuilds`, `ignoredBuilds`, `allowBuilds`, `packageManager` and
`virtualStoreOnly`. When they change in a way that matters, the files a
task reads in `node_modules` change too, and those reads are tracked.
- Some of the hashed fields still vary across machines, so tasks missed
with `'node_modules/.modules.yaml' modified`:
  - `packageManager`: the version of the pnpm that installed
  - `pendingBuilds`: filled after `pnpm install --ignore-scripts`
  - `virtualStoreOnly`: set after `pnpm fetch`
- `skipped`: differs between glibc and musl for platform packages under
the same cache key
- The list of fields to leave out has to track pnpm's format.

## Changes

- `node_modules/.modules.yaml` is now fingerprinted only by whether it
exists, the same treatment #824 gives files in `node_modules/.bin`. A
missing manifest still differs from an existing one, since tools detect
pnpm by the file existing (Vite's try/catch, which-pm).
- Trade-off: a task whose output depends on these fields without any
file it reads in `node_modules` differing no longer misses. The examples
#823 named are LavaMoat's allow-scripts check
(`pendingBuilds`/`ignoredBuilds`), which-pm's pnpm version, and Nx's
`hoistedDependencies`. These rarely run inside cached vp tasks, and in
realistic cases `node_modules` differs too.
- The JSON/YAML parsing, the canonical JSON writer and the fallback for
unparsable content are removed, along with `vt`'s `serde_norway`
dependency. Both files are hashed as empty content without being read,
which is the hash files in `node_modules/.bin` already had, so their
cache entries stay valid.
- The `pnpm_modules_manifest` e2e fixture now shows that changing the
manifest's content keeps the cache hit and removing it misses.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
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.

1 participant