fix(serve): persist running->ready promotion in CLI liveness refresh

[michaelroy-amd]

Summary

refresh_managed_service_runtime_liveness() in apps/rocm/src/main.rs already runs the real HTTP readiness probe (managed_service_endpoint_model_ready()), but only used a passing probe to skip demotion — it never persisted the running -> ready transition. This means a service manifest can sit forever at status: "running" even after the model has actually finished loading and is serving requests.

Root cause

The twin code path in apps/rocm/src/providers.rs::ready_local_services() already does this correctly: on a passing probe it sets record.status = "ready" and writes the manifest back to disk. refresh_managed_service_runtime_liveness() (used by load_managed_services(), which backs the services MCP tool and — via pick_managed_chat_endpoint() — chat's endpoint selection, which requires the exact string "ready") never got the same fix. As a result chat could never discover a genuinely ready local server if the record had been left at "running".

Fix

Mirror ready_local_services()'s promotion: when the probe passes and record.status == "running", set it to "ready" and signal a change so the existing write-back logic in load_managed_services() / load_managed_service() persists it.

Test Plan

• Added load_managed_services_promotes_running_to_ready_once_probe_passes — spins up a mock /v1/models HTTP endpoint, writes a manifest with status: "running", calls load_managed_services, and asserts the in-memory and on-disk (load_managed_service re-read) status is "ready".
• cargo build -p rocm
• cargo clippy -p rocm --all-targets --all-features -- -D warnings
• cargo test -p rocm -- --test-threads=1 (338 passed)

Relates to EAI-7352

[rominf]

LGTM. The running→ready promotion is genuinely persisted (returns true; both callers load_managed_service and load_managed_services then record.write()), idempotent (a second call finds status already ready and returns false — no write thrash), and only ever promotes running→ready, never the reverse. It faithfully mirrors the established providers.rs::ready_local_services() path, and readers that special-case ready now observe the persisted value. The regression test asserts the on-disk manifest (not just the returned Vec), so it would catch a "returned but not persisted" bug.

Approving.

[volen-silo]

🔴 Automated review · pr-review-watcher · 0561199

Summary

• Change type / scope: Bug fix — one function in apps/rocm/src/main.rs (net diff: +92 lines, single file), plus one added test.
• Overall assessment: Approve. The change is small, correct, well-scoped, mirrors an existing verified pattern, and ships with a meaningful, non-vacuous test.
• Blocking findings: 0
• Non-blocking findings: 0 substantive (one minor description-accuracy note below)

What the change does

refresh_managed_service_runtime_liveness() already ran the real HTTP readiness probe but, on a passing probe, only used the result to skip demotion — it never persisted the running -> ready transition. The fix adds a promotion branch: when the probe passes and record.status == "running", set it to "ready" and return true so the caller's existing write-back (load_managed_services / load_managed_service) persists it. This mirrors the twin path providers.rs::ready_local_services().

Verification performed

Two independent reviewers (production logic + blast radius; test quality) plus cross-file synthesis. All findings were verified against source, including checking the pre-fix state.

Production logic — clean.

• Control flow is correct: already-"ready" records still return false (no write); only "running" records with a passing probe are promoted. One true/write per promotion, no double-write.
• The early return true does not skip necessary PID bookkeeping — the endpoint_ready early-return already existed before this PR and already short-circuited the PID logic whenever the probe passed. This diff only changes what happens inside that pre-existing branch.
• Blast radius is safe. Every other status consumer treats "ready" and "running" as equivalent via matches!(_, "ready" | "running") — sidebar counts (~L13024), managed_service_is_live (~L13032), managed_service_running_state (~L13065), local_assistant_service_ready_for_chat (~L7177), and the existing_live_managed_service idempotency guard. Promotion is a no-op for all of them. The only observable effect is that exact-match == "ready" consumers (the provider/chat local-endpoint path via ready_local_services) now correctly see a genuinely ready server, which is the intended fix.

Test — clean and meaningful.

• The mock /v1/models body {"data":[{"id":"Qwen3-0.6B-GGUF"}]} is load-bearing: managed_service_endpoint_model_ready requires an actual model-id match (model_refs_match), not just a 200. An empty/mismatched body would fail the probe and the test.
• Exactly 2 probes occur (one per load_managed_services / load_managed_service read), matching the mock's for _ in 0..2; refresh_from_engine_state short-circuits with zero network I/O because no engine-state file is written. No 3rd probe, no accept()/server.join() deadlock.
• Non-vacuous: record.status = "running" is required to arm the promotion branch (constructor defaults to "starting"), and against the pre-fix code the in-memory assert_eq!(promoted.status, "ready") would fail. The raw fs::read + manual deserialize (added in db61e2e) correctly closes the gap where load_managed_service's own re-read would re-run the promotion and mask a missing persist.
• Clean resource/error handling: Connection: close ensures clean socket teardown; server.join().expect(...)? correctly propagates both thread panics and the thread's inner Result.

Commit / PR quality — clean.

• Three intentional, well-named commits (fix / test / chore: revert). The THIRD_PARTY_NOTICES revert is fully clean — net diff vs base is exactly one file (main.rs), zero residual notices change.
• Signed-off-by trailers only; no AI-generated footers.

Minor note (non-blocking, description only — no code change needed)

The PR description states chat's endpoint selection "requires the exact string "ready"" and implies load_managed_services (which backs local_assistant_service_ready_for_chat) is the exact-match consumer. In the current code local_assistant_service_ready_for_chat (L7177) actually accepts matches!("ready" | "running"), so it was not itself blocked by the stale "running" status. The genuine exact-"ready" consumer is the provider/chat local-endpoint path via ready_local_services (providers.rs). The fix is correct and beneficial either way — this is only a slight imprecision in the rationale, not a defect in the change.

Tradeoffs (deliberate, noted for awareness)

• Deferred write vs immediate write: unlike ready_local_services() (writes inside the fn), this path returns bool and lets the caller persist. This is consistent with how refresh_managed_service_runtime_liveness already handled its other transitions (stopped/starting), so the pattern is internally consistent — not a new inconsistency.
• Probe timeout differs from the mirror: this path uses SERVICE_LIVENESS_CHECK_TIMEOUT (750ms) vs ready_local_services's LOCAL_SERVICE_READY_TIMEOUT. Both predate this PR; the shorter liveness-refresh timeout is appropriate for a frequently-called refresh and is not introduced here.
• Non-atomic manifest writes (fs::write with no locking in ManagedServiceRecord::write) leave a pre-existing TOCTOU window if two processes refresh the same manifest concurrently. Pre-existing and unchanged by this PR — noted only for awareness.

Positive signals

• The added test proves persistence to disk, not just in-memory mutation, via a raw file re-read that deliberately bypasses the code path that would otherwise mask a missing write — exactly the right assertion for this bug class.
• The fix explicitly references and mirrors the already-correct twin path, keeping the two liveness/readiness code paths consistent.