[Router] Derive error status from a failure class; preserve the worker's status (1/3) (#39463)
Co-authored-by: Kangyan Zhou <kangyan.zhou@radixark.ai> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Kangyan Zhou
Claude Opus 5
parent
1fdd6c8921
commit
6de4666e43
@@ -511,7 +511,7 @@
|
||||
{
|
||||
"type": "timeseries",
|
||||
"title": "Responses by HTTP status",
|
||||
"description": "Client-visible HTTP status codes (2xx/4xx/5xx, 504=stale-cancel).",
|
||||
"description": "Client-visible HTTP status codes. A 504 is any router-side give-up \u2014 the stale-request deadline or an upstream timeout \u2014 or a worker 504 forwarded unchanged; worker_requests_total{outcome} tells them apart.",
|
||||
"datasource": {
|
||||
"type": "prometheus",
|
||||
"uid": "${datasource}"
|
||||
|
||||
@@ -170,10 +170,15 @@ impl Proxy {
|
||||
let bytes = match resp.bytes().await {
|
||||
Ok(b) => b,
|
||||
Err(e) => {
|
||||
// Walk the full source chain (`{:#}`) like the connect-error
|
||||
// handler in `classify_reqwest_error_for` — a mid-body drop's
|
||||
// real cause (incomplete message, connection reset) lives in the
|
||||
// wrapped source, not the outer reqwest error.
|
||||
let cause = anyhow::Error::new(e);
|
||||
tracing::warn!(
|
||||
upstream = %url,
|
||||
status = %status,
|
||||
error = ?e,
|
||||
error = %format_args!("{cause:#}"),
|
||||
"upstream dropped connection mid-body",
|
||||
);
|
||||
breaker.record_failure();
|
||||
|
||||
@@ -9,6 +9,54 @@ use thiserror::Error;
|
||||
|
||||
pub const X_ROUTER_ERROR_CODE: HeaderName = HeaderName::from_static("x-router-error-code");
|
||||
|
||||
/// Carries the worker's *own* HTTP status when the router had to synthesize a
|
||||
/// status of its own over a worker that did respond (today: a mid-body drop,
|
||||
/// where headers arrived but the body did not). Lets a gateway / operator
|
||||
/// recover what the engine actually said instead of seeing only the router's
|
||||
/// synthesized 502. Absent on every other response: a forwarded worker response
|
||||
/// already carries the worker's status in the status line, and a router-only
|
||||
/// condition (no workers, breaker open, ...) has no upstream status to report.
|
||||
pub const X_ROUTER_UPSTREAM_STATUS: HeaderName =
|
||||
HeaderName::from_static("x-router-upstream-status");
|
||||
|
||||
/// Coarse failure class for a router-originated error. The router's HTTP status
|
||||
/// is a pure function of the class, so two conditions that mean the same thing
|
||||
/// — e.g. a per-request timeout and a stale-deadline cancel — can never drift to
|
||||
/// different status codes. The *precise* condition travels in
|
||||
/// `x-router-error-code` (see [`ApiError::error_code`]): a gateway in front
|
||||
/// converts on that header (the authoritative signal), while the status stays a
|
||||
/// self-sufficient HTTP-honest default for a direct caller. The class never
|
||||
/// contradicts the precise code — it only generalizes it.
|
||||
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
|
||||
enum ErrorClass {
|
||||
/// 400 — request rejected at ingress as malformed / invalid.
|
||||
BadRequest,
|
||||
/// 404 — requested model / resource not found.
|
||||
NotFound,
|
||||
/// 502 — a selected worker failed to return a usable response (unreachable,
|
||||
/// or started a response then dropped the body).
|
||||
Upstream,
|
||||
/// 503 — the router had no worker to dispatch to, or declined to.
|
||||
NoTarget,
|
||||
/// 504 — the router gave up waiting (any timeout / deadline).
|
||||
Timeout,
|
||||
/// 500 — internal router fault.
|
||||
Internal,
|
||||
}
|
||||
|
||||
impl ErrorClass {
|
||||
fn status(self) -> StatusCode {
|
||||
match self {
|
||||
ErrorClass::BadRequest => StatusCode::BAD_REQUEST,
|
||||
ErrorClass::NotFound => StatusCode::NOT_FOUND,
|
||||
ErrorClass::Upstream => StatusCode::BAD_GATEWAY,
|
||||
ErrorClass::NoTarget => StatusCode::SERVICE_UNAVAILABLE,
|
||||
ErrorClass::Timeout => StatusCode::GATEWAY_TIMEOUT,
|
||||
ErrorClass::Internal => StatusCode::INTERNAL_SERVER_ERROR,
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
#[derive(Debug, Error)]
|
||||
pub enum ApiError {
|
||||
#[error("bad request: {0}")]
|
||||
@@ -52,7 +100,7 @@ pub enum ApiError {
|
||||
/// Distinct from `UpstreamUnreachable` (no reply at all) and from a
|
||||
/// well-formed non-2xx (which `Proxy` forwards verbatim with the worker's
|
||||
/// own body).
|
||||
#[error("upstream returned status {status}")]
|
||||
#[error("upstream response body incomplete after status {status}")]
|
||||
UpstreamStatus { status: StatusCode },
|
||||
|
||||
/// Wall-clock timeout exceeded while waiting for the upstream worker's
|
||||
@@ -89,10 +137,12 @@ pub enum ApiError {
|
||||
/// token wins, the handler returns this variant → HTTP 504 →
|
||||
/// client sees `stale_request_expired`.
|
||||
///
|
||||
/// Mapped to 504 (not 503) because the failure is a router-side
|
||||
/// gateway timeout from the client's perspective: the upstream
|
||||
/// worker is still potentially fine, the router gave up because
|
||||
/// the per-request budget elapsed.
|
||||
/// Classed as [`ErrorClass::Timeout`] (→ 504), the same class as
|
||||
/// `UpstreamTimeout`: from the client's perspective both are a router-side
|
||||
/// gateway timeout. Here the upstream worker is still potentially fine — the
|
||||
/// router gave up because the per-request budget elapsed. The shared class
|
||||
/// is what keeps the two timeouts on the same status; they stay tellable
|
||||
/// apart only by `x-router-error-code`.
|
||||
#[error("stale request expired for model {model}")]
|
||||
StaleRequestExpired { model: String },
|
||||
|
||||
@@ -126,48 +176,90 @@ pub enum ApiError {
|
||||
}
|
||||
|
||||
impl ApiError {
|
||||
fn status_and_code(&self) -> (StatusCode, &'static str) {
|
||||
/// The failure class — the sole determinant of the HTTP status. Grouping by
|
||||
/// class is what makes the status non-divergent: every timeout is
|
||||
/// [`ErrorClass::Timeout`], so a per-request timeout and a stale-deadline
|
||||
/// cancel are guaranteed the same status, and no future variant can quietly
|
||||
/// pick a different one.
|
||||
fn class(&self) -> ErrorClass {
|
||||
match self {
|
||||
ApiError::BadRequest(_) => (StatusCode::BAD_REQUEST, "bad_request"),
|
||||
ApiError::SamplingContract { .. } => {
|
||||
(StatusCode::BAD_REQUEST, "sampling_contract_violation")
|
||||
}
|
||||
ApiError::ModelNotFound(_) => (StatusCode::NOT_FOUND, "model_not_found"),
|
||||
ApiError::UpstreamUnreachable { .. } => {
|
||||
(StatusCode::BAD_GATEWAY, "upstream_unreachable")
|
||||
}
|
||||
ApiError::UpstreamStatus { .. } => (StatusCode::BAD_GATEWAY, "upstream_status"),
|
||||
ApiError::UpstreamTimeout { .. } => (StatusCode::BAD_GATEWAY, "upstream_timeout"),
|
||||
ApiError::NoHealthyWorkers { .. } => {
|
||||
(StatusCode::SERVICE_UNAVAILABLE, "no_healthy_workers")
|
||||
}
|
||||
ApiError::NoPrefillWorkersAvailable { .. } => (
|
||||
StatusCode::SERVICE_UNAVAILABLE,
|
||||
"no_prefill_workers_available",
|
||||
),
|
||||
ApiError::NoDecodeWorkersAvailable { .. } => (
|
||||
StatusCode::SERVICE_UNAVAILABLE,
|
||||
"no_decode_workers_available",
|
||||
),
|
||||
ApiError::StaleRequestExpired { .. } => {
|
||||
(StatusCode::GATEWAY_TIMEOUT, "stale_request_expired")
|
||||
}
|
||||
ApiError::PolicySelectionFailed { .. } => {
|
||||
(StatusCode::SERVICE_UNAVAILABLE, "policy_selection_failed")
|
||||
}
|
||||
ApiError::BreakerOpen { .. } => (StatusCode::SERVICE_UNAVAILABLE, "breaker_open"),
|
||||
ApiError::WorkerMisconfigured { .. } => {
|
||||
(StatusCode::SERVICE_UNAVAILABLE, "worker_misconfigured")
|
||||
}
|
||||
ApiError::Internal(_) => (StatusCode::INTERNAL_SERVER_ERROR, "internal_error"),
|
||||
ApiError::BadRequest(_) => ErrorClass::BadRequest,
|
||||
ApiError::SamplingContract { .. } => ErrorClass::BadRequest,
|
||||
ApiError::ModelNotFound(_) => ErrorClass::NotFound,
|
||||
ApiError::UpstreamUnreachable { .. } => ErrorClass::Upstream,
|
||||
ApiError::UpstreamStatus { .. } => ErrorClass::Upstream,
|
||||
ApiError::UpstreamTimeout { .. } => ErrorClass::Timeout,
|
||||
ApiError::NoHealthyWorkers { .. } => ErrorClass::NoTarget,
|
||||
ApiError::NoPrefillWorkersAvailable { .. } => ErrorClass::NoTarget,
|
||||
ApiError::NoDecodeWorkersAvailable { .. } => ErrorClass::NoTarget,
|
||||
ApiError::StaleRequestExpired { .. } => ErrorClass::Timeout,
|
||||
ApiError::PolicySelectionFailed { .. } => ErrorClass::NoTarget,
|
||||
ApiError::BreakerOpen { .. } => ErrorClass::NoTarget,
|
||||
ApiError::WorkerMisconfigured { .. } => ErrorClass::NoTarget,
|
||||
ApiError::Internal(_) => ErrorClass::Internal,
|
||||
}
|
||||
}
|
||||
|
||||
/// The HTTP status this error maps to — same value the client receives via
|
||||
/// `into_response`. Exposed so the access log records the real status
|
||||
/// (e.g. 502/503/504) instead of a sentinel.
|
||||
/// Stable, machine-readable `x-router-error-code` — the authoritative signal
|
||||
/// a gateway converts on. Distinct per condition even when two conditions
|
||||
/// share a class (and thus a status): `upstream_timeout` and
|
||||
/// `stale_request_expired` both map to 504 but stay tellable apart here.
|
||||
fn error_code(&self) -> &'static str {
|
||||
match self {
|
||||
ApiError::BadRequest(_) => "bad_request",
|
||||
// Shares `ErrorClass::BadRequest` (and thus the 400) with
|
||||
// `BadRequest`, but keeps its own code: an operator rolling a
|
||||
// sampling contract across a fleet has to be able to alert on
|
||||
// contract rejections separately from malformed client requests.
|
||||
ApiError::SamplingContract { .. } => "sampling_contract_violation",
|
||||
ApiError::ModelNotFound(_) => "model_not_found",
|
||||
ApiError::UpstreamUnreachable { .. } => "upstream_unreachable",
|
||||
// Mid-body drop: headers (incl. a status) arrived, then the body
|
||||
// didn't. We surface a 502 but echo the worker's status in
|
||||
// `x-router-upstream-status` (see `into_response`).
|
||||
ApiError::UpstreamStatus { .. } => "upstream_body_incomplete",
|
||||
ApiError::UpstreamTimeout { .. } => "upstream_timeout",
|
||||
ApiError::NoHealthyWorkers { .. } => "no_healthy_workers",
|
||||
ApiError::NoPrefillWorkersAvailable { .. } => "no_prefill_workers_available",
|
||||
ApiError::NoDecodeWorkersAvailable { .. } => "no_decode_workers_available",
|
||||
ApiError::StaleRequestExpired { .. } => "stale_request_expired",
|
||||
ApiError::PolicySelectionFailed { .. } => "policy_selection_failed",
|
||||
ApiError::BreakerOpen { .. } => "breaker_open",
|
||||
ApiError::WorkerMisconfigured { .. } => "worker_misconfigured",
|
||||
ApiError::Internal(_) => "internal_error",
|
||||
}
|
||||
}
|
||||
|
||||
/// The worker's *own* status to echo in `x-router-upstream-status`, for the
|
||||
/// case where the router synthesized its own status over a worker that did
|
||||
/// respond. Today only the mid-body-drop (`UpstreamStatus`) carries one. This
|
||||
/// is an exhaustive, wildcard-free match (not an `if let` at the call site) so
|
||||
/// a future "synthesized over a responding worker" variant is forced to decide
|
||||
/// whether it echoes a status, rather than silently inheriting `None`.
|
||||
fn upstream_status(&self) -> Option<StatusCode> {
|
||||
match self {
|
||||
ApiError::UpstreamStatus { status } => Some(*status),
|
||||
ApiError::BadRequest(_)
|
||||
| ApiError::SamplingContract { .. }
|
||||
| ApiError::ModelNotFound(_)
|
||||
| ApiError::UpstreamUnreachable { .. }
|
||||
| ApiError::UpstreamTimeout { .. }
|
||||
| ApiError::NoHealthyWorkers { .. }
|
||||
| ApiError::NoPrefillWorkersAvailable { .. }
|
||||
| ApiError::NoDecodeWorkersAvailable { .. }
|
||||
| ApiError::StaleRequestExpired { .. }
|
||||
| ApiError::PolicySelectionFailed { .. }
|
||||
| ApiError::BreakerOpen { .. }
|
||||
| ApiError::WorkerMisconfigured { .. }
|
||||
| ApiError::Internal(_) => None,
|
||||
}
|
||||
}
|
||||
|
||||
/// The HTTP status this error maps to — the same value the client receives
|
||||
/// via `into_response`. Exposed so a caller holding the error, rather than
|
||||
/// the response, can label it with the status the client actually saw.
|
||||
pub fn status_code(&self) -> StatusCode {
|
||||
self.status_and_code().0
|
||||
self.class().status()
|
||||
}
|
||||
}
|
||||
|
||||
@@ -186,7 +278,8 @@ struct ErrorBody<'a> {
|
||||
|
||||
impl IntoResponse for ApiError {
|
||||
fn into_response(self) -> Response {
|
||||
let (status, code) = self.status_and_code();
|
||||
let status = self.class().status();
|
||||
let code = self.error_code();
|
||||
let typ = match status.as_u16() {
|
||||
400..=499 => "invalid_request_error",
|
||||
_ => "server_error",
|
||||
@@ -209,11 +302,14 @@ impl IntoResponse for ApiError {
|
||||
"upstream unavailable".to_string()
|
||||
}
|
||||
ApiError::UpstreamStatus { status } => {
|
||||
// The worker's status is usually 200 here — it answered, then
|
||||
// dropped the body — so neither message may call it an error
|
||||
// status.
|
||||
tracing::warn!(
|
||||
upstream_status = %status,
|
||||
"upstream returned an error status",
|
||||
"upstream response body did not complete",
|
||||
);
|
||||
"upstream returned an error status".to_string()
|
||||
"upstream response was incomplete".to_string()
|
||||
}
|
||||
ApiError::UpstreamTimeout { worker } => {
|
||||
tracing::warn!(upstream = %worker, "upstream request timed out");
|
||||
@@ -273,6 +369,15 @@ impl IntoResponse for ApiError {
|
||||
.into_response();
|
||||
resp.headers_mut()
|
||||
.insert(X_ROUTER_ERROR_CODE, HeaderValue::from_static(code));
|
||||
// Preserve the worker's real status when we synthesized our own (today,
|
||||
// only the mid-body-drop case: the worker sent a status, then dropped the
|
||||
// body, so we report a 502 but don't throw away what it said).
|
||||
if let Some(upstream) = self.upstream_status() {
|
||||
resp.headers_mut().insert(
|
||||
X_ROUTER_UPSTREAM_STATUS,
|
||||
HeaderValue::from(upstream.as_u16()),
|
||||
);
|
||||
}
|
||||
resp
|
||||
}
|
||||
}
|
||||
@@ -348,7 +453,10 @@ mod tests {
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn upstream_status_envelope_has_code() {
|
||||
fn upstream_body_incomplete_preserves_worker_status_in_header() {
|
||||
// Mid-body drop: the worker sent a status (here 500) then dropped the
|
||||
// body. The router synthesizes its own 502, but the worker's real status
|
||||
// is preserved in `x-router-upstream-status` rather than silently lost.
|
||||
let err = ApiError::UpstreamStatus {
|
||||
status: StatusCode::INTERNAL_SERVER_ERROR,
|
||||
};
|
||||
@@ -358,10 +466,20 @@ mod tests {
|
||||
resp.headers()
|
||||
.get("x-router-error-code")
|
||||
.and_then(|v| v.to_str().ok()),
|
||||
Some("upstream_status"),
|
||||
Some("upstream_body_incomplete"),
|
||||
);
|
||||
assert_eq!(
|
||||
resp.headers()
|
||||
.get("x-router-upstream-status")
|
||||
.and_then(|v| v.to_str().ok()),
|
||||
Some("500"),
|
||||
"the worker's real status must be preserved, not discarded",
|
||||
);
|
||||
let body = collect_body(resp);
|
||||
assert!(body.contains("\"code\":\"upstream_status\""), "{body}");
|
||||
assert!(
|
||||
body.contains("\"code\":\"upstream_body_incomplete\""),
|
||||
"{body}"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
@@ -372,7 +490,9 @@ mod tests {
|
||||
worker: worker.clone(),
|
||||
};
|
||||
let resp = err.into_response();
|
||||
assert_eq!(resp.status(), StatusCode::BAD_GATEWAY);
|
||||
// A timeout is a gateway timeout (504), not a bad gateway (502): the
|
||||
// same class — and so the same status — as the stale-deadline cancel.
|
||||
assert_eq!(resp.status(), StatusCode::GATEWAY_TIMEOUT);
|
||||
assert_eq!(
|
||||
resp.headers()
|
||||
.get("x-router-error-code")
|
||||
@@ -485,4 +605,165 @@ mod tests {
|
||||
env.error.message
|
||||
);
|
||||
}
|
||||
|
||||
/// The three client-facing signals of the router status-code contract:
|
||||
/// the HTTP status, `x-router-error-code`, and `x-router-upstream-status`.
|
||||
fn signals(err: ApiError) -> (StatusCode, Option<String>, Option<String>) {
|
||||
let resp = err.into_response();
|
||||
let status = resp.status();
|
||||
let header = |name: &str| {
|
||||
resp.headers()
|
||||
.get(name)
|
||||
.and_then(|v| v.to_str().ok())
|
||||
.map(str::to_owned)
|
||||
};
|
||||
(
|
||||
status,
|
||||
header("x-router-error-code"),
|
||||
header("x-router-upstream-status"),
|
||||
)
|
||||
}
|
||||
|
||||
/// Every router-*originated* condition maps to a fixed (status, error-code)
|
||||
/// pair, and ONLY the mid-body-drop case echoes the worker's real status in
|
||||
/// `x-router-upstream-status`. This pins the router half of the status-code
|
||||
/// contract:
|
||||
/// * same condition class → same status — both timeouts are 504, so the
|
||||
/// per-request timeout and the stale-deadline cancel cannot diverge;
|
||||
/// * a worker's real status is preserved, never silently rewritten.
|
||||
#[test]
|
||||
fn router_originated_scenarios_match_status_and_headers() {
|
||||
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![
|
||||
(
|
||||
"mid-body drop preserves worker status",
|
||||
ApiError::UpstreamStatus {
|
||||
status: StatusCode::OK,
|
||||
},
|
||||
StatusCode::BAD_GATEWAY,
|
||||
"upstream_body_incomplete",
|
||||
Some("200"),
|
||||
),
|
||||
(
|
||||
"unreachable",
|
||||
ApiError::UpstreamUnreachable {
|
||||
worker: worker.clone(),
|
||||
source: anyhow::anyhow!("connect refused"),
|
||||
},
|
||||
StatusCode::BAD_GATEWAY,
|
||||
"upstream_unreachable",
|
||||
None,
|
||||
),
|
||||
(
|
||||
"request timeout",
|
||||
ApiError::UpstreamTimeout {
|
||||
worker: worker.clone(),
|
||||
},
|
||||
StatusCode::GATEWAY_TIMEOUT,
|
||||
"upstream_timeout",
|
||||
None,
|
||||
),
|
||||
(
|
||||
"stale-deadline cancel",
|
||||
ApiError::StaleRequestExpired { model: "m".into() },
|
||||
StatusCode::GATEWAY_TIMEOUT,
|
||||
"stale_request_expired",
|
||||
None,
|
||||
),
|
||||
(
|
||||
"no healthy workers",
|
||||
ApiError::NoHealthyWorkers { model: "m".into() },
|
||||
StatusCode::SERVICE_UNAVAILABLE,
|
||||
"no_healthy_workers",
|
||||
None,
|
||||
),
|
||||
(
|
||||
"bad request",
|
||||
ApiError::BadRequest("bad".into()),
|
||||
StatusCode::BAD_REQUEST,
|
||||
"bad_request",
|
||||
None,
|
||||
),
|
||||
(
|
||||
"sampling contract rejection",
|
||||
ApiError::SamplingContract {
|
||||
param: "temperature",
|
||||
detail: "got 0.5, expected 1".into(),
|
||||
},
|
||||
StatusCode::BAD_REQUEST,
|
||||
"sampling_contract_violation",
|
||||
None,
|
||||
),
|
||||
(
|
||||
"model not found",
|
||||
ApiError::ModelNotFound("ghost".into()),
|
||||
StatusCode::NOT_FOUND,
|
||||
"model_not_found",
|
||||
None,
|
||||
),
|
||||
(
|
||||
"no prefill workers",
|
||||
ApiError::NoPrefillWorkersAvailable { model: "m".into() },
|
||||
StatusCode::SERVICE_UNAVAILABLE,
|
||||
"no_prefill_workers_available",
|
||||
None,
|
||||
),
|
||||
(
|
||||
"no decode workers",
|
||||
ApiError::NoDecodeWorkersAvailable { model: "m".into() },
|
||||
StatusCode::SERVICE_UNAVAILABLE,
|
||||
"no_decode_workers_available",
|
||||
None,
|
||||
),
|
||||
(
|
||||
"policy selection failed",
|
||||
ApiError::PolicySelectionFailed { model: "m".into() },
|
||||
StatusCode::SERVICE_UNAVAILABLE,
|
||||
"policy_selection_failed",
|
||||
None,
|
||||
),
|
||||
(
|
||||
"breaker open",
|
||||
ApiError::BreakerOpen {
|
||||
worker: "http://w:1".into(),
|
||||
},
|
||||
StatusCode::SERVICE_UNAVAILABLE,
|
||||
"breaker_open",
|
||||
None,
|
||||
),
|
||||
(
|
||||
"worker misconfigured",
|
||||
ApiError::WorkerMisconfigured {
|
||||
worker: "http://w:1".into(),
|
||||
source: anyhow::anyhow!("unparsable url"),
|
||||
},
|
||||
StatusCode::SERVICE_UNAVAILABLE,
|
||||
"worker_misconfigured",
|
||||
None,
|
||||
),
|
||||
(
|
||||
"internal",
|
||||
ApiError::Internal(anyhow::anyhow!("boom")),
|
||||
StatusCode::INTERNAL_SERVER_ERROR,
|
||||
"internal_error",
|
||||
None,
|
||||
),
|
||||
];
|
||||
for (label, err, want_status, want_code, want_upstream) in cases {
|
||||
let (status, code, upstream) = signals(err);
|
||||
assert_eq!(status, want_status, "status mismatch for `{label}`");
|
||||
assert_eq!(
|
||||
code.as_deref(),
|
||||
Some(want_code),
|
||||
"x-router-error-code mismatch for `{label}`",
|
||||
);
|
||||
assert_eq!(
|
||||
upstream.as_deref(),
|
||||
want_upstream,
|
||||
"x-router-upstream-status mismatch for `{label}`",
|
||||
);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -467,11 +467,17 @@ async fn non_streaming_upstream_429_preserved() {
|
||||
res.headers().get("content-type").unwrap().to_str().unwrap(),
|
||||
"application/json",
|
||||
);
|
||||
// Router envelope code header must NOT be set — this is upstream's response.
|
||||
// Router envelope headers must NOT be set — this is upstream's response.
|
||||
// Their absence is exactly how a gateway tells "the engine said this" from
|
||||
// "the router said this".
|
||||
assert!(
|
||||
res.headers().get("x-router-error-code").is_none(),
|
||||
"router envelope header must NOT be set on upstream-passthrough responses",
|
||||
);
|
||||
assert!(
|
||||
res.headers().get("x-router-upstream-status").is_none(),
|
||||
"no status was synthesized over the worker, so no x-router-upstream-status",
|
||||
);
|
||||
let bytes = res.into_body().collect().await.unwrap().to_bytes();
|
||||
let got: serde_json::Value = serde_json::from_slice(&bytes).unwrap();
|
||||
assert_eq!(got, upstream_body, "body bytes must round-trip unchanged");
|
||||
@@ -511,6 +517,11 @@ async fn non_streaming_upstream_500_preserved() {
|
||||
res.headers().get("x-router-error-code").is_none(),
|
||||
"router envelope must NOT wrap upstream 5xx — passthrough",
|
||||
);
|
||||
assert!(
|
||||
res.headers().get("x-router-upstream-status").is_none(),
|
||||
"a complete worker 500 is forwarded verbatim — distinct from a \
|
||||
synthesized 502 mid-body drop, which DOES echo the worker status",
|
||||
);
|
||||
let bytes = res.into_body().collect().await.unwrap().to_bytes();
|
||||
let got: serde_json::Value = serde_json::from_slice(&bytes).unwrap();
|
||||
assert_eq!(got, upstream_body);
|
||||
@@ -670,12 +681,14 @@ async fn chat_rejects_string_body_400() {
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn non_streaming_mid_body_drop_classified_as_upstream_status() {
|
||||
async fn non_streaming_mid_body_drop_classified_as_upstream_body_incomplete() {
|
||||
// Regression: when the upstream replies with a status line and headers
|
||||
// but drops the connection mid-body, the failure is NOT
|
||||
// "upstream_unreachable" (the upstream demonstrably DID reply). It must
|
||||
// be classified as `upstream_status` so the operator-visible envelope
|
||||
// reflects that the worker partially served the request.
|
||||
// be classified as `upstream_body_incomplete` so the operator-visible
|
||||
// envelope reflects that the worker partially served the request — and the
|
||||
// worker's own status must survive in `x-router-upstream-status` rather
|
||||
// than being replaced by the router's synthesized 502.
|
||||
let worker = crate::common::mock_worker::MockWorker::start_returning_partial_body(
|
||||
StatusCode::OK,
|
||||
b"{\"partial\": ",
|
||||
@@ -705,8 +718,13 @@ async fn non_streaming_mid_body_drop_classified_as_upstream_status() {
|
||||
);
|
||||
assert_eq!(
|
||||
res.headers().get("x-router-error-code").unwrap(),
|
||||
"upstream_status",
|
||||
"mid-body drop must be upstream_status (worker DID reply), not upstream_unreachable",
|
||||
"upstream_body_incomplete",
|
||||
"mid-body drop must be upstream_body_incomplete (worker DID reply), not upstream_unreachable",
|
||||
);
|
||||
assert_eq!(
|
||||
res.headers().get("x-router-upstream-status").unwrap(),
|
||||
"200",
|
||||
"the worker's own status must be echoed, not discarded behind the 502",
|
||||
);
|
||||
}
|
||||
|
||||
@@ -976,6 +994,61 @@ async fn forward_streaming_to_records_failure_on_mid_stream_drop() {
|
||||
);
|
||||
}
|
||||
|
||||
/// Client-visible contract of a streaming mid-body drop, through `build_router`.
|
||||
/// This is the asymmetric half of the non-streaming case: headers were already
|
||||
/// sent as 200, so the client keeps a 200 (NOT the synthesized 502 of the
|
||||
/// non-streaming path), there is NO `x-router-error-code` /
|
||||
/// `x-router-upstream-status`, and `responses_total` counts it as a 200 (the
|
||||
/// breaker / duration metrics capture the mid-stream failure — see the breaker
|
||||
/// test above).
|
||||
#[tokio::test]
|
||||
async fn streaming_mid_body_drop_stays_200_with_no_router_headers() {
|
||||
let worker = crate::common::mock_worker::MockWorker::start_returning_partial_body(
|
||||
StatusCode::OK,
|
||||
b"data: hi\n\n",
|
||||
)
|
||||
.await;
|
||||
let ctx = build_ctx_with_worker(&worker.url);
|
||||
let app = build_router(ctx.clone());
|
||||
|
||||
let req = Request::builder()
|
||||
.method("POST")
|
||||
.uri("/v1/chat/completions")
|
||||
.header("content-type", "application/json")
|
||||
.body(Body::from(
|
||||
serde_json::to_vec(&serde_json::json!({
|
||||
"model": "tiny",
|
||||
"messages": [{"role": "user", "content": "hi"}],
|
||||
"stream": true
|
||||
}))
|
||||
.unwrap(),
|
||||
))
|
||||
.unwrap();
|
||||
let res = app.oneshot(req).await.unwrap();
|
||||
assert_eq!(
|
||||
res.status(),
|
||||
StatusCode::OK,
|
||||
"streaming mid-drop: headers were already sent as 200, so the client keeps 200",
|
||||
);
|
||||
assert!(
|
||||
res.headers().get("x-router-error-code").is_none(),
|
||||
"a 200-then-drop stream is not router-originated — no x-router-error-code",
|
||||
);
|
||||
assert!(
|
||||
res.headers().get("x-router-upstream-status").is_none(),
|
||||
"no status was synthesized over the worker, so no x-router-upstream-status",
|
||||
);
|
||||
let _ = res.into_body().collect().await;
|
||||
|
||||
let m = ctx.metrics.render();
|
||||
assert!(
|
||||
m.contains(
|
||||
r#"sgl_router_responses_total{route="/v1/chat/completions",method="POST",status_code="200"} 1"#
|
||||
),
|
||||
"a streaming mid-drop counts as a 200 at the edge: {m}",
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn forward_json_to_records_failure_on_5xx() {
|
||||
use sgl_router::health::circuit_breaker::{CircuitBreaker, CircuitBreakerConfig};
|
||||
|
||||
@@ -7,7 +7,8 @@
|
||||
//! Without a configured `.timeout(...)` on the reqwest client, a stalled
|
||||
//! backend hangs the axum handler future forever and the test harness
|
||||
//! would just timeout. We assert here that the router returns a fast,
|
||||
//! clean 502 (`upstream_timeout`) instead.
|
||||
//! clean 504 (`upstream_timeout`) instead — a timeout is a gateway timeout,
|
||||
//! the same status class as the stale-deadline cancel.
|
||||
|
||||
use axum::body::Body;
|
||||
use axum::http::{Request, StatusCode};
|
||||
@@ -103,7 +104,7 @@ async fn non_streaming_request_times_out_when_worker_hangs() {
|
||||
elapsed < Duration::from_secs(1),
|
||||
"router must short-circuit on upstream timeout; elapsed {elapsed:?}"
|
||||
);
|
||||
assert_eq!(res.status(), StatusCode::BAD_GATEWAY);
|
||||
assert_eq!(res.status(), StatusCode::GATEWAY_TIMEOUT);
|
||||
assert_eq!(
|
||||
res.headers().get("x-router-error-code").unwrap(),
|
||||
"upstream_timeout"
|
||||
|
||||
Reference in New Issue
Block a user