diff --git a/headroom/proxy/handlers/anthropic.py b/headroom/proxy/handlers/anthropic.py index ca119f4dc..2da5f9afe 100644 --- a/headroom/proxy/handlers/anthropic.py +++ b/headroom/proxy/handlers/anthropic.py @@ -318,6 +318,34 @@ class AnthropicHandlerMixin: return True return False + def _can_salvage_buffered_upstream(self, resp_json: Any) -> bool: + """May this upstream response be relayed when post-processing failed? + + Only when the client can actually consume it. The buffered path exists + because a ``headroom_retrieve`` call has to be resolved server-side, so + a response still carrying one is exactly the case the handler already + fails closed on: relaying it would hand the client a tool call naming an + endpoint it is not expected to reach, and a marker nobody expanded. + + Anything else — an ordinary answer, a client tool call, a turn whose + retrieval already resolved — is a complete provider turn and is safer in + the client's hands than a synthesized error (#3088). + """ + if not isinstance(resp_json, dict): + return False + handler = getattr(self, "ccr_response_handler", None) + if handler is None: + return True + try: + from headroom.ccr.response_handler import RESIDUAL_CCR_ERROR + + if handler.residual_ccr_status(resp_json, "anthropic") == RESIDUAL_CCR_ERROR: + return False + return not handler.has_ccr_tool_calls(resp_json, "anthropic") + except Exception: # pragma: no cover - defensive + logger.debug("CCR: salvage check failed; not salvaging", exc_info=True) + return False + @staticmethod def _outgoing_body_has_redeemable_marker(body: Any) -> bool: """Does the body about to be sent carry a marker retrieval could expand? @@ -3610,6 +3638,9 @@ class AnthropicHandlerMixin: session_key=session_key, ) else: + # Populated once the upstream answers 200 with parseable + # JSON, so the guard below can fall back to it (#3088). + _salvageable_upstream: dict[str, Any] = {} async def _buffered_ccr_operation(): async with stage_timer.measure("upstream_connect"): @@ -3781,6 +3812,16 @@ class AnthropicHandlerMixin: resp_json = None try: resp_json = response.json() + if buffered_stream_ccr and response.status_code == 200 and resp_json: + # Remember the upstream's own answer before any + # post-processing touches it. Everything from here + # to the SSE resynthesis — retrieval, memory tool + # calls, turn hooks, usage accounting, caching — is + # work layered on top of a turn the provider has + # already produced and billed. If any of it raises + # unexpectedly, this is what the client should get + # instead of a synthesized error (#3088). + _salvageable_upstream["resp_json"] = resp_json except (json.JSONDecodeError, ValueError) as e: # DEBUG is right for the buffered non-stream path, where # an unparseable body is just "no CCR handling". On the @@ -4496,8 +4537,58 @@ class AnthropicHandlerMixin: headers=response_headers, ) + async def _buffered_ccr_operation_salvaging(): + """Never trade a successful upstream turn for a synthesized error. + + Everything the buffered path does after the provider answers + — server-side retrieval, memory tool calls, turn hooks, usage + accounting, caching, SSE resynthesis — is post-processing on + a turn that already succeeded and was already billed. When a + step raised unexpectedly the whole turn surfaced as a generic + ``api_error``, so the client lost a complete 69KB answer the + provider had produced (#3088), and the cause was unlogged. + + Relay the upstream's own answer instead. Two things are + deliberately preserved: the exception is logged with a + traceback so the real defect stays diagnosable rather than + being papered over, and a response the client cannot safely + consume is never salvaged — see ``_can_salvage``. + """ + try: + return await _buffered_ccr_operation() + except asyncio.CancelledError: + raise + except Exception: + salvaged = _salvageable_upstream.get("resp_json") + if salvaged is None or not self._can_salvage_buffered_upstream(salvaged): + raise + logger.error( + f"[{request_id}] CCR: buffered post-processing failed after a " + "successful upstream turn; relaying the upstream response " + "instead of failing the request (#3088)", + exc_info=True, + ) + try: + events = self._response_to_sse(salvaged, "anthropic") + except Exception: + logger.error( + f"[{request_id}] CCR: could not resynthesize the salvaged " + "upstream response; failing the request", + exc_info=True, + ) + raise + + async def _salvaged_sse(): + for event in events: + yield event + + return StreamingResponse( + _salvaged_sse(), + media_type="text/event-stream", + ) + if buffered_stream_ccr: - operation = asyncio.create_task(_buffered_ccr_operation()) + operation = asyncio.create_task(_buffered_ccr_operation_salvaging()) # Holds out for the real status, then keeps the stream alive # once waiting silently would risk the client's idle @@ -4520,7 +4611,7 @@ class AnthropicHandlerMixin: await _buffered_call(scope, receive, send) return _BufferedCCRResponse(media_type="text/event-stream") - return await _buffered_ccr_operation() + return await _buffered_ccr_operation_salvaging() except HTTPException: # FastAPI HTTPException carries its own status code, headers, # and client-facing message (e.g. 429 with Retry-After, 413 for diff --git a/tests/test_buffered_ccr_salvage.py b/tests/test_buffered_ccr_salvage.py new file mode 100644 index 000000000..672d353a7 --- /dev/null +++ b/tests/test_buffered_ccr_salvage.py @@ -0,0 +1,228 @@ +"""A successful upstream turn must never become a synthesized error (#3088). + +The buffered CCR path flips a streaming turn to ``stream: false`` so retrieval +can be resolved server-side. Everything it does *after* the provider answers — +retrieval, memory tool calls, turn hooks, usage accounting, caching, SSE +resynthesis — is post-processing layered on a turn that already succeeded and +was already billed. + +When one of those steps raised, the whole turn surfaced to the client as: + + event: error + data: {"type":"error","error":{"type":"api_error", ...}} + +In the reported capture the provider had returned a complete 69,351-byte answer +in 1.9s; the client received 1,841 bytes of keepalives and that error. The +answer was paid for and thrown away, and no traceback was logged, so the real +defect stayed invisible. + +These tests pin the two halves of the fix: relay the upstream's own answer +rather than inventing a failure, and refuse to relay a response the client +cannot safely consume. +""" + +from __future__ import annotations + +import json + +import pytest + +fastapi = pytest.importorskip("fastapi") +httpx = pytest.importorskip("httpx") + +from fastapi.testclient import TestClient # noqa: E402 + +from headroom.cache.backends import InMemoryBackend # noqa: E402 +from headroom.cache.compression_store import ( # noqa: E402 + get_compression_store, + reset_compression_store, +) +from headroom.ccr.tool_injection import create_ccr_tool_definition # noqa: E402 +from headroom.proxy.server import ProxyConfig, create_app # noqa: E402 + + +def _config() -> ProxyConfig: + return ProxyConfig( + optimize=False, + cache_enabled=False, + rate_limit_enabled=False, + memory_enabled=False, + ccr_inject_tool=True, + ccr_handle_responses=True, + ccr_context_tracking=False, + image_optimize=False, + # Commit immediately, so a failure is exercised on the committed path + # too — the shape the report was filed against. + buffered_ccr_grace_seconds=5.0, + ) + + +@pytest.fixture(autouse=True) +def _store(): + reset_compression_store() + get_compression_store(backend=InMemoryBackend()) + try: + yield + finally: + reset_compression_store() + + +def _marker() -> str: + return get_compression_store().store( + original=json.dumps({"earlier": "tool output"}), + compressed="{}", + original_item_count=1, + ) + + +def _upstream(content: list[dict], stop_reason: str = "end_turn") -> dict: + return { + "id": "msg_upstream", + "type": "message", + "role": "assistant", + "model": "claude-sonnet-4-6", + "content": content, + "stop_reason": stop_reason, + "usage": { + "input_tokens": 1200, + "output_tokens": 295, + "cache_read_input_tokens": 0, + "cache_creation_input_tokens": 0, + }, + } + + +def _body() -> dict: + return { + "model": "claude-sonnet-4-6", + "max_tokens": 512, + "stream": True, + "tools": [create_ccr_tool_definition("anthropic")], + "messages": [{"role": "user", "content": f"go (earlier output at <>)"}], + } + + +def _headers() -> dict[str, str]: + return {"x-api-key": "test-key", "anthropic-version": "2023-06-01"} + + +def _run(upstream: dict, *, break_post_processing: bool): + """Drive one buffered turn, optionally exploding after the upstream answers.""" + app = create_app(_config()) + with TestClient(app) as client: + proxy = client.app.state.proxy + + async def _fake_retry(method, url, headers, body, stream=False, **kwargs): # noqa: ANN001 + return httpx.Response(200, json=upstream) + + proxy._retry_request = _fake_retry # type: ignore[assignment] + + if break_post_processing: + # Stand in for any of the post-upstream steps failing. The point is + # that the provider already answered; what broke is ours. + real = proxy._record_request_outcome + + async def _boom(*args, **kwargs): # noqa: ANN002, ANN003 + raise RuntimeError("post-processing exploded") + + proxy._record_request_outcome = _boom # type: ignore[assignment] + assert real is not None + + return client.post("/v1/messages", json=_body(), headers=_headers()) + + +# --------------------------------------------------------------------------- # +# The reported failure +# --------------------------------------------------------------------------- # +def test_a_successful_turn_survives_post_processing_blowing_up() -> None: + """The whole point: the client gets the answer the provider produced.""" + upstream = _upstream( + [ + {"type": "thinking", "thinking": "reasoning", "signature": "sig-1"}, + {"type": "text", "text": "here is the answer"}, + ] + ) + + resp = _run(upstream, break_post_processing=True) + + assert resp.status_code == 200, resp.text + assert "text/event-stream" in resp.headers["content-type"] + # The provider's content reaches the client... + assert "here is the answer" in resp.text + assert "message_start" in resp.text + # ...and no invented failure does. + assert "api_error" not in resp.text + + +def test_a_client_tool_call_is_salvaged_too() -> None: + """The captured failure was a `bash` tool_use turn with no retrieve call.""" + upstream = _upstream( + [ + {"type": "thinking", "thinking": "plan", "signature": "sig-2"}, + { + "type": "tool_use", + "id": "toolu_bash", + "name": "bash", + "input": {"command": "ls"}, + }, + ], + stop_reason="tool_use", + ) + + resp = _run(upstream, break_post_processing=True) + + assert resp.status_code == 200, resp.text + assert "toolu_bash" in resp.text + assert "api_error" not in resp.text + + +def test_the_healthy_path_is_untouched() -> None: + """Salvage must not change a turn that never failed.""" + upstream = _upstream([{"type": "text", "text": "ordinary answer"}]) + + resp = _run(upstream, break_post_processing=False) + + assert resp.status_code == 200, resp.text + assert "ordinary answer" in resp.text + assert "api_error" not in resp.text + + +# --------------------------------------------------------------------------- # +# What must never be salvaged +# --------------------------------------------------------------------------- # +def test_an_unresolved_retrieve_call_is_not_relayed() -> None: + """Failing closed here is deliberate and stays that way. + + The buffered path exists to resolve ``headroom_retrieve`` server-side. A + response still carrying one is precisely the case the handler already fails + closed on — relaying it would hand the client a tool call it is not expected + to service and a marker nobody expanded. + """ + app = create_app(_config()) + with TestClient(app) as client: + proxy = client.app.state.proxy + unresolved = _upstream( + [ + { + "type": "tool_use", + "id": "toolu_ccr", + "name": "headroom_retrieve", + "input": {"hash_key": "deadbeefcafe"}, + } + ], + stop_reason="tool_use", + ) + assert proxy._can_salvage_buffered_upstream(unresolved) is False + + # An ordinary turn is salvageable, so the guard is not simply off. + assert ( + proxy._can_salvage_buffered_upstream(_upstream([{"type": "text", "text": "hi"}])) + is True + ) + + +@pytest.mark.parametrize("bad", [None, "not-a-dict", 42, []]) +def test_a_non_dict_response_is_never_salvaged(bad) -> None: # type: ignore[no-untyped-def] + app = create_app(_config()) + with TestClient(app) as client: + assert client.app.state.proxy._can_salvage_buffered_upstream(bad) is False