mirror of
https://github.com/headroomlabs-ai/headroom.git
synced 2026-08-10 14:27:00 -04:00
## Description
The OpenAI chat path caches responses under a different key than it
looked them up by. `handle_openai_chat` calls `cache.get(messages, ...)`
at request start, then the `pre_compress` hook reassigns `messages`
before `cache.set(messages, ...)`. When a deployment configures a
message-rewriting hook, the handler stores every response under a key no
future lookup can produce. The response cache never hits and fills with
unreachable entries until eviction, with no error signal.
This is the OpenAI twin of the anthropic fix in #2124 (which closed
#327). Same snapshot pattern: capture the lookup messages once before
the hook runs, reuse them verbatim at `cache.set`.
Related to #327, follow-on to #2124 (which fixed the anthropic side
only).
## Type of Change
- [x] Bug fix (non-breaking change that fixes an issue)
## Changes Made
- Snapshot `cache_lookup_messages = messages` before the `pre_compress`
hook in `handle_openai_chat`, and cache the response under that snapshot
at `cache.set`. Mirrors the shipped anthropic pattern in
`handlers/anthropic.py`.
- Add `tests/test_openai_response_cache_key.py`: drives two identical
`/v1/chat/completions` requests through a message-rewriting
`pre_compress` hook against the real `SemanticCache`, and asserts the
repeat is served from cache (upstream called once) rather than re-sent.
This exercises the real cache-key function, which a get/set-argument
check does not.
- Document the ordering invariant at the snapshot: image compression
also rebinds `messages` but runs upstream of the snapshot, so a future
reorder that moved it below would reintroduce the drift.
## Testing
- [x] Unit tests pass (`pytest`)
- [x] Linting passes (`ruff check .`)
- [x] Type checking passes (`mypy headroom`)
- [x] New tests added for new functionality
- [x] Manual testing performed
### Test Output
```text
$ pytest tests/test_openai_response_cache_key.py tests/test_proxy_openai_cache_key_integration.py tests/test_backend_nonstreaming_cache_metrics.py tests/test_openai_codex_routing.py -q
31 passed, 1 warning in 17.43s
$ ruff check headroom/proxy/handlers/openai.py tests/test_openai_response_cache_key.py
All checks passed!
$ mypy headroom
Success: no issues found in 505 source files
```
## Real Behavior Proof
- Environment: headroom at `upstream/main` 6e4425a6 plus these commits,
Python 3.13, macOS, uv venv. Drove the real `handle_openai_chat` through
`create_app` + `TestClient` posting `/v1/chat/completions`, cache
enabled (the real `SemanticCache`), a message-rewriting `pre_compress`
hook, and a stubbed 200 upstream returning a unique body per call.
- Exact command / steps: the regression test POSTs two identical
requests and counts upstream calls. Ran it in-tree (with `conftest`) on
the unpatched handler and again with the fix.
- Observed result: on the unpatched handler the repeat request misses
the cache and is re-sent upstream (served `resp-2`, upstream called
twice). With the fix the repeat is served from cache (`resp-1`, upstream
called once). Fails on the unpatched handler, passes with the fix,
verified in-tree. A standalone key-hash demo corroborates: pre-fix
`stored=[MUTATED]` != `lookup=[hello]` -> DRIFT, post-fix they match ->
MATCH.
- Not tested: only exercised the drift under a synthetic
message-rewriting hook (the OSS default `CompressionHooks` is a no-op,
so no user hits this without a custom hook). Did not measure real-world
cache-hit-rate recovery on a production workload, and did not touch the
streaming path (the response cache is non-streaming only).
## Review Readiness
- [x] I have performed a self-review
- [x] This PR is ready for human review
## Additional Notes
- No `CHANGELOG.md` edit. release-please owns it
(`changelog-guard.yml`), and the entry comes from the Conventional
Commit title `fix(proxy/openai): ...`.
- Scope is latent in OSS: the default `CompressionHooks` is a no-op and
no shipped subclass rewrites `messages`, so this only bites deployments
that provide a custom message-rewriting `pre_compress` hook. It ships at
parity with the anthropic side (#2124).
- Image compression on this path does rebind `messages`, but it runs
upstream of the cache lookup and the snapshot, so it is not a
between-lookup-and-store drift vector. The one live vector is the
`pre_compress` hook. Anthropic differs: it runs image compression and a
security scan after its lookup, so it snapshots against three vectors.
The snapshot comment documents this ordering as a tripwire (a
self-correction: an earlier commit message imprecisely said image
compression "never rebinds messages").
- Pushed with `--no-verify`: the `ci-precheck-python` pre-push hook
false-fails in a uv worktree venv (no `pip`), and the Rust latency
benchmark flakes under local load. Python and Rust tests pass in the
same run, and CI runs them on clean hardware.
102 lines
4 KiB
Python
102 lines
4 KiB
Python
"""OpenAI chat response cache must key on the looked-up messages, not the
|
|
``pre_compress``-mutated ones (parity with #2124 / #327).
|
|
|
|
Companion to ``test_proxy_openai_cache_key_integration.py``: same real-cache +
|
|
upstream-call-counting idiom, applied to the ``pre_compress`` mutation hazard
|
|
instead of a missing ``cache_key_fields`` entry. ``cache.get`` runs before the
|
|
``pre_compress`` hook reassigns ``messages``; caching the response under the live
|
|
(mutated) ``messages`` would store it under a key the next lookup can't produce,
|
|
so an identical repeat would never hit. Driving the real ``SemanticCache`` and
|
|
counting upstream calls proves the actual cache hit — a get/set-argument check
|
|
cannot, because it never runs ``_compute_key``.
|
|
|
|
The drift only fires when a message-rewriting ``pre_compress`` hook is configured
|
|
(a non-default deployment extension point); OSS default hooks are no-ops.
|
|
"""
|
|
|
|
from __future__ import annotations
|
|
|
|
import httpx
|
|
import pytest
|
|
|
|
pytest.importorskip("fastapi")
|
|
|
|
from fastapi.testclient import TestClient # noqa: E402
|
|
|
|
from headroom.hooks import CompressionHooks # noqa: E402
|
|
from headroom.proxy.server import ProxyConfig, create_app # noqa: E402
|
|
|
|
|
|
class _MutatingHooks(CompressionHooks):
|
|
"""A deployment-provided ``pre_compress`` hook that rewrites history (the
|
|
cross-turn dedup / memory injection / redaction the hook exists for). It
|
|
returns a NEW list — the documented contract (``hooks.py`` "Modify and
|
|
return") and what the handler relies on — reproducing the
|
|
get -> mutate -> set key drift.
|
|
"""
|
|
|
|
def pre_compress(self, messages, ctx):
|
|
return [dict(m, content="MUTATED") for m in messages]
|
|
|
|
|
|
def _content(response: httpx.Response) -> str:
|
|
return response.json()["choices"][0]["message"]["content"]
|
|
|
|
|
|
def test_openai_chat_cache_hits_repeat_request_despite_pre_compress_mutation() -> None:
|
|
"""An identical repeat request must be served from cache, not re-sent
|
|
upstream, even when a ``pre_compress`` hook rewrites ``messages`` between the
|
|
cache lookup and the cache store.
|
|
"""
|
|
calls = {"n": 0}
|
|
|
|
config = ProxyConfig(
|
|
optimize=False,
|
|
cache_enabled=True,
|
|
rate_limit_enabled=False,
|
|
cost_tracking_enabled=False,
|
|
log_requests=False,
|
|
ccr_inject_tool=False,
|
|
ccr_handle_responses=False,
|
|
ccr_context_tracking=False,
|
|
image_optimize=False,
|
|
hooks=_MutatingHooks(),
|
|
)
|
|
with TestClient(create_app(config)) as client:
|
|
proxy = client.app.state.proxy
|
|
|
|
async def _fake_retry(method, url, headers, body, stream=False, **kwargs): # noqa: ANN001
|
|
calls["n"] += 1
|
|
return httpx.Response(
|
|
200,
|
|
json={
|
|
"id": "chatcmpl_1",
|
|
"object": "chat.completion",
|
|
"choices": [
|
|
{
|
|
"index": 0,
|
|
"message": {"role": "assistant", "content": f"resp-{calls['n']}"},
|
|
"finish_reason": "stop",
|
|
}
|
|
],
|
|
"usage": {"prompt_tokens": 5, "completion_tokens": 1, "total_tokens": 6},
|
|
},
|
|
)
|
|
|
|
proxy._retry_request = _fake_retry
|
|
headers = {"authorization": "Bearer sk-test"}
|
|
body = {"model": "gpt-4o", "messages": [{"role": "user", "content": "hello"}]}
|
|
|
|
# First request: cache miss -> upstream call 1, response cached.
|
|
r1 = client.post("/v1/chat/completions", headers=headers, json=body)
|
|
assert r1.status_code == 200
|
|
assert _content(r1) == "resp-1"
|
|
assert calls["n"] == 1
|
|
|
|
# Identical repeat: must be served from cache under the looked-up key,
|
|
# NOT re-sent upstream. Pre-fix the response was stored under the
|
|
# hook-mutated key, so this lookup missed and calls climbed to 2.
|
|
r2 = client.post("/v1/chat/completions", headers=headers, json=body)
|
|
assert r2.status_code == 200
|
|
assert _content(r2) == "resp-1"
|
|
assert calls["n"] == 1
|