feat(serve): surface a coarse startup phase while a service comes up - #107
Conversation
…tch-all InstanceStatus had no Ready state, so merge_instance hardcoded every discovered instance to Running regardless of whether it had actually passed a readiness probe. Managed services with a "ready" record status, Lemonade endpoints (built only after a health probe), and Docker containers all collapsed into the same Running label, hiding real lifecycle differences from the TUI and any future readiness-aware consumer. Add InstanceStatus::Ready, thread a status field through DiscoveredService so each discovery source reports what it actually knows, and drop merge_instance's hardcoded override in favor of svc.status. Managed-service records now map status strings (including "recovering", previously dropped from the scrapeable set entirely) through to Ready/Running/Starting. Lemonade endpoints are built as Ready since every call site only constructs them post-health-probe. Docker-discovered instances have no status source of their own, so they stay Starting until their first successful vLLM Prometheus (or Lemonade stats) scrape promotes them to Ready. Add an is_serving() helper (Ready | Running) for TUI call sites that only care about "is this actively serving" rather than the precise state, and add Ready arms to the exhaustive status_meta/status_role matches in the instances tab. Relates to EAI-7350 Signed-off-by: Michael Roy <michael.roy@amd.com>
A service that is still coming up showed only a generic STARTING/spinner
with no indication of whether it was pulling weights, loading them, or
warming up — so a slow first-token launch was indistinguishable from a
hang. The serve supervisor already streams the engine's stdout/stderr to
the service log file but never inspected it, and the dashboard had no
field to carry a startup stage.
Add a coarse startup phase (Downloading/Loading/Warmup) end to end:
- rocm-core: add ManagedServiceRecord.startup_phase (Option<String>,
serde-default so old on-disk records still load), cleared once the
service reaches ready.
- rocmd: tail the service log during wait_for_service_ready and classify
each line into a phase, persisting transitions to the record. Parsing
is split into small pure helpers (last_cr_segment collapses \r progress
redraws the same way the dashboard job console does; classify_startup_phase
matches the vLLM/llama.cpp/HF startup vocabulary) so it is unit-testable.
- rocm-dash-core: add a StartupPhase enum and turn InstanceStatus::Starting
into Starting { phase: Option<StartupPhase> }. StartupPhase is fieldless
and Copy, so InstanceStatus stays Copy and the only ripple is pattern
shape. A new InstanceStatus::label() owns the status text (READY /
DOWNLOADING / LOADING / WARMUP / …).
- rocm-dash-daemon registry: read the record's startup_phase and map it
onto Starting { phase } (recovering/starting only); Docker discovery and
the scrape-success promotion keep phase None.
- rocm-dash-tui: status_meta/status_role and the services list render via
label(), so the phase shows on instance cards, the detail pane, and the
services manager. (serving.rs is a static verb menu with no per-instance
status, so the live rendering lives in instances.rs + services_manager.rs.)
Stacked on #106 (EAI-7350).
Relates to EAI-7355
Signed-off-by: Michael Roy <michael.roy@amd.com>
…ing` supervise_service (apps/rocmd/src/lib.rs) flips a managed-service record's on-disk status to "running" immediately after spawning the engine child -- before it starts polling and classifying startup_phase from the serve log -- and only clears startup_phase once the service reaches "ready". So a record can sit at status="running" with startup_phase=Some(...) for the entire download/load/warmup window. registry::instance_status_for_record only read startup_phase in the "starting"/"recovering" arm, so that overlap fell straight through to a bare InstanceStatus::Running and DOWNLOADING/LOADING/WARMUP never rendered during a real `rocm serve <large-model>` cold start. The feature was inert end-to-end despite the plumbing all being in place. Chose the lower-risk fix: teach instance_status_for_record to also honor startup_phase on the "running" arm, rather than delaying the "running" write in supervise_service until the service is ready (rejected: that would leave the record at status="starting" for the whole download, and manifest_service_recovery_reason's 5-minute staleness check keys off "starting"/"recovering" -- a slow cold start could then get misclassified as a stale/stuck launch and trigger an unwanted recovery restart, a regression supervise_service's current "running"-first ordering was implicitly guarding against). The existing registry test for this path constructed a status="starting" + startup_phase record -- a shape the real producer never writes -- which masked the bug. Replaced it with a case driven through the actual load_service_records + discovered_from_record path using the real status="running" + startup_phase shape, plus a starting/recovering parametrized case and a running-with-no-phase steady-state case. Also bump PROTOCOL_VERSION: InstanceStatus::Starting went from a unit variant to a struct variant carrying `phase` in the prior commit on this branch, which changes Event::InstanceDiscovered/Snapshot's external-tag JSON shape for that variant; the version bump was missing. Regenerated THIRD_PARTY_NOTICES.txt (pre-existing ordering drift, unrelated to this change). Relates to EAI-7355 Signed-off-by: Michael Roy <michael.roy@amd.com>
…o dep change) Signed-off-by: Michael Roy <michael.roy@amd.com>
…g instances serving on unknown phase
- Add a cross-crate serde contract test: serialize rocm-core's real
ManagedServiceRecord (with startup_phase set) and deserialize it as the
daemon's ServiceRecord, so a future #[serde(rename)] on either side fails
the test instead of silently dropping the phase (previously the field-name
match rested only on a hand-written JSON literal). Adds rocm-core as a
workspace-local dev-dependency (no third-party dep, TPN unaffected).
- Harden the running-status mapping: a 'running' record with no phase OR an
unrecognized/foreign phase token now stays Running (serving); only the
pinned tokens (downloading/loading/warmup) map to Starting{phase}. Prevents
wrongly downgrading a genuinely-serving instance to non-serving STARTING.
Signed-off-by: Michael Roy <michael.roy@amd.com>
rominf
left a comment
There was a problem hiding this comment.
LGTM. The startup-phase enum migration is complete (every Starting construction site updated; compiler-enforced exhaustiveness), the cross-crate serde contract is pinned by a real ManagedServiceRecord→ServiceRecord round-trip test, the PROTOCOL_VERSION 1→2 bump is correct with warn-and-continue on mismatch, unknown/forward tokens keep a running record serving (never downgraded to a non-serving Starting), and #[serde(default)] keeps old on-disk records readable. Right layering: phase derived in the supervisor, mapped once in the registry, labeled once in InstanceStatus::label. Log truncation on restart avoids stale-phase pickup.
Low-severity notes:
read_new_log_phaseadvances the read cursor by the bytes read regardless of line boundaries, so a phase line split across two ~200ms polls can be missed (cosmetic — the next matching line re-triggers; a seek-to-last-newline guard would fix).- The PR adds a
record.write()per phase transition on a non-atomic writer (pre-existingfs::writepattern; negligible given ≤3 transitions and a best-effort reader).
Approving.
|
🔴 Automated review · pr-review-watcher · 7e1d04e Summary
Remaining ConcernsBlockingNone. Non-blocking suggestions
Tradeoffs (deliberate, no action required)
Positive signals
Deployment Notes
Automated review — plain comment only, no approval/merge. The producer-side log-scrape partial-line and replay backward-compat items are the two most worth a maintainer's glance; neither blocks merge. |
The readiness-reason helper hand-rolled two authority parsers
(port_from_base_url / host_from_base_url) with rsplit(':'), which mangles
a bracketed IPv6 loopback: host_from_base_url("http://[::1]:8000") kept
the brackets so is_loopback_host("[::1]") was false, and
port_from_base_url("http://[::1]/v1") parsed "1]" and failed. Either
path silently disabled chat_backend_wait_reason for a legitimate local
IPv6 endpoint.
Replace both helpers with the crate's existing parse_host_port, which
already strips IPv6 brackets, defaults the port from the scheme, and is
pinned by parse_host_port_handles_bracketed_ipv6. Drop the now-redundant
per-helper unit tests (covered by the llm.rs suite) and add a
bracketed-IPv6 regression test against chat_backend_wait_reason.
Also tighten the doc comment: Running is not a hard HTTP-readiness
guarantee, and record the #106/#107 status-signal dependency in-tree so
the Starting/Stopped/Error arms are discoverable as pending until that
work lands beneath this change.
Addresses pr-review-watcher blocking finding #2 (IPv6) and the doc /
test-coverage non-blocking notes on #108.
Signed-off-by: Michael Roy <michael.roy@amd.com>
Parent PR #106 (real Ready status pipeline) merged to main as d78f28f. Merge main back into the startup-phase branch, resolving the InstanceStatus conflicts by composing both changes: - Keep #107's protocol-v2 migration: InstanceStatus::Starting carries { phase: Option<StartupPhase> }, the phase-aware label(), the phase-aware registry mapping (instance_status_for_record), and PROTOCOL_VERSION = 2. - Keep #106's Ready pipeline: Ready variant, is_serving() = Ready|Running, and the Starting -> Ready promotion on first successful scrape (runner.rs), adapted to matches!(.., Starting { .. }). No catch-all downgrade: a running record with an unrecognized phase token stays Running/serving. No lost serde compatibility: the cross-crate startup_phase contract test and legacy NDJSON back-compat tests pass. Conflicts resolved: docker.rs, metrics.rs, registry.rs, runner.rs, instances.rs. Signed-off-by: Michael Roy <michael.roy@amd.com>
The phase-tracking test built its scratch dir from std::env::temp_dir() +
process::id(), which CodeQL models as a tainted path-injection source; that
path then flowed to create_dir_all/write/open/remove_dir_all, producing four
high 'uncontrolled data used in path expression' alerts.
Reuse the crate's existing unique_test_root helper (rooted at
env!("CARGO_MANIFEST_DIR"), a trusted compile-time constant) — the same
pattern every other rocmd test already uses and which CodeQL does not flag.
This removes the env::temp_dir() source, so the source-to-sink flow for all
four alerts is gone. No new dependency or abstraction; the behavioral
assertions (downloading -> cursor-stable None -> loading) are unchanged.
Signed-off-by: Michael Roy <michael.roy@amd.com>
Final refresh: main advanced 9830e57 -> 8308450 via #104 (decouple vLLM Prometheus scraping from enable_docker) and #97 (discover served model for configured chat endpoint). Clean auto-merge (runner.rs only), no conflicts. Preserves the startup-phase / Ready composition (InstanceStatus::Starting { phase }, PROTOCOL_VERSION = 2, phase-aware registry mapping, and the Starting -> Ready scrape-success promotion in runner.rs) alongside #104's scraping-decouple changes, plus the CodeQL test-path fix (rocmd tests use the CARGO_MANIFEST_DIR-rooted unique_test_root helper, not env::temp_dir()). Signed-off-by: Michael Roy <michael.roy@amd.com>
Summary
While a managed service is coming up, the dashboard showed only a generic
STARTINGlabel (or a spinner) with no indication of what it was doing — pulling weights, loading them onto the device, or warming up. A slow first launch was therefore indistinguishable from a hang. This PR surfaces a coarse startup phase (Downloading / Loading / Warmup) end to end.Root Cause
The serve supervisor (
apps/rocmd) already redirects the engine's stdout/stderr into the per-service log file, butwait_for_service_readyonly polled a healthcheck RPC and never inspected that output.ManagedServiceRecordhad no field to carry a startup stage, and the dashboard'sInstanceStatus::Startingwas a unit variant with nowhere to put one.Changes
ManagedServiceRecord.startup_phase: Option<String>(#[serde(default)]so older on-disk records still deserialize), cleared back toNoneonce the service reachesready.wait_for_service_readynow tails the service log while polling and reports each phase transition, which the caller persists to the record. Parsing is split into small pure, unit-tested helpers:last_cr_segmentcollapses\rprogress redraws (pip/tqdm/HF), the same rule the dashboard job console uses.classify_startup_phasematches the common vLLM / llama.cpp / Hugging Face startup vocabulary into adownloading/loading/warmuptoken.StartupPhaseenum and changeInstanceStatus::StartingtoStarting { phase: Option<StartupPhase> }.StartupPhaseisCopy, soInstanceStatusstaysCopy— the only ripple is pattern shape (Starting { .. }), not aCopy-removal cascade. A newInstanceStatus::label()centralizes the status text (READY/DOWNLOADING/LOADING/WARMUP/ …).startup_phaseontoStarting { phase }forstarting/recoveringrecords; Docker discovery and the scrape-success promotion keepphase: None.status_meta/status_roleand the services-manager list now render vialabel(), so the phase shows on instance cards, the detail pane, and the services list. Note:serving.rsis a static verb menu with no per-instance status rendering, so the live phase display lives ininstances.rs+services_manager.rs(the actual instance-status surfaces).Scope / Notes
STARTING.Startingserde wire shape changes from the bare string"starting"to an externally-tagged object; this only affects the in-version daemon socket/replay stream, and the round trip is covered by a new test.Test Plan
cargo build(workspace)cargo clippy --all-targets --all-features -- -D warnings(workspace, clean)cargo test -p rocm-core -p rocm-dash-core -p rocm-dash-collectors -p rocm-dash-daemon— green, incl. new tests:instance_status_label_surfaces_startup_phase,instance_status_starting_round_trips_through_serde,startup_phase_from_token_round_trips_labels,discovered_from_record_surfaces_startup_phase_while_startingcargo test -p rocmd --lib— 109 green, incl.last_cr_segment_keeps_final_progress_redraw,classify_startup_phase_maps_engine_vocabulary,classify_startup_phase_emits_only_dashboard_known_tokens,read_new_log_phase_advances_and_tracks_latestcargo test -p rocm-dash-tui -- --test-threads=1— 543 + 16 + 5 greenRelates to EAI-7355