feat(observability): serve per-model runtime health on the status listener#778
Merged
Conversation
…tener GET /status/models on the dedicated metrics/status listener serves the per-model runtime health view (cooldown / background-check state) as an operational read next to /status/config and /status/ready, so day-2 reads no longer depend on the admin listener (which managed mode does not bind at all). The response is exactly the admin listener's GET /admin/v1/models/status body: one shared render (render_models_status) behind both endpoints, reading through the same ConfigStore handle and the same shared runtime status tracker, so the two responses cannot drift while both exist. The admin endpoint is unchanged. Unauthenticated like the rest of the listener (model names + health states, the same sensitivity class as /status/config's resource_counts and rejected[] ids); restrict at the network layer. Modes: etcd and file standalone share the admin store handle; managed mode serves the applied snapshot through the same read-only view file mode uses, keeping the metrics/status listener identical across modes. e2e: AdminClient.listModelStatuses() now reads GET /status/models on the metrics listener (method signature/return unchanged), so the existing runtime-status consumers exercise the new endpoint end-to-end; a new status-models-e2e case pins deep-equal AND raw-byte equality against the admin endpoint with a model in real 401-tripped cooldown, the auth split, and file-mode serving. A unit test pins the two bodies byte-for-byte over the same store.
📝 WalkthroughWalkthroughThe dedicated metrics/status listener now exposes ChangesModel Status Listener
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant MetricsListener
participant ConfigStore
participant RuntimeTracker
participant ModelStatusRenderer
Client->>MetricsListener: GET /status/models
MetricsListener->>ConfigStore: List configured models
ConfigStore-->>MetricsListener: Model entries
MetricsListener->>RuntimeTracker: Read optional runtime state
RuntimeTracker-->>MetricsListener: Runtime snapshots
MetricsListener->>ModelStatusRenderer: Render model status views
ModelStatusRenderer-->>MetricsListener: JSON model status list
MetricsListener-->>Client: Status response
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…d status listener
GET /status/models is served without auth, so a store backend failure
(e.g. an etcd outage) must not echo client error detail — endpoints,
connection errors — to anonymous callers the way the admin-key-gated
endpoint may. Answer a fixed {"error_msg": "failed to list models"}
500 and keep the detail in the server log; success bodies remain
byte-identical to the admin endpoint. A unit test pins the redaction
with a failing store.
Collaborator
Author
|
Review triage
|
Collaborator
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Collaborator
Author
|
@coderabbitai full review |
✅ Action performedFull review finished. Your plan includes PR reviews subject to rate limits. More reviews will be available in 29 minutes. |
Collaborator
Author
|
@coderabbitai summary |
✅ Action performedSummary regeneration triggered. |
A stalled configuration store (blackholed etcd) must not park anonymous status pollers indefinitely, or hold the shared store client while they pile up: the unauthenticated endpoint is built for probes and dashboards to poll, and its contract is that store failures answer a fixed 500. Wrap the store read in a 5s timeout that degrades into that same fixed answer, keeping backend detail in the server log. Pinned by a paused-clock unit test with a never-resolving store.
Collaborator
Author
|
Independent cold audit (second pass, at
All four earlier triage dispositions were independently re-verified as sound (redaction covers the only fallible arm; route collisions panic loudly at boot rather than shadowing; managed mode shares the file-mode serving path; byte-for-byte pinned across empty/tracker-unknown/virtual rows). |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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-admin/src/lib.rs`:
- Around line 494-498: Canonicalize model ordering before rendering the status
response. In the handler flow around
models_status_handler::render_models_status, sort all_models by a stable key
such as model ID before serialization, preserving the renderer’s behavior
otherwise. Add validation using stores that return identical models in opposite
orders and assert the admin and status responses remain byte-identical.
🪄 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: de9768bf-a9d9-4666-88c4-175a3a8d0d75
📒 Files selected for processing (2)
crates/aisix-admin/Cargo.tomlcrates/aisix-admin/src/lib.rs
Comment on lines
+494
to
+498
| axum::Json(models_status_handler::render_models_status( | ||
| all_models, | ||
| state.models_status.runtime_status_tracker.as_deref(), | ||
| )) | ||
| .into_response() |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Canonicalize model ordering before serialization.
ConfigStore::list_models has no ordering contract, while the shared renderer preserves its input order. Because the admin and status endpoints perform separate reads, an implementation returning the same entries in different orders breaks the byte-identical response guarantee; the current test only exercises one stable InMemoryStore.
Sort by an explicit stable key—preferably model ID—and test with stores returning opposite orders.
Proposed renderer fix
pub(crate) fn render_models_status(
- all_models: Vec<ResourceEntry<Model>>,
+ mut all_models: Vec<ResourceEntry<Model>>,
tracker: Option<&ModelRuntimeStatusTracker>,
) -> Vec<ModelStatusView> {
+ all_models.sort_unstable_by(|left, right| left.id.cmp(&right.id));
all_models
.into_iter()As per coding guidelines, “Define verifiable success criteria, write reproducing or validation tests where appropriate.”
Also applies to: 916-1007
🤖 Prompt for 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.
In `@crates/aisix-admin/src/lib.rs` around lines 494 - 498, Canonicalize model
ordering before rendering the status response. In the handler flow around
models_status_handler::render_models_status, sort all_models by a stable key
such as model ID before serialization, preserving the renderer’s behavior
otherwise. Add validation using stores that return identical models in opposite
orders and assert the admin and status responses remain byte-identical.
Source: Coding guidelines
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
1 participant
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.
What
GET /status/modelson the dedicated metrics/status listener: the per-model runtime health view (cooldown / background-check state) as an operational read, next to/status/configand/status/ready. The response is exactly whatGET /admin/v1/models/statusserves; the admin endpoint is unchanged.Why
Operational reads now have a home on the operational listener. The runtime health view is proxy-runtime state — which targets are cooling down and why — the kind of state probes, dashboards, and scrape-side tooling poll on day 2. Serving it only on the admin listener couples those reads to the management surface and its admin-key auth (and managed mode does not bind an admin listener at all). The metrics/status listener is already the unauthenticated, network-restricted operational surface (#774), identical in every mode, which mirrors the ecosystem-wide split between a management API and a dedicated status/health port.
Sensitivity class matches the listener: the view exposes the model catalog (ids and display names) and health states — the same class as
/status/config'sresource_countsandrejected[]resource ids already served there. No auth, like the scrape — restrict at the network layer.Design: one render, one source
render_models_status(...)is extracted inmodels_status_handler.rsand is the single render behind both endpoints, so the two responses cannot drift while both exist.Arc<dyn ConfigStore>handle the admin surface uses (etcd store in etcd mode, snapshot read-view in file mode) plus the same sharedModelRuntimeStatusTracker— same data, same ordering, same serialization. A unit test pins the two bodies byte-for-byte.{"error_msg": "failed to list models"}500 on this unauthenticated surface — backend detail (etcd endpoints, connection errors) stays in the server log, while the admin-key-gated endpoint keeps its detailed envelope. Success bodies stay byte-identical./status/configprecedent. Docs-site follow-up will list/status/modelsalongside the other status endpoints.e2e harness repoint
AdminClient.listModelStatuses()now readsGET /status/modelson the metrics/status listener (new optionalmetricsBaseUrlconstructor argument; method signature and return shape unchanged). The existing runtime-status consumers — background-health, cooldown-contract, retry-on-429-vs-background-ignore, runtime-mixed-filtering, runtime-status — exercise the new endpoint end-to-end with their assertions untouched.Tests
status_models_serves_the_admin_view_byte_for_byte(same store + tracker handles → byte-identical bodies, covering a cooldown row and a virtualnot_applicablerow);status_models_store_failure_never_leaks_backend_detail(failing store → fixed 500 body, no backend detail);status_models_answers_the_fixed_500_when_the_store_hangs(paused-clock test with a never-resolving store → the timeout arm answers the same fixed body);admin_router_does_not_serve_status_models; the existing metrics-listener tests updated for the new router argument.status-models-e2e.test.ts, new):GET /status/modelsvsGET /admin/v1/models/statusasserted deep-equal and raw-text-equal (same order, same bytes), plus the expected cooldown contract (status=cooldown,status_reason=upstream_auth_failure,cooldown_untilset).cargo fmt --check,cargo clippy --workspace --all-targets -- -D warnings,cargo test --workspacegreen; full e2e suite green twice locally.Summary by CodeRabbit
New Features
GET /status/modelsendpoint on the metrics/status listener.Bug Fixes