feat(downstream): configurable idle timeout and SSE heartbeat on the inbound side - #818
Conversation
…inbound side `upstream:` (#808) bounded the connections the gateway dials out with. The inbound half had nothing: an accepted connection was held open forever once idle, and only `/v1/chat/completions` sent an SSE heartbeat, on a hardcoded 15s interval. New `downstream:` block, the mirror of `upstream:`: - `idle_timeout_secs` closes a connection that sits idle between requests. Applied through hyper's header-read timer, which arms only while waiting for a request head — and hyper only waits for one after the previous response is fully written — so a slow model or a long SSE stream is never interrupted. hyper's own 30s default was silently dropped because neither axum nor axum_server installs a timer, so the plain-HTTP listener moves from `axum::serve` to `axum_server` (which exposes the connection builder; the TLS listener already used it). HTTP/1.1 only — hyper has no h2 equivalent. Defaults to 0 (never close), i.e. today's behaviour. A node in front pools its own connections — Envoy's upstream idle default is an hour — and closing first is what hands *it* a connection it still considers usable, the same failure this gateway avoids upstream by keeping `pool_idle_timeout_secs` low. Set it above the pool idle timeout of whatever sits in front. - `sse_keepalive_interval_secs` (default 15, 0 disables) now drives the heartbeat on every SSE surface, not just chat: bridged `/v1/messages`, bridged and passthrough `/v1/responses`. A model slow to its first token no longer looks like an abandoned connection to a proxy in front. Also finishes what #808 started: the guardrail (7), MCP OAuth, A2A, telemetry, heartbeat, and OTLP-exporter clients were still on reqwest's defaults — no connect timeout, TCP keepalive off, 90s pooled-connection lifetime. They all build from `aisix_gateway::client_builder()` now, and a test walks the workspace to keep the next one from drifting. Not implemented, deliberately: per-provider overrides of the pool settings (the pool idle timeout only needs to be below the *shortest* hop on the path, so one global value suffices; per-provider pools multiply FDs and memory for no reachability gain), `max_requests_per_connection` / `max_connection_lifetime` (reqwest exposes neither), and a separate upstream stream-idle timeout (per-model `stream_timeout` already bounds each inter-chunk gap). Refs AISIX-Cloud#1126
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 4 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 ignored due to path filters (1)
📒 Files selected for processing (31)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
…in() `run` is the entry the integration tests drive directly, so reading `downstream.sse_keepalive_interval_secs` in `main` left those paths on the default while `idle_timeout_secs` — read inside `run` — was honoured. Both now come from the same place.
…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.
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai full review |
Action performedFull review triggered. |
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. Your recent PR review activity is in the 95th percentile or higher among CodeRabbit users, so adaptive limits apply. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 47 minutes. |
|
@coderabbitai full review |
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. Your recent PR review activity is in the 95th percentile or higher among CodeRabbit users, so adaptive limits apply. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 8 minutes. |
1 participant
Problem
upstream:(#808) bounded the connections the gateway dials out with. The inbound half had nothing:header_read_timeoutto 30s, butTime::checksilently drops a defaulted duration when no timer is installed — and neitheraxum::servenoraxum_serverinstalls one. So an accepted connection was held open forever once it went idle./v1/chat/completions, at a hardcoded 15s./v1/messagesand/v1/responsesstreams sent nothing while the model was silent.client_builder(); the 7 guardrail clients, MCP OAuth, A2A, telemetry, heartbeat and the OTLP exporter were still on reqwest's defaults (no connect timeout, TCP keepalive off, 90s pooled-connection lifetime).Implementation
New
downstream:block, the mirror ofupstream::idle_timeout_secsgoes on hyper's header-read timer. That timer arms only while hyper is waiting for a request head, and hyper only waits for one once the previous response has been fully written (Conn::can_read_headneeds the read half back atInit, whichtry_keep_alivereaches only when reading and writing are done). So it covers exactly the between-requests window — a slow model or a long SSE stream is never interrupted. Reaching the builder means the plain-HTTP listener moves fromaxum::servetoaxum_server, which the TLS listener already used;serve_connection_with_upgradesandConnectInfo<SocketAddr>are preserved, so WebSocket upgrades and real-ip resolution are unaffected. HTTP/1.1 only — hyper has no h2 equivalent.Default
0= today's behaviour. Closing first is what hands the node in front a connection it still considers usable — the same failure this gateway avoids upstream by keepingpool_idle_timeout_secslow — and a fronting Envoy pools for an hour by default. Set it above the pool idle timeout of whatever sits in front.sse_keepalive_interval_secs(default 15,0disables) now drives every SSE surface: axum'sSsekeep-alive on chat, and awith_heartbeatstream wrapper on bridged/v1/messages, bridged/v1/responses, and the/v1/responsesbyte passthrough. The frame is:\n\n— the same comment axum writes, ignored by every conforming SSE parser. Not applied to opaque binary passthroughs (audio, images), where it would corrupt the body.Uniform connection layer: guardrails, MCP OAuth, A2A, telemetry, heartbeat and the OTLP exporter now build from
aisix_gateway::client_builder(). A test walks every crate's production source and fails on a barereqwest::Client::builder()/::new(), so the next client can't drift back (verified it catches a reintroduced offender).Which surfaces the heartbeat covers
Audited every response-body outlet in
aisix-proxy, since the issue asks whether the raw passthrough paths need one too:/v1/chat/completions(both stream sites)Sse/v1/messagesbridged + native passthrough/v1/responsesbridged + native passthrough/passthrough/:provider/*restbytes().await) for the output-guardrail scan/v1/completionsJson(...)/v1/realtimestream_timeout) and would need WS ping frames, not SSE commentsWorth flagging from that audit: the raw tunnel buffers, so a slow provider call through
/passthrough/...puts zero bytes on the client connection until the whole body has arrived — the same "looks idle" exposure the heartbeat fixes for SSE, and one that can't be fixed the same way because the content type is unknown. Out of scope here; noting it so it isn't assumed covered.Not implemented, deliberately
max_requests_per_connection/max_connection_lifetime. reqwest exposes neither.stream_timeoutalready bounds each inter-chunk gap (/v1/completions: add output guardrails on generated choices[].text #554, corrected for Azure in fix(azure): apply stream_timeout per chunk, not as a whole-response ceiling #809); this would be a second name for it.Behaviour changes
/v1/messagesand/v1/responsesnow carry heartbeat comments during upstream silence.Tests
downstream-connection-e2e.test.ts: an idle keep-alive connection is closed at the deadline; a request in flight past the deadline still completes; heartbeats appear on all three SSE code paths while the upstream is silent, without disturbing the payload. Mutation-checked — with the idle timeout un-applied and the interval hardcoded, 4 of the 5 fail (the in-flight case is the control and keeps passing).downstream:config defaults/overrides.upstream-pool-idle-e2e.test.ts:pool_idle_timeout_secsshipped in fix(upstream): name the real transport fault and bound the connection layer #808 without behavioural coverage. Against an upstream that never closes first, a request after the deadline opens a new TCP connection; the same gap with the knob off reuses the pooled one. The two cases are each other's control.#554timeout fallback, audio timeout, streaming usage, guardrails.Fixes api7/AISIX-Cloud#1126
Summary by CodeRabbit
New Features
Tests