fix(core): reorder LruCache entries on get() for falsy values#2968
Open
chinesepowered wants to merge 1 commit intoQwenLM:mainfrom
Open
fix(core): reorder LruCache entries on get() for falsy values#2968chinesepowered wants to merge 1 commit intoQwenLM:mainfrom
chinesepowered wants to merge 1 commit intoQwenLM:mainfrom
Conversation
LruCache.get() guarded the LRU reorder with 'if (value)' — a truthy check that skipped the reorder when the cached value was 0, '', false, or null. The value was still returned correctly, but it stayed in its original insertion-order slot rather than being promoted to most- recently-used, so legitimate falsy values were evicted earlier than they should have been. Switch to Map.has() for existence so all values — truthy or not — get the reorder.
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.
Fix
LruCache.get()skipping the LRU reorder for falsy values.TLDR
LruCache.get()guarded the LRU reorder with a truthy check (if (value)), so cached values that were0,'',false, ornullwere never promoted to most-recently-used on access. The returned value was still correct, but the internal Map ordering was wrong — a legitimate falsy value stayed in its original insertion slot and would be evicted earlier than its true access pattern warranted. Switch toMap.has()for existence so every cached entry gets the reorder regardless of its value.Screenshots / Video Demo
N/A — internal cache semantics fix, no user-visible UI.
Dive Deeper
Before:
JavaScript falsy values (
0,'',false,null,NaN) fail theif (value)guard, so the delete-and-reinsert never runs.Mapiteration order is insertion order, and the eviction logic inset()(lines 29-33) usesthis.cache.keys().next().valueto pick the victim — always the oldest insertion. A frequently-accessed entry whose value happens to be0would therefore be evicted before a rarely-accessed entry whose value is a non-empty object, silently violating the LRU invariant.LruCacheis generic overV, so any consumer that stores falsy values is affected:0(token counts, file sizes)''/nullmight be a legitimate sentinelAfter:
Map.has()is the existence-correct check. Using it avoids ambiguity withundefined(whichcache.get()also returns on miss) and ensures every cached entry — regardless of value falsiness — gets the reorder on access.Modified file:
packages/core/src/utils/LruCache.ts— usehas()for existence, always reorder on hitReviewer Test Plan
new LruCache<string, number>(3)and.set('a', 0),.set('b', 1),.set('c', 2).get('a')— before the fix,'a'stayed at the front of the eviction queue because0is falsy; after the fix,'a'is promoted to most-recent.set('d', 3)to trigger eviction — before:'a'was evicted (wrong); after:'b'is evicted (correct, it's now the oldest non-accessed entry)LruCache<string, boolean>storingfalsevalues, andLruCache<string, string>storing''values — all should now reorder correctly on getTesting Matrix