Skip to content

Fix TLRUCache keeping stale value when overwriting a key with an expired value#400

Open
gaoflow wants to merge 1 commit into
tkem:masterfrom
gaoflow:fix-tlru-overwrite-expired-stale-value
Open

Fix TLRUCache keeping stale value when overwriting a key with an expired value#400
gaoflow wants to merge 1 commit into
tkem:masterfrom
gaoflow:fix-tlru-overwrite-expired-stale-value

Conversation

@gaoflow

@gaoflow gaoflow commented Jun 24, 2026

Copy link
Copy Markdown

Assigning an already-expired value to an existing key in a TLRUCache is silently swallowed: __setitem__ returns early before storing the new value, but it leaves the previous entry in place. The cache then keeps returning the old value, even though the assignment should have replaced (or removed) it.

from cachetools import TLRUCache
c = TLRUCache(maxsize=10, ttu=lambda k, v, now: now + v, timer=lambda: 0.0)
c['x'] = 5
c['x'] = 0          # ttu -> now, i.e. immediately expired
c['x']              # -> 5  (stale old value; expected: KeyError / absent)
'x' in c            # -> True

When the new value is dead-on-arrival, drop any existing entry for that key so it is evicted instead of retaining the stale value. New-key inserts of expired values remain a no-op as before. Added a regression test.

Assigning an already-expired value (ttu yields a time that is not in the
future) to an existing key returned early before storing it, but left the
previous entry untouched.  As a result the assignment was silently ignored
and the cache kept returning the old value, violating the mapping contract
that `cache[key] = value` either stores `value` or removes the key.

Drop the existing entry in that case so the key is evicted instead of
retaining a stale value.
@tkem

tkem commented Jun 24, 2026

Copy link
Copy Markdown
Owner

Wow, nice catch! Thanks for reporting, and especially for providing a PR including sound regression tests!
However, I think it could be argued both ways - I wouldn't necessarily consider an "old" value that's not expired "stale", in this case.
So if the current value has still some time left, trying to replace it with something "dead-on-arrival" (I like that term very much!) may as well leave the old item in place, as far as I'm concerned. IIRC, this is also the approach with caches that provide a getsize parameter - trying to set a key/value pair that by itself exceeds the maximum cache size will be rejected immediately, but will leave an existing entry with the same key intact. So maybe that's existing practice which does support the way it's currently implemented, but I'll have to think a bit about it.
Anyway, thanks for bringing this up and providing a high-quality PR!

@codecov

codecov Bot commented Jun 24, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (48284d7) to head (9ea6948).

Additional details and impacted files
@@            Coverage Diff            @@
##            master      #400   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            5         5           
  Lines         1084      1089    +5     
=========================================
+ Hits          1084      1089    +5     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

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.

2 participants