Skip to content

fix: cache entries use self hold to avoid eviction while mounting, and other fixes - #20182

Merged
clintropolis merged 3 commits into
apache:masterfrom
clintropolis:partial-mount-self-hold
Aug 28, 2026
Merged

fix: cache entries use self hold to avoid eviction while mounting, and other fixes#20182
clintropolis merged 3 commits into
apache:masterfrom
clintropolis:partial-mount-self-hold

Conversation

@clintropolis

@clintropolis clintropolis commented Aug 28, 2026

Copy link
Copy Markdown
Member

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.

@clintropolis clintropolis changed the title fix: cache entries use self hold to avoid eviction while mounting fix: cache entries use self hold to avoid eviction while mounting, and other fixes Aug 28, 2026

@FrankChen021 FrankChen021 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

@clintropolis
clintropolis merged commit 9e17b1c into apache:master Aug 28, 2026
28 checks passed
@github-actions github-actions Bot added this to the 39.0.0 milestone Aug 28, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants