From 248b23bf0606969dfb9918a5ed01761dbf696ffc Mon Sep 17 00:00:00 2001 From: allen0099 Date: Sat, 26 Sep 2026 12:32:46 +0000 Subject: [PATCH] fix(memory): stop listing and counting expired entries 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 --- CHANGELOG.md | 11 +++++++ fastapi_cachex/backends/memory.py | 26 +++++++++++----- tests/backends/test_memory.py | 49 +++++++++++++++++++++---------- tests/test_cache_manager.py | 19 ++++++++++++ tests/test_routes.py | 23 ++++++++++----- 5 files changed, 97 insertions(+), 31 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 03ee00c..bff1d18 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -99,6 +99,17 @@ Note that 0.3.3 was never released; 0.3.4 follows 0.3.2. monitoring routes. Scanned keys are now deduplicated. ([#173](https://github.com/allen0099/FastAPI-CacheX/issues/173)) +- **`MemoryBackend` no longer lists or counts expired entries.** Entries + that had expired but not yet been swept showed up in `get_all_keys()` and + `get_cache_data()`, and `clear_pattern()`, `clear_path()` and + `delete_many()` counted them as removed. Redis never returns an expired key, + so `CacheManager.clear_prefix()` and the monitoring routes reported + different numbers for the same live keys depending on the backend. The + expired entries are still removed; they are just not reported. As on Redis, + the monitoring routes' `expired_*` counts now stay at zero, apart from an + entry that expires while the route runs. + ([#178](https://github.com/allen0099/FastAPI-CacheX/issues/178)) + ## [0.3.7] - 2026-09-25 ### Added diff --git a/fastapi_cachex/backends/memory.py b/fastapi_cachex/backends/memory.py index bf44bb4..bd99735 100644 --- a/fastapi_cachex/backends/memory.py +++ b/fastapi_cachex/backends/memory.py @@ -243,12 +243,15 @@ async def clear(self) -> None: logger.debug("Memory cache CLEAR; all entries removed") async def _evict(self, matches: Callable[[str], bool]) -> int: - """Remove every entry whose key satisfies ``matches``; returns the count.""" + """Remove every entry whose key satisfies ``matches``. + + Returns how many of them had not expired yet: an expired entry is + already gone as far as callers can tell, as it is on Redis. + """ async with self.lock: + now = time.time() doomed = [key for key in self.cache if matches(key)] - for key in doomed: - del self.cache[key] - return len(doomed) + return sum(_is_live(self.cache.pop(key), now) for key in doomed) async def clear_path(self, path: str, include_params: bool = False) -> int: """Clear cached responses for a specific path. @@ -306,19 +309,26 @@ async def get_all_keys(self) -> list[str]: """Get all cache keys in the backend. Returns: - List of all cache keys currently stored in the backend + List of every cache key that has not expired """ async with self.lock: - return list(self.cache.keys()) + now = time.time() + return [key for key, item in self.cache.items() if _is_live(item, now)] async def get_cache_data(self) -> dict[str, tuple[CacheEntry, float | None]]: """Get all cache data with expiry information. Returns: - Dictionary mapping cache keys to (CacheEntry, expiry) tuples + Dictionary mapping cache keys to (CacheEntry, expiry) tuples, for + entries that have not expired """ async with self.lock: - return {key: (item.value, item.expiry) for key, item in self.cache.items()} + now = time.time() + return { + key: (item.value, item.expiry) + for key, item in self.cache.items() + if _is_live(item, now) + } async def _cleanup_task_impl(self) -> None: try: diff --git a/tests/backends/test_memory.py b/tests/backends/test_memory.py index 3d6b403..02752c8 100644 --- a/tests/backends/test_memory.py +++ b/tests/backends/test_memory.py @@ -1,5 +1,7 @@ import asyncio import time +from collections.abc import Awaitable +from collections.abc import Callable import pytest import pytest_asyncio @@ -445,29 +447,44 @@ async def test_memory_backend_get_cache_data_with_entries( assert expiry2 is None +async def _with_one_expired_entry(backend: MemoryBackend) -> None: + """Store "live" and "stale", then expire "stale" without sleeping.""" + entry = CacheEntry(fingerprint="e", content=b"v") + await backend.set("live", entry) + await backend.set("stale", entry, ttl=60) + backend.cache["stale"].expiry = time.time() - 1 + + @pytest.mark.asyncio -async def test_memory_backend_get_cache_data_expired_entries( +async def test_memory_backend_enumeration_skips_expired_entries( memory_backend: MemoryBackend, ) -> None: - """Test get_cache_data includes expired entries.""" - key = "GET|||localhost|||/test" - value = CacheEntry(fingerprint="test_etag", content=b"test_value") + """Redis never lists an expired key, and neither does this backend (#178).""" + await _with_one_expired_entry(memory_backend) - # Set with very short TTL - await memory_backend.set(key, value, ttl=1) + assert await memory_backend.get_all_keys() == ["live"] + assert list(await memory_backend.get_cache_data()) == ["live"] - # Wait for expiry - await asyncio.sleep(1.1) - cache_data = await memory_backend.get_cache_data() +@pytest.mark.asyncio +@pytest.mark.parametrize( + "clear", + [ + lambda backend: backend.clear_pattern("stale"), + lambda backend: backend.clear_path("stale"), + lambda backend: backend.delete_many(["stale"]), + ], + ids=["clear_pattern", "clear_path", "delete_many"], +) +async def test_memory_backend_does_not_count_expired_entries_as_cleared( + memory_backend: MemoryBackend, clear: Callable[[MemoryBackend], Awaitable[int]] +) -> None: + """An expired entry is removed but not counted, as Redis DEL would (#178).""" + await _with_one_expired_entry(memory_backend) - # Expired entries should still be in the raw cache data - # but get() won't return them - assert key in cache_data - retrieved_value, expiry = cache_data[key] - assert retrieved_value == value - assert expiry is not None - assert expiry <= time.time() + assert await clear(memory_backend) == 0 + assert "stale" not in memory_backend.cache + assert "live" in memory_backend.cache def test_ensure_cleanup_started_without_event_loop() -> None: diff --git a/tests/test_cache_manager.py b/tests/test_cache_manager.py index 588ca20..2f18aad 100644 --- a/tests/test_cache_manager.py +++ b/tests/test_cache_manager.py @@ -1,6 +1,7 @@ """Tests for CacheManager application-level caching.""" import asyncio +import time from collections.abc import AsyncGenerator from functools import partial from typing import TYPE_CHECKING @@ -443,6 +444,24 @@ async def test_clear_prefix_removes_only_matching_keys( assert await memory_backend.get("unrelated:key") is not None +@pytest.mark.asyncio +async def test_clear_prefix_does_not_count_expired_keys( + cache_manager: CacheManager, +) -> None: + """Every backend reports the same count for the same live keys (#178).""" + await cache_manager.set("kept", 1) + await cache_manager.set("lapsed", 2, ttl=60) + backend = cache_manager.backend + key = "cache:lapsed" + if isinstance(backend, MemoryBackend): + backend.cache[key].expiry = time.time() - 1 + else: + await backend.client.pexpire(backend._make_key(key), 1) # type: ignore[attr-defined] + await asyncio.sleep(0.01) + + assert await cache_manager.clear_prefix() == 1 + + @pytest.mark.asyncio async def test_clear_prefix_with_subprefix_argument( memory_backend: MemoryBackend, diff --git a/tests/test_routes.py b/tests/test_routes.py index d8e818a..a777556 100644 --- a/tests/test_routes.py +++ b/tests/test_routes.py @@ -514,6 +514,20 @@ def test_add_routes_with_none_dependencies_no_error(self, app, client, setup_cac assert response.status_code == 200 +def _report_expired(backend: MemoryBackend, key: str, entry: CacheEntry) -> None: + """Make ``get_cache_data`` return an entry whose expiry has passed. + + The built-in backends never list an expired entry (#178), but one can + expire between the listing and the route reading the clock, and a + third-party backend may list them. + """ + + async def get_cache_data() -> dict[str, tuple[CacheEntry, float | None]]: + return {key: (entry, time.time() - 1.0)} + + backend.get_cache_data = get_cache_data # type: ignore[method-assign] + + class TestExpiredEntryMonitoring: """Test monitoring routes show expired entries correctly.""" @@ -521,15 +535,12 @@ def test_cached_hits_shows_expired_entry(self, app, client, setup_cache): """/cached-hits marks is_expired=True for entries whose TTL has passed.""" add_routes(app) - # Directly inject an already-expired entry into the backend's internal dict # TestClient sends Host: testserver by default cache_key = "GET|||testserver|||/expired-route|||" expired_entry = CacheEntry( fingerprint='W/"expiredtag"', content=b"old data", media_type="text/plain" ) - setup_cache.cache[cache_key] = CacheItem( - value=expired_entry, expiry=time.time() - 1.0 - ) + _report_expired(setup_cache, cache_key, expired_entry) response = client.get("/cached-hits") assert response.status_code == 200 @@ -550,9 +561,7 @@ def test_cached_records_shows_expired_entry(self, app, client, setup_cache): expired_entry = CacheEntry( fingerprint='W/"expireddata"', content=b"stale", media_type="text/plain" ) - setup_cache.cache[cache_key] = CacheItem( - value=expired_entry, expiry=time.time() - 1.0 - ) + _report_expired(setup_cache, cache_key, expired_entry) response = client.get("/cached-records") assert response.status_code == 200