From cf6367add4fce3294a790bd1a6733c97e62072d9 Mon Sep 17 00:00:00 2001 From: Abhay Singh Date: Mon, 13 Jul 2026 20:20:23 +0530 Subject: [PATCH] fix(cache/semantic): don't evict an unrelated entry on an update at capacity (#2094) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Description `SemanticCache.put` can evict a perfectly good, unrelated entry when it merely updates a key that is already cached. The method runs its at-capacity eviction loop *before* it computes the entry's key: ```python self._cleanup_expired() # Evict if at capacity while len(self._cache) >= self.config.max_entries: self._evict_oldest() ... key = messages_hash or self._generate_key(query) ... self._cache[key] = entry ``` So when the same key is stored again while the cache is full (a duplicate store, or a retried request that produces the same `messages_hash`), the loop fires because `len == max_entries`, evicts the LRU-oldest *distinct* entry, and only then overwrites the existing key in place. Writing to an already-present key does not grow the map, so nothing needed to be evicted — but an unrelated live entry is now gone, and the next `get` for it is a false miss. Concretely, with `max_entries=2` and keys `[h1, h2]`, re-storing `h2` evicts `h1`, leaving `[h2]` even though only two distinct keys were ever stored. The sibling `CompressionCache.store_compressed` gets this right: it deletes the existing key first, inserts, and only then trims — so re-storing a present key never drops an unrelated entry. ## Fix Compute the key first, then run the eviction loop only while the key is genuinely new: ```python key = messages_hash or self._generate_key(query) while key not in self._cache and len(self._cache) >= self.config.max_entries: self._evict_oldest() ``` An in-place update of an existing key no longer evicts anything; adding a new key still trims to make room exactly as before. Closes # ## Type of Change - [x] Bug fix (non-breaking change that fixes an issue) - [ ] New feature (non-breaking change that adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to change) - [ ] Documentation update - [ ] Performance improvement - [ ] Code refactoring (no functional changes) ## Changes Made - `headroom/cache/semantic.py`: move the cache-key computation above the eviction loop and gate the loop on `key not in self._cache` so an in-place update never evicts. - `tests/test_cache/test_semantic.py`: add `test_update_at_capacity_does_not_evict_unrelated_entry`. - `CHANGELOG.md`: Bug Fixes entry. ## Testing - [ ] Unit tests pass (`pytest`) - [x] Linting passes (`ruff check .`) - [ ] Type checking passes (`mypy headroom`) - [x] New tests added for new functionality - [ ] Manual testing performed ### Test Output ```text $ uvx ruff@0.15.17 check headroom/cache/semantic.py tests/test_cache/test_semantic.py All checks passed! $ python -m py_compile headroom/cache/semantic.py tests/test_cache/test_semantic.py OK ``` ## Real Behavior Proof - Environment: Windows 11, Python 3.12, `uvx ruff@0.15.17`. Importing `headroom` pulls in the torch/transformers stack and a full `pytest` gets OOM-killed on this box, so I verified the eviction logic with a dependency-free script that replicates the `OrderedDict` + `_evict_oldest` (popitem last=False) behavior for the old vs new loop, and left the full pytest to CI. - Exact command / steps: with `max_entries=2`, store `h1` then `h2`, then re-store the already-present `h2`, under both the old loop (evict before key dedup) and the new loop (evict only when key is new). - Observed result: old loop leaves `['h2']` and `get(h1)` returns `None` (h1 wrongly evicted); new loop leaves `['h1', 'h2']` with `get(h1)` intact and `h2` updated. The regression test asserts h1 survives and h2 reflects the update. - Not tested: a live embedding-backed cache round-trip; full local `pytest` deferred to CI (OOM, per above). ## Review Readiness - [x] I have performed a self-review - [x] This PR is ready for human review ## Checklist - [x] My code follows the project's style guidelines - [x] I have performed a self-review of my code - [x] I have commented my code, particularly in hard-to-understand areas - [ ] I have made corresponding changes to the documentation - [x] My changes generate no new warnings - [x] I have added tests that prove my fix is effective or that my feature works - [ ] New and existing unit tests pass locally with my changes - [x] I have updated the CHANGELOG.md if applicable ## Additional Notes The "unit tests pass locally" and "type checking" boxes are unchecked because the full suite imports the ML stack, which I can't run in this environment; the change is a localized reordering of two existing statements plus a loop guard, verified by the standalone proof and the new regression test for CI. This is a different defect from the earlier messages-hash keying fix — that one was about which slot a request maps to; this one is about eviction dropping a live entry on an in-place update. Co-authored-by: Tejas Chopra --- CHANGELOG.md | 1 + headroom/cache/semantic.py | 23 ++++++++++++++--------- tests/test_cache/test_semantic.py | 25 +++++++++++++++++++++++++ 3 files changed, 40 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f161cf555..f5f7811f7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -102,6 +102,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Bug Fixes +* **cache/semantic:** don't evict an unrelated entry when re-storing a key that is already cached. `SemanticCache.put` ran its at-capacity eviction loop before computing the entry's key, so overwriting a key that was already present (a duplicate or retried store) still evicted the LRU-oldest distinct entry even though an in-place update grows nothing. That silently dropped a live entry and turned a later lookup for it into a false cache miss. The key is now computed first and the eviction loop only runs when the key is genuinely new (mirroring `CompressionCache.store_compressed`, which deletes-then-inserts). * **tokenizers:** stop `TiktokenCounter.count_messages` from exploding on non-text content blocks. Its multi-part branch handled only `text` and OpenAI `image_url`; every other shape (Anthropic `image`/`tool_result`/`tool_use`, Strands blocks) fell through to `count_text(str(part))`, which json-stringified the base64 payload and tokenized it as text — a 1MB image counted as ~330K phantom tokens (~218x overcount in a standalone repro), corrupting every downstream budgeting/compression decision for multimodal OpenAI-model requests. Unknown block shapes now delegate to the base `_count_content_parts`, which prices images/documents by a bounded estimate (the overcount that helper already exists to prevent). * **install:** don't let a host env export override the manifest in persistent-docker deployments. `build_runtime_command` emitted the manifest's pinned `--env NAME=VALUE` pairs and then, for every host var matching a passthrough prefix, a bare `--env NAME`. Docker resolves duplicate `--env` last-wins, so a stale host export (e.g. `HEADROOM_BACKEND=anyllm`) that shared a passthrough prefix with a pinned manifest value (`HEADROOM_BACKEND=anthropic`) was appended after it and silently won, diverging the container from its deployment config. The bare passthrough is now skipped for any name the manifest already pins. * **memory:** honor explicit `store=false` on OpenAI `/v1/responses` requests by skipping Headroom memory-tool injection that depends on stored-response continuations. Memory context injection stays available, and requests no longer get rewritten to `store=true` behind the client's back ([#1944](https://github.com/headroomlabs-ai/headroom/issues/1944)). diff --git a/headroom/cache/semantic.py b/headroom/cache/semantic.py index bcf968a03..0ea2effba 100644 --- a/headroom/cache/semantic.py +++ b/headroom/cache/semantic.py @@ -192,15 +192,6 @@ class SemanticCache: """ self._cleanup_expired() - # Evict if at capacity - while len(self._cache) >= self.config.max_entries: - self._evict_oldest() - - # Generate embedding if available - embedding: list[float] = [] - if self._embedding_fn: - embedding = self._embedding_fn(query) - # Create cache key. Prefer the full-context hash: two requests that share # a trailing user message ("continue", "yes", "run the tests") but differ # in earlier context must NOT collide on one query-derived slot and @@ -208,6 +199,20 @@ class SemanticCache: # messages_hash is supplied (e.g. embedding-only usage). key = messages_hash or self._generate_key(query) + # Evict if adding a NEW key would exceed capacity. Overwriting a key that + # is already present is an in-place update that does not grow the map, so + # it must NOT evict — the old code ran the eviction loop before computing + # the key, so re-storing an existing entry at capacity dropped an + # unrelated live entry and turned a later lookup for it into a false miss. + # (Mirrors CompressionCache.store_compressed, which deletes-then-inserts.) + while key not in self._cache and len(self._cache) >= self.config.max_entries: + self._evict_oldest() + + # Generate embedding if available + embedding: list[float] = [] + if self._embedding_fn: + embedding = self._embedding_fn(query) + now = time.time() entry = CacheEntry( embedding=embedding, diff --git a/tests/test_cache/test_semantic.py b/tests/test_cache/test_semantic.py index 8cbb15da9..c24d261f9 100644 --- a/tests/test_cache/test_semantic.py +++ b/tests/test_cache/test_semantic.py @@ -96,6 +96,31 @@ class TestSemanticCache: assert cache.get("query3", messages_hash="h3") is not None assert cache.get("query4", messages_hash="h4") is not None + def test_update_at_capacity_does_not_evict_unrelated_entry(self): + """Re-storing an existing key at capacity must not drop another entry. + + The eviction loop used to run before the cache key was computed, so + overwriting a key that was already present (a retried/duplicate store) + still evicted the LRU-oldest distinct entry even though the update grows + nothing. That silently dropped a live entry and turned a later lookup for + it into a false miss. + """ + config = SemanticCacheConfig(max_entries=2) + cache = SemanticCache(config) + + cache.put("query1", "response1", messages_hash="h1") + cache.put("query2", "response2", messages_hash="h2") + + # Re-store the already-present h2 (e.g. a duplicate/retried request). + cache.put("query2", "response2b", messages_hash="h2") + + # h1 must still be there — updating h2 must not evict it. + got1 = cache.get("query1", messages_hash="h1") + assert got1 is not None and got1.response == "response1" + # h2 reflects the update. + got2 = cache.get("query2", messages_hash="h2") + assert got2 is not None and got2.response == "response2b" + def test_ttl_expiration(self): """Test TTL expiration.""" config = SemanticCacheConfig(ttl_seconds=1)