lib/store: force cleanup now removes in-memory cache entries - #652
Open
sileshidev-lab wants to merge 1 commit into
Open
lib/store: force cleanup now removes in-memory cache entries#652sileshidev-lab wants to merge 1 commit into
sileshidev-lab wants to merge 1 commit into
Conversation
DeleteCacheFile had no CAStore-level override, unlike GetCacheFileStat and ListCacheFiles which both already check the memory cache. Calls fell straight through to the disk-only cacheStore.DeleteCacheFile, so entries that only lived in the memory cache were never actually removed. This meant the force-cleanup endpoint would list an in-memory entry as a deletion candidate (ListCacheFiles already includes memory entries) but silently fail to remove it, since the delete step only touched disk. Add a DeleteCacheFile override that removes the entry from the memory cache first, then falls through to the disk delete. A not-exist error from the disk delete is only swallowed when we know the entry was memory-only (never had a disk file to begin with), so the not-exist signal is preserved for entries that genuinely don't exist anywhere. Fixes uber#490
|
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #490 — the force-cleanup endpoint gets entries that only exist in the memory cache, but never actually removes them, since the current deletion logic only touches disk files.
Root cause:
CAStore.GetCacheFileStatandCAStore.ListCacheFilesboth already have overrides that check the memory cache (ListCacheFilesexplicitly merges memory + disk entries).DeleteCacheFilehas no such override — calls fall straight through to the embedded, disk-onlycacheStore.DeleteCacheFilevia struct embedding. So the force-cleanup handler lists an in-memory entry as a valid deletion candidate, then silently no-ops on actually deleting it.Fix: add a
CAStore.DeleteCacheFileoverride, matching the pattern of the existing memory-aware overrides — remove the entry from the memory cache first (BlobMemoryCache.Remove, a documented no-op if absent), then fall through to the disk delete. A not-exist error from the disk delete is only swallowed when we know the entry was memory-only (tracked before removal), so the not-exist signal is preserved for entries that genuinely don't exist anywhere — existing disk-only and disabled-memory-cache behavior is unchanged.Test plan
TestCAStore_DeleteCacheFilewith 4 subtests: memory-only entry is removed with no error, disk-only entry still deletes as before (regression), memory cache disabled falls back to disk-only behavior (regression), and a truly nonexistent entry still returns the not-exist errorgo build ./...— whole repo compilesgo test -race --tags "unit" ./lib/store/...— all pass (matches the project's exact CI test command/flags)golangci-lint run ./lib/store/...— 0 issues