Add mode-aware management health endpoint - #1351
Conversation
|
This pull request is currently a draft. Reviews will not take place until the PR is marked as ready for review. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan includes up to 8 reviews per rolling hour; 5 remain after this review. 📝 WalkthroughWalkthroughAdded a cache-based ChangesManagement health endpoint
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: ⚪ Minimal · up to The change adds a management health endpoint with documented behavior and reported passing validation; no actionable merge-blocking risk remains beyond normal checks and review. Sequence Diagram(s)sequenceDiagram
participant ManagementClient
participant APIRouter
participant health_handle as health::handle
participant Node
participant RuntimeCaches
ManagementClient->>APIRouter: GET /health
APIRouter->>health_handle: dispatch request
health_handle->>Node: read connectivity_snapshot
Node-->>health_handle: peer counts
health_handle->>RuntimeCaches: read cached runtime data
RuntimeCaches-->>health_handle: mode and serving data
health_handle-->>ManagementClient: HTTP 200 JSON response
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Review: #1351 — mode-aware management health endpointOverall: the shape is right — My concerns are concentrated in one place: the process-state strings in 1. The
|
| string | writer |
|---|---|
ready |
runtime/local.rs:937 (local_process_snapshot) |
shutting down |
model_lifecycle.rs:272, model_lifecycle/unload.rs:212,221, startup_handles.rs:981 |
stopped |
model_lifecycle.rs:291 (shutdown_runtime_loaded_models only) |
exited |
model_lifecycle/unload.rs:328,332 (run_auto_handle_runtime_exit) |
So:
"failed"and"error"are never emitted for a local process. The only
status: "error"writers in the tree areplugin/mod.rs:321and
plugin/installed.rs:223(plugin summaries, different field)."failed"for
a process only exists asInstanceLifecycleState::Failed.as_str()
(instance_lifecycle.rs:82), which populateslifecycle_instances, not
local_processes. That means
management_health_reports_failed_host_process_as_unhealthyasserts on a
string the runtime never produces — the test passes without covering the real
failure path. The real one to assert is"exited"."serving"in the healthy filter is likewise dead — same reason
(InstanceLifecycleState::Serving, not a process state)."stopped"is a clean state, not a failure. It is emitted only by
shutdown_runtime_loaded_models(control_loop.rs:387), which upserts
stoppedand does not remove the row. Combined withhas_workbeing true
for any non-emptylocal_processes, a node in normal shutdown reports
serving.status: "unhealthy". For a probe wired to alerting that is a false
alarm on every graceful stop."shutting down"matches neither arm, so a draining single-model node
reportsstarting. Probably harmless, but it's an accident rather than a
decision.
Suggested fix: don't hand-maintain a string set here. runtime/dashboard.rs:349
already has runtime_status_from_process_status mapping exactly these strings to
the RuntimeStatus enum, and it is currently #[allow(dead_code)]. Match on
that enum instead, and the compiler starts carrying the invariant. If that's too
much surface for this PR, at minimum: drop "failed"/"error"/"serving",
drop "stopped" from the failure set, decide explicitly about
"shutting down", and retarget the failed-host-process test at "exited".
2. Mode is exclusive, so a serving node's split stages are invisible
health_mode returns one of three, and local_stage_statuses is only fetched
if matches!(mode, HealthMode::Worker) (health.rs:~110). A NodeRole::Host
node that also owns a local split stage reports mode: serving and never looks
at stage_runtime_statuses(), so a StageRuntimeState::Failed stage on a
serving host yields serving.status: healthy. If that combination can't happen,
say so in the doc comment; otherwise consider collecting both sources
regardless of mode and letting mode be presentation only.
3. cached_plugin_models degrades a serving host to idle, not degraded
inference_models().await.unwrap_or_default() — the comment says "fail closed",
but the effect is that an unavailable cached plugin inventory on a
plugin-only serving host produces models: [] → idle. That reads as "nothing
configured" rather than "I can't tell". A probe cannot distinguish the two.
4. Minor
mesh.status: "disconnected"for a healthy standalone single-node mesh is
technically true but will read as a fault in dashboards.standalone(or
documenting it more loudly) would be kinder. It is documented as advisory,
so not blocking./healthis not inrequires_trusted_local_access(api/access.rs:3), so
under--listen-all(server.rs:146) it discloses model names and peer
counts to the LAN unauthenticated. This matches the existing posture of
/api/status, so it's not a regression — but it's a new, deliberately
cheap-to-poll surface, so worth a conscious yes.openai-frontendalready serves/healthand/healthz
(router.rs:206-207) on the serving port. Two different/healthendpoints
on two ports will confuse operators; consider a/healthzalias here for
symmetry, and a line in the docs distinguishing the two.connectivity_snapshotwalksstate.peerstwice; one pass computing both
counters would be equivalent and cheaper. Cosmetic.eligible_management_route(management_lifecycle.rs:51) excludes/health
because it isn't under/api/, so it won't pollute the workload ledger —
correct and probably intentional;logging/request_metadata.rs:57already has
thehealthlabel. Worth one sentence in the module doc so a future move under
/api/doesn't silently start recording probe traffic.
Verification I did / did not do
Source read at PR head 2bf0fb3d, fetched as pr1351. I did not build or
run the tests locally, and I did not curl a live node. All claims above are
source-grep claims scoped to crates/ in this checkout; the "never emitted"
claims in §1 come from grepping every non-test writer of those two fields and
could be wrong if a state string is constructed dynamically somewhere I didn't
match on a literal.
|
@michaelneale Thanks for the detailed source-level review — addressed in Changes made:
Validation:
|
|
Follow-up verification after the branch was synchronized with latest main:
The local worktree is clean and matches the pushed PR head. |
michaelneale
left a comment
There was a problem hiding this comment.
good to get in when green
Summary
GET /healthmanagement endpoint that always returns HTTP 200/status: okwhen the management API can answerstandalone,connected, ordisconnected) plus Worker, Client, and Serving mode health without active probes or network refreshesSimulated responses
These are representative examples. Both return HTTP
200 OKbecause/healthis a management-process liveness endpoint; the nested fields carry mesh and model-serving readiness.Healthy serving node
{ "status": "ok", "mode": "serving", "mesh": { "status": "connected", "admitted_peer_count": 2, "connected_peer_count": 2 }, "serving": { "status": "healthy", "models": [ "Qwen3-8B-Q4_K_M", "Llama-3.2-3B-Instruct-Q4_K_M" ] } }Non-healthy worker node
This simulates an admitted mesh member whose live control connection has dropped and whose local split stage has failed.
{ "status": "ok", "mode": "worker", "mesh": { "status": "disconnected", "admitted_peer_count": 1, "connected_peer_count": 0 }, "serving": { "status": "unhealthy", "models": [] } }Validation
just buildjust website-buildjust with-lld cargo fmt --all -- --checkjust with-lld cargo test -p mesh-llm-host-runtime health --lib(50 passed)just with-lld cargo test -p mesh-llm-host-runtime --lib(2,483 passed, 8 ignored)just with-lld cargo clippy -p mesh-llm-host-runtime --libCloses #1349
Summary by CodeRabbit
New Features
GET /healthendpoint returning HTTP 200 when the management process is reachable.Documentation
Tests