Conversation
merge_insert()/add() commits insert new entries into the session's metadata/index caches keyed by dataset version (ManifestKey, TransactionKey, RowAddrMaskKey, RowIdIndexKey, IndexMetadataKey), but nothing in the write, commit, or cleanup paths ever evicted them. A long-running writer sharing one Session across many commits saw these caches grow roughly linearly with commit count -- looking exactly like a leak from the outside, even though every entry was legitimately "live" (referenced) cache data, bounded only by DEFAULT_METADATA_CACHE_SIZE/DEFAULT_INDEX_CACHE_SIZE (1 GiB each), a ceiling often above a container's actual memory limit. CleanupTask::run() now invalidates the metadata/index cache entries for every manifest version it physically deletes, once deletion actually succeeds (not during a side-effect-free `explain`). This engages on every path that calls cleanup, including optimize()'s auto-cleanup hook and explicit cleanup_old_versions(), which is exactly the maintenance cadence Lance recommends for long-running upsert workloads. The existing invalidate_prefix only matches an entry's namespace `prefix` field (a dataset URI), never its per-entry `key` field where version numbers live, so it can't evict a single version's entries. Added invalidate_key_prefix (backend trait + Moka impl + LanceCache wrapper) that matches on `key` with a `/`-boundary check, so evicting version 5 doesn't also evict version 50, and covers every optional suffix a key type may carry (e.g. a manifest's e_tag) without needing to know it in advance. Scoped to version-level cache entries for this PR; per-fragment entries (DeletionFileKey, RowIdSequenceKey) are a smaller, documented follow-up. See lance-format#1983 for the investigation (jemalloc-instrumented reproduction showing this is a genuine live-heap accumulation, not allocator retention or on-disk version bloat).
Contributor
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughCleanup now supports key-prefix cache invalidation through the cache backend and Moka implementation. Dataset cleanup tracks deleted manifest versions and evicts related metadata and index entries after successful execution, with regression tests covering matching and stale-version removal. ChangesCache eviction and cleanup
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant CleanupTask
participant LanceCache
participant MokaCacheBackend
CleanupTask->>CleanupTask: identify unreachable manifest versions
CleanupTask->>CleanupTask: delete stale files
CleanupTask->>LanceCache: invalidate stale version key prefixes
LanceCache->>MokaCacheBackend: evict matching namespace entries
MokaCacheBackend-->>CleanupTask: complete eviction
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rust/lance-core/src/cache/mod.rs`:
- Around line 240-255: Add a runnable Rustdoc # Examples block to
LanceCache::invalidate_key_prefix, following the nearby keys() documentation
style. Include the necessary imports and a minimal CacheKey implementation, then
demonstrate inserting an entry with insert_with_key, invalidating it via
invalidate_key_prefix, and asserting it is absent with get_with_key.
In `@rust/lance-core/src/cache/moka.rs`:
- Around line 132-141: Hoist the key-prefix formatting in invalidate_key_prefix
by creating a single "{key_prefix}/" String before invalidate_entries_if, then
have the per-entry closure reuse it for starts_with checks instead of calling
format! for each entry. Verify this behavior against the installed moka version,
where the predicate may run per cached entry and be invoked repeatedly during
retrieval processing.
In `@rust/lance/src/dataset/cleanup.rs`:
- Around line 511-546: Optimize invalidate_stale_version_caches by batching
invalidation work instead of issuing five sequential invalidate_key_prefix calls
per stale version. Build combined predicates or use a batched invalidation API,
matching stale versions across manifest, transaction, row address mask, row ID
index, and index cache keys while preserving the existing cache scopes and
behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: d2b32b86-b11a-492b-b89e-61dc8ef8f037
📒 Files selected for processing (4)
rust/lance-core/src/cache/backend.rsrust/lance-core/src/cache/mod.rsrust/lance-core/src/cache/moka.rsrust/lance/src/dataset/cleanup.rs
- Add a runnable # Examples block to LanceCache::invalidate_key_prefix,
matching this file's convention for public API docs.
- Add invalidate_key_prefixes (plural): invalidates entries matching any of
several key prefixes in one pass over the cache, instead of one pass per
prefix. invalidate_key_prefix now defaults to a single-element call to it,
which also hoists the "{key_prefix}/" allocation out of the per-entry
closure (it was being rebuilt on every entry scanned).
- CleanupTask::invalidate_stale_version_caches now calls
invalidate_key_prefixes twice total (once per cache) instead of 5 times
per stale version, so a cleanup run clearing many versions at once stays
cheap.
Addresses CodeRabbit review comments on lance-format#7718.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rust/lance-core/src/cache/backend.rs`:
- Around line 137-159: Clarify the documentation for invalidate_key_prefix and
invalidate_key_prefixes to state that implementing invalidate_key_prefixes is
sufficient because invalidate_key_prefix forwards to it, while implementing only
invalidate_key_prefix leaves the batched method as a no-op. Replace the
misleading “implement one of the two” guidance with explicit advice to override
invalidate_key_prefixes for backend support.
In `@rust/lance/src/dataset/cleanup.rs`:
- Around line 522-554: Add regression coverage for the index-cache invalidation
path in cleanup_evicts_stale_version_caches (or a sibling test): create and
populate an index before appending and cleaning stale versions, then assert via
session.index_cache_stats() or index_cache_keys() that entries for the stale
version are removed. Ensure the assertions validate the actual index-cache key
format and that cleanup reduces/removes stale index entries, exercising
invalidate_stale_version_caches.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 7f4fbbda-2232-493e-a8ef-be9cd469a525
📒 Files selected for processing (4)
rust/lance-core/src/cache/backend.rsrust/lance-core/src/cache/mod.rsrust/lance-core/src/cache/moka.rsrust/lance/src/dataset/cleanup.rs
- Clarify invalidate_key_prefix's doc comment: default-forwarding to invalidate_key_prefixes only works in one direction. Overriding invalidate_key_prefixes gives both methods; overriding only invalidate_key_prefix leaves invalidate_key_prefixes's no-op default in place for callers that invoke it directly (as LanceCache:: invalidate_key_prefixes and cleanup.rs do). The old "implement one of the two" wording implied a symmetry that doesn't exist. - cleanup_evicts_stale_version_caches previously never created an index, so the index_cache.invalidate_key_prefixes call ran against an empty cache with no way to catch a key-format mismatch. Now builds a real vector index and calls optimize_indices() partway through the write sequence, so the index cache actually accumulates version-keyed IndexMetadataKey entries the same way production code populates them, and asserts both caches shrink with no stale entries left in either. Verified the strengthened test catches the regression it's meant to catch: temporarily disabled the index_cache invalidation call, confirmed the test fails (before/after num_entries unchanged), restored the fix. Addresses two more CodeRabbit review comments on lance-format#7718's second review pass.
wjones127
reviewed
Sep 2, 2026
wjones127
left a comment
Contributor
There was a problem hiding this comment.
I think addressing this is a good idea, but I think it needs to be adapted to handle the new scheme for keys. Perhaps we can have a list of CacheKeys that we know we can drop and invalidate those one-by-one?
Comment on lines
+147
to
+150
| async fn invalidate_key_prefix(&self, prefix: &str, key_prefix: &str) { | ||
| self.invalidate_key_prefixes(prefix, std::slice::from_ref(&key_prefix.to_owned())) | ||
| .await; | ||
| } |
Contributor
There was a problem hiding this comment.
issue(blocking): we are planning on removing these APIs, since the new style of keys (the 128-bit opaque keys) no longer are strings with prefixes. Is there a way where we could avoid relying on prefixes but still invalidate known keys?
wjones127
requested changes
Sep 2, 2026
This branch has not been deployed
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
2 participants
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
#1983 was originally about
merge_insert's DataFusion join having no configured RAM limit. In investigating it I found that the join's RAM usage is only part of the story — there's a separate, more fundamental issue that also affects plainadd(), not justmerge_insert().A jemalloc-instrumented reproduction (table held at a constant row count,
cleanup_older_than=0on everyoptimize(), forced allocatorarena.purge()) showed:stats.allocated(jemalloc live heap, not just RSS) grows on every commit, even with disk usage and fragment/version counts provably bounded.table.add()alone leaks (+77.8 MB live heap over 1,500 calls), not justmerge_insert()(+117.8 MB) — ruling out the join as the sole cause.Root cause
Every commit inserts new entries into the session's metadata/index caches (
rust/lance/src/session/caches.rs,rust/lance/src/session/index_caches.rs), keyed by dataset version:Nothing in the write, commit, or cleanup paths ever evicts these. A repo-wide search confirms the only call sites for the existing
invalidate_prefixare the trait definition, its Moka implementation, and two test-only mock backends — never incommit.rs,cleanup.rs, oroptimize.rs. So a long-running writer sharing oneSessionacross many commits sees these caches grow roughly linearly with commit count — indistinguishable from a leak from the outside — bounded only byDEFAULT_METADATA_CACHE_SIZE/DEFAULT_INDEX_CACHE_SIZE(1 GiB each), a ceiling that's often above a container's actual memory limit.Cleanup (
cleanup_old_versions, andoptimize()'s auto-cleanup hook — exactly the maintenance cadence the docs recommend for long-running upsert workloads) already knows precisely which versions become unreachable when it deletes their files. It just never told the cache.Fix
CleanupTask::run()(rust/lance/src/dataset/cleanup.rs) now invalidates the metadata/index cache entries for every manifest version it physically deletes, gated onself.action.deletes_files()so a side-effect-freeexplain()stays side-effect-free.The existing
invalidate_prefixcouldn't be reused for this: it matches an entry's namespaceprefixfield (a dataset URI), never its per-entrykeyfield where version numbers actually live (seeInternalCacheKey::starts_with). So I addedinvalidate_key_prefix(backend trait method + Moka implementation +LanceCachewrapper) that matches onkeywith a/-boundary check — evicting version5doesn't also evict version50, and it covers every optional suffix a key type may carry (e.g. a manifest's e-tag) without needing to know it in advance.Scope
This PR covers version-scoped cache entries, which the jemalloc evidence points to as the dominant driver. Per-fragment entries (
DeletionFileKey,RowIdSequenceKey) aren't covered yet — documented as a follow-up in the code. The original ask in #1983 (a configurable RAM limit formerge_insert's DataFusion join) is a separate, complementary fix I'll follow up with in another PR — it bounds the join's own transient memory, not this cache-retention gap, and doesn't address the fact that plainadd()leaks too.Testing
test_cache_invalidate_key_prefix(rust/lance-core/src/cache/mod.rs) verifies the new primitive's boundary-safety (evicting"manifest/5"doesn't touch"manifest/50") and that it covers suffixed variants ("manifest/5/etag-abc").cleanup_evicts_stale_version_caches(rust/lance/src/dataset/cleanup.rs) writes 11 versions under one sharedSession, runs cleanup to retain only the latest, and asserts both that the cache shrinks and that no cached key still references a version whose files were just deleted.num_entriesunchanged before/after cleanup); restored the fix and confirmed it passes again.dataset::cleanup::,session::, andlance-core'scache::modules pass unmodified (34 + 11 + 22 tests respectively).cargo check,cargo fmt --check, andcargo clippy -- -D warningsare clean onlanceandlance-core, run under the toolchain pinned inrust-toolchain.toml(1.94.0) to match CI exactly.Summary by CodeRabbit
key_prefix/-prefixed variants while avoiding unrelated shorter numeric-overlap prefixes.