fix(serve): gate readiness on an inference probe

[volen-silo]

• If this PR fixes a bug, searched tests/e2e-cucumber/expectations.toml for the fixed ticket ID and removed/narrowed any now-stale xfail rows.

Summary

A managed service was reported ready as soon as its /v1/models endpoint listed the model. Engines advertise a model within seconds of accepting its name, while the weights can take minutes to become usable, so anything that waited on readiness before sending traffic — users, automation, the E2E suite — got a hang or a failed first request. One observed lemonade serve listed the model in 4s and could not answer for the next 6 minutes.

Root cause: every readiness path treated "the listing endpoint answered" as "the model can serve". The initial serve gate on lemonade did smoke-test inference, but the ongoing healthcheck — the signal services list surfaces — did not, and the vLLM path had no inference check at all.

Readiness now requires a completed chat request. The probe lives in rocm-core and is shared by all five paths that can report ready: both engine healthchecks, the serve-time wait behind rocm serve, the provider listing, and the runtime liveness refresh.

Non-obvious decisions

• The verdict is latched per service. Readiness is polled repeatedly; probing on every poll would queue a generation request behind the user's own traffic. A service is probed until it answers once, then falls back to the cheap listing check. Trade-off: a service that degrades after start still reports ready — no worse than before this gate existed. A restart clears the latch, since the new child has an unloaded model.
• A failed probe cannot latch, so those are throttled. A still-loading service is re-probed at most once per 15s, and the attempt is persisted — each CLI invocation is a fresh process, so an in-memory throttle would do nothing. Without this, every services list against a warming model pays a full probe timeout.
• A probe passes on any answered request below HTTP 500, not only a 200. A 4xx proves the inference path is up and the model is resident — the request was understood and refused on its merits — while the failure being caught is a hang or the 5xx an engine returns while warming up. Requiring 200 with content would also wrongly fail a reasoning model, which can spend a two-token budget before emitting any content.
• Lemonade's post-load smoke test keeps its stricter 200 bar. It shares the probe's transport but not its acceptance rule: there, a refusal means the model that came up is not the one that was asked for, which should fail the serve rather than be reported as working.
• A listed-but-unservable model is reported as still coming up, never as failed or stale. rocmd restarts a service left in a recoverable or stale-starting state, so a naive gate would have converted this false positive into a restart loop on exactly the slow-loading models that motivated the fix.
• Responses are read against a deadline, not to end-of-stream. Connection: close is a request, not a guarantee; a server that answers in full and holds the socket open would otherwise stall to the timeout and have its answer discarded. A socket read timeout also bounds each read, not the sequence of them.

Behavior change worth knowing

rocm serve still waits a fixed 45s for readiness — the bar is stricter but the budget is unchanged, so a model that needs longer now returns with status running instead of a false ready. It is promoted to ready by the next readiness check once inference answers; nothing is stuck. A launch in that state says so and explains that no smoke test was run, rather than showing empty metrics.

Verification

The MI300X lane (gfx943, ROCm 7.13.0, vLLM 0.23.0+rocm723, effective_serve_engine: vllm) ran both EAI-7333 scenarios and every step passed, with the inference request succeeding 0.4s after the CLI reported ready:

8 - A service reported ready can immediately serve inference
    Given a managed runtime is active              -> passed
    And a model is being served on GPU             -> passed
    When the CLI reports the service as ready      -> passed
    Then an inference request succeeds immediately -> passed

Both expectations.toml rows are removed on that evidence. Removal also restores the default 600s serve timeout in place of the 90s known-bug cap — worth noting because the confirming run used 69.2s of that 90s, so the cap itself had become a flake risk.

• cargo test --workspace — 19 new tests covering the probe's accept/reject rules, the latch, the retry throttle, bounded reads (including a server that answers without closing, and one that dribbles bytes forever), per-engine latch persistence, the serve-time wait, the no-demotion rule, and the restart reset.
• cargo clippy --workspace --all-targets, cargo fmt --check
• Pre-existing local failures confirmed unrelated: proc_lifecycle::tree_* fail identically on an unmodified main worktree (WSL2 process-tree signals).

Risk

Medium. Touches every readiness path. The restart-loop interaction above is the main hazard and is covered by a regression test; the serve-time behavior change is described above.

[rominf]

Thanks for taking this on — the Listing → "running" mapping is the right call, and the reasoning at main.rs:16099 about manifest_service_recovery_reason only stale-restarting "starting"/"recovering" is correct and well tested. Bounding connect with connect_timeout is a good independent fix, and the new tests pinning exact request sequences genuinely lock in the "don't re-probe" behavior.

Requesting changes on three items, plus some things worth a look.

Blocking

1. The 45s serve wait can't satisfy the motivating case — apps/rocm/src/main.rs:4737

wait_for_service_http_ready_with_progress is called with a hard Duration::from_secs(45). The description says rocm serve now returns only once inference works and the wall-clock grows by the model's real load time. It doesn't: for the reported case (lists in ~4s, can't answer for ~6 min) the loop exhausts 45s, never reaches Serving, and returns Listing → "running". Readiness self-heals later via refresh_managed_service_runtime_liveness, so nothing is permanently broken, but the serve-time gate is only fixed for models that load in under 45s. Either raise the budget alongside the stricter bar, or drop that claim from the description.

2. Post-launch smoke test is now silently skipped for slow models — apps/rocm/src/main.rs:4251

The gate is !no_smoke_test && !report.already_running && report.status == "ready". Previously a listed model hit "ready" in seconds and the smoke test ran. Now anything missing the 45s window lands at "running", so the smoke test is skipped and SmokeMetrics::default() renders an empty metrics block with no explanation — on exactly the slow-loading models this PR targets. Not covered by a test and not mentioned in the description.

3. Stale doc comment — apps/rocm/src/main.rs:4509

status_for_readiness now also emits "running" for Listing; the doc comment still says "ready", "starting", or the existing service's status.

Worth addressing

4. Restart path bypasses reset_for_restart() — apps/rocmd/src/lib.rs:4784

restart_managed_service hand-rolls the restart_count bump and never clears inference_verified_at_unix_ms, while refresh_from_engine_state only ever adopts a verification. I don't believe this is a live false positive: the respawned child runs supervise_service which writes a fresh ManagedServiceRecord::new(...), and the interim "recovering" status is excluded by the matches!(status, "ready"|"running") probe gate. But this is the one site violating the invariant the PR introduces, and it's safe only by accident of two unrelated behaviors — plus the parent rewrites the record at lib.rs:4797 after spawn, carrying the stale latch, which can land after the child's fresh write. Suggest calling reset_for_restart() here for defence in depth.

Separately (pre-existing, own issue): because supervise_service always builds a fresh record, the restart_count bump on this path is discarded on every crash recovery.

5. Probe response reading has no Content-Length/chunked handling — crates/rocm-core/src/lib.rs:490

read_tcp_stream_to_string relies entirely on the peer closing the socket for EOF. Connection: close is sent, but nothing forces a server or intervening proxy to honor it. A service that writes a complete, valid 200 and holds the socket open blocks to the read timeout, the response is discarded, and the service sits in Listing forever. The new hang test only covers "server writes nothing" — worth a test for "server answers but doesn't close."

6. Timeout is per-read, not per-call — crates/rocm-core/src/lib.rs:471

set_read_timeout bounds each read(); read_to_string loops. A slow-drip responder extends the call to an arbitrary multiple of INFERENCE_PROBE_TIMEOUT, contradicting the test assertion at lib.rs:432 that the probe is bounded by its timeout. Pre-existing in the GET helper, but this PR promotes it into a supervisor/dash polling primitive.

7. Listing is never cached

Serving latches; Listing doesn't. refresh_managed_service_runtime_liveness (main.rs:13705) and ready_local_services (providers.rs:504) re-run listing plus up to 8s of probe on every call while a model warms, with no backoff. If real time-to-first-token exceeds 8s — which INFERENCE_PROBE_TIMEOUT's own doc comment anticipates for a large model's first prompt-processing pass — it never latches and every poll stalls ~10s indefinitely.

8. Serialized probing on a user-facing path — apps/rocm/src/providers.rs:504

ready_local_services iterates with a plain for; each unlatched service costs up to 2s listing + 8s probe. It's reached from LocalProvider::status() and local-service selection, both in chat routing, so N services started together can block a list or chat request for up to N × 10s.

Smaller

• engines/vllm/src/lib.rs:1619 vs engines/lemonade/src/lib.rs:905 — the inference_verified / record_inference_verified latch bookkeeping is duplicated verbatim (doc comments included). Only the raw HTTP probe is actually shared; this will drift.
• engines/lemonade/src/lib.rs:2259 — healthcheck can now reach ~14s (3s health + 3s /v1/models fallback + 8s probe) against a 5s watcher tick, evaluated serially with no timeout around the engine subprocess call. One slow service head-of-line-blocks healthchecks for all others.
• tests/e2e-cucumber/expectations.toml — the 90s serve_timeout_secs on the two EAI-7333 rows may now trip on legitimate cold-load time rather than the original race (serving_steps.rs notes cold load "can far exceed a minute", hence the 600s default). That would prevent the hardware run from confirming the fix.
• No test drives healthcheck_service(...) end-to-end asserting .status/.model_loaded — only the lower-level inference_verified helper. The mapping does check out by hand (every "failed" literal is a genuine crash path; listed && !ready always maps to "loading" at lemonade:882 / vllm:770), but the PR's self-identified main hazard has no regression test where the response is assembled.
• crates/rocm-core/src/lib.rs:~300 — status < 500 treats a 404 as serving. Latent only today (both engines implement /v1/chat/completions, and a bad key fails the same-key listing first), but worth a comment for future non-OpenAI-shaped engines.
• Description says 12 new tests; the diff adds 15.

Not verified

Real GPU behavior — items 1, 5, 7 and the e2e cap all hinge on whether 8s is enough for a first token on MI300X/Strix, which I can't tell from code. The four E2E lanes hadn't concluded when I read the rollup.

[volen-silo]

Thanks — this was a genuinely useful review. Verified every item against the code before acting; all the blocking ones held up. Pushed as dbc07a5.

1 — 45s serve wait. Correct, and the description was simply wrong. I kept the 45s budget and fixed the claim instead: blocking a CLI for the length of a cold model load is worse than returning and letting the next readiness check promote it, which is what the design already does. The description now states the real behavior — a slow model returns running, not a false ready, and is promoted once inference answers. Happy to revisit if you'd rather the budget move.

2 — smoke test skipped. Partly. The metrics rows render n/a, the heading flips to "not ready yet", and there was already a note pointing at the logs, so it wasn't unexplained. What was wrong is that the note blamed the startup timeout, which isn't what happened to a running service — it did answer, it just can't serve yet. The note now distinguishes the two and says the smoke test was skipped. Test added.

3 — stale doc comment. Fixed; it covers running now.

4 — restart_managed_service. Agreed, and your reasoning about why it isn't currently live matches what I found. It now goes through reset_for_restart() — the post-spawn write racing the child's fresh record is exactly the kind of accident worth not depending on. The restart_count discard on that path is pre-existing and left alone.

5 + 6 — response framing and per-read timeouts. Both real. Responses are now read against a wall-clock deadline and complete on their own framing (Content-Length, chunked terminator, or EOF). Two tests: a server that answers in full and holds the socket open, and one that dribbles bytes forever inside the per-read timeout. Both would have passed before while doing the wrong thing.

7 + 8 — no Listing cache. Fixed together, since 8 is the visible cost of 7. A failed probe now records the attempt and backs off for 15s, persisted on the record — an in-memory throttle would do nothing when every CLI run is a fresh process.

Duplication. Hoisted into one rocm-core helper used by both engines, which is also where the backoff lives now, so the engine healthchecks get it too — that shortens the worst-case lemonade healthcheck you flagged, though the underlying serial-per-service evaluation is unchanged and pre-existing.

On your two open questions — the lanes have since finished, and they answer both. MI300X (gfx943, vLLM 0.23.0+rocm723) ran both EAI-7333 scenarios with every step passing; inference succeeded 0.4s after the CLI reported ready. So 8s is comfortably enough for first token there.

Your e2e-cap concern was better founded than it looked: the confirming run used 69.2s of the 90s cap. I've removed both rows on the hardware evidence, which restores the 600s default and retires the cap.

404-as-serving is now commented. Test count corrected — 19.