Repository navigation
fix(cache): leave a disk victim that a read hydrated - #62
Merged
Merged
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
📊 Benchmark ComparisonCurrent:
Compared Liquid vs DataFusionDefault on the same runner |
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.
Problem
Under memory and disk pressure, a scan reading through the cache can panic with:
Disk-pressure eviction in
write_batch_to_disktakes victims from the cache policy's disk queue (find_disk_victim), then removes each one withremove_disk_entry. Between those two steps, a concurrent read of the same entry can hydrate it back into memory. With the defaultAlwaysHydrate, every disk read does this. Soremove_disk_entryfinds aMemoryArrow/MemoryLiquidrecord:find_disk_victimpops X from the disk queue. The policy lock is released.index.insert(X, Memory*), andnotify_insertfiles X in a memory queue.remove_disk_entry(X)removes the memory record and panics.awaits on earlier victims in the same batch widen the window, and so does a multi-threaded runtime. The memory record is removed before the panic and its memory charge is never released, so each occurrence also leaks memory budget.evict_victim_inner'sEvictionOutcome::Removepath has the same gap: it reads the slot, then removes it later.The same code and race exist on
datafusion-contrib/liquid-cachemain.Fix
ArtIndex::remove_if(entry_id, pred)removes a key only ifpredholds for the record the key holds at removal time. It is built on congee'scompute_if_present, so the check and the removal are one step. A record installed afterpredlooked makes the tree reject the removal, andpredis asked again.remove_disk_entryremoves only aDiskLiquid/DiskArrowrecord. If the key now holds a memory record, it returns without touching the budget or the policy.Why this is correct for budget and policy state:
CacheFullwhen there is none.Test
disk_victim_hydrated_before_its_removal_is_left_in_memoryis deterministic. It uses the realLiquidPolicy,TranscodeEvict,AlwaysHydrateand t4 store. A wrapper policy runs a hook once, right afterfind_disk_victimreturns: the hook performs a realgetof the victim on another thread, thenflush_all_to_diskcontinues. Without the fix, the test panics with the message above. With it, the test passes.remove_if_removes_only_a_matching_slotcovers the new index method.There is no shuttle variant. Mounting a t4 store inside a shuttle test deadlocks, because
IoWorker::newblocks on a channel waiting for a shuttle-spawned thread. That is why no existing shuttle test builds aLiquidCache.This change was investigated and written with AI assistance (Claude Code).