fix: cache entries use self hold to avoid eviction while mounting, and other fixes - #20182
Conversation
FrankChen021
left a comment
There was a problem hiding this comment.
| Severity | Findings |
|---|---|
| P0 | 0 |
| P1 | 0 |
| P2 | 1 |
| P3 | 0 |
| Total | 1 |
The review focused on the new mount/eviction synchronization and its cache-lifecycle tests; the eviction gate still has a residency race described in the inline finding.
Reviewed 1 of 9 changed files with findings; all 9 changed files were reviewed.
Validation: focused git diff --check passed. Builds and tests were not run.
This is an automated review by Codex GPT-5.6-Luna(max)
| if (containerFetchesInFlight[containerIndex] > 0) { | ||
| // A fetch is writing into this container. Deleting the file now would leave it writing to a null File, so | ||
| // hand the eviction to whichever fetch finishes last rather than blocking here. | ||
| containerEvictionPending[containerIndex] = true; |
There was a problem hiding this comment.
[P2] Eviction can clear a newer fetch's residency
The new in-flight counter only protects fetches that have already entered beginContainerFetch. Once this branch releases containerLocks, eviction clears downloadedFiles and bitmap bits later in the method outside that lock. A new fetch can enter during that gap, initialize the replacement container, write its bytes, and mark the files downloaded; the old eviction then removes those entries and bits, so the fetch returns with inconsistent residency (isFullyDownloaded() becomes false and mapFile can fail). Keep the clear and generation work under the same gate, or prevent new fetches until eviction finishes.
Description
Fixes a possible race condition where a load is abandoned mid-mount, then reclaimed, but then another caller re-creates the entry before the mount can clean up after itself, leaving an abandoned filemapper. Mounts now use a self-hold to prevent this mixed state from being possible (and makes the state of things a bit easier to reason about).
Also fixes some issues related to partial container evictions, related to bundle mount failure rollbacks, abandoned full downloads, etc. Container eviction is now done with a sort of ref counting and deferred eviction, so that we don't try to delete a file in the middle of being written to.