Classify an upstream 4xx as a request error, not a transport failure #118

Merged

coilysiren merged 1 commit from upstream-error-classification into main

2026-08-13 17:57:25 +00:00

coilyco-ops commented

2026-08-13 17:55:00 +00:00

Member

Bug

Fixes #114.

One trace. LiteLLM answered 400 Bad Request. agent-proxy logged it as dispatch.transport_error, retried twice more with a byte-identical body, and answered the caller:

502 {"error":{"message":"sirens-echo/deepseek: all backends failed (litellm: Client
     error '400 Bad Request' ...)","type":"upstream_error"}}

Nothing about the transport had failed. LiteLLM answered promptly on every attempt and served a different request successfully 7.5 seconds later. Its own log recorded No fallback model group found, so no fallback was attempted either. One backend rejected one payload, and the response pointed the operator at capacity.

Root cause

upstream.py flattened every httpx.HTTPError into a bare UpstreamError, so a status-bearing rejection and a refused connection were indistinguishable downstream. The dispatcher's only policy for UpstreamError is retry-then-fail-over-then-AllBackendsFailed.

Fix

UpstreamStatusError carries status_code and a bounded body. is_retryable_status draws the line where the protocol draws it: every 5xx, plus 408, 425, 429.
A settled 4xx is not retried, does not walk the fallback chain, and leaves the backend's breaker closed. It raises UpstreamRequestRejected, logged as dispatch.request_rejected. Both streaming and non-streaming paths.
The caller receives the upstream's own status and body. A body that parses as the OpenAI error shape passes through with upstream_status added. A 400 is answered 400.
AllBackendsFailed becomes a subclass of a new BackendUnavailable, raised only when the chain actually offered more than one backend. The routes catch the base, so genuine chain exhaustion is still a 502.
An upstream non-2xx sets http.response.status_code on the upstream span and records an error. That is the span-fidelity half of #106 - a 500 that left every agent-proxy span reading has_error: false is why the reported 0.74% error rate could not be trusted.

How to verify

ward exec test - 9 new cases.
test_settled_4xx_is_not_retried asserts exactly one upstream call for a 400 with max_retries=2.
test_settled_4xx_leaves_the_breaker_closed runs four rejections against a threshold of 2 and asserts the breaker never opens.
test_retryable_statuses_still_retry is parametrized over 408/425/429/500/502/503.
test_upstream_rejection_reaches_the_caller_as_itself asserts the route answers 400 with the provider's message and no "all backends failed".

Test plan

Regression tests for the reported trace
Existing tests still pass (319 passed)
ward exec format-check, lint, typecheck, pre-commit all clean

Risk

Medium, and concentrated in one place: callers that treated every failure as a 502 will now see 4xx pass through. That is the point of the change, but it is a response-contract change for anything downstream that branches on status. sirens-echo currently surfaces "model backend unavailable, retry shortly" for these, which will become the provider's own message.

Three existing tests were updated rather than added to: a single-backend chain now raises BackendUnavailable instead of AllBackendsFailed, which is the acceptance criterion asking for it.

Also changed

Two new codes in the closed exception taxonomy: upstream_request_rejected, upstream_5xx. Doc and cardinality assertion updated (13 -> 15).
One new RequestOutcome value, upstream_rejected, kept apart from upstream_failed in the trajectory record.
New docs/upstream-error-classification.md.

Note

Branched off main, independent of #116 and #117.

## Bug Fixes #114. One trace. LiteLLM answered `400 Bad Request`. agent-proxy logged it as `dispatch.transport_error`, retried twice more with a byte-identical body, and answered the caller: ``` 502 {"error":{"message":"sirens-echo/deepseek: all backends failed (litellm: Client error '400 Bad Request' ...)","type":"upstream_error"}} ``` Nothing about the transport had failed. LiteLLM answered promptly on every attempt and served a different request successfully 7.5 seconds later. Its own log recorded `No fallback model group found`, so no fallback was attempted either. One backend rejected one payload, and the response pointed the operator at capacity. ## Root cause `upstream.py` flattened every `httpx.HTTPError` into a bare `UpstreamError`, so a status-bearing rejection and a refused connection were indistinguishable downstream. The dispatcher's only policy for `UpstreamError` is retry-then-fail-over-then-`AllBackendsFailed`. ## Fix - **`UpstreamStatusError`** carries `status_code` and a bounded `body`. `is_retryable_status` draws the line where the protocol draws it: every 5xx, plus `408`, `425`, `429`. - A settled 4xx is **not retried**, does **not walk the fallback chain**, and leaves the backend's **breaker closed**. It raises `UpstreamRequestRejected`, logged as `dispatch.request_rejected`. Both streaming and non-streaming paths. - The caller receives the **upstream's own status and body**. A body that parses as the OpenAI error shape passes through with `upstream_status` added. A 400 is answered 400. - `AllBackendsFailed` becomes a subclass of a new `BackendUnavailable`, raised only when the chain actually offered more than one backend. The routes catch the base, so genuine chain exhaustion is still a 502. - An upstream non-2xx sets `http.response.status_code` on the upstream span and records an error. That is the span-fidelity half of #106 - a 500 that left every agent-proxy span reading `has_error: false` is why the reported 0.74% error rate could not be trusted. ## How to verify 1. `ward exec test` - 9 new cases. 2. `test_settled_4xx_is_not_retried` asserts exactly one upstream call for a 400 with `max_retries=2`. 3. `test_settled_4xx_leaves_the_breaker_closed` runs four rejections against a threshold of 2 and asserts the breaker never opens. 4. `test_retryable_statuses_still_retry` is parametrized over 408/425/429/500/502/503. 5. `test_upstream_rejection_reaches_the_caller_as_itself` asserts the route answers 400 with the provider's message and no "all backends failed". ## Test plan - [x] Regression tests for the reported trace - [x] Existing tests still pass (319 passed) - [x] `ward exec format-check`, `lint`, `typecheck`, `pre-commit` all clean ## Risk Medium, and concentrated in one place: callers that treated every failure as a 502 will now see 4xx pass through. That is the point of the change, but it is a response-contract change for anything downstream that branches on status. `sirens-echo` currently surfaces "model backend unavailable, retry shortly" for these, which will become the provider's own message. Three existing tests were updated rather than added to: a single-backend chain now raises `BackendUnavailable` instead of `AllBackendsFailed`, which is the acceptance criterion asking for it. ## Also changed - Two new codes in the closed exception taxonomy: `upstream_request_rejected`, `upstream_5xx`. Doc and cardinality assertion updated (13 -> 15). - One new `RequestOutcome` value, `upstream_rejected`, kept apart from `upstream_failed` in the trajectory record. - New `docs/upstream-error-classification.md`. ## Note Branched off `main`, independent of #116 and #117.

coilyco-ops added 1 commit

2026-08-13 17:55:01 +00:00

Classify an upstream 4xx as a request error, not a transport failure

ci / smoke (pull_request) Successful in 7s

Details

ci / quality (pull_request) Successful in 23s

Details

23407be5a6

Issue #114 read one trace. LiteLLM answered 400 Bad Request. agent-proxy logged
it as dispatch.transport_error, retried twice more with a byte-identical body,
burned ~2.9 seconds of the caller's budget and three round-trips of backend
capacity to re-learn a settled fact, then answered the caller:

    502 {"error":{"message":"sirens-echo/deepseek: all backends failed (...)",
         "type":"upstream_error"}}

Nothing about the transport had failed. LiteLLM answered promptly on every
attempt and served a different request successfully 7.5 seconds later. Its own
log recorded no fallback model group, so no fallback was attempted either. One
backend rejected one payload, and the response pointed the operator at capacity.

upstream.py now raises UpstreamStatusError, carrying the status and a bounded
body, where it previously flattened every httpx failure into a bare
UpstreamError. is_retryable_status draws the line where the protocol draws it:
every 5xx, plus 408, 425, and 429. Every other 4xx is a statement about a body
that will not change between attempts.

The dispatcher stops retrying a settled 4xx, stops walking the fallback chain
for it, and leaves the backend's breaker closed. Asking a second backend the
same invalid question cannot help, and a backend that answers promptly is not
the broken thing. Both the non-streaming and streaming paths raise
UpstreamRequestRejected instead, logged as dispatch.request_rejected.

The caller receives the upstream's own status and body. Where the body parses as
the OpenAI error shape it passes through with upstream_status added, so the
caller reads the provider's account of what was wrong rather than a synthesized
one. A 400 is answered 400.

AllBackendsFailed becomes a subclass of a new BackendUnavailable and is raised
only when the chain actually offered more than one backend, so the phrase stops
appearing for a single backend that rejected a single payload. The routes catch
the base, so a genuine chain exhaustion is still a 502.

An upstream non-2xx now also sets http.response.status_code on the upstream
span and records an error, which is the span-fidelity half of #106: a 500 that
left every agent-proxy span reading has_error false is why the service's 0.74%
error rate could not be trusted. Cancellation propagation, the other half of
#106, is not in this change.

closes #114

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-authored-by: Kai Siren <coilysiren@gmail.com>
Co-authored-by: Claude <noreply@anthropic.com>