fix(cache): serve entries only to their own file - #48
Conversation
A cache key packs the file id into 16 bits, so the 65,537th distinct file a process registers aliases the first. Entries recorded nothing about where they came from and the read API took no expected identity, so an aliased lookup returned the other file's data: a panic when the column types differed, silently wrong rows when they matched. Record the unnarrowed file id alongside each entry and compare it on read. A mismatch reads as a miss, so the caller re-reads from its source and gets correct data, and it is counted. Maintenance that rewrites a key in place keeps the identity already recorded, and can no longer resurrect a key that has since been removed. Lease file ids instead of assigning them permanently. A lease is held by the file handle and by every row group and column derived from it, and returns to a FIFO pool when the last holder drops, so the id space tracks the files being read rather than every file ever read. Entries deliberately hold no lease: an id reused while its old entries are resident leaves them unreachable rather than readable, which is what lets the release stay out of index removal. Stop panicking when a cached entry cannot answer a predicate. try_eval_predicate now returns Option, which every caller already treated as "materialize from the source", so an entry built for a different column degrades to a re-read instead of unwinding the stream mid-flight. Drop the debug_assert on the narrowing conversions. It made debug builds panic where release builds truncate, which left the shipped behaviour untestable; a test pins the wrap down instead.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
The ## Full diff block was omitted from the review prompt, so the patch came from gh pr diff 48. Additional context was read from src/datafusion/src/cache/file_id.rs, src/datafusion/src/cache/column.rs, src/datafusion/src/cache/mod.rs, src/datafusion/src/cache/id.rs, src/datafusion/src/sync.rs, src/datafusion/src/reader/plantime/opener.rs, src/datafusion/src/reader/runtime/liquid_stream.rs, src/core/src/cache/core.rs and src/core/src/cache/budget.rs. Every CI check was queued or in progress at review time, so no check result is cited here.
Blocking issues:
-
src/datafusion/src/cache/column.rs:93 — the recorded identity is the leased id, and the pool recycles that id, so the identity does not name the file. Two files holding the same recycled id record the same identity and the check passes. A query on the second file is served the first file rows. Leasing makes this the common case, not a rare one.
-
src/datafusion/src/cache/file_id.rs:87-94 — an id is recycled while entries keyed from it are still resident. Once identity names the file rather than the id, every key the new holder computes is held by the previous holder identity, so reads miss and
ArtIndex::insertrefuses. The new holder caches nothing until those entries are evicted, which on a cache below its budget never happens. The same change also drops the path-to-id map, so a file re-registered later computes different keys and loses its cache between queries. -
src/core/src/cache/core.rs:602-604 —
try_update_memory_usage(new_memory_size, old_memory_size)reserves memory when the new entry is smaller than the old one, so it can returnErrand theexpect("memory release cannot fail")panics.
Action required:
- Derive the entry identity from something that names the file across id reuse, such as a stable hash of the file path.
- Make the id lifetime cover the entries keyed from it, or give each path a stable id, so a recycled id does not land on keys that are still occupied.
- Release the reservation at core.rs:603 without asserting success, or subtract directly.
- Add a test that caches data for one file, drops the handle, registers a second file with the same schema, and asserts the second file reads a miss. The two new identity tests do not cover id reuse: one holds every filler handle so no id is recycled, and the other recycles id 0 but caches nothing.
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Leasing file ids reintroduced the aliasing the identity check exists to prevent: the identity was the id itself, so a file that inherited a released id was indistinguishable from the one that gave it back and read the entries it left behind. TPC-H caught it as a wrong column. Split the two. The id stays narrow and recycled, because the key's file field is 16 bits and has to be reusable. The identity is a separate u64 that is never handed to a different file, so an inherited id carries no claim on what the previous holder cached. A released id remembers the path and identity it had, so re-opening the same file keeps both and its cached entries stay readable — a file read twice is a cache hit, not a collision. Any other path gets a fresh identity. Also fix the callers outside the three crates I had been building: examples/core.rs and the core README doctests.
There was a problem hiding this comment.
Review
The full diff was omitted from the review prompt. gh pr diff was too large to read in one pass, so these files were read from the checkout: src/datafusion/src/cache/file_id.rs, src/datafusion/src/cache/mod.rs, src/datafusion/src/cache/column.rs, src/datafusion/src/cache/id.rs, src/core/src/cache/index.rs, src/core/src/cache/core.rs, src/core/src/cache/builders.rs, src/core/src/cache/budget.rs. The liquid_array predicate changes were not read.
Blocking Issues
-
src/datafusion/src/cache/file_id.rs:121-140— a file that inherits a recycled id can neither read nor write the keys built from that id. The previous holder's entries hold those keys under a different identity,get_checkedmisses, andArtIndex::insertrefuses. A multi-file scan caches only its first files. Re-reading a file loses its cache whenever the matchingReleasedis not at the front offree. -
src/core/src/cache/core.rs:603—try_update_memory_usage(new_memory_size, old_memory_size)reserves memory when the rewrite shrinks the entry, so theexpectcan panic. Raised in cycle 1 and unchanged.
Action Required
Fix both findings. For 1, tie id release to removal of the entries keyed from that id, or let a caller-originated insert take over a key held by another identity. For 2, drop the result or subtract directly.
Cycle 1 finding on identity naming the recycled id is resolved by commit ddeae58. The three prior nits are unchanged and are not repeated here.
Refusing an insert whose key is held by another identity left the key occupied by data nobody can read: the holder is a file that has since let its id go, and the file that now owns the id cannot read it either. On a cache below its budget nothing evicts it, so that file would never cache the key again. Overwrite instead, and keep counting. The incumbent loses an entry it could not have served anyway. Also stop asserting that restoring a memory reservation succeeds. When the entry being replaced was larger, restoring it grows the reservation again and can fail on a full cache, which would panic. Nothing is stored on that path either way, so leave the budget under-counted rather than bring the process down.
|
Thanks — three real findings. All addressed; the branch is now three commits. 1. Identity was the recycled id. Correct, and it was the worst of the three: leasing made the aliasing routine rather than rare. Fixed in This is also what the TPC-H job was failing on ( 3. 2. A recycled id lands on keys that are still occupied. Agreed on the problem, including the part I had understated — I'd written this off as "unreachable until evicted", but on a cache below its budget nothing evicts, so the new holder never caches that key at all. A permanent cliff, not a transient one. Resolved differently from either option suggested, because leasing makes a third one available: the insert now overwrites rather than refuses. With leases, a key held by a different identity necessarily belongs to a file that has already let its id go — so nothing live can read it, and the incumbent loses an entry it could not have served. Refusing optimised for two files sharing a key while both are live, which only happens past the 16-bit ceiling and is already counted by That keeps id lifetime decoupled from entry lifetime, which is what stops the release path needing a process-wide lock underneath a crossbeam-epoch pin — where it would deadlock against Tests. You were right that the two identity tests didn't cover id reuse; they were written before the lease existed. Separately: the earlier failures in |
There was a problem hiding this comment.
Review
The full diff was omitted from the review context. I read src/core/src/cache/index.rs, src/core/src/cache/core.rs, src/core/src/cache/builders.rs, src/datafusion/src/cache/file_id.rs, src/datafusion/src/cache/column.rs, src/datafusion/src/cache/id.rs and src/datafusion/src/cache/mod.rs from the checkout.
Both blocking findings from cycle 2 are fixed. ArtIndex::insert now lets a new owner take a colliding key over, so an inheriting file can cache. The expect on the rollback in try_insert is gone.
The takeover introduced one new blocking issue.
Blocking Issues
src/core/src/cache/index.rs:156— a maintenance write (identity = None) adopts whatever identity holds the key at write time. A caller takeover landing between a squeeze read and its write relabels the old file data with the new file identity. The new owner then reads the old file rows as a hit. See the inline comment for the interleaving.src/core/src/cache/core.rs:753andsrc/core/src/cache/core.rs:801are the two reachable call sites.
Action Required
Make a maintenance write carry the identity observed when the entry was read. Store the payload only when the key still holds that identity, and drop the write otherwise. Add a test that inserts under identity A, takes the key over with identity B, then replays a maintenance write built from the entry A left, and asserts identity B does not read those rows.
📊 Benchmark ComparisonCurrent:
Compared Liquid vs DataFusionDefault on the same runner |
Maintenance builds its payload from an entry it read earlier and stores it after an await — a squeeze reads, writes bytes to disk, then inserts. Adopting whatever identity held the key by then relabels the old file's data as the new owner's, and the new owner reads those rows as a hit. Carry the identity the entry was read under. A rewrite lands only while the key still holds it, and is dropped otherwise: the payload belongs to a source that no longer owns the key. Replaces the Option<u64> on the insert path with an explicit WriteIdentity, so a caller storing its own data and maintenance rewriting someone else's are no longer the same call shape. The index read that feeds maintenance now reports the identity alongside the entry.
|
Good catch, and it is a real hole the takeover opened. Fixed in The interleaving is exactly as described: A maintenance write now carries the identity the entry was read under, and lands only while the key still holds it. Otherwise it is dropped: the payload belongs to a source that no longer owns the key. Concretely,
Those were the same call shape before, which is what let the two behaviours blur. Test added as suggested — Workspace is green — One note on the review's framing, since it has come up each cycle: the finding in cycle 2 about a recycled id landing on occupied keys was resolved by takeover rather than by tying id lifetime to entry lifetime. That was deliberate. Releasing ids from index removal needs a process-wide lock underneath a crossbeam-epoch pin and deadlocks against |
There was a problem hiding this comment.
Review
The full diff was omitted from the review prompt. I fetched it with gh pr diff and read src/core/src/cache/core.rs, src/core/src/cache/index.rs, src/core/src/cache/builders.rs, src/core/src/cache/io_context.rs, src/core/src/cache/utils.rs, src/datafusion/src/cache/file_id.rs, src/datafusion/src/cache/column.rs, src/datafusion/src/cache/mod.rs and src/datafusion/src/io/mod.rs from the checkout.
Prior findings are resolved. The identity is now independent of the recycled id, the recycled key is taken over, the expect on the rollback at core.rs:618 is gone, and WriteIdentity::Rewrite drops a stale maintenance write.
Blocking Issues
src/core/src/cache/core.rs:767— a declined rewrite abandons the disk write it already performed. Thedisk_copiesrecord and the store object keep the old file's bytes under the key.reuse_disk_copy(line 545) andwrite_in_memory_batch_to_disk(line 397) later adopt that copy onkindalone, so the new owner's entry points at the old owner's bytes and reads them as a hit. The interleaving is in the inline comment.
Action Required
Make the disk tier identity-aware, or clean up after a declined rewrite:
- Add the identity to
DiskCopy, and treat a copy recorded under a different identity as absent in both adoption sites. - Or delete the store object and the
disk_copiesrecord whentry_insertdeclines aRewriteafter a write.
Untested risk
CI results for this push are not available to me. The failure above is a race, so no existing test covers it. A test that takes the key over between the disk write and the index write would pin it down.
One non-blocking comment is inline on src/datafusion/src/cache/file_id.rs.
A declined rewrite has already written its bytes, so the store object and its `disk_copies` record outlive the entry they were built for. The record named only a kind and a length, so the next owner of the key adopted the object on those alone and read the previous owner's rows as a hit — through `reuse_disk_copy` or the spill path. Record the identity that wrote each copy, and treat a copy belonging to anyone else as absent at both adoption sites. The paths that act on whatever object is there regardless of owner — superseding it, discarding it, releasing its reservation — read it unscoped.
|
Confirmed and fixed in Took the first option — the disk tier is now identity-aware, mirroring the index:
Preferred over cleaning up after a declined rewrite because the cleanup has its own window: the decline and the delete are not atomic either, and an object deleted after a successful takeover write would take the new owner's bytes with it. Making the record unadoptable needs no ordering. On the untested-risk note — you are right that no test covers the race, and orchestrating "take the key over between the disk write and the index write" deterministically is more than the invariant needs. Workspace green: Also picked up the non-blocking inline note on |
There was a problem hiding this comment.
Review
The full diff was omitted from the review prompt. I read src/datafusion/src/cache/file_id.rs, src/datafusion/src/cache/mod.rs, src/datafusion/src/cache/column.rs, src/core/src/cache/index.rs, src/core/src/cache/core.rs, src/core/src/cache/builders.rs and src/datafusion/src/reader/plantime/opener.rs from the checkout.
Prior findings are addressed. The identity is separate from the recycled id, the rollback at core.rs:658 no longer panics, WriteIdentity::Rewrite drops a stale maintenance write, and DiskCopy now records its writer.
Blocking Issues
src/datafusion/src/cache/file_id.rs:121—acquirerestores a path's identity only when that path'sReleasedrecord is at the front offree. Any other release order gives a re-opened file a fresh identity, which orphans every entry the file cached earlier. Release order is stream completion order, and acquire order is partition open order, so the two diverge for any scan over more than one file.
Action Required
Match the path against the whole free queue, not against its front element only. Take the matching record, and fall back to the front record when the path has none. Add a test that releases two paths and re-acquires the second one first.
The pool took its Mutex from std::sync rather than crate::sync, which under the shuttle test feature resolves to shuttle's primitives. A std::sync::Mutex is opaque to the model checker, so the Shuttle job could not explore any interleaving across acquire and release — the pool was excluded from the one job meant to cover it. Import through crate::sync and add a shuttle test for the invariant the scheme rests on: two leases alive at the same time never share an id, and never share an identity. A lease is released from Drop, on whatever thread held it last, so these race by construction. Also correct the module doc, which still said a new owner's writes are refused. They take the key over.
|
Correction to my previous comment: I said I had picked up the inline note on The Fixed the import, and added The stale module doc. Also right — it still described the pre- The other two inline comments (lines 94 and 140) are the cycle-2 finding, resolved by the takeover in |
There was a problem hiding this comment.
Review
The full diff was omitted from the review prompt. I fetched it with gh pr diff and read src/datafusion/src/cache/file_id.rs, src/datafusion/src/cache/mod.rs, src/datafusion/src/cache/column.rs, src/datafusion/src/sync.rs, src/core/src/cache/index.rs, src/core/src/cache/core.rs, src/datafusion/src/reader/plantime/opener.rs, Cargo.toml and .github/workflows/ci.yml from the checkout.
The disk-tier finding from the previous cycle is resolved. DiskCopy records its writer and disk_copy filters on it, while supersede_disk_copy and discard_disk_copy read through any_disk_copy. The WriteIdentity split, the identity separation and the non-panicking rollback all still hold.
Blocking Issues
src/datafusion/src/cache/file_id.rs:134—acquirerestores a path's identity only when that path'sReleasedrecord is at the front offree, andpop_frontdiscards the front record on a mismatch. Any other release order gives a re-opened file both a different id and a fresh identity, which orphans every entry the file cached earlier. Release order is stream completion order; acquire order is partition open order (plantime/opener.rs:317). The two diverge for any scan over more than one file, so a multi-file table re-reads its whole working set on every query. This was raised in the previous cycle andacquireis unchanged.
Action Required
Match the path against the whole free queue, not against its front element only. Take the matching record, and fall back to the front record when the path has none. Add a test that releases two paths and re-acquires the second one first.
Untested risk
The Shuttle Test job runs cd src/core then cargo test --features "shuttle" (.github/workflows/ci.yml lines 140-141). That selects the liquid-cache package only. shuttle_concurrent_leases_stay_distinct lives in the liquid-cache-datafusion package, so the job does not build or run it, and the pool stays uncovered by the model checker. Either extend the job to run the datafusion package with its shuttle feature, or move the pool test into a target that job selects.
CI results for this push are not available to me, so I make no claim about what passed.
A path's identity was restored only when its released record happened to sit at the front of the free queue, and a mismatch discarded that record anyway. Release order is stream completion order and acquire order is partition open order, so for any scan over more than one file the two diverge: a re-read got a fresh identity, orphaned everything it had cached, and the working set was re-read on every query. Match the path against the whole queue and take its own record, falling back to the front only when the path has none. Also run the shuttle job over this package. It does `cd src/core`, which selects the liquid-cache package only, so the pool's model-checked test was never built or run.
|
Both correct, both fixed in Front-of-queue matching. Test as suggested — The Shuttle job doesn't select this package. Also right, and this is the second time I've claimed Shuttle coverage it did not have. Last cycle I fixed the Added a second step running the same command in Workspace green: |
There was a problem hiding this comment.
Cycle 7 review. The full diff was omitted from the review context, so this cycle was reviewed from the since-diff (9aaf76d to bef24d09) plus the checkout: src/datafusion/src/cache/file_id.rs, src/datafusion/src/cache/mod.rs, src/datafusion/src/cache/column.rs, src/datafusion/src/cache/id.rs, src/core/src/cache/index.rs, src/core/src/cache/core.rs, src/core/src/cache/builders.rs.
All prior blocking findings are fixed in code:
- Identity was the recycled id —
FileIdnow carries a separateidentityfromnext_identity, which is never reused (file_id.rs:56-75). - A recycled id landing on occupied keys —
WriteIdentity::Ownedtakes the key over (index.rs:175-180). try_update_memory_usage(new, old)panic on a declined write — the result is now discarded (core.rs:658-660).- A maintenance rewrite adopting a new owner's identity —
WriteIdentity::Rewritelands only while the observed identity still holds the key (index.rs:183-187). - A stale disk copy adopted by the next owner —
DiskCopyrecords the writing identity anddisk_copyfilters on it (core.rs:46-54,core.rs:516-523). - A re-opened path losing its identity behind the queue front —
acquiresearchesfreeby path and falls back to the front (file_id.rs:140-154), covered bya_reopened_path_finds_its_record_anywhere_in_the_queue.
The pool invariants hold under the new lookup: a record sits in free only while its id is unleased, and a record is removed when its id and identity are handed back out, so two live leases share neither number.
CI was still queued or in progress at review time, including Rust CI / Shuttle Test, which runs the new src/datafusion shuttle step. Nothing here was executed as part of this review.
Scoping the DiskCopy record was not enough. The store object itself was addressed by the packed entry id alone, so two identities that share a key share the object. A write is issued before the index rewrite that would decline it, so it can land after another owner has taken the key over and become disk-backed — overwriting bytes that owner's index entry and record both agree are its own. Silent wrong data, past every check. Put the identity in the store key. The two then address different objects and a late write cannot reach the other's bytes at all, with no ordering to get right. Removes take the identity from the record they are dropping, which is what that record is for.
|
Reopening: a reviewer on the consuming repo found a real silent-corruption path that survived the approval here. Fixed in The hole. Scoping the
Every check passes and the data is wrong — the exact failure this PR exists to eliminate, on the tier I had only half-covered. Filtering at adoption time cannot catch it, because the object is clobbered after the new owner's entry is installed. The fix is to put the identity in the store key, so the two never address one object and a stale write cannot reach the other's bytes at all. That needs no ordering to be correct, which is why I preferred it to synchronising the write against the index. Removes take the identity from the record they are dropping — which is what that record is for. Regression test as asked: Workspace green — Worth recording for whoever reads this later: this was in the tree when the PR was approved with 14/14 CI green, and it was found by a second reviewer looking at the same code from the consuming side. I had also exercised the branch on a live workspace and seen nothing — that test used three small files against a multi-gigabyte cache, so nothing ever evicted and the disk tier was never touched. Neither the approval, nor the CI, nor the live test was evidence about this path. |
There was a problem hiding this comment.
Review
The full diff was omitted from the review context. Fetched it with gh pr diff, and read src/core/src/cache/core.rs, src/core/src/cache/index.rs, src/core/src/cache/io_context.rs, src/core/src/cache/builders.rs, src/datafusion/src/cache/file_id.rs, src/datafusion/src/cache/column.rs and src/datafusion/src/cache/mod.rs from the checkout.
All prior blocking findings are resolved in the code: the path lookup in free (bef24d09), the Rewrite guard on maintenance writes (f4dd8a53), the identity on DiskCopy (5da657f0), the identity in the store key (015d6d85), and the dropped expect on the reservation rollback at core.rs:659.
Blocking Issues
src/core/src/cache/core.rs:1105 — the store key now includes the identity, so a write for one owner no longer overwrites a previous owner's object. Line 1129 still releases that object's reservation. The previous object is then unreachable and unaccounted, and no path removes it.
Action Required
In write_batch_to_disk, when the replaced DiskCopy names a different identity, remove its store object as well as releasing its reservation.
CI was queued or in progress when this review started, so no check result is reported here.
Putting the identity in the store key stopped a write landing on another owner's object, but left the reservation handling behind: the previous copy's bytes were released while its object stayed on disk, unreachable and uncounted, with nothing that would ever remove it. Removing it whenever the identities differ is wrong in the other direction — a write built before a takeover would delete the live owner's object. The two cases are not distinguishable by identity alone, so ask the index: whoever it says owns the key is the only writer whose bytes can still be read. A write from anyone else is stale and is dropped before it touches the store, which is also why the supersede below can only ever be the current owner reclaiming what a previous one left. The stale case was caught by the test from the previous commit, which started failing when the supersede was added.
|
Right, and it is a direct consequence of the previous commit. Fixed in Before the identity went into the store key, a put genuinely overwrote the previous object, so releasing its reservation was correct. Afterwards the keys differ, the old object survives its own record, and that release left disk held by a blob nothing reaches and the budget no longer counts. The obvious fix is wrong in the other direction, which is worth recording. Removing the previous object whenever the identities differ means a write built before a takeover deletes the live owner's object — reintroducing the corruption two commits back, mirrored. My own test from The two cases are not distinguishable by identity values alone — there is no ordering between them — so the fix asks the index instead. Whoever it says owns the key is the only writer whose bytes can still be read, so a write from anyone else is stale and is dropped before it touches the store. That also means the supersede below can only ever be the current owner reclaiming what a previous one left, which is what makes removing that object safe. Dropping the stale write early is better than cleaning up after it: it never creates the object, so there is nothing to strand even if the cleanup path is missed.
Workspace green: |
There was a problem hiding this comment.
Review
The full diff was omitted from the review prompt. Reviewed the since-diff (015d6d8..b5cf5a5) plus these files from the checkout: src/core/src/cache/core.rs, src/core/src/cache/index.rs, src/core/src/cache/io_context.rs, src/core/src/cache/budget.rs, src/core/src/cache/builders.rs, src/datafusion/src/cache/file_id.rs.
Prior blocking findings are resolved. acquire now searches the free queue for the path (file_id.rs:140), the takeover deletes the previous owner's object (core.rs:1146), and the shrinking rewrite no longer asserts (core.rs:659). Two new blocking findings follow, both in the stale-write guard added by b5cf5a5.
Blocking Issues
src/core/src/cache/core.rs:1111— the dropped write keeps the disk reservation taken at line 1088. NoDiskCopyrecord names those bytes, so no path releases them. Repeated takeovers during squeezes drivedisk_usage_bytestomax_disk_bytes.src/core/src/cache/core.rs:1108— the guard also drops anOwnedwrite, which is not stale. The caller then installs a disk stub over bytes that were never written, and the next read panics inread_disk_arrow_array.
Action Required
- Release the reservation before the early return:
self.budget.release_disk(len);. - Pass the
WriteIdentityintowrite_batch_to_diskand apply the drop only toRewrite. - Add a test that asserts
disk_usage_bytesis unchanged after a dropped write.
CI checks were queued or in progress when this review started, so no result is reported here.
🤖 Generated with Claude Code
Two faults in the stale-write guard, both from taking a bare identity where the kind of write was what mattered. An owned write is the caller taking the key, and the index has not caught up yet by construction — the bytes land before the entry does. Treating that as stale dropped the write and left the caller's own entry pointing at an object that was never written, which panics on the next read. Take a WriteIdentity and drop only a Rewrite. A dropped write also kept the disk it had reserved. No DiskCopy names those bytes, so nothing would ever release them, and repeated takeovers during squeezes would walk the tally to the limit while holding nothing. Release before returning. Both are covered now: an owned write whose index entry lags must still find its bytes, and a dropped rewrite must leave disk usage unchanged.
|
Both correct, both fixed in The owned write. Right — an owned write is the caller taking the key, and the index has not caught up by construction: the bytes land before the entry does. Treating that as stale dropped the write and left the caller's own entry pointing at an object that was never written, panicking on the next read. The guard now takes a The reservation. Also right. Nothing records those bytes, so nothing would ever release them, and repeated takeovers during squeezes walk the tally to the limit while holding nothing. Released before the early return. Tests as asked, both mutation-verified:
Worth noting on the second: before writing it I re-ran the existing suite with the unconditional guard restored, and everything passed. So nothing covered that path at all — the finding was not merely unnoticed, it was untestable by what was there. Context on how this was missed. Between your last review and this one I did a pass enumerating every site that can reach a cache entry or a store object — index, store, Workspace green: |
There was a problem hiding this comment.
The full diff was omitted from the review prompt. Reviewed by reading the since-diff plus these files from the checkout: src/core/src/cache/core.rs, src/core/src/cache/index.rs, src/core/src/cache/io_context.rs, src/core/src/cache/builders.rs, src/core/src/cache/budget.rs, src/core/src/cache/policies/squeeze.rs, src/datafusion/src/cache/file_id.rs, src/datafusion/src/cache/column.rs, src/datafusion/src/cache/id.rs, src/datafusion/src/cache/mod.rs.
All prior blocking findings are now fixed in code:
acquiresearchesfreefor the path (file_id.rs:140-154), so a re-opened file keeps its own id and identity regardless of queue position.a_reopened_path_finds_its_record_anywhere_in_the_queuecovers it.write_batch_to_diskguards onlyRewrite(core.rs:1119-1125), so an owned write always lands and its index entry cannot point at missing bytes.- A dropped rewrite releases its disk reservation (
core.rs:1123). - A superseded object belonging to a different identity is removed, not just un-recorded (
core.rs:1160-1165). try_insertno longer asserts on the restore path (core.rs:664-666).DiskCopycarries an identity anddisk_copyfilters on it (core.rs:522-529), and the store key includes the identity (io_context.rs:51).- The pool uses
crate::sync::Mutexand is exercised by a Shuttle test.
CI checks were queued or in progress when this review started, so the test results are not visible here.
The identity recorded against an entry is the file id alone, so it cannot tell row group 0 from row group 65,536, or column 0 from column 65,536, inside one file. Dropping every assertion left those two narrowing silently with nothing to catch the collision — the doc claimed the identity check absorbed it, which holds for the file id only. Restore the assertion for both. Only the file id wraps unguarded, and it is the one the identity covers. Also leave the multi-column predicate loop rather than the function when an array cannot answer, matching the two arms above it: being unable to answer does not mean the column is unreadable, and the arrow fallback below may still serve it from the cache.
There was a problem hiding this comment.
Review
The full diff was omitted from the review prompt. Fetched it with gh pr diff, and read src/core/src/cache/core.rs, src/core/src/cache/index.rs, src/core/src/cache/io_context.rs, src/core/src/cache/builders.rs, src/core/src/store.rs, src/datafusion/src/cache/file_id.rs, src/datafusion/src/cache/column.rs, src/datafusion/src/cache/mod.rs, .github/workflows/ci.yml, src/datafusion-server/src/admin_server/handlers.rs and benchmark/src/inprocess_runner.rs from the checkout.
Every blocking finding from cycle 10 is fixed. The guard in write_batch_to_disk now covers Rewrite only, it releases the disk reservation it took, and it removes a superseded object when the previous owner differs. The two nits from cycle 10 are addressed as well. One new blocking finding, from the store key change in 015d6d8.
Blocking issues
src/core/src/cache/core.rs:257-261—resetleaves every store object behind, and a new identity means a new store key, so nothing overwrites those objects again. The store grows by up tomax_disk_bytesper reset, anddisk_usage_bytesdoes not count the growth.
Action required
Delete the recorded store objects in reset, before clearing disk_copies and the budget. The fix and the failure scenario are in the inline comment.
CI checks were queued or in progress when this review started, so this review reports no check result.
The store key carries the identity that wrote it, so everything written after a reset lands under new keys. Nothing overwrites the old objects, and with disk_copies cleared nothing can find them either — every reset stranded up to max_disk_bytes, unreachable and uncounted. Delete the recorded objects before forgetting the records that name them. reset becomes async, which both callers already are. Before the identity went into the key this was invisible: a new process reused the same keys and overwrote what the old one left.
There was a problem hiding this comment.
The full diff was omitted from this review prompt, so the diff since the last review was read from the prompt and the following files were read from the checkout: src/core/src/cache/core.rs, src/core/src/cache/index.rs, src/core/src/cache/io_context.rs, src/core/src/cache/builders.rs, src/core/src/cache/budget.rs, src/datafusion/src/cache/file_id.rs, src/datafusion/src/cache/mod.rs, src/datafusion/src/cache/column.rs, src/datafusion/src/cache/id.rs.
Every blocking thread from the previous cycles is addressed in the code:
- Identity separate from the recycled id (
file_id.rs:56-75). - A re-opened path finds its own record anywhere in
free(file_id.rs:140-154). RewriteversusOwnedwrite identity (index.rs:53-76,index.rs:174-188).- Restored memory reservation no longer asserted (
core.rs:679-682). - Store key carries the identity (
io_context.rs:51), and a superseded object of another owner is deleted (core.rs:1175-1181). - A dropped rewrite hands back its disk reservation (
core.rs:1138) and onlyRewriteis guarded (core.rs:1134). resetdeletes the store objects before dropping the records that name them (core.rs:263-276), and both callers await it.
CI checks were still queued or in progress when this review started, so no test result is claimed here.
1 participant
The problem
A cache key is a packed 64-bit integer with a 16-bit file field (
src/datafusion/src/cache/id.rs), and file ids come from a counter that only ever climbs (src/datafusion/src/cache/mod.rs,register_or_get_file_with_hints). The narrowing is guarded only bydebug_assert!, so in any release build the 65,537th distinct path a process registers is keyed identically to the first.Nothing catches the collision, because an entry records no identity:
and the read API takes no expected identity, so it cannot check one:
Whatever is under the key is returned as the answer. Depending on which path touches it first that is a panic (mismatched types), a swallowed miss (mismatched lengths,
arrow::compute::filter(..).ok()), or silently wrong rows when the types happen to line up. Insert is sticky —CachedColumn::insertreturnsAlreadyCachedwhen the key is occupied — so the colliding file never replaces the incumbent.Note this is not hypothetical arithmetic: a long-lived process that streams new files (ingest, compaction rewrites) reaches the ceiling on elapsed files seen, not files resident.
The fix
1. Entries carry their identity. The index slot records the unnarrowed
u64file id, compared on read. The key may truncate; the identity does not.get_checkedreturns an entry only to its owner and counts a mismatch; plaingetstays unchecked for maintenance, which legitimately acts on whatever occupies a key.inserttakesOption<u64>—Somefor a caller,Nonefor maintenance rewriting in place, which therefore cannot change whose an entry is nor resurrect a removed key. A key held by another identity is refused rather than overwritten, so two colliding files do not evict each other in turn.src/corestores an opaqueu64and learns nothing about parquet.Any collision now degrades to a miss and a re-read: correct data, no cache benefit, counted.
2. File ids are leased.
cache/file_id.rshands out a lease held by the file handle and everything derived from it, returned to a FIFO pool when the last holder drops — so the live id count tracks files being read, not files ever read. FIFO on purpose: a just-released id is handed out last, giving its old entries the longest window to evict.Entries deliberately hold no lease. An id reused while its old entries are resident leaves them unreachable rather than readable, which is exactly what (1) guarantees. That is what keeps id release out of index removal, where it would need a process-wide lock underneath a crossbeam-epoch pin and would deadlock against
reset.3. The predicate path stops panicking.
try_eval_predicatereturnsOption<BooleanArray>across both traits, both default impls and every override.Nonealready meant "cache cannot answer, materialize from the source" to every caller, so an entry built for a different column degrades to a re-read instead of unwinding the stream. This covered more than the obvious sites — a second copy ofeval_predicate_on_arrayreached from ~40 call sites, plus four further.expects indecimal_array,float_arrayandhybrid_primitive_arraythat only surfaced once the signature changed. Those sit on the hybrid fast path, i.e. the normal state after transcoding.The
debug_assert!s on the narrowing conversions are gone. They made debug builds panic where release builds truncate, which left the shipped behaviour untestable — the end-to-end test below cannot run with them in place. A test pins the wrap down as a known, reproducible condition instead.New counters on
LiquidCacheParquet:leased_file_ids,file_ids_over_key_width,identity_mismatches. The last should read zero forever.Deliberately not done
dev-toolsdecoder, the committed trace parquets. Worth revisiting only iffile_ids_over_key_widthever moves.congeeis hard-wired to 8-byte keys, so this means forking a third-party crate; (1) gives the same guarantee far cheaper. If the index ever stops beingusize-keyed, a 128-bit hash becomes the right answer.ipc.rs), not only to build it, so dropping it while its entries are reachable makes them unreadable. Out of scope for a correctness fix.Testing
373 tests green;
cargo fmt --checkandclippy --all-targets -- -D warningsclean.Two tests carry the argument, both mutation-verified — disabling the identity comparison makes each fail on the right assertion:
an_entry_is_never_served_to_a_different_identity— a colliding file reads a miss, the owner still reads its own data, the collider cannot displace it.a_file_past_the_key_ceiling_does_not_read_the_first_file_s_data— walks the real registration path, registers 65,537 files, asserts the packed keys genuinely collide, then asserts the newcomer reads a miss.Also
reading_files_one_after_another_does_not_consume_the_id_space(opens 66,535 files one at a time, asserts every one gets id 0 — the shape that used to wrap the counter),maintenance_preserves_identity_and_cannot_resurrect_a_removed_key, and sevenFileIdPooltests covering sharing, release, FIFO reuse, over-width counting and stale-lease release.Two existing tests failed when the lease landed; both were the lease working correctly — they opened files in a loop and let each handle drop, so every file got the same recycled id. Fixed by holding the handles, which is what "N files in the cache at once" means.
Note for anyone building this: the repo has no
rust-toolchain.tomland default stable is now too old for its owndatafusion55 dependencies. Built with+1.95.0.