fix: delete no longer referenced files from the filesystem cache directory - #21528
Conversation
🦋 Changeset detectedLatest commit: c310c1a The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
This PR is packaged and the instant preview is available (964bbfa). Install it locally:
npm i -D webpack@https://pkg.pr.new/webpack@964bbfa
yarn add -D webpack@https://pkg.pr.new/webpack@964bbfa
pnpm add -D webpack@https://pkg.pr.new/webpack@964bbfa |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #21528 +/- ##
==========================================
+ Coverage 93.79% 93.81% +0.02%
==========================================
Files 620 620
Lines 73798 73504 -294
Branches 21306 21251 -55
==========================================
- Hits 69220 68961 -259
+ Misses 4578 4543 -35
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Merging this PR will regress 2 benchmarks
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | Memory | benchmark "many-modules-esm", scenario '{"name":"mode-production","mode":"production"}' |
7.5 MB | 9.6 MB | -21.86% |
| ❌ | Memory | benchmark "many-chunks-commonjs", scenario '{"name":"mode-production","mode":"production"}' |
8 MB | 10.1 MB | -21.14% |
| ⚡ | Memory | benchmark "devtool-eval", scenario '{"name":"mode-development-rebuild","mode":"development","watch":true}' |
3.1 MB | 1.3 MB | ×2.4 |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing fix/cache-cleanup-unused-files (c310c1a) with main (ec39088)
PR SummaryMedium Risk Overview Serialization now records which pack segments were written in the current run and which were retained via existing lazy pointers (without re-writing them). Cleanup ( Reviewed by Cursor Bugbot for commit 00002f0. Bugbot is set up for automated code reviews on this repo. Configure here. |
alexander-akait
left a comment
There was a problem hiding this comment.
Reviewed the cache-cleanup logic end to end (serialize instrumentation, the on-disk reference walker, and _cleanupUnusedFiles). The design is careful and fail-safe by construction — retained packs are correctly detected via their existing lazy pointer (getLazySerializedValue), the walk is transitive and memoized, and walk/readdir errors abort the whole cleanup. I could not find a data-loss or crash bug in the common paths. Two observations below.
1. Compressed reference walker can under-report silently (defense-in-depth). See the inline comment on parsePointerNames: unlike the uncompressed path, it doesn't bounds-check sections, so an internally-inconsistent-but-decompressable file yields an incomplete name list without erroring — which can let _cleanupUnusedFiles delete a still-referenced pack. Narrow window, but it breaks the "never delete with an incomplete live set" invariant for that path. Fix suggestion is in the inline comment.
2. Grace period vs. cache-restore mtime (question, not a blocker). The 30-min grace period keys off mtimeMs. If a CI cache restore refreshes file mtimes to "now" (some restore mechanisms do; tar-based ones preserve them), orphans in a restored cache look fresh, so a build shorter than 30 min would never age them past the grace period and the directory would keep growing across runs — the exact scenario #13291 is about. The code comment shows you considered this; could you confirm the intended behavior for sub-30-min CI builds that restore the cache each run? If restore preserves mtimes this is a non-issue.
Non-blocking notes:
- Version bump —
patchis correct. No schema/config/public-API change; this is a behavioral bug fix for unbounded cache growth (#13291), and it's not breaking (only files unreachable from the just-stored index are removed;unlink-less filesystems skip cleanup). It's notminor(no new capability/option) normajor(no breaking change). - CodSpeed memory regression looks like noise. The flagged benchmark (
many-modules-commonjs, production) configures no filesystem cache, so none of this PR's code runs in it; combined with CodSpeed's own "different runtime environments"/"7 commits behind" warnings, the 8.5→11.3 MB delta almost certainly isn't from this change. Worth a re-run / acknowledge rather than a code change.
Generated by Claude Code
| const length = buf.readInt32LE(8 + i * 4); | ||
| if (length < 0) { | ||
| // pointer section: u64 size + utf-8 file name | ||
| names.push(buf.toString("utf8", offset + 8, offset - length)); |
There was a problem hiding this comment.
Compressed walker can silently under-report references → risk of deleting a still-referenced pack.
parsePointerNames walks the section table but never validates that a section stays inside buf, whereas getReferencedFilenamesUncompressed does (it fail()s on EOF / oversize reads). So the two paths diverge on a file that decompresses cleanly but whose section header is internally inconsistent (e.g. a positive content length larger than the actual bytes present): the running offset drifts past the end, and here buf.toString("utf8", offset + 8, offset - length) just clamps to the buffer end and pushes "" instead of the real name — no error is thrown.
Reproduced against this exact function (2 sections: a content length that lies + one real pointer to REFERENCED-FILE):
compressed -> [""] // real reference MISSED, no throw
uncompressed -> throws "Unexpected end of file" // fail-safe, aborts cleanup
Because _cleanupUnusedFiles deletes every file not in the live set, an under-reported list from a retained compressed pack can drop a genuinely-referenced file from liveFiles and unlink it once it's past the grace period. That contradicts the PR's own invariant ("Any walk error aborts the whole cleanup — never delete with an incomplete live set"): for the compressed path the walk can produce an incomplete set without erroring.
It's a narrow window (the compression layer's own integrity checks make internal inconsistency unlikely), but given the whole feature is about the safety of deletion it's worth closing. Suggestion: make parsePointerNames validate as it walks and throw on any inconsistency, mirroring the uncompressed path — e.g. reject when offset would move past buf.length, when a pointer's end (offset - length) exceeds buf.length, or when the final offset !== buf.length. Then a corrupt compressed file aborts the cleanup instead of quietly shrinking the live set.
A second, lower-severity note on the same walker: MAX_HEADER_OR_POINTER_SIZE is applied to the section-lengths read (readAt(8, sectionCount * 4)) in the uncompressed path, so a file with a genuinely huge sectionCount is rejected as Invalid section size — fail-safe, but the message is misleading; a dedicated sectionCount sanity check would read clearer.
Generated by Claude Code
alexander-akait
left a comment
There was a problem hiding this comment.
Posted the concrete fix for the compressed-walker under-reporting issue as an applicable suggestion on parsePointerNames (click "Apply suggestion" to commit it). It adds bounds validation so a corrupt-but-decompressable file aborts the cleanup instead of quietly handing back an incomplete live set — upholding the "never delete with an incomplete live set" invariant on the compressed path too.
Generated by Claude Code
|
@bjohansebas let’s fix and we can merge |
alexander-akait
left a comment
There was a problem hiding this comment.
Addressed the Cursor Bugbot finding ("Uncompressed walker skips size check") with an applicable suggestion on getReferencedFilenamesUncompressed. It's the mirror of the compressed-path fix that already landed: verify the section table sums to exactly the file size before trusting pointer offsets, so the default (no-compression) path also fails safe instead of under-reporting the live set. Click "Apply suggestion" to commit it.
Generated by Claude Code
alexander-akait
left a comment
There was a problem hiding this comment.
The size-check I suggested earlier introduced the two CI failures — here are the fixes as applicable suggestions:
lint(Prettier): the inline-castif (…)line runs ~87 cols over the 80printWidth. Suggestion 1 extracts the cast to a local (76 cols).basic/ unit tests:getReferencedFilenamesUncompressednow callsfs.stat, but thefsWithFilemock has nostat, so every uncompressed case throwsfs.stat is not a function. Suggestion 2 adds astatreturning the buffer's real size (equal to the section-table total for a valid file, so existing cases stay green).codecov/patch: Suggestion 3 adds two cases covering the new size-mismatch and stat-error branches.
Apply all three, then a yarn lint / yarn test:unit locally should come back clean. I couldn't run the suite in my environment to fully verify formatting (dependency install is blocked here), so a quick yarn fix before pushing will catch any stray Prettier nit.
Generated by Claude Code
alexander-akait
left a comment
There was a problem hiding this comment.
The lint failure this round is lint:types-test (tsc), not Prettier: the two new coverage tests assign fs.stat = (…) where fs is typed IntermediateFileSystem, so _file: string isn't assignable to Stat's PathLike param (TS2322 at lines 227 and 239). The two suggestions cast each fs handle to EXPECTED_ANY — the same pattern fsWithFile and the "rejects on read errors" test already use — which clears both errors without touching the assertions. unit, basic, codecov/patch, and integration are all green on this commit.
I'll follow up separately on the new Bugbot review (the stale-memo finding).
Generated by Claude Code
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit fd01272. Configure here.
Restore the missing content binding and drop the duplicated fs declaration that a mis-anchored suggestion left in the stat-error case.
eaba921 to
c310c1a
Compare
Summary
Fixes #13291. The filesystem cache directory grows without bound because files orphaned by cache invalidations (version/config changes, compression changes, stale
index.pack.old) are never removed — up to 27 GB reported on CI. After a successful store, the strategy now deletes files that are no longer reachable from the stored index: it collects written and retained file names during serialization (decoding retained lazy pointers, the part that blocked #14661), walks retained files on disk for nested references (header + pointer ranges only, memoized across stores), and unlinks unreferenced files older than a 30-minute grace period so concurrent builds sharing the directory are never disturbed. Any walk error aborts the whole cleanup (never delete with an incomplete live set), and the cleanup runs in the idle store phase, off the build's critical path (~100 µs per deleted file, one-time; build times unchanged in benchmarks).What kind of change does this PR introduce?
fix
Did you add tests for your changes?
Yes — integration cases in
test/PersistentCaching.test.js(orphan deleted, fresh files kept, cache restores fully afterwards), direct strategy tests intest/PackFileCacheStrategyCleanup.test.js(nested retained walk, grace period, memoization/invalidation), and walker unit tests intest/FileMiddleware.unittest.js(pointer parsing, compression variants, truncated/corrupt files); all verified via mutation testing to fail when the fix is removed.Does this PR introduce a breaking change?
No — only files unreachable from the just-stored cache index are deleted, so no valid cache is ever invalidated; filesystems without
unlinkskip the cleanup entirely. Known limitation: compressed packs > 2 GB abort the cleanup with a warning (uncompressed packs are read by byte range and have no such limit).If relevant, what needs to be documented once your changes are merged or what have you already documented?
n/a
Use of AI
This PR was developed with the assistance of Claude Code (Anthropic): investigation of the issue and prior PRs (#13875, #14661), implementation, tests, benchmarks, and mutation-testing verification were done by the AI under my direction and review; I validated the approach and results.
Note
Medium Risk
Touches persistent cache on disk and concurrent shared cache dirs; mistakes could delete still-needed packs, though abort-on-error and the grace period mitigate that.
Overview
Fixes unbounded growth of the filesystem cache directory by deleting pack files that are no longer reachable from the index after a successful store.
PackFileCacheStrategynow runs post-store cleanup when the filesystem supportsunlink: it builds a live file set from the new index, files written in that store, and retained lazy segments (plus a BFS walk of nested references viagetReferencedFilenames). Anything else in the cache dir that is a regular file and older than a 30-minute grace period is unlinked. Walk/parse failures abort the whole cleanup (warning only, no partial deletes); failed stores clear the reference memo.FileMiddlewarerecordswrittenFiles/retainedFilesduring serialization (including names from reused lazy pointer buffers) and exportsgetReferencedFilenamesto read pointer targets from pack headers without full deserialization (byte-range reads for uncompressed packs; full decompress for.gz/.br/.zst, with corrupt-file guards).Integration and strategy tests cover orphans, grace period, nested retention, memoization, concurrent in-place rewrites, and entry-churn pack expiry.
Reviewed by Cursor Bugbot for commit c310c1a. Bugbot is set up for automated code reviews on this repo. Configure here.