Skip to content

[Router] Derive error status from a failure class; preserve the worker's status (1/3) - #39463

Merged
ShangmingCai merged 1 commit into
mainfrom
router-4d-1-error-class
Sep 18, 2026
Merged

[Router] Derive error status from a failure class; preserve the worker's status (1/3)#39463
ShangmingCai merged 1 commit into
mainfrom
router-4d-1-error-class

Conversation

@Kangyan-Zhou

@Kangyan-Zhou Kangyan-Zhou commented Sep 14, 2026 •
edited by github-actions Bot
Loading

Copy link
Copy Markdown
Collaborator

Motivation

1 of 3 in the router request-attribution stack. Base is main.
#39464 stops an engine's backpressure from tripping the router's circuit
breaker; #39465 moves per-request logging and outcome accounting to one edge
middleware and consumes the status contract this PR defines.

A router-originated error picks its HTTP status per ApiError variant, at the
point the variant is written, with nothing tying the choice to what the
condition actually is. Two defects live in that gap, and both are visible to a
gateway or a client sitting in front of the router:

  • One condition, two statuses. UpstreamTimeout answers 502 while
    StaleRequestExpired answers 504, though both mean "the router gave up
    waiting". A retry policy or an alert keyed on 504 sees only half of the
    router's give-ups, and which half depends on which timer fired first.
  • A mid-body drop discards the worker's real status. When a worker answers
    — usually 200 — and then its body dies mid-flight, the client gets a bare
    502 and the one fact worth having, what the worker had actually said, is
    gone. The error's code (upstream_status) and message
    ("upstream returned an error status") also describe a failure that did not
    happen: the status it is carrying is generally a success.

Modifications

Status becomes a pure function of a new ErrorClass. Each ApiError
variant declares its class (BadRequest, NotFound, Upstream, NoTarget,
Timeout, Internal) and ErrorClass::status() is the only place a status is
chosen. UpstreamTimeout and StaleRequestExpired now share
ErrorClass::Timeout, so they answer the same 504 and cannot drift apart
again. The precise condition stays machine-readable in the existing
x-router-error-code, which is what a caller should branch on — the status is
deliberately coarse.

The worker's own status is preserved, in a new x-router-upstream-status
header. It is produced by ApiError::upstream_status(), an exhaustive match
rather than a catch-all, so a future variant that synthesizes a status over a
worker that did respond has to decide whether it echoes one. The mid-body
drop's code becomes upstream_body_incomplete, and its Display and
client-facing message now name an incomplete body instead of an error status.

A worker's own response is still forwarded verbatim and carries neither
header. That absence is the contract: a gateway tells engine-origin from
router-origin by whether x-router-error-code is present at all.

The streaming half is asymmetric on purpose. Once the response head has gone
out as 200 the status cannot be recalled, so a stream that dies mid-body leaves
the client with its 200 and no router headers; the error is recorded, not
re-signalled.

Dashboard: the status panel no longer reads 504 as "stale-cancel". After
this change three different conditions produce one — the stale deadline, the
router's own upstream timeout, and a worker 504 forwarded unchanged — and
worker_requests_total{outcome} is what tells them apart.

Tests

  • A table pinning the (status, error-code, upstream-status) triple for every
    ApiError variant, so a new variant cannot quietly join the contract.
  • Both timeout paths at 504 (fails before this change: UpstreamTimeout was
    502).
  • A non-streaming mid-body drop echoing the worker's status in
    x-router-upstream-status.
  • The streaming half: headers already sent as 200, client keeps the 200, no
    router headers at all.

cargo test green on this branch — 559 lib, 4 bin, 58 component, 94 proxy.
cargo clippy --all-targets -- -D warnings clean; pre-commit clean.

Checklist

  • Format your code according to the Format code with pre-commit.
  • Add unit tests according to the Run and add unit tests.
  • Update documentation — the Grafana dashboard's status-panel description is updated for the new 504 semantics; the new types and headers are documented in place.
  • Provide accuracy and speed benchmark results — N/A, error-path-only change with no effect on the serving hot path.
  • Follow the SGLang code style guidance.

🤖 Generated with Claude Code

https://claude.ai/code/session_012xa7Ey1ujKRSrfw4CMrAF5

CI States

Latest PR Test (Base): ✅ Run #35305340928
Latest PR Test (Extra): ✅ Run #35321306980
Latest PR Test (AMD ROCm 10): ➖ No AMD PR run found for this commit.

This was referenced Sep 14, 2026
Merged
Merged
Closed
@Kangyan-Zhou
Kangyan-Zhou added this pull request to stack #39890 September 17, 2026 03:30
ShangmingCai
ShangmingCai approved these changes Sep 17, 2026
View reviewed changes

@ShangmingCai ShangmingCai left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

Comment thread experimental/sgl-router/src/server/error.rs
let worker = reqwest::Url::parse("http://host:1/").unwrap();
// (label, error, expected status, expected x-router-error-code,
// expected x-router-upstream-status)
let cases: Vec<(&str, ApiError, StatusCode, &str, Option<&str>)> = vec![

@ShangmingCai ShangmingCai Sep 17, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: This table is a Vec, so unlike the matches nothing forces a new variant into it; the "complete contract" table goes stale on the first variant added after this lands.

@ShangmingCai

ShangmingCai commented Sep 17, 2026

Copy link
Copy Markdown
Collaborator

/tag-and-rerun-ci

@github-actions github-actions Bot added the run-ci CI: run the baseline test suite on this PR label Sep 17, 2026
@kzhou-radixark @claude
[Router] Derive error status from a failure class; preserve the worke…
95cccdb 
…r's status

The router's HTTP status for a router-originated error is now a pure function
of a new `ErrorClass`, and the precise condition travels in
`x-router-error-code`. Two defects fall out of that:

- `UpstreamTimeout` mapped to 502 while `StaleRequestExpired` mapped to 504,
  even though both are "the router gave up waiting". They now share
  `ErrorClass::Timeout` and cannot drift apart; `UpstreamTimeout` becomes 504.
- A mid-body drop discarded the worker's real status. Its code is now
  `upstream_body_incomplete` and the worker's status is echoed in a new
  `x-router-upstream-status` header, routed through an exhaustive
  `ApiError::upstream_status()` so a future variant that synthesizes a status
  over a responding worker has to decide whether it echoes one. The variant's
  `Display` and its client-facing message name an incomplete body rather than
  an error status, since the status it carries is usually a worker 200 that
  then dropped its body.

A worker's own response is still forwarded verbatim and carries neither header
— that absence is how a gateway tells engine-origin from router-origin.

The dashboard's status panel no longer reads 504 as "stale-cancel": after this
change three different conditions produce one, and `worker_requests_total`
is what tells them apart.

Tests: a table pinning the (status, error-code, upstream-status) triple for
every `ApiError` variant; the 502->504 change on both timeout paths; the
non-streaming mid-body drop echoing the worker status; and the asymmetric
streaming half, where headers already went out as 200 so the client keeps the
200 with no router headers at all.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012xa7Ey1ujKRSrfw4CMrAF5
@Kangyan-Zhou
Kangyan-Zhou force-pushed the router-4d-1-error-class branch from 80f848b to 95cccdb Compare September 18, 2026 04:00
@ShangmingCai

ShangmingCai commented Sep 18, 2026

Copy link
Copy Markdown
Collaborator

/rerun-failed-ci

@ShangmingCai
ShangmingCai marked this pull request as ready for review September 18, 2026 07:49
@ShangmingCai
ShangmingCai merged commit 6de4666 into main Sep 18, 2026
157 of 170 checks passed
@ShangmingCai
ShangmingCai deleted the router-4d-1-error-class branch September 18, 2026 08:32
ShangmingCai added a commit that referenced this pull request Sep 19, 2026
@ShangmingCai @claude
Merge main into router-4a-abort-on-disconnect
214b9b7 
#39460's three commits landed on main as #39463 / #39464 / #39465, reworked in
review, so this branch's own copies of them are superseded. The merge is
resolved to main's version of everything they touched, plus this branch's abort
commit rebuilt on top — the resulting tree is identical to rebasing 505ef50
straight onto main, so none of the pre-review code can come back.

Three collisions needed real decisions:

* `Proxy` no longer has one `client`. #39006 split it into `default_client` /
  `h2c_client`, selected per worker by `WireProtocol`. `abort_guard_for` now
  takes the worker's `protocol` and the streaming arm's abort uses
  `client_for(protocol)`, so the abort rides the same client as the forward it
  covers. `admin_client()` would have been wrong: it is the negotiating client,
  which cannot reach an h2c-only worker on a cleartext port — the abort would
  have silently failed on exactly the fleets that set `--enable-http2`.

* `RequestProbe` is no longer `#[derive(Deserialize)]`. Main hand-writes the
  visitor for the sampling array, so the `#[serde(default)] rid` field was
  inert. `rid` is now a `ProbeKey::Rid` variant, deliberately NOT a
  `RoutingKey`: the router does not route on it, so a repeated `rid` stays
  last-wins like the engine's own `json.loads` instead of becoming a 400.
  An explicit `null` still reads as absent.

* `build_outgoing_body` gained main's `sampling` parameter alongside `rid`.
  Note the interaction: #39002 added `splice_top_level`, a raw-bytes injection
  that skips the parse entirely, taken when only sampling is injected. A minted
  `rid` never overwrites a client key either — `resolve_engine_rid` returns
  `None` the moment the caller sets its own — so it has the same splice-safe
  shape, but `splice_top_level` writes `(SamplingField, Number)` members and so
  cannot carry it. Until that is generalized, minting a rid pulls every
  plain-mode request back onto the full `serde_json::Value` round-trip that
  #39002 had just removed. Flagged in the code; worth a decision before merge.

Seven main-side tests pinned "the forwarded body equals the request exactly",
which a minted `rid` breaks. They now strip the router-minted `rid` and assert
its shape separately, keeping each test's actual subject (no `input_ids` added,
`messages` preserved). `h2c_forward.rs` gained an explicit `None` abort rid.

cargo fmt --check, cargo clippy --all-targets -- -D warnings, and cargo test
--lib --tests (758 + 7 + 65 + 132 = 962 passed, 0 failed) are clean.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ShangmingCai ShangmingCai ShangmingCai approved these changes

Assignees

No one assigned

Labels

high priority run-ci CI: run the baseline test suite on this PR

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Kangyan-Zhou @ShangmingCai @kzhou-radixark