Repository navigation
fix(cache): release disk when an entry is hydrated - #64
Merged
Merged
Conversation
A hydrating read replaced a disk record with a memory one without releasing the disk charge or deleting the stored object, so every hydrate -> spill cycle charged another copy until the disk cap filled. Disk charges are now owned values released exactly once when a record is replaced or removed, or when an operation is dropped. Store operations on one key are ordered by a striped async lock, so a delete decided earlier cannot remove a newer spill's object. Objects left by a cancelled delete, spill or removal are deleted by the next spill.
anoop-narang
requested review from
shefeek-jinnah
and removed request for
a team
November 13, 2026 05:58
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review — Blocking Issues: src/core/src/cache/core.rs:545 (insert_inner): A failed try_insert of a written disk record drops the DiskWrite and releases its charge. Line 571 keeps the disk record and sets write to None. The retry then installs the record with no charge. A later remove_disk_entry releases the charge again, and release_disk (fetch_sub) wraps used_disk_bytes. After that, every disk reservation fails.
Action Required: Never install a disk record without a live DiskWrite on a retry. For example, keep the bytes and rewrite before the next try_insert. Do not hold the key lock across evict_victims. Add a test where the install of a written disk record fails once and then succeeds. The test must assert assert_disk_matches_index afterwards.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
📊 Benchmark ComparisonCurrent:
Compared Liquid vs DataFusionDefault on the same runner |
try_insert reserved memory for every record, including disk records that hold none. With no slot under the key, that zero-byte reservation fails whenever the memory budget already reads over its limit, which drift in the memory accounting can leave it at. A spill refused there dropped its write, releasing the charge, and insert_inner retried the same disk record with no write: the record went in holding no charge, and its later removal released that charge a second time, wrapping the disk budget so that every later reservation failed. A record that holds no memory now installs however full the memory budget reads, so a written disk record always goes in with its write. A debug assertion states the invariant: a disk record is installed only by the write that charged it.
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 goes wrong
With a disk cap set (
with_max_disk_bytes) and a hydrating policy such asAlwaysHydrate, the disk tier fills up even though it holds little or nothing.Each time a read hydrates a disk entry back into memory, the memory record replaces the disk record, but the disk charge for it is never given back and its stored object is left in place. When that entry is spilled again, the spill charges a second copy. So every hydrate → spill cycle of one entry adds a full copy to
stats().disk_usage_bytes. Once the charge reaches the cap, spills start failing, flushes drop the entries they cannot place, and the reported disk usage stays pinned at the cap with nothing left on disk to evict. Recovering needs a reset.A loop of insert →
flush_all_to_disk→get(hydrates) → flush, on one 4.6 KB entry with a 4-copy cap, onmaintoday:With this change, every round charges 4624 after the spill and 0 after the read, with no object left behind.
The same accounting gap shows up on two other paths:
flush should only drop memory entries).The fix
remove_disk_entry) now takes the same lock.CachePolicy::notify_removeis called while the cache holds the entry's lock, so an implementation must not wait on a cache operation there. This is documented on the trait.This ports the intent of #52, which reclaimed disk on the same kind of replacement under the earlier store-key scheme. That code did not survive the move to the current identity-trailer scheme in #60. #62 handled the reverse race: a disk victim that a read hydrates before its removal.
Tests
hydrate_and_spill_cycles_hold_one_disk_copy: the loop above, asserting the charge and the stored object count each round. It fails onmainwith the table shown.a_hydrating_read_waits_for_a_spill_in_flight_on_its_key: a realgethydrates while a spill's write has landed but its record is not installed. The read must wait, then be woken when the record goes in.a_read_dropped_while_reclaiming_still_releases_the_charge: the same read, aborted while it waits.a_read_dropped_while_reclaiming_leaves_its_object_to_the_next_spill: the read is blocked by another key sharing its lock; its charge comes back once, and the next spill deletes its object.a_spill_dropped_after_its_put_leaves_its_object_to_the_next_spilla_removal_whose_policy_panics_leaves_its_object_to_the_next_spillan_insert_whose_policy_panics_leaves_the_displaced_object_to_the_next_spilla_spill_dropped_before_its_record_releases_its_reservationa_reclaim_leaves_the_object_a_later_spill_installeda_second_spill_of_an_entry_releases_the_first_ones_chargea_disk_record_an_eviction_keeps_keeps_its_charge: an eviction policy hands a disk record back unchanged; it keeps its charge, and removing it later releases that charge once.a_stale_disk_record_an_eviction_keeps_is_not_reinstalled: a read hydrates the entry while the policy decides; the stale record is not put back over it.a_failed_flush_leaves_a_record_another_writer_spilled: a real flush runs out of disk while an eviction spills the same entry in a smaller form.a_disk_removal_waits_for_a_spill_in_flight_on_its_keyKnown issues, not changed here
These were found while working on this and exist on
mainalready:reset()is not coordinated with operations in flight. A charge released after a reset can underflow the disk budget. It also leaves stored objects in place.try_insertcan drift under concurrent replacement. It is computed from the slot read before the insert, not the one actually replaced, so concurrent replacements of one entry can drift or underflow it.getreturns the new owner's bytes. Tracked in Disk reads can return another entry's bytes (t4 slot reuse) #63.putwhile its value write is pending can leak the file space the store allocated for it. This happens inside the store, below anything the cache can clean up by key.This change was investigated and written with AI assistance (Claude Code), and reviewed by Codex.