diff --git a/src/youtube_extension/backend/services/intelligent_cache.py b/src/youtube_extension/backend/services/intelligent_cache.py index a92cbe171..d4de006be 100644 --- a/src/youtube_extension/backend/services/intelligent_cache.py +++ b/src/youtube_extension/backend/services/intelligent_cache.py @@ -149,8 +149,11 @@ async def get(self, key: str) -> Optional[Any]: entry = self.cache[key] # Check expiration - if entry.expires_at and datetime.now(timezone.utc) > entry.expires_at: - del self.cache[key] + if self._is_expired(entry): + # Lazy expiry on read is the only place expired entries are + # ever removed -- there is no background sweeper -- so this + # branch has to release everything delete() releases. + self._release_entry(key, entry) self.stats.miss_count += 1 return None @@ -201,6 +204,11 @@ async def set(self, key: str, value: Any, ttl: Optional[int] = None, tags: list[ # Update existing or add new if key in self.cache: old_entry = self.cache[key] + if self._is_expired(old_entry): + # Replacing an entry that has already died is a fresh + # insert, not a re-write of a hot key: the successor + # must not inherit the dead entry's access history. + self.access_patterns.pop(key, None) self.stats.total_size_bytes -= old_entry.size_bytes else: self.stats.total_entries += 1 @@ -215,18 +223,48 @@ async def set(self, key: str, value: Any, ttl: Optional[int] = None, tags: list[ logger.error(f"Failed to set L1 cache entry {key}: {e}") return False + @staticmethod + def _is_expired(entry: CacheEntry) -> bool: + """Whether ``entry`` has passed its TTL. + + Both the lazy-expiry branch in ``get()`` and the replacement branch in + ``set()`` have to agree on what "dead" means. Expressing the predicate + once keeps them from drifting -- a disagreement here would resurrect + exactly the bookkeeping inconsistency this helper set exists to close. + """ + return bool(entry.expires_at and datetime.now(timezone.utc) > entry.expires_at) + + def _release_entry(self, key: str, entry: CacheEntry) -> None: + """Drop an entry and every piece of bookkeeping that tracks it. + + Three paths remove entries -- ``delete()``, ``_evict_if_needed()`` and + the lazy-expiry branch in ``get()`` -- and each one previously repeated + this teardown by hand. Eviction forgot ``access_patterns`` and expiry + forgot all three counters, so an entry's footprint outlived the entry + itself. Routing every removal through one helper makes that class of + omission structural rather than a thing each new path has to remember. + + ``set()`` replacing an already-expired key is a fourth way an entry + stops existing, but it reuses the slot instead of freeing it, so it + drops only ``access_patterns`` rather than calling this helper. + + ``total_size_bytes`` in particular is not just a reported number: + ``_evict_if_needed()`` budgets against it, so bytes that are never + released permanently shrink the layer's usable capacity. + + Callers must already hold ``self._lock``. + """ + del self.cache[key] + self.stats.total_entries -= 1 + self.stats.total_size_bytes -= entry.size_bytes + self.access_patterns.pop(key, None) + async def delete(self, key: str) -> bool: """Delete value from in-memory cache""" with self._lock: if key in self.cache: entry = self.cache[key] - del self.cache[key] - self.stats.total_entries -= 1 - self.stats.total_size_bytes -= entry.size_bytes - - # Clean access patterns - if key in self.access_patterns: - del self.access_patterns[key] + self._release_entry(key, entry) logger.debug(f"L1 Cache DELETE: {key}") return True @@ -254,10 +292,8 @@ async def _evict_if_needed(self, new_size_bytes: int): # Remove oldest entry (LRU) oldest_key = next(iter(self.cache)) entry = self.cache[oldest_key] - del self.cache[oldest_key] + self._release_entry(oldest_key, entry) - self.stats.total_entries -= 1 - self.stats.total_size_bytes -= entry.size_bytes self.stats.eviction_count += 1 logger.debug(f"L1 Cache EVICTED: {oldest_key}") diff --git a/tests/unit/test_intelligent_cache.py b/tests/unit/test_intelligent_cache.py index b59b3be50..d1c7a36c3 100644 --- a/tests/unit/test_intelligent_cache.py +++ b/tests/unit/test_intelligent_cache.py @@ -1841,3 +1841,285 @@ def test_subsequent_call_uses_ema(self): layer._update_avg_access_time(20.0) # EMA: 0.1 * 20 + 0.9 * 10 = 11.0 assert layer.stats.avg_access_time_ms == pytest.approx(11.0, rel=0.01) + + +# --------------------------------------------------------------------------- +# Removal paths release entry bookkeeping +# --------------------------------------------------------------------------- + + +def _expire_now(layer: InMemoryCacheLayer, key: str) -> None: + """Backdate a resident entry's expiry so the next ``get()`` expires it. + + Deterministic stand-in for sleeping past a real TTL; the lazy-expiry branch + only compares ``expires_at`` against the wall clock, so this exercises the + identical code path without adding seconds to the suite. + """ + layer.cache[key].expires_at = datetime.now(timezone.utc) - timedelta(seconds=1) + + +class TestEvictionReleasesAccessHistory: + """``delete()`` already drops a key's access history (see + ``test_delete_cleans_access_patterns``). ``_evict_if_needed`` must do the + same: an evicted key leaves no cache entry behind, so nothing else will ever + reclaim its history.""" + + async def test_evicted_key_history_is_released(self): + layer = InMemoryCacheLayer("evict_one", max_size=1, max_size_bytes=1024 * 1024) + await layer.set("first", "value") + await layer.get("first") + assert "first" in layer.access_patterns + + # Inserting a second key evicts "first" (max_size=1). + await layer.set("second", "value") + + assert "first" not in layer.cache + assert "first" not in layer.access_patterns, ( + "history for an evicted key was retained; nothing will ever " + "reclaim it because the cache entry is already gone" + ) + + async def test_eviction_churn_leaves_no_orphaned_histories(self): + """The accumulating case: many keys cycle through a small cache. Every + key that is evicted must take its history with it, so the number of + tracked histories stays proportional to the number of resident keys.""" + layer = InMemoryCacheLayer("churn", max_size=5, max_size_bytes=1024 * 1024) + + for i in range(200): + key = f"k{i}" + await layer.set(key, "value") + await layer.get(key) + + orphans = set(layer.access_patterns) - set(layer.cache) + assert not orphans, ( + f"{len(orphans)} evicted keys still have access history retained " + f"while only {len(layer.cache)} entries are resident; this memory " + "is invisible to stats.total_size_bytes so the max_size_bytes " + "budget can never reclaim it" + ) + + async def test_resident_keys_keep_their_history(self): + """Releasing evicted histories must not disturb keys still in the + cache — the fix must be a narrow cleanup, not a blanket clear.""" + layer = InMemoryCacheLayer("keep", max_size=3, max_size_bytes=1024 * 1024) + + for i in range(10): + await layer.set(f"k{i}", "value") + await layer.get(f"k{i}") + + for key in layer.cache: + assert layer.access_patterns.get(key), ( + f"resident key {key!r} lost its access history; adaptive TTL " + "would fall back to the base value for a live key" + ) + + +class TestExpiryReleasesEntryBookkeeping: + """Lazy expiry in ``get()`` is the only place expired entries are ever + removed — there is no background sweeper — so it must release exactly what + ``delete()`` releases. Dropping the entry alone left the counters and the + access history describing an entry that no longer exists.""" + + async def test_expired_key_history_is_released(self): + layer = InMemoryCacheLayer( + "expiry_hist", max_size=100, max_size_bytes=1024 * 1024 + ) + await layer.set("k", "value", ttl=60) + + # History only accrues on a hit, so read the key while it is still + # live — otherwise there would be nothing to orphan. + await layer.get("k") + assert layer.access_patterns["k"], "precondition: history was recorded" + + _expire_now(layer, "k") + assert await layer.get("k") is None + + assert "k" not in layer.access_patterns, ( + "access history survived the entry it describes; with no sweeper " + "and no cache entry left, nothing can ever reclaim it" + ) + + async def test_expired_key_releases_size_accounting(self): + """``total_size_bytes`` is not merely reported — ``_evict_if_needed`` + budgets against it, so bytes left behind by expiry permanently reduce + the layer's usable capacity.""" + layer = InMemoryCacheLayer( + "expiry_bytes", max_size=100, max_size_bytes=1024 * 1024 + ) + await layer.set("k", "x" * 500, ttl=60) + + _expire_now(layer, "k") + assert await layer.get("k") is None + + assert layer.stats.total_size_bytes == 0, ( + f"cache holds {len(layer.cache)} entries but still reports " + f"{layer.stats.total_size_bytes} bytes in use; these phantom bytes " + "are charged against max_size_bytes forever" + ) + + async def test_expired_key_releases_entry_count(self): + layer = InMemoryCacheLayer( + "expiry_count", max_size=100, max_size_bytes=1024 * 1024 + ) + await layer.set("k", "value", ttl=60) + + _expire_now(layer, "k") + assert await layer.get("k") is None + + assert ( + layer.stats.total_entries == 0 + ), f"cache is empty but reports {layer.stats.total_entries} entries" + + async def test_reused_key_does_not_inherit_expired_history(self): + """A key that expires and is later written again is a *new* entry. If + the dead entry's timestamps survive, ``_calculate_adaptive_ttl`` reads + them as the new entry's access frequency and over-extends its TTL.""" + layer = InMemoryCacheLayer("reuse", max_size=100, max_size_bytes=1024 * 1024) + await layer.set("k", "value", ttl=60) + for _ in range(5): + await layer.get("k") + assert len(layer.access_patterns["k"]) == 5 + + _expire_now(layer, "k") + assert await layer.get("k") is None + + # Same key, brand-new entry. + await layer.set("k", "fresh", ttl=60) + + assert len(layer.access_patterns.get("k", [])) == 0, ( + "a freshly written entry inherited its expired predecessor's " + "access timestamps, so it is treated as an established hot key " + "from the moment it is created" + ) + + +class TestExpiredBytesDoNotConsumeCapacity: + """The user-visible consequence of the accounting leak: because + ``_evict_if_needed`` evicts while ``total_size_bytes`` exceeds the budget, + bytes that expiry never released push live entries out of the cache.""" + + async def test_capacity_survives_expiry_churn(self): + payload = "x" * 900 + + async def fill_and_count(with_expiry_churn: bool) -> int: + layer = InMemoryCacheLayer("cap", max_size=10_000, max_size_bytes=100_000) + if with_expiry_churn: + for i in range(50): + await layer.set(f"gone{i}", payload, ttl=60) + _expire_now(layer, f"gone{i}") + await layer.get(f"gone{i}") + for i in range(200): + await layer.set(f"live{i}", payload) + return sum(1 for key in layer.cache if key.startswith("live")) + + baseline = await fill_and_count(with_expiry_churn=False) + after_churn = await fill_and_count(with_expiry_churn=True) + + # Control: eviction is still doing its job in both runs, so this is a + # test of correct accounting and not of a disabled size limit. + assert baseline < 200, "precondition: the byte budget must force eviction" + + assert after_churn == baseline, ( + f"a layer that has seen expiry churn holds only {after_churn} live " + f"entries where a fresh layer of the same size holds {baseline}; " + "the expired entries' bytes are still charged against the budget" + ) + + +# ---------------------------------------------------------------------------- +# set() replacing an already-expired key +# ---------------------------------------------------------------------------- + + +class TestResetOfExpiredKeyStartsCleanHistory: + """``set()`` is a fourth way an entry stops existing. + + ``delete()``, eviction and lazy expiry all free the slot, so they route + through ``_release_entry``. ``set()`` instead *reuses* the slot, which is + why it needs its own handling: if the resident entry has already expired, + the value being installed is a brand-new entry that happens to share a key, + and it must not inherit the dead entry's access history. + + The distinction matters because ``access_patterns`` is the frequency signal + behind adaptive TTL -- inherited timestamps make a cold key look hot. + """ + + async def test_reset_after_expiry_drops_dead_history(self): + layer = InMemoryCacheLayer("reset_hist", max_size=100) + await layer.set("k", {"v": 1}, ttl=300) + for _ in range(5): + await layer.get("k") + + # Precondition: the live entry really did accumulate history, so a + # later empty result is the fix and not an artefact of never recording. + assert ( + len(layer.access_patterns["k"]) == 5 + ), "precondition: reads on a live entry must record access timestamps" + + _expire_now(layer, "k") + # Deliberately no get() and no delete() in between: this is the path + # where the caller overwrites a dead entry directly. + await layer.set("k", {"v": 2}, ttl=300) + + assert len(layer.access_patterns["k"]) == 0, ( + "a value written over an expired entry inherited " + f"{len(layer.access_patterns['k'])} timestamps from its dead " + "predecessor; the successor is a new entry and starts cold" + ) + + async def test_reset_of_live_key_keeps_history(self): + """Control: re-writing a *live* key is a genuine update, not a reuse.""" + layer = InMemoryCacheLayer("reset_live", max_size=100) + await layer.set("k", {"v": 1}, ttl=300) + for _ in range(5): + await layer.get("k") + + await layer.set("k", {"v": 2}, ttl=300) + + assert len(layer.access_patterns["k"]) == 5, ( + "overwriting a live key discarded its access history; only expired " + "entries should reset the frequency signal" + ) + + async def test_reset_after_expiry_keeps_entry_count_stable(self): + """The slot is reused, so the entry count must not move.""" + layer = InMemoryCacheLayer("reset_count", max_size=100) + await layer.set("k", {"v": 1}, ttl=300) + _expire_now(layer, "k") + await layer.set("k", {"v": 2}, ttl=300) + + assert layer.stats.total_entries == 1, ( + f"replacing an expired entry reported {layer.stats.total_entries} " + "entries for a single resident key" + ) + + async def test_adaptive_ttl_does_not_treat_reused_key_as_hot(self): + """End-to-end: the inherited history reached the TTL calculation.""" + from youtube_extension.backend.services.intelligent_cache import ( + IntelligentCacheSystem, + ) + + layer = InMemoryCacheLayer("reset_ttl", max_size=100) + await layer.set("k", {"v": 1}, ttl=300) + # 20 reads in a tight loop is the frequency profile that earns the + # hot-key TTL, so the inherited history is unambiguously load-bearing. + for _ in range(20): + await layer.get("k") + _expire_now(layer, "k") + await layer.set("k", {"v": 2}, ttl=300) + + system = IntelligentCacheSystem.__new__(IntelligentCacheSystem) + system.layers = [layer] + system.adaptive_ttl_enabled = True + system.cache_warming_enabled = False + system.auto_invalidation_enabled = False + system.performance_history = [] + system.optimization_suggestions = [] + + ttl = system._calculate_adaptive_ttl("k") + + assert ttl == 3600, ( + f"a key with no accesses since it was rewritten was given a {ttl}s " + "TTL instead of the 3600s base; it inherited its expired " + "predecessor's access timestamps and was scored as a hot key" + )