fix: more chill abandoned partial loads and safer deferred eviction of weak held entries - #20073
fix: more chill abandoned partial loads and safer deferred eviction of weak held entries#20073clintropolis wants to merge 6 commits into
Conversation
clintropolis
changed the title
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| @SuppressWarnings("unused") | ||
| ListenableFuture<?> unused = gatedPool.getExecutorService().submit(() -> { | ||
| atGate.countDown(); | ||
| return openGate.await(30, TimeUnit.SECONDS); | ||
| }); |
FrankChen021
left a comment
FrankChen021
left a comment
There was a problem hiding this comment.
| Severity | Findings |
|---|---|
| P0 | 0 |
| P1 | 1 |
| P2 | 1 |
| P3 | 0 |
| Total | 2 |
Reviewed 4 of 4 changed files.
The review found one weak-entry identity race and one failed-partial-entry cleanup issue.
This is an automated review by Codex GPT-5.6-Luna(max)
| weakCacheEntries.computeIfPresent( | ||
| weakEntry.cacheEntry.getId(), | ||
| (cacheEntryIdentifier, weakCacheEntry) -> { | ||
| if (weakCacheEntry != weakEntry || weakCacheEntry.isHeld()) { |
There was a problem hiding this comment.
[P1] Replacement entry can be mistaken for the in-flight entry
If an abandoned mount entry is reclaimed and a replacement with the same ID is registered, the mount verification and reservation adjustment check only the ID. The old mount can therefore commit against the replacement's reservation and leave its mapper and files live. Verify object identity before committing or adjusting.
| final boolean isMounted = weakCacheEntry.cacheEntry.isMounted(); | ||
| if ((isNewEntry && !isMounted) | ||
| || (areWeakEntriesEphemeral && !weakCacheEntry.isHeld())) { | ||
| if ((isNewEntry && !isMounted) || areWeakEntriesEphemeral) { |
There was a problem hiding this comment.
[P2] Failed partial entries can lose restart metadata
A failed mount consumes the onUnmount hook and deletes the info file. If another hold remains, this guard retains the unmounted entry; the final non-creator hold does not remove it. A later acquire reuses it without rewriting the info file or restoring the hook, so bootstrap can lose the segment after restart.
| @SuppressWarnings("unused") | ||
| ListenableFuture<?> unused = gatedPool.getExecutorService().submit(() -> { | ||
| atGate.countDown(); | ||
| return openGate.await(30, TimeUnit.SECONDS); | ||
| }); |
| if (!existing.metadata().isMounted()) { | ||
| // rewrite the info file if it is missing | ||
| try { | ||
| storeInfoFile(dataSegment); |
There was a problem hiding this comment.
Two things:
-
The code that writes info files could use a cleanup. I think it would improve things to run it all through
rewriteInfoFile, i.e., makestoreInfoFilecallrewriteInfoFile, and also make theif (hold != null)branch inacquireSegmentcallrewriteInfoFile. It would be easier to track through all the places that can write an info file. -
Do we need to register a hook here to delete the info file on unmount?
4 participants
Description
Relaxes cache manager partial segment loading to not throw when pinning reference holds if the caller closed the holds (and so no longer cares to finish mounting and loading the segment), and also fixes a problem where a weak held entry with deferred unmount could be incorrectly unmounted since it was looking up by id without confirming it was the same reference (and not a newly re-added entry).