From eecb81e8478dbe4337a48dcdc2ace6537df0784f Mon Sep 17 00:00:00 2001 From: Abhay Singh Date: Mon, 13 Jul 2026 20:23:55 +0530 Subject: [PATCH] fix(cache/ccr): don't count a successful eviction as a retrieval (#2106) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Description The compression feedback learner treats a *successful* compression as evidence that it should compress less, which inverts the learning signal. When `CompressionStore` evicts an entry that was never retrieved, it emits a synthetic event to tell the learner the compression was fine (the model never needed the original): ```python success_event = RetrievalEvent(..., retrieval_type="eviction_success") self._pending_feedback_events.append(success_event) ``` `process_pending_feedback` forwards every pending event to `CompressionFeedback.record_retrieval` unconditionally. But `record_retrieval` has no branch for `"eviction_success"` — and since that string isn't `"full"`, it lands in the `else`: ```python self._total_retrievals += 1 pattern.total_retrievals += 1 if event.retrieval_type == "full": pattern.full_retrievals += 1 else: pattern.search_retrievals += 1 # <-- eviction_success counted here ``` So a compression that worked is booked as a *search retrieval*, which raises the tool's `retrieval_rate` and `search_rate`. `get_compression_hints` reads a high retrieval rate as "we're compressing too aggressively" and recommends larger `max_items` / lower aggressiveness (or `skip_compression`). Net effect: the more often compression succeeds, the more the learner backs off from compressing. A standalone repro books a single successful eviction as a 100% retrieval rate. Every sibling consumer of the event distinguishes the type — telemetry and TOIN both receive `retrieval_type="eviction_success"` and handle it as its own thing. Only the local feedback counter ignores the distinction. ## Fix Recognize `"eviction_success"` in `record_retrieval` and leave it out of the retrieval counters. The compression itself is already counted by `record_compression` at store time, so an entry that is compressed and never retrieved already yields a low retrieval rate — which is the correct "compression worked" signal. Genuine `full`/`search` retrievals are unchanged. 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/compression_feedback.py`: early-return in `record_retrieval` for `retrieval_type == "eviction_success"` so it is not counted as a retrieval, with a comment explaining the signal. - `tests/test_ccr_feedback.py`: add `test_eviction_success_is_not_counted_as_retrieval` (asserts the counters stay at zero after a successful eviction, and that a genuine retrieval afterward still counts). - `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/compression_feedback.py tests/test_ccr_feedback.py All checks passed! $ python -m py_compile headroom/cache/compression_feedback.py tests/test_ccr_feedback.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 counting logic with a dependency-free script that replicates `record_compression`/`record_retrieval` and the `retrieval_rate`/`search_rate` properties, and left the full pytest to CI. - Exact command / steps: recorded one compression, then a `retrieval_type="eviction_success"` event, under the old counting (no branch) and the new counting (early return), plus a genuine `search` retrieval as a control. - Observed result: old counting books the successful eviction as a retrieval — `retrieval_rate=1.0`, `search_rate=1.0` — so the learner would back off from compressing; new counting leaves `retrieval_rate=0.0` and `total_retrievals=0`; a real retrieval afterward still increments to 1. The new test asserts exactly this. - Not tested: an end-to-end store-evict-then-hint cycle through `CompressionStore.process_pending_feedback`; 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 fix is a single early-return in a pure counting method, verified by the standalone proof and the new regression test (which reuses the existing `test_ccr_feedback.py` pattern) for CI. Scope is deliberately limited to the local feedback learner — telemetry and TOIN already receive the `eviction_success` type and handle it separately. Co-authored-by: Tejas Chopra --- CHANGELOG.md | 1 + headroom/cache/compression_feedback.py | 13 ++++++++ tests/test_ccr_feedback.py | 44 ++++++++++++++++++++++++++ 3 files changed, 58 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 85b360cff..bb71d3f99 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:** stop counting a successful eviction as a retrieval in the compression feedback learner, which inverted the learning signal. When an entry is evicted without ever being retrieved, `CompressionStore` emits a synthetic `retrieval_type="eviction_success"` event to mark that the compression was sufficient (the LLM never needed the original). `CompressionFeedback.record_retrieval` had no branch for it, so — because the type is not `"full"` — it was counted as a *search retrieval*, inflating the tool's `retrieval_rate`/`search_rate`. `get_compression_hints` reads a high retrieval rate as "compressing too aggressively" and backs off, so a compression that actually worked pushed the learner toward *less* compression (a standalone repro scores one successful eviction as a 100% retrieval rate). The event is now recognized and left out of the retrieval counters; the compression is still counted by `record_compression` at store time, so a never-retrieved entry correctly yields a low retrieval rate. Genuine retrievals are unaffected. * **proxy/anthropic:** don't launder a non-2xx upstream into HTTP 200 when enterprise security scans the response. On the non-streaming `/v1/messages` path the response-scan branch rebuilt the reply as `httpx.Response(status_code=200)` and returned it without checking the upstream status, so a rate-limit (429), overloaded (529), or other 4xx error whose JSON body was scanned reached the client as an HTTP 200 — the client's retry/backoff never fired and an error looked like success. The branch is now gated on a 200 upstream, matching the sibling CCR/cache/buffered-stream blocks in the same handler; non-2xx responses fall through and keep their real status. * **learn:** classify timeout and connection tool failures correctly instead of as generic runtime errors. In `classify_error` the generic `RUNTIME_ERROR` pattern (`Traceback|Exception:|Error:`) was checked before the dedicated `TIMEOUT` and `CONNECTION_ERROR` patterns. Because every Python exception repr is `XxxError: ...`, a `TimeoutError: ...` or `ConnectionError: ...` matched the catch-all first and was miscategorized as `RUNTIME_ERROR`, leaving those two categories unreachable for the common colon-repr form (they only fired for tokenless phrasings like `deadline exceeded`). The `TIMEOUT` and `CONNECTION_ERROR` patterns are now checked before the generic catch-all; tokenless generic errors still classify as `RUNTIME_ERROR`. * **tokenizers:** resolve HuggingFace tokenizer names by the most-specific prefix. `get_tokenizer_name` scanned `MODEL_TO_TOKENIZER` in dict-insertion order and returned the first key the model merely starts with, so a short family key shadowed a more-specific one — `qwen2-7b-instruct` matched `qwen` before `qwen2`/`qwen2-7b` and resolved to the Qwen1 tokenizer (a different vocabulary, hence wrong token counts); `qwen2.5-*` and `deepseek-v2.x` were mis-resolved the same way. It now picks the longest matching prefix, mirroring the order-dependent-prefix guard the sibling tiktoken `get_encoding_for_model` already documents. diff --git a/headroom/cache/compression_feedback.py b/headroom/cache/compression_feedback.py index e4a320444..c5fdf7de0 100644 --- a/headroom/cache/compression_feedback.py +++ b/headroom/cache/compression_feedback.py @@ -273,6 +273,19 @@ class CompressionFeedback: if not tool_name: return + # An entry evicted without ever being retrieved is a compression + # SUCCESS, not a retrieval: the LLM never needed the original data (see + # CompressionStore._record_eviction_success). It arrives here as + # retrieval_type="eviction_success"; because that is not "full" it used + # to fall into the search_retrievals branch below and inflate + # retrieval_rate/search_rate, which drove get_compression_hints toward + # LESS aggressive compression -- the inverse of the intended signal. The + # compression itself was already counted by record_compression at store + # time, so a never-retrieved entry already yields a low retrieval rate; + # this event must not be counted as a retrieval. + if event.retrieval_type == "eviction_success": + return + with self._lock: self._total_retrievals += 1 diff --git a/tests/test_ccr_feedback.py b/tests/test_ccr_feedback.py index 077a495e4..5cd8ab1e5 100644 --- a/tests/test_ccr_feedback.py +++ b/tests/test_ccr_feedback.py @@ -86,6 +86,50 @@ class TestCompressionFeedback: assert pattern.retrieval_rate == 0.5 assert pattern.full_retrieval_rate == 1.0 # All were full retrievals + def test_eviction_success_is_not_counted_as_retrieval(self): + """An eviction-without-retrieval is a compression success, not a retrieval. + + The event arrives with retrieval_type="eviction_success". Because that + isn't "full" it used to fall into the search_retrievals branch and + inflate retrieval_rate/search_rate, driving get_compression_hints toward + less aggressive compression — the inverse of the intended signal. It must + leave the retrieval counters untouched. + """ + feedback = CompressionFeedback() + feedback.record_compression("test_tool", 100, 10) + + event = RetrievalEvent( + hash="abc123", + query=None, + items_retrieved=0, + total_items=100, + tool_name="test_tool", + timestamp=time.time(), + retrieval_type="eviction_success", + ) + feedback.record_retrieval(event, strategy="smart") + + pattern = feedback.get_all_patterns()["test_tool"] + assert pattern.total_retrievals == 0 + assert pattern.search_retrievals == 0 + assert pattern.retrieval_rate == 0.0 # a successful compression, not a retrieval + + # A genuine retrieval afterward is still counted. + feedback.record_retrieval( + RetrievalEvent( + hash="def456", + query="find errors", + items_retrieved=50, + total_items=100, + tool_name="test_tool", + timestamp=time.time(), + retrieval_type="search", + ) + ) + pattern = feedback.get_all_patterns()["test_tool"] + assert pattern.total_retrievals == 1 + assert pattern.search_retrievals == 1 + def test_hints_default_with_no_data(self): """Default hints returned when no data exists.""" feedback = CompressionFeedback()