fix(memory): stop listing and counting expired entries - #200
Merged
Merged
Conversation
Entries that had expired but not yet been swept were returned by get_all_keys() and get_cache_data(), and counted as removed by clear_pattern(), clear_path() and delete_many(). Redis never returns an expired key, so CacheManager.clear_prefix() and the monitoring routes reported different numbers depending on the backend. Closes #178
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.
Closes #178.
Problem
An entry whose TTL had passed stayed in
MemoryBackend.cacheuntil the next sweep orget(). During that time, it was still visible:get_all_keys()andget_cache_data()listed it.clear_pattern(),clear_path()anddelete_many()counted it as removed.Redis never returns an expired key, so the same live data produced different numbers per backend. For example,
CacheManager.clear_prefix()with one live key and one expired key returned2on memory and1on Redis.Change
_evict(), whichclear_pattern,clear_pathanddelete_manyshare, still removes every matching entry but counts only the unexpired ones.get_all_keys()andget_cache_data()skip expired entries.Effect on the monitoring routes
/cached-hitsand/cached-recordsstill computeis_expiredand theexpired_*counts. With the built-in backends, those now stay at zero apart from an entry that expires between the listing and the route reading the clock, as they already did on Redis. The route logic is kept for that case and for third-party backends that do list expired entries.TestExpiredEntryMonitoringinjected an expired entry into the memory backend's dict to exercise that logic. It now stubsget_cache_data()to return one, so it tests the route without relying on the old backend behaviour.Tests
test_memory_backend_enumeration_skips_expired_entries.test_memory_backend_does_not_count_expired_entries_as_cleared, parametrized overclear_pattern,clear_pathanddelete_many.test_clear_prefix_does_not_count_expired_keys, run on both memory and Redis to pin the parity.expiryon memory and withPEXPIRE 1on Redis, not by sleeping for a TTL.CACHEX_REQUIRE_LIVE_SERVERS=1): 857 passed, 100% coverage.