fix(upstream): name the real transport fault and bound the connection layer - #808
Conversation
… layer
Two connection-layer gaps behind the intermittent upstream failures in
AISIX-Cloud#1122.
**Transport errors were undiagnosable.** Every bridge built its error as
`BridgeError::Transport(e.to_string())`, and reqwest's top-level Display
is only ever "error sending request for url (…)" — the identical string
for a DNS failure, a refused connection, a TLS handshake error, and a
pooled connection the far end already closed. The `source()` chain that
names the actual fault was dropped on the floor:
before: error sending request for url (http://host/v1/chat/completions)
after: error sending request for url (http://host/v1/chat/completions):
client error (Connect): tcp connect error: Connection refused (os error 111)
after: … : client error (Connect): dns error: failed to lookup address
information: Name or service not known
`aisix_gateway::transport_error_message` flattens the chain into the
message, deduplicating the tail cause hyper restates at several levels.
Credential-bearing query parameters are redacted first — Vertex/Gemini
accept `?key=` and `?access_token=`, and reqwest embeds the full URL in
the message, so the pre-existing single-line form could already echo a
live key into a log.
**Connection settings were reqwest's defaults.** No connect timeout, TCP
keepalive off, and a 90s pooled-connection lifetime that outlives the
idle timeout of a typical LB / NAT / proxy hop — so a connection reaped
upstream can still be handed out here and fails the request. All bridges
plus the proxy's shared client now build through
`aisix_gateway::client_builder()`, driven by a new `upstream:` config
block. Defaults follow LiteLLM's reasoning for the same problem (it sets
SO_KEEPALIVE explicitly, citing NAT idle timers reaping connections
while a slow model is still producing): connect 5s, keepalive 60s /
interval 30s / 5 retries, pool idle 30s. `0` disables any single knob.
Tests: cause-chain flattening and dedup, credential redaction (including
case-insensitive), config defaults + per-knob overrides, and a real
`reqwest::Error` against a closed port asserting the top-level Display
hides the cause while the rendered message names it.
Fixes api7/AISIX-Cloud#1122
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 32 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (15)
📝 WalkthroughWalkthroughChangesUpstream HTTP configuration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Config
participant Server
participant GatewayHTTP
participant ProviderBridge
participant Reqwest
Config->>Server: load cfg.upstream
Server->>GatewayHTTP: initialize UpstreamHttpConfig
ProviderBridge->>GatewayHTTP: request client_builder()
GatewayHTTP->>Reqwest: apply timeout, keepalive, and pool settings
Reqwest-->>ProviderBridge: response or transport error
ProviderBridge->>GatewayHTTP: format transport error with causes
Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/aisix-gateway/src/upstream_http.rs`:
- Around line 299-307: Update real_transport_error_names_the_root_cause to bind
a loopback TcpListener on an ephemeral port, capture its assigned address, drop
the listener, and construct the request URL from that address before sending.
Preserve the assertion that the resulting connection attempt fails without
relying on a fixed port.
- Around line 25-35: Extend SENSITIVE_QUERY_PARAMS with the requested credential
aliases, including client_secret, api-key, and x-amz-signature plus other common
hyphenated/vendor variants, so redact_url removes them from api_base URLs. Add
regression cases covering each newly supported alias and verify provider
transport errors do not retain their values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fcb3e137-efcb-4cd4-9319-f46c6adb5fa2
📒 Files selected for processing (16)
config.example.yamlconfig.managed.yamlcrates/aisix-core/src/config.rscrates/aisix-gateway/src/lib.rscrates/aisix-gateway/src/upstream_http.rscrates/aisix-provider-anthropic/src/bridge.rscrates/aisix-provider-azure-openai/src/bridge.rscrates/aisix-provider-openai/src/bridge.rscrates/aisix-provider-vertex/src/bridge.rscrates/aisix-proxy/src/audio.rscrates/aisix-proxy/src/dispatch.rscrates/aisix-proxy/src/http_client.rscrates/aisix-proxy/src/jobs.rscrates/aisix-proxy/src/passthrough.rscrates/aisix-proxy/src/realtime.rscrates/aisix-server/src/main.rs
CodeRabbit review on #808: the redaction denylist missed `client_secret`, `api-key`, and `x-amz-signature`. Enumerating exact names loses that race — SigV4 alone contributes `X-Amz-Signature`, `X-Amz-Credential`, and `X-Amz-Security-Token`, and every vendor spells the same concept differently. Match instead on a suffix of the parameter name after lowercasing and stripping `-`/`_`, so `api-key` / `api_key` / `apiKey` all resolve through `key`, `client_secret` through `secret`, and the SigV4 trio through `signature` / `credential` / `token`. Over-redacting an unrelated parameter costs a little diagnostic detail; under-redacting puts a live key in a log store, so the asymmetry favours the broader match. `api-version`, `alt`, `keyword`, and `signature_version` are covered by a test to pin that the diagnostic parameters stay readable. Also drops the assumption that port 1 is closed in the transport-error test (same review): bind an ephemeral loopback port and release it, so the connect is refused without depending on a fixed port being free.
The Transport arm of `reqwest_error_to_bridge` now renders its cause
chain, but the Timeout arm next to it did not — and `is_timeout()` is
satisfied by three unrelated conditions:
- hyper's request-budget timeout (the configured `timeout`),
- an expired `connect_timeout`, which this PR itself introduces,
- the kernel's `ETIMEDOUT` on an unanswered SYN (`io::ErrorKind::TimedOut`).
All three collapsed into the same `upstream request timed out after
{elapsed_ms}ms`, so an operator could not tell a slow upstream from one
that was never reached — the open question in AISIX-Cloud#1093. Adding
`connect_timeout` without this would have made that worse by folding a
fourth cause into the same sentence.
`BridgeError::Timeout` gains a `cause` field, empty for the gateway's own
deadlines (message byte-identical to before) and populated from the
reqwest chain when the timeout came from the connection layer:
upstream request timed out after 5002ms: error sending request for url
(http://10.1.2.3:8080/v1/messages): client error (Connect):
tcp connect error: deadline has elapsed
which is distinguishable from the kernel's `Connection timed out (os
error 110)` and from a budget expiry, where no connect-layer cause
appears at all.
The cause names the upstream host, so `envelope()` keeps it out of the
caller's response and leaves the bare sentence there — the same split
`render_bridge_upstream_envelope` already applies to upstream 5xx bodies.
Transport is deliberately left as-is: it has always surfaced the request
URL to the caller, so no new topology is exposed there either.
Verified end-to-end against a black-holed upstream: the caller receives
`upstream request timed out after 5002ms`, while the WARN log and the
per-attempt telemetry carry the full chain. The 5002ms is itself the new
`connect_timeout` doing its job — without it the same request rode the
kernel's SYN retry schedule (7s / 11s / 127s depending on
`tcp_syn_retries`).
|
Follow-up in b631d2e — a gap this PR was itself widening.
if err.is::<TimedOut>() { return true; } // connect_timeout — introduced by THIS PR
if hyper_err.is_timeout() { return true; } // the configured request budget
if io.kind() == io::ErrorKind::TimedOut { ... } // kernel ETIMEDOUT on an unanswered SYNAll three rendered as the same
Exposure boundary. The cause names the upstream host, so Worth noting for #1093 separately: the 5002ms above is the new Credit: gap identified by a parallel review pass on #1093. |
Conflict in the Azure per-chunk timeout, where #808 landed `BridgeError::Timeout`'s new `cause` field on the same lines this branch rewrote. Resolved by keeping both: this branch's `d.as_millis()` (the per-chunk gap budget — the whole point of the change, and what `with_read_timeout` already reports) plus `cause: String::new()`, since an elapsed gateway-owned deadline has no transport-layer cause to name.
…led connections The knob shipped in #808 without behavioural coverage. The transport failure it prevents only reproduces deterministically when the far end vanishes silently (hyper discards a connection that sent a clean FIN before reusing it), but the mechanism underneath is testable: against an upstream that never closes first, a request after the deadline must open a new TCP connection. The two cases are each other's control — same gap, knob on vs off (0 = reqwest's 90s default) — so a regression that stops applying the pool config flips the first assertion from 2 connections to 1.
1 participant
Two connection-layer gaps behind the intermittent upstream failures reported in AISIX-Cloud#1122.
1. Transport errors were undiagnosable
Every bridge built its error as
BridgeError::Transport(e.to_string()). reqwest's top-levelDisplayis only evererror sending request for url (…)— the identical string for a DNS failure, a refused connection, a TLS handshake error, and a pooled connection the far end already closed. Thesource()chain that names the actual fault was dropped.Measured against a real
reqwest::Error:aisix_gateway::transport_error_messageflattens the chain into the message and drops the tail cause hyper restates at several levels. Applied at all 21Transport(...)construction sites across the openai / azure / anthropic / vertex bridges and the proxy's passthrough, audio, jobs, realtime, and shareddispatchhelper.Incidental credential fix
reqwest embeds the full URL in that message, and Vertex/Gemini accept
?key=/?access_token=— which an operator can put straight into a ProviderKeyapi_base. The pre-existing single-line form could therefore echo a live key into a log. Credential-bearing query parameters are now redacted before the message is built; non-credential params (api-version,alt, …) stay readable because they're the diagnostic part.2. Connection settings were reqwest's defaults
http_client.rsand all four providerdefault_client()s were a bareClient::builder().user_agent(...). That means no connect timeout, TCP keepalive off, and a 90s pooled-connection lifetime — longer than the idle timeout of a typical LB / NAT gateway / corporate proxy / service mesh hop. A connection reaped upstream is still handed out by the pool, and the request fails with exactly the opaque transport error above.All of them now build through
aisix_gateway::client_builder(), driven by a newupstream:config block:connect_timeout_mstcp_keepalive_secstcp_keepalive_interval_secstcp_keepalive_retriespool_idle_timeout_secspool_max_idle_per_host0disables any single knob. Settings are process-wide and installed at boot before any bridge builds its client, because the pools can't be reconfigured afterwards.Baseline
LiteLLM solves the same problem the same way — it sets
SO_KEEPALIVEexplicitly on its upstream sockets, with the stated reason that NAT/LB hops reap the flow before a slow provider response arrives, and uses a 5s connect timeout withTCP_KEEPIDLE=60/KEEPINTVL=30/KEEPCNT=5. Our defaults match those; the pool-idle knob is ours (httpx expires idle connections after 5s by default, reqwest after 90s — 30s sits between them).Scope note
Guardrail dispatchers (lakera, presidio, aliyun, …) build their own clients and are untouched here: they call guardrail services, not the LLM upstreams this issue is about. Same hardening applies to them and is worth a follow-up.
Tests
upstream:block, and per-knob overrides with0round-tripping;reqwest::Erroragainst a closed port asserting reqwest's ownDisplayhides the cause while the rendered message names it.Full workspace suite passes.
Fixes api7/AISIX-Cloud#1122
Summary by CodeRabbit
New Features
Bug Fixes