fix(proxy): stop cached responses replaying the producing turn's wire framing - #3024
fix(proxy): stop cached responses replaying the producing turn's wire framing#3024Parideboy wants to merge 2 commits into
Conversation
… framing A response-cache hit rebuilt the client's response from the cached entry's stored headers, dropping only content-encoding, content-length and content-type. Everything else described how the *producing* upstream framed its body on a different connection, and transfer-encoding: chunked is the damaging one: RFC 9112 6.1 makes Transfer-Encoding override Content-Length, so the client parses a plain JSON body as chunked frames, finds no valid chunk-size line, and reads an empty body out of an HTTP 200. Move the OpenAI handler's private sanitiser into headroom/proxy/helpers.py, add the framing headers it was missing (transfer-encoding, connection, keep-alive), and use it at both cache-hit sites so they strip the same set as every other forwarding site. One OpenAI call site already passed transfer-encoding and connection by hand; that gap is now closed centrally. Also gate the Anthropic cache.set on the client's original stream flag, mirroring the read side. The cache key has no stream component, and CCR's buffered-stream conversion is the one path that reaches the store site with a client stream:true turn: it forces stream:false upstream to buffer and rewrite the reply, then falls through past the streaming return. The stored body was shaped by that flip plus CCR tool injection, so serving it to a later non-streaming caller answers a request that caller never made. Finally, make the hit visible. A cache-served turn contacts no upstream, so it has no outbound_request line, no upstream stage timings and all-zero token counters, which is byte-for-byte what a dead turn looks like. Log RESPONSE-CACHE-HIT and append cached=1 to the PERF line (only on a hit, so existing parsers are unaffected); headroom perf reads it back. The new PerfRecord field is appended to PERF_RECORD_FIELDS as well, since headroom perf --format csv --raw uses that list as its DictWriter column set; appending keeps every existing column in its current position. And the backpressure test's cache-hit double was a partial stand-in for CacheEntry carrying only a body and headers, which broke once the hit path started reading the entry's age and hit count, so it now builds a real CacheEntry. Closes headroomlabs-ai#3019 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
PR governanceThis PR follows the template and is marked ready for human review. |
github-actions
Bot
added
the
status: ready for review
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
JerrettDavis
left a comment
JerrettDavis
left a comment
There was a problem hiding this comment.
Reviewed the latest merged head across both provider cache paths, the shared header policy, and the PERF parser/export contract. The key invariant is now symmetric: a client stream: true request cannot populate a cache that is only read by non-streaming requests, including the buffered-CCR fallthrough. Rebuilding the response after stripping stale content/framing metadata is also correct for decoded response.content, and the additive cached=1 parser path remains backward-compatible.
The focused replay suite passes locally on the exact head (9/9), including poisoned framing, buffered CCR, cache-hit logging, and raw PERF parsing. No blocking correctness findings.
Non-blocking follow-up: if this sanitizer becomes the general hop-by-hop boundary, consider also stripping headers named by the incoming Connection field (plus the remaining RFC hop-by-hop names). That is broader hardening and is not required for the reported stale transfer-encoding failure.
3 participants
Description
Closes #3019
A response-cache hit could hand the client an HTTP 200 that the client could not read, and nothing in the logs marked the turn as anything other than normal.
Two separate problems combine to produce the reported failure.
The unreadable 200. A cache entry stores the producing upstream's response headers verbatim. When the entry is replayed, the Anthropic handler removed only
content-encoding,content-lengthandcontent-typebefore handing those headers to a brand-newResponse. Anything else describing how that other connection framed its body rode along — most damaginglytransfer-encoding: chunked. RFC 9112 §6.1 makesTransfer-EncodingoverrideContent-Length, so the client is told to parse a plain JSON body as chunked frames, finds no valid chunk-size line, and reads an empty body out of a 200. Every other response-forwarding site in the Python proxy already strips that header; the two cache-hit sites were the only ones that did not.How a CCR turn could put a foreign response in the cache. On the Anthropic path,
cache.getis gated onnot streambutcache.setwas not, and the cache key has nostreamcomponent. A CCR buffered-stream conversion takes a request the client sent withstream: true, forcesstream: falseupstream, and — unlike every other streaming turn, which returns via_stream_responseand never touches the cache — falls through to the store site. The stored reply was shaped by that forced flip plus CCR tool injection, and the key cannot distinguish it from an ordinary non-streaming reply, so a later non-streaming caller could be served a response built for a request it never made. This is why the reporters saw the failures pair with CCR activity and stop under--lossless/--no-ccr.Why it was invisible. The cache-hit block emitted no log line at all, and the
PERFline rendered no field forRequestOutcome.from_response_cache. A cache-served turn contacts no upstream, so it has nooutbound_requestline, no upstream stage timings, and all-zero token counters — byte-for-byte what a turn that died would look like. That is whyheadroom doctorreported zero failures while turns were dying.Scope note
The header fix also lands on the OpenAI cache-hit site, which additionally never received the
content-typefix from #2952. Thenot streamgate is added to the OpenAI store site too, where it is currently redundant — a streaming chat request returns via_stream_responselong before that point — purely to state the invariant, since the Anthropic handler had exactly that shape until a buffered-CCR branch began falling through to it.Because the strip list now lives in one shared helper, the OpenAI handler's other five forwarding sites strip the three added headers as well. That is a widening, so it is worth being explicit about: each of those sites builds a fresh fixed-length
Response(or, atopenai.py:6122, synthesises SSE) fromresponse.content, so replaying the upstream's framing there was the same latent bug, just without a cache to make it outlive the request that produced it. The precedent is already in the file —openai.py:9865passes"transfer-encoding", "connection"as extra names by hand, which is exactly the gap this PR closes centrally. That call site keeps its now-redundant arguments; removing them is a cleanup for another PR.Type of Change
Changes Made
sanitize_forwarded_response_headerstoheadroom/proxy/helpers.py, promoting the private helper that already lived inheadroom/proxy/handlers/openai.pyand extending it with the remaining wire-framing headers (transfer-encoding,connection,keep-alive). Matching is now case-insensitive; surviving headers keep their original casing.openai.py's_sanitize_forwarded_response_headersis now a thin alias so its six call sites and the Anthropic handler strip an identical set.headroom/proxy/handlers/anthropic.py: the response-cache hit now sanitises through that helper (passingcontent-typeas an extra name, preserving [BUG] Signed thinking blocks discard the buffered-CCR stream:false flip, so Claude Code getsAPI returned an empty or malformed response (HTTP 200)#2952) instead of three hand-rolledpopcalls.headroom/proxy/handlers/openai.py: the response-cache hit sanitises the same way, gains thecontent-typehandling it was missing, and setsmedia_type="application/json"explicitly.headroom/proxy/handlers/anthropic.py:cache.setis now gated onnot stream, mirroring the read gate.streamstill holds the client's original flag at that point — the buffered-CCR conversion flipsbody["stream"], never the local variable.headroom/proxy/handlers/openai.py: the samenot streamgate on its store site, as an invariant guard.RESPONSE-CACHE-HIT: model=… bytes=… age_s=… hits=…, following the existingCACHE-MISS-ATTRIBUTIONline style.headroom/proxy/outcome.py: thePERFline appendscached=1on a response-cache hit. It is appended only on a hit, so every other PERF line is byte-identical to before and existing parsers are unaffected.headroom/perf/analyzer.py:PerfRecord.from_response_cachereads that field, soheadroom perfcan tell a cache-served turn from a dead one. It defaults toFalse, so older logs still parse.PERF_RECORD_FIELDSgains the name at the end of the list, which is whatheadroom perf --format csv --rawuses as its column set; appending keeps every existing column at its current position.--format json --rawgains the key too.tests/test_anthropic_pre_upstream_backpressure.py: its cache-hit double was a partial hand-rolled stand-in forCacheEntrycarrying only a body and headers, so it broke once the hit path started reading the entry's age and hit count. It now constructs a realCacheEntry, which is what the cache actually returns.Testing
pytest)ruff check .)mypy headroom)Test Output
Real Behavior Proof
upstream/mainat2d88e31a.FRAMING_RESPONSE_HEADERS, restoredcache.settoif self.cache and response.status_code == 200 and resp_json is not None:), ranpython -m pytest tests/test_proxy_response_cache_replay.py -q, then restored the fixes and re-ran. (2) Regression sweep: ran the full suite on this branch, then checked outupstream/maininto a second worktree and re-ran, in that worktree, exactly the tests that failed here and not there.test_buffered_ccr_turn_does_not_write_the_response_cachefails withAssertionError: Expected mock to not have been awaited. Awaited 1 times.— a turn the client sent asstream: truereally does reachcache.setthrough the buffered-CCR branch.test_cache_hit_replays_a_body_the_client_can_actually_readfails withAssertionError: assert 'transfer-encoding' not in {'transfer-encoding': 'chunked', 'connection': 'keep-alive', 'request-id': ..., 'content-length': '228', ...}— the replayed 200 carries the producing turn's chunked framing alongside a freshcontent-length, which is the exact framing conflict a client cannot parse. With the fixes restored, all 9 pass, the replayed body arrives intact asapplication/json, and the run logs bothRESPONSE-CACHE-HITand aPERF … cached=1line. The full suite on this branch gives83 failed, 10493 passed, 657 skipped, 80 errors; 33 of those failures were not in my baseline list, so I ran those 33 in theupstream/mainworktree and 20 failed there identically (Windows-specific:sqlite:///C:\…path handling, private-directory permissions, fsync, ONNX thread caps, serena config discovery). Re-running the remaining 13 serially on this branch gave1 failed, 25 passed— the other 12 were xdist parallelism flakes, including all fourtests/test_proxy/test_anthropic_ccr_deferred_injection.pytests, which are the only ones in this change's blast radius and which pass serially. The one real serial failure,tests/test_savings_ledger_offload.py::test_concurrent_requests_all_land_their_events(AssertionError: a concurrent append was lost / assert 23 == 24), fails the same way onupstream/mainrun serially. The 80 errors are dashboard-template collection errors unrelated to the proxy. Net: no failure attributable to this change.transfer-encoding: chunked. Anthropic direct is HTTP/2, where the header is forbidden, but any HTTP/1.1 hop (corporate proxy, third-party gateway, local relay) reintroduces it. I have also not measured whether thenot streamgate reduces the cache hit rate in practice; by construction it can only drop entries that were unsafe to serve. A reporter running unmodified 0.35.0 withheadroom proxy --no-cachewould confirm the cache path is the one involved, and that flag is a lighter workaround than--losslessor--no-ccrbecause it keeps CCR and compression enabled.Runtime Rollout Safety
cache_enableddefaults toTrue).server, on the Anthropic side). Forwarded responses on the OpenAI handler's other five sanitiser call sites no longer carrytransfer-encoding,connectionorkeep-aliveeither, since the strip list is now shared; all five build a fixed-length response fromresponse.content, so none of them could legitimately replay that framing. A turn whose client asked forstream: trueno longer writes the response cache on the Anthropic path.PERFlines gain a trailingcached=1on a response-cache hit only; all other PERF lines are unchanged.headroom proxy --no-cachedisables the response cache entirely and bypasses every path this PR touches.cached=1PERF field, which_parse_kvalready handles the same way it handles the existing trailingclient=field, and afrom_response_cachecolumn appended toheadroom perf --format csv --raw(plus the matching key in--format json --raw). Anything consuming that CSV positionally keeps working because the column is last; anything reading it by name is unaffected.Review Readiness
Checklist
CHANGELOG.md— it is generated by release-please from my Conventional Commit PR title (a CI guard enforces this)Additional Notes
Documentation is marked N/A: no user-facing surface changes, and the new
cached=PERF field is additive and self-describing.Relationship to nearby open PRs, since several touch adjacent code:
resp_json is not Noneguard at the same Anthropic store site. That stops an SSE body being stored; it does not stop a JSON-bodied response storing chunked framing headers, and it does not add thestreamgate. The two changes are complementary.buffered_stream_ccras a fallback, so the store site this PR gates remains reachable. If fix(ccr): splice private retrieval streams by event #3013 lands first I am happy to rebase.mypy headroom --ignore-missing-importsreports 12 pre-existing errors inheadroom/release_version.py,headroom/ccr/mcp_server.pyandheadroom/memory/mcp_server.pyfrom MCP SDK version drift in my local environment. None are in the files this PR touches, and the count is identical before and after the change.