diff --git a/CHANGELOG.md b/CHANGELOG.md index da5e62d23..de9c10ce2 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/ccr:** don't evict a live entry when a duplicate hash is re-stored at capacity. `CompressionStore.store` ran `_evict_if_needed()` before checking whether the key already existed, so re-storing an already-present hash while the store was full evicted the oldest *distinct* entry to "make room" and then merely overwrote the existing key in place — no room was ever needed. The store dropped below `max_entries` and a live, never-retrieved entry was destroyed, so its `<>` marker (still in the conversation) resolved to a 404. The CCR mirror bridge re-stores the same `explicit_hash` every turn a marker is re-encountered, so this fired routinely. Eviction now runs only for a genuinely new key. * **tokenizers:** recurse into a native `tool_result` whose content is a list of blocks instead of JSON-serializing it. `_count_content_parts` counted a `tool_result` with list content via `_count_serialized` (json.dumps + sample), so a base64 image nested in a tool result (computer-use / MCP screenshot tools) was priced as text — a ~50-200x overcount (a ~200KB screenshot read as ~70K tokens instead of ~1600). It now recurses into the nested blocks, matching the sibling Strands `toolResult` branch, so the image is priced structurally. The overcount made a single screenshot appear to blow past the model's context window and triggered unnecessary/over-aggressive compression. * **tokenizers:** price dense scripts (CJK/Kana/Hangul) in the fixed-ratio estimator path. `EstimatingTokenCounter.count_text` applied the CJK correction only on the auto-detect path; its fixed-ratio early return divided by the Latin ratio with no adjustment. The registry builds every provider-calibrated counter with a fixed ratio (Anthropic 3.5, Google 4.0, Cohere 4.0, Moonshot 3.1), and the Anthropic/Gemini proxy handlers count via `get_tokenizer(model).count_messages`, so a CJK-heavy context read as ~40-55% of its true token size — it could fall under the size/backpressure gates and skip compression, and every `x-headroom-tokens-before` metric for CJK traffic was materially wrong. The fixed-ratio path now applies the same dense-script split. * **ccr:** don't crash `parse_tool_call` on a CCR tool call whose arguments aren't an object. For the OpenAI/`openai_responses` shape the arguments are `json.loads`-decoded and only `JSONDecodeError` was caught, so a model that emitted `arguments='[]'`/`'"abc"'`/`'123'` (decoding to a list/str/number) — or a non-dict Anthropic `input` — reached `input_data.get("hash")` and raised `AttributeError`; a null `arguments` raised an uncaught `TypeError` from `json.loads(None)`. Both are now handled: the decode also catches `TypeError`, and a non-dict `input_data` returns `None` (not a valid CCR call) instead of crashing CCR response processing. diff --git a/headroom/cache/compression_store.py b/headroom/cache/compression_store.py index 84d3e2cf6..f78fedcbb 100644 --- a/headroom/cache/compression_store.py +++ b/headroom/cache/compression_store.py @@ -347,16 +347,22 @@ class CompressionStore: self.process_pending_feedback() with self._lock: - self._evict_if_needed() - - # CRITICAL FIX: Hash collision detection - # If hash already exists with DIFFERENT content, log a warning. - # This indicates either a hash collision or duplicate store calls. + # Decide whether this is a NEW key before evicting. Evicting to make + # room only applies to a genuinely new entry; a re-store of an + # existing key overwrites in place (no room needed). Evicting first + # for a duplicate would needlessly destroy a live, unrelated entry + # and drop the store below capacity, making that entry's <> + # marker (still sitting in the conversation) unredeemable — a 404. + # The CCR mirror bridge re-stores the same explicit_hash on every + # turn a marker is re-encountered, so duplicate stores are common. existing = self._backend.get(hash_key) - if existing is not None: + if existing is None: + self._evict_if_needed() + else: + # Hash already present. Different content means a true (extremely + # rare with SHA256[:24]) collision; same content is a duplicate + # re-store. Either way we overwrite in place. if existing.original_content != original: - # True hash collision - different content, same hash - # This is extremely rare with SHA256[:24] but should be logged logger.warning( "Hash collision detected: hash=%s tool=%s (existing_len=%d, new_len=%d)", hash_key, @@ -365,12 +371,11 @@ class CompressionStore: len(original), ) else: - # Same content being stored again - this is fine, just update logger.debug( "Duplicate store for hash=%s, updating entry", hash_key, ) - # Mark old heap entry as stale since we're replacing + # Mark old heap entry as stale since we're replacing it. self._stale_heap_entries += 1 self._backend.set(hash_key, entry) diff --git a/tests/test_compression_store.py b/tests/test_compression_store.py index 112c972d5..7b9168245 100644 --- a/tests/test_compression_store.py +++ b/tests/test_compression_store.py @@ -706,6 +706,32 @@ class TestCompressionStoreEviction: assert store_with_small_capacity.exists(hashes[2]) assert store_with_small_capacity.exists(new_hash) + def test_duplicate_store_at_capacity_does_not_evict( + self, store_with_small_capacity: CompressionStore + ): + """Re-storing an already-present hash at capacity overwrites in place and + must NOT evict an unrelated live entry (which would drop below capacity + and make that entry's marker unredeemable). The CCR mirror bridge + re-stores the same hash on later turns, so this is a common path.""" + hashes = [] + for i in range(3): + hashes.append( + store_with_small_capacity.store( + original=f"content_{i}", compressed=f"compressed_{i}" + ) + ) + time.sleep(0.01) + assert store_with_small_capacity.get_stats()["entry_count"] == 3 + + # Re-store the SAME content for the oldest entry (a duplicate -> same hash). + dup = store_with_small_capacity.store(original="content_0", compressed="compressed_0") + assert dup == hashes[0] + + # No eviction happened: all three entries survive and count stays at 3. + for h in hashes: + assert store_with_small_capacity.exists(h) + assert store_with_small_capacity.get_stats()["entry_count"] == 3 + def test_eviction_cleans_expired_first(self): """Eviction cleans expired entries before evicting valid ones.""" store = CompressionStore(max_entries=3, default_ttl=1)