From 736c583cc14ca0de7655af61f21c22d766f839ac Mon Sep 17 00:00:00 2001 From: Michael Roy Date: Fri, 10 Jul 2026 18:07:33 -0700 Subject: [PATCH 1/4] feat(serve): surface a coarse startup phase while a service comes up MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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, 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 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 --- apps/rocmd/src/lib.rs | 195 +++++++++++++++++- crates/rocm-core/src/lib.rs | 6 + crates/rocm-dash-collectors/src/docker.rs | 9 +- crates/rocm-dash-core/src/metrics.rs | 125 ++++++++++- crates/rocm-dash-daemon/src/registry.rs | 64 +++++- crates/rocm-dash-daemon/src/runner.rs | 4 +- .../rocm-dash-tui/src/ui/services_manager.rs | 4 +- crates/rocm-dash-tui/src/ui/tabs/instances.rs | 34 ++- 8 files changed, 403 insertions(+), 38 deletions(-) diff --git a/apps/rocmd/src/lib.rs b/apps/rocmd/src/lib.rs index 62e38edf..14e333ba 100644 --- a/apps/rocmd/src/lib.rs +++ b/apps/rocmd/src/lib.rs @@ -37,7 +37,7 @@ use sha2::{Digest, Sha256}; use std::collections::{HashSet, VecDeque}; use std::ffi::OsString; use std::fs; -use std::io::{self, BufRead, Read, Write}; +use std::io::{self, BufRead, Read, Seek, SeekFrom, Write}; use std::net::{SocketAddr, TcpStream}; use std::path::{Path, PathBuf}; use std::process::{Command as ProcessCommand, Stdio}; @@ -3107,14 +3107,25 @@ fn supervise_service( record.status = "running".to_owned(); record.write()?; - if wait_for_service_ready( - &record.engine, - &record.service_id, - &record.host, - record.port, + // Clone the fields the poller reads so the `on_phase` closure can borrow + // `record` mutably to persist each startup-phase transition to disk. + let ready_engine = record.engine.clone(); + let ready_service_id = record.service_id.clone(); + let ready_log_path = record.log_path.clone(); + let became_ready = wait_for_service_ready( + &ready_engine, + &ready_service_id, + &ready_log_path, Duration::from_mins(3), - ) { + |phase| { + record.startup_phase = Some(phase.to_owned()); + let _ = record.write(); + }, + ); + if became_ready { record.status = "ready".to_owned(); + // The phase only describes the coming-up window; clear it once ready. + record.startup_phase = None; record.write()?; } @@ -4974,6 +4985,95 @@ mod tests { use rocm_core::ModelRecipeArtifactSourcePolicyRecord; use std::path::PathBuf; + #[test] + fn last_cr_segment_keeps_final_progress_redraw() { + // A tqdm/HF-style in-place redraw collapses to its last segment. + assert_eq!( + last_cr_segment("Downloading: 10%\rDownloading: 55%\rDownloading: 100%"), + "Downloading: 100%" + ); + // A plain line is unchanged. + assert_eq!( + last_cr_segment("Loading model weights"), + "Loading model weights" + ); + } + + #[test] + fn classify_startup_phase_maps_engine_vocabulary() { + assert_eq!( + classify_startup_phase("Downloading shards: 100%"), + Some("downloading") + ); + assert_eq!( + classify_startup_phase("Fetching 12 files"), + Some("downloading") + ); + assert_eq!( + classify_startup_phase("INFO: Loading model weights took 4.2s"), + Some("loading") + ); + assert_eq!( + classify_startup_phase("llama_model_loader: loaded meta data"), + Some("loading") + ); + assert_eq!( + classify_startup_phase("Capturing CUDA graph shapes"), + Some("warmup") + ); + assert_eq!( + classify_startup_phase("Warming up the engine"), + Some("warmup") + ); + // Ordinary chatter carries no phase signal. + assert_eq!( + classify_startup_phase("Uvicorn running on http://..."), + None + ); + } + + #[test] + fn classify_startup_phase_emits_only_dashboard_known_tokens() { + // These tokens are the wire contract with the dashboard's + // `StartupPhase::from_token` (rocm-dash-core); emitting anything else + // would be silently dropped there. rocmd can't link that crate, so the + // contract is pinned here by literal. + for line in [ + "Downloading shards", + "Loading model weights", + "Capturing CUDA graph", + ] { + let token = classify_startup_phase(line).expect("line is a phase signal"); + assert!( + matches!(token, "downloading" | "loading" | "warmup"), + "token {token:?} must be one the dashboard understands" + ); + } + } + + #[test] + fn read_new_log_phase_advances_and_tracks_latest() { + use std::io::Write as _; + let dir = std::env::temp_dir().join(format!("rocmd-phase-{}", std::process::id())); + std::fs::create_dir_all(&dir).unwrap(); + let log = dir.join("svc.log"); + std::fs::write(&log, "boot\nDownloading shards: 100%\n").unwrap(); + + let mut pos = 0_u64; + assert_eq!(read_new_log_phase(&log, &mut pos), Some("downloading")); + // No new bytes → no phase, cursor unchanged. + let after_first = pos; + assert_eq!(read_new_log_phase(&log, &mut pos), None); + assert_eq!(pos, after_first); + + // Appending a later stage advances the phase. + let mut f = std::fs::OpenOptions::new().append(true).open(&log).unwrap(); + writeln!(f, "Loading model weights took 3s").unwrap(); + assert_eq!(read_new_log_phase(&log, &mut pos), Some("loading")); + + let _ = std::fs::remove_dir_all(&dir); + } + #[test] fn engine_serve_http_args_forward_engine_recipe_json() { let engine_recipe_json = r#"{"contract_version":"0.1.0","engine":"vllm","required_flags":["--enable-auto-tool-choice"]}"#; @@ -8545,15 +8645,92 @@ fn optional_arg(flag: &str, value: Option<&str>) -> Vec { } } +/// Keep only the final visible segment of a `\r`-redrawn progress line. +/// +/// Progress tools (pip, tqdm, Hugging Face) redraw a line in place with a bare +/// carriage return and no newline, so the segment after the last `\r` is its +/// final visible state. Lines without `\r` pass through unchanged. (Same +/// collapse rule the dashboard job console applies to streamed job output.) +fn last_cr_segment(line: &str) -> &str { + line.rsplit('\r').next().unwrap_or(line) +} + +/// Classify a single serve-log line into a coarse startup phase token +/// (`downloading`/`loading`/`warmup`), or `None` when the line carries no phase +/// signal. Case-insensitive substring match over the common vLLM / llama.cpp / +/// Hugging Face startup vocabulary. Checked warmup → loading → downloading so +/// the latest lifecycle stage a line mentions wins. +fn classify_startup_phase(line: &str) -> Option<&'static str> { + let lower = line.to_ascii_lowercase(); + if lower.contains("capturing cuda graph") + || lower.contains("capturing the model") + || lower.contains("warming up") + || lower.contains("warmup") + { + Some("warmup") + } else if lower.contains("loading weights") + || lower.contains("loading model") + || lower.contains("load_tensors") + || lower.contains("llama_model_loader") + || lower.contains("model loading took") + { + Some("loading") + } else if lower.contains("downloading") || lower.contains("fetching") { + Some("downloading") + } else { + None + } +} + +/// Read log bytes appended since `*pos`, advance `*pos`, and return the most +/// recent recognizable startup phase in that new output (later lines win, so a +/// download → load → warmup progression advances naturally). +/// +/// Best-effort: any I/O error (file not created yet, transient read) yields +/// `None`. A shrunk file (rotation/truncation) resets the cursor to the top. +fn read_new_log_phase(log_path: &Path, pos: &mut u64) -> Option<&'static str> { + let mut file = fs::File::open(log_path).ok()?; + let len = file.metadata().ok()?.len(); + if len < *pos { + *pos = 0; + } + if len == *pos { + return None; + } + file.seek(SeekFrom::Start(*pos)).ok()?; + let mut bytes = Vec::new(); + let read = file.read_to_end(&mut bytes).ok()?; + *pos += read as u64; + let text = String::from_utf8_lossy(&bytes); + let mut phase = None; + for line in text.lines() { + if let Some(found) = classify_startup_phase(last_cr_segment(line)) { + phase = Some(found); + } + } + phase +} + +/// Poll a freshly-spawned service until its healthcheck reports ready (or the +/// timeout elapses), tailing its log file meanwhile and reporting each coarse +/// startup phase transition via `on_phase`. fn wait_for_service_ready( engine: &str, service_id: &str, - _host: &str, - _port: u16, + log_path: &Path, timeout: Duration, + mut on_phase: impl FnMut(&str), ) -> bool { let start = std::time::Instant::now(); + let mut log_pos: u64 = 0; + let mut last_phase: Option<&'static str> = None; while start.elapsed() < timeout { + if let Some(phase) = read_new_log_phase(log_path, &mut log_pos) + && last_phase != Some(phase) + { + last_phase = Some(phase); + on_phase(phase); + } if engine_healthcheck_ready(engine, service_id).unwrap_or(false) { return true; } diff --git a/crates/rocm-core/src/lib.rs b/crates/rocm-core/src/lib.rs index afb1e28d..fb206fe2 100644 --- a/crates/rocm-core/src/lib.rs +++ b/crates/rocm-core/src/lib.rs @@ -5455,6 +5455,11 @@ pub struct ManagedServiceRecord { pub restart_count: u32, #[serde(default)] pub last_restart_unix_ms: Option, + /// Coarse startup stage (`downloading`/`loading`/`warmup`) parsed from the + /// serve process's own log output while it is coming up. Set to `None` once + /// the service reaches `ready`, and absent on older on-disk records. + #[serde(default)] + pub startup_phase: Option, pub manifest_path: PathBuf, pub log_path: PathBuf, pub engine_state_path: PathBuf, @@ -5502,6 +5507,7 @@ impl ManagedServiceRecord { engine_recipe_json: None, restart_count: 0, last_restart_unix_ms: None, + startup_phase: None, manifest_path, log_path, engine_state_path, diff --git a/crates/rocm-dash-collectors/src/docker.rs b/crates/rocm-dash-collectors/src/docker.rs index 5f7465c6..f84e4e94 100644 --- a/crates/rocm-dash-collectors/src/docker.rs +++ b/crates/rocm-dash-collectors/src/docker.rs @@ -307,10 +307,11 @@ fn parse_container(inspect: &ContainerInspectResponse) -> Result, } +/// A coarse phase within an instance's startup, parsed from the serve logs. +/// +/// Ordered by the lifecycle it represents: pull the weights, load them onto +/// the device, then warm the engine up. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, Serialize, Deserialize)] +#[serde(rename_all = "lowercase")] +pub enum StartupPhase { + /// Fetching model weights (e.g. a Hugging Face download). + Downloading, + /// Loading weights onto the device / initializing the engine. + Loading, + /// Weights loaded; warming up (CUDA-graph capture, warmup requests). + Warmup, +} + +impl StartupPhase { + /// Short UPPERCASE label for status chips. + #[must_use] + pub const fn label(self) -> &'static str { + match self { + Self::Downloading => "DOWNLOADING", + Self::Loading => "LOADING", + Self::Warmup => "WARMUP", + } + } + + /// Parse a lowercase phase token (as persisted in a `ManagedServiceRecord`'s + /// `startup_phase` field) back into a phase. Unknown tokens yield `None`. + #[must_use] + pub fn from_token(token: &str) -> Option { + match token.trim().to_ascii_lowercase().as_str() { + "downloading" => Some(Self::Downloading), + "loading" => Some(Self::Loading), + "warmup" => Some(Self::Warmup), + _ => None, + } + } +} + #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, Serialize, Deserialize, Default)] #[serde(rename_all = "lowercase")] pub enum InstanceStatus { @@ -69,7 +108,12 @@ pub enum InstanceStatus { /// equivalent) and is serving inference requests. Ready, Running, - Starting, + /// Coming up but not yet ready. `phase` carries a coarse startup stage + /// parsed from the serve logs when known (`None` for sources that expose + /// no readiness detail, e.g. Docker-discovered containers). + Starting { + phase: Option, + }, Stopped, Error, #[default] @@ -85,6 +129,21 @@ impl InstanceStatus { pub const fn is_serving(self) -> bool { matches!(self, Self::Ready | Self::Running) } + + /// Short UPPERCASE label for status chips. `Starting` surfaces its startup + /// phase (e.g. `LOADING`) when one is known, else the generic `STARTING`. + #[must_use] + pub const fn label(self) -> &'static str { + match self { + Self::Ready => "READY", + Self::Running => "RUNNING", + Self::Starting { phase: Some(phase) } => phase.label(), + Self::Starting { phase: None } => "STARTING", + Self::Stopped => "STOPPED", + Self::Error => "ERROR", + Self::Unknown => "UNKNOWN", + } + } } #[derive(Debug, Clone, Serialize, Deserialize, Default)] @@ -178,9 +237,71 @@ mod tests { fn instance_status_is_serving_covers_ready_and_running_only() { assert!(InstanceStatus::Ready.is_serving()); assert!(InstanceStatus::Running.is_serving()); - assert!(!InstanceStatus::Starting.is_serving()); + assert!(!InstanceStatus::Starting { phase: None }.is_serving()); + assert!( + !InstanceStatus::Starting { + phase: Some(StartupPhase::Loading) + } + .is_serving() + ); assert!(!InstanceStatus::Stopped.is_serving()); assert!(!InstanceStatus::Error.is_serving()); assert!(!InstanceStatus::Unknown.is_serving()); } + + #[test] + fn instance_status_label_surfaces_startup_phase() { + assert_eq!(InstanceStatus::Starting { phase: None }.label(), "STARTING"); + assert_eq!( + InstanceStatus::Starting { + phase: Some(StartupPhase::Downloading) + } + .label(), + "DOWNLOADING" + ); + assert_eq!( + InstanceStatus::Starting { + phase: Some(StartupPhase::Loading) + } + .label(), + "LOADING" + ); + assert_eq!( + InstanceStatus::Starting { + phase: Some(StartupPhase::Warmup) + } + .label(), + "WARMUP" + ); + assert_eq!(InstanceStatus::Ready.label(), "READY"); + } + + #[test] + fn instance_status_starting_round_trips_through_serde() { + // The data-carrying variant serializes as an externally-tagged object + // and survives a round trip, both with and without a phase. + for status in [ + InstanceStatus::Starting { phase: None }, + InstanceStatus::Starting { + phase: Some(StartupPhase::Warmup), + }, + ] { + let json = serde_json::to_string(&status).expect("serialize"); + let back: InstanceStatus = serde_json::from_str(&json).expect("deserialize"); + assert_eq!(back, status); + } + } + + #[test] + fn startup_phase_from_token_round_trips_labels() { + for phase in [ + StartupPhase::Downloading, + StartupPhase::Loading, + StartupPhase::Warmup, + ] { + let token = phase.label().to_ascii_lowercase(); + assert_eq!(StartupPhase::from_token(&token), Some(phase)); + } + assert_eq!(StartupPhase::from_token("nonsense"), None); + } } diff --git a/crates/rocm-dash-daemon/src/registry.rs b/crates/rocm-dash-daemon/src/registry.rs index 8f496816..e806be27 100644 --- a/crates/rocm-dash-daemon/src/registry.rs +++ b/crates/rocm-dash-daemon/src/registry.rs @@ -36,7 +36,7 @@ use std::path::Path; use crate::runner::instance_from_discovered; use rocm_dash_collectors::engine_registry::EngineKind; -use rocm_dash_core::metrics::{Instance, InstanceStatus}; +use rocm_dash_core::metrics::{Instance, InstanceStatus, StartupPhase}; use rocm_dash_core::traits::DiscoveredService; use serde::Deserialize; @@ -61,6 +61,11 @@ pub struct ServiceRecord { pub port: u16, #[serde(default)] pub status: String, + /// Coarse startup stage (`downloading`/`loading`/`warmup`) the rocm-cli + /// supervisor parsed from the serve logs while the service was coming up. + /// Absent on older records and once the service reaches `ready`. + #[serde(default)] + pub startup_phase: Option, #[serde(default)] pub created_at_unix_ms: u128, } @@ -78,15 +83,19 @@ pub fn is_scrapeable_status(status: &str) -> bool { ) } -/// Map a rocm-cli managed-service record's `status` string onto the -/// dashboard's `InstanceStatus`. Only called after `is_scrapeable_status` has -/// already filtered out `failed`/`stopped`/unrecognized strings, so the `_` -/// fallback arm is unreachable in practice but kept total for safety. -fn instance_status_for_record_status(status: &str) -> InstanceStatus { +/// Map a rocm-cli managed-service record's `status` string (plus any parsed +/// `startup_phase`) onto the dashboard's `InstanceStatus`. Only called after +/// `is_scrapeable_status` has already filtered out `failed`/`stopped`/unrecognized +/// strings, so the `_` fallback arm is unreachable in practice but kept total +/// for safety. The `startup_phase` token is only honored for the `Starting` +/// states; it is meaningless once a service is `ready`/`running`. +fn instance_status_for_record(status: &str, startup_phase: Option<&str>) -> InstanceStatus { match status.trim().to_ascii_lowercase().as_str() { "ready" => InstanceStatus::Ready, "running" => InstanceStatus::Running, - "starting" | "recovering" => InstanceStatus::Starting, + "starting" | "recovering" => InstanceStatus::Starting { + phase: startup_phase.and_then(StartupPhase::from_token), + }, _ => InstanceStatus::Unknown, } } @@ -144,7 +153,7 @@ pub fn discovered_from_record(record: &ServiceRecord) -> Option( id: i.container_id.clone(), model: i.model_name.clone(), port: i.port, - status: format!("{:?}", i.status), + // `label()` surfaces the DOWNLOADING/LOADING/WARMUP startup phase + // when the instance is `Starting`; never a raw `{:?}` debug string. + status: i.status.label().to_owned(), gen_tps: i.gen_tps, }) .collect(); diff --git a/crates/rocm-dash-tui/src/ui/tabs/instances.rs b/crates/rocm-dash-tui/src/ui/tabs/instances.rs index 970a99f2..e83ac6db 100644 --- a/crates/rocm-dash-tui/src/ui/tabs/instances.rs +++ b/crates/rocm-dash-tui/src/ui/tabs/instances.rs @@ -299,14 +299,15 @@ const fn status_meta( status: InstanceStatus, theme: &Theme, ) -> (ratatui::style::Color, &'static str) { - match status { - InstanceStatus::Running => (theme.ok, "RUNNING"), - InstanceStatus::Ready => (theme.ok, "READY"), - InstanceStatus::Starting => (theme.warn, "STARTING"), - InstanceStatus::Stopped => (theme.err, "STOPPED"), - InstanceStatus::Error => (theme.err, "ERROR"), - InstanceStatus::Unknown => (theme.muted, "UNKNOWN"), - } + // The text label (incl. the DOWNLOADING/LOADING/WARMUP startup phases) is + // owned by `InstanceStatus::label`; only the color is chosen here. + let color = match status { + InstanceStatus::Ready | InstanceStatus::Running => theme.ok, + InstanceStatus::Starting { .. } => theme.warn, + InstanceStatus::Stopped | InstanceStatus::Error => theme.err, + InstanceStatus::Unknown => theme.muted, + }; + (color, status.label()) } /// Map an instance status to a bento box role so unselected cards take a @@ -314,7 +315,7 @@ const fn status_meta( const fn status_role(status: InstanceStatus) -> BoxRole { match status { InstanceStatus::Running | InstanceStatus::Ready => BoxRole::Success, - InstanceStatus::Starting => BoxRole::Warning, + InstanceStatus::Starting { .. } => BoxRole::Warning, InstanceStatus::Stopped | InstanceStatus::Error => BoxRole::Danger, InstanceStatus::Unknown => BoxRole::Muted, } @@ -799,7 +800,20 @@ mod tests { fn status_meta_maps_each_variant() { let theme = Theme::default_dark(); assert_eq!(status_meta(InstanceStatus::Running, &theme).1, "RUNNING"); - assert_eq!(status_meta(InstanceStatus::Starting, &theme).1, "STARTING"); + assert_eq!( + status_meta(InstanceStatus::Starting { phase: None }, &theme).1, + "STARTING" + ); + assert_eq!( + status_meta( + InstanceStatus::Starting { + phase: Some(rocm_dash_core::metrics::StartupPhase::Downloading) + }, + &theme + ) + .1, + "DOWNLOADING" + ); assert_eq!(status_meta(InstanceStatus::Stopped, &theme).1, "STOPPED"); assert_eq!(status_meta(InstanceStatus::Error, &theme).1, "ERROR"); assert_eq!(status_meta(InstanceStatus::Unknown, &theme).1, "UNKNOWN"); From b40b16dfeac05e8dd61f11263a2144f5201e9b6e Mon Sep 17 00:00:00 2001 From: Michael Roy Date: Fri, 10 Jul 2026 20:45:44 -0700 Subject: [PATCH 2/4] fix(dash): honor startup_phase while a managed service is still `running` 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 ` 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 --- THIRD_PARTY_NOTICES.txt | 2 +- crates/rocm-dash-core/src/protocol.rs | 7 +- crates/rocm-dash-daemon/src/registry.rs | 105 +++++++++++++++++++----- 3 files changed, 93 insertions(+), 21 deletions(-) diff --git a/THIRD_PARTY_NOTICES.txt b/THIRD_PARTY_NOTICES.txt index 6f051d11..2988c8f6 100644 --- a/THIRD_PARTY_NOTICES.txt +++ b/THIRD_PARTY_NOTICES.txt @@ -1725,7 +1725,6 @@ Apache License 2.0 The following component(s) are licensed under Apache License 2.0: - rustls-platform-verifier 0.7.0 - - ureq 2.12.1 Apache License Version 2.0, January 2004 @@ -5642,6 +5641,7 @@ The following component(s) are licensed under Apache License 2.0: - unicode-truncate 2.0.1 - unicode-width 0.1.14 - unicode-width 0.2.0 + - ureq 2.12.1 - url 2.5.8 - uuid 1.23.3 - wasi 0.11.1+wasi-snapshot-preview1 diff --git a/crates/rocm-dash-core/src/protocol.rs b/crates/rocm-dash-core/src/protocol.rs index 47ea677e..01e9888b 100644 --- a/crates/rocm-dash-core/src/protocol.rs +++ b/crates/rocm-dash-core/src/protocol.rs @@ -10,7 +10,12 @@ use serde::{Deserialize, Serialize}; use crate::bench_schema::BenchmarkRow; use crate::metrics::{Instance, Snapshot}; -pub const PROTOCOL_VERSION: u32 = 1; +// Bumped for the `InstanceStatus::Starting` shape change (unit variant -> +// `{ phase: Option }`): the external-tag JSON for +// `Event::InstanceDiscovered`/`Snapshot` changes from the string `"Starting"` +// to an object `{"Starting":{"phase":...}}`, which a client on the previous +// version cannot deserialize. +pub const PROTOCOL_VERSION: u32 = 2; /// Sent by the TUI to the daemon. One per line. #[derive(Debug, Clone, Serialize, Deserialize)] diff --git a/crates/rocm-dash-daemon/src/registry.rs b/crates/rocm-dash-daemon/src/registry.rs index e806be27..6b9048b2 100644 --- a/crates/rocm-dash-daemon/src/registry.rs +++ b/crates/rocm-dash-daemon/src/registry.rs @@ -87,15 +87,26 @@ pub fn is_scrapeable_status(status: &str) -> bool { /// `startup_phase`) onto the dashboard's `InstanceStatus`. Only called after /// `is_scrapeable_status` has already filtered out `failed`/`stopped`/unrecognized /// strings, so the `_` fallback arm is unreachable in practice but kept total -/// for safety. The `startup_phase` token is only honored for the `Starting` -/// states; it is meaningless once a service is `ready`/`running`. +/// for safety. +/// +/// **`running` + `startup_phase` overlap:** the rocm-cli supervisor +/// (`apps/rocmd/src/lib.rs::supervise_service`) flips the on-disk record to +/// `running` immediately after spawning the engine child — *before* it starts +/// polling and parsing `startup_phase` from the serve log — and only clears +/// `startup_phase` once the service reaches `ready`. So a record can be +/// `running` with a `startup_phase` still set while the model is downloading +/// or loading; that combination must still surface as `Starting{phase}`, not +/// `Running`, or the coarse phase never renders during a real cold start. A +/// `running` record without a phase (the steady-state case) still maps to +/// plain `Running`. fn instance_status_for_record(status: &str, startup_phase: Option<&str>) -> InstanceStatus { + let phase = || startup_phase.and_then(StartupPhase::from_token); match status.trim().to_ascii_lowercase().as_str() { "ready" => InstanceStatus::Ready, - "running" => InstanceStatus::Running, - "starting" | "recovering" => InstanceStatus::Starting { - phase: startup_phase.and_then(StartupPhase::from_token), - }, + "running" => startup_phase.map_or(InstanceStatus::Running, |_| InstanceStatus::Starting { + phase: phase(), + }), + "starting" | "recovering" => InstanceStatus::Starting { phase: phase() }, _ => InstanceStatus::Unknown, } } @@ -344,37 +355,93 @@ mod tests { #[test] fn discovered_from_record_surfaces_startup_phase_while_starting() { - // A `starting` record with a parsed phase must carry that phase into the - // instance status so the dashboard can show DOWNLOADING/LOADING/WARMUP. + // A `starting`/`recovering` record with a parsed phase must carry that + // phase into the instance status so the dashboard can show + // DOWNLOADING/LOADING/WARMUP. + for status in ["starting", "recovering"] { + let rec: ServiceRecord = serde_json::from_str(&format!( + r#"{{ + "service_id": "svc-load", "engine": "vllm", "model_ref": "m", + "canonical_model_id": "m", "host": "127.0.0.1", "port": 8000, + "status": "{status}", "startup_phase": "loading", + "created_at_unix_ms": 1 + }}"# + )) + .unwrap(); + let svc = discovered_from_record(&rec).expect("record is scrapeable"); + assert_eq!( + svc.status, + InstanceStatus::Starting { + phase: Some(StartupPhase::Loading) + }, + "status {status:?}" + ); + } + + // An unrecognized phase token degrades to a phase-less Starting. let rec: ServiceRecord = serde_json::from_str( r#"{ - "service_id": "svc-load", "engine": "vllm", "model_ref": "m", + "service_id": "svc-x", "engine": "vllm", "model_ref": "m", "canonical_model_id": "m", "host": "127.0.0.1", "port": 8000, - "status": "starting", "startup_phase": "loading", + "status": "starting", "startup_phase": "bogus", "created_at_unix_ms": 1 }"#, ) .unwrap(); let svc = discovered_from_record(&rec).expect("starting record is scrapeable"); + assert_eq!(svc.status, InstanceStatus::Starting { phase: None }); + } + + #[test] + fn discovered_from_record_surfaces_startup_phase_while_running() { + // The real producer (`apps/rocmd/src/lib.rs::supervise_service`) flips + // the on-disk record to `running` *before* it starts polling and + // parsing `startup_phase` from the serve log, and only clears + // `startup_phase` once the service reaches `ready`. Exercise that + // exact `running` + `startup_phase` shape end to end through + // `load_service_records` + `discovered_from_record` (not a + // hand-picked shape the producer never writes) to prove the coarse + // phase actually renders during a real cold start. + let dir = test_dir("running-phase"); + fs::write( + dir.join("svc.json"), + r#"{ + "service_id": "svc-cold", "engine": "vllm", "model_ref": "m", + "canonical_model_id": "m", "host": "127.0.0.1", "port": 8000, + "status": "running", "startup_phase": "downloading", + "created_at_unix_ms": 1 + }"#, + ) + .unwrap(); + + let records = load_service_records(&dir); + let svc = discovered_from_record(&records[0]).expect("running record is scrapeable"); assert_eq!( svc.status, InstanceStatus::Starting { - phase: Some(StartupPhase::Loading) - } + phase: Some(StartupPhase::Downloading) + }, + "a `running` record with a live startup_phase must still render as Starting" ); + let _ = fs::remove_dir_all(&dir); - // An unrecognized phase token degrades to a phase-less Starting. - let rec: ServiceRecord = serde_json::from_str( + // Once the supervisor clears `startup_phase` (service reached + // `ready`... though status flips to `ready` too — this proves the + // `running`-with-no-phase steady state still maps to plain Running). + let dir = test_dir("running-no-phase"); + fs::write( + dir.join("svc.json"), r#"{ - "service_id": "svc-x", "engine": "vllm", "model_ref": "m", + "service_id": "svc-steady", "engine": "vllm", "model_ref": "m", "canonical_model_id": "m", "host": "127.0.0.1", "port": 8000, - "status": "starting", "startup_phase": "bogus", - "created_at_unix_ms": 1 + "status": "running", "created_at_unix_ms": 1 }"#, ) .unwrap(); - let svc = discovered_from_record(&rec).expect("starting record is scrapeable"); - assert_eq!(svc.status, InstanceStatus::Starting { phase: None }); + let records = load_service_records(&dir); + let svc = discovered_from_record(&records[0]).expect("running record is scrapeable"); + assert_eq!(svc.status, InstanceStatus::Running); + let _ = fs::remove_dir_all(&dir); } #[test] From 43ad7183146ad414d8c2267df841707df23e596e Mon Sep 17 00:00:00 2001 From: Michael Roy Date: Fri, 10 Jul 2026 21:21:48 -0700 Subject: [PATCH 3/4] chore(notices): revert accidental THIRD_PARTY_NOTICES regeneration (no dep change) Signed-off-by: Michael Roy --- THIRD_PARTY_NOTICES.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/THIRD_PARTY_NOTICES.txt b/THIRD_PARTY_NOTICES.txt index 2988c8f6..6f051d11 100644 --- a/THIRD_PARTY_NOTICES.txt +++ b/THIRD_PARTY_NOTICES.txt @@ -1725,6 +1725,7 @@ Apache License 2.0 The following component(s) are licensed under Apache License 2.0: - rustls-platform-verifier 0.7.0 + - ureq 2.12.1 Apache License Version 2.0, January 2004 @@ -5641,7 +5642,6 @@ The following component(s) are licensed under Apache License 2.0: - unicode-truncate 2.0.1 - unicode-width 0.1.14 - unicode-width 0.2.0 - - ureq 2.12.1 - url 2.5.8 - uuid 1.23.3 - wasi 0.11.1+wasi-snapshot-preview1 From 7e1d04ec2f54cdf91d2cc31d8cba3a32bf3f9edf Mon Sep 17 00:00:00 2001 From: Michael Roy Date: Fri, 10 Jul 2026 21:53:07 -0700 Subject: [PATCH 4/4] test(dash): pin cross-crate startup_phase serde contract; keep running 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 --- Cargo.lock | 1 + crates/rocm-dash-daemon/Cargo.toml | 5 ++ crates/rocm-dash-daemon/src/registry.rs | 109 ++++++++++++++++++++++-- 3 files changed, 107 insertions(+), 8 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 59dd53de..7d25aed1 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3284,6 +3284,7 @@ dependencies = [ "clap", "color-eyre", "futures", + "rocm-core", "rocm-dash-collectors", "rocm-dash-core", "serde", diff --git a/crates/rocm-dash-daemon/Cargo.toml b/crates/rocm-dash-daemon/Cargo.toml index 7dfa8c3e..aecc9919 100644 --- a/crates/rocm-dash-daemon/Cargo.toml +++ b/crates/rocm-dash-daemon/Cargo.toml @@ -31,3 +31,8 @@ chrono = { version = "0.4", features = ["serde"] } [dev-dependencies] tempfile = "3" +# Test-only: pins the cross-crate `startup_phase` serde contract by +# round-tripping the producer's real `ManagedServiceRecord` into this crate's +# read-only `ServiceRecord`. Workspace-local path crate; adds no third-party +# dependency to the tree. +rocm-core = { path = "../rocm-core" } diff --git a/crates/rocm-dash-daemon/src/registry.rs b/crates/rocm-dash-daemon/src/registry.rs index 6b9048b2..06568a6b 100644 --- a/crates/rocm-dash-daemon/src/registry.rs +++ b/crates/rocm-dash-daemon/src/registry.rs @@ -95,18 +95,22 @@ pub fn is_scrapeable_status(status: &str) -> bool { /// polling and parsing `startup_phase` from the serve log — and only clears /// `startup_phase` once the service reaches `ready`. So a record can be /// `running` with a `startup_phase` still set while the model is downloading -/// or loading; that combination must still surface as `Starting{phase}`, not -/// `Running`, or the coarse phase never renders during a real cold start. A -/// `running` record without a phase (the steady-state case) still maps to -/// plain `Running`. +/// or loading; a *recognized* phase there must surface as `Starting{phase}`, +/// not `Running`, or the coarse phase never renders during a real cold start. +/// +/// A `running` record with **no** phase (the steady-state case) — or with a +/// phase token we don't recognize (a forward/foreign value from a newer +/// producer) — is a genuinely serving instance and stays plain `Running`. An +/// unrecognized token must **not** downgrade it to a non-serving `Starting`; +/// only the pinned tokens (`downloading`/`loading`/`warmup`) do. fn instance_status_for_record(status: &str, startup_phase: Option<&str>) -> InstanceStatus { - let phase = || startup_phase.and_then(StartupPhase::from_token); + let phase = startup_phase.and_then(StartupPhase::from_token); match status.trim().to_ascii_lowercase().as_str() { "ready" => InstanceStatus::Ready, - "running" => startup_phase.map_or(InstanceStatus::Running, |_| InstanceStatus::Starting { - phase: phase(), + "running" => phase.map_or(InstanceStatus::Running, |phase| InstanceStatus::Starting { + phase: Some(phase), }), - "starting" | "recovering" => InstanceStatus::Starting { phase: phase() }, + "starting" | "recovering" => InstanceStatus::Starting { phase }, _ => InstanceStatus::Unknown, } } @@ -444,6 +448,95 @@ mod tests { let _ = fs::remove_dir_all(&dir); } + #[test] + fn discovered_running_with_unknown_phase_stays_serving() { + // Robustness against a forward/foreign phase token: a genuinely + // serving `running` instance whose `startup_phase` we don't recognize + // (e.g. a newer producer emits a token this dashboard predates) must + // stay `Running`/serving — never be downgraded to a non-serving + // `Starting`. Only the pinned tokens flip a `running` record to + // Starting; everything else leaves it serving. + let dir = test_dir("running-unknown-phase"); + fs::write( + dir.join("svc.json"), + r#"{ + "service_id": "svc-fwd", "engine": "vllm", "model_ref": "m", + "canonical_model_id": "m", "host": "127.0.0.1", "port": 8000, + "status": "running", "startup_phase": "quantizing", + "created_at_unix_ms": 1 + }"#, + ) + .unwrap(); + let records = load_service_records(&dir); + let svc = discovered_from_record(&records[0]).expect("running record is scrapeable"); + assert_eq!( + svc.status, + InstanceStatus::Running, + "an unrecognized phase token must not downgrade a running instance" + ); + assert!( + svc.status.is_serving(), + "a running instance with an unknown phase token is still serving" + ); + let _ = fs::remove_dir_all(&dir); + } + + #[test] + fn managed_service_record_startup_phase_survives_cross_crate_serde() { + // Cross-crate wire contract: the rocm-cli supervisor persists a + // `rocm_core::ManagedServiceRecord`, and this daemon reads it back as + // the local `ServiceRecord`. The `startup_phase` handoff currently + // relies on both structs naming the field identically; nothing but a + // hand-written JSON literal pins that today. Serialize the *producer's* + // real type and deserialize it as *our* type so a future + // `#[serde(rename)]` on either side fails here instead of silently + // dropping the phase on the floor at runtime. + use rocm_core::{AppPaths, ManagedServiceRecord}; + + let root = PathBuf::from("/tmp/rocm-xcrate-test"); + let paths = AppPaths { + config_dir: root.join("config"), + data_dir: root.join("data"), + cache_dir: root.join("cache"), + }; + let mut produced = ManagedServiceRecord::new( + &paths, + "svc-xcrate", + "vllm", + "meta-llama/Llama-3.1-8B", + "meta-llama/Llama-3.1-8B", + "127.0.0.1", + 8000, + "serve", + 4242, + None, + None, + None, + ); + produced.status = "running".to_owned(); + produced.startup_phase = Some("loading".to_owned()); + + let json = serde_json::to_string(&produced).expect("producer record serializes"); + let consumed: ServiceRecord = + serde_json::from_str(&json).expect("daemon record deserializes the producer's JSON"); + + assert_eq!( + consumed.startup_phase.as_deref(), + Some("loading"), + "startup_phase must survive the producer→daemon serde handoff by field name" + ); + // The field name is the actual contract; prove the map end to end + // resolves the live phase (guards a rename on either side *and* the + // consumer-side mapping in one shot). + let svc = discovered_from_record(&consumed).expect("running record is scrapeable"); + assert_eq!( + svc.status, + InstanceStatus::Starting { + phase: Some(StartupPhase::Loading) + } + ); + } + #[test] fn discovered_skips_zero_port() { // A record whose port field is absent (serde default 0) is not a real