fix(store): deterministic LFU cache eviction (tie-break by key) - #6707
Open
SnowingFox wants to merge 1 commit into
Open
fix(store): deterministic LFU cache eviction (tie-break by key)#6707SnowingFox wants to merge 1 commit into
SnowingFox wants to merge 1 commit into
Conversation
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.
Root cause (issue #6706)
EntityCache's LFU cache (LfuCacheingraph/src/util/lfu_cache.rs) evicts a different set of entries on every process for the same deployment, the same blocks and the same binary.The chain, as analyzed in the issue:
EntityCache.updatesis astd::collections::HashMap(RandomState, seeded once per process), soas_modificationsiterates it in a different order on every run.self.currentas the loop runs (graph/src/components/store/entity_cache.rs).LfuCacheuses apriority_queue::PriorityQueuewhose eviction priority is(stale, Reverse<frequency>). Ties on that priority are the normal case, not the exception — within one block the overwhelming majority of entries sit at exactly(false, Reverse(1)).PriorityQueue::pop()breaks priority ties by heap position, which is determined by insertion order. So which tied entry is evicted depends on the random HashMap iteration order → different eviction sets per process.The indexing result is unaffected (entity data, modification counts, PoI digests are identical across runs); what moves is provenance — which reads are served from the cache vs. from the store. That makes cache hit rate and store read counts non-reproducible between otherwise identical runs (see the issue's measurements).
The fix
Break eviction-priority ties deterministically on the cache key. The priority type becomes
(stale, Reverse<frequency>, Reverse<K>). Keys are unique in the cache andK: Ord, so the priority is now a strict total order andPriorityQueue::pop()no longer falls back to the insertion-order-dependent heap layout for ties.This fixes all callers of
LfuCacheat once (today that isEntityCachevia theEntityLfuCachealias), regardless of the order in which they insert entries.Test
Adds
graph/tests/lfu_cache_determinism.rs→eviction_is_independent_of_insertion_order: insert the same six entries (equal weight, equal frequency → every candidate tied), in three different orders, evict down to a fixed max weight, and assert the same entries survive each time. On the old code the eviction set changes with insertion order (the test fails); on the new code it is deterministic.Scope note
(stale, frequency)entries". The specific order is arbitrary; the important property is that it is deterministic.EntityKeyis interned/cheap to clone (anInputSchemahandle + interned atoms +CausalityRegion(i32)+ an internedId), so the per-entry overhead is on the order of a small struct and is not counted towardmax_weight. If that is a concern for very large caches, a caller-side fix (deterministically ordered iteration inas_modifications) could be layered on, but the cache-level fix is sufficient and covers every caller.