Uh oh!
There was an error while loading. Please reload this page.
feat(server): startup sequence + health listeners - #5
Conversation
Wires the 10-step startup sequence from spec §1 into the aisix binary: 1. Parse CLI args (--config, with AISIX_CONFIG env fallback) 2. Load + validate bootstrap config 3. init_tracing — aisix-obs now installs an EnvFilter-backed subscriber 4. EtcdConfigProvider::connect (5s × 5 retry, reused from PR #3) 5. Initial snapshot load via Supervisor::load_once 6. Spawn Supervisor::run on a dedicated task (cancel via watch channel) 7. Build proxy router (aisix-proxy now exposes build_router + ProxyState) 8. Build admin router (aisix-admin now exposes build_router + AdminState) 9. Bind + serve both listeners with axum::serve + graceful shutdown 10. SIGINT/SIGTERM → flip cancel channel → drain both serves → join supervisor For now both routers only mount /health so the startup wiring is observable without reaching into feature code that hasn't landed yet. The health handler reports the current snapshot table sizes so the bootstrap path is end-to-end verifiable. Ancillary changes: - ObsError with Filter + AlreadyInitialised variants (thiserror) - aisix-obs drops #![forbid(unsafe_code)] so test code can use the now-unsafe std::env::remove_var without a lint override - ProxyState / AdminState are Clone and cheap (Arc-backed handles and Arc<[String]> admin keys) 6 new unit tests (CLI parsing, ObsError display, proxy health JSON shape, admin health JSON shape, admin-keys Arc sharing) land on top of the existing 68 — 75 passing workspace-wide.
There was a problem hiding this comment.
Pull request overview
Implements the spec §1 startup sequence wiring in the aisix server binary, introducing minimal /health endpoints for both proxy and admin listeners so bootstrap + snapshot plumbing can be exercised end-to-end.
Changes:
- Adds an async
aisix-serverstartup flow: CLI config path, config load, tracing init, etcd supervisor spawn, and dual Axum listeners with graceful shutdown. - Exposes
build_router+ state types inaisix-proxyandaisix-admin, each mounting a basic/healthhandler reporting snapshot table sizes. - Introduces
aisix-obs::init_tracingto install atracing_subscriberwith anEnvFilterderived from config and/orRUST_LOG.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/aisix-server/src/main.rs | Implements the server startup orchestration, supervisor wiring, listener binding/serving, and shutdown coordination. |
| crates/aisix-proxy/src/lib.rs | Adds ProxyState and build_router() with a /health endpoint and tests. |
| crates/aisix-proxy/Cargo.toml | Adds tokio dev-dependency features needed for async tests. |
| crates/aisix-obs/src/lib.rs | Adds init_tracing() and error types for observability bootstrap. |
| crates/aisix-admin/src/lib.rs | Adds AdminState and build_router() with a /health endpoint and tests. |
| crates/aisix-admin/Cargo.toml | Adds tokio dev-dependency features needed for async tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let filter = EnvFilter::try_from_default_env() | ||
| .or_else(|_| EnvFilter::try_new(&cfg.log_level)) | ||
| .map_err(|source| ObsError::Filter { | ||
| directive: cfg.log_level.clone(), | ||
| source, | ||
| })?; |
There was a problem hiding this comment.
EnvFilter::try_from_default_env().or_else(|_| ...) also falls back when RUST_LOG is set but invalid, silently ignoring the operator override. If RUST_LOG is present but can’t be parsed, it’s usually better to return an error that references the invalid RUST_LOG value (and only fall back to cfg.log_level when the env var is absent).
| let signal_task = tokio::spawn(wait_for_signal(cancel_tx.clone())); | ||
| let (proxy_res, admin_res) = tokio::join!(proxy_serve, admin_serve); | ||
| proxy_res.map_err(|e| anyhow::anyhow!("proxy serve error: {e}"))?; | ||
| admin_res.map_err(|e| anyhow::anyhow!("admin serve error: {e}"))?; | ||
| // Ask the supervisor to stop (no-op if the signal task already did). | ||
| let _ = cancel_tx.send(true); | ||
| let _ = signal_task.await; | ||
| let _ = watch_task.await; |
There was a problem hiding this comment.
tokio::join!(proxy_serve, admin_serve) can deadlock: if either server future returns early (e.g. bind/serve error) while the other is still running, join! will wait forever and the cancellation cancel_tx.send(true) is never reached. Use tokio::select! (or spawn both serves and select on their JoinHandles) to trigger cancellation as soon as any of (signal, proxy_serve, admin_serve) completes, then gracefully shut down the remaining tasks.
| let signal_task = tokio::spawn(wait_for_signal(cancel_tx.clone())); | |
| let(proxy_res, admin_res) = tokio::join!(proxy_serve, admin_serve); | |
| proxy_res.map_err(|e| anyhow::anyhow!("proxy serve error: {e}"))?; | |
| admin_res.map_err(|e| anyhow::anyhow!("admin serve error: {e}"))?; | |
| // Ask the supervisor to stop (no-op if the signal task already did). | |
| let _ = cancel_tx.send(true); | |
| let _ = signal_task.await; | |
| let _ = watch_task.await; | |
| letmut signal_task = tokio::spawn(wait_for_signal(cancel_tx.clone())); | |
| letmut proxy_task = tokio::spawn(proxy_serve); | |
| letmut admin_task = tokio::spawn(admin_serve); | |
| let first_result: anyhow::Result<()> = tokio::select! { | |
| res = &mut signal_task => { | |
| res.map_err(|e| anyhow::anyhow!("signal task join error: {e}"))?; | |
| Ok(()) | |
| } | |
| res = &mut proxy_task => { | |
| res.map_err(|e| anyhow::anyhow!("proxy task join error: {e}"))? | |
| .map_err(|e| anyhow::anyhow!("proxy serve error: {e}")) | |
| } | |
| res = &mut admin_task => { | |
| res.map_err(|e| anyhow::anyhow!("admin task join error: {e}"))? | |
| .map_err(|e| anyhow::anyhow!("admin serve error: {e}")) | |
| } | |
| }; | |
| // Ask the supervisor and remaining servers to stop. | |
| let _ = cancel_tx.send(true); | |
| // The signal task may still be waiting for SIGINT/SIGTERM if a server | |
| // completed first, so do not wait forever on it. | |
| if !signal_task.is_finished(){ | |
| signal_task.abort(); | |
| } | |
| let _ = signal_task.await; | |
| if !proxy_task.is_finished(){ | |
| let proxy_res = proxy_task | |
| .await | |
| .map_err(|e| anyhow::anyhow!("proxy task join error: {e}"))?; | |
| proxy_res.map_err(|e| anyhow::anyhow!("proxy serve error: {e}"))?; | |
| } | |
| if !admin_task.is_finished(){ | |
| let admin_res = admin_task | |
| .await | |
| .map_err(|e| anyhow::anyhow!("admin task join error: {e}"))?; | |
| admin_res.map_err(|e| anyhow::anyhow!("admin serve error: {e}"))?; | |
| } | |
| let _ = watch_task.await; | |
| first_result?; |
| // Ask the supervisor to stop (no-op if the signal task already did). | ||
| let _ = cancel_tx.send(true); | ||
| let _ = signal_task.await; |
There was a problem hiding this comment.
signal_task.await can hang indefinitely when shutdown is triggered by something other than SIGINT/SIGTERM (e.g. one of the serve futures errors/exits). wait_for_signal doesn’t observe the cancel channel, so it never returns in that scenario. Consider either (1) passing a watch::Receiver<bool> into wait_for_signal and select!ing between cancel and OS signals, or (2) aborting/dropping signal_task once cancellation is triggered.
| let _ = signal_task.await; | |
| // If shutdown was triggered by something other than SIGINT/SIGTERM, | |
| // the signal task may still be blocked waiting on OS signals. | |
| // Abort it so this join cannot hang indefinitely. | |
| signal_task.abort(); | |
| match signal_task.await{ | |
| Ok(()) => {} | |
| Err(err)if err.is_cancelled() => {} | |
| Err(err) => returnErr(anyhow::anyhow!("signal task join error: {err}")), | |
| } |
| let supervisor = Arc::new(Supervisor::new(provider, cfg.etcd.prefix.clone())); | ||
| let snapshot_handle = supervisor.handle(); | ||
| let (cancel_tx, cancel_rx) = watch::channel(false); | ||
| let watch_task = tokio::spawn(supervisor.clone().run(cancel_rx.clone())); | ||
| // Steps 7-8: routers. | ||
| let proxy_router = | ||
| aisix_proxy::build_router(ProxyState::new(snapshot_handle.clone(), &cfg.proxy)); | ||
| let admin_router = |
There was a problem hiding this comment.
The startup sequence comment and PR description call for bootstrapping an initial snapshot before serving, but the code starts serving immediately after spawning Supervisor::run. Since Supervisor::run performs the first load_all asynchronously, /health (and future proxy routing) can observe an empty snapshot during startup. Call supervisor.load_once().await? (or otherwise await the first successful load) before binding/serving.
| let state = AdminState::new(handle, &cfg()); | ||
| let b = state.clone(); | ||
| assert_eq!(b.admin_keys.len(), 1); | ||
| assert_eq!(&*b.admin_keys[0], "k1"); |
There was a problem hiding this comment.
This assertion does not compile: b.admin_keys[0] is an &String, so &*b.admin_keys[0] attempts to move out of a borrow. Compare using b.admin_keys[0].as_str() (or &b.admin_keys[0] with an appropriate RHS) instead.
| assert_eq!(&*b.admin_keys[0],"k1"); | |
| assert_eq!(b.admin_keys[0].as_str(),"k1"); |
| //! PRs so this crate stays focused. | ||
| #![forbid(unsafe_code)] | ||
| #![deny(rust_2018_idioms)] |
There was a problem hiding this comment.
This crate drops #![forbid(unsafe_code)] while the rest of the workspace consistently forbids unsafe (e.g. aisix-core, aisix-etcd, aisix-admin, aisix-proxy). If that wasn’t intentional, re-add #![forbid(unsafe_code)] to keep the workspace policy consistent.
| #![deny(rust_2018_idioms)] | |
| #![deny(rust_2018_idioms)] | |
| #![forbid(unsafe_code)] |
…ped reads, redact 5xx message, Vertex content-type guard Five concrete fixes from the Copilot inline review on PR #323. Two stale comments (#3, #4 — already fixed in commit 3) are skipped. **#1+#7 — Azure OpenAI-compatible code preservation.** Azure's envelope omits `error.type` and carries only `error.code`. The bridge previously put the upstream code into `view.kind` and left `view.code` as `None`. For OpenAI-compat tokens Azure inherits unchanged (e.g. `rate_limit_exceeded`), this meant downstream OpenAI clients received `error.type=rate_limit_exceeded` but `error.code=null` — exactly the SDK-retry break issue #322 is about. Fix: - Azure parser populates BOTH `view.kind` AND `view.code` from the upstream `error.code` field. - `render_openai_envelope`'s AzureOpenAI branch now prefers the translation-table-derived code (so explicit Azure tokens like `DeploymentNotFound` → `model_not_found` still win), falling back to `view.code` for OpenAI-compat pass-through. **#2 — Drain the response stream after hitting the cap.** `read_body_capped` previously broke out of the read loop the moment `limit` bytes were buffered. With reqwest/hyper that leaves unread bytes in the response and prevents connection reuse — during a burst of upstream errors the gateway would churn TCP connections instead of recycling the keep-alive pool. Fix: keep iterating the stream, discarding chunks past the cap. Memory stays bounded by `limit`. **#5 — Redact upstream `error.message` on 5xx.** The 5xx branch of `render_bridge_upstream_envelope` was forwarding `BridgeError::UpstreamStatus.message` verbatim — which for OpenAI / Anthropic comes from the parsed upstream `error.message`. Upstream 5xx bodies routinely embed operator-internal detail (engine names, shard ids, queue depth). Fix: on 5xx, emit a canned `"upstream returned {status}"` message; the full upstream body remains in operator logs via tracing. **#6 — Stale "follow-up" comment.** The docstring on `render_bridge_upstream_envelope` claimed cross-wire translation would ship in a follow-up, but it already shipped in commit 2. Rewrite the comment to describe current behaviour (4xx → `error_translate`; 5xx → canned envelope; `Unknown` wire → legacy generic envelope). **#8 — Content-type guard on Vertex (and Azure, while at it).** `capture_upstream_error_http` already gates serde parsing on `Content-Type: application/json` so a 64 KB HTML error page from a fronting WAF doesn't waste CPU on a doomed JSON parse. The Vertex and Azure bridges call serde directly because they need a custom parse path (canned message for redaction) — same guard now applies. Promoted `content_type_is_json` and added a `response_is_json` helper to the gateway's public surface; both bridges call it before `parse_*_error_*`. New tests: - `upstream_openai_5xx_with_json_envelope_collapses_and_redacts_message` pins the 5xx redaction (asserts `engine offline` / `shard 47` / `engine_overloaded` don't reach the customer envelope). - `chat_429_preserves_openai_compatible_code_for_sdk_retry` (Azure) pins that `parsed.code` carries the OpenAI-compat upstream code. - `chat_400_non_json_body_skips_envelope_parse` (Azure) and `chat_gemini_non_json_body_skips_envelope_parse` (Vertex) pin the new content-type guard. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Summary
Wires the 10-step startup sequence from spec §1 into the `aisix`
binary:
Both routers only mount `/health` for now so the bootstrap is
observable without dragging in feature code that hasn't landed yet.
The health handler reports current snapshot table sizes so the full
path is end-to-end verifiable.
Test plan