diff --git a/config.example.yaml b/config.example.yaml index c9ca0b26..f42adf2b 100644 --- a/config.example.yaml +++ b/config.example.yaml @@ -153,6 +153,37 @@ ratelimit: # hung upstream) is reclaimed. Redis backend only. # concurrency_ttl_secs: 300 +# Connection-layer settings for outbound calls to LLM providers. These +# describe the network path to the upstream, so they are deployment +# config rather than per-model or per-provider-key fields. +# +# The values below are the defaults; uncomment to override. Every +# duration accepts 0 to switch that knob off. +upstream: + # Max time for DNS + TCP + TLS before an attempt fails. Without it a + # black-holed upstream is bounded only by the model's own timeout. + # connect_timeout_ms: 5000 + + # TCP keepalive. Idle time before the first probe, the interval + # between probes, and how many unacknowledged probes end the + # connection. Keeps a NAT / LB idle timer from reaping a connection + # while a slow model is still producing its first token. + # tcp_keepalive_secs: 60 + # tcp_keepalive_interval_secs: 30 + # tcp_keepalive_retries: 5 + + # How long an idle connection may sit in the pool before being + # discarded. KEEP THIS BELOW the shortest idle timeout on the path to + # the provider (load balancer, NAT gateway, corporate proxy, service + # mesh). If it is longer, the pool eventually hands out a connection + # the far end has already closed and the request fails with a + # transport error. Lower it if you see intermittent transport errors + # against an otherwise healthy provider. + pool_idle_timeout_secs: 30 + + # Cap on idle connections kept per upstream host. Unset = unbounded. + # pool_max_idle_per_host: 32 + # Models, API keys, provider keys, guardrails, cache policies, and # observability exporters are NOT defined in this file. They are stored # in etcd and managed via the Admin API (see docs/api-admin.md). This diff --git a/config.managed.yaml b/config.managed.yaml index 0652cf5d..68c6d423 100644 --- a/config.managed.yaml +++ b/config.managed.yaml @@ -99,3 +99,17 @@ managed: cache: backend: "memory" + +# Connection-layer settings for outbound calls to LLM providers. Shown +# with their defaults; uncomment to override. `pool_idle_timeout_secs` +# is the one to lower when a load balancer, NAT gateway, or service mesh +# between this DP and the provider closes idle connections sooner than +# the gateway expires them — the symptom is intermittent transport +# errors against an otherwise healthy upstream. +upstream: + # connect_timeout_ms: 5000 + # tcp_keepalive_secs: 60 + # tcp_keepalive_interval_secs: 30 + # tcp_keepalive_retries: 5 + pool_idle_timeout_secs: 30 + # pool_max_idle_per_host: 32 diff --git a/crates/aisix-core/src/config.rs b/crates/aisix-core/src/config.rs index a9989980..3948cf3d 100644 --- a/crates/aisix-core/src/config.rs +++ b/crates/aisix-core/src/config.rs @@ -61,6 +61,12 @@ pub struct Config { /// one global window instead of one-per-replica (api7/AISIX-Cloud#798). #[serde(default)] pub ratelimit: RateLimitConfig, + /// Connection-layer tuning for outbound calls to LLM providers. + /// Defaults bound the connect phase, keep TCP keepalive on, and expire + /// pooled connections well before a typical LB/NAT/proxy hop would — + /// see [`UpstreamConfig`]. + #[serde(default)] + pub upstream: UpstreamConfig, /// Optional managed-mode configuration. When `managed.enabled = true` /// the admin API and Playground endpoints are **not** bound — the DP /// is a pure etcd reader driven by the aisix.cloud control plane. @@ -755,6 +761,56 @@ pub enum RateLimitBackend { Redis, } +/// Connection-layer settings for the HTTP clients that call LLM providers. +/// +/// These are deployment properties of the network path to the upstream, not +/// per-tenant configuration, so they live in the DP config file rather than +/// on a Model or ProviderKey resource. +/// +/// The defaults exist because reqwest's own are wrong for a gateway sitting +/// behind an LB/NAT/proxy hop: no connect timeout, TCP keepalive off, and a +/// 90s pooled-connection lifetime that outlives the idle timeout of a +/// typical hop — so a connection reaped upstream can still be handed out +/// here, and the request fails with an opaque transport error. +/// +/// Every duration accepts `0` to disable that individual knob. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] +#[serde(deny_unknown_fields, default)] +pub struct UpstreamConfig { + /// Max time for DNS + TCP + TLS before an attempt fails. Without it a + /// black-holed upstream is bounded only by the model's overall timeout. + pub connect_timeout_ms: u64, + /// Idle seconds before the kernel sends its first TCP keepalive probe. + /// Keeps a long wait for a slow first token from being reaped by a NAT + /// or LB idle timer. + pub tcp_keepalive_secs: u64, + /// Seconds between subsequent keepalive probes. + pub tcp_keepalive_interval_secs: u64, + /// Unacknowledged probes before the kernel drops the connection. + pub tcp_keepalive_retries: u32, + /// How long an idle connection may sit in the pool before it is + /// discarded. **Keep this below the shortest idle timeout on the path + /// to the provider** (LB, NAT gateway, corporate proxy, service mesh), + /// or the pool will hand out connections the far end already closed. + pub pool_idle_timeout_secs: u64, + /// Cap on idle connections kept per upstream host. `null` (the + /// default) leaves reqwest's unbounded behaviour. + pub pool_max_idle_per_host: Option, +} + +impl Default for UpstreamConfig { + fn default() -> Self { + Self { + connect_timeout_ms: 5_000, + tcp_keepalive_secs: 60, + tcp_keepalive_interval_secs: 30, + tcp_keepalive_retries: 5, + pool_idle_timeout_secs: 30, + pool_max_idle_per_host: None, + } + } +} + impl Config { /// Load + merge + validate. /// @@ -1234,6 +1290,61 @@ admin: assert_eq!(cfg.ratelimit.concurrency_ttl_secs, 300); } + /// An `upstream:` block is optional; the defaults must still bound the + /// connect phase, keep TCP keepalive on, and expire pooled connections + /// sooner than reqwest's own 90s (AISIX-Cloud#1122). + #[test] + fn upstream_defaults_apply_when_the_block_is_absent() { + let f = write_yaml( + r#" +etcd: + endpoints: ["http://localhost:2379"] +proxy: + addr: "0.0.0.0:3000" +admin: + addr: "127.0.0.1:3001" + admin_keys: ["k1"] +"#, + ); + let cfg = Config::load_from_path(Some(f.path())).unwrap(); + assert_eq!(cfg.upstream.connect_timeout_ms, 5_000); + assert_eq!(cfg.upstream.tcp_keepalive_secs, 60); + assert_eq!(cfg.upstream.tcp_keepalive_interval_secs, 30); + assert_eq!(cfg.upstream.tcp_keepalive_retries, 5); + assert!(cfg.upstream.pool_idle_timeout_secs < 90); + assert!(cfg.upstream.pool_max_idle_per_host.is_none()); + } + + /// Operators behind a proxy with a short idle timeout need to lower + /// `pool_idle_timeout_secs`; every knob must be individually settable + /// and `0` must round-trip (it means "leave this one off"). + #[test] + fn upstream_block_overrides_individual_knobs() { + let f = write_yaml( + r#" +etcd: + endpoints: ["http://localhost:2379"] +proxy: + addr: "0.0.0.0:3000" +admin: + addr: "127.0.0.1:3001" + admin_keys: ["k1"] +upstream: + connect_timeout_ms: 2000 + pool_idle_timeout_secs: 10 + tcp_keepalive_secs: 0 + pool_max_idle_per_host: 16 +"#, + ); + let cfg = Config::load_from_path(Some(f.path())).unwrap(); + assert_eq!(cfg.upstream.connect_timeout_ms, 2_000); + assert_eq!(cfg.upstream.pool_idle_timeout_secs, 10); + assert_eq!(cfg.upstream.tcp_keepalive_secs, 0); + assert_eq!(cfg.upstream.pool_max_idle_per_host, Some(16)); + // Unspecified knobs keep their defaults. + assert_eq!(cfg.upstream.tcp_keepalive_interval_secs, 30); + } + #[test] fn ratelimit_redis_backend_requires_redis_block() { let f = write_yaml( diff --git a/crates/aisix-gateway/src/bridge.rs b/crates/aisix-gateway/src/bridge.rs index 1487fedb..066e7dab 100644 --- a/crates/aisix-gateway/src/bridge.rs +++ b/crates/aisix-gateway/src/bridge.rs @@ -121,13 +121,32 @@ impl BridgeContext { } } +/// `": {cause}"` when a transport-layer cause is known, otherwise empty — +/// keeps the timeout message unchanged for the gateway's own deadlines. +fn timeout_cause_suffix(cause: &str) -> String { + if cause.is_empty() { + String::new() + } else { + format!(": {cause}") + } +} + /// Error surfaced by any Bridge. Each variant maps to a stable /// client-visible HTTP status and OpenAI-style error code so the proxy /// layer can translate without further inspection. #[derive(Debug, thiserror::Error)] pub enum BridgeError { - #[error("upstream request timed out after {elapsed_ms}ms")] - Timeout { elapsed_ms: u64 }, + /// An upstream call exceeded a time budget. + /// + /// `cause` names the transport-layer reason when reqwest reported one, + /// and is empty when one of the gateway's own deadlines elapsed. Three + /// unrelated conditions all satisfy `reqwest::Error::is_timeout()` — a + /// `connect_timeout`, hyper's request timeout, and the kernel's own + /// `ETIMEDOUT` on an unanswered SYN — so without the cause they render + /// as the same sentence and an operator cannot tell "the upstream is + /// slow" from "we never reached it" (AISIX-Cloud#1093). + #[error("upstream request timed out after {elapsed_ms}ms{}", timeout_cause_suffix(.cause))] + Timeout { elapsed_ms: u64, cause: String }, /// Upstream returned a non-2xx HTTP status. `retry_after` carries /// the upstream's `Retry-After` header parsed to a Duration when /// present — used by the cooldown layer to honor provider-supplied @@ -485,7 +504,43 @@ mod tests { #[test] fn timeout_maps_to_504() { - let e = BridgeError::Timeout { elapsed_ms: 30_000 }; + let e = BridgeError::Timeout { + cause: String::new(), + elapsed_ms: 30_000, + }; + assert_eq!(e.http_status(), 504); + assert_eq!(e.error_type(), "timeout"); + } + + /// A gateway-owned deadline has no transport cause, and its message + /// must stay byte-identical to what it was before `cause` existed — + /// operators and log queries key on this sentence. + #[test] + fn timeout_without_cause_renders_unchanged() { + let e = BridgeError::Timeout { + elapsed_ms: 30_000, + cause: String::new(), + }; + assert_eq!(e.to_string(), "upstream request timed out after 30000ms"); + } + + /// With a transport cause the message names it, so an expired + /// `connect_timeout` and an expired request budget stop looking + /// identical (AISIX-Cloud#1093). + #[test] + fn timeout_with_cause_appends_it() { + let e = BridgeError::Timeout { + elapsed_ms: 5_001, + cause: "error sending request: client error (Connect): tcp connect error: \ + Connection timed out (os error 110)" + .to_string(), + }; + assert_eq!( + e.to_string(), + "upstream request timed out after 5001ms: error sending request: \ + client error (Connect): tcp connect error: Connection timed out (os error 110)" + ); + // Status and telemetry class are unaffected by the added detail. assert_eq!(e.http_status(), 504); assert_eq!(e.error_type(), "timeout"); } diff --git a/crates/aisix-gateway/src/lib.rs b/crates/aisix-gateway/src/lib.rs index 3cba96f2..37ec09b5 100644 --- a/crates/aisix-gateway/src/lib.rs +++ b/crates/aisix-gateway/src/lib.rs @@ -13,10 +13,12 @@ //! - [`sse`] — a provider-agnostic SSE line decoder. Bridges that stream //! over SSE feed it raw bytes and pull typed events back out. //! - [`credential`] — cache keys for credential-derived upstream tokens. +//! - [`upstream_http`] — connection-layer settings every provider client +//! shares (connect timeout, TCP keepalive, pool expiry) plus the +//! cause-chain rendering for transport errors. //! -//! The concrete HTTP transport lives in the provider crates — keeping -//! this crate free of `reqwest` at the public-API level makes it testable -//! without wiremock. +//! Request/response translation lives in the provider crates; this crate +//! owns only what all of them must agree on. #![forbid(unsafe_code)] #![deny(rust_2018_idioms)] @@ -26,6 +28,7 @@ pub mod chat; pub mod credential; pub mod hub; pub mod sse; +pub mod upstream_http; pub use bridge::{ capture_upstream_error_http, content_type_is_json, parse_retry_after, read_body_capped, @@ -39,3 +42,6 @@ pub use chat::{ pub use credential::credential_fingerprint; pub use hub::Hub; pub use sse::{SseDecoder, SseEvent}; +pub use upstream_http::{ + client_builder, error_with_causes, transport_error_message, UpstreamHttpConfig, +}; diff --git a/crates/aisix-gateway/src/upstream_http.rs b/crates/aisix-gateway/src/upstream_http.rs new file mode 100644 index 00000000..d79a7fe4 --- /dev/null +++ b/crates/aisix-gateway/src/upstream_http.rs @@ -0,0 +1,392 @@ +//! Connection-layer settings and error rendering for upstream HTTP calls. +//! +//! Every provider bridge talks to its upstream through a `reqwest::Client`. +//! Two things live here because they must be identical across all of them: +//! +//! - [`client_builder`] — a `ClientBuilder` pre-loaded with the process-wide +//! connection settings ([`UpstreamHttpConfig`]). reqwest's own defaults +//! leave TCP keepalive off, impose no connect timeout, and keep idle +//! pooled connections for 90s — longer than the idle timeout of a typical +//! LB/NAT/proxy hop in front of a provider, so a pooled connection can be +//! reaped upstream and still be handed out here, failing the next request. +//! - [`transport_error_message`] — renders a `reqwest::Error` with its full +//! `source()` chain. The top-level `Display` is only ever +//! "error sending request for url (…)", which is the same string for a DNS +//! failure, a TCP reset, a TLS handshake error, and a stale pooled +//! connection. The chain is what tells them apart. + +use std::sync::OnceLock; +use std::time::Duration; + +/// Suffixes marking a query parameter whose value is a credential and must +/// be redacted out of logged URLs. Vertex/Gemini accept `?key=` and +/// `?access_token=`, and an operator can put either directly in a +/// ProviderKey `api_base`, so a URL echoed into a log line can carry live +/// credentials. +/// +/// Matched as a **suffix** of the parameter name after lowercasing and +/// stripping `-`/`_`, which covers the vendor-prefixed and punctuation +/// variants without enumerating them: `api-key` / `api_key` / `apiKey` all +/// end in `key`; `client_secret` in `secret`; SigV4's `X-Amz-Signature`, +/// `X-Amz-Security-Token`, and `X-Amz-Credential` in `signature`, `token`, +/// and `credential`. Over-redacting an unrelated parameter costs a little +/// diagnostic detail; under-redacting leaks a live key into a log store. +const SENSITIVE_PARAM_SUFFIXES: &[&str] = &[ + "key", + "token", + "secret", + "password", + "credential", + "sig", + "signature", +]; + +/// Whether a query parameter's value is credential material. +fn is_sensitive_param(name: &str) -> bool { + let normalized: String = name + .chars() + .filter(|c| *c != '-' && *c != '_') + .flat_map(|c| c.to_lowercase()) + .collect(); + SENSITIVE_PARAM_SUFFIXES + .iter() + .any(|s| normalized.ends_with(s)) +} + +/// Cap on how many `source()` links are walked. Real reqwest/hyper chains +/// are 3-5 deep; the bound just keeps a pathological cycle from running away. +const MAX_SOURCE_DEPTH: usize = 8; + +/// Connection-layer settings shared by every upstream provider client. +/// +/// Defaults follow the same reasoning LiteLLM applies to its own upstream +/// pool: bound the connect phase, keep the kernel probing so a NAT/LB hop +/// can't silently reap a connection while a slow model is still thinking, +/// and expire pooled connections well before a typical upstream idle +/// timeout would. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct UpstreamHttpConfig { + /// Max time for DNS + TCP + TLS before the attempt fails. Without it a + /// black-holed upstream is only bounded by the model's overall timeout. + pub connect_timeout: Option, + /// Idle time before the kernel sends the first TCP keepalive probe. + /// Keeps a long wait for a slow first token from being reaped by a NAT + /// or LB idle timer. + pub tcp_keepalive: Option, + /// Interval between subsequent keepalive probes. + pub tcp_keepalive_interval: Option, + /// Unacknowledged probes before the kernel drops the connection. + pub tcp_keepalive_retries: Option, + /// How long an idle connection may sit in the pool before it is + /// discarded. Must stay below the shortest idle timeout on the path to + /// the provider, or the pool will hand out connections the far end has + /// already closed. + pub pool_idle_timeout: Option, + /// Cap on idle connections kept per upstream host. `None` leaves + /// reqwest's default (unbounded). + pub pool_max_idle_per_host: Option, +} + +impl Default for UpstreamHttpConfig { + fn default() -> Self { + Self { + connect_timeout: Some(Duration::from_secs(5)), + tcp_keepalive: Some(Duration::from_secs(60)), + tcp_keepalive_interval: Some(Duration::from_secs(30)), + tcp_keepalive_retries: Some(5), + pool_idle_timeout: Some(Duration::from_secs(30)), + pool_max_idle_per_host: None, + } + } +} + +static CONFIG: OnceLock = OnceLock::new(); + +/// Install the process-wide upstream connection settings. Called once +/// during boot, before any bridge builds its client. Later calls are +/// ignored — the pools are already built, so a second set would silently +/// not apply. +pub fn init(cfg: UpstreamHttpConfig) { + let _ = CONFIG.set(cfg); +} + +/// The active settings, defaulting when [`init`] was never called (tests, +/// embedded uses). +pub fn config() -> &'static UpstreamHttpConfig { + CONFIG.get_or_init(UpstreamHttpConfig::default) +} + +/// A `reqwest::ClientBuilder` with the connection settings applied. Callers +/// add their own `user_agent` / TLS options and `build()`. +pub fn client_builder() -> reqwest::ClientBuilder { + let cfg = config(); + let mut b = reqwest::Client::builder() + .pool_idle_timeout(cfg.pool_idle_timeout) + .tcp_keepalive(cfg.tcp_keepalive); + if let Some(d) = cfg.connect_timeout { + b = b.connect_timeout(d); + } + if let Some(d) = cfg.tcp_keepalive_interval { + b = b.tcp_keepalive_interval(d); + } + if let Some(n) = cfg.tcp_keepalive_retries { + b = b.tcp_keepalive_retries(n); + } + if let Some(n) = cfg.pool_max_idle_per_host { + b = b.pool_max_idle_per_host(n); + } + b +} + +/// Render a `reqwest::Error` as a single diagnostic line: the top-level +/// message followed by every distinct `source()` cause, with credentials +/// stripped from any embedded URL. +/// +/// reqwest's own `Display` stops at "error sending request for url (…)", +/// which is identical for a DNS failure, a refused connection, a TLS +/// handshake error, and a pooled connection the far end already closed. +/// The causes below it are what name the actual fault, e.g. +/// `… : client error (Connect): tcp connect error: Connection refused (os error 111)`. +pub fn transport_error_message(err: &reqwest::Error) -> String { + let mut msg = err.to_string(); + if let Some(url) = err.url() { + let raw = url.as_str(); + if msg.contains(raw) { + msg = msg.replace(raw, &redact_url(url)); + } + } + append_source_chain(&mut msg, err); + msg +} + +/// Same as [`transport_error_message`] for error types that aren't +/// `reqwest::Error` (websocket handshakes, SDK dispatch errors) — no URL +/// is available to redact, so only the cause chain is appended. +pub fn error_with_causes(err: &(dyn std::error::Error + 'static)) -> String { + let mut msg = err.to_string(); + append_source_chain(&mut msg, err); + msg +} + +fn append_source_chain(msg: &mut String, err: &(dyn std::error::Error + 'static)) { + let mut source = err.source(); + let mut depth = 0; + while let Some(cause) = source { + if depth >= MAX_SOURCE_DEPTH { + break; + } + let text = cause.to_string(); + // hyper repeats the innermost message at several levels; only add + // a cause that isn't already the tail of what we have. + if !text.is_empty() && !msg.ends_with(&text) { + msg.push_str(": "); + msg.push_str(&text); + } + source = cause.source(); + depth += 1; + } +} + +/// Replace the values of credential-bearing query parameters with +/// `REDACTED`, leaving everything else (host, path, `api-version`, …) +/// intact for diagnosis. +fn redact_url(url: &reqwest::Url) -> String { + if url.query().is_none() { + return url.as_str().to_string(); + } + let mut out = url.clone(); + let pairs: Vec<(String, String)> = url + .query_pairs() + .map(|(k, v)| { + if is_sensitive_param(&k) { + (k.into_owned(), "REDACTED".to_string()) + } else { + (k.into_owned(), v.into_owned()) + } + }) + .collect(); + { + let mut q = out.query_pairs_mut(); + q.clear(); + for (k, v) in &pairs { + q.append_pair(k, v); + } + } + out.to_string() +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn defaults_bound_connect_and_expire_idle_before_reqwest_would() { + let cfg = UpstreamHttpConfig::default(); + assert!(cfg.connect_timeout.is_some(), "connect must be bounded"); + assert!(cfg.tcp_keepalive.is_some(), "keepalive must be on"); + // reqwest's own default is 90s; anything at or above that reopens + // the stale-pooled-connection window this config exists to close. + assert!(cfg.pool_idle_timeout.unwrap() < Duration::from_secs(90)); + } + + #[test] + fn client_builder_applies_settings() { + // Smoke: the builder must accept every configured knob. + let client = client_builder().user_agent("aisix-test").build(); + assert!(client.is_ok(), "{:?}", client.err()); + } + + #[test] + fn redacts_credential_query_params_only() { + let url = reqwest::Url::parse( + "https://generativelanguage.googleapis.com/v1beta/models/gemini:generateContent\ + ?key=AIzaSy-super-secret&alt=sse", + ) + .unwrap(); + let out = redact_url(&url); + assert!(!out.contains("AIzaSy-super-secret"), "{out}"); + assert!(out.contains("key=REDACTED"), "{out}"); + // Non-credential params stay readable — they're the diagnostic bit. + assert!(out.contains("alt=sse"), "{out}"); + } + + #[test] + fn redacts_access_token_case_insensitively() { + let url = + reqwest::Url::parse("https://example.com/v1/chat?Access_Token=ya29.live&x=1").unwrap(); + let out = redact_url(&url); + assert!(!out.contains("ya29.live"), "{out}"); + assert!(out.contains("x=1"), "{out}"); + } + + /// Suffix matching exists so vendor-prefixed and punctuation variants + /// don't have to be enumerated one by one — each of these would have + /// slipped through an exact-name denylist. + #[test] + fn redacts_credential_parameter_aliases() { + for name in [ + "api-key", + "api_key", + "apiKey", + "client_secret", + "client-secret", + "X-Amz-Signature", + "X-Amz-Security-Token", + "X-Amz-Credential", + "SIG", + "refresh_token", + "subscription-key", + ] { + let url = reqwest::Url::parse(&format!("https://h/p?{name}=live-secret-value&keep=1")) + .unwrap(); + let out = redact_url(&url); + assert!( + !out.contains("live-secret-value"), + "{name} was not redacted: {out}" + ); + assert!(out.contains("keep=1"), "{name} over-redacted: {out}"); + } + } + + /// The flip side: parameters that merely look credential-ish must stay + /// readable, since they are the diagnostic content of the URL. + #[test] + fn keeps_non_credential_parameters_readable() { + let url = reqwest::Url::parse( + "https://h/p?api-version=2024-10-21&alt=sse&keyword=hello&signature_version=4", + ) + .unwrap(); + let out = redact_url(&url); + assert!(out.contains("api-version=2024-10-21"), "{out}"); + assert!(out.contains("alt=sse"), "{out}"); + assert!(out.contains("keyword=hello"), "{out}"); + assert!(out.contains("signature_version=4"), "{out}"); + } + + #[test] + fn url_without_query_is_untouched() { + let url = reqwest::Url::parse("https://api.openai.com/v1/chat/completions").unwrap(); + assert_eq!( + redact_url(&url), + "https://api.openai.com/v1/chat/completions" + ); + } + + #[derive(Debug)] + struct Layer(&'static str, Option>); + + impl std::fmt::Display for Layer { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.write_str(self.0) + } + } + + impl std::error::Error for Layer { + fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { + self.1 + .as_ref() + .map(|b| b.as_ref() as &(dyn std::error::Error + 'static)) + } + } + + #[test] + fn cause_chain_is_flattened_into_one_line() { + let err = Layer( + "error sending request", + Some(Box::new(Layer( + "client error (Connect)", + Some(Box::new(Layer("tcp connect error: refused", None))), + ))), + ); + assert_eq!( + error_with_causes(&err), + "error sending request: client error (Connect): tcp connect error: refused" + ); + } + + #[test] + fn repeated_tail_cause_is_not_duplicated() { + // hyper commonly restates the innermost message one level up. + let err = Layer("outer: refused", Some(Box::new(Layer("refused", None)))); + assert_eq!(error_with_causes(&err), "outer: refused"); + } + + /// The whole point of `transport_error_message`, against a real + /// `reqwest::Error` rather than a hand-built chain: reqwest's own + /// `Display` is "error sending request for url (…)" for a refused + /// connection, a DNS failure, a TLS error, and a stale pooled + /// connection alike. Operators can't tell those apart, which is what + /// made AISIX-Cloud#1122 undiagnosable from the logs. + #[tokio::test] + async fn real_transport_error_names_the_root_cause() { + // Bind an ephemeral loopback port and immediately release it, so the + // connect is refused straight away without assuming any fixed port + // is free (no timeout, no external network needed). + let listener = std::net::TcpListener::bind("127.0.0.1:0").expect("bind loopback"); + let addr = listener.local_addr().expect("local addr"); + drop(listener); + + let client = client_builder().build().expect("client builds"); + let err = client + .get(format!("http://{addr}/v1/chat/completions")) + .send() + .await + .expect_err("connect to a closed port must fail"); + + let top_level = err.to_string(); + let with_causes = transport_error_message(&err); + + assert!( + !top_level.to_lowercase().contains("refused"), + "reqwest's Display is expected to hide the cause; got: {top_level}" + ); + assert!( + with_causes.to_lowercase().contains("refused"), + "the cause chain must name the actual fault; got: {with_causes}" + ); + assert!( + with_causes.len() > top_level.len(), + "causes must add information" + ); + } +} diff --git a/crates/aisix-provider-anthropic/src/bridge.rs b/crates/aisix-provider-anthropic/src/bridge.rs index a13cf7f0..b0ee1ac5 100644 --- a/crates/aisix-provider-anthropic/src/bridge.rs +++ b/crates/aisix-provider-anthropic/src/bridge.rs @@ -68,7 +68,7 @@ impl Default for AnthropicBridge { } fn default_client() -> Client { - Client::builder() + aisix_gateway::client_builder() .user_agent("aisix/0.1") .build() .unwrap_or_else(|_| Client::new()) @@ -226,6 +226,7 @@ where Ok(r) => r, Err(_) => Err(BridgeError::Timeout { elapsed_ms: started.elapsed().as_millis() as u64, + cause: String::new(), }), }, } @@ -265,7 +266,7 @@ impl Bridge for AnthropicBridge { .json(&body) .send() .await - .map_err(|e| BridgeError::Transport(e.to_string()))?; + .map_err(|e| BridgeError::Transport(aisix_gateway::transport_error_message(&e)))?; let status = resp.status(); if !status.is_success() { @@ -310,7 +311,7 @@ impl Bridge for AnthropicBridge { .json(&body) .send() .await - .map_err(|e| BridgeError::Transport(e.to_string())) + .map_err(|e| BridgeError::Transport(aisix_gateway::transport_error_message(&e))) }) .await?; @@ -337,7 +338,7 @@ where let mut state = StreamState::default(); while let Some(next) = stream.next().await { - let chunk = next.map_err(|e| BridgeError::Transport(e.to_string()))?; + let chunk = next.map_err(|e| BridgeError::Transport(aisix_gateway::transport_error_message(&e)))?; for event in decoder.feed(chunk.as_ref()) { let SseEvent::Data(payload) = event else { continue }; let parsed: AnthropicStreamEvent = serde_json::from_str(&payload) diff --git a/crates/aisix-provider-azure-openai/src/bridge.rs b/crates/aisix-provider-azure-openai/src/bridge.rs index accb8907..b9089721 100644 --- a/crates/aisix-provider-azure-openai/src/bridge.rs +++ b/crates/aisix-provider-azure-openai/src/bridge.rs @@ -151,7 +151,7 @@ impl Default for AzureOpenAiBridge { } fn default_client() -> Client { - Client::builder() + aisix_gateway::client_builder() .user_agent("aisix/0.1") .build() .unwrap_or_else(|_| Client::new()) @@ -545,6 +545,7 @@ where Ok(r) => r, Err(_) => Err(BridgeError::Timeout { elapsed_ms: started.elapsed().as_millis() as u64, + cause: String::new(), }), }, } @@ -700,7 +701,7 @@ impl Bridge for AzureOpenAiBridge { .json(&body) .send() .await - .map_err(|e| BridgeError::Transport(e.to_string()))?; + .map_err(|e| BridgeError::Transport(aisix_gateway::transport_error_message(&e)))?; let status = resp.status(); if !status.is_success() { @@ -757,7 +758,7 @@ impl Bridge for AzureOpenAiBridge { .json(&body) .send() .await - .map_err(|e| BridgeError::Transport(e.to_string())) + .map_err(|e| BridgeError::Transport(aisix_gateway::transport_error_message(&e))) }) .await?; @@ -827,6 +828,7 @@ where Err(_) => { Err(BridgeError::Timeout { elapsed_ms: started.elapsed().as_millis() as u64, + cause: String::new(), })?; unreachable!() } @@ -834,7 +836,7 @@ where None => stream.next().await, }; let Some(next) = next else { break 'outer; }; - let chunk = next.map_err(|e| BridgeError::Transport(e.to_string()))?; + let chunk = next.map_err(|e| BridgeError::Transport(aisix_gateway::transport_error_message(&e)))?; for event in decoder.feed(chunk.as_ref()) { match event { SseEvent::Done => { diff --git a/crates/aisix-provider-bedrock/src/bridge.rs b/crates/aisix-provider-bedrock/src/bridge.rs index 4de6a3cb..46dcb6d3 100644 --- a/crates/aisix-provider-bedrock/src/bridge.rs +++ b/crates/aisix-provider-bedrock/src/bridge.rs @@ -505,6 +505,7 @@ fn map_sdk_error( }; BridgeError::Timeout { elapsed_ms: reported, + cause: String::new(), } } SdkError::DispatchFailure(_) => BridgeError::Transport("upstream dispatch failed".into()), @@ -1501,6 +1502,7 @@ where if started.elapsed() >= d { return BridgeError::Timeout { elapsed_ms: started.elapsed().as_millis() as u64, + cause: String::new(), }; } } @@ -1508,6 +1510,7 @@ where SdkError::ServiceError(svc) => bedrock_service_error_to_upstream_status(svc), SdkError::TimeoutError(_) => BridgeError::Timeout { elapsed_ms: started.elapsed().as_millis() as u64, + cause: String::new(), }, other => BridgeError::Transport(format!("{other}")), } diff --git a/crates/aisix-provider-openai/src/bridge.rs b/crates/aisix-provider-openai/src/bridge.rs index 40c3083f..43ab2c57 100644 --- a/crates/aisix-provider-openai/src/bridge.rs +++ b/crates/aisix-provider-openai/src/bridge.rs @@ -148,7 +148,7 @@ impl Default for OpenAiBridge { } fn default_client() -> Client { - Client::builder() + aisix_gateway::client_builder() .user_agent("aisix/0.1") .build() .unwrap_or_else(|_| Client::new()) @@ -282,6 +282,7 @@ where Ok(r) => r, Err(_) => Err(BridgeError::Timeout { elapsed_ms: started.elapsed().as_millis() as u64, + cause: String::new(), }), }, } @@ -407,7 +408,7 @@ impl Bridge for OpenAiBridge { .json(&body) .send() .await - .map_err(|e| BridgeError::Transport(e.to_string()))?; + .map_err(|e| BridgeError::Transport(aisix_gateway::transport_error_message(&e)))?; let status = resp.status(); if !status.is_success() { @@ -457,7 +458,7 @@ impl Bridge for OpenAiBridge { .json(&body) .send() .await - .map_err(|e| BridgeError::Transport(e.to_string()))?; + .map_err(|e| BridgeError::Transport(aisix_gateway::transport_error_message(&e)))?; let status = resp.status(); if !status.is_success() { @@ -514,7 +515,7 @@ impl Bridge for OpenAiBridge { .json(&outbound) .send() .await - .map_err(|e| BridgeError::Transport(e.to_string()))?; + .map_err(|e| BridgeError::Transport(aisix_gateway::transport_error_message(&e)))?; let status = resp.status(); if !status.is_success() { @@ -569,7 +570,7 @@ impl Bridge for OpenAiBridge { .json(&outbound) .send() .await - .map_err(|e| BridgeError::Transport(e.to_string()))?; + .map_err(|e| BridgeError::Transport(aisix_gateway::transport_error_message(&e)))?; let status = resp.status(); if !status.is_success() { @@ -616,7 +617,7 @@ impl Bridge for OpenAiBridge { .json(&body) .send() .await - .map_err(|e| BridgeError::Transport(e.to_string())) + .map_err(|e| BridgeError::Transport(aisix_gateway::transport_error_message(&e))) }) .await?; @@ -668,7 +669,7 @@ where let mut stream = Box::pin(byte_stream); let mut done_marker_seen = false; 'outer: while let Some(next) = stream.next().await { - let chunk = next.map_err(|e| BridgeError::Transport(e.to_string()))?; + let chunk = next.map_err(|e| BridgeError::Transport(aisix_gateway::transport_error_message(&e)))?; for event in decoder.feed(chunk.as_ref()) { match event { SseEvent::Done => { diff --git a/crates/aisix-provider-vertex/src/bridge.rs b/crates/aisix-provider-vertex/src/bridge.rs index 9841d769..91b19414 100644 --- a/crates/aisix-provider-vertex/src/bridge.rs +++ b/crates/aisix-provider-vertex/src/bridge.rs @@ -245,7 +245,7 @@ impl Default for VertexBridge { } fn default_client() -> Client { - Client::builder() + aisix_gateway::client_builder() .user_agent("aisix/0.1") .build() .unwrap_or_else(|_| Client::new()) @@ -520,6 +520,7 @@ where Ok(r) => r, Err(_) => Err(BridgeError::Timeout { elapsed_ms: started.elapsed().as_millis() as u64, + cause: String::new(), }), }, } @@ -699,7 +700,7 @@ impl Bridge for VertexBridge { .json(&body) .send() .await - .map_err(|e| BridgeError::Transport(e.to_string()))?; + .map_err(|e| BridgeError::Transport(aisix_gateway::transport_error_message(&e)))?; let status = resp.status(); if !status.is_success() { return Err(map_http_error(status, resp).await); @@ -827,7 +828,7 @@ impl VertexBridge { .json(&body) .send() .await - .map_err(|e| BridgeError::Transport(e.to_string()))?; + .map_err(|e| BridgeError::Transport(aisix_gateway::transport_error_message(&e)))?; let status = resp.status(); if !status.is_success() { @@ -923,7 +924,7 @@ impl VertexBridge { .json(&body_value) .send() .await - .map_err(|e| BridgeError::Transport(e.to_string()))?; + .map_err(|e| BridgeError::Transport(aisix_gateway::transport_error_message(&e)))?; let status = resp.status(); if !status.is_success() { @@ -1011,7 +1012,7 @@ impl VertexBridge { .json(&body_value) .send() .await - .map_err(|e| BridgeError::Transport(e.to_string())) + .map_err(|e| BridgeError::Transport(aisix_gateway::transport_error_message(&e))) }) .await?; @@ -1033,7 +1034,7 @@ impl VertexBridge { let mut byte_stream = Box::pin(byte_stream); while let Some(item) = byte_stream.next().await { - let bytes: Bytes = item.map_err(|e| BridgeError::Transport(e.to_string()))?; + let bytes: Bytes = item.map_err(|e| BridgeError::Transport(aisix_gateway::transport_error_message(&e)))?; for event in decoder.feed(bytes.as_ref()) { let SseEvent::Data(data) = event else { continue }; let parsed: AnthropicStreamEvent = @@ -1123,7 +1124,7 @@ impl VertexBridge { .json(&body) .send() .await - .map_err(|e| BridgeError::Transport(e.to_string()))?; + .map_err(|e| BridgeError::Transport(aisix_gateway::transport_error_message(&e)))?; let status = resp.status(); if !status.is_success() { return Err(map_http_error(status, resp).await); @@ -1180,7 +1181,7 @@ impl VertexBridge { .json(&body) .send() .await - .map_err(|e| BridgeError::Transport(e.to_string())) + .map_err(|e| BridgeError::Transport(aisix_gateway::transport_error_message(&e))) }) .await?; @@ -1195,7 +1196,7 @@ impl VertexBridge { let mut byte_stream = Box::pin(byte_stream); while let Some(item) = byte_stream.next().await { - let bytes: Bytes = item.map_err(|e| BridgeError::Transport(e.to_string()))?; + let bytes: Bytes = item.map_err(|e| BridgeError::Transport(aisix_gateway::transport_error_message(&e)))?; for event in decoder.feed(bytes.as_ref()) { match event { SseEvent::Data(data) => { @@ -1317,7 +1318,7 @@ impl VertexBridge { .json(&body) .send() .await - .map_err(|e| BridgeError::Transport(e.to_string()))?; + .map_err(|e| BridgeError::Transport(aisix_gateway::transport_error_message(&e)))?; let status = resp.status(); if !status.is_success() { return Err(map_http_error(status, resp).await); @@ -1387,7 +1388,7 @@ impl VertexBridge { .json(&body) .send() .await - .map_err(|e| BridgeError::Transport(e.to_string())) + .map_err(|e| BridgeError::Transport(aisix_gateway::transport_error_message(&e))) }) .await?; @@ -1402,7 +1403,7 @@ impl VertexBridge { let mut byte_stream = Box::pin(byte_stream); while let Some(item) = byte_stream.next().await { - let bytes: Bytes = item.map_err(|e| BridgeError::Transport(e.to_string()))?; + let bytes: Bytes = item.map_err(|e| BridgeError::Transport(aisix_gateway::transport_error_message(&e)))?; for event in decoder.feed(bytes.as_ref()) { match event { SseEvent::Data(data) => { @@ -1502,7 +1503,7 @@ impl VertexBridge { .json(&body) .send() .await - .map_err(|e| BridgeError::Transport(e.to_string())) + .map_err(|e| BridgeError::Transport(aisix_gateway::transport_error_message(&e))) }) .await?; @@ -1524,7 +1525,7 @@ impl VertexBridge { let mut byte_stream = Box::pin(byte_stream); while let Some(item) = byte_stream.next().await { - let bytes: Bytes = item.map_err(|e| BridgeError::Transport(e.to_string()))?; + let bytes: Bytes = item.map_err(|e| BridgeError::Transport(aisix_gateway::transport_error_message(&e)))?; for event in decoder.feed(bytes.as_ref()) { if let SseEvent::Data(data) = event { let parsed: GeminiGenerateContentResponse = diff --git a/crates/aisix-proxy/src/audio.rs b/crates/aisix-proxy/src/audio.rs index 58f4c958..822dec40 100644 --- a/crates/aisix-proxy/src/audio.rs +++ b/crates/aisix-proxy/src/audio.rs @@ -626,7 +626,7 @@ async fn multipart_dispatch( &state.runtime_status, &model_entry.id, model.cooldown.as_ref(), - aisix_gateway::BridgeError::Transport(e.to_string()), + aisix_gateway::BridgeError::Transport(aisix_gateway::transport_error_message(&e)), ) }) .map_err(ProxyError::Bridge)?; @@ -989,7 +989,7 @@ async fn speech_dispatch( &state.runtime_status, &model_entry.id, model.cooldown.as_ref(), - aisix_gateway::BridgeError::Transport(e.to_string()), + aisix_gateway::BridgeError::Transport(aisix_gateway::transport_error_message(&e)), ) }) .map_err(ProxyError::Bridge)?; diff --git a/crates/aisix-proxy/src/chat.rs b/crates/aisix-proxy/src/chat.rs index 090ba573..4a3a63f1 100644 --- a/crates/aisix-proxy/src/chat.rs +++ b/crates/aisix-proxy/src/chat.rs @@ -4792,7 +4792,14 @@ mod cooldown_tests { #[test] fn default_config_cooldowns_timeout_and_transport_errors() { - assert!(decide_cooldown(&BridgeError::Timeout { elapsed_ms: 30_000 }, None).is_some()); + assert!(decide_cooldown( + &BridgeError::Timeout { + cause: String::new(), + elapsed_ms: 30_000 + }, + None + ) + .is_some()); assert!(decide_cooldown(&BridgeError::Transport("conn refused".into()), None).is_some()); assert!(decide_cooldown(&BridgeError::StreamAborted, None).is_some()); assert!(decide_cooldown(&BridgeError::UpstreamDecode("bad json".into()), None).is_some()); @@ -4856,7 +4863,14 @@ mod cooldown_tests { trigger_on_timeout: Some(false), ..Default::default() }; - assert!(decide_cooldown(&BridgeError::Timeout { elapsed_ms: 1 }, Some(&cfg)).is_none()); + assert!(decide_cooldown( + &BridgeError::Timeout { + cause: String::new(), + elapsed_ms: 1 + }, + Some(&cfg) + ) + .is_none()); } #[test] diff --git a/crates/aisix-proxy/src/cooldown.rs b/crates/aisix-proxy/src/cooldown.rs index f5d7d22f..9a926d1d 100644 --- a/crates/aisix-proxy/src/cooldown.rs +++ b/crates/aisix-proxy/src/cooldown.rs @@ -161,7 +161,14 @@ mod tests { }; assert!(decide_cooldown(&upstream(429), Some(&cfg)).is_none()); assert!(decide_cooldown(&upstream(503), Some(&cfg)).is_none()); - assert!(decide_cooldown(&BridgeError::Timeout { elapsed_ms: 1 }, Some(&cfg)).is_none()); + assert!(decide_cooldown( + &BridgeError::Timeout { + cause: String::new(), + elapsed_ms: 1 + }, + Some(&cfg) + ) + .is_none()); } #[test] diff --git a/crates/aisix-proxy/src/dispatch.rs b/crates/aisix-proxy/src/dispatch.rs index 796f091f..d55e89f2 100644 --- a/crates/aisix-proxy/src/dispatch.rs +++ b/crates/aisix-proxy/src/dispatch.rs @@ -26,18 +26,25 @@ use aisix_gateway::{Bridge, BridgeError, Hub}; /// Map a `reqwest` transport error from a raw-passthrough dispatch /// (`/v1/responses`, `/v1/messages` Anthropic, `/v1/messages/count_tokens`) -/// into the gateway's [`BridgeError`]. A timed-out request (the per-request -/// `.timeout(model.request_timeout())` budget elapsed) becomes +/// into the gateway's [`BridgeError`]. A timed-out request becomes /// [`BridgeError::Timeout`] so it surfaces as 504, classifies as `"timeout"` /// in telemetry, and participates in routing failover exactly like the /// Bridge-trait path (#554). Everything else stays a transport error. +/// +/// `is_timeout()` is satisfied by three unrelated conditions — the +/// configured request budget expiring in hyper, the `connect_timeout` +/// expiring, and the kernel returning `ETIMEDOUT` for an unanswered SYN — +/// so the reqwest cause chain is carried onto the error. Without it all +/// three render as one sentence and an operator cannot tell a slow +/// upstream from one that was never reached (AISIX-Cloud#1093). pub(crate) fn reqwest_error_to_bridge(e: &reqwest::Error, started: Instant) -> BridgeError { if e.is_timeout() { BridgeError::Timeout { elapsed_ms: started.elapsed().as_millis() as u64, + cause: aisix_gateway::transport_error_message(e), } } else { - BridgeError::Transport(e.to_string()) + BridgeError::Transport(aisix_gateway::transport_error_message(e)) } } @@ -267,6 +274,73 @@ mod tests { use super::*; use aisix_core::resource::ResourceEntry; + /// AISIX-Cloud#1093: `reqwest::Error::is_timeout()` is satisfied by an + /// expired request budget, an expired `connect_timeout`, and the + /// kernel's `ETIMEDOUT`. The mapped error must carry the cause chain so + /// those are distinguishable — otherwise every one of them renders as + /// the same "timed out after Nms" sentence. + #[tokio::test] + async fn timeout_mapping_carries_the_transport_cause() { + use wiremock::matchers::method; + use wiremock::{Mock, MockServer, ResponseTemplate}; + + let server = MockServer::start().await; + Mock::given(method("POST")) + .respond_with(ResponseTemplate::new(200).set_delay(std::time::Duration::from_secs(5))) + .mount(&server) + .await; + + let started = Instant::now(); + let err = reqwest::Client::new() + .post(server.uri()) + .timeout(std::time::Duration::from_millis(50)) + .send() + .await + .expect_err("the 50ms budget must expire against a 5s upstream"); + assert!(err.is_timeout(), "precondition: reqwest reports a timeout"); + + let mapped = reqwest_error_to_bridge(&err, started); + match &mapped { + BridgeError::Timeout { cause, .. } => { + assert!(!cause.is_empty(), "timeout must carry its cause chain"); + } + other => panic!("expected Timeout, got {other:?}"), + } + // The rendered message keeps the elapsed budget AND names the cause. + let rendered = mapped.to_string(); + assert!( + rendered.contains("upstream request timed out"), + "{rendered}" + ); + assert!( + rendered.len() > "upstream request timed out after 50ms".len(), + "cause must widen the message: {rendered}" + ); + } + + /// A non-timeout transport failure still maps to `Transport`, so the + /// two stay distinguishable by variant as well as by message. + #[tokio::test] + async fn non_timeout_transport_error_stays_transport() { + let listener = std::net::TcpListener::bind("127.0.0.1:0").unwrap(); + let addr = listener.local_addr().unwrap(); + drop(listener); + + let err = reqwest::Client::new() + .post(format!("http://{addr}/")) + .send() + .await + .expect_err("connect to a closed port must fail"); + assert!(!err.is_timeout()); + + match reqwest_error_to_bridge(&err, Instant::now()) { + BridgeError::Transport(msg) => { + assert!(msg.to_lowercase().contains("refused"), "{msg}"); + } + other => panic!("expected Transport, got {other:?}"), + } + } + fn snapshot_with(provider_key_id: &str) -> AisixSnapshot { let snap = AisixSnapshot::new(); let pk: ProviderKey = serde_json::from_str( diff --git a/crates/aisix-proxy/src/ensemble.rs b/crates/aisix-proxy/src/ensemble.rs index 5f5b4232..44e12b84 100644 --- a/crates/aisix-proxy/src/ensemble.rs +++ b/crates/aisix-proxy/src/ensemble.rs @@ -409,6 +409,7 @@ async fn call_with_optional_timeout( Ok(result) => result, Err(_) => Err(BridgeError::Timeout { elapsed_ms: d.as_millis() as u64, + cause: String::new(), }), }, None => caller.call(target, req).await, @@ -626,7 +627,10 @@ mod tests { .on( "judge", vec![ - Err(BridgeError::Timeout { elapsed_ms: 1 }), + Err(BridgeError::Timeout { + cause: String::new(), + elapsed_ms: 1, + }), ok("judge", "second attempt wins"), ], ); diff --git a/crates/aisix-proxy/src/error.rs b/crates/aisix-proxy/src/error.rs index 54537005..1cebab01 100644 --- a/crates/aisix-proxy/src/error.rs +++ b/crates/aisix-proxy/src/error.rs @@ -320,6 +320,18 @@ impl ProxyError { { return render_bridge_upstream_envelope(*status, message, parsed.as_deref(), *wire); } + // A timeout's transport cause names the upstream host and the + // connection-layer fault it hit. Same rule as the 5xx body below: + // that is operator diagnostics, so it reaches the logs and the + // per-attempt telemetry through `Display`, while the caller keeps + // the bare sentence it has always had — an `api_base` is internal + // topology and does not belong in a customer-facing envelope. + if let ProxyError::Bridge(aisix_gateway::BridgeError::Timeout { elapsed_ms, .. }) = self { + return ErrorEnvelope::new( + format!("upstream request timed out after {elapsed_ms}ms"), + self.kind(), + ); + } let env = ErrorEnvelope::new(self.to_string(), self.kind()); match self { ProxyError::BudgetExceeded(r) => env.with_code("budget_exceeded").with_budget(r), @@ -543,6 +555,37 @@ pub(crate) fn proxy_error_from_json_rejection( mod tests { use super::*; + /// AISIX-Cloud#1093 carries the transport cause on a timeout so an + /// operator can tell a `connect_timeout` from an expired request + /// budget. That cause names the upstream host, so it must reach the + /// logs and telemetry (`Display`) but NOT the caller's envelope — + /// same split the 5xx path already enforces. + #[test] + fn timeout_cause_reaches_logs_but_not_the_caller() { + let err = ProxyError::Bridge(aisix_gateway::BridgeError::Timeout { + elapsed_ms: 5_002, + cause: "error sending request for url (http://10.1.2.3:8080/v1/messages): \ + client error (Connect): tcp connect error: deadline has elapsed" + .to_string(), + }); + + // Operator-facing: the full chain, which is what the WARN log line + // and the per-attempt `error_message` are built from. + let logged = err.to_string(); + assert!(logged.contains("deadline has elapsed"), "{logged}"); + assert!(logged.contains("10.1.2.3"), "{logged}"); + + // Customer-facing: the bare sentence, byte-identical to what it + // was before `cause` existed, with no internal topology in it. + let envelope = err.envelope(); + assert_eq!( + envelope.error.message, + "upstream request timed out after 5002ms" + ); + assert!(!envelope.error.message.contains("10.1.2.3")); + assert_eq!(err.status(), StatusCode::GATEWAY_TIMEOUT); + } + #[test] fn missing_auth_maps_to_401_invalid_api_key() { let e = ProxyError::MissingAuth; diff --git a/crates/aisix-proxy/src/http_client.rs b/crates/aisix-proxy/src/http_client.rs index b0369acd..3b724210 100644 --- a/crates/aisix-proxy/src/http_client.rs +++ b/crates/aisix-proxy/src/http_client.rs @@ -2,6 +2,9 @@ //! //! Initialised lazily once and reused across all calls so the connection //! pool is shared and we don't pay TLS handshake cost on every request. +//! Connection-layer settings come from `aisix_gateway::upstream_http`, the +//! same source the provider bridges use — this client talks to the same +//! upstreams, so it must expire pooled connections on the same schedule. use reqwest::Client; use std::sync::OnceLock; @@ -10,7 +13,7 @@ use std::sync::OnceLock; pub fn client() -> &'static Client { static CLIENT: OnceLock = OnceLock::new(); CLIENT.get_or_init(|| { - Client::builder() + aisix_gateway::client_builder() .user_agent("aisix/0.1") .build() .unwrap_or_else(|_| Client::new()) diff --git a/crates/aisix-proxy/src/jobs.rs b/crates/aisix-proxy/src/jobs.rs index 064136c2..a6590e98 100644 --- a/crates/aisix-proxy/src/jobs.rs +++ b/crates/aisix-proxy/src/jobs.rs @@ -366,7 +366,7 @@ async fn send_upstream( &state.runtime_status, &target.model_entry.id, target.model_entry.value.cooldown.as_ref(), - aisix_gateway::BridgeError::Transport(e.to_string()), + aisix_gateway::BridgeError::Transport(aisix_gateway::transport_error_message(&e)), ) }) .map_err(ProxyError::Bridge)?; diff --git a/crates/aisix-proxy/src/passthrough.rs b/crates/aisix-proxy/src/passthrough.rs index f5fc6c11..b1381b45 100644 --- a/crates/aisix-proxy/src/passthrough.rs +++ b/crates/aisix-proxy/src/passthrough.rs @@ -506,7 +506,7 @@ async fn dispatch( &state.runtime_status, &model_entry.id, model.cooldown.as_ref(), - aisix_gateway::BridgeError::Transport(e.to_string()), + aisix_gateway::BridgeError::Transport(aisix_gateway::transport_error_message(&e)), ) }) .map_err(ProxyError::Bridge)?; diff --git a/crates/aisix-proxy/src/realtime.rs b/crates/aisix-proxy/src/realtime.rs index 650c3f43..b0b4a7d1 100644 --- a/crates/aisix-proxy/src/realtime.rs +++ b/crates/aisix-proxy/src/realtime.rs @@ -405,7 +405,7 @@ async fn run_session( &state.runtime_status, &model_entry.id, model_entry.value.cooldown.as_ref(), - aisix_gateway::BridgeError::Transport(e.to_string()), + aisix_gateway::BridgeError::Transport(aisix_gateway::error_with_causes(&e)), ); emit_access_log( &Method::GET, diff --git a/crates/aisix-proxy/src/responses.rs b/crates/aisix-proxy/src/responses.rs index d1bf6573..8452665c 100644 --- a/crates/aisix-proxy/src/responses.rs +++ b/crates/aisix-proxy/src/responses.rs @@ -1075,6 +1075,7 @@ async fn responses_to_target( model.cooldown.as_ref(), aisix_gateway::BridgeError::Timeout { elapsed_ms: d.as_millis() as u64, + cause: String::new(), }, ) }) diff --git a/crates/aisix-proxy/src/routing.rs b/crates/aisix-proxy/src/routing.rs index 37abc150..8005d01f 100644 --- a/crates/aisix-proxy/src/routing.rs +++ b/crates/aisix-proxy/src/routing.rs @@ -1073,7 +1073,10 @@ mod tests { &[] )); assert!(is_retryable( - &BridgeError::Timeout { elapsed_ms: 1 }, + &BridgeError::Timeout { + cause: String::new(), + elapsed_ms: 1 + }, false, &[] )); diff --git a/crates/aisix-proxy/src/stream_timeout.rs b/crates/aisix-proxy/src/stream_timeout.rs index f1880d9c..63d9962c 100644 --- a/crates/aisix-proxy/src/stream_timeout.rs +++ b/crates/aisix-proxy/src/stream_timeout.rs @@ -49,6 +49,7 @@ pub(crate) fn with_read_timeout( Err(_) => { yield Err(BridgeError::Timeout { elapsed_ms: d.as_millis() as u64, + cause: String::new(), }); break; } @@ -102,6 +103,7 @@ pub(crate) async fn send_with_deadline( Ok(res) => res.map_err(|e| crate::dispatch::reqwest_error_to_bridge(&e, started)), Err(_) => Err(BridgeError::Timeout { elapsed_ms: started.elapsed().as_millis() as u64, + cause: String::new(), }), }, None => req diff --git a/crates/aisix-server/src/main.rs b/crates/aisix-server/src/main.rs index faceb15d..42ea0f88 100644 --- a/crates/aisix-server/src/main.rs +++ b/crates/aisix-server/src/main.rs @@ -31,7 +31,7 @@ use aisix_core::{ SourceKind, }; use aisix_etcd::{EtcdConfigProvider, SnapshotCache, Supervisor}; -use aisix_gateway::Hub; +use aisix_gateway::{Hub, UpstreamHttpConfig}; use aisix_obs::{init_tracing, install_otlp_tracer, Metrics}; use aisix_provider_anthropic::AnthropicBridge; use aisix_provider_azure_openai::AzureOpenAiBridge; @@ -44,6 +44,7 @@ use aisix_proxy::{CacheBackends, ProxyState}; use aisix_ratelimit::{Limiter, RedisStore}; use clap::Parser; use etcd_client::{Certificate, ConnectOptions, Identity, TlsOptions}; +use std::time::Duration; use tokio::sync::watch; #[derive(Debug, Parser)] @@ -150,6 +151,10 @@ async fn main() -> anyhow::Result<()> { let _otlp = install_otlp_tracer(&cfg.observability) .map_err(|e| anyhow::anyhow!("otlp init failed: {e}"))?; + // Before any bridge builds its `reqwest::Client` — the connection + // pools are constructed once and can't be reconfigured afterwards. + aisix_gateway::upstream_http::init(upstream_http_config(&cfg.upstream)); + run(cfg).await } @@ -1220,6 +1225,25 @@ fn load_heartbeat_config_from_disk( /// ). Cohere's `/v1/rerank` /// native surface is keyed off `Model.provider == "cohere"` in /// `crates/aisix-proxy/src/rerank.rs` and bypasses the Bridge. +/// Translate the `upstream:` config block into the gateway's client +/// settings. Every duration treats `0` as "leave this knob off". +fn upstream_http_config(cfg: &aisix_core::config::UpstreamConfig) -> UpstreamHttpConfig { + fn ms(v: u64) -> Option { + (v > 0).then(|| Duration::from_millis(v)) + } + fn secs(v: u64) -> Option { + (v > 0).then(|| Duration::from_secs(v)) + } + UpstreamHttpConfig { + connect_timeout: ms(cfg.connect_timeout_ms), + tcp_keepalive: secs(cfg.tcp_keepalive_secs), + tcp_keepalive_interval: secs(cfg.tcp_keepalive_interval_secs), + tcp_keepalive_retries: (cfg.tcp_keepalive_retries > 0).then_some(cfg.tcp_keepalive_retries), + pool_idle_timeout: secs(cfg.pool_idle_timeout_secs), + pool_max_idle_per_host: cfg.pool_max_idle_per_host, + } +} + fn build_hub() -> Hub { let hub = Hub::new();