Skip to content

fix(router-core): reset oldest pointer when evicting the sole LRU cache entry#7742

Open
FrancoKaddour wants to merge 1 commit into
TanStack:mainfrom
FrancoKaddour:fix/lru-oldest-not-reset-on-single-entry-eviction
Open

fix(router-core): reset oldest pointer when evicting the sole LRU cache entry#7742
FrancoKaddour wants to merge 1 commit into
TanStack:mainfrom
FrancoKaddour:fix/lru-oldest-not-reset-on-single-entry-eviction

Conversation

@FrancoKaddour

@FrancoKaddour FrancoKaddour commented Jul 3, 2026

Copy link
Copy Markdown

What

When set() evicts the only entry in the LRU cache (oldest === newest), the node has no next pointer, so the branch that updated oldest was never taken. oldest kept a stale reference to the deleted node while newest was correctly set to undefined.

On the next set() call the stale oldest caused cache.delete() to run against a key that no longer existed — leaving the new entry untracked by oldest. Subsequent insertions then grew the cache silently beyond its max capacity.

Root cause

// Before (buggy)
if (toDelete.next) {
  oldest = toDelete.next      // ← only set when a successor exists
  toDelete.next.prev = undefined
}
if (toDelete === newest) {
  newest = undefined          // ← correctly cleared
}
// When oldest === newest, toDelete.next is undefined → oldest is never cleared

Fix

// After
oldest = toDelete.next        // always update oldest (undefined when no successor)
if (toDelete.next) {
  toDelete.next.prev = undefined
}
if (toDelete === newest) {
  newest = undefined
}

Test

Added a test case respects max=1 across multiple replacements to lru.test.ts that reproduces the bug and passes with the fix.

Summary by CodeRabbit

  • Bug Fixes

    • Improved least-recently-used cache eviction so the oldest entry is updated correctly when items are removed.
    • Fixed behavior for single-item caches to ensure older entries are evicted as new values are added.
  • Tests

    • Added coverage for repeated replacements in a cache with a maximum size of 1.

…ntry

When set() evicts the only entry in the LRU (oldest === newest),
oldest.next is undefined so the if-branch that updated oldest was
skipped. oldest retained a stale pointer to the deleted node while
newest was correctly set to undefined.

On the next set() call the stale oldest caused cache.delete() to
operate on a key that no longer existed, leaving the new entry
untracked by oldest. Repeated insertions then grew the cache beyond
its max capacity.

Fix: unconditionally assign oldest = toDelete.next so that when
the evicted node has no successor, oldest becomes undefined.
@coderabbitai

coderabbitai Bot commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 0a68a162-a90b-42ff-b659-bca118c50fe9

📥 Commits

Reviewing files that changed from the base of the PR and between 208100b and 1fcf45e.

📒 Files selected for processing (2)
  • packages/router-core/src/lru-cache.ts
  • packages/router-core/tests/lru.test.ts

📝 Walkthrough

Walkthrough

The eviction logic in the LRU cache's set method was modified so that oldest is assigned from toDelete.next unconditionally, rather than only within the conditional block. A test was added verifying correct behavior when max=1 across multiple replacements.

Changes

LRU eviction fix

Layer / File(s) Summary
Eviction pointer fix and test coverage
packages/router-core/src/lru-cache.ts, packages/router-core/tests/lru.test.ts
oldest is now assigned from toDelete.next before the conditional cleanup, ensuring it becomes undefined when there is no successor; a new test verifies max=1 correctly evicts prior keys across multiple replacements.

Estimated code review effort: 1 (Trivial) | ~5 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the core fix to LRU eviction when removing the sole cache entry.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant