fix: propagate upstream HTTP errors in streaming responses instead of silent 200 - #11
Merged
Merged
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe proxy streaming handler now explicitly detects upstream HTTP errors, parses error bodies as JSON/text, and returns synchronous JSONResponses for errors; successful SSE streams are relayed while extracting token usage and recording latency/TTFT. OpenCode token totals are normalized (use max of stored total and derived sum) in OTLP ingestion, upsert logic, and reporting SQL expressions. Tests are added/updated and a ChangesStreaming Error Handling, Usage Normalization, Tests, Docs
Sequence DiagramsequenceDiagram
participant Client
participant forward as forward()
participant handler as _forward_stream_or_error()
participant Upstream
participant Recorder as record_usage()
Client->>forward: send streaming request
forward->>handler: invoke _forward_stream_or_error
handler->>Upstream: build_request/send(stream=True)
Upstream-->>handler: response(status, headers, body)
alt status >= 400
handler->>handler: read error body (aread)
handler->>handler: parse JSON or decode text
handler-->>Client: return JSONResponse(status, error_body)
handler->>Recorder: record usage/latency metadata
else status < 400
loop for each SSE chunk
handler->>handler: relay bytes to Client
handler->>handler: parse SSE `data:` lines
handler->>handler: extract_stream_usage updates usage fields
handler->>handler: track TTFT (first token time)
end
handler->>Recorder: record_usage(final usage, latency, TTFT)
handler-->>Client: StreamingResponse(text/event-stream)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/proxy.py`:
- Around line 151-160: The early return on upstream errors skips recording
usage, so failed streaming attempts (4xx/5xx) aren't logged; modify the error
branch that reads error_body and returns a JSONResponse to call
record_usage(...) with the same arguments used in the successful/non-streaming
path (include status_code and measured latency/context) before closing the
client and returning; ensure you still await client.aclose(), compute/propagate
the same status and timing values, and then return JSONResponse(error_content,
status_code=upstream.status_code) so failed stream setups are recorded the same
way as non-streaming responses.
- Around line 147-153: The httpx AsyncClient is leaked on exceptions in
_forward_stream_or_error because client.aclose() is only called on the
happy/failure paths after await upstream.aread() or after the relay; wrap the
client lifetime so it's always closed — either use "async with
httpx.AsyncClient(timeout=REQUEST_TIMEOUT_SECONDS) as client:" around the
send/aread/relay logic or surround the client/send/aread calls with try/finally
and call await client.aclose() in the finally block; ensure this covers the
calls to client.send(...), upstream.aread(), and any downstream relay so client
and upstream are always closed even on errors.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2e5696a5-4587-45d9-9ced-ab133fa9d8b1
📒 Files selected for processing (2)
src/proxy.pytests/test_proxy.py
Haannbboo
force-pushed
the
fix/proxy-upstream-error-propagation
branch
from
May 23, 2026 01:48
6ea1e74 to
9ecc050
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/proxy.py (1)
155-157:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle
aread()failures without leaking the client.If
upstream.aread()raises after the status code is already known, this branch exits beforeclient.aclose()and before returning the upstream 4xx/5xx, so the proxy both leaks thehttpxclient and turns the upstream failure into a local 500.Suggested fix
if upstream.status_code >= 400: - error_body = await upstream.aread() - await client.aclose() + error_body = b"" + try: + error_body = await upstream.aread() + except httpx.HTTPError: + pass + finally: + await client.aclose() latency_ms = int((time.monotonic() - started_at) * 1000)As per coding guidelines "Treat streaming, tool-call partial chunks, retry, timeout, and idempotency behavior as high-risk review areas. Review for correctness and edge cases."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/proxy.py` around lines 155 - 157, The current branch calls await upstream.aread() then client.aclose(), but if aread() raises we leak client and convert the upstream error into a 500; wrap the aread() call in a try/except/finally (or try/finally) so client.aclose() is always awaited regardless of aread() outcome, and propagate or handle the original exception appropriately; specifically, modify the block around upstream.aread() and client.aclose() to ensure client.aclose() runs in a finally clause and that you either return the upstream status with its body when aread() succeeds (using upstream.aread() result) or re-raise/forward the aread() exception after closing the client so the error is not masked.
🧹 Nitpick comments (1)
tests/test_proxy.py (1)
505-525: ⚡ Quick winAssert the new error-path usage logging too.
This test no-ops
record_usage, so it won't catch regressions in the new failed-stream logging path. Capture the call and assert the basics (status,endpoint,ttft_ms) alongside the response assertions.Suggested assertion update
- monkeypatch.setattr(proxy_module, "record_usage", lambda **fields: None) + usage = {} + monkeypatch.setattr( + proxy_module, "record_usage", lambda **fields: usage.update(fields) + ) @@ assert response.status_code == 401 assert ( response.body == b'{"error":{"message":"Missing Authentication header","code":401}}' ) + assert usage["status"] == 401 + assert usage["endpoint"] == "/v1/chat/completions" + assert usage["ttft_ms"] is NoneAs per coding guidelines "Treat streaming, tool-call partial chunks, retry, timeout, and idempotency behavior as high-risk review areas. Review for correctness and edge cases."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_proxy.py` around lines 505 - 525, The test currently no-ops record_usage so it won't detect the new failed-stream logging; change the monkeypatch for proxy_module.record_usage to a small capture function (e.g., append kwargs to a list) before calling proxy_module.forward, then after the response assert that one usage record was captured and its basic fields match the failure: check status == 401, endpoint == "/v1/chat/completions" (or equivalent), and that ttft_ms exists/is a number; keep the existing response.status_code and body assertions intact and reference proxy_module.forward and proxy_module.record_usage to locate the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@AGENTS.md`:
- Around line 162-244: The committed claude-mem-context block in AGENTS.md
contains private session/memory data and must be removed: delete the entire
<claude-mem-context>...</claude-mem-context> section (the block with session IDs
and observations) and move any non-public details to a gitignored local file
such as AGENTS.local.md or .claude/*.local.md; update the commit (amend or new
commit) so the public file no longer contains that block and ensure repository
guidance (the lines describing keeping private data out) remains intact.
---
Duplicate comments:
In `@src/proxy.py`:
- Around line 155-157: The current branch calls await upstream.aread() then
client.aclose(), but if aread() raises we leak client and convert the upstream
error into a 500; wrap the aread() call in a try/except/finally (or try/finally)
so client.aclose() is always awaited regardless of aread() outcome, and
propagate or handle the original exception appropriately; specifically, modify
the block around upstream.aread() and client.aclose() to ensure client.aclose()
runs in a finally clause and that you either return the upstream status with its
body when aread() succeeds (using upstream.aread() result) or re-raise/forward
the aread() exception after closing the client so the error is not masked.
---
Nitpick comments:
In `@tests/test_proxy.py`:
- Around line 505-525: The test currently no-ops record_usage so it won't detect
the new failed-stream logging; change the monkeypatch for
proxy_module.record_usage to a small capture function (e.g., append kwargs to a
list) before calling proxy_module.forward, then after the response assert that
one usage record was captured and its basic fields match the failure: check
status == 401, endpoint == "/v1/chat/completions" (or equivalent), and that
ttft_ms exists/is a number; keep the existing response.status_code and body
assertions intact and reference proxy_module.forward and
proxy_module.record_usage to locate the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2dc322ed-dabc-4c4d-b31d-027c3e535b94
📒 Files selected for processing (7)
AGENTS.mdsrc/database/usage.pysrc/otlp.pysrc/proxy.pytests/test_database.pytests/test_otlp.pytests/test_proxy.py
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Why / Context
When Hermes (or any client) sends a streaming request with an invalid API key, the proxy forwarded the request to OpenRouter, got a 401, but returned it as a
StreamingResponsewith status 200. The 401 error body was embedded as stream data. The client saw empty content and retried endlessly instead of getting a proper 401.How It Works
Instead of immediately creating a
StreamingResponsefrom an async generator that callsclient.stream(), the new_forward_stream_or_error()function opens the upstream connection first, checks the response status, and:StreamingResponsewith a relay generatorJSONResponsewith the correct status codeThe relay generator manages the
httpx.AsyncClientlifecycle directly (client created in the outer scope, closed in the generator'sfinallyblock).Manual QA
401with error body (was 200 with embedded error)openrouter/owl-alpha)Testing
uv run python -m pytest -q: 481 passed (was 478, +1 new error-path test + 2 already accounted for)Risk Areas
Review
AGENTS.mdand.agents/commands/llm-tracker.md: yesKnown Limitations / Follow-ups
finallyblock; a client disconnect before the generator is consumed could leak the handle. This is a pre-existing pattern (old code had the same issue).