feat(proxy, admin): runtime model status, cooldown, background health check, /admin/v1/models/status - #268
Merged
Merged
Conversation
…alth check, and /admin/v1/models/status - Add ModelRuntimeStatusTracker with cooldown/unhealthy/healthy states - Set cooldown on request-path retryable failures (30s TTL) - Add background_model_check config (direct-model only) for periodic health probes - Mark unhealthy on background check failure, ignore configured transient statuses (408/429) - Filter routing targets by runtime status (skip cooldown/unhealthy unless all would be excluded) - Add GET /admin/v1/models/status returning per-model runtime status with stale awareness - Add BackgroundModelCheck schema (direct-model only, routing models rejected) - Wire background checker in aisix-server with configurable interval
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Free Run ID: 📒 Files selected for processing (11)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughThis PR adds per-direct-model background health checks, a ModelRuntimeStatusTracker with cooldown and stale semantics, Retry-After propagation, health-aware routing/filtering that prefers healthy targets, a GET /admin/v1/models/status admin endpoint (with OpenAPI), server background task wiring, and extensive unit and end-to-end test coverage. ChangesBackground Model Health Checking and Runtime Status
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Note 🎁 Summarized by CodeRabbit FreeYour organization has reached its limit of developer seats under the Pro Plan. For new users, CodeRabbit will generate a high-level summary and a walkthrough for each pull request. For a comprehensive line-by-line review, please add seats to your subscription by visiting https://app.coderabbit.ai/login.If you believe this is a mistake and have available seats, please assign one to the pull request author through the subscription management page using the link above. Comment |
…st on all-filtered Addresses HIGH/MEDIUM findings from issue #264 audit on PR #268. HIGH: - H1: cooldown trigger is now a configurable status list (default [401, 408, 429, 500, 502, 503, 504]) independent of is_retryable. 401 / 404 / 408 cool down even though they are non-retryable: the same target will keep failing on subsequent requests, so the routing filter should skip it. - H2: upstream Retry-After header (seconds form) is parsed by the OpenAI / Anthropic bridges and threaded through BridgeError::UpstreamStatus.retry_after. The cooldown layer honors it (clamped by max_seconds, default 600s). - H3: filter_attempt_models returns an AllUnhealthy outcome when every candidate is excluded; the dispatch loop converts it into a new ProxyError::AllCandidatesUnavailable that maps to 503 + Retry-After. Routing models can opt into the legacy "send to known-bad" behavior via `on_all_filtered: original_order`. MEDIUM: - M1: 429 always cools down regardless of retry_on_429. Retry (same request) and cooldown (next request) are independent layers. - M3: background checks run under a 4-permit semaphore so 100 direct models cannot fan out 100 concurrent provider probes; schema enforces interval_seconds >= 5. New per-direct-model `cooldown` config block (all fields optional): enabled / default_seconds / max_seconds / honor_retry_after / trigger_statuses / trigger_on_timeout / trigger_on_transport. New per-routing-model `on_all_filtered` policy: `fail` (default) or `original_order`. E2E: - 4 baseline files (runtime-status, background-health, runtime-mixed-filtering, retry-on-429-vs-background-ignore) that were claimed in the original PR description but never committed. Now committed and updated for the new contract (status_reason names, interval_seconds=5 floor). - 1 new file `cooldown-contract-e2e.test.ts` with 5 contract tests: H1 401 cooldown, M1 429-cools-down-without-retry, H2 Retry-After-drives-TTL, H3 fail-fast 503, H3 original_order escape hatch. - 2 pre-existing tests (fallback-e2e, routing-strategies-e2e) fixed to avoid warming cooldown in their readiness probes — use bare waitConfigPropagation() so per-target hit counts measure the test request, not the probe. Rust unit coverage: 22 new chat.rs tests for decide_cooldown + filter_attempt_models (covers default policy, override, Retry-After clamping, all-filtered branching, on_all_filtered policy enforcement). cargo test --workspace: 686 pass / 0 fail cargo clippy --workspace --all-targets -- -D warnings: clean cargo fmt --all -- --check: clean e2e: 92 pass / 0 fail across 47 files
Collaborator
Author
Update: addressed audit findings from issue #264Force-pushed 6b109ed with the contract fixes the issue review flagged. Summary: HIGH
MEDIUM
New config surfaces# Direct model (all optional)
cooldown:
enabled: true # default
default_seconds: 30 # default
max_seconds: 600 # default
honor_retry_after: true # default
trigger_statuses: [401, 408, 429, 500, 502, 503, 504] # default
trigger_on_timeout: true # default
trigger_on_transport: true # default
# Routing model (optional)
on_all_filtered: fail # fail | original_order; default failTest coverage deltaE2E (real backend + mock upstreams, no stubbed bridges):
Rust unit coverage: 22 new chat.rs tests ( Checks
|
Independent audit on commit 6b109ed flagged these blockers: H-1: cooldown was wired only into chat.rs. Anthropic /v1/messages, OpenAI /v1/responses, audio/speech, audio/transcriptions, and /v1/rerank all silently dropped the cooldown signal. A 401 from Anthropic via /v1/messages would never take that target out of rotation. - Extract decide_cooldown + reason_for_status into crate::cooldown - Call from messages.rs (both passthrough + cross_provider_dispatch paths), responses.rs, audio.rs (speech + transcribe), rerank.rs H-2: routing-strategies-e2e readiness was weakened to bare waitConfigPropagation(), losing the verification that the virtual record had reached the DP. Restored by gating on admin.listModels() finding the virtual entry — proves propagation without sending dispatcher traffic that would warm cooldown. M-1: H2 e2e relied on undocumented serde-default SystemTime shape. Added wire-shape assertion so a future serialization change fails with a clear message instead of NaN. M-2: cooldown.default_seconds: 0 was schema-accepted but at runtime silently re-derived as max_seconds (default 600s) — the opposite of the operator's likely intent. Now treated as "disable cooldown on this model" with new unit test pinning the contract. M-5: OnAllFilteredPolicy::Fail rustdoc claimed Retry-After is "derived from the next cooldown expiry"; impl returns a fixed 30s fallback. Doc corrected to match the actual constant-fallback behavior with rationale. Drive-by: tests/e2e/src/harness/admin.ts listModels was decoding the response as `{items: [...]}` but the admin endpoint returns a bare array of ResourceEntry. The helper had no callers before now, so the bug was latent. cargo test --workspace: 689 pass / 0 fail (+3 for the new cooldown.rs unit tests). cargo clippy --workspace --all-targets -- -D warnings: clean. cargo fmt --all -- --check: clean. e2e: 92 pass / 0 fail across 47 files against real backend. Audit-flagged LOW items deferred: HTTP-date Retry-After parsing and CooldownConfig::default() docs. M-3 (BridgeError field addition) and M-4 (interval_seconds floor 1→5) are doc-only — call them out as breaking-on-config-reload in the PR body for upgrade notes.
Collaborator
Author
Audit response — addressed findings from independent reviewForce-pushed 2ea8a17 with the fixes for the independent audit that ran against the prior commit. HIGH — both addressed
MEDIUM — all addressed
Breaking-change call-outs (audit M-3 / M-4) — doc-onlyFor upgrade docs:
Drive-by fix
Audit-flagged LOW deferred
Checks
|
…hrough mark_healthy Round-2 independent audit on commit 2ea8a17 caught that the cooldown wiring on the five non-chat dispatchers was incomplete: only the `!status.is_success()` branch ran `decide_cooldown`. The `?`-escape paths on `.send().await` (transport / TCP reset / DNS) and on `.json()` / `.bytes()` decode skipped cooldown entirely. So a hard upstream outage on /v1/messages — exactly the H-1 failure mode the prior commit claimed to fix — would still leave the target healthy in routing. Round-2 HIGH: - Add `crate::cooldown::note_failure(tracker, model_id, cfg, err)` that runs the cooldown decision and marks the tracker, returning the BridgeError unchanged so call sites can keep using `?`. - Wire it into every `.map_err(|e| BridgeError::Transport(...))` and `.map_err(|e| BridgeError::UpstreamDecode(...))` on messages.rs (2 sites), responses.rs (2 sites), rerank.rs (2 sites), audio.rs (4 sites across multipart + speech). Round-2 MEDIUM: - messages.rs Anthropic-passthrough success path now also calls `state.runtime_status.mark_healthy(&model_entry.id)` so a target that recovers via /v1/messages exits `cooldown` cleanly on /admin/v1/models/status. Cross-provider sibling paths already did this; passthrough was the outlier. 3 new unit tests pin the contract: - `note_failure_marks_cooldown_for_transport_errors` - `note_failure_marks_cooldown_for_decode_errors` - `note_failure_no_op_when_cooldown_disabled` cargo test --workspace: 692 pass / 0 fail. cargo clippy --workspace --all-targets -- -D warnings: clean. cargo fmt --all -- --check: clean. e2e (real backend + mock upstreams): 92 pass / 0 fail across 47 files. Audit LOW (admin-snapshot readiness race window) deferred: practical gap is sub-100ms with 50ms poll cadence, no observed flakes.
Collaborator
Author
Audit round-2 responseRound-2 independent audit on commit 2ea8a17 caught a real gap: the H-1 fix wired cooldown into the Force-pushed 7c4dfb2 with: HIGH — Transport/Decode error cooldown
MEDIUM — Anthropic passthrough mark_healthy
Audit LOW deferred
New unit coverage
Checks
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds runtime model status tracking, request-path cooldown, background health probe (
background_model_check), and a new read-only endpointGET /admin/v1/models/status.Runtime status tracker (
ModelRuntimeStatusTracker)idhealthy,cooldown(request-path retryable failure),unhealthy(background check failure)stale_after_secondsRequest-path cooldown
Background model check
background_model_checkconfig block on direct models only (rejected on routing models)enabled,interval_seconds,timeout_seconds,prompt,max_tokens,ignore_statuses,stale_after_secondsignore_statuses(default [408, 429]) prevents transient failures from flipping unhealthyunhealthy; success -> clears unhealthy; ignored -> recorded as healthy with observability/admin/v1/models/statusnot_applicablelast_checked_at,last_check_status,status_reasonwhen availableImplementation notes
DeploymentKey,target,deployment)SystemTime(nottokio::time::Instant) for cross-thread snapshot consistencyaisix-server/src/main.rsTest coverage
HealthTracker,ModelRuntimeStatusTracker, background check logic, routing filteringGET /admin/v1/models/statusresponse shapebackground_model_checkschema validation (direct accepted, routing rejected)runtime-status-e2e.test.ts,background-health-e2e.test.ts,runtime-mixed-filtering-e2e.test.ts,retry-on-429-vs-background-ignore-e2e.test.tsSummary by CodeRabbit
New Features
Tests