From f2e896438f4c2d30c5b3626b9f09db496a8693c4 Mon Sep 17 00:00:00 2001 From: Roman Inflianskas Date: Thu, 27 Aug 2026 10:47:20 +0000 Subject: [PATCH 1/6] test(e2e): activate a runtime explicitly in the shared tree The shared pre-warm tree used to hold exactly one runtime, so serve scenarios could rely on the CLI auto-selecting it. That auto-selection is deliberately all-or-nothing: with more than one ready runtime installed the CLI refuses to guess. Once the pre-warm started adopting a newer runtime side by side with the old one, every GPU serve scenario began failing with "no active ROCm runtime is configured", and the precondition step still passed because a runtime *was* present. Name the runtime instead of relying on the count. The pre-warm's own activation is recorded in /active.json, which lives inside the shared tree and so is visible through the symlink even though every scenario keeps its own config dir. Read it back, fall back to the sole registry manifest, and refuse to guess otherwise -- activating an arbitrary one of several would serve against an unintended ROCm version and pass. Signed-off-by: Roman Inflianskas --- tests/e2e-cucumber/src/lib.rs | 1 + tests/e2e-cucumber/src/shared_runtime.rs | 198 ++++++++++++++++++ tests/e2e-cucumber/tests/e2e.rs | 42 +++- tests/e2e-cucumber/tests/e2e/runtime_steps.rs | 10 + 4 files changed, 245 insertions(+), 6 deletions(-) create mode 100644 tests/e2e-cucumber/src/shared_runtime.rs diff --git a/tests/e2e-cucumber/src/lib.rs b/tests/e2e-cucumber/src/lib.rs index 7fccc5695..0d01ba4de 100644 --- a/tests/e2e-cucumber/src/lib.rs +++ b/tests/e2e-cucumber/src/lib.rs @@ -13,6 +13,7 @@ pub mod panic_capture; pub mod reader_failure; pub mod send_until; pub mod serve_log; +pub mod shared_runtime; use std::path::{Path, PathBuf}; diff --git a/tests/e2e-cucumber/src/shared_runtime.rs b/tests/e2e-cucumber/src/shared_runtime.rs new file mode 100644 index 000000000..ef8a3d902 --- /dev/null +++ b/tests/e2e-cucumber/src/shared_runtime.rs @@ -0,0 +1,198 @@ +// Copyright © Advanced Micro Devices, Inc., or its affiliates. +// +// SPDX-License-Identifier: MIT + +//! Pick which runtime a scenario should activate out of a shared pre-warm tree. +//! +//! Scenarios that only need *a* runtime present point their `data/runtimes` at a +//! shared, install-once tree (see `E2eWorld::use_shared_runtimes`). Serving out of +//! that tree used to need no further wiring: the CLI auto-selects a runtime when +//! exactly one is installed, and the tree held exactly one. +//! +//! That stopped being true once the pre-warm started adopting a newer runtime +//! side by side with the old one. The CLI's auto-selection is deliberately +//! all-or-nothing — with two installed it refuses to guess and serve fails with +//! "no active ROCm runtime is configured" — so a tree holding more than one +//! silently broke every GPU serve scenario. The count is not something the suite +//! controls: it follows whatever the upstream channel index has published. +//! +//! So the scenario names its runtime explicitly instead of relying on the count. +//! The pre-warm activates the runtime it installed and the CLI records that in +//! `/active.json`, which lives *inside* the shared tree and is therefore +//! visible through the symlink even though each scenario keeps its own config dir. +//! Reading it back is what makes selection deterministic for any tree contents. + +use std::path::Path; + +/// The runtime key a scenario should activate for a shared tree at `runtimes_dir`. +/// +/// `Some(key)` means "run `rocm runtimes activate `"; `None` means there is +/// nothing to name and the caller should leave selection to the CLI — either the +/// tree is empty (the scenario installs its own) or it holds exactly one runtime, +/// which the CLI already auto-selects. +/// +/// Prefers `active.json` because that records the runtime the pre-warm actually +/// installed and verified — but only when the registry still holds it, so a +/// marker left behind by a pruned runtime doesn't strand the scenario. Falls back +/// to the sole registry manifest so a tree written before the pre-warm learned to +/// activate still resolves. When neither names one runtime, returns `None` rather +/// than picking arbitrarily: choosing the wrong one of several would serve against +/// an unintended ROCm version, and a clear "no active runtime" failure beats a +/// silently mismatched pass. +#[must_use] +pub fn runtime_key_to_activate(runtimes_dir: &Path) -> Option { + let installed = registry_runtime_keys(runtimes_dir); + active_runtime_key(runtimes_dir) + .filter(|key| installed.iter().any(|installed| installed == key)) + .or_else(|| match installed.as_slice() { + [only] => Some(only.clone()), + _ => None, + }) +} + +/// The `runtime_key` recorded in `/active.json`, if it names one. +fn active_runtime_key(runtimes_dir: &Path) -> Option { + let text = std::fs::read_to_string(runtimes_dir.join("active.json")).ok()?; + let key = serde_json::from_str::(&text) + .ok()? + .get("runtime_key")? + .as_str()? + .trim() + .to_owned(); + (!key.is_empty()).then_some(key) +} + +/// Every runtime key present in the registry, empty when it is absent. +fn registry_runtime_keys(runtimes_dir: &Path) -> Vec { + let Ok(entries) = std::fs::read_dir(runtimes_dir.join("registry")) else { + return Vec::new(); + }; + entries + .filter_map(|entry| { + let path = entry.ok()?.path(); + if path.extension().is_some_and(|ext| ext == "json") { + // Runtime key = the manifest file stem, the same convention + // `capability::active_runtime_install_root` relies on. + path.file_stem()?.to_str().map(str::to_owned) + } else { + None + } + }) + .collect() +} + +#[cfg(test)] +mod tests { + use super::*; + + fn write(path: &Path, body: &str) { + std::fs::create_dir_all(path.parent().expect("path has a parent")).expect("create dir"); + std::fs::write(path, body).expect("write file"); + } + + fn manifest(dir: &Path, key: &str) { + write(&dir.join("registry").join(format!("{key}.json")), "{}"); + } + + fn active(dir: &Path, key: &str) { + write( + &dir.join("active.json"), + &format!(r#"{{"runtime_key": "{key}"}}"#), + ); + } + + /// The regression this module exists for: a tree the pre-warm grew a second + /// runtime in must still name one, or every GPU serve scenario fails with + /// "no active ROCm runtime is configured". + #[test] + fn names_the_active_runtime_when_several_are_installed() { + let tmp = tempfile::TempDir::with_prefix("shared-runtime-").expect("temp dir"); + let dir = tmp.path(); + manifest(dir, "release-wheel-gfx94x-dcgpu-7-13-0"); + manifest(dir, "release-wheel-multi-arch-7-14-0"); + active(dir, "release-wheel-multi-arch-7-14-0"); + + assert_eq!( + runtime_key_to_activate(dir).as_deref(), + Some("release-wheel-multi-arch-7-14-0") + ); + } + + /// A tree from before the pre-warm activated: one runtime, no marker. The + /// CLI auto-selects here, so naming it is optional — but resolving it keeps + /// the step's behaviour identical whether or not a marker was written. + #[test] + fn falls_back_to_the_sole_manifest_without_a_marker() { + let tmp = tempfile::TempDir::with_prefix("shared-runtime-").expect("temp dir"); + let dir = tmp.path(); + manifest(dir, "release-wheel-gfx94x-dcgpu-7-13-0"); + + assert_eq!( + runtime_key_to_activate(dir).as_deref(), + Some("release-wheel-gfx94x-dcgpu-7-13-0") + ); + } + + /// Several runtimes and no marker: refuse to guess. Activating an arbitrary + /// one would serve against an unintended ROCm version and pass, which is + /// worse than the CLI's own explicit failure. + #[test] + fn refuses_to_guess_between_several_without_a_marker() { + let tmp = tempfile::TempDir::with_prefix("shared-runtime-").expect("temp dir"); + let dir = tmp.path(); + manifest(dir, "release-wheel-gfx94x-dcgpu-7-13-0"); + manifest(dir, "release-wheel-multi-arch-7-14-0"); + + assert_eq!(runtime_key_to_activate(dir), None); + } + + /// An empty tree is the first scenario on a cold runner; it installs its own + /// runtime, so there is nothing to activate beforehand. + #[test] + fn names_nothing_for_an_empty_tree() { + let tmp = tempfile::TempDir::with_prefix("shared-runtime-").expect("temp dir"); + assert_eq!(runtime_key_to_activate(tmp.path()), None); + } + + /// A marker that survives the runtime it names (a prune, a hand-cleaned tree) + /// must not strand the scenario: fall through to the registry. + #[test] + fn ignores_a_marker_naming_no_installed_runtime() { + let tmp = tempfile::TempDir::with_prefix("shared-runtime-").expect("temp dir"); + let dir = tmp.path(); + manifest(dir, "release-wheel-gfx94x-dcgpu-7-13-0"); + active(dir, "release-wheel-multi-arch-7-14-0"); + + assert_eq!( + runtime_key_to_activate(dir).as_deref(), + Some("release-wheel-gfx94x-dcgpu-7-13-0") + ); + } + + /// Corrupt or half-written markers are treated as absent, not fatal: the + /// registry still answers the question. + #[test] + fn treats_an_unreadable_marker_as_absent() { + let tmp = tempfile::TempDir::with_prefix("shared-runtime-").expect("temp dir"); + let dir = tmp.path(); + manifest(dir, "release-wheel-gfx94x-dcgpu-7-13-0"); + write(&dir.join("active.json"), "{ not json"); + + assert_eq!( + runtime_key_to_activate(dir).as_deref(), + Some("release-wheel-gfx94x-dcgpu-7-13-0") + ); + } + + /// A marker with a blank key names nothing. + #[test] + fn treats_a_blank_marker_key_as_absent() { + let tmp = tempfile::TempDir::with_prefix("shared-runtime-").expect("temp dir"); + let dir = tmp.path(); + manifest(dir, "release-wheel-gfx94x-dcgpu-7-13-0"); + manifest(dir, "release-wheel-multi-arch-7-14-0"); + active(dir, " "); + + assert_eq!(runtime_key_to_activate(dir), None); + } +} diff --git a/tests/e2e-cucumber/tests/e2e.rs b/tests/e2e-cucumber/tests/e2e.rs index 9a28e44d3..e75729b43 100644 --- a/tests/e2e-cucumber/tests/e2e.rs +++ b/tests/e2e-cucumber/tests/e2e.rs @@ -141,8 +141,8 @@ fn shared_uv_cache_dir() -> Option { validated_shared_dir("E2E_SHARED_UV_CACHE_DIR") } -/// A persistent directory holding ONE installed managed-runtime tree -/// (`runtimes/registry/*` + the TheRock venv) shared across scenarios that only +/// A persistent directory holding the installed managed-runtime trees +/// (`runtimes/registry/*` + the TheRock venvs) shared across scenarios that only /// need *a* runtime active. Set by CI (`E2E_SHARED_RUNTIMES_DIR`) on the runner's /// persistent disk; unset for local runs, where every scenario installs its own. /// @@ -154,10 +154,14 @@ fn shared_uv_cache_dir() -> Option { /// (see [`E2eWorld::use_shared_runtimes`]) so the install happens once per runner. /// Scenarios that ASSERT a clean slate ("a machine with no CLI-managed runtimes", /// "Installing the SDK") deliberately do NOT opt in — they keep their empty -/// isolated runtimes dir. A serve resolves the shared runtime via -/// `single_ready_runtime` (no active-key wiring needed) and `runtimes list` -/// reports it `status=ready`, so both the precondition and serve are satisfied -/// (verified by hand on MI300X: serve + chat completion through a symlinked tree). +/// isolated runtimes dir. +/// +/// The tree may hold MORE THAN ONE runtime: `xtask e2e-prewarm` installs a newer +/// one side by side when the channel index publishes it, so the count tracks +/// upstream releases rather than anything the suite controls. A serve therefore +/// cannot lean on the CLI's `single_ready_runtime` fallback, which deliberately +/// refuses to guess once two are installed — the scenario names its runtime +/// explicitly instead (see [`E2eWorld::activate_shared_runtime`]). fn shared_runtimes_dir() -> Option { validated_shared_dir("E2E_SHARED_RUNTIMES_DIR") } @@ -362,6 +366,32 @@ impl E2eWorld { Ok(real) } + /// Point this scenario's config at a specific runtime in the shared tree. + /// + /// The shared tree is reached through a symlinked `data/runtimes`, but the + /// config dir stays per-scenario — so the activation `xtask e2e-prewarm` + /// performed is invisible here and every scenario starts with no active + /// runtime. That was harmless only while the CLI could auto-select, which it + /// stops doing as soon as the tree holds a second runtime (serve then fails + /// with "no active ROCm runtime is configured"). Re-activating from the + /// tree's own `active.json` makes the choice explicit and independent of how + /// many runtimes the pre-warm has accumulated. + /// + /// Best-effort and deliberately quiet: a no-op for local runs (no shared + /// tree), for a tree with nothing to name, and if the activation itself + /// fails. Nothing here should turn a scenario red on its own — the + /// precondition's own assertion below is what reports a genuinely unusable + /// runtime, with the full `runtimes list` output. + pub fn activate_shared_runtime(&self) { + let Some(shared) = shared_runtimes_dir() else { + return; + }; + let Some(key) = e2e_cucumber::shared_runtime::runtime_key_to_activate(&shared) else { + return; + }; + let _ = run_rocm(self, &["runtimes", "activate", &key]); + } + /// Plant a fake pre-existing (non-CLI) ROCm install in the scenario's isolated /// tree and record its path so `isolate_cmd` exports it as `ROCM_PATH`. The /// CLI's `detect_legacy_rocm_summary` treats any directory containing a known diff --git a/tests/e2e-cucumber/tests/e2e/runtime_steps.rs b/tests/e2e-cucumber/tests/e2e/runtime_steps.rs index bec98c56c..fb018a036 100644 --- a/tests/e2e-cucumber/tests/e2e/runtime_steps.rs +++ b/tests/e2e-cucumber/tests/e2e/runtime_steps.rs @@ -129,6 +129,12 @@ async fn setup_active_runtime(world: &mut E2eWorld) { if stdout.contains("installed: none") { crate::run_rocm_ok(world, &["install", "sdk"]); } + // Name the runtime rather than leaving the CLI to infer it: the shared tree + // grows a second runtime whenever the channel index publishes one, and the + // CLI refuses to auto-select from more than one (see + // `E2eWorld::activate_shared_runtime`). Without this the step still passes — + // a runtime IS present — and the serve that follows fails instead. + world.activate_shared_runtime(); let (stdout, _, _) = crate::run_rocm(world, &["runtimes", "list"]); assert!( !stdout.contains("installed: none"), @@ -147,6 +153,10 @@ async fn setup_runtime_with_engine(world: &mut E2eWorld) { if stdout.contains("installed: none") { crate::run_rocm_ok(world, &["install", "sdk"]); } + // Same reason as `a managed runtime is active`: pin the runtime explicitly so + // the engine lookup and the serve that follows resolve the same one whatever + // the shared tree happens to hold. + world.activate_shared_runtime(); assert_engine_ready(world); } From 61e9bae420daff51fbadbee04f6726bd71352864 Mon Sep 17 00:00:00 2001 From: Roman Inflianskas Date: Thu, 27 Aug 2026 11:27:29 +0000 Subject: [PATCH 2/6] test(e2e): report on the active runtime, not an arbitrary one Follow-up to the previous commit, from review. The shared tree holding more than one runtime breaks three more places, all the same way: they pick whichever runtime came first and present it as the one in use. - The report attributed a run to an arbitrary manifest's ROCm version when `active.json` named none, so it could name a version the run did not serve on. Report nothing instead -- absent reads as unknown, wrong reads as fact -- and resolve the runtime the way the scenarios do. - The update-freshness scenario asserted on the first `runtime` line, which is the newest rather than the active one. Select by key. - A failed or skipped activation was silent, and neither precondition assertion can detect it: `installed: none` reports an empty registry, not an unset active key, and `engines list` scans every manifest regardless of which is active. Say what happened on stderr so the serve failure that follows is diagnosable. Also corrects two comments that claimed checks the code does not perform. Signed-off-by: Roman Inflianskas --- docs/ci-hardware-testing.md | 11 +- tests/e2e-cucumber/src/capability.rs | 104 ++++++++++++------ tests/e2e-cucumber/src/shared_runtime.rs | 6 +- tests/e2e-cucumber/tests/e2e.rs | 37 ++++++- tests/e2e-cucumber/tests/e2e/runtime_steps.rs | 42 +++++-- 5 files changed, 150 insertions(+), 50 deletions(-) diff --git a/docs/ci-hardware-testing.md b/docs/ci-hardware-testing.md index c5886620f..9369f6c42 100644 --- a/docs/ci-hardware-testing.md +++ b/docs/ci-hardware-testing.md @@ -142,10 +142,19 @@ gh workflow run e2e-selfhosted.yml --ref -f platform=app-dev-gpu ## The shared pre-warmed runtime Nearly every GPU serve scenario points its `data/runtimes` at one shared, -pre-warmed managed runtime (`E2E_SHARED_RUNTIMES_DIR`), so a multi-GiB +pre-warmed managed runtime tree (`E2E_SHARED_RUNTIMES_DIR`), so a multi-GiB `rocm install sdk` happens once per runner instead of once per scenario. The tree lives on the runner's persistent workspace and survives `git clean`. +The tree may hold **more than one** runtime — the pre-warm installs a newer one +side by side when the channel index publishes it (below) — so scenarios must not +rely on the CLI auto-selecting a runtime, which it deliberately declines to do +once two are installed. Each scenario keeps its own config dir, so the pre-warm's +`--activate` is invisible to it; the precondition steps re-activate from the +tree's own `active.json`, which lives inside the shared tree and is therefore +visible through the symlink. Without that, a serve fails with `no active ROCm +runtime is configured` while the precondition still passes. + It is a **cache with invalidation**, not a one-shot install. Each self-hosted lane calls ```bash diff --git a/tests/e2e-cucumber/src/capability.rs b/tests/e2e-cucumber/src/capability.rs index 9ca0d6a0a..19d48aa65 100644 --- a/tests/e2e-cucumber/src/capability.rs +++ b/tests/e2e-cucumber/src/capability.rs @@ -196,8 +196,16 @@ pub fn collect_versions(runtimes_dir: Option<&std::path::Path>) -> PlatformVersi } /// Read the active managed runtime's `(version, install_root)` from the runtimes -/// registry: prefer the runtime named by `active.json`, else the sole installed -/// manifest. Returns `None` when nothing is installed. +/// registry. Returns `None` when the tree names no single runtime. +/// +/// Which runtime that is comes from [`crate::shared_runtime::runtime_key_to_activate`], +/// the same answer the scenarios activate — so the version this report attributes +/// a run to is the version the run actually served on. This used to fall back to +/// the first `read_dir` entry when `active.json` named nothing, which was a +/// coin flip as soon as the pre-warm started keeping a newer runtime alongside +/// the old one: the report could name one ROCm version while the serve used +/// another. Reporting no version is the better failure — an absent field reads +/// as unknown, a wrong one reads as fact. /// /// The install_root is resolved from `runtimes_dir` (the shared tree we were /// handed) as `/wheel/`, NOT from the manifest's own @@ -210,41 +218,18 @@ pub fn collect_versions(runtimes_dir: Option<&std::path::Path>) -> PlatformVersi fn active_runtime_install_root( runtimes_dir: &std::path::Path, ) -> Option<(String, std::path::PathBuf)> { - let registry = runtimes_dir.join("registry"); - let entries: Vec = std::fs::read_dir(®istry) - .ok()? - .filter_map(|e| e.ok().map(|e| e.path())) - .filter(|p| p.extension().is_some_and(|x| x == "json")) - .collect(); - // Prefer the active runtime's key if active.json names one. - let active_key = std::fs::read_to_string(runtimes_dir.join("active.json")) - .ok() - .and_then(|t| { - serde_json::from_str::(&t) - .ok()? - .get("runtime_key")? - .as_str() - .map(str::to_owned) - }); - let pick = entries - .iter() - .find(|p| { - active_key - .as_deref() - .is_some_and(|k| p.file_stem().and_then(|s| s.to_str()) == Some(k)) - }) - .or_else(|| entries.first())?; + let key = crate::shared_runtime::runtime_key_to_activate(runtimes_dir)?; + let manifest = runtimes_dir.join("registry").join(format!("{key}.json")); let json: serde_json::Value = - serde_json::from_str(&std::fs::read_to_string(pick).ok()?).ok()?; + serde_json::from_str(&std::fs::read_to_string(manifest).ok()?).ok()?; let version = json.get("version")?.as_str()?.to_owned(); - // Runtime key = the manifest file stem (e.g. release-wheel-gfx1151-7-13-0). - let key = pick.file_stem().and_then(|s| s.to_str()); // Resolve the root inside the shared tree first; fall back to the manifest's // recorded install_root only if that derived path doesn't exist. - let derived = key.map(|k| runtimes_dir.join("wheel").join(k)); - let root = match derived { - Some(d) if d.is_dir() => d, - _ => std::path::PathBuf::from(json.get("install_root")?.as_str()?), + let derived = runtimes_dir.join("wheel").join(&key); + let root = if derived.is_dir() { + derived + } else { + std::path::PathBuf::from(json.get("install_root")?.as_str()?) }; Some((version, root)) } @@ -789,4 +774,57 @@ Local model engines "strix-halo-wsl" ); } + + fn write_manifest(runtimes_dir: &std::path::Path, key: &str, version: &str) { + let registry = runtimes_dir.join("registry"); + std::fs::create_dir_all(®istry).expect("create registry"); + let install_root = runtimes_dir.join("wheel").join(key); + std::fs::create_dir_all(&install_root).expect("create install root"); + std::fs::write( + registry.join(format!("{key}.json")), + serde_json::json!({ + "runtime_key": key, + "version": version, + "install_root": install_root, + }) + .to_string(), + ) + .expect("write manifest"); + } + + /// The report must name the ROCm version the run actually served on. The + /// pre-warm keeps a newer runtime beside the old one, so picking whichever + /// manifest `read_dir` yielded first could attribute a run to the version it + /// did NOT use — and read as fact. + #[test] + fn reports_the_active_runtimes_version_when_several_are_installed() { + let tmp = tempfile::TempDir::with_prefix("capability-").expect("temp dir"); + let dir = tmp.path(); + write_manifest(dir, "release-wheel-gfx94x-dcgpu-7-13-0", "7.13.0"); + write_manifest(dir, "release-wheel-multi-arch-7-14-0", "7.14.0"); + std::fs::write( + dir.join("active.json"), + r#"{"runtime_key": "release-wheel-multi-arch-7-14-0"}"#, + ) + .expect("write marker"); + + let (version, root) = active_runtime_install_root(dir).expect("a runtime is named"); + assert_eq!(version, "7.14.0"); + assert_eq!( + root, + dir.join("wheel").join("release-wheel-multi-arch-7-14-0") + ); + } + + /// Several runtimes and no marker: report nothing rather than guess. An + /// absent version reads as unknown; a wrong one reads as fact. + #[test] + fn reports_no_version_when_the_tree_names_no_runtime() { + let tmp = tempfile::TempDir::with_prefix("capability-").expect("temp dir"); + let dir = tmp.path(); + write_manifest(dir, "release-wheel-gfx94x-dcgpu-7-13-0", "7.13.0"); + write_manifest(dir, "release-wheel-multi-arch-7-14-0", "7.14.0"); + + assert!(active_runtime_install_root(dir).is_none()); + } } diff --git a/tests/e2e-cucumber/src/shared_runtime.rs b/tests/e2e-cucumber/src/shared_runtime.rs index ef8a3d902..45a984383 100644 --- a/tests/e2e-cucumber/src/shared_runtime.rs +++ b/tests/e2e-cucumber/src/shared_runtime.rs @@ -63,7 +63,11 @@ fn active_runtime_key(runtimes_dir: &Path) -> Option { } /// Every runtime key present in the registry, empty when it is absent. -fn registry_runtime_keys(runtimes_dir: &Path) -> Vec { +/// +/// Public so a caller that declines to activate can name what it found: "no +/// runtime to activate" is only actionable alongside the list it chose from. +#[must_use] +pub fn registry_runtime_keys(runtimes_dir: &Path) -> Vec { let Ok(entries) = std::fs::read_dir(runtimes_dir.join("registry")) else { return Vec::new(); }; diff --git a/tests/e2e-cucumber/tests/e2e.rs b/tests/e2e-cucumber/tests/e2e.rs index e75729b43..57c1d7643 100644 --- a/tests/e2e-cucumber/tests/e2e.rs +++ b/tests/e2e-cucumber/tests/e2e.rs @@ -377,19 +377,44 @@ impl E2eWorld { /// tree's own `active.json` makes the choice explicit and independent of how /// many runtimes the pre-warm has accumulated. /// - /// Best-effort and deliberately quiet: a no-op for local runs (no shared - /// tree), for a tree with nothing to name, and if the activation itself - /// fails. Nothing here should turn a scenario red on its own — the - /// precondition's own assertion below is what reports a genuinely unusable - /// runtime, with the full `runtimes list` output. + /// Also writes the shared tree's `active.json`, because for a symlinked + /// scenario that marker IS the shared one — the activation is not confined + /// to this scenario's config. + /// + /// Best-effort: a no-op for local runs (no shared tree), and it does not + /// fail the scenario when the tree names no runtime or the activation is + /// refused. Both of those leave the serve that follows failing with "no + /// active ROCm runtime is configured", and NEITHER precondition assertion + /// can see it — `installed: none` reports an empty registry, not an + /// unset active key, and `engines list` scans every manifest regardless of + /// which is active. So say what happened on stderr instead of failing here: + /// the serve's own failure is the one worth reading, and this is the line + /// that explains it. pub fn activate_shared_runtime(&self) { let Some(shared) = shared_runtimes_dir() else { return; }; let Some(key) = e2e_cucumber::shared_runtime::runtime_key_to_activate(&shared) else { + eprintln!( + "shared runtime: no runtime to activate in {}; a serve will fail unless exactly \ + one is installed. Installed: {:?}", + shared.display(), + e2e_cucumber::shared_runtime::registry_runtime_keys(&shared) + ); return; }; - let _ = run_rocm(self, &["runtimes", "activate", &key]); + let (stdout, stderr, rc) = run_rocm(self, &["runtimes", "activate", &key]); + if rc != 0 { + eprintln!( + "shared runtime: {}", + e2e_cucumber::cli_failure_report( + &["runtimes", "activate", &key], + rc, + &stdout, + &stderr + ) + ); + } } /// Plant a fake pre-existing (non-CLI) ROCm install in the scenario's isolated diff --git a/tests/e2e-cucumber/tests/e2e/runtime_steps.rs b/tests/e2e-cucumber/tests/e2e/runtime_steps.rs index fb018a036..d884189fa 100644 --- a/tests/e2e-cucumber/tests/e2e/runtime_steps.rs +++ b/tests/e2e-cucumber/tests/e2e/runtime_steps.rs @@ -153,9 +153,10 @@ async fn setup_runtime_with_engine(world: &mut E2eWorld) { if stdout.contains("installed: none") { crate::run_rocm_ok(world, &["install", "sdk"]); } - // Same reason as `a managed runtime is active`: pin the runtime explicitly so - // the engine lookup and the serve that follows resolve the same one whatever - // the shared tree happens to hold. + // Same reason as `a managed runtime is active`: pin the runtime explicitly, + // or the serve that follows refuses to pick one. Not for `assert_engine_ready` + // below — `engines list` scans every registered manifest and never consults + // the active key, which is exactly why it cannot stand in for this call. world.activate_shared_runtime(); assert_engine_ready(world); } @@ -285,6 +286,16 @@ async fn user_checks_for_updates(world: &mut E2eWorld) { world.cli_rc = Some(rc); } +/// The runtime key `runtimes list` reports as active, if any. +fn active_runtime_key(world: &mut E2eWorld) -> Option { + let (stdout, _, _) = crate::run_rocm(world, &["runtimes", "list"]); + let key = stdout + .lines() + .find_map(|line| line.trim().strip_prefix("active_runtime_key:"))? + .trim(); + (!key.is_empty() && key != "").then(|| key.to_owned()) +} + /// Freshness verdicts `runtime_update_plan` can emit, plus the degraded `error` /// form used when the index cannot be reached. `xtask e2e-prewarm` routes on /// exactly these, so a rename here must break this scenario rather than silently @@ -293,16 +304,29 @@ const UPDATE_STATUSES: [&str; 4] = ["up_to_date", "update_available", "ahead_of_ #[then("the report states the runtime's freshness against the channel index")] async fn assert_update_reports_freshness(world: &mut E2eWorld) { - let stdout = world.cli_output.as_deref().unwrap_or(""); + let stdout = world.cli_output.clone().unwrap_or_default(); + let stdout = stdout.as_str(); let rc = world.cli_rc.expect("no command was run"); assert_eq!(rc, 0, "`rocm update` failed:\n{stdout}"); // The line `xtask e2e-prewarm` parses: `runtime ... status=`. - let Some(line) = stdout - .lines() - .map(str::trim) - .find(|line| line.starts_with("runtime ")) - else { + // The report carries one such line per installed runtime, newest first, and + // the shared tree holds more than one — so select the ACTIVE runtime's line + // rather than whichever came first, or this scenario reports on a runtime the + // run never used. Falls back to the first line when nothing is active, which + // is the single-runtime case this scenario was written against. + let active = active_runtime_key(world); + let runtime_lines = || { + stdout + .lines() + .map(str::trim) + .filter(|line| line.starts_with("runtime ")) + }; + let line = active + .as_deref() + .and_then(|key| runtime_lines().find(|line| line.split_whitespace().nth(1) == Some(key))) + .or_else(|| runtime_lines().next()); + let Some(line) = line else { panic!("no `runtime …` line in the update report:\n{stdout}"); }; let status = line From 7d6a882368355d6c769188e35a2eb72e93538d54 Mon Sep 17 00:00:00 2001 From: Roman Inflianskas Date: Thu, 27 Aug 2026 12:40:12 +0000 Subject: [PATCH 3/6] test(e2e): skip a shared runtime whose install root is gone MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Activating a runtime by name replaced one silent failure with a loud one on the GPU lane: every activate died with "install root is missing" and pointed at a per-scenario temp dir that no longer existed. A scenario that installs through its own `data/runtimes` symlink records that temp dir as the runtime's install root, so the registry entry outlives the folder. The pre-warm evicts such entries, but its repair is deliberately non-fatal and skips the tree when `runtimes list` itself fails — which a poisoned entry makes it do. The tree therefore still holds corpses when a scenario reads it, and naming one guarantees a failed activate. Choose only among entries the CLI would accept, judged by the same rule the pre-warm's repair uses: an install root inside the shared tree is sound, one outside it is a corpse. Existence alone would be the wrong test — a foreign path that happens to exist would have the scenario serve against a runtime outside the tree. The diagnostic listing stays unfiltered, so a skipped entry is still named when nothing is left to activate. Signed-off-by: Roman Inflianskas --- tests/e2e-cucumber/src/shared_runtime.rs | 134 ++++++++++++++++++++++- 1 file changed, 133 insertions(+), 1 deletion(-) diff --git a/tests/e2e-cucumber/src/shared_runtime.rs b/tests/e2e-cucumber/src/shared_runtime.rs index 45a984383..ab6b160f9 100644 --- a/tests/e2e-cucumber/src/shared_runtime.rs +++ b/tests/e2e-cucumber/src/shared_runtime.rs @@ -21,6 +21,11 @@ //! `/active.json`, which lives *inside* the shared tree and is therefore //! visible through the symlink even though each scenario keeps its own config dir. //! Reading it back is what makes selection deterministic for any tree contents. +//! +//! Naming a runtime is not the same as naming a *usable* one, which is the second +//! half of this. A registry entry can outlive the folder it points at, and a name +//! the CLI rejects fails the scenario just as surely as no name at all — so what +//! gets named is filtered down to the entries `activate` can accept. use std::path::Path; @@ -31,6 +36,10 @@ use std::path::Path; /// tree is empty (the scenario installs its own) or it holds exactly one runtime, /// which the CLI already auto-selects. /// +/// Chooses only among [`activatable_runtime_keys`] — entries the CLI would +/// actually accept — so neither branch below can name a runtime that fails to +/// activate. +/// /// Prefers `active.json` because that records the runtime the pre-warm actually /// installed and verified — but only when the registry still holds it, so a /// marker left behind by a pruned runtime doesn't strand the scenario. Falls back @@ -41,7 +50,7 @@ use std::path::Path; /// silently mismatched pass. #[must_use] pub fn runtime_key_to_activate(runtimes_dir: &Path) -> Option { - let installed = registry_runtime_keys(runtimes_dir); + let installed = activatable_runtime_keys(runtimes_dir); active_runtime_key(runtimes_dir) .filter(|key| installed.iter().any(|installed| installed == key)) .or_else(|| match installed.as_slice() { @@ -62,10 +71,50 @@ fn active_runtime_key(runtimes_dir: &Path) -> Option { (!key.is_empty()).then_some(key) } +/// The registry keys `rocm runtimes activate` would actually accept. +/// +/// A registry entry is not enough: the CLI refuses to activate a runtime whose +/// recorded `install_root` is gone ("install root is missing"). That is not +/// hypothetical here — a scenario that installs through its own `data/runtimes` +/// symlink records its per-scenario temp dir as the install root, and the folder +/// dies with the scenario, leaving a registry entry pointing at nothing (see +/// rocm-cli#315/#316). The pre-warm normally evicts those, but its repair is +/// deliberately non-fatal and skips the whole tree when `runtimes list` itself +/// fails — which a poisoned entry makes it do. So the suite must expect to meet +/// one and step over it rather than name it and fail. +/// +/// Judged by the same rule the pre-warm's own repair uses: an install root +/// inside this tree is sound, one outside it is a corpse. Checking existence +/// alone would be wrong — on a runner where a foreign path happens to exist the +/// scenario would serve against a runtime outside the shared tree. +fn activatable_runtime_keys(runtimes_dir: &Path) -> Vec { + registry_runtime_keys(runtimes_dir) + .into_iter() + .filter(|key| { + let Ok(text) = + std::fs::read_to_string(runtimes_dir.join("registry").join(format!("{key}.json"))) + else { + return false; + }; + let Ok(json) = serde_json::from_str::(&text) else { + return false; + }; + // No recorded root: nothing to disqualify it on, so keep it and let + // the CLI have the final say. + let Some(root) = json.get("install_root").and_then(|v| v.as_str()) else { + return true; + }; + Path::new(root).starts_with(runtimes_dir) + }) + .collect() +} + /// Every runtime key present in the registry, empty when it is absent. /// /// Public so a caller that declines to activate can name what it found: "no /// runtime to activate" is only actionable alongside the list it chose from. +/// Deliberately unfiltered — a diagnostic should report the tree as it is, so a +/// key skipped by [`activatable_runtime_keys`] still shows up in the message. #[must_use] pub fn registry_runtime_keys(runtimes_dir: &Path) -> Vec { let Ok(entries) = std::fs::read_dir(runtimes_dir.join("registry")) else { @@ -98,6 +147,29 @@ mod tests { write(&dir.join("registry").join(format!("{key}.json")), "{}"); } + /// A runtime installed in place: its recorded root lives inside the tree. + fn installed(dir: &Path, key: &str) { + let root = dir.join("wheel").join(key); + std::fs::create_dir_all(&root).expect("create install root"); + write( + &dir.join("registry").join(format!("{key}.json")), + &serde_json::json!({ "runtime_key": key, "install_root": root }).to_string(), + ); + } + + /// A runtime whose recorded root is a dead per-scenario temp dir — the + /// rocm-cli#315 poisoning the pre-warm failed to evict. + fn poisoned(dir: &Path, key: &str) { + write( + &dir.join("registry").join(format!("{key}.json")), + &serde_json::json!({ + "runtime_key": key, + "install_root": format!("/tmp/rocm-e2e-gone/data/runtimes/wheel/{key}"), + }) + .to_string(), + ); + } + fn active(dir: &Path, key: &str) { write( &dir.join("active.json"), @@ -188,6 +260,66 @@ mod tests { ); } + /// The regression from the first attempt at this fix: the sole *registry* + /// entry was a corpse pointing at a dead per-scenario temp dir, so naming it + /// made every activate fail with "install root is missing" — the suite + /// traded one silent breakage for a louder one. Skip it and name the runtime + /// that is really there. + #[test] + fn skips_a_runtime_whose_install_root_left_the_tree() { + let tmp = tempfile::TempDir::with_prefix("shared-runtime-").expect("temp dir"); + let dir = tmp.path(); + poisoned(dir, "release-wheel-gfx94x-dcgpu-7-13-0"); + installed(dir, "release-wheel-multi-arch-7-14-0"); + + assert_eq!( + runtime_key_to_activate(dir).as_deref(), + Some("release-wheel-multi-arch-7-14-0") + ); + } + + /// A marker naming a poisoned runtime must not override a sound one either: + /// the pre-warm activates before a scenario poisons the tree, so the stale + /// marker outlives the runtime it names. + #[test] + fn ignores_a_marker_naming_a_runtime_that_left_the_tree() { + let tmp = tempfile::TempDir::with_prefix("shared-runtime-").expect("temp dir"); + let dir = tmp.path(); + poisoned(dir, "release-wheel-gfx94x-dcgpu-7-13-0"); + installed(dir, "release-wheel-multi-arch-7-14-0"); + active(dir, "release-wheel-gfx94x-dcgpu-7-13-0"); + + assert_eq!( + runtime_key_to_activate(dir).as_deref(), + Some("release-wheel-multi-arch-7-14-0") + ); + } + + /// Every runtime is a corpse: name none. Letting the CLI report "no active + /// ROCm runtime is configured" beats an activate that cannot succeed. + #[test] + fn names_nothing_when_every_runtime_left_the_tree() { + let tmp = tempfile::TempDir::with_prefix("shared-runtime-").expect("temp dir"); + let dir = tmp.path(); + poisoned(dir, "release-wheel-gfx94x-dcgpu-7-13-0"); + + assert_eq!(runtime_key_to_activate(dir), None); + } + + /// The diagnostic reports the tree as it is: a skipped runtime still has to + /// appear, or "no runtime to activate" names an empty tree that isn't empty. + #[test] + fn reports_a_skipped_runtime_in_the_registry_listing() { + let tmp = tempfile::TempDir::with_prefix("shared-runtime-").expect("temp dir"); + let dir = tmp.path(); + poisoned(dir, "release-wheel-gfx94x-dcgpu-7-13-0"); + + assert_eq!( + registry_runtime_keys(dir), + vec!["release-wheel-gfx94x-dcgpu-7-13-0"] + ); + } + /// A marker with a blank key names nothing. #[test] fn treats_a_blank_marker_key_as_absent() { From 3f4e24345312fe69b3e641c9bdd097b3eb7154ed Mon Sep 17 00:00:00 2001 From: Roman Inflianskas Date: Fri, 28 Aug 2026 13:20:33 +0000 Subject: [PATCH 4/6] test(e2e): match a runtime tree reached through a symlink MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Selection compared a recorded install root against the shared tree using only the spelling it was handed, but the two sides are resolved-vs-as-given by construction: the CLI canonicalizes an install root before writing it, while E2E_SHARED_RUNTIMES_DIR arrives verbatim — validated_shared_dir checks it is absolute and free of `..` and deliberately does not resolve. Reach the tree through a symlinked component of the workspace and every healthy entry then looks out-of-tree. Selection returns nothing, the scenario declines to activate, and the serve fails with the same "no active ROCm runtime is configured" this module exists to prevent — with a single stderr line as the only trace, since declining is best-effort. Compare against the resolved spelling too, matching the rule the pre-warm's own repair uses and stating in the doc why one comparison cannot be enough. Windows needs a third: canonicalize yields a `\\?\` path there while the CLI records a plain one, so the prefix has to come off or the comparison fails on the prefix rather than the folder. That trap is already documented on linked_runtimes_target, which does the same thing for the same reason. The three older tests wrote a bare `{}` manifest, taking the "no recorded root" branch — all three passed with the root check deleted entirely, and that shape cannot occur in CI because install_root is non-optional and unparseable manifests are dropped. Use the realistic helper, so they exercise the filter they appear to protect, and add the symlinked-parent case that pins this fix. Signed-off-by: Roman Inflianskas --- tests/e2e-cucumber/src/shared_runtime.rs | 128 ++++++++++++++++++++--- 1 file changed, 114 insertions(+), 14 deletions(-) diff --git a/tests/e2e-cucumber/src/shared_runtime.rs b/tests/e2e-cucumber/src/shared_runtime.rs index ab6b160f9..4b8d66ce1 100644 --- a/tests/e2e-cucumber/src/shared_runtime.rs +++ b/tests/e2e-cucumber/src/shared_runtime.rs @@ -88,6 +88,7 @@ fn active_runtime_key(runtimes_dir: &Path) -> Option { /// alone would be wrong — on a runner where a foreign path happens to exist the /// scenario would serve against a runtime outside the shared tree. fn activatable_runtime_keys(runtimes_dir: &Path) -> Vec { + let roots = comparable_roots(runtimes_dir); registry_runtime_keys(runtimes_dir) .into_iter() .filter(|key| { @@ -104,11 +105,56 @@ fn activatable_runtime_keys(runtimes_dir: &Path) -> Vec { let Some(root) = json.get("install_root").and_then(|v| v.as_str()) else { return true; }; - Path::new(root).starts_with(runtimes_dir) + let root = Path::new(root); + roots.iter().any(|base| root.starts_with(base)) }) .collect() } +/// Every spelling of `runtimes_dir` a recorded `install_root` might match. +/// +/// The two sides are resolved-vs-as-given by construction, so one comparison is +/// not enough. The CLI canonicalizes an install root before recording it, while +/// `E2E_SHARED_RUNTIMES_DIR` reaches us verbatim — `validated_shared_dir` checks +/// that it is absolute and free of `..`, and deliberately does not resolve it. +/// Reach the tree through a symlinked component of the workspace and the two +/// spellings differ, so *every healthy* entry looks out-of-tree: selection comes +/// back empty and the serve fails with the very error this module exists to +/// prevent — silently, since declining to activate is best-effort by design. +/// +/// This mirrors `xtask::e2e_prewarm::assess`, which compares against both +/// spellings for the same reason. Windows needs the third: `canonicalize` yields +/// a `\\?\` verbatim path there and the CLI records a plain one, so the prefix +/// has to come off or the comparison fails on it rather than on the folder. That +/// is the same trap `runtime_steps::linked_runtimes_target` documents, and 8.3 +/// short paths (which `canonicalize` expands) are why resolving matters even +/// when no symlink is involved. +fn comparable_roots(runtimes_dir: &Path) -> Vec { + let mut roots = vec![runtimes_dir.to_path_buf()]; + let Ok(resolved) = runtimes_dir.canonicalize() else { + return roots; + }; + for candidate in [strip_verbatim_prefix(&resolved), resolved] { + if !roots.contains(&candidate) { + roots.push(candidate); + } + } + roots +} + +/// Drop Windows' `\\?\` verbatim prefix, which never `starts_with`-matches the +/// plain path the CLI records. A no-op on every other platform. +fn strip_verbatim_prefix(path: &Path) -> std::path::PathBuf { + let text = path.to_string_lossy(); + if let Some(rest) = text.strip_prefix(r"\\?\UNC\") { + return std::path::PathBuf::from(format!(r"\\{rest}")); + } + match text.strip_prefix(r"\\?\") { + Some(rest) => std::path::PathBuf::from(rest), + None => path.to_path_buf(), + } +} + /// Every runtime key present in the registry, empty when it is absent. /// /// Public so a caller that declines to activate can name what it found: "no @@ -143,11 +189,14 @@ mod tests { std::fs::write(path, body).expect("write file"); } - fn manifest(dir: &Path, key: &str) { - write(&dir.join("registry").join(format!("{key}.json")), "{}"); - } - /// A runtime installed in place: its recorded root lives inside the tree. + /// + /// This is the shape the CLI really writes, so it is what the tests use. + /// A manifest with no `install_root` at all takes the "nothing to judge on" + /// branch and would pass whether or not the root is checked; since + /// `InstalledRuntimeManifest::install_root` is non-optional and + /// `load_runtime_manifests` drops anything that fails to deserialize, such a + /// manifest cannot arise in CI either. Testing against it proved nothing. fn installed(dir: &Path, key: &str) { let root = dir.join("wheel").join(key); std::fs::create_dir_all(&root).expect("create install root"); @@ -184,8 +233,8 @@ mod tests { fn names_the_active_runtime_when_several_are_installed() { let tmp = tempfile::TempDir::with_prefix("shared-runtime-").expect("temp dir"); let dir = tmp.path(); - manifest(dir, "release-wheel-gfx94x-dcgpu-7-13-0"); - manifest(dir, "release-wheel-multi-arch-7-14-0"); + installed(dir, "release-wheel-gfx94x-dcgpu-7-13-0"); + installed(dir, "release-wheel-multi-arch-7-14-0"); active(dir, "release-wheel-multi-arch-7-14-0"); assert_eq!( @@ -201,7 +250,7 @@ mod tests { fn falls_back_to_the_sole_manifest_without_a_marker() { let tmp = tempfile::TempDir::with_prefix("shared-runtime-").expect("temp dir"); let dir = tmp.path(); - manifest(dir, "release-wheel-gfx94x-dcgpu-7-13-0"); + installed(dir, "release-wheel-gfx94x-dcgpu-7-13-0"); assert_eq!( runtime_key_to_activate(dir).as_deref(), @@ -216,8 +265,8 @@ mod tests { fn refuses_to_guess_between_several_without_a_marker() { let tmp = tempfile::TempDir::with_prefix("shared-runtime-").expect("temp dir"); let dir = tmp.path(); - manifest(dir, "release-wheel-gfx94x-dcgpu-7-13-0"); - manifest(dir, "release-wheel-multi-arch-7-14-0"); + installed(dir, "release-wheel-gfx94x-dcgpu-7-13-0"); + installed(dir, "release-wheel-multi-arch-7-14-0"); assert_eq!(runtime_key_to_activate(dir), None); } @@ -236,7 +285,7 @@ mod tests { fn ignores_a_marker_naming_no_installed_runtime() { let tmp = tempfile::TempDir::with_prefix("shared-runtime-").expect("temp dir"); let dir = tmp.path(); - manifest(dir, "release-wheel-gfx94x-dcgpu-7-13-0"); + installed(dir, "release-wheel-gfx94x-dcgpu-7-13-0"); active(dir, "release-wheel-multi-arch-7-14-0"); assert_eq!( @@ -251,7 +300,7 @@ mod tests { fn treats_an_unreadable_marker_as_absent() { let tmp = tempfile::TempDir::with_prefix("shared-runtime-").expect("temp dir"); let dir = tmp.path(); - manifest(dir, "release-wheel-gfx94x-dcgpu-7-13-0"); + installed(dir, "release-wheel-gfx94x-dcgpu-7-13-0"); write(&dir.join("active.json"), "{ not json"); assert_eq!( @@ -278,6 +327,57 @@ mod tests { ); } + /// Reaching the tree through a symlinked parent must not disqualify every + /// healthy runtime in it. + /// + /// The CLI records a canonicalized `install_root`; `E2E_SHARED_RUNTIMES_DIR` + /// arrives unresolved. Compare only the spelling we were handed and the two + /// never match, so selection comes back empty and the serve fails with the + /// error this module exists to prevent — the original bug through a different + /// door, and silent. Approximates a symlinked component of the runner's + /// workspace, which is the shape that would trigger it in CI. + #[test] + fn resolves_a_tree_reached_through_a_symlinked_parent() { + let tmp = tempfile::TempDir::with_prefix("shared-runtime-").expect("temp dir"); + let real = tmp.path().join("real"); + std::fs::create_dir_all(&real).expect("create real tree"); + // The manifest records the resolved root, as the CLI writes it. + installed(&real, "release-wheel-multi-arch-7-14-0"); + + let link = tmp.path().join("link"); + #[cfg(unix)] + std::os::unix::fs::symlink(&real, &link).expect("symlink"); + #[cfg(windows)] + std::os::windows::fs::symlink_dir(&real, &link).expect("symlink"); + + // Selection is handed the unresolved spelling, exactly as CI would. + assert_eq!( + runtime_key_to_activate(&link).as_deref(), + Some("release-wheel-multi-arch-7-14-0"), + "a tree reached through a symlink must still resolve its runtimes" + ); + } + + /// The symlink tolerance must not become "any path anywhere": a corpse + /// pointing outside the tree stays disqualified when the tree is reached + /// through a link, or the fix above would have re-admitted every poisoned + /// entry it was meant to skip. + #[test] + fn still_skips_a_corpse_when_the_tree_is_reached_through_a_symlink() { + let tmp = tempfile::TempDir::with_prefix("shared-runtime-").expect("temp dir"); + let real = tmp.path().join("real"); + std::fs::create_dir_all(&real).expect("create real tree"); + poisoned(&real, "release-wheel-gfx94x-dcgpu-7-13-0"); + + let link = tmp.path().join("link"); + #[cfg(unix)] + std::os::unix::fs::symlink(&real, &link).expect("symlink"); + #[cfg(windows)] + std::os::windows::fs::symlink_dir(&real, &link).expect("symlink"); + + assert_eq!(runtime_key_to_activate(&link), None); + } + /// A marker naming a poisoned runtime must not override a sound one either: /// the pre-warm activates before a scenario poisons the tree, so the stale /// marker outlives the runtime it names. @@ -325,8 +425,8 @@ mod tests { fn treats_a_blank_marker_key_as_absent() { let tmp = tempfile::TempDir::with_prefix("shared-runtime-").expect("temp dir"); let dir = tmp.path(); - manifest(dir, "release-wheel-gfx94x-dcgpu-7-13-0"); - manifest(dir, "release-wheel-multi-arch-7-14-0"); + installed(dir, "release-wheel-gfx94x-dcgpu-7-13-0"); + installed(dir, "release-wheel-multi-arch-7-14-0"); active(dir, " "); assert_eq!(runtime_key_to_activate(dir), None); From f26e6a1cf4e4d65315b96519754e981456ec2ca0 Mon Sep 17 00:00:00 2001 From: Roman Inflianskas Date: Fri, 28 Aug 2026 13:20:44 +0000 Subject: [PATCH 5/6] test(e2e): fail rather than report on an unintended runtime MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The freshness assertion picked the active runtime's line and otherwise fell back to the first one. Its comment said that fallback was for "when nothing is active", but it also caught the case where something IS active and no line matches it — and then asserted against a runtime the run did not use, passing while doing so. That is the misattribution this selection was added to remove. Fall back only when nothing is active; when something is, assert on its line or fail naming it. Also correct active_runtime_install_root's rationale. It turned on a per-scenario temp dir that no longer exists by report time, which can no longer reach that code — such an entry is filtered out before it gets there. The manifest fallback still earns its keep and the comment undersold why: a tarball runtime lives at /tarball/, which the derived wheel path never matches, so for those it is the only correct answer. Calling it a legacy-layout leftover reads as an invitation to delete live code. Record too that a tree of entirely out-of-tree entries now reports no version at all, which is a deliberate absent-beats-wrong trade rather than an oversight. active_runtime_key took &mut E2eWorld while only calling run_rocm, which borrows shared; dropping it removes a clone and a re-borrow at the call site. Signed-off-by: Roman Inflianskas --- tests/e2e-cucumber/src/capability.rs | 22 ++++++++++---- tests/e2e-cucumber/tests/e2e/runtime_steps.rs | 30 +++++++++++++------ 2 files changed, 37 insertions(+), 15 deletions(-) diff --git a/tests/e2e-cucumber/src/capability.rs b/tests/e2e-cucumber/src/capability.rs index 19d48aa65..de2197cff 100644 --- a/tests/e2e-cucumber/src/capability.rs +++ b/tests/e2e-cucumber/src/capability.rs @@ -209,12 +209,22 @@ pub fn collect_versions(runtimes_dir: Option<&std::path::Path>) -> PlatformVersi /// /// The install_root is resolved from `runtimes_dir` (the shared tree we were /// handed) as `/wheel/`, NOT from the manifest's own -/// `install_root` field. That field records the absolute path where the runtime -/// was first installed — on Strix a per-scenario temp dir that no longer exists -/// by report time — so trusting it made `vllm`/`lemonade` probe a dead path and -/// come back `None`. On MI300X the two coincide (prewarm installs in place), -/// which is why it worked there but not on Strix. Falls back to the manifest -/// path if the derived one is absent, for any tree that predates the wheel layout. +/// `install_root` field, which records where the runtime was first installed and +/// need not be where it lives now. On MI300X the two coincide (the pre-warm +/// installs in place); on Strix they did not, and trusting the field made +/// `vllm`/`lemonade` probe a dead path and report no versions at all. +/// +/// The manifest fallback is load-bearing, not legacy — do not read it as dead +/// code. `MANAGED_RUNTIME_FORMATS` is `["wheel", "tarball"]`, and a tarball +/// runtime lives at `/tarball/`, which the derived `wheel` path never +/// matches. For those the fallback is the only correct answer. It also still +/// covers a tree predating the `wheel/` layout. +/// +/// One shape can no longer reach here: a runtime whose recorded root left the +/// tree entirely. `runtime_key_to_activate` now filters those out, so the +/// fallback yields an in-tree path or nothing. Where every entry is such a +/// corpse this returns `None` and the report simply omits the version — absent +/// reads as unknown, which is the honest outcome; a wrong version reads as fact. fn active_runtime_install_root( runtimes_dir: &std::path::Path, ) -> Option<(String, std::path::PathBuf)> { diff --git a/tests/e2e-cucumber/tests/e2e/runtime_steps.rs b/tests/e2e-cucumber/tests/e2e/runtime_steps.rs index d884189fa..caae41986 100644 --- a/tests/e2e-cucumber/tests/e2e/runtime_steps.rs +++ b/tests/e2e-cucumber/tests/e2e/runtime_steps.rs @@ -287,7 +287,7 @@ async fn user_checks_for_updates(world: &mut E2eWorld) { } /// The runtime key `runtimes list` reports as active, if any. -fn active_runtime_key(world: &mut E2eWorld) -> Option { +fn active_runtime_key(world: &E2eWorld) -> Option { let (stdout, _, _) = crate::run_rocm(world, &["runtimes", "list"]); let key = stdout .lines() @@ -304,8 +304,7 @@ const UPDATE_STATUSES: [&str; 4] = ["up_to_date", "update_available", "ahead_of_ #[then("the report states the runtime's freshness against the channel index")] async fn assert_update_reports_freshness(world: &mut E2eWorld) { - let stdout = world.cli_output.clone().unwrap_or_default(); - let stdout = stdout.as_str(); + let stdout = world.cli_output.as_deref().unwrap_or(""); let rc = world.cli_rc.expect("no command was run"); assert_eq!(rc, 0, "`rocm update` failed:\n{stdout}"); @@ -313,8 +312,7 @@ async fn assert_update_reports_freshness(world: &mut E2eWorld) { // The report carries one such line per installed runtime, newest first, and // the shared tree holds more than one — so select the ACTIVE runtime's line // rather than whichever came first, or this scenario reports on a runtime the - // run never used. Falls back to the first line when nothing is active, which - // is the single-runtime case this scenario was written against. + // run never used. let active = active_runtime_key(world); let runtime_lines = || { stdout @@ -322,10 +320,24 @@ async fn assert_update_reports_freshness(world: &mut E2eWorld) { .map(str::trim) .filter(|line| line.starts_with("runtime ")) }; - let line = active - .as_deref() - .and_then(|key| runtime_lines().find(|line| line.split_whitespace().nth(1) == Some(key))) - .or_else(|| runtime_lines().next()); + let line = match active.as_deref() { + // Something is active: assert on ITS line or not at all. Falling back to + // the first line here would report on a runtime the run did not use — + // the misattribution this selection exists to remove — and it would pass + // while doing it. + Some(key) => { + let found = runtime_lines().find(|line| line.split_whitespace().nth(1) == Some(key)); + assert!( + found.is_some(), + "runtime `{key}` is active but the update report has no `runtime {key} …` \ + line:\n{stdout}" + ); + found + } + // Nothing active: the single-runtime case this scenario was written + // against, where the sole line is unambiguously the right one. + None => runtime_lines().next(), + }; let Some(line) = line else { panic!("no `runtime …` line in the update report:\n{stdout}"); }; From c8021409c51a7103d12bc8eaa2415498d1e16f18 Mon Sep 17 00:00:00 2001 From: Roman Inflianskas Date: Fri, 28 Aug 2026 13:38:31 +0000 Subject: [PATCH 6/6] test(e2e): record a canonical install root in the test fixture `resolves_a_tree_reached_through_a_symlinked_parent` failed on Windows. The fixture wrote the install root exactly as `TempDir` handed it over, but Windows hands out an 8.3 short path (`RUNNER~1\...`) while the CLI records a canonicalized one. The two spellings never `starts_with`-match, so the entry was disqualified and selection came back empty. The bug was in the fixture, not the filter: it built a manifest the CLI cannot produce. Canonicalize the root before recording it, matching what `rocm install sdk` writes. The test now covers the symlink tolerance it was written for on every platform instead of passing on Unix by accident of the two spellings already agreeing there. Signed-off-by: Roman Inflianskas --- tests/e2e-cucumber/src/shared_runtime.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/tests/e2e-cucumber/src/shared_runtime.rs b/tests/e2e-cucumber/src/shared_runtime.rs index 4b8d66ce1..df9ec0a0a 100644 --- a/tests/e2e-cucumber/src/shared_runtime.rs +++ b/tests/e2e-cucumber/src/shared_runtime.rs @@ -200,6 +200,12 @@ mod tests { fn installed(dir: &Path, key: &str) { let root = dir.join("wheel").join(key); std::fs::create_dir_all(&root).expect("create install root"); + // Record the CANONICAL root, because that is what the CLI writes. It + // matters on Windows, where a temp dir is handed to us in 8.3 short form + // (`RUNNER~1`) and canonicalizing expands it: writing the short spelling + // here would test a manifest the CLI never produces, and the two spellings + // do not `starts_with`-match. + let root = root.canonicalize().expect("canonicalize install root"); write( &dir.join("registry").join(format!("{key}.json")), &serde_json::json!({ "runtime_key": key, "install_root": root }).to_string(),