Skip to content

[Router] Treat an upstream 503/429 as backpressure, not a breaker fault (2/3) - #39464

Merged
ShangmingCai merged 1 commit into
mainfrom
router-4d-2-backpressure
Sep 18, 2026
Merged

ShangmingCai merged 1 commit into
mainfrom
router-4d-2-backpressure

Conversation

@Kangyan-Zhou

@Kangyan-Zhou Kangyan-Zhou commented Sep 14, 2026 •

Copy link
Copy Markdown
Collaborator

Motivation

2 of 3 in the router request-attribution stack. #39463 (1/3) has
merged, so this now targets main directly. #39465 (3/3) makes
worker_requests_total{outcome} agree with the judgement this PR teaches the
circuit breaker.

A saturated engine answers with its own queue-full 503s. The router counts each
one as a worker fault, and after the failure threshold the circuit breaker
opens. With a single worker behind the router that sheds every request for
the whole cool-down — and it keeps shedding after the engine has drained and
gone idle, because nothing is being sent to notice that it recovered.

The signal is backwards at exactly the wrong moment: the response that means
"I am alive and busy" is treated as evidence the worker is dead.

Modifications

A new breaker_outcome classifies the upstream status into three
dispositions, applied identically on the JSON and the streaming dispatch arm:

upstream status outcome breaker call
any non-5xx — 2xx, and a 4xx the worker answered cleanly Success record_success (streaming defers to the pump's completion hook)
503, 429 Neutral record_backpressure
other 5xx Failure record_failure

Transport errors, timeouts, mid-body drops, a malformed discovery URL, and a
stream whose body dies after a 2xx head are all recorded as failures at their
call sites, unchanged.

CircuitBreaker::record_backpressure is the new third verb, and its
behaviour is per-state rather than a blanket no-op:

  • Closed: do nothing, and specifically do not reset the failure streak
    the way record_success does. A worker interleaving genuine 5xx faults with
    503s still trips.
  • HalfOpen: close. Any response observed here proves the worker is
    answering, which is what the probe exists to find out. Leaving HalfOpen
    unresolved would wedge the breaker permanently — the probe slot is released
    only by a success or a failure, and backpressure is neither — shutting a
    recovered-but-busy worker out forever, a worse false-shed than the one this
    PR removes.
  • Open: no-op. This state is reachable, not dead code: allow gates
    admission, not completion, so a request admitted while Closed can return
    after concurrent failures have opened the breaker. A late 503 must not reset
    a breaker that has already tripped, exactly as record_failure ignores
    failures while Open.

The tradeoff is stated at breaker_outcome, not hidden: a worker stuck
returning 503 forever is no longer detected here. HTTP status alone cannot tell
"busy" from "broken-and-saying-503", and counting it as a fault produced the
worse, fleet-wide false-shed. Detecting a wedged-at-503 worker needs a
different signal than this one.

Tests

Three fail without the change, at both levels:

  • Breaker-level: probe resolution from HalfOpen, failure-streak preservation
    while Closed, and backpressure alone never opening a Closed breaker.
  • End-to-end through forward_json_to / forward_streaming_to against a live
    mock worker: an engine answering 503 far past any plausible threshold leaves
    the breaker Closed and admitting; a 500 still opens it; a half-open probe
    answered with 503 recovers the breaker.

The threshold tests loop on would_allow() rather than a hard-coded count, so
a change to the default CircuitBreakerConfig cannot silently make them
vacuous.

cargo test green on this branch — 725 lib, 7 bin, 65 component, 121 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 — N/A, no user-facing configuration surface; the dispositions and the tradeoff are documented at breaker_outcome and record_backpressure.
  • Provide accuracy and speed benchmark results — N/A, one status classification per dispatch.
  • Follow the SGLang code style guidance.

🤖 Generated with Claude Code

https://claude.ai/code/session_012xa7Ey1ujKRSrfw4CMrAF5


CI States

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

A saturated engine returning its own queue-full 503s trips the router's
circuit breaker on that backpressure. With a single worker the router then
sheds *every* request for the whole cool-down — including after the engine has
drained and gone idle.

Classify the upstream status through a new `breaker_outcome`: 503 and 429 map
to a `Neutral` outcome wired to `CircuitBreaker::record_backpressure`, which
never opens the breaker and, while Closed, leaves an in-progress failure streak
intact — but still resolves a half-open probe, so a recovered-but-busy worker
answering a probe with 503 isn't wedged shut forever. A backpressure answer
that arrives while the breaker is already Open is a no-op rather than a reset:
`allow` gates admission, not completion, so a request admitted while Closed can
land there after concurrent failures have tripped it. Genuine 5xx faults
(500/502/504/…) and transport errors, timeouts and mid-body drops still count
as failures. Applied to both the JSON and the streaming dispatch arm.

The tradeoff is stated at `breaker_outcome`: a worker stuck returning 503
forever is no longer detected here, because HTTP status alone can't tell "busy"
from "broken-and-saying-503" — and counting it caused the worse fleet-wide
false-shed.

Tests, all three failing without the fix: breaker-level (probe resolution,
streak preservation, backpressure-alone-never-opens) and end-to-end through
`forward_json_to` / `forward_streaming_to` (engine 503 stays closed, 500 still
trips, half-open 503 recovers). The threshold tests loop on `would_allow()`
rather than a hard-coded count, so a change to the default
`CircuitBreakerConfig` cannot make them vacuous.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012xa7Ey1ujKRSrfw4CMrAF5
@ShangmingCai
ShangmingCai force-pushed the router-4d-2-backpressure branch from e2769f7 to 972ea41 Compare September 18, 2026 08:32
@ShangmingCai
ShangmingCai marked this pull request as ready for review September 18, 2026 16:39
@ShangmingCai

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 18, 2026
@ShangmingCai
ShangmingCai merged commit 4e0b56c into main Sep 18, 2026
116 of 131 checks passed
@ShangmingCai
ShangmingCai deleted the router-4d-2-backpressure branch September 18, 2026 16:46
ShangmingCai added a commit that referenced this pull request Sep 19, 2026
#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

Labels

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants