From 0e699bbf1ea1dfe72df1ac3b20f11ab1d42f8c62 Mon Sep 17 00:00:00 2001 From: Jarvis Date: Wed, 29 Jul 2026 23:06:15 +0800 Subject: [PATCH] fix(health): /readyz must not withdraw an idle gateway MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit /readyz returned 503 once the config watch had gone 300s without an event. That reads as "the watch is wedged", but the signal is elapsed time since the last config *event*, and an environment whose resources are not changing produces none — which for most deployments is the steady state. A healthy gateway therefore reported itself unready five minutes after its environment went quiet. Under Kubernetes that is an outage, not a degraded signal: every replica crosses the threshold at the same moment, so the Service loses all of its endpoints. Observed on a real cluster — /livez 200, /status/config reporting the etcd watch connected, /readyz 503, pods 0/1, and the autoscaler adding replicas that could not become Ready either. Readiness is also the wrong lever for the case the threshold was aimed at. A genuinely wedged watch is a property of the config source and hits every replica at once, so withdrawing them all turns "serving the last accepted configuration" into a total outage with nowhere to shift traffic. Config freshness stays observable and alertable on /admin/v1/health, /status/config, and the aisix_config_* metrics — none of which are touched here. Readiness now gates on what it can actually answer: not draining, and configuration applied at least once. This matches LiteLLM, whose /health/readiness checks shutdown state and dependency reachability and has no event-recency condition. Both listeners share config_readiness_block, so the admin listener's /readyz is fixed with the proxy's; the /readyz OpenAPI descriptions are updated to match. Tests: an idle-environment case on the helper and a router-level case asserting /readyz stays 200 with a two-hour apply age. Both fail with the threshold restored. They sit at the unit and router level rather than in the e2e suite because the threshold is a compile-time constant — an end-to-end proof would have to idle for five real minutes. --- crates/aisix-admin/src/openapi.rs | 4 +-- crates/aisix-proxy/src/health.rs | 57 ++++++++++++++++++++++--------- crates/aisix-proxy/src/lib.rs | 26 ++++++++++++++ 3 files changed, 68 insertions(+), 19 deletions(-) diff --git a/crates/aisix-admin/src/openapi.rs b/crates/aisix-admin/src/openapi.rs index 061fe7e1..092379a4 100644 --- a/crates/aisix-admin/src/openapi.rs +++ b/crates/aisix-admin/src/openapi.rs @@ -113,7 +113,7 @@ const OPENAPI_JSON_BASE: &str = r##"{ } }, "503": { - "description": "Not ready: shutting down (drain), still starting up, or the config watch is stale", + "description": "Not ready: shutting down (drain), or no configuration applied yet", "content": { "text/plain": { "schema": { @@ -126,7 +126,7 @@ const OPENAPI_JSON_BASE: &str = r##"{ "tags": [ "Health" ], - "description": "Traffic eligibility (readiness): 200 when the instance can serve, 503 while draining, before the first config apply, or when the etcd config watch is stale. Distinct from /livez (process liveness)." + "description": "Traffic eligibility (readiness): 200 when the instance can serve, 503 while draining or before the first config apply. Stays 200 for as long as the instance keeps serving that configuration, however long ago the last config event was — see /admin/v1/health and /status/config for configuration freshness. Distinct from /livez (process liveness)." } }, "/admin/openapi.json": { diff --git a/crates/aisix-proxy/src/health.rs b/crates/aisix-proxy/src/health.rs index a46a1452..c763354e 100644 --- a/crates/aisix-proxy/src/health.rs +++ b/crates/aisix-proxy/src/health.rs @@ -59,21 +59,28 @@ impl LivezState { } } -/// A config snapshot older than this — or never applied — means the etcd -/// watch isn't delivering fresh config, so the instance shouldn't be -/// counted ready for traffic (#591). Matches the freshness threshold the -/// admin health aggregate uses. -pub const READYZ_STALE_AFTER: Duration = Duration::from_secs(300); - -/// Decide whether config freshness blocks readiness. `last_apply_age` is -/// the time since the config watch last applied an event: `None` means no -/// apply yet (still starting up / disconnected), `Some(age)` past the -/// stale threshold means a wedged watch. Returns `Some(reason)` when the -/// instance is not ready, `None` when config is fresh enough. +/// Decide whether config availability blocks readiness. `last_apply_age` is +/// the time since the config watch last applied an event; `None` means no +/// apply yet — still starting up, or restarted with no usable snapshot +/// cache. Returns `Some(reason)` when the instance cannot serve, `None` +/// when it can. +/// +/// The *age* is deliberately not an input. It measures time since the last +/// config **event**, and an environment whose resources are not changing +/// produces none — so a threshold on it reports a healthy gateway as +/// unready once the environment goes quiet, which for most deployments is +/// the steady state. `/readyz` previously blocked past 300s and took every +/// replica out of its Service five minutes after a deployment went idle. +/// +/// Nor is readiness the right lever for the case that threshold was aimed +/// at. A genuinely wedged watch is a property of the config source, so it +/// hits every replica at once: withdrawing them all converts "serving the +/// last accepted config" into a total outage, with no healthy instance to +/// shift traffic to. Config freshness stays observable — and alertable — +/// on `/status/config` and the `aisix_config_*` metrics. pub fn config_readiness_block(last_apply_age: Option) -> Option<&'static str> { match last_apply_age { None => Some("config not yet applied"), - Some(age) if age > READYZ_STALE_AFTER => Some("config watch is stale"), Some(_) => None, } } @@ -797,12 +804,28 @@ mod tests { fn config_readiness_block_logic() { // No apply yet → not ready (startup). assert!(config_readiness_block(None).is_some()); - // Fresh apply → ready. + // Applied → ready. assert!(config_readiness_block(Some(Duration::from_secs(5))).is_none()); - // Beyond the stale threshold → not ready (wedged watch). - assert!( - config_readiness_block(Some(READYZ_STALE_AFTER + Duration::from_secs(1))).is_some() - ); + } + + #[test] + fn an_idle_environment_stays_ready() { + // The age is time since the last config EVENT, and an environment + // whose resources are not changing produces none. Blocking on it + // took every replica out of its Service five minutes after the + // deployment went quiet, while the watch was healthy the whole + // time. Ready must not depend on how long ago the last event was. + for age in [ + Duration::from_secs(299), + Duration::from_secs(301), + Duration::from_secs(3600), + Duration::from_secs(86_400 * 7), + ] { + assert!( + config_readiness_block(Some(age)).is_none(), + "config applied {age:?} ago must still be ready", + ); + } } #[tokio::test] diff --git a/crates/aisix-proxy/src/lib.rs b/crates/aisix-proxy/src/lib.rs index c37e45b8..742a60cc 100644 --- a/crates/aisix-proxy/src/lib.rs +++ b/crates/aisix-proxy/src/lib.rs @@ -966,6 +966,32 @@ mod tests { assert!(text.contains("[-]config failed: not ready")); } + #[tokio::test] + async fn readyz_stays_200_when_no_config_event_has_arrived_in_hours() { + // A gateway whose environment is not changing receives no config + // events, so the apply age grows without bound while the gateway is + // perfectly healthy. Readiness must not read that as a fault: it + // used to 503 past five minutes, which emptied the Kubernetes + // Service of every replica once a deployment went idle. + let hub = Arc::new(Hub::new()); + let snap = seed_snapshot("my-gpt4", &["my-gpt4"], "http://unused"); + let state = build_state(snap, hub) + .with_config_apply_age(Arc::new(|| Some(std::time::Duration::from_secs(7200)))); + let app = build_router(state); + + let req = Request::builder() + .method("GET") + .uri("/readyz?verbose") + .body(Body::empty()) + .unwrap(); + + let resp = run(app, req).await; + assert_eq!(resp.status(), StatusCode::OK); + let bytes = to_bytes(resp.into_body(), 1024).await.unwrap(); + let text = std::str::from_utf8(&bytes).unwrap(); + assert!(text.contains("[+]config ok")); + } + #[tokio::test] async fn health_route_is_not_found() { let hub = Arc::new(Hub::new());