From 8ef956755bb1008f541c9eace79760556092faa1 Mon Sep 17 00:00:00 2001 From: Hayden <154503486+groupthinking@users.noreply.github.com> Date: Mon, 3 Aug 2026 17:14:56 -0500 Subject: [PATCH 1/2] perf: release full entry bookkeeping on all L1 removal paths InMemoryCacheLayer had three paths that remove an entry, and only delete() released everything the entry owned. Eviction left the key's access history behind; lazy expiry in get() left the access history and both size counters behind. total_size_bytes is budgeted against by _evict_if_needed, so bytes that expiry never released were charged against max_size_bytes forever. A 100 KB layer that has seen 50 entries expire retains 60 live entries instead of 110 -- a 45% capacity loss that never recovers, because the layer has no background sweeper and that branch is the only place expiry is ever handled. Route all three paths through a single private _release_entry() helper so that omitting a step is structurally impossible rather than something each future removal path has to remember. delete() is behaviour-identical and stays covered by its existing test. Closes #1297 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../backend/services/intelligent_cache.py | 38 ++-- tests/unit/test_intelligent_cache.py | 185 ++++++++++++++++++ 2 files changed, 212 insertions(+), 11 deletions(-) diff --git a/src/youtube_extension/backend/services/intelligent_cache.py b/src/youtube_extension/backend/services/intelligent_cache.py index a92cbe171..8f19240d6 100644 --- a/src/youtube_extension/backend/services/intelligent_cache.py +++ b/src/youtube_extension/backend/services/intelligent_cache.py @@ -150,7 +150,10 @@ async def get(self, key: str) -> Optional[Any]: # Check expiration if entry.expires_at and datetime.now(timezone.utc) > entry.expires_at: - del self.cache[key] + # 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 @@ -215,18 +218,33 @@ 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 + 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. + + ``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 +272,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..d67bdcb79 100644 --- a/tests/unit/test_intelligent_cache.py +++ b/tests/unit/test_intelligent_cache.py @@ -1841,3 +1841,188 @@ 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" + ) From 481969fd29af4cec548ab4529415aa22c005b2f5 Mon Sep 17 00:00:00 2001 From: Hayden <154503486+groupthinking@users.noreply.github.com> Date: Mon, 3 Aug 2026 17:31:52 -0500 Subject: [PATCH 2/2] perf: clear access history when overwriting an expired key InMemoryCacheLayer.set() replaced an existing entry without consulting its TTL. When the key had already expired, the successor inherited the dead entry's access timestamps, so a brand-new value presented to _calculate_adaptive_ttl as a hot key and was granted 14400s instead of the 3600s base TTL. This was the fourth path that stops an entry from existing. The other three (delete, eviction, lazy expiry) release bookkeeping via _release_entry; replacement reuses the slot, so total_entries must not move and only access_patterns is dropped. Adds a private _is_expired() predicate so the expiry check in get() and the new one in set() cannot drift apart. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../backend/services/intelligent_cache.py | 22 +++- tests/unit/test_intelligent_cache.py | 109 +++++++++++++++++- 2 files changed, 124 insertions(+), 7 deletions(-) diff --git a/src/youtube_extension/backend/services/intelligent_cache.py b/src/youtube_extension/backend/services/intelligent_cache.py index 8f19240d6..d4de006be 100644 --- a/src/youtube_extension/backend/services/intelligent_cache.py +++ b/src/youtube_extension/backend/services/intelligent_cache.py @@ -149,7 +149,7 @@ 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: + 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. @@ -204,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 @@ -218,6 +223,17 @@ 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. @@ -228,6 +244,10 @@ def _release_entry(self, key: str, entry: CacheEntry) -> None: 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. diff --git a/tests/unit/test_intelligent_cache.py b/tests/unit/test_intelligent_cache.py index d67bdcb79..d1c7a36c3 100644 --- a/tests/unit/test_intelligent_cache.py +++ b/tests/unit/test_intelligent_cache.py @@ -1966,9 +1966,9 @@ async def test_expired_key_releases_entry_count(self): _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" - ) + 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 @@ -2002,9 +2002,7 @@ 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 - ) + 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) @@ -2026,3 +2024,102 @@ async def fill_and_count(with_expiry_churn: bool) -> int: 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" + )