fix: keep disk cache file state consistent on main - #24511
Merged
mergify[bot] merged 15 commits intoMay 21, 2026
Merged
Conversation
Move FIFO post-evict callbacks outside internal locks safely and prevent stale disk cache eviction callbacks from deleting files that have been reinserted. Also repair stale disk cache index entries when SetFile sees an index hit but the physical file is gone. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
There was a problem hiding this comment.
Pull request overview
This PR cherry-picks a disk cache stale-index fix onto main by tightening coordination between the FIFO cache index and on-disk cache files during eviction and stale-file repair, and by adding regression tests to lock in the behavior.
Changes:
- Update
fifocache.Cacheeviction flow to runpostEvictcallbacks outsidequeueLock, addContains/Replace, and track item queue membership to improve eviction correctness. - Harden
DiskCacheeviction/write paths to avoid deleting files that are concurrently being updated, and to repair stale index entries when the physical file is missing. - Add/extend unit tests covering stale-index repair, eviction skipping updated paths, size replacement correctness, and post-evict concurrency behavior.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| pkg/fileservice/fifocache/fifo.go | Refactors eviction/post-evict flow, adds Contains/Replace, and introduces queue-state tracking for items. |
| pkg/fileservice/fifocache/fifo_test.go | Adds regression tests for Replace accounting and for postEvict executing outside queueLock (including panic behavior). |
| pkg/fileservice/fifocache/data_cache.go | Adjusts path deletion to defer post-evict callbacks until after shard unlock. |
| pkg/fileservice/disk_cache.go | Prevents evict/remove races with in-flight writes, repairs stale index entries, and uses Replace to update FIFO accounting after rewrites. |
| pkg/fileservice/disk_cache_test.go | Adds regression tests for stale index repair and eviction/write race scenarios. |
Comments suppressed due to low confidence (1)
pkg/fileservice/fifocache/fifo.go:416
EvictWithWaitdefers allpostEvictcallbacks until after the whole eviction loop completes, which can keep many large values alive at once under heavy over-capacity conditions. IfpostEvictis responsible for releasing memory/resources, consider flushing callbacks in batches (or otherwise streaming them) to reduce peak retained memory during large evictions.
func (c *Cache[K, V]) EvictWithWait(ctx context.Context, capacityCut int64) {
c.queueLock.Lock()
var pendingPostEvicts []_PendingPostEvict[K, V]
defer func() {
c.queueLock.Unlock()
if c.postEvict != nil {
for i := range pendingPostEvicts {
c.postEvictItem(ctx, pendingPostEvicts[i], true)
pendingPostEvicts[i] = _PendingPostEvict[K, V]{}
}
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…k-cache-stale-index-main
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Retain memory cache values before publishing them in FIFO cache so delete/evict paths cannot release cache ownership before it is acquired. Keep slow post-set callbacks outside FIFO locks while preserving the existing public constructor. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Use the same shard-locked validation for immediate enqueue as pending enqueue so a slow post-set callback cannot enqueue an item deleted before enqueue. Add coverage for enqueue-after-delete accounting. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
XuPeng-SH
approved these changes
May 21, 2026
XuPeng-SH
left a comment
XuPeng-SH
left a comment
Contributor
There was a problem hiding this comment.
LGTM.
The stale-index fix looks correct end-to-end: delayed eviction callbacks no longer delete a reinserted disk-cache file, stale full-file index entries are repaired on rewrite, and the FIFO/mem-cache hardening around pending enqueue jobs, retain-before-publish, and same-key callback ordering closes the new lifecycle races introduced by moving post-evict work outside queueLock. The regression coverage also hits the key concurrency and accounting edges.
Contributor
Merge Queue Status
This pull request spent 1 hour 36 minutes 24 seconds in the queue, including 1 hour 6 minutes 27 seconds running CI. Required conditions to merge
|
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
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.
What type of PR is this?
Which issue(s) this PR fixes:
issue #24493
What this PR does / why we need it:
This ports the disk-cache stale-index fix and its follow-up hardening to
main.The core bug is that, after FIFO
postEvictcallbacks were allowed to run outsidequeueLock, an old disk-cache eviction callback could delete the physical full-file cache file for a newer entry of the same key. That left the in-memory disk-cache index present while the file was gone, so later reads missed disk cache and repeatedly fetched the full object from S3 again.This PR keeps the main-branch behavior aligned with the hardened 4.0-dev fix and also carries the conservative follow-up guards needed to avoid new cache lifecycle races:
postEvictflow, includingEvictWithWaitpostSetcallback returnsValidation
source /Users/shenjiangwei/Work/code/matrixone/setup_env.sh && go test -race -mod=mod ./pkg/fileservice ./pkg/fileservice/fifocache -run 'TestMemCacheRetainsBeforeSetVisible|TestMemCacheSkipsStalePostEvictAfterReinsert|TestMemCacheSerializesSameKeyCallbacks|TestDirectEnqueueSkipsDeletedItem|TestEvictSkipsDeletedPendingEnqueueJob|TestReplaceAccountsPendingEnqueueJob|TestEvictAccountsPendingEnqueueJob' -count=1 -timeout=120ssource /Users/shenjiangwei/Work/code/matrixone/setup_env.sh && go test -mod=mod ./pkg/objectio ./pkg/fileservice ./pkg/fileservice/fifocache -count=1 -timeout=180s