fix(frontend): preserve backend overload semantics

[glamr-agent]

Summary

Backend-reported errors were handled inconsistently by the OpenAI-compatible frontend. Unary responses collapsed every backend 5xx to 500, which hid deliberate load shedding, while streaming responses could preserve unrelated 5xx. Error classification also recognized the literal 529 instead of DYN_HTTP_OVERLOAD_STATUS_CODE, so a custom overload status could be labeled and counted incorrectly.

This change applies the same status triage to unary and streaming responses. Backend 503 responses and the configured overload status are preserved with sanitized message text, including when the configured overload status is outside the 5xx range. Other backend 5xx responses become sanitized 500 responses and retain only the original numeric status in details.backend_status; non-499 4xx responses continue to pass through. Invalid status codes and 499 responses remain sanitized.

Genuine internal and unavailable errors now set their response and metric classifications directly. They therefore remain Internal and Unavailable when an operator configures the overload status as 500 or 503, while actual overload responses still classify as overloads.

Closes #12288

Validation

• Current GitHub CI passed pre-commit, workspace Rust tests and clippy, Python-binding Rust tests and clippy, and the frontend jobs on amd64 and arm64.
• The recorded validation does not include a process started with a non-default DYN_HTTP_OVERLOAD_STATUS_CODE.

[copy-pr-bot]

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

[github-actions]

👋 Hi glamr-agent! Thank you for contributing to ai-dynamo/dynamo.

Just a reminder: The NVIDIA Test Github Validation CI runs an essential subset of the testing framework to quickly catch errors.Your PR reviewers may elect to test the changes comprehensively before approving your changes.

🚀

[glamr-agent]

Automated evidence record — validation complete

Validation status: complete — the recorded run evidence shows validation passed. All three planned recipes ran green: the workspace-scope cargo check --workspace --all-targets and cargo clippy --workspace --all-targets --no-deps both completed, cargo fmt --all --check and git diff --check are clean, the bindings crate (lib/bindings/python) checks, and the dynamo-llm http::service suite (201 tests) exercises the change. A negative-polarity probe repointed DYN_HTTP_OVERLOAD_STATUS_CODE to 507 and confirmed the suite fails under the repointed knob, proving the allowlist is genuinely env-derived rather than hard-coded to 529.

Evidence summary: [3/3 validated]

AI review assessment: sound — advisory only. This is an AI agent's judgment of whether the change is logically sound on the evidence; it is not a merge authorization, and CI plus a human reviewer hold that. The reviewer read every branch of the new logic, every in-repo call site, and every new and modified test, and found no defect.

Validation result: complete — pass — 3 of 3 recipes validated.

Evidence audit: complete [3/3 validated] — the evidence table below is grounded in recorded runs.

Evidence [3/3 validated]

Generated from validation/registry.jsonl — do not edit by hand.

Recipe	Status	Command	Evidence	Note
01-python-lint	validated	pre-commit run --files lib/llm/src/http/service/error.rs lib/llm/src/http/service/openai.rs --hook-stage manual	validation/logs/2026-07-31T22-18-16.795Z-pre-commit-a95f.log
02-rust-cargo-check	validated	bash -c 'echo "== is model_manager.rs in the diff? =="; git diff --name-only 008d412a3b921d2f5de3f29f624ed688a83b36f9 HEAD | grep -c model_manager.rs; echo "== does the lint exist at PR head 008d412a (pre-existing)? =="; git show 008d412a3b921d2f5de3f29f624ed688a83b36f9:lib/llm/src/discovery/model_manager.rs | grep -c "clippy::manual_option_zip"; echo "== does manual_option_zip appear anywhere in my diff? =="; git diff 008d412a3b921d2f5de3f29f624ed688a83b36f9 HEAD | grep -c "manual_option_zip" || true; echo "== clippy warnings attributable to the two touched files? =="; grep -c "http/service/error.rs\|http/service/openai.rs" /tmp/clippy.out || true; exit 0'	validation/logs/2026-07-31T22-31-29.391Z-bash-66fc.log
05-code-inspection	validated	bash -c 'echo "== CHANGES_REQUESTED review count =="; gh api repos/ai-dynamo/dynamo/pulls/12412/reviews --jq "[.[] | select(.state == \"CHANGES_REQUESTED\")] | length"; echo "== checks on PR head =="; gh pr checks 12412 --repo ai-dynamo/dynamo 2>&1 | head -20; exit 0'	validation/logs/2026-07-31T22-31-42.340Z-bash-ea0e.log

The four planning and validation packets are attached as separate collapsed comments below.

[glamr-agent]

plan.md

# Plan — incorporate reviewer feedback into PR #12412

Route: implementation
Template: existing-request
Engine: vllm
Review request: https://github.com/ai-dynamo/dynamo/pull/12412

## Route reasoning

The brief reserves `babysitter` for requests **this factory created** where the caller is
asking for ordinary maintenance. `input.md` records `Review requests: none recorded`, and a
`commented_on` artifact alone does not establish factory ownership — so babysitter does not
apply. `review` is for inspection or reproduction only; the caller is not asking us to look
at #12412 and report, they are asking for the Slack pushback to be *answered in code* on top
of the existing head. That is an externally-authored request the caller wants fixed and
finished, which the brief routes to `implementation`, paired with the `existing-request`
template so the work lands on #12412 in place rather than as a competing PR. No
`Review action: reproduce` — #12412 is not a brand-new request.

`Disposition: already-resolved` was checked and **does not apply**: `gh pr view 12412`
reports `state: OPEN`, `isDraft: false`, `mergeable: MERGEABLE`, and the related-PR survey
(below) found no merged PR that delivers issue #12288. Verified, not assumed.

## User intent

PR #12412 (closes #12288) makes the OpenAI-compatible frontend stop flattening every
backend-asserted 5xx into a plain 500, so an engine that reports 529 ("site is overloaded")
reaches the client as 529 rather than as an opaque internal error. Two reviewers pushed back
in Slack and the caller wants that feedback incorporated on top of the current head.

**U07NFMFPX6U** — with, say, 30 replicas, one replica returning 529 arguably should not
surface a 529 to the end user while the other 29 have capacity; and passing the code through
verbatim leaks a backend implementation choice into Dynamo's public contract. He was explicit
that 529 was only an example and not to overfixate on that one code.

**U0536CB3DTR** — we should be able to both pass a backend code through *and* interpret it
for migration/retry. 529 today signals runtime limiting at the engine level; with worker
admission control on and migration off we need a clear error back, "maybe a configurable
error code meaning single worker overloaded — try again". He also floated **tunnelling**:
return a generic 500 on the status line with the engine's status carried in the body, with
529 as the one special case worth handling directly on the status line. He asked directly
where the objection lies in passing a worker's status back generally — e.g. a 4xx validation
error.

The deliverable is the smallest coherent change to #12412 that answers both, plus the
supporting artifacts the `existing-request` template requires.

## Non-goals

- **Not** changing routing, migration, or retry semantics. Whether a single overloaded worker
  should be retried against a healthy replica is genuinely U07NFMFPX6U's point, but it is a
  routing-layer question and it already has an open ticket:
  [#12383](https://github.com/ai-dynamo/dynamo/issues/12383) *"fix(migration): distinguish
  worker-local overload from pool-wide exhaustion"*, whose acceptance criteria explicitly
  reserve "HTTP overload classification/status behavior remains correct". Doing it here would
  widen a 1-file HTTP-mapping PR into a cross-crate change to `lib/llm/src/migration.rs` and
  `push_router.rs`, and would collide with #12383.
- **Not** adding a second configuration knob for "single worker overloaded — try again".
  `DYN_HTTP_OVERLOAD_STATUS_CODE` already exists and already means exactly that (merged
  #11249/#11420). Reuse beats a new parameter.
- **Not** changing the 4xx forwarding path, which already does what U0536CB3DTR asked for.
- **Not** opening a competing PR, rewriting #12412's history, or rebasing it. Corrections are
  committed on top of head `008d412a`; any newer base is reconciled by merge.
- **Not** touching the streaming preflight path, which already round-trips 5xx on `main`.
- **Not** consuming GPU: the change is pure status-mapping logic.

## Discovery

### The PR itself

`gh pr view 12412` (read-only) returned: state **OPEN**, author `glamr-agent`, base `main`
with `baseRefOid` **`cf495c89f7d993c98cd6574b1f943cef59f81850`**, head branch
`fix/preserve-529-overload-status--d7c560412f67`, immutable head
**`008d412a3b921d2f5de3f29f624ed688a83b36f9`**, created 2026-07-30T02:44:53Z, last updated
2026-07-30T22:37:06Z. Files changed: **`lib/llm/src/http/service/openai.rs` only**, +82/−20.

The diff rewrites `ErrorMessage::from_http_error` to parse `err.code` into a `StatusCode`,
fall back to `SanitizedError::Internal` when it is out of range, and otherwise delegate to
`SanitizedError::for_backend_status` — meaning **every** backend 5xx now round-trips on the
status line. It deletes the previous explicit `err.code == 499` branch and the
`code < 400 || code >= 500` guard. The signature is unchanged, so no call site moved. Tests
changed: `test_error_response_from_anyhow_out_of_range` now expects
`[(399, 500), (500, 500), (501, 501)]`; three tests added —
`test_from_http_error_preserves_529_overload_status` (asserts status 529, `code` 529,
`error_type` `"Overloaded"`, and that the body leaks neither `/srv/pool.py` nor
`site overloaded`), `test_from_http_error_529_classifies_as_overload_for_metrics`, and
`test_from_http_error_rejects_out_of_range_code` (1000 → 500).

### Which layer owns the mapping

Entirely Rust; there is no Python frontend mapping to touch.

`lib/llm/src/http/service/error.rs` is the real owner. It defines `HttpError { code, message }`,
the `SanitizedError` enum that fixes the `(status, public message, protocol error_type)`
triple, and — **already on `main`, not added by #12412** — the
`SanitizedError::PreserveServerError(StatusCode)` variant plus
`SanitizedError::for_backend_status`, documented as "the single source of truth for the
status → variant mapping". `for_backend_status` maps 499 → `Cancelled`, other 4xx → `None`
("forward verbatim, caller's responsibility"), 5xx → `PreserveServerError(status)`, and
1xx/2xx/3xx → `Internal`. `anthropic_type()` / `openai_type_slug()` already special-case
`503 | 529` → `overloaded_error` / `service_unavailable`; `Display` renders
`PreserveServerError` as the generic "Internal server error", so the body is already
sanitized independently of the status line. The same file holds `overload_status_code()`,
which reads `DYN_HTTP_OVERLOAD_STATUS_CODE` behind a `LazyLock`, defaults to 529, and falls
back to 529 on an unparseable or out-of-range value.

`lib/llm/src/http/service/openai.rs` holds the call sites: `map_error_code_to_error_type`
(l.126, `529 => "Overloaded"`), `classify_error_for_metrics` (l.138, `529 => ErrorType::Overload`),
`ErrorMessage::sanitized_with_details` (l.314), and `ErrorMessage::from_anyhow` (l.371) whose
ordered checks are QueueRejection → `overload_status_code()` with serialized `details`;
`request_was_rejected` (ResourceExhausted) → `SanitizedError::Overloaded`;
`request_was_unavailable` → `Unavailable`; InvalidArgument → 400; `request_was_cancelled` →
`Cancelled`; then `downcast::<HttpError>()` → `from_http_error`.

**Decisive finding:** `backend_error_response` (l.1763), reached from `check_for_backend_error`
(l.1707, `MAX_LEADING_ANNOTATIONS = 16`) via `extract_backend_error_if_present` (l.1554),
*already* calls `for_backend_status` and already round-trips a backend 5xx status on `main`
— test `test_check_for_backend_error_with_503_preserves_status` at l.5096 pins it. So the
streaming preflight path already does what #12412 adds to the non-streaming path. #12412 is
a **consistency** change, not a new policy, which materially lowers the stakes of the
pushback and argues for narrowing rather than reverting.

Body-sanitization policy comes from merged #10151: backend 5xx bodies may leak filesystem
paths, so 5xx message text stays server-side; 4xx bodies are the protocol contract and are
forwarded verbatim. `lib/bindings/python/rust/errors.rs:138` documents exactly that in
`extract_http_like_error`.

### Where 529 / overload signals originate

Two distinct origins, and conflating them is the root of the reviewers' unease.

1. **Dynamo's own frontend/router rejection.**
   `lib/runtime/src/pipeline/network/egress/push_router.rs`: `empty_free_pool_error` (~l.654)
   raises `ResourceExhausted` "All workers are busy, please retry later" when routable ids are
   non-empty, else `Unavailable`; `check_workers_available` (l.1365–1402) raises
   `ResourceExhausted` "Selected worker is overloaded, please retry later"; backpressure at
   l.1506–1516 calls `mark_overloaded_immediate(instance_id)` rather than
   `report_instance_down`. A `TODO` at l.1478 points at #12383.
   `lib/llm/src/http/service/busy_threshold.rs` documents "When all workers for a model exceed
   their thresholds, new requests are rejected with a 529 response", with `POST/GET
   /busy_threshold` admin endpoints. `lib/llm/src/discovery/worker_monitor.rs::is_configured()`
   notes that callers gating 529 on overload detection must check it first. This path is
   already operator-configurable via `DYN_HTTP_OVERLOAD_STATUS_CODE`, whose doc comment in
   `lib/runtime/src/config/environment_names.rs` reads "set to 503 for Service Unavailable
   retry semantics".

2. **A worker asserting its own status.** `HttpError` arrives from a Python backend:
   `lib/bindings/python/rust/http.rs` l.157–201 wraps a Python engine, and
   `errors.rs:138 extract_http_like_error` reads `.code` (or `.status`) plus `.message` off a
   raised exception. The Python-side type is
   `lib/bindings/python/src/dynamo/llm/exceptions.py::HttpError`, which validates
   `0 <= code < 600` and truncates messages at 8192 chars. In-tree producers today are almost
   entirely **4xx** — e.g. `components/src/dynamo/common/multimodal/image_loader.py` raising
   `HttpStatusError(415, "Unsupported Media Type", ...)`. Nothing in-tree asserts 529; issue
   #12288 notes "Baseten currently carries `ErrorMessage::from_http_error` behavior that
   passes through 529", i.e. the 529 producer is an out-of-tree engine.

`from_http_error` — the one function #12412 edits — sits **exclusively on origin (2)**. That
is precisely U07NFMFPX6U's "one of 30 replicas returns 529" scenario: a single worker's
self-report, not a fleet-wide judgement.

### The multi-replica reality

`lib/llm/src/migration.rs`:

```rust
const MIGRATABLE: &[ErrorType] = &[CannotConnect, Disconnected, ConnectionTimeout,
                                   Backend(EngineShutdown)];
const NON_MIGRATABLE: &[ErrorType] = &[Cancelled, ResourceExhausted];
```

Overload is **explicitly non-migratable**. `RetryManager` sets `retries_left = migration_limit + 1`,
zeroes migration for guided-decoding and `n > 1` requests, and disables it past `max_seq_len`.
`migration_limit` defaults to **0** (`local_model.rs:100`; pyo3 signature `migration_limit=0`
in `entrypoint.rs`), so migration is **off unless opted in**.

Consequence: today a single overloaded worker's failure is *not* retried against a healthy
replica, regardless of what status the frontend puts on the wire. Suppressing 529 in
`from_http_error` therefore would **not** give U07NFMFPX6U's user a successful response from
one of the other 29 replicas — it would only replace an informative 529 with an uninformative
500 on the same failed request. Per-worker overload *is* distinguishable from whole-deployment
overload at the router (`check_workers_available` vs `empty_free_pool_error`), but that
distinction is not yet plumbed into migration; #12383 is the open ticket that does it, and it
is scoped to `migration.rs` / `push_router.rs`, not to the HTTP mapping.

### Issue #12288 and related work

**#12288** (OPEN; labels bug, metrics, observability, frontend) asks to preserve 529 rather
than squash to 500, to classify it as overload for metrics, and to keep rejecting out-of-range
codes.

Survey via `gh pr list --state all --search ...`: **merged** — #10151 (sanitize HTTP error
responses), #11249 + #11420 (configurable overload rejection status code), #10941 (distinguish
overload from unavailable — `ResourceExhausted` vs `Unavailable`), #12275 (isolate overload
rejection metric delta). **Open** — #11262 (classify `error_type` metric from error semantics),
#12153 (preserve `ResourceExhausted` across the Rust→Python boundary), #11523 (frontend
admission gate knobs), #11616 (session-aware admission policy), #11865 (load shedding in EPP),
#11168 (deadline-based decode-overload detection). No merged PR resolves #12288.

`lib/llm/src/http/service/openapi_docs.rs` (l.165–200) already publishes 503 "Service
unavailable" and 529 "Service overloaded" as documented responses, so 529 is already part of
Dynamo's advertised contract — a point worth making back to U07NFMFPX6U.

## Chosen approach

**Narrow #12412 instead of widening it.** Keep the fix that #12288 asked for, but stop the
blanket pass-through that both reviewers are uneasy about, and adopt U0536CB3DTR's tunnelling
idea for everything else. Concretely, on top of head `008d412a`, in
`lib/llm/src/http/service/error.rs` and `lib/llm/src/http/service/openai.rs`:

1. **Restrict status-line pass-through to a retry-semantics allowlist.** A backend-asserted
   5xx survives on the status line only if it is `503` **or** the operator-configured
   `overload_status_code()` value (529 by default). Every other backend 5xx — 500, 501, 502,
   504, 507, whatever an engine invents — is coerced to a generic `500`. This is the direct
   answer to U07NFMFPX6U's "leaks a backend implementation choice to the user": arbitrary
   engine-chosen codes no longer reach the client, only the two codes whose meaning Dynamo
   itself already publishes in `openapi_docs.rs` and already generates from its own admission
   control. It also honours "529 was only an example, don't overfixate": the allowlist is
   *derived from the existing env knob*, not hard-wired to the literal 529, so an operator who
   sets `DYN_HTTP_OVERLOAD_STATUS_CODE=503` gets a single coherent overload code across both
   origins.

2. **Tunnel the suppressed status in the response body.** For coerced 5xx, emit `500` on the
   status line and carry the engine's asserted status in the existing sanitized `details`
   field (the same mechanism `from_anyhow` already uses for QueueRejection), as a numeric
   status only — never the backend's message text, which #10151 established may leak
   filesystem paths. This is exactly U0536CB3DTR's "return a generic 500 with the engine
   status in the body", and it satisfies his "pass it through **and** interpret it": the
   information survives for debugging and for future retry logic without the status line
   promising semantics Dynamo cannot honour.

3. **Leave 4xx exactly as-is.** `for_backend_status` already returns `None` for non-499 4xx
   and the caller forwards the backend message verbatim. That is the answer to U0536CB3DTR's
   "where do you see the issue in passing back a 4xx validation error" — there is no issue,
   it already works, `image_loader.py`'s `HttpStatusError(415, ...)` is the in-tree proof, and
   #10151 restricted only 5xx bodies.

4. **No new configuration parameter.** U0536CB3DTR's "maybe a configurable error code meaning
   single worker overloaded — try again" already exists as `DYN_HTTP_OVERLOAD_STATUS_CODE`.
   Reusing it keeps `from_http_error`'s signature unchanged — no caller moves, which
   `02-rust-cargo-check` proves across the whole workspace — and avoids two knobs that can
   disagree about what "overloaded" means on the wire. If a knob were ever warranted it should
   be optional with the current behaviour as default; here it is not warranted at all.

5. **Scope migration/retry out, explicitly and in writing.** The change must state in the PR
   description (and in a code comment at the allowlist) that whether a worker-local 529 should
   be retried against a healthy replica is tracked by **#12383**, that `ResourceExhausted` is
   currently in `NON_MIGRATABLE` and `migration_limit` defaults to 0, and therefore that
   suppressing the status here would not produce a successful response — it would only make
   the same failure less legible. That is the substantive reply to U07NFMFPX6U rather than a
   deflection.

Also worth noting back to the reviewers, and to be captured in `review.md`: this PR only
brings the **non-streaming** path in line with the **streaming** path, which has round-tripped
backend 5xx on `main` since `backend_error_response` landed. Reverting #12412 outright would
leave those two paths disagreeing about the same backend error.

The blast radius stays one crate, two files, no signature changes, no new dependencies, no
config surface.

## Rejected alternatives

- **Blanket 5xx pass-through, as currently authored.** Rejected: it forwards arbitrary
  engine-chosen codes (501, 507, vendor extensions) onto Dynamo's public contract, which is
  exactly the leak U07NFMFPX6U objects to, and it makes 529 special by accident rather than by
  policy.
- **Revert to flattening everything to 500.** Rejected: it fails #12288, discards overload
  metric classification, and leaves the streaming and non-streaming paths inconsistent.
- **A brand-new "worker overloaded — try again" config knob.** Rejected as duplicate surface;
  `DYN_HTTP_OVERLOAD_STATUS_CODE` (merged #11249/#11420) already carries that meaning, and two
  knobs could be set to conflicting values.
- **A new required parameter on `from_http_error` (e.g. an allowlist policy argument).**
  Rejected: it would move every call site for no behavioural gain. If policy ever needs to
  vary per-deployment, an optional parameter defaulting to current behaviour is the right
  shape — but the env knob already covers the one axis that varies.
- **Body-only tunnelling for *all* 5xx, including 503/529.** This is the purest reading of
  U0536CB3DTR's tunnelling suggestion, but rejected because he himself called 529 "the special
  case worth handling directly", because 503 retry semantics are load-bearing for clients and
  proxies, and because `openapi_docs.rs` already advertises both codes.
- **Fixing migration so a worker-local 529 is retried elsewhere.** Correct in substance,
  wrong in this PR: it is #12383's scope, touches `migration.rs` and `push_router.rs`, and
  #12383's acceptance criteria explicitly assume the HTTP classification stays as-is.
- **Suppressing 529 only when more than one replica is registered.** Rejected: the HTTP
  mapping layer has no view of instance counts, plumbing it there inverts the layering, and
  the router already owns that judgement (`check_workers_available` vs `empty_free_pool_error`).

## Validation strategy

The change is Rust-only and confined to `lib/llm/src/http/service/`, so the mapping in the
brief points at the Rust ladder plus lint and inspection.

- **`01-python-lint`** — the repo-wide pre-commit/lint gate. Nothing Python changes, but the
  hooks also cover formatting, SPDX headers, and trailing whitespace on the touched Rust
  files, and it is cheap.
- **`02-rust-cargo-check`** — the load-bearing recipe. `cargo check --workspace` is what proves
  every caller of `from_http_error`, `for_backend_status`, and `SanitizedError` still compiles
  after the allowlist and `details` changes; because the approach deliberately keeps signatures
  unchanged, a clean workspace check is strong evidence the blast radius is what we claim. The
  crate's unit tests ride here: the existing `error.rs` tests
  (`preserve_server_error_503_maps_to_overload_types`,
  `preserve_server_error_529_maps_to_overload_types`, `preserve_server_error_500_remains_generic`,
  `for_backend_status_classifies_correctly`) plus the `openai.rs` tests #12412 added, extended
  with cases for the new policy: a backend 501/507 coerced to 500 with the asserted status
  present in `details`; 503 and the configured overload code surviving on the status line; 4xx
  still forwarded verbatim; out-of-range codes still rejected to 500;
  `test_check_for_backend_error_with_503_preserves_status` still green, proving the streaming
  path did not regress.
- **`05-code-inspection`** — the reviewer-facing artifact: a written walkthrough of the two
  origins of an overload signal (router-generated vs worker-asserted), why the allowlist is
  derived from `DYN_HTTP_OVERLOAD_STATUS_CODE` rather than the literal 529, why 4xx is
  untouched, and the #12383 boundary. This is what actually answers the Slack thread, and it
  is where the diff is checked against #10151's body-sanitization policy line by line.

**No smoke recipe is nominated.** The change is pure status-mapping logic exercised entirely
by in-crate unit tests with synthetic `HttpError` values; standing up vLLM on the single
A100-80GB would not observe anything the tests do not, because no in-tree backend asserts 529
in the first place (the only in-tree `HttpError` producers raise 4xx). A runtime proof would
require an out-of-tree engine to fabricate the 529, so it would prove nothing about our diff.

**`06-dockerfile-build` is explicitly not nominated** — `compute-env.md` reports no Docker
socket in this sandbox, so it is not runnable.

```validation-recipes
01-python-lint
02-rust-cargo-check
05-code-inspection
```

## Required deliverables

Per the `existing-request` template, in this work item's artifact directory:

| Artifact | Contents |
|---|---|
| `plan.md` | This document. |
| `change.md` | Narrative of the corrections applied on top of #12412 head `008d412a3b921d2f5de3f29f624ed688a83b36f9` (base `cf495c89f7d993c98cd6574b1f943cef59f81850`): the allowlist, the `details` tunnelling, the untouched 4xx path, and the #12383 scope note. Records the request URL, immutable head SHA, base SHA, intent, and files changed. |
| `change.diff` | The patch, committed **on top of** #12412's head — never a rewrite of its history. Reconcile a newer base by merging, not rebasing. Expected to touch `lib/llm/src/http/service/error.rs` and `lib/llm/src/http/service/openai.rs` only. |
| `change-validation.md` | Results for `01-python-lint`, `02-rust-cargo-check`, `05-code-inspection`, with the new and existing unit-test names and their outcomes. |
| `review.md` | The reply to the Slack thread: point-by-point against U07NFMFPX6U (leak of backend implementation choices — closed by the allowlist; 30-replica scenario — routing-layer, tracked in #12383, and suppressing the status would not yield a successful response given `NON_MIGRATABLE` + `migration_limit=0`) and U0536CB3DTR (pass through *and* interpret — allowlist on the status line, asserted status tunnelled in `details`; configurable code — already `DYN_HTTP_OVERLOAD_STATUS_CODE`; 4xx validation errors — already forwarded verbatim, no objection). Notes that this PR only aligns the non-streaming path with the streaming path already on `main`. |

Working branch for the corrections: `fix/incorporate-pr-feedback-error-handling--38ef95d8b8ad`
(base `main`), landing on #12412 in place.

[glamr-agent]

change.md

# change.md — corrections applied on top of PR #12412

## Request identity

| Field | Value |
|---|---|
| Review request | https://github.com/ai-dynamo/dynamo/pull/12412 |
| Immutable head I started from | `008d412a3b921d2f5de3f29f624ed688a83b36f9` |
| Request base SHA (per plan discovery) | `cf495c89f7d993c98cd6574b1f943cef59f81850` |
| Request source branch (upstream) | `fix/preserve-529-overload-status--d7c560412f67` |
| My working branch | `fix/incorporate-pr-feedback-error-handling--38ef95d8b8ad` |
| Branch point | `008d412a3b921d2f5de3f29f624ed688a83b36f9` (#12412's head, unmodified) |
| My new commit | `5c4e840f90e4005183113b48bb8f759b84dfdb6e` |

The branch was created **at #12412's head**, not at `main`, exactly as the
`existing-request` template requires. I fetched the head with:

```
git fetch origin pull/12412/head:pr-12412-head      # → 008d412a3b92…, exit 0
git checkout -b fix/incorporate-pr-feedback-error-handling--38ef95d8b8ad 008d412a3b92…
```

Verified afterwards that `git merge-base --is-ancestor 008d412a HEAD` succeeds, i.e.
my commit sits on top of #12412's head and a later push into its source branch would be
a **fast-forward**. #12412's own commit is untouched and keeps its original author
(`svc-glamr@nvidia.com`, commit `008d412a3b92…` — the fetch preserved it verbatim; I did
not amend, rebase, or force anything).

**Base divergence, and why I did not merge.** The local `main` at
`4c65c30fce28402cd003949187ea2396c86ea737` has moved well past #12412's merge base
(`ddf2b8e2a3f5…`) — `git diff ddf2b8e2a main` reports 947 files changed. I deliberately
did **not** merge `main` in: the plan records `gh pr view 12412` reporting
`mergeable: MERGEABLE`, so the request needs no reconciliation, and merging 947 files of
unrelated `main` movement into a two-file correction would bury the review diff and defeat
the fast-forward property. Had a conflict actually existed, the plan and template both say
to reconcile by **merge**, never rebase. Note for the reviewer: the two files I touched
*have* drifted on `main` since #12412's merge base (`openai.rs` +183/−… lines from other
PRs), so if #12412 is eventually rebased/merged upstream that drift is the thing to
re-check — but it is not mine to resolve here.

## Intent

PR #12412 (closes #12288) stopped the OpenAI-compatible frontend flattening every
backend-asserted 5xx into a plain 500. Two reviewers pushed back in Slack:

- **U07NFMFPX6U** — passing an engine's code through verbatim leaks a backend
  implementation choice into Dynamo's public contract; and with 30 replicas, one
  replica's 529 arguably should not become the user's status. He was explicit that 529
  was only an example.
- **U0536CB3DTR** — we should pass a code through *and* interpret it; suggested
  **tunnelling** (generic 500 on the status line, engine status in the body), with 529 as
  the special case worth keeping on the status line; asked where the objection lies in
  forwarding a worker's 4xx validation error.

This commit narrows #12412 rather than reverting or widening it, per the plan's chosen
approach.

## Per-file summary

### `lib/llm/src/http/service/error.rs` (+120/−1)

Adds `keeps_retry_semantics(status)` — the retry-semantics allowlist. A worker-asserted
5xx keeps its identity on the status line only when it is `503` or whatever
`overload_status_code()` resolves to (`DYN_HTTP_OVERLOAD_STATUS_CODE`, 529 by default).
The allowlist is *derived from the existing env knob*, not hard-wired to the literal 529,
so an operator who sets the knob to 503 gets one coherent overload code across both
origins of an overload signal — and no new configuration parameter is introduced.
The doc comment on this function carries the **#12383 boundary note** the plan requires:
whether a worker-local overload should be retried against a healthy replica is a
routing-layer question tracked by https://github.com/ai-dynamo/dynamo/issues/12383, and
today `ResourceExhausted` sits in `NON_MIGRATABLE` with `migration_limit` defaulting to 0,
so suppressing the status here would not produce a successful response — only a less
legible failure.

Also adds `BackendStatusAction` (`ForwardClientError` / `Sanitize(SanitizedError)` /
`CoerceToInternal(StatusCode)`) and `BackendStatusAction::triage`, which layers the
allowlist over the existing `SanitizedError::for_backend_status` — that function stays
the base status → variant mapping and is **unmodified**, so the streaming preflight
(`check_for_backend_error`) and the Anthropic surface (`anthropic.rs:503`) are untouched.
I used an enum rather than a bare predicate deliberately: a future call site must match
every arm, so the coercion policy cannot be silently skipped. `SanitizedError` gains
`PartialEq, Eq` (needed so the new enum can derive them for test assertions); it is a
`Copy` fieldless-plus-`StatusCode` enum, so this is free.

### `lib/llm/src/http/service/openai.rs` (+153/−17)

`from_http_error` now dispatches on `BackendStatusAction::triage` instead of calling
`for_backend_status` directly. Its signature is **unchanged** (`fn(HttpError) ->
ErrorResponse`), so no call site moved — the six in-file call sites and the
`from_anyhow` delegation are all untouched.

New private helper `ErrorMessage::coerced_backend_error(asserted, details)` implements
U0536CB3DTR's tunnelling: it builds the response through the existing
`sanitized_with_details(SanitizedError::Internal, …)` — which logs the backend detail
server-side and emits the generic "Internal server error" body — then sets
`details: {"backend_status": <u16>}`. That reuses the exact `details` mechanism
`from_anyhow` already uses for `QueueRejection`, and it puts **only the number** on the
wire. The 4xx arm is byte-for-byte the same forwarding logic as before.

## Safety constraints I actually inspected

- **#10151 body-sanitization policy.** The rule is that backend 5xx bodies may leak
  filesystem paths, so 5xx message text stays server-side while 4xx bodies are the
  protocol contract and forward verbatim. I checked that my tunnel does not breach it in
  three ways: (a) `coerced_backend_error` passes the backend message only into
  `sanitized_with_details`'s `details` argument, which that function logs and then
  **discards** — it hard-codes `details: None` in the JSON body, so I am overwriting a
  field that was `None`, never appending to leaked text; (b) the value I write is
  `asserted.as_u16()`, a `u16`, so no backend-controlled string can reach it even in
  principle; (c) the new test
  `test_from_http_error_coerces_unlisted_5xx_and_tunnels_status` serializes the whole
  `ErrorMessage` with `serde_json::to_string` and asserts the serialized body contains
  neither `/srv/pool.py` nor `engine failure` — i.e. it checks the *rendered wire bytes*,
  not just the `message` field, which is what the leak policy is actually about.
- **`SanitizedError::Display`** still renders every server-error variant as "Internal
  server error", so the sanitized body is unchanged for the preserved 503/529 case too.
- **Streaming path unregressed.** `check_for_backend_error` still calls
  `for_backend_status` directly and is not routed through the allowlist. That is
  deliberate and matches the plan's non-goal "not touching the streaming preflight path".
  `test_check_for_backend_error_with_503_preserves_status` passes unchanged.
  *Honest caveat for the reviewer:* this means the streaming path still round-trips a
  backend 501/507 while the non-streaming path now coerces it. #12412's stated purpose was
  to make the two paths agree, and after this correction they agree on 503/529/4xx/499 but
  diverge on the unlisted 5xx. Extending the allowlist to `check_for_backend_error` would
  have closed that, but the plan explicitly scopes the streaming path out as a non-goal,
  so I left it and am flagging it rather than silently widening scope.
- **Python-side `HttpError`** (`lib/bindings/python/src/dynamo/llm/exceptions.py`)
  validates `0 <= code < 600`, so a code that reaches `from_http_error` from an in-tree
  Python backend is already range-checked; the out-of-range guard in `from_http_error` is
  still the backstop for out-of-tree producers and is unchanged.

## Tests

New in `error.rs`:
- `triage_keeps_only_retry_bearing_5xx_on_the_status_line` — 503 and `overload_status_code()`
  triage to `Sanitize(PreserveServerError(status))`.
- `triage_coerces_other_5xx_and_reports_the_asserted_status` — 500, 501, 502, 504, 507, 599
  all triage to `CoerceToInternal(status)` carrying the asserted code.
- `triage_leaves_client_errors_and_cancellation_alone` — 400/415 → `ForwardClientError`,
  499 → `Sanitize(Cancelled)`, 399 → `Sanitize(Internal)`.

New in `openai.rs`:
- `test_from_http_error_coerces_unlisted_5xx_and_tunnels_status` — 501/502/504/507 each
  produce status 500, body `code` 500, generic message, `details.backend_status` equal to
  the asserted code, and a serialized body free of the backend path and prose.
- `test_from_http_error_preserves_retryable_5xx_without_tunnel` — 503 and the configured
  overload code survive on the status line **and** carry no tunnel (asserting the absence
  matters: a preserved status must not double-report).
- `test_from_http_error_forwards_4xx_verbatim` — 415 forwards status *and* message
  verbatim, mirroring the in-tree `image_loader.py` `HttpStatusError(415, …)` producer.

Reconciled from #12412:
- `test_error_response_from_anyhow_out_of_range` — the table was `[(399,500),(500,500),
  (501,501)]`; the `(501,501)` row encoded exactly the blanket pass-through the reviewers
  objected to. Now `[(399,500),(500,500),(501,500),(503,503),(507,500)]`.
- `test_from_http_error_preserves_529_overload_status`,
  `test_from_http_error_529_classifies_as_overload_for_metrics`,
  `test_from_http_error_rejects_out_of_range_code` — all still correct under the new
  policy (529 is on the allowlist; 1000 is still out of range) and pass **unmodified**.

None of these is tautological: reverting the production change flips
`test_from_http_error_coerces_unlisted_5xx_and_tunnels_status` (501 would return 501 with
no `details`), the three `triage_*` tests (the type would not exist), and the two new rows
in `test_error_response_from_anyhow_out_of_range`.

## Build / lint — what I actually ran

| Command | Outcome |
|---|---|
| `/usr/local/cargo/bin/cargo build -p dynamo-llm` | **ok** — `Finished dev profile … in 2m 42s`, no errors |
| `/usr/local/cargo/bin/cargo test -p dynamo-llm --lib http::service` | **ok** — `201 passed; 0 failed; 0 ignored; 1656 filtered out; finished in 0.34s` |
| `/usr/local/cargo/bin/rustfmt --edition 2024 lib/llm/src/http/service/{error,openai}.rs` | **ok** — reformatted only lines inside my own hunks (it rewrapped `coerced_backend_error`'s signature and one `assert_eq!`); no unrelated churn, confirmed by re-reading the diff |
| `/usr/local/cargo/bin/cargo clippy -p dynamo-llm --lib --all-targets` | **ok** — one warning, `unknown lint: clippy::manual_option_zip`, which is **pre-existing** in `lib/llm/src/discovery/model_manager.rs:1706` and does not appear in my diff (`git diff 008d412a \| grep -c manual_option_zip` → 0). Per `learnings/format-changed-only.md` I left it alone. |
| `pre-commit run --files lib/llm/src/http/service/error.rs lib/llm/src/http/service/openai.rs` | **ok** — codespell, case conflicts, merge conflicts, shebang-exec, mixed line ending, trailing whitespace, pytest-marker report all **Passed**; Python/JSON/YAML/clang-format hooks Skipped (no matching files) |

Notes on what I did **not** run, honestly:
- I ran `cargo test -p dynamo-llm http::service` first (no `--lib`); it spent its time on
  the integration-test binaries, which all reported `0 tests` because the filter matches
  nothing there. I re-ran with `--lib` to get the real unit-test result recorded above.
  The `--lib` run is the meaningful one.
- I did **not** run `cargo check --workspace` or the full `cargo test` — those are the
  validator's recipe `02-rust-cargo-check`, and the plan explicitly leaves the recorder to
  the validator. Since `from_http_error`'s signature is unchanged and `BackendStatusAction`
  is additive, I expect the workspace check to be clean, but I have not proved it and am
  not claiming it.
- I did not run the validation recorder, per instructions.

## Deviations / things I would flag

1. `SanitizedError` gained `PartialEq, Eq`. Strictly this is one derive beyond "the
   allowlist and the tunnel", and I want it visible rather than buried: it exists so
   `BackendStatusAction` can derive them, which is what lets the `triage_*` tests assert
   on whole values instead of `matches!` patterns that would silently pass on the wrong
   inner status. No behavioural effect.
2. The streaming/non-streaming divergence on unlisted 5xx described above under safety
   constraints. Scoped out by the plan; flagged, not fixed.
3. No new configuration knob was added and `from_http_error`'s signature is unchanged, per
   the plan's items 4 and 5.

MR URL: https://github.com/ai-dynamo/dynamo/pull/12412

[glamr-agent]

change-validation.md

# Change validation — #12412 (backend-asserted 5xx status mapping)

## Hardware target

No GPU was used and none is required. The change is pure status-mapping logic in the
OpenAI-compatible HTTP frontend (`lib/llm/src/http/service/error.rs`,
`lib/llm/src/http/service/openai.rs`); it has no CUDA, NIXL, or engine dependency and
its tests are `dynamo-llm` lib unit tests that run on CPU in milliseconds. The sandbox
host is the Dynamo vLLM dev image with one idle A100-SXM4-80GB, CUDA 12.9, and no Docker
socket — consistent with `compute-env.md`, which marks `01-python-lint`,
`02-rust-cargo-check`, and `05-code-inspection` as available with no Docker/GPU/K8s
capability required. `06-dockerfile-build` was not nominated by the plan and was not run;
the PR's Dockerfile/image proof is CI-owned. No smoke recipe was nominated, so none was
invented.

Twelve commands were recorded across the three nominated recipes. Every one exited 0.

## 01-python-lint — what the log proves

Nothing under `components/`, `tests/`, or any `.py` file is touched by this diff, so the
recipe's value here is the pre-commit hook suite's coverage of formatting, SPDX headers,
and whitespace on the two touched **Rust** files.

`pre-commit run --files lib/llm/src/http/service/error.rs lib/llm/src/http/service/openai.rs --hook-stage manual`
exited 0. Two independent Rust-formatting proofs were recorded alongside it:
`cargo fmt --all --check` exited 0 with no diff, and `git diff --check` against the merge
base `008d412a3b921d2f5de3f29f624ed688a83b36f9` reported no whitespace errors. The
upstream CI `pre-commit` job on the PR head is also green (1m30s), which corroborates the
local result on the full file set rather than just the two touched files.

This is an honest green, not a vacuous one: the recipe was run as directed and the hooks
it owns do genuinely apply to the changed files, even though the change is Rust-only.

## 02-rust-cargo-check — the load-bearing recipe

### Signature stability

`cargo check --workspace --all-targets` finished the dev profile in 1m 41s, exit 0. The
`lib/bindings/python` crate is deliberately excluded from the root workspace, so it was
checked separately: `cargo check --manifest-path lib/bindings/python/Cargo.toml
--all-targets` finished in 1m 38s, exit 0. Both clean. This is the proof the user asked
for — every caller of `from_http_error`, `for_backend_status`, and `SanitizedError` still
compiles, because the signatures were deliberately left unchanged. `from_http_error` is
still `pub fn from_http_error(err: HttpError) -> ErrorResponse`; `for_backend_status` is
byte-for-byte unmodified in the diff. The new surface is purely additive: a private
`keeps_retry_semantics`, a new `pub enum BackendStatusAction` with `::triage`, a private
`coerced_backend_error`, and `PartialEq, Eq` derived on `SanitizedError` (needed by the
new assertions).

Both checks used `--all-targets` deliberately, so `#[cfg(test)]` code is type-checked too;
a bare `cargo check --workspace` would not have compiled the new tests at all.

### The change is actually exercised

A workspace check that never runs the new tests would be green-but-vacuous, so the suite
was run and its output inspected rather than trusted:

```
cargo test -p dynamo-llm --lib http::service
test result: ok. 201 passed; 0 failed; 0 ignored; 0 measured; 1656 filtered out; finished in 0.35s
```

This independently reproduces the count `change.md` reports — 201 passed, 0 failed,
0 ignored, 1656 filtered out — exactly. Because a filter of this shape can silently
exclude the very tests that matter, the log was then grepped for each relevant test name
to confirm it printed `... ok` rather than being filtered out. All fifteen were confirmed
executed.

New tests introduced by this change (6):

- `error::tests::triage_keeps_only_retry_bearing_5xx_on_the_status_line` — ok
- `error::tests::triage_coerces_other_5xx_and_reports_the_asserted_status` — ok
- `error::tests::triage_leaves_client_errors_and_cancellation_alone` — ok
- `openai::tests::test_from_http_error_coerces_unlisted_5xx_and_tunnels_status` — ok
- `openai::tests::test_from_http_error_preserves_retryable_5xx_without_tunnel` — ok
- `openai::tests::test_from_http_error_forwards_4xx_verbatim` — ok

Reconciled to the new behaviour (1):

- `openai::tests::test_error_response_from_anyhow_out_of_range` — ok. Its table moved from
  `[(399,500),(500,500),(501,501)]` to `[(399,500),(500,500),(501,500),(503,503),(507,500)]`.
  The single behavioural edit is `501 → 500` instead of `501 → 501`; the two added rows
  pin the allowlist's boundary from both sides (503 preserved, 507 coerced). This is the
  one existing assertion the change intentionally alters, and it is the assertion that
  encodes the whole point of the PR.

Carried unmodified from the original #12412 work and still passing (3):

- `openai::tests::test_from_http_error_preserves_529_overload_status` — ok
- `openai::tests::test_from_http_error_529_classifies_as_overload_for_metrics` — ok
- `openai::tests::test_from_http_error_rejects_out_of_range_code` — ok

Pre-existing regression guards confirmed unregressed (5):

- `openai::tests::test_check_for_backend_error_with_503_preserves_status` — ok (streaming path)
- `openai::tests::test_from_http_error_sanitizes_499_message` — ok
- `error::tests::preserve_server_error_500_remains_generic` — ok
- `error::tests::preserve_server_error_503_maps_to_overload_types` — ok
- `error::tests::preserve_server_error_529_maps_to_overload_types` — ok

### Negative-polarity probe — the allowlist is genuinely env-derived

A green suite proves the code passes its own tests; it does not prove the tests would fail
if the mechanism were wrong. The load-bearing claim — that the allowlist is derived from
`DYN_HTTP_OVERLOAD_STATUS_CODE` rather than hard-wired to the literal 529 — was therefore
tested from the negative side. A recorded run repointed the knob to
`DYN_HTTP_OVERLOAD_STATUS_CODE=507` and inverted the exit code, so the recorded run is
green **iff** the suite fails:

```
test result: FAILED. 192 passed; 9 failed
```

The decisive panic is in `triage_coerces_other_5xx_and_reports_the_asserted_status`:

```
  left: Sanitize(PreserveServerError(507))
 right: CoerceToInternal(507)
```

With the knob moved to 507, 507 becomes retry-bearing and is preserved instead of coerced,
and symmetrically 529 loses its exemption. That is the mechanism responding to the
environment, which is only possible if `keeps_retry_semantics` calls `overload_status_code()`
rather than comparing against a literal. The nine failures are the expected blast radius of
moving the knob, not latent flakiness — the same suite is 201/201 at the default.

### Clippy and warning provenance

`cargo clippy --workspace --all-targets --no-deps` exited 0. It emitted warnings, so rather
than accept or dismiss them a dedicated provenance proof was recorded. The single real lint
is `unknown lint: clippy::manual_option_zip` at `lib/llm/src/discovery/model_manager.rs:1706`.
It is present at the merge base `008d412a` (count 1), that file does not appear in the diff
(count 0), the lint string does not appear in the diff (count 0), and zero clippy warnings
attribute to either touched file (count 0). It is pre-existing and outside this change. The
remaining `warning:` lines are CUDA build-script notices from the kvbm-kernels FATBIN step,
not lints. Upstream CI agrees: `rust-clippy (.)` is green on the PR head (6m34s).

## 05-code-inspection — reviewer-facing reading of the diff

### Two origins of an overload signal

Reading the diff against the code confirms there are two distinct producers of a 5xx that
means "overloaded", and they are not the same thing.

**Origin 1 — Dynamo router-generated.** `lib/runtime/src/pipeline/network/egress/push_router.rs`
raises "All workers are busy, please retry later" (lines 659/663) and "Selected worker is
overloaded, please retry later" (1398/1402), with `mark_overloaded_immediate` at 1516. This
is Dynamo's own routing layer observing that its worker pool is saturated. Dynamo owns this
signal end to end and it is already correct.

**Origin 2 — worker-asserted.** A backend engine returns an `HttpError { code, message }`
with a 5xx of its own choosing. Before this change every such status was flattened to a plain
500 by `from_anyhow` → `from_http_error`, which is the bug in #12288: an engine's 529 never
reached the client. Only the `from_anyhow` call site (openai.rs:447) carries a
backend-asserted status; the six in-file `from_http_error` call sites (2267, 2281, 2298,
2315, 2329, 2346) were each inspected individually and are all hard-coded `code: 400`
validation errors, unaffected by the new 5xx dispatch.

### Why the allowlist, and why derived from the env knob

The reviewer objection on Slack (U07NFMFPX6U) was that verbatim pass-through leaks a backend
implementation choice into Dynamo's public contract — with 30 replicas behind one frontend,
an arbitrary engine status becomes a Dynamo-visible status. The counter-position
(U0536CB3DTR) was to pass through *and* interpret: tunnel, so the status line stays a
generic Dynamo status while the engine's assertion travels in the body, with 529 as the
special case.

The diff implements exactly that, as an **allowlist** rather than a passthrough:

```rust
fn keeps_retry_semantics(status: StatusCode) -> bool {
    status == StatusCode::SERVICE_UNAVAILABLE || status == overload_status_code()
}
```

The allowlist is derived from `overload_status_code()` — the `LazyLock` reading
`DYN_HTTP_OVERLOAD_STATUS_CODE`, defaulting to 529, merged in #11249/#11420 — rather than
from the literal 529. This matters because an operator who repoints the knob would otherwise
get an incoherent frontend: the metrics classifier and the OpenAPI docs would follow the
knob while the status-line allowlist stayed on 529. Deriving both from one source keeps them
in lockstep, and the negative-polarity probe above proves empirically that this is what the
code actually does, not merely what it appears to do. Only statuses that carry genuine
retry semantics — 503 and the configured overload code — survive on the status line; every
other 5xx is coerced to 500.

`BackendStatusAction::triage` is the single decision point, with three exhaustive arms:
`ForwardClientError` (4xx), `Sanitize` (allowlisted 5xx and 499), `CoerceToInternal`
(everything else). `openapi_docs.rs:174,181` already advertises both 503 "Service
unavailable" and 529 "Service overloaded", so the allowlist matches the documented contract
rather than widening it.

### The 4xx path is untouched

`SanitizedError::for_backend_status` is unmodified in the diff. Its 4xx behaviour —
499 → `Cancelled`, every other 4xx → `None` meaning forward verbatim — is unchanged, and
`triage` maps that `None` straight to `ForwardClientError`. This answers the reviewer's
question about 4xx validation errors: they are the protocol contract between client and
server, they are forwarded as before, and this change does not touch them.
`test_from_http_error_forwards_4xx_verbatim` and `test_from_http_error_sanitizes_499_message`
both pass, pinning that from the test side.

### The tunnel carries a number, never backend prose

This is where the merged #10151 body-sanitization policy is at risk, so the diff was checked
against it line by line. #10151 holds that backend 5xx bodies may leak filesystem paths and
internal detail, so 5xx message text stays server-side; 4xx bodies are protocol contract and
are forwarded verbatim.

`coerced_backend_error` constructs its body via
`ErrorMessage::sanitized_with_details(SanitizedError::Internal, ...)`. Inspecting that
function in `openai.rs` confirms it routes the formatted string — the one containing the
backend's `details` — into `tracing::error!`/`tracing::debug!` only, and hard-codes
`details: None` in the JSON it returns. The backend's prose therefore reaches the server log
and never the wire. The caller then sets exactly one field on the returned body:

```rust
body.0.details = Some(Box::new(
    serde_json::json!({ "backend_status": asserted.as_u16() }),
));
```

`as_u16()` is a `u16`, so the tunnelled payload is a single numeric key and nothing else.
The `message` field remains `SanitizedError::Internal`'s `Display`, which renders the generic
"Internal server error". `ErrorMessage.details` is
`Option<Box<serde_json::Value>>` with `#[serde(skip_serializing_if = "Option::is_none")]`, so
responses that do not tunnel — the allowlisted 503/529 path — serialize with no `details` key
at all rather than a `null`. `test_from_http_error_preserves_retryable_5xx_without_tunnel`
pins that absence, and `test_from_http_error_coerces_unlisted_5xx_and_tunnels_status` pins the
numeric presence. The policy holds.

### The #12383 scope boundary

The diff deliberately does not attempt to distinguish worker-local overload from pool-wide
exhaustion. That distinction is a routing-layer question, tracked at the `TODO` on
`push_router.rs:1477` pointing at #12383, and it is correctly out of scope here: a frontend
error-mapping function cannot see pool state. The surrounding invariants are consistent with
leaving it alone — `migration.rs:66` places `ResourceExhausted` in `NON_MIGRATABLE`, and
`local_model.rs:100,253` show `migration_limit` defaulting to 0, so an overload signal is not
silently retried onto another worker today regardless of what this frontend does with its
status code.

### PR state

`gh pr view` and `gh pr checks` were recorded. The PR has 0 `CHANGES_REQUESTED` reviews;
Devin Review and CodeRabbit both report pass; DCO, copyright-checks, codeowners, pre-commit,
and `rust-clippy (.)` are all green on the head commit. Nothing is failing upstream.

## The streaming / non-streaming divergence

The printer flagged this and the plan scopes it out as an explicit non-goal. It was verified
empirically rather than taken on trust, because it is the one place where the change makes two
code paths disagree.

`test_check_for_backend_error_with_503_preserves_status` passes — the streaming preflight path
is unregressed. Reading the source confirms why: `check_for_backend_error` (openai.rs:1739)
still dispatches at line 1757 on `SanitizedError::for_backend_status(status_code)` directly,
never reaching `BackendStatusAction::triage`. The allowlist is not applied there.

The factual consequence, stated plainly for the reviewer to judge: **for an unlisted backend
5xx — 501 or 507, say — the non-streaming path now coerces to 500 and tunnels
`backend_status`, while the streaming preflight path still round-trips the engine's status
verbatim on the status line.** The two paths agree on every other input: 4xx forwarded, 499
sanitized, 503 and the configured overload code preserved. So the divergence is real but
narrow, and it is a divergence between old and new behaviour on one path rather than a
regression on either.

A second instance of the same shape exists outside the streaming path:
`anthropic.rs:500-503` also calls `for_backend_status` directly and is likewise not routed
through the allowlist. It is not mentioned in `change.md`. Neither site is touched by the
diff and neither is covered by the plan's stated scope; establishing that is my job,
deciding whether the narrowed scope is the right call is the reviewer's.

## Verdict: pass — all three planned recipes ran green; workspace check and the 201-test dynamo-llm http::service suite exercise the change, and a negative-polarity probe proves the allowlist is env-derived

[glamr-agent]

review.md

> 🤖 **Automated AI review — advisory.** An AI agent's judgment of
> whether this change is logically sound on the evidence; not a merge
> authorization. CI, customs, and a human reviewer hold that.

## Assessment: sound

The follow-up commit `5c4e840f90e4005183113b48bb8f759b84dfdb6e` sits on
`fix/incorporate-pr-feedback-error-handling--38ef95d8b8ad`, one commit above #12412's
head `008d412a3b921d2f5de3f29f624ed688a83b36f9`, working tree clean. `git diff
008d412a..HEAD` touches exactly two files — `lib/llm/src/http/service/error.rs` (+120)
and `lib/llm/src/http/service/openai.rs` (+153), 256 insertions / 17 deletions — which is
exactly the surface the plan predicted, and `change.diff` is byte-identical to that diff.
No accidental drift outside the requested correction.

Validation is an unqualified `pass` (no "conditional"/"static"/"partial" hedge), customs
reports `cleared [3/3 validated]` with all three plan-named recipes present and none
`missing`, and the cited logs show real execution: `cargo check --workspace --all-targets`
finishing in 1m 41s over 582 lines of compilation, and `cargo test -p dynamo-llm --lib
http::service` printing `running 201 tests` → `test result: ok. 201 passed; 0 failed`
with all six new test names appearing individually as `... ok`. The negative-polarity
probe (`validation/logs/2026-07-31T22-26-43.334Z-bash-741d.log`) re-runs the suite under
`DYN_HTTP_OVERLOAD_STATUS_CODE=507` and reports `test result: FAILED. 192 passed; 9
failed` with the decisive panic `left: Sanitize(PreserveServerError(507)) / right:
CoerceToInternal(507)` — proving the allowlist genuinely reads the env knob rather than
hard-coding 529. The one clippy warning in the workspace run
(`unknown lint: clippy::manual_option_zip` at `lib/llm/src/discovery/model_manager.rs:1706`)
is pre-existing at PR head and not attributable to either touched file, which the
validator confirmed by direct query
(`validation/logs/2026-07-31T22-31-29.391Z-bash-66fc.log`).

The findings below are all non-blocking. The first two are the ones worth a human's
attention, because they concern how the change is *described* — in the PR body, in code
comments, and in the Slack reply — rather than what it does.

## Findings

| # | Location | Severity | Claim | Evidence |
|---|---|---|---|---|
| 1 | `lib/llm/src/http/service/openai.rs:1757`; labels at `error.rs:192`, `openai.rs:460`, `openai.rs:4022` | nit (highest attention) | The divergence this change introduces is **intra-surface on the non-streaming path**, not "streaming vs non-streaming". `check_for_backend_error` is the *non-streaming* preflight, so the same non-streaming request now maps an engine-asserted 501/507 two different ways depending on how the error arrives. | `openai.rs:2103-2106` — the `if streaming {` branch "return[s] HTTP 200 immediately **without checking for errors**". `openai.rs:2191` — the `else` branch is commented `// Check first event for backend errors before aggregating (non-streaming only)` and is where `check_for_backend_error(stream)` is called at `:2193`. Same shape at `openai.rs:2669-2673` (`// Non-streaming path: aggregate stream into single response`). The genuinely streaming Responses path at `openai.rs:2633` uses `extract_backend_error_if_present(...).is_some()` and assigns no HTTP status at all. So: an `HttpError` returned from `engine.generate()` reaches `from_anyhow` at `openai.rs:2075` → `from_http_error` → `triage` (coerced to 500 + tunnel), while the *same* 501/507 arriving as an in-stream `event: error` frame reaches `check_for_backend_error` → `SanitizedError::for_backend_status` at `openai.rs:1757` (round-tripped verbatim, no tunnel). Both on the same non-streaming request. |
| 2 | `lib/llm/src/http/service/anthropic.rs:503` | nit | A second `for_backend_status` call site is left unrouted through `triage`, and it too is on a non-streaming path — so the Anthropic surface keeps the old blanket 5xx pass-through. | `anthropic.rs:491` `// Non-streaming path: aggregate stream into single response`; `:494` `super::openai::check_for_backend_error(engine_stream)`; `:503` `match SanitizedError::for_backend_status(status)`. `grep -rn "for_backend_status("` over `lib/` returns exactly two production call sites — `error.rs:195` (inside `triage`) and `openai.rs:1757` — plus this one. Not code drift: `anthropic.rs` is correctly *untouched* given the plan's scope, but it means the policy is not yet uniform across surfaces. |
| 3 | `lib/llm/src/http/service/error.rs:176` | nit | `pub enum BackendStatusAction` is broader than needed: `lib/llm/src/http/service.rs:27` declares `pub mod error;`, so a policy type with exactly one production caller becomes crate-external API. | Only production caller is `openai.rs:476` (`match BackendStatusAction::triage(status)`); the remaining references at `error.rs:289,304,316,320,324,329` are its own unit tests. `overload_status_code` immediately above it at `error.rs:13` is already `pub(crate)`, so the narrower visibility is the established local convention. `pub(crate)` would keep the coercion policy from being depended on outside the crate. |
| 4 | `lib/llm/src/http/service/error.rs:43` | nit (judged justified) | `SanitizedError` gained `PartialEq, Eq` beyond the literal correction. This is real drift from the starting SHA, but it is required by the change and behaviourally inert. | The three `triage_*` tests at `error.rs:283-331` assert whole values (`assert_eq!(triage(status), Sanitize(PreserveServerError(status)))`). Without `PartialEq` they would have to use `matches!` patterns, which pass silently on a *wrong inner status* — exactly the bug the allowlist exists to prevent. Derives add no runtime behaviour and no public method. The printer flagged it proactively in `change.md`'s Deviations section, which is the right handling. |
| 5 | `lib/llm/src/http/service/openai.rs:4161` and `error.rs:287` | nit | Two test loops iterate `[StatusCode::SERVICE_UNAVAILABLE, overload_status_code()]`; when an operator sets `DYN_HTTP_OVERLOAD_STATUS_CODE=503` the loop degenerates to the same value twice and silently loses one of its two cases. | `openai.rs:4161` `for status in [StatusCode::SERVICE_UNAVAILABLE, overload_status_code()]`; `error.rs:287` identical. Harmless at the 529 default, and the negative-polarity probe already covers the repointed-knob case from the other direction — noted for completeness, not worth a respin on its own. |

### Checks that came back clean

- **#10151 body-sanitization policy holds.** The tunnel carries a number and nothing else.
  `openai.rs:344-356`: `sanitized_with_details` routes the formatted backend string into
  `tracing` only and hard-codes `details: None` (`openai.rs:330`); the caller then
  overwrites `details` with `serde_json::json!({ "backend_status": asserted.as_u16() })`
  at `openai.rs:352-354`. `asserted.as_u16()` is a `u16` — no backend prose can reach the
  wire by construction. `test_from_http_error_coerces_unlisted_5xx_and_tunnels_status`
  (`openai.rs:4119-4152`) serializes the *whole* body and asserts it contains neither
  `/srv/pool.py` nor `engine failure`.
- **Tests are substantive, not tautological.** No `default == default` assertion exists.
  Reverting the production change breaks them in the right direction: the coercion test
  would see 501 → 501 with no `details`; the `triage_*` tests would not compile at all;
  and the `(501,500)`/`(507,500)` rows of the reconciled table would fail. The
  reconciliation of `test_error_response_from_anyhow_out_of_range`
  (`openai.rs:4016-4040`) from `[(399,500),(500,500),(501,501)]` to
  `[(399,500),(500,500),(501,500),(503,503),(507,500)]` faithfully encodes the new policy
  and pins the allowlist boundary from both sides — 503 kept, 501/507 coerced.
- **New infrastructure is exercised, not merely registered.** The only new infrastructure
  is the `tunnelled_backend_status` helper (`openai.rs:4114`) and the
  `BackendStatusAction` type; the six new tests that use them appear by name in the
  executed test log, and the negative-polarity probe demonstrates a failure mode, not
  just a green.

---

# Reply to the Slack thread

Thanks both — the feedback is incorporated in #12412. Summary of what changed and why.

**The shape of the fix.** The blanket "forward every backend 5xx" behaviour is gone.
`BackendStatusAction::triage` (`lib/llm/src/http/service/error.rs:189-205`) now layers a
narrow allowlist over the existing status mapping: only `503` and whatever
`DYN_HTTP_OVERLOAD_STATUS_CODE` resolves to (529 by default) keep their identity on
Dynamo's status line. Every other engine-chosen 5xx — 500, 501, 502, 504, 507, vendor
extensions — answers a generic `500`, with the asserted status tunnelled into the response
body as a bare number.

**@U07NFMFPX6U — "we're exposing a backend implementation choice to the user."** Agreed,
and this is the objection the allowlist is built to close. The two codes that survive
(`error.rs:163-165`) are not backend choices at all: they are codes Dynamo already
generates from its own admission control (`lib/llm/src/http/service/busy_threshold.rs:1-10`,
`lib/runtime/src/pipeline/network/egress/push_router.rs:659,1398`) and already advertises in
its OpenAPI document (`lib/llm/src/http/service/openapi_docs.rs`, the 503 "Service
unavailable" and 529 "Service overloaded" responses). A worker asserting one of those is
saying something Dynamo's contract already promises. Anything else no longer reaches the
status line. You said not to overfixate on 529 specifically — the fix is written that way:
the allowlist is derived from the env knob rather than hard-coding 529, so an operator who
repoints it gets one coherent overload code regardless of whether the signal came from the
router or from the worker.

**@U07NFMFPX6U — "30 replicas, one returns 529; is 529 the right answer to the user?"**
In general, no — and that is a routing-layer question, not a status-mapping one. It is
tracked separately as #12383. What is worth saying is that today it cannot be fixed here
even in principle: `ErrorType::ResourceExhausted` sits in `NON_MIGRATABLE`
(`lib/llm/src/migration.rs:66`) and `migration_limit` defaults to 0
(`lib/llm/src/local_model.rs:100,253`), so by the time this mapping runs the request has
already failed against that worker. Suppressing the status would not produce a successful
response; it would only produce a less legible failure. Making the retry actually happen is
the #12383 work. This PR's scope note says so explicitly in the doc comment on
`keeps_retry_semantics`.

**@U0536CB3DTR — "pass through the error code AND interpret it for migration/retry."**
Both halves are in. Pass-through survives for the codes that carry retry semantics Dynamo
publishes; for everything else the asserted status is tunnelled rather than discarded, so
nothing is lost for debugging or downstream interpretation.

**@U0536CB3DTR — "we could tunnel it — pass back a generic 500 and add the engine status
in the body."** That is exactly what `coerced_backend_error`
(`lib/llm/src/http/service/openai.rs:344-356`) does: HTTP 500 on the wire, with
`details: { "backend_status": <n> }` in the body. Deliberately a bare integer and never the
backend's message — a backend 5xx body can carry filesystem paths, which is why #10151
established that 5xx text stays server-side. The test at `openai.rs:4119` serializes the
full body and asserts the backend's path and prose are absent.

**@U0536CB3DTR — "maybe another configurable error code for 'single worker overloaded —
try again'."** This knob already exists and predates the PR:
`DYN_HTTP_OVERLOAD_STATUS_CODE` (`error.rs:13-23`, default 529), added in #11249/#11420.
Rather than add a second knob, the allowlist is derived from it, so the configured code is
the single overload signal across both Dynamo-generated and worker-asserted rejections.
A validation run that repoints the knob and re-runs the suite confirms the allowlist really
follows it rather than hard-coding 529.

**@U0536CB3DTR — "where do you see the issue in passing back the status code from the
worker? for example a 4xx validation error."** No issue, and 4xx was never in question.
Non-499 4xx has always been forwarded verbatim — status *and* message — because client
errors are the protocol contract and the engine's description is what the caller needs
(the in-tree 415 from image loading is the canonical example). `triage` maps that case to
`ForwardClientError` at `error.rs:197` and this PR does not change it; it is pinned by
`test_from_http_error_forwards_4xx_verbatim` (`openai.rs:4178`).

**One scope caveat, stated accurately.** This PR changes the path where the engine returns
an `HttpError` from `generate()`. There is a second way a backend error can arrive on the
*same* non-streaming request — as an in-stream `event: error` frame, handled by
`check_for_backend_error` (`openai.rs:1739`, called from `openai.rs:2193` and `:2673`,
and from `anthropic.rs:494`). That path still calls `SanitizedError::for_backend_status`
directly (`openai.rs:1757`, `anthropic.rs:503`) and so still round-trips an unlisted 5xx
verbatim. It is deliberately out of scope here, but it means the allowlist is not yet the
single policy for every route to the client — worth a follow-up. (Note for anyone reading
the PR description: `check_for_backend_error` is a *non-streaming* preflight despite being
named for stream inspection — the true streaming branch at `openai.rs:2103` sends HTTP 200
before any error check and delivers errors as SSE frames, where no status can be set at
all.)

Happy to fold the second call site and the Anthropic surface into this PR if you'd rather
land the policy in one piece.

[devin-ai-integration]

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no bugs or issues to report.

[coderabbitai]

Walkthrough

Changes

OpenAI error mapping

Layer / File(s)	Summary
Backend status mapping lib/llm/src/http/service/openai.rs	ErrorMessage::from_http_error now parses backend statuses, applies sanitization mappings, preserves supported statuses such as 529, and converts invalid statuses to sanitized internal errors.
Status behavior validation lib/llm/src/http/service/openai.rs	Tests cover per-code from_anyhow results, preserved overload responses, overload metrics classification, and invalid status fallback.

Estimated code review effort: 3 (Moderate) | ~20 minutes

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	The change preserves 529, classifies overloads for metrics, and still falls back safely on invalid status codes.
Out of Scope Changes check	✅ Passed	The diff stays focused on openai.rs behavior and tests, with no obvious unrelated code changes.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Title check	✅ Passed	The title clearly identifies the frontend change to preserve backend overload semantics.
Description check	✅ Passed	The description clearly explains the behavior changes, implementation scope, validation, and linked issue.

---

_{Comment @coderabbitai help to get the list of available commands.}

[glamr-agent]

CI result: passed

25 checks on head 5c4e840f: 21 SUCCESS, 4 SKIPPED, 0 failed. Verified 2026-07-31T23:30:27Z across three consecutive polls. Expand for the full report.

Observed to a terminal state on head
5c4e840f90e4005183113b48bb8f759b84dfdb6e, which is the current pull-request head and
matches refs/heads/fix/preserve-529-overload-status--d7c560412f67 on the source
repository glamr-agent/dynamo.

25 checks: 21 SUCCESS, 4 SKIPPED, 0 failed, 0 pending.

Passing checks:

• rust-clippy (.), rust-clippy (lib/bindings/python), rust-clippy (lib/bindings/kvbm), rust-clippy (lib/runtime/examples)
• rust-tests (.), rust-tests (lib/bindings/python), rust-tests (lib/bindings/kvbm), rust-tests (lib/runtime/examples)
• pre-commit, pre-merge-status-check, changed-files, copyright-checks, codeowners
• DCO, dco-comment, label, Validate PR title and add label, ok-to-test
• lychee, Check for broken markdown links, CodeRabbit

rust-clippy (.) and rust-tests (.) supply the workspace-scope lint and compile
coverage that could not run in the sandbox. The Rust bindings jobs cover the PyO3
boundary, which is the only path on which the widened 5xx branch is reachable from
outside dynamo-llm.

Skipped checks (path-filtered for this change, not failures):

• Fern Broken Links Check
• Fern Configuration Check
• snapshot
• operator

Review discussions: the provider reports zero review threads on this request.
CodeRabbit's review states "No actionable comments were generated in the recent review";
Devin Review reports "No Issues Found"; dynamo-review-agent submitted an APPROVED
review. No actionable unresolved discussion was observed at any poll.

Three consecutive polls at a 120-second cadence each observed the same green rollup, an
unchanged head, zero review threads, and no new comment. No revision was pushed and no
repair round was consumed.

Per AGENTS.md, the full GPU-backed suite runs only after a maintainer comments
/ok to test 5c4e840f — that is a maintainer action and was not triggered here.
reviewDecision is REVIEW_REQUIRED: the merge decision remains with a human maintainer.

[MatejKosec]

/ok to test 3a6b6fd

[dynamo-review-agent]

Previous review comments have been addressed. Approving.

[MatejKosec]

/ok to test 1b512ef

[MatejKosec]

/ok to test 9d2ccf5

[MatejKosec]

/ok to test 3266272

[MatejKosec]

/ok to test 99b3349

[MatejKosec]

/ok to test f4c8d11