fix(proxy/streaming): tolerate malformed upstream SSE events - #2558
fix(proxy/streaming): tolerate malformed upstream SSE events#2558nkamali wants to merge 3 commits into
Conversation
PR governanceThis PR does not yet satisfy the required template fields:
Please update the PR body, or move the PR back to draft while it is still in progress. |
## Description
Three parsers read upstream-controlled SSE bodies with `json.loads` and then
reach into the result without checking its shape:
`_parse_sse_usage`, `_parse_sse_usage_from_buffer` and `_parse_sse_to_response`.
A valid-JSON value of the wrong shape passes the `json.JSONDecodeError` guard
and then raises. Three distinct ways:
- a non-object event (`["x"]`, `"str"`, `42`) makes `.get` raise `AttributeError`
- an explicit `"key": null` bypasses a `.get(key, {})` default, because the
default applies only to a *missing* key, not a present-and-null one
- an unhashable `index` (a list or dict) raises `TypeError: unhashable type`
when used to key the block map
In `_parse_sse_to_response` the raise escapes into `_finalize_stream_response`
and tears down a stream the client is already reading. Several call sites reach
it from a bare `finally`, so there is no handler above it at all.
**This is a robustness invariant, not a bug report about any specific upstream.**
I am not claiming Anthropic emits these shapes; I have no evidence of that. The
Anthropic handler serves any Anthropic-shaped upstream (`--backend
anthropic|bedrock|openrouter|anyllm|litellm-<provider>`,
`ANTHROPIC_TARGET_API_URL`), so Bedrock, OpenRouter, LiteLLM, vLLM and Vertex
all flow through these parsers. A parser on a proxy's critical streaming path
should not raise on a well-formed-JSON frame of unexpected shape, whatever the
source. The guard costs one `isinstance`; being wrong costs the user's session.
**Scope: frame shape only.** Value validity — a token count that is a string,
`Infinity`, or a >4300-digit integer literal — is a separate defect class that
reaches the same parsers and is deliberately left for a follow-up.
## Type of Change
- [x] Bug fix (non-breaking change that fixes an issue)
## Changes Made
- **headroom/proxy/handlers/streaming.py**
- Add three shared normalizers — `_sse_dict`, `_sse_str`, `_sse_index` — so
every upstream-derived container is shape-checked with one idiom instead of
a mix of inline `isinstance` and `.get(..., {})` defaults.
- `_sse_index` rejects `bool` as well as non-`int`, so `true` cannot silently
alias block index `1`, and unhashable values cannot reach the block map.
- Guard the positions the earlier pass missed: `content_block_delta.delta`,
`message_delta.delta`, `message_delta.usage` (`dict.update` on a non-mapping
raises — the `message_start` twin was already guarded, this sibling was not),
the `index` on all three `content_block_*` events, and the string
accumulators for `text` / `partial_json` / `thinking`.
- Guard `_parse_sse_usage`, which had no shape checks at all — not on the
top-level event, nor on the anthropic, openai or gemini branches.
- Guard the gemini `usageMetadata` branch in `_parse_sse_usage_from_buffer`,
which used a truthiness check where its two sibling branches used
`isinstance`.
- Stop the non-standard-block copy-through from seeding the parser's own
scratch keys (`_partial_json`, `thinking_buffer`) via `_BLOCK_SCRATCH_KEYS`,
and tolerate a non-list `citations` that a copy-through already placed.
- **tests/test_streaming_sse_malformed_events.py** — new file, 408 cases.
## Testing
- [x] Unit tests pass (`pytest`)
- [x] Linting passes (`ruff check`, `ruff format`)
- [x] Type checking passes (`mypy headroom`)
- [x] New tests added
The sweep is enumerated, not randomized: the failure surface is finite, so the
cases can be exact and no `hypothesis` dependency is needed. This matches the
existing precedent in `tests/test_toin_observation_only.py`.
Happy-path fixtures are transcribed from real `api.anthropic.com` streams
rather than hand-written, which corrected four wrong assumptions in the
previous fixtures: `message_delta.usage` repeats the *full* usage block (not
just `output_tokens`); `message_start.message` carries `stop_details: null`
alongside the other two nulls; `usage.cache_creation` is always present as a
nested object and is what `_extract_anthropic_cache_ttl_metrics` reads; and a
`tool_use` block carries a non-standard `caller` field.
### Test Output
```text
$ pytest tests/test_streaming_sse_malformed_events.py
408 passed
# same file against origin/main's parsers
191 failed, 217 passed
# regression sweep: all 22 test files touching SSE parsing
710 passed, 5 skipped
$ mypy headroom --ignore-missing-imports
Success: no issues found in 509 source files
```
## Real Behavior Proof
- **Environment:** macOS 15 (Darwin 25.4.0, arm64), Python 3.13.7, Rust 1.95.0
per `rust-toolchain.toml`, `uv sync --extra dev`.
- **Real upstream capture:** three live streaming `POST /v1/messages` calls to
`api.anthropic.com` (`claude-haiku-4-5`) — a text stream, a `tool_use` stream
and an extended-thinking stream — captured as raw wire bytes and used to build
the happy-path fixtures. This is what surfaced the four fixture errors above.
- **Differential:** the 408-case file was run against `origin/main`'s parsers via
`git show origin/main:...` (not `git stash`, which silently no-ops once the fix
is committed and produces a false pass). 191 cases fail there and pass here.
- **Exhaustive shape fuzz:** 700 generated cases crossing every upstream-derived
container position in the three parsers with `None`, `"str"`, `42`, `3.5`,
`True`, `False`, `["x"]`, `[]`, `{}`, `{"k":"v"}`. Zero raises after the change;
before it, the positions listed under Changes Made all raise.
- **Per-position legitimacy check:** each injection position in the sweep was
confirmed to produce at least one genuine failure against `origin/main`, so no
case passes vacuously. This caught four delta-shaped tests that were injecting
their fault before any content block had opened, where the parser skips the
body and the guard is never reached.
- **Not tested:**
- **No evidence these shapes occur in the wild.** The fault shapes are derived
from the parsers' own unguarded access positions, not observed in real
upstream traffic. I searched the tracker for this crash signature and both
function names and found no report. The claim is survivability, not incidence.
- The real API cannot be made to emit malformed frames on demand, so the fault
injection is synthetic; only the happy-path fixtures come from live traffic.
- No live third-party gateway (Bedrock/OpenRouter/LiteLLM/Vertex) exercised,
despite those being the motivating case.
- No WebSocket path, no Windows, not run against a live Claude Code session.
- Value-validity faults are out of scope and still raise; see Additional Notes.
## Additional Notes
Two findings left deliberately unfixed, both pre-existing and both outside the
frame-shape scope of this change:
1. **Value validity.** `_extract_anthropic_cache_ttl_metrics` calls `int()` on an
upstream value with no `try`; `_usage_int` does not catch `OverflowError`, and
`json.loads` accepts bare `Infinity`; `except json.JSONDecodeError` is too
narrow for a >4300-digit integer literal (plain `ValueError`) or deep nesting
(`RecursionError`); and the anthropic and gemini branches export non-`int`
token values out of a `dict[str, int]`-annotated function.
2. **Reconstruction fidelity.** `content_block_start` copies through unknown
fields only for non-standard block types, so the real `caller` field on a
`tool_use` block is dropped on reconstruction.
`test_real_tool_use_stream_reconstructs_tool_input` pins the current
behaviour rather than asserting the desired one.
## 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 — n/a, no
user-facing surface changed
- [x] My changes generate no new warnings
- [x] I have added tests that prove my fix is effective
- [x] New and existing unit tests pass locally with my changes
- [x] I did **not** edit `CHANGELOG.md`
No new dependencies added.
ae6e772 to
8369b7f
Compare
JerrettDavis
left a comment
There was a problem hiding this comment.
The shape-hardening approach looks sound: expected dict/string/index values stay identity-preserving, wrong-shaped upstream frames degrade without escaping parser errors, and the reconstruction changes avoid the scratch-key/citations traps called out in the PR. Local verification passed after copying the built Rust extension into the worktree: pytest tests/test_streaming_sse_malformed_events.py -q (408 passed), nearby SSE parser tests (32 passed), pytest tests/test_streaming_sse_malformed_events.py tests/test_proxy/test_anthropic_streaming_ccr_retrieve.py tests/test_openai_responses_context_compaction.py -q (429 passed), and streaming resilience/logger/cache subsets (39 passed). Ruff passed on the touched files.
|
@chopratejas @DevanshiVyas mind reviewing please? |
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
# Conflicts: # headroom/proxy/handlers/streaming.py
JerrettDavis
left a comment
There was a problem hiding this comment.
Re-reviewed and maintained the latest source against current upstream main. The conflict resolution composes the PR's shape normalizers with main's strict stream-completeness tracking: message/error/block lifecycle flags remain intact, malformed containers and indexes still degrade safely, and stop_sequence/usage reconstruction preserves both sides' contracts. Exact merged-tree verification: 437 malformed-SSE, buffered-SSE, and streaming-CCR tests passed; targeted Ruff and the full merge pre-commit hooks passed. The branch is now mergeable. Only governance checks were scheduled remotely, which is not an author action item. No blocking finding.
Description
Three parsers read upstream-controlled SSE bodies with
json.loadsand thenreach into the result without checking its shape:
_parse_sse_usage,_parse_sse_usage_from_bufferand_parse_sse_to_response.A valid-JSON value of the wrong shape passes the
json.JSONDecodeErrorguardand then raises. Three distinct ways:
["x"],"str",42) makes.getraiseAttributeError"key": nullbypasses a.get(key, {})default, because thedefault applies only to a missing key, not a present-and-null one
index(a list or dict) raisesTypeError: unhashable typewhen used to key the block map
In
_parse_sse_to_responsethe raise escapes into_finalize_stream_responseand tears down a stream the client is already reading. Several call sites reach
it from a bare
finally, so there is no handler above it at all.This is a robustness invariant, not a bug report about any specific upstream.
I am not claiming Anthropic emits these shapes; I have no evidence of that. The
Anthropic handler serves any Anthropic-shaped upstream (
--backend anthropic|bedrock|openrouter|anyllm|litellm-<provider>,ANTHROPIC_TARGET_API_URL), so Bedrock, OpenRouter, LiteLLM, vLLM and Vertexall flow through these parsers. A parser on a proxy's critical streaming path
should not raise on a well-formed-JSON frame of unexpected shape, whatever the
source. The guard costs one
isinstance; being wrong costs the user's session.Scope: frame shape only. Value validity — a token count that is a string,
Infinity, or a >4300-digit integer literal — is a separate defect class thatreaches the same parsers and is deliberately left for a follow-up.
Type of Change
Changes Made
The whole change is three one-line normalizers plus their call sites
(docstrings elided here; they are in the diff):
At a call site, in
_parse_sse_usage_from_buffer:Nested because
messageandmessage.usageare independentlyupstream-controlled — either can be
nullor the wrong type, and the outerguard cannot protect the inner access. The
.getdefaults are dropped asredundant, since
_sse_dictmaps absent,nulland wrong-type onto one result.Why a helper and not
.get(key) or {}:orbranches on truthiness, so itcloses the
nullcase and leaves every truthy non-dict ("overloaded",42,["x"]) still reaching.get.isinstancekeys off the property the next lineactually requires — is this a mapping? — and it narrows the type for
mypy, sothe next unguarded
.getadded to one of these chains becomes a type errorrather than being inferred as
Any.Well-formed frames are unaffected. Each normalizer is the identity function
on a value of the expected type, so no happy-path value is rewritten. The
real-stream happy-path fixtures pass against
origin/mainand this branchalike — they sit inside the 217 of 408 cases that already passed before the fix.
A malformed frame degrades rather than being swallowed. A wrong-shaped
container becomes an empty one of the right type, so the affected field
contributes nothing:
_parse_sse_usage*omits that key (or returnsNone) and_parse_sse_to_responseskips the block. The cost is one lost usage number,which
_finalize_stream_responsealready handles — it falls back to abyte-derived estimate and tags the outcome
output_tokens_source=estimated_bytesinstead of passing a guess off asmeasured. The normalizers themselves are deliberately silent: they sit on a
per-frame, per-field hot path, so even a debug line would fire thousands of
times per stream.
headroom/proxy/handlers/streaming.py
_sse_dict,_sse_str,_sse_index— soevery upstream-derived container is shape-checked with one idiom instead of
a mix of inline
isinstanceand.get(..., {})defaults._sse_indexrejectsboolas well as non-int, sotruecannot silentlyalias block index
1, and unhashable values cannot reach the block map.content_block_delta.delta,message_delta.delta,message_delta.usage(dict.updateon a non-mappingraises — the
message_starttwin was already guarded, this sibling was not),the
indexon all threecontent_block_*events, and the stringaccumulators for
text/partial_json/thinking._parse_sse_usage, which had no shape checks at all — not on thetop-level event, nor on the anthropic, openai or gemini branches.
usageMetadatabranch in_parse_sse_usage_from_buffer,which used a truthiness check where its two sibling branches used
isinstance.scratch keys (
_partial_json,thinking_buffer) via_BLOCK_SCRATCH_KEYS,and tolerate a non-list
citationsthat a copy-through already placed.tests/test_streaming_sse_malformed_events.py — new file, 408 cases.
Testing
pytest)ruff check,ruff format)mypy headroom)The sweep is enumerated, not randomized: the failure surface is finite, so the
cases can be exact and no
hypothesisdependency is needed. This matches theexisting precedent in
tests/test_toin_observation_only.py.Happy-path fixtures are transcribed from real
api.anthropic.comstreamsrather than hand-written, which corrected four wrong assumptions in the
previous fixtures:
message_delta.usagerepeats the full usage block (notjust
output_tokens);message_start.messagecarriesstop_details: nullalongside the other two nulls;
usage.cache_creationis always present as anested object and is what
_extract_anthropic_cache_ttl_metricsreads; and atool_useblock carries a non-standardcallerfield.Test Output
Real Behavior Proof
Environment: macOS 15 (Darwin 25.4.0, arm64), Python 3.13.7, Rust 1.95.0
per
rust-toolchain.toml,uv sync --extra dev. Proxy build0.33.0-dev(editable install of this branch), confirmed per run via
/health.Exact command / steps: enumerated unit sweep, a parser differential against
origin/main, HTTP-level fault injection through a local Anthropic-shapedupstream, then a live Claude Code session on this build.
Observed result: (1)
408 passed. (2) all three parsers raiseAttributeError: 'NoneType' object has no attribute 'get'onorigin/mainand return
Noneon this branch from identical input. (3) the malformedstream completes end to end —
HTTP 200, 568 bytes, all six SSE framesincluding the text delta delivered to the client, zero tracebacks,
error: nullin the request record. Usage degrades partially and correctly:output_tokens=7survives from the well-formedmessage_delta, whilecache_read/cache_write— which live inside the nulledmessage_start.message.usage— fall back to0instead of raising. (4) 421 liverequests over 24h (
claude-opus-5,claude-sonnet-5) in an unrelatedrepository,
error: nullon all 421 and zero streaming exceptions in theproxy log. Of the 340 that reached the streaming finalizer, 328 carry
output_tokens_source=provider; 12 fell back to the byte estimate, every oneon a stream truncated in its first frames —
output_tokens = total_bytes // 40puts those bodies at 120-919 bytes against inputs of 149k-197k tokens, and that
fallback is pre-existing and identical on
origin/main, not a guard dropping aframe. The other 81 requests never reached the streaming finalizer, which sets
that tag unconditionally, so they are non-streaming.
cache_readclimbed to256,027 with 407/421 cache hits and 120,129 tokens saved. So the guards are
identity-transparent on well-formed frames, and the sustained prefix-cache
growth means
_parse_sse_to_responseran throughout — all three parsersexercised on live traffic. (5)
Success: no issues found in 509 source files;ci-precheck PASSED.Real upstream capture: three live streaming
POST /v1/messagescalls toapi.anthropic.com(claude-haiku-4-5) — a text stream, atool_usestreamand an extended-thinking stream — captured as raw wire bytes and used to build
the happy-path fixtures. This is what surfaced the four fixture errors above.
Differential: the 408-case file was run against
origin/main's parsers viagit show origin/main:...(notgit stash, which silently no-ops once the fixis committed and produces a false pass). 191 cases fail there and pass here.
Exhaustive shape fuzz: 700 generated cases crossing every upstream-derived
container position in the three parsers with
None,"str",42,3.5,True,False,["x"],[],{},{"k":"v"}. Zero raises after the change;before it, the positions listed under Changes Made all raise.
Per-position legitimacy check: each injection position in the sweep was
confirmed to produce at least one genuine failure against
origin/main, so nocase passes vacuously. This caught four delta-shaped tests that were injecting
their fault before any content block had opened, where the parser skips the
body and the guard is never reached.
Not tested: the malformed frames are synthetic and the incidence claim is
unproven — limits below.
from the parsers' own unguarded access positions, not observed in real
upstream traffic. I searched the tracker for this crash signature and both
function names and found no report. The claim is survivability, not incidence.
injection is synthetic; only the happy-path fixtures and the live positive
control come from real traffic.
despite those being the motivating case. The fault injection used a local
stand-in reached via
ANTHROPIC_TARGET_API_URL, not a real gateway.well-formed handling. It does not show a guard firing, and cannot: the
normalizers are silent and
api.anthropic.comsends well-formed frames.Additional Notes
Two findings left deliberately unfixed, both pre-existing and both outside the
frame-shape scope of this change:
_extract_anthropic_cache_ttl_metricscallsint()on anupstream value with no
try;_usage_intdoes not catchOverflowError, andjson.loadsaccepts bareInfinity;except json.JSONDecodeErroris toonarrow for a >4300-digit integer literal (plain
ValueError) or deep nesting(
RecursionError); and the anthropic and gemini branches export non-inttoken values out of a
dict[str, int]-annotated function.content_block_startcopies through unknownfields only for non-standard block types, so the real
callerfield on atool_useblock is dropped on reconstruction.test_real_tool_use_stream_reconstructs_tool_inputpins the currentbehaviour rather than asserting the desired one.
Review Readiness
Checklist
user-facing surface changed
CHANGELOG.mdNo new dependencies added.