fix(cache): stop a reclaim deleting an object written after it was decided - #52
Merged
Merged
Conversation
anoop-narang
requested review from
eddietejeda
and removed request for
a team
September 24, 2026 10:19
![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 |
…cided A store key is `(entry id, identity)` and an identity comes back: the file-id pool hands a re-opened path its previous record so its cached entries stay readable. `settle` decides to delete such a key and does it behind an await, and in between the key can be occupied again — by that same identity's next spill, or by the identity itself returning. The delete then destroyed a live record's bytes and left it pointing at nothing. `reclaim_orphaned_disk` now asks the index whether a live record names the object before deleting, as late as it can, and releases the byte count either way: an object left standing is one a newer record overwrote, and that record reserved its own bytes for it. `remove_disk_entry` goes through the same reclaim, since the gap between its removal and its delete is the same gap. That narrows the window rather than closing it. A put that has landed but whose record is not yet installed is invisible to the check. Closing it needs the store key to name the write and not just the writer, which means a generation in the key — and with one, a disk write never lands on the object it displaces, so there is no in-place overwrite left to keep. That is a larger change than this, and it is written up rather than made here. `ArtIndex::remove_checked` is closed outright. It tested the identity, removed, then put back what it found if the test had gone stale — and the restore was an unconditional insert, so a writer that took the key over in between lost its record while `entry_count` went on counting it. Deciding and removing are now one `compute_if_present`, so a key that changed hands is left alone instead. The verdict is recomputed on every run of that closure rather than latched on the first. `compute_if_present` decides under an optimistic read and upgrades to a write afterwards, so a removal whose upgrade loses runs the closure again — against whatever the key holds by then. A first run that finds the record ours followed by a retry that finds it someone else's acts on the retry, and must report the mismatch: latching the first verdict would decrement `entry_count` for a record still in the tree and hand the caller the new owner's entry to release. Tests: - `a_settlement_does_not_delete_an_object_written_after_it_was_decided` stages the A-B-A order: identity 7 spills, 9 takes the key over, 7 comes back and spills again, and only then does the settlement 9 displaced run. Without the check the settlement deletes 7's new object and the read misses. - `checked_removal_against_takeovers` takes the identities that race the removal, so the three tests over it state only what they vary. It runs as one round under threads, as every interleaving under shuttle, and as `a_checked_removal_that_retries_reports_the_verdict_it_acted_on`: more writers and enough rounds to reach a removal whose upgrade loses, which one round reaches too rarely to rely on. Against a latched verdict the last fails every run while the single round passes. - `an_in_place_disk_overwrite_releases_the_copy_it_supersedes` could not fail. It asserted through `insert`, which reached the branch it names only as a consequence of budget arithmetic, and the whole crate passed with the superseded handling deleted. It now writes out the two steps a spill performs — bytes to the store, then the record — and asserts the branch was taken. Its memory budget goes back to an ordinary one, since it no longer needs to provoke a particular eviction. - `only_a_same_identity_disk_write_lands_on_the_object_it_displaces` covers `DiskResidue::displacing` directly, for each combination of written form and displaced form.
anoop-narang
force-pushed
the
fix/reclaim-identity-aba
branch
from
September 24, 2026 10:49
b3079f6 to
9524196
Compare
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
1 participant
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.
Follows up the two findings review raised on the identity-scoped store keys.
A store key is
(entry id, identity), and an identity comes back — the file-idpool hands a re-opened path its previous record so its cached entries stay
readable. Two consequences were unhandled:
A settlement could delete a live object.
settledecides to reclaim a keyand deletes behind an
await. In between, a fresh write can occupy that key,and the delete then destroyed bytes a live record named.
reclaim_orphaned_disknow asks the index whether a record still names the object, as late as it can,
and releases the byte count either way.
This narrows the window rather than closing it — a put that has landed but whose
record is not yet installed is still invisible. Closing it needs the store key to
name the write and not just the writer, i.e. a generation in the key. That also
removes the in-place-overwrite case entirely, so it is written up in the code
rather than done here.
remove_checkedcould strand a record. It tested the identity, removed, thenput back what it found if the test had gone stale — an unconditional insert, so a
writer that took the key over in between lost its record while
entry_countkeptcounting it. Deciding and removing are now one
compute_if_present.That closure runs under an optimistic read and the tree upgrades afterwards, so a
losing upgrade runs it again against whatever the key holds by then. The verdict
is recomputed each run rather than latched on the first: latching it would
decrement
entry_countfor a record still in the tree and hand the calleranother writer's entry to release.
Tests. Each was run against the unfixed code first.
a_settlement_does_not_delete_an_object_written_after_it_was_decidedstagesA-B-A and fails without the guard.
a_checked_removal_that_retries_reports_the_verdict_it_acted_onkeeps writerson one key until the tree makes a removal retry — caught a latched verdict
10/10 runs, where the single-round test passed 30/30.
an_in_place_disk_overwrite_releases_the_copy_it_supersedeswas vacuous: thecrate passed with the handling it names deleted. It now drives the two steps a
spill performs and asserts the branch was taken.
only_a_same_identity_disk_write_lands_on_the_object_it_displacescoversDiskResidue::displacingover every written/displaced combination.Workspace suite 273 passed / 0 failed; shuttle 3 passed; fmt and clippy clean.