install: never delete a concurrently published cache entry on Windows - #33884
install: never delete a concurrently published cache entry on Windows#33884Jarred-Sumner wants to merge 5 commits into
Code review found 3 potential issues
Found 4 candidates, confirmed 3. See review comments for details.
Details
| Severity | Count |
|---|---|
| 🔴 Important | 0 |
| 🟡 Nit | 3 |
| 🟣 Pre-existing | 0 |
| Severity | File:Line | Issue |
|---|---|---|
| 🟡 Nit | src/install/extract_tarball.rs:619-627 |
TOCTOU: path-based eviction can delete a fresh entry published between the package.json check and renameat |
| 🟡 Nit | src/install/extract_tarball.rs:590-603 |
package.json discriminator misclassifies GitHub cache entries that legitimately lack package.json |
| 🟡 Nit | src/install/extract_tarball.rs:629-631 |
Backoff sleep dropped from the stale-eviction retry path |
Annotations
Check warning on line 627 in src/install/extract_tarball.rs
claude / Claude Code Review
TOCTOU: path-based eviction can delete a fresh entry published between the package.json check and renameat
The stale-entry eviction is still a check-then-act by path: after `drop(dest)`, `sys::renameat(cache_dir, folder_name_z, …)` re-resolves `folder_name` at call time, so if a concurrent process has already evicted the stale entry and published its fresh extraction in that gap, this renames *their* fresh entry into tmpdir and `delete_tree`s it — the same failure mode this PR eliminates for the common case, just gated on a pre-existing corrupt entry. Opening `dest` with `can_rename_or_delete: true`
Check warning on line 603 in src/install/extract_tarball.rs
claude / Claude Code Review
package.json discriminator misclassifies GitHub cache entries that legitimately lack package.json
The `package.json` sentinel doesn't hold for `ResolutionTag::Github` — `package_missing_from_cache()` only checks `package.json` for `Npm` (the `_ =>` arm at `PackageInstall.rs:2351` uses bare `directory_exists_at`), and this file explicitly supports GitHub tarballs with no `package.json` ("allow git dependencies without package.json"). So two concurrent Windows installs of the same `github:` dep whose repo lacks `package.json` will still fall into the eviction branch and delete the winner's fre
Check warning on line 631 in src/install/extract_tarball.rs
claude / Claude Code Review
Backoff sleep dropped from the stale-eviction retry path
The 10/20/40/80ms backoff sleep is now only reachable when `cache_dir.open_at(folder_name)` fails; the stale-eviction arm does `retries += 1; continue` with no delay. If `sys::renameat` on the stale entry fails (e.g. AV/Search Indexer holds it with `FILE_SHARE_READ` so `open_at` succeeds but rename hits a sharing violation), all 4 retries burn in microseconds where the pre-PR code gave ~150ms cumulative for the handle to close. Consider keeping the sleep before this `continue` — at minimum when