fix(cache): re-check ownership after the disk write, not only before - #49
Merged
Merged
Conversation
The ownership check ran before `store.put().await`, so a takeover landing during the write left a rewrite still reaching the record swap — where it replaced the current owner's record and deleted the object that record named, leaving that owner's index entry pointing at nothing. The comment there claimed a stale writer never reaches it, which was the wrong assumption rather than a wrong line. Ask the index again after the write. The index, not the record: the record may still name a previous owner this writer is legitimately superseding, while the index names whoever the bytes can be read by. A rewrite that is no longer that owner removes the object it just wrote and hands back its reservation instead. This narrows the window rather than closing it — the index read and the record swap are still not atomic, as nothing else in the cache is either (see the note above `supersede_disk_copy`). Closing it needs the two under one lock, which is a larger change than this fix.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Approved with nits. The post-write index re-check is a strict improvement: a stale rewrite no longer reaches the record swap that deletes the current owner's object. The remaining window and the missing test are stated in the PR description, so both are accepted as known. Nits cover a duplicated comment, a nested Option<Option<_>>, an unclaimed store.remove, and one blank line.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
📊 Benchmark ComparisonCurrent:
Compared Liquid vs DataFusionDefault on the same runner |
Drop the duplicated comment, early-return instead of nesting the record swap in an Option, and note that the removal on this path holds no claim on the key.
There was a problem hiding this comment.
All four prior nits are addressed in 70c4c94. No new findings.
The stale path leaves the previous DiskCopy record in place while deleting the object it names, but the index no longer names that identity, and every reader filters records by the index identity (disk_copy at src/core/src/cache/core.rs:537), so no read path reaches the deleted object. The reclaim paths only remove and release, which Ok(false) covers.
CI was still queued or in progress at review time, so no claim is made here about test results.
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.
Follow-up to #48. A reviewer on the consuming repo found this in the merged code.
The race
write_batch_to_diskchecks that aRewritestill owns the key, then awaitsstore.put. A takeover landing during that await leaves the rewrite still reaching the record swap below, where it replaces the current owner'sDiskCopyand deletes the object that record names:previous.identity != identity, and deletes B's object.read_disk_arrow_array.The comment on that branch claimed a stale writer never reaches it. That was the wrong assumption rather than a wrong line: the check is before the await, the swap is after.
This is only reachable because the identity went into the store key in #48 — before that, a put overwrote the shared object rather than writing beside it, so there was no other owner's object to delete.
The fix
Ask the index again after the write. The index, not the record: the record may still name a previous owner this writer is legitimately superseding, while the index names whoever the bytes can actually be read by. A rewrite that is no longer that owner removes the object it just wrote and hands back its reservation, leaving the current owner's record and object alone.
I first guarded on the record instead, which broke the legitimate case —
flush_all_to_diskwrites as aRewrite, so the current owner flushing over a previous owner's stale record was turned away.taking_a_key_over_removes_the_previous_owner_s_objectcaught it.What this does not do
It narrows the window, it does not close it. The index read and the record swap are still not atomic. Closing it needs both under one lock, which is a larger change and has to reason about lock ordering against the ART — the cache does not currently serialise any check-then-act pair, as the note above
supersede_disk_copysays.There is no test for it, and I want to be explicit about that. Two attempts, both discarded:
Reproducing the real interleaving needs the rewrite paused mid-await, which this suite has no way to do. Shuttle is the right tool — it is already wired here and already covers the file-id pool — and I would rather see the whole check-then-act class model-checked than keep patching instances. That is a bigger piece of work than this fix and I have not attempted it.
So: this is reasoned, not test-proven, which is weaker than the rest of #48. Judge it on that basis.
Existing suite is green —
fmt,clippy -D warnings, 23 test targets, and the shuttle job's own command.