fix(browser-cleaner): report Chromium caches the scan was silently dropping (#265) - #266
Conversation
…opping (#265) Chrome's Code Cache never appeared in a scan while Edge's did. Both are the same Chromium directory; the difference was that Edge wasn't being used. scanDirectory skips any entry modified in the last 60 minutes. Code Cache contains exactly two entries, `js` and `wasm`, whose mtimes move on every write, so any browser used within the hour had both skipped and the whole result discarded for having no items. Browser caches are regenerated from scratch by the browser and a locked file still fails its delete safely as 'in-use', so the guard is turned off for this category. Also adds the Chromium cache directories that were never scanned at all. Four of them sit beside the profiles rather than inside one, which needed chromiumCacheDirs to grow a `shared` list alongside the per-profile entries: profile: DawnWebGPUCache, DawnGraphiteCache shared: component_crx_cache, extensions_crx_cache, GrShaderCache, ShaderCache, GraphiteDawnCache, GPUPersistentCache, PnaclTranslationCache Newer builds moved the Dawn and GPU caches up to the user-data level, so both spellings are listed; missing directories are skipped as before. The browser cleaner IPC, the CLI, and the cloud agent each kept their own copy of the browser list and cache-dir loop — which is how these directories stayed missing from all three. They now share services/chromium-cache.
🧹 Rules BotChanges
Validation ✅All rules are valid Full validation output |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 51256c8551
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| * directories from scratch, and a locked file still fails the delete safely as | ||
| * 'in-use', so there is nothing left for the guard to protect here. | ||
| */ | ||
| export const BROWSER_CACHE_SKIP_RECENT_MINUTES = 0 |
There was a problem hiding this comment.
On macOS and Linux, when a browser is still running, open files can generally be unlinked rather than causing safeDelete to return in-use, so setting this to zero exposes actively written cache entries to deletion; with secure deletion enabled, Kudu can even overwrite those files before unlinking them. This affects manual cleans because closeBrowsersBeforeClean defaults to false, as well as CLI and cloud cleans that do not close browsers, and can corrupt an active browser's cache or disrupt the process. Disable the guard only after the relevant browser has been closed, or retain a nonzero cutoff for these flows.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch, and you're right — my justification was Windows-only. safeDelete returning in-use depends on Windows' mandatory locking; on macOS and Linux the unlink just succeeds, and with secureDelete on, secureOverwrite writes over the file in place before unlinking. For the cache block files a running browser keeps memory-mapped, that is a real hazard rather than just a cold cache.
Fixed in 797547a by scoping the guard instead of switching it off.
The actual defect was applying a file-recency heuristic at directory granularity. A directory's mtime moves whenever an entry is added or removed inside it, which says nothing about whether its contents are in use — and skipping it throws away the entire subtree. That is precisely why Chrome's Code Cache vanished: it holds exactly two entries, the js and wasm directories.
So directories are now exempt from the guard and files are not:
if (stats.mtimeMs > cutoff && !(filesOnly && isDirectory)) continuedata_0–data_3 and index are files, so they stay excluded exactly as before — the actively-written set you named keeps its full protection, and browser scans go back to the standard 60-minute cutoff rather than zero. This also happens to be the behaviour the issue reporter identified as correct, so the "Note for review" caveat in the PR description is gone.
Verified against a synthetic replica of the reported layout:
Chrome - Default Cache before: 98.00 MB (1 items) after: 98.00 MB (1 items)
Chrome - Default Code Cache before: 0.00 MB (0 items) after: 310.00 MB (2 items)
Follow-up to review feedback on #266. The previous commit disabled the guard outright for browser caches, justified by locked files failing their delete as 'in-use'. That reasoning only holds on Windows: on macOS and Linux an open file is unlinked happily, and with secure delete on, Kudu overwrites it in place first — which for the cache block files a running browser keeps memory-mapped could disrupt the browser rather than just cost it a cold cache. The guard is now scoped instead of switched off. A directory's mtime moves whenever an entry is added or removed inside it, which says nothing about whether its contents are in use, and skipping it discards the whole subtree — that is the actual defect behind Chrome's missing Code Cache, which holds exactly the `js` and `wasm` directories. Directories are exempt; files are not, so `data_0`-`data_3` and `index` stay out of a scan exactly as before, which is also the behaviour the issue reported as correct. scanDirectory's recency parameter now takes either the existing number or { skipRecentMinutes, filesOnly }, so the trash scans that pass 0 are untouched. Verified against a synthetic replica of the reported layout: Code Cache goes from 0 items to 310 MB across its two subdirectories, while Cache_Data stays at 98 MB with the hot block files still excluded.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 797547a50d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const isDirectory = stats.isDirectory() | ||
|
|
||
| if (stats.mtimeMs > cutoff) continue | ||
| if (stats.mtimeMs > cutoff && !(filesOnly && isDirectory)) continue |
There was a problem hiding this comment.
When a cache target contains directories such as Code Cache's js and wasm, filesOnly admits each directory as one ScanItem without inspecting descendant mtimes; cleaning that item then calls safeDelete, which recursively removes or securely overwrites every descendant, including files written within the last hour by a running browser. The fresh evidence beyond the earlier recency comment is that the replacement guard only checks top-level entries, so it still bypasses recency for all files nested under a directory; with browser-closing disabled by default, this can corrupt an active cache or disrupt the browser.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Right again — exempting directories fixed the reporting but not the delete. The directory went in as one ScanItem with nothing having looked at its descendants, and safeDelete then recursed through all of them.
Fixed in 7a5b221 by making the guard descend rather than exempt.
A directory now collapses into a single item only when nothing beneath it falls inside the cutoff — that is what makes the recursive delete which follows safe. When something under it is live, the directory is opened and its children judged the same way, so the settled space around a live file is still reclaimed without the live file ever being offered:
const children = await resolveChildren(path, ctx, depth - 1)
if (!children.complete) return children // something inside is live — hand back only what settled
return { items: [{ path, size: children.size, ... }], complete: true, size: children.size }The 60-minute cutoff is unchanged and now genuinely holds at every depth, where before it only ever looked at the top level. Two details worth flagging: symlinks are never descended into, since rm only unlinks the link and the check would otherwise be passing judgement on a tree this scan cannot remove; and depth is capped at 8, with a subtree that cannot be shown settled left alone rather than offered up.
Synthetic replica of the reported layout, run both ways:
### Chrome closed (all entries settled)
Chrome - Default Cache before: 98.00 MB (1) after: 98.00 MB (1)
-> \Default\Cache\Cache_Data\f_000001
Chrome - Default Code Cache before: 0.00 MB (0) after: 310.00 MB (2)
-> \Default\Code Cache\js
-> \Default\Code Cache\wasm
### Chrome running (js holds a live entry)
Chrome - Default Cache before: 98.00 MB (1) after: 98.00 MB (1)
-> \Default\Cache\Cache_Data\f_000001
Chrome - Default Code Cache before: 0.00 MB (0) after: 310.00 MB (2)
-> \Default\Code Cache\js\settled
-> \Default\Code Cache\wasm
In the running case js is no longer offered at all — only js/settled is, so js/live is unreachable from any item and can be neither unlinked nor overwritten. Cache_Data is unchanged in both, with the memory-mapped block files still excluded.
Covered by six tests in file-utils.recency.test.ts, including the exact case you describe (never offers a directory that still holds a live file) and a nested one proving the check is not single-level.
Follow-up to review feedback on #266. Exempting directories from the recency guard fixed the reporting but not the delete: a directory went in as a single scan item without anything checking its descendants, and safeDelete then recursed through every one of them — unlinking, and with secure delete on overwriting in place, files a running browser had written moments earlier. The guard now descends. A directory collapses into one item only when nothing beneath it falls inside the cutoff, which is what makes the recursive delete that follows safe. When something under it is live, the directory is opened and its children judged the same way, so the settled space around a live file is still reclaimed without the live file ever being offered. The cutoff itself is unchanged at 60 minutes and now genuinely holds at every depth, where before it only ever looked at the top level. Symlinks are never descended into, since rm only unlinks the link and would otherwise have the check pass judgement on a tree this scan cannot remove. Depth is capped at 8; a subtree that cannot be shown to be settled is left alone rather than offered up. Synthetic replica of the reported layout, both ways: Chrome closed Code Cache 0 items -> 310 MB across js and wasm Chrome running Code Cache 0 items -> 310 MB, js opened so that js/settled is offered and js/live is not Both Cache_Data 98 MB unchanged, block files still excluded
|
@codex review Both P1s from the earlier passes are addressed in 7a5b221, which has not been reviewed yet. The recency guard now descends: a directory is offered as a single item only when nothing beneath it falls inside the cutoff, otherwise it is opened and its children judged the same way. Please check in particular that no live descendant is reachable from any offered item. |
|
Codex Review: Didn't find any major issues. Another round soon, please! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Fixes #265.
Why Chrome's Code Cache never showed up
scanDirectoryskips any entry whose mtime falls insideskipRecentMinutes(default 60).Code Cachecontains exactly two entries — thejsandwasmdirectories — and a directory's mtime moves every time an entry is added or removed inside it. So for any browser used in the last hour both entries were skipped, the result came back with zero items, and the browser cleaner dropped it:Edge appeared in the reporter's scan only because Edge wasn't being used. Nothing about Chrome was special — same directory, same code, different usage pattern. Closing Chrome doesn't help either: mtimes don't rewind, which is exactly what the reporter observed.
The defect is that a directory was being judged by its own mtime, which says nothing about whether its contents are in use. It now gets judged by what is inside it. A directory collapses into a single scan item only when nothing beneath it falls inside the cutoff — which is what makes the recursive delete that follows safe. When something under it is live, the directory is opened and its children judged the same way, so the settled space around a live file is still reclaimed without the live file ever being offered.
The 60-minute cutoff is unchanged, and it now genuinely holds at every depth where before it only ever looked at the top level. Symlinks are never descended into, since
rmonly unlinks the link. Depth is capped at 8, and a subtree that cannot be shown to be settled is left alone rather than offered up.scanDirectory's recency parameter accepts either the existing number or{ skipRecentMinutes, deepRecencyCheck }, so the trash scans that pass0are untouched. Only browser cache scans opt in.Directories that were never scanned at all
Four of these sit beside the profiles rather than inside one, so
chromiumCacheDirsgrew asharedlist next to the per-profile entries:DawnWebGPUCache,DawnGraphiteCachecomponent_crx_cache,extensions_crx_cache,GrShaderCache,ShaderCache,GraphiteDawnCache,GPUPersistentCache,PnaclTranslationCacheThe last three aren't in the issue. Newer Chromium builds moved the Dawn and GPU caches up to the user-data level, and a local Chrome install had all three with content, so both spellings are listed — a directory that doesn't exist is skipped exactly as before.
All are regenerable caches. The two
*_crx_cachedirectories hold downloaded CRX packages that get re-fetched on the next update check; they are not the installed extensions inExtensions/.Safe Browsingis still excluded, as the issue asked.One shared code path
The browser cleaner IPC, the CLI, and the cloud agent each carried their own copy of the browser list, the profile enumeration, and the cache-dir loop. That triplication is how these directories stayed missing from all three at once. They now go through
services/chromium-cache, so the rules file is the only place a directory has to be added.Verified
npm run validate:rulespasses for all three platforms;npm run buildclean.Code Cache/js:The Code Cache result goes from being discarded entirely to reporting its 310 MB. With a live entry present,
jsis not offered at all — onlyjs/settledis, so the live file can be neither unlinked nor overwritten.Cache_Datais unchanged in both, with the memory-mapped block files still excluded, which is the behaviour the issue identifies as correct.Note
settings.cleaner.skipRecentMinutesis exposed in Settings but no caller ever passes it toscanDirectory, so the dropdown currently does nothing. Left alone here — happy to wire it up in its own PR.