From 916e9851efffe2130b77bd57b2c67cc901b8a1ec Mon Sep 17 00:00:00 2001 From: mateo-berri <277851410+mateo-berri@users.noreply.github.com> Date: Fri, 1 May 2026 10:05:26 -0700 Subject: [PATCH] tests(e2e-cassette-proxy): fix replay path so cache hits actually serve MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every cache hit was silently falling through to the upstream because the addon's replay path called http.Response.make(status, body, list[(str,str)]), and mitmproxy 11's Headers constructor demands bytes — it raised ``TypeError: Header fields must be bytes.`` inside the addon, mitmdump logged ``Addon error: Header fields must be bytes.`` to its background log, and the request continued out to the real provider as if it had been a miss. (Stats counted it as a hit because the log line was emitted before the raise.) Net effect: the proxy was record-only. Three fixes: 1. _build_replay_response constructs http.Response directly, encoding the cached headers back to bytes (latin-1, RFC 7230) and handing mitmproxy the raw on-the-wire body. Going through Response.make/set_content would also have re-encoded the body (e.g. double-gzip), so we bypass that codepath entirely. 2. The recording side now calls flow.response.headers.items(multi=True) so repeated headers (notably multiple Set-Cookie) are preserved as distinct entries instead of being silently merged. 3. Adds a contract test file that runs against *real* mitmproxy (skipped automatically when only the test stub is loaded). This is what would have caught the original bug — the existing fake stubs don't model Headers's bytes-strictness, which is precisely why the issue hid for the whole record-only run on the previous commit. The existing addon unit tests now prefer real mitmproxy when it's installed, so the contract tests run in the same process when both are available. --- tests/e2e_cassette_proxy/addon.py | 63 +++++++- .../e2e_cassette_proxy/test_addon.py | 63 +++++++- .../test_replay_response_contract.py | 146 ++++++++++++++++++ 3 files changed, 263 insertions(+), 9 deletions(-) create mode 100644 tests/test_litellm/e2e_cassette_proxy/test_replay_response_contract.py diff --git a/tests/e2e_cassette_proxy/addon.py b/tests/e2e_cassette_proxy/addon.py index 4a131a0eb4a..ed23c6bcc80 100644 --- a/tests/e2e_cassette_proxy/addon.py +++ b/tests/e2e_cassette_proxy/addon.py @@ -18,7 +18,8 @@ from __future__ import annotations import logging import os -from typing import Optional +import time +from typing import Optional, Tuple from mitmproxy import ctx, http # type: ignore[import-not-found] @@ -73,6 +74,54 @@ def _is_2xx(status_code: int) -> bool: return 200 <= status_code < 300 +def _to_header_bytes(value: str) -> bytes: + """Encode a header name or value back to bytes for mitmproxy. + + HTTP/1.1 header field names are 7-bit ASCII per RFC 7230 § 3.2; + values are also ASCII in practice but technically allow opaque + octets, which mitmproxy round-trips via the surrogateescape codec. + Latin-1 covers both cases without raising on stray high bytes that + might have leaked in via misbehaved upstreams. + """ + return value.encode("latin-1", errors="replace") + + +def _build_replay_response(cached: CachedResponse) -> http.Response: + """Build a mitmproxy ``Response`` from a cached payload. + + We bypass ``http.Response.make`` because: + + 1. ``make`` requires a ``Headers`` instance or a ``dict`` to do its + own bytes-conversion. Passing a list of ``(str, str)`` tuples + falls into its iterable branch which calls ``Headers(headers)`` + directly — and ``Headers`` raises ``TypeError: Header fields + must be bytes``. (This was the original bug: every cache hit + silently fell through to upstream because the addon raised here.) + 2. ``make`` calls ``Message.set_content`` which re-encodes the body + per the cached ``Content-Encoding`` header. Since we recorded + ``raw_content`` (i.e. the already-encoded wire bytes), going + through ``set_content`` would double-gzip those payloads. + + Constructing the ``Response`` directly lets us hand mitmproxy bytes + headers + the raw body in one shot, exactly mirroring what was on + the wire when we recorded it. + """ + fields: Tuple[Tuple[bytes, bytes], ...] = tuple( + (_to_header_bytes(k), _to_header_bytes(v)) for k, v in cached.headers + ) + now = time.time() + return http.Response( + http_version=b"HTTP/1.1", + status_code=cached.status_code, + reason=_to_header_bytes(cached.reason) or b"OK", + headers=fields, + content=cached.body, + trailers=None, + timestamp_start=now, + timestamp_end=now, + ) + + class CassetteAddon: """mitmproxy addon class. Created once at startup; ``request`` and ``response`` are invoked per flow.""" @@ -151,11 +200,7 @@ class CassetteAddon: f"status={cached.status_code} bytes={len(cached.body)}" ) self._stats["hit"] += 1 - flow.response = http.Response.make( - cached.status_code, - cached.body, - list(cached.headers), - ) + flow.response = _build_replay_response(cached) def response(self, flow: http.HTTPFlow) -> None: if self._store is None or self._should_skip(flow): @@ -172,7 +217,11 @@ class CassetteAddon: key = self._key_for(flow) cached = CachedResponse( status_code=flow.response.status_code, - headers=tuple((k, v) for k, v in flow.response.headers.items()), + # ``items(multi=True)`` preserves repeated headers (e.g. multiple + # ``Set-Cookie``) which the plain ``items()`` flattens into a + # single comma-joined string. Strings are fine on the wire to + # Redis; we re-encode to bytes on replay. + headers=tuple((k, v) for k, v in flow.response.headers.items(multi=True)), body=flow.response.raw_content or b"", reason=flow.response.reason or "", ) diff --git a/tests/test_litellm/e2e_cassette_proxy/test_addon.py b/tests/test_litellm/e2e_cassette_proxy/test_addon.py index 9c251e42224..96d43e270af 100644 --- a/tests/test_litellm/e2e_cassette_proxy/test_addon.py +++ b/tests/test_litellm/e2e_cassette_proxy/test_addon.py @@ -21,6 +21,17 @@ sys.path.insert( 0, os.path.abspath(os.path.join(os.path.dirname(__file__), "..", "..", "..")) ) +# Prefer the *real* mitmproxy if it's installed: that way the contract +# tests in ``test_replay_response_contract.py`` exercise the same +# objects this file does, and any future skew between our fake and +# real mitmproxy can't silently hide regressions. Fall through to the +# fake stub when mitmproxy isn't available (the typical unit-test env). +try: + import mitmproxy # noqa: F401 pre-load so setdefault below is a no-op + import mitmproxy.http # noqa: F401 +except ImportError: + pass + # Install a fake `mitmproxy` package *before* importing the addon, so the # real mitmproxy doesn't need to be present in the unit-test env. _mitmproxy_pkg = types.ModuleType("mitmproxy") @@ -28,16 +39,64 @@ _http_mod = types.ModuleType("mitmproxy.http") class _FakeHeaders(dict): - def items(self): + def items(self, multi: bool = False): + # Real mitmproxy ``Headers.items(multi=True)`` would return one + # tuple per repeated header (e.g. multiple ``Set-Cookie``). The + # fake doesn't model duplicates, so the kwarg is a no-op — we + # just need to accept it without ``TypeError``. return list(super().items()) class _FakeResponse: - def __init__(self, status_code, body=b"", headers=None, reason=""): + """Fake mitmproxy ``http.Response`` that accepts both the real + constructor signature (used by the addon's replay path) and a + simplified positional signature (used by these tests when faking an + upstream response). + """ + + def __init__( + self, + *args, + status_code=None, + body=None, + headers=None, + reason="", + # real-mitmproxy kwargs (the addon passes these via _build_replay_response) + http_version=b"HTTP/1.1", + content=None, + trailers=None, + timestamp_start=None, + timestamp_end=None, + ): + # Positional support: _FakeResponse(status_code, body=..., headers=...) + if args: + if status_code is None: + status_code = args[0] + if len(args) > 1 and body is None: + body = args[1] + # ``content`` (real mitmproxy) and ``body`` (test shorthand) are + # the same field — raw_content. + if body is None: + body = content if content is not None else b"" + if isinstance(headers, list): + # Bytes-tuple list (real mitmproxy contract) → str dict for + # the fake's case-insensitive lookups. + headers = { + (k.decode("latin-1") if isinstance(k, bytes) else k): ( + v.decode("latin-1") if isinstance(v, bytes) else v + ) + for k, v in headers + } + if isinstance(reason, bytes): + reason = reason.decode("latin-1") self.status_code = status_code self.raw_content = body self.headers = _FakeHeaders(headers or {}) self.reason = reason + self.http_version = http_version + self.trailers = trailers + self.timestamp_start = timestamp_start + self.timestamp_end = timestamp_end @classmethod def make(cls, status_code, body=b"", headers=None): diff --git a/tests/test_litellm/e2e_cassette_proxy/test_replay_response_contract.py b/tests/test_litellm/e2e_cassette_proxy/test_replay_response_contract.py new file mode 100644 index 00000000000..60c36020204 --- /dev/null +++ b/tests/test_litellm/e2e_cassette_proxy/test_replay_response_contract.py @@ -0,0 +1,146 @@ +"""Contract tests against real mitmproxy. + +The other addon unit tests stub out ``mitmproxy.http`` so they can run +without mitmproxy installed (it's a CI-only dependency installed via +``uv tool install`` rather than declared in ``pyproject.toml``). + +That stubbing once hid a real production bug: the addon called +``http.Response.make(status, body, list[(str, str)])`` which the fake +accepted, but real mitmproxy 11 raises ``TypeError: Header fields must +be bytes``. Every cache hit silently fell through to the upstream +because the addon raised inside ``request()``. + +This file exercises the *real* mitmproxy ``Response`` contract for the +replay path, so any future regression is caught locally instead of only +showing up as ``Addon error: ...`` in CI logs while tests pass. +""" + +from __future__ import annotations + +import pytest + +mitmproxy_http = pytest.importorskip("mitmproxy.http") + +# ``test_addon.py`` (collected alongside this file) installs a stubbed +# ``mitmproxy`` package into ``sys.modules`` so it can run without the +# real dependency. ``importorskip`` will happily return that stub and +# defeat the whole point of *this* file. Detect the stub by checking +# whether the loaded ``Headers`` enforces bytes the way real mitmproxy +# does, and skip if it doesn't. +try: + mitmproxy_http.Headers([("not", "bytes")]) # type: ignore[arg-type] +except TypeError: + pass # real mitmproxy — proceed +except Exception: + pytest.skip( + "real mitmproxy not available (fake stub detected); " + "install ``mitmproxy==11.0.2`` to run these contract tests", + allow_module_level=True, + ) +else: + pytest.skip( + "real mitmproxy not available (fake stub detected); " + "install ``mitmproxy==11.0.2`` to run these contract tests", + allow_module_level=True, + ) + +from tests.e2e_cassette_proxy.addon import _build_replay_response # noqa: E402 +from tests.e2e_cassette_proxy.redis_store import CachedResponse # noqa: E402 + + +def test_replay_response_accepts_string_headers_and_emits_bytes(): + """Cached headers are stored as ``str`` (msgpack roundtrip); the + replay builder must encode them back to ``bytes`` because mitmproxy + 11's ``Headers`` rejects anything else.""" + cached = CachedResponse( + status_code=200, + headers=( + ("Content-Type", "application/json"), + ("X-Trace-Id", "abc123"), + ), + body=b'{"id":"chatcmpl-1"}', + reason="OK", + ) + + response = _build_replay_response(cached) + + assert isinstance(response, mitmproxy_http.Response) + assert response.status_code == 200 + # mitmproxy's ``Headers`` decodes back to str on read; the underlying + # ``fields`` are bytes — verify both directions. + assert response.headers["Content-Type"] == "application/json" + assert response.headers["X-Trace-Id"] == "abc123" + for name, value in response.headers.fields: + assert isinstance(name, bytes), name + assert isinstance(value, bytes), value + + +def test_replay_response_preserves_raw_content_without_double_encoding(): + """When the upstream replied with ``Content-Encoding: gzip`` we recorded + the *gzipped* bytes (i.e. ``flow.response.raw_content``). On replay the + body must be set as ``raw_content`` so mitmproxy serves the same wire + payload — going through ``set_content`` would gzip-encode an already- + gzipped blob.""" + gzipped_body = ( + b"\x1f\x8b\x08\x00\x00\x00\x00\x00\x00\x03\xab\xe6" + b"R\xa8\xe5\x02\x00\x9b\xae\x10\xee\x05\x00\x00\x00" + ) # arbitrary gzip-magic-prefixed bytes + cached = CachedResponse( + status_code=200, + headers=( + ("Content-Type", "application/json"), + ("Content-Encoding", "gzip"), + ("Content-Length", str(len(gzipped_body))), + ), + body=gzipped_body, + reason="OK", + ) + + response = _build_replay_response(cached) + + assert response.raw_content == gzipped_body, ( + "raw_content must be byte-identical to what we recorded; " + "going through set_content would re-gzip and corrupt it" + ) + + +def test_replay_response_handles_repeated_set_cookie_headers(): + """``items(multi=True)`` is used on the recording side specifically so + multi-value headers like ``Set-Cookie`` aren't silently merged into a + comma-joined string (which is RFC 7230 illegal for ``Set-Cookie``).""" + cached = CachedResponse( + status_code=200, + headers=( + ("Set-Cookie", "session=abc; Path=/"), + ("Set-Cookie", "csrf=xyz; Path=/"), + ("Content-Type", "text/html"), + ), + body=b"", + reason="OK", + ) + + response = _build_replay_response(cached) + + set_cookies = [ + v.decode("latin-1") + for k, v in response.headers.fields + if k.lower() == b"set-cookie" + ] + assert set_cookies == ["session=abc; Path=/", "csrf=xyz; Path=/"] + + +def test_replay_response_tolerates_empty_reason_phrase(): + cached = CachedResponse( + status_code=204, + headers=(), + body=b"", + reason="", + ) + + response = _build_replay_response(cached) + + assert response.status_code == 204 + # mitmproxy expects bytes; an empty reason should default to ``b"OK"`` + # at the wire level so the response is still serializable by + # mitmproxy's HTTP/1.1 codec. ``reason`` decodes back to str on read. + assert response.reason == "OK"