From 17e71c91a3957b1e6c27f20769cdd6da06de7e84 Mon Sep 17 00:00:00 2001 From: Juho Vainio Date: Mon, 7 Sep 2026 15:11:01 +0000 Subject: [PATCH 01/15] fix(e2e): share the therock/tool archive cache across scenarios Each scenario's TempDir wiped ROCM_CLI_CACHE_DIR along with everything else, so the ~3.3GB llamacpp:rocm backend archive was re-downloaded from scratch by every scenario that needed it (5x in one ~25min job). Route it through the same shared, persistent dir CI already provides for HF_HOME/PIP_CACHE_DIR when E2E_SHARED_CACHE_DIR is set; local runs without a shared dir keep the old fully-isolated cache. Fixes EAI-8572. Signed-off-by: Juho Vainio --- tests/e2e-cucumber/tests/e2e.rs | 24 +++++++++++++++++++----- 1 file changed, 19 insertions(+), 5 deletions(-) diff --git a/tests/e2e-cucumber/tests/e2e.rs b/tests/e2e-cucumber/tests/e2e.rs index 10462fc16..47e785643 100644 --- a/tests/e2e-cucumber/tests/e2e.rs +++ b/tests/e2e-cucumber/tests/e2e.rs @@ -130,13 +130,16 @@ fn validated_shared_dir(env_var: &str) -> Option { } /// A persistent directory shared across scenarios for heavy, immutable artifacts -/// (TheRock runtime wheels, HF model weights, engine venvs). Set by CI to a path +/// (HF model weights, the pip cache, and the CLI's own therock/tool archive +/// download cache — engine backends like `llamacpp:rocm`). Set by CI to a path /// on the runner's persistent disk; unset for local runs, where every scenario /// stays fully isolated (nothing shared). /// -/// Sharing these read-only artifacts avoids re-downloading multi-GB runtimes and -/// model weights per scenario. Only immutable artifacts are shared — service -/// records, config, and per-service engine state stay isolated per scenario. +/// Sharing these read-only artifacts avoids re-downloading multi-GB engine +/// backends and model weights per scenario. Only immutable artifacts are +/// shared — service records, config, and the runtimes registry stay isolated +/// per scenario (see [`shared_runtimes_dir`] for the runtimes' own, opt-in, +/// shared tree). fn shared_cache_dir() -> Option { validated_shared_dir("E2E_SHARED_CACHE_DIR") } @@ -232,7 +235,18 @@ impl E2eWorld { let root = root.path(); env.push(("ROCM_CLI_CONFIG_DIR", root.join("config").into_os_string())); env.push(("ROCM_CLI_DATA_DIR", root.join("data").into_os_string())); - env.push(("ROCM_CLI_CACHE_DIR", root.join("cache").into_os_string())); + // The CLI's own therock/tool archive download cache (plus its HTTP/TUF + // metadata cache) holds nothing the suite asserts on — every byte in it + // is content-addressed and re-fetchable (see storage::download_cache_dir / + // tool_download_cache_dir). Route it through the same shared, persistent + // dir as HF_HOME/PIP_CACHE_DIR below instead of this scenario's TempDir, + // so the ~3.3GB llamacpp:rocm backend archive is downloaded once per + // runner rather than once per scenario (EAI-8572). Local runs (no shared + // dir) keep the old fully-isolated cache. + let cache_dir = shared_cache_dir() + .map(|shared| shared.join("rocm-cli")) + .unwrap_or_else(|| root.join("cache")); + env.push(("ROCM_CLI_CACHE_DIR", cache_dir.into_os_string())); } // Share only STATE-FREE, content-addressed caches across scenarios when // CI provides a persistent shared dir (see shared_cache_dir): HF model From dfb0a61ce70d7279ef4be2aa4cc46a3cacf32180 Mon Sep 17 00:00:00 2001 From: Juho Vainio Date: Mon, 7 Sep 2026 15:18:47 +0000 Subject: [PATCH 02/15] fix(e2e): satisfy clippy::map_unwrap_or in shared cache dir lookup CI's e2e clippy lane denies warnings and flags map().unwrap_or_else() on Option; use map_or_else instead. Signed-off-by: Juho Vainio --- tests/e2e-cucumber/tests/e2e.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tests/e2e-cucumber/tests/e2e.rs b/tests/e2e-cucumber/tests/e2e.rs index 47e785643..d01a8bcd9 100644 --- a/tests/e2e-cucumber/tests/e2e.rs +++ b/tests/e2e-cucumber/tests/e2e.rs @@ -244,8 +244,7 @@ impl E2eWorld { // runner rather than once per scenario (EAI-8572). Local runs (no shared // dir) keep the old fully-isolated cache. let cache_dir = shared_cache_dir() - .map(|shared| shared.join("rocm-cli")) - .unwrap_or_else(|| root.join("cache")); + .map_or_else(|| root.join("cache"), |shared| shared.join("rocm-cli")); env.push(("ROCM_CLI_CACHE_DIR", cache_dir.into_os_string())); } // Share only STATE-FREE, content-addressed caches across scenarios when From 090231e80f82f88ddb69caa9626464c34e397488 Mon Sep 17 00:00:00 2001 From: Juho Vainio Date: Mon, 7 Sep 2026 15:34:27 +0000 Subject: [PATCH 03/15] fix(core): serialize fix.rs tests that mutate process-global ROCM_PATH Two tests set ROCM_PATH and read it back without synchronization, so under parallel test execution one test's install dir could leak into the other's read and fail its assertion. Mirrors the PROCESS_ENV_TEST_LOCK pattern already used in therock.rs/main.rs for the same class of race. Signed-off-by: Juho Vainio --- crates/rocm-core/src/fix.rs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/crates/rocm-core/src/fix.rs b/crates/rocm-core/src/fix.rs index 695f86009..5750a0411 100644 --- a/crates/rocm-core/src/fix.rs +++ b/crates/rocm-core/src/fix.rs @@ -992,6 +992,11 @@ fn newest_rocm_install_dir() -> String { mod tests { use super::*; + // Serializes tests that replace the process-global `ROCM_PATH` env var while + // they run. Because env is shared across all test threads, two such tests + // running concurrently can otherwise see each other's value mid-test. + static PROCESS_ENV_TEST_LOCK: std::sync::Mutex<()> = std::sync::Mutex::new(()); + /// Plant a directory the shared resolver will accept as a ROCm install. /// `bin/rocminfo` is one of the markers it gates on; a bare directory is /// deliberately not enough. @@ -1008,6 +1013,7 @@ mod tests { // outright, so it returned nothing here no matter what was planted. // Going through the shared resolver is what makes this pass -- and is // what stops fix-6-path putting 6.2 on PATH when 6.10 is installed. + let _guard = PROCESS_ENV_TEST_LOCK.lock().unwrap(); let root = std::env::temp_dir().join(format!( "rocm-fix-path-resolver-{}-{:?}", std::process::id(), @@ -1042,6 +1048,7 @@ mod tests { // The old scan accepted any directory whose name started with a digit, // so an empty leftover could be put on PATH. The resolver requires a // marker. + let _guard = PROCESS_ENV_TEST_LOCK.lock().unwrap(); let root = std::env::temp_dir().join(format!( "rocm-fix-path-empty-{}-{:?}", std::process::id(), From b8f9a5e00a8217507591b0c4b9b5e4e4f48456cb Mon Sep 17 00:00:00 2001 From: Juho Vainio Date: Mon, 7 Sep 2026 16:41:23 +0000 Subject: [PATCH 04/15] fix(lemonade): stop wiping the shared runtime tree on every e2e scenario MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit prepare_embeddable() decided whether to re-extract the embeddable archive by reading the installed version from the CLI's own manifest, which lives under the per-invocation data dir. Under the e2e suite each scenario gets an isolated data dir but shares one runtime tree (EAI-8572), so that manifest read always missed, needs_extraction() always returned true, and the shared runtime_dir was removed and re-extracted from scratch on every scenario — destroying the llamacpp:rocm backend a prior scenario had just installed into it and forcing its ~3.3GB download to repeat every time, even though the archive download itself was correctly cache-hit. Track the installed version in a marker colocated with the runtime tree itself (inside lemonade_root) instead, so it survives regardless of which data dir asks. Also correct isolate_env()'s comment, which credited the ROCM_CLI_CACHE_DIR routing for this savings; that path only covers therock/tool archives and never touched the Lemonade backend. Signed-off-by: Juho Vainio --- engines/lemonade/src/lib.rs | 54 +++++++++++++++++++++++++++++++-- tests/e2e-cucumber/tests/e2e.rs | 10 ++++-- 2 files changed, 58 insertions(+), 6 deletions(-) diff --git a/engines/lemonade/src/lib.rs b/engines/lemonade/src/lib.rs index 94090b464..3022611c7 100644 --- a/engines/lemonade/src/lib.rs +++ b/engines/lemonade/src/lib.rs @@ -40,6 +40,14 @@ const DEFAULT_MODEL_REPO_DIR: &str = "models--unsloth--Qwen3-4B-Instruct-2507-GG const DEFAULT_MODEL_GGUF: &str = "Qwen3-4B-Instruct-2507-Q4_K_M.gguf"; const LLAMACPP_RECIPE: &str = "llamacpp"; const ROCM_BACKEND_NAME: &str = "rocm"; +/// Records the extracted embeddable version next to the runtime tree itself +/// (inside `lemonade_root`, not the CLI's own manifest) so the version check +/// survives even when the manifest lives under a data dir that is not shared +/// with the runtime tree (e.g. each e2e scenario's isolated data dir vs. a +/// shared runtime tree). Without this, [`prepare_embeddable`] can never prove +/// the shared tree is already current, wipes it, and re-extracts on every +/// call — destroying any llama.cpp backend installed into it in the process. +const RUNTIME_VERSION_MARKER: &str = "runtime-version.txt"; #[cfg(feature = "e2e-test-hooks")] const BACKEND_INSTALL_FAILURE_TEST_ENV: &str = "ROCM_E2E_LEMONADE_BACKEND_INSTALL_FAILURE"; /// Preferred llama.cpp backends, best first. Lemonade reports per-GPU support; @@ -1076,10 +1084,18 @@ fn prepare_embeddable( // archive into a tree that already holds `lemond` from the previous // version. Without comparing versions the extraction would be skipped and // the old binaries reported as the new version. - let installed_version = read_manifest(paths) + // + // The version is read from a marker inside `root` (the same shared tree + // as `runtime_dir`), not from the CLI's own manifest: the manifest lives + // under `paths` (the per-invocation data dir), which is not guaranteed to + // be the same data dir that last extracted into this `runtime_dir` (e.g. + // each e2e scenario gets an isolated data dir but shares one runtime + // tree). Sourcing the check from the isolated manifest would report + // "unknown version" on every call, wipe the shared tree, and re-extract + // over it — destroying any llama.cpp backend already installed there. + let installed_version = fs::read_to_string(root.join(RUNTIME_VERSION_MARKER)) .ok() - .filter(|manifest| manifest.runtime_dir == runtime_dir) - .map(|manifest| manifest.version); + .map(|s| s.trim().to_owned()); if needs_extraction( reinstall, installed_version.as_deref(), @@ -1098,6 +1114,12 @@ fn prepare_embeddable( let embeddable_root = find_embeddable_root(&extract_root)?; copy_tree(&embeddable_root, &runtime_dir)?; fs::remove_dir_all(&extract_root).ok(); + fs::write(root.join(RUNTIME_VERSION_MARKER), &version).with_context(|| { + format!( + "failed to record installed Lemonade runtime version under {}", + root.display() + ) + })?; } drop(archive_guard); let lemond = lemond_path_in(&runtime_dir); @@ -5008,6 +5030,32 @@ mod tests { assert!(needs_extraction(true, Some(pin), pin, true)); } + #[test] + fn runtime_version_marker_survives_without_a_manifest() { + // The regression this guards: an isolated caller (e.g. a data dir that + // has never seen this `root` before) must still recognize a runtime + // tree extracted by a *different* caller sharing the same `root`, or + // `prepare_embeddable` wipes and re-extracts it on every call. + let dir = std::env::temp_dir().join(format!( + "rocm-lemonade-marker-test-{}", + std::process::id() + )); + fs::create_dir_all(&dir).unwrap(); + let marker = dir.join(RUNTIME_VERSION_MARKER); + + assert!(fs::read_to_string(&marker).is_err()); + + fs::write(&marker, "7.13.0\n").unwrap(); + let installed = fs::read_to_string(&marker) + .ok() + .map(|s| s.trim().to_owned()); + assert_eq!(installed.as_deref(), Some("7.13.0")); + assert!(!needs_extraction(false, installed.as_deref(), "7.13.0", true)); + assert!(needs_extraction(false, installed.as_deref(), "7.14.0", true)); + + fs::remove_dir_all(&dir).ok(); + } + #[test] fn lemonade_root_uses_requested_engine_root() { let paths = AppPaths { diff --git a/tests/e2e-cucumber/tests/e2e.rs b/tests/e2e-cucumber/tests/e2e.rs index d01a8bcd9..1c3b856f5 100644 --- a/tests/e2e-cucumber/tests/e2e.rs +++ b/tests/e2e-cucumber/tests/e2e.rs @@ -240,9 +240,13 @@ impl E2eWorld { // is content-addressed and re-fetchable (see storage::download_cache_dir / // tool_download_cache_dir). Route it through the same shared, persistent // dir as HF_HOME/PIP_CACHE_DIR below instead of this scenario's TempDir, - // so the ~3.3GB llamacpp:rocm backend archive is downloaded once per - // runner rather than once per scenario (EAI-8572). Local runs (no shared - // dir) keep the old fully-isolated cache. + // so therock SDK/tool archives are downloaded once per runner rather than + // once per scenario. Local runs (no shared dir) keep the old fully-isolated + // cache. This does NOT cover the ~3.3GB llamacpp:rocm Lemonade backend + // (EAI-8572) — that lives under the shared runtimes tree (see + // `use_shared_runtimes`) and was actually fixed by making + // `prepare_embeddable` stop wiping that shared tree on every scenario + // (rocm-engine-lemonade's `RUNTIME_VERSION_MARKER`). let cache_dir = shared_cache_dir() .map_or_else(|| root.join("cache"), |shared| shared.join("rocm-cli")); env.push(("ROCM_CLI_CACHE_DIR", cache_dir.into_os_string())); From 34a4d78db5878f914da171c64dbfa02c492a093e Mon Sep 17 00:00:00 2001 From: Juho Vainio Date: Mon, 7 Sep 2026 19:34:32 +0000 Subject: [PATCH 05/15] fmt: run cargo fmt on the lemonade marker fix prek's cargo-fmt hook failed CI on the previous push (unformatted test code in the runtime-version-marker regression test). Signed-off-by: Juho Vainio --- engines/lemonade/src/lib.rs | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/engines/lemonade/src/lib.rs b/engines/lemonade/src/lib.rs index 3022611c7..e171a2d6d 100644 --- a/engines/lemonade/src/lib.rs +++ b/engines/lemonade/src/lib.rs @@ -5036,10 +5036,8 @@ mod tests { // has never seen this `root` before) must still recognize a runtime // tree extracted by a *different* caller sharing the same `root`, or // `prepare_embeddable` wipes and re-extracts it on every call. - let dir = std::env::temp_dir().join(format!( - "rocm-lemonade-marker-test-{}", - std::process::id() - )); + let dir = + std::env::temp_dir().join(format!("rocm-lemonade-marker-test-{}", std::process::id())); fs::create_dir_all(&dir).unwrap(); let marker = dir.join(RUNTIME_VERSION_MARKER); @@ -5050,8 +5048,18 @@ mod tests { .ok() .map(|s| s.trim().to_owned()); assert_eq!(installed.as_deref(), Some("7.13.0")); - assert!(!needs_extraction(false, installed.as_deref(), "7.13.0", true)); - assert!(needs_extraction(false, installed.as_deref(), "7.14.0", true)); + assert!(!needs_extraction( + false, + installed.as_deref(), + "7.13.0", + true + )); + assert!(needs_extraction( + false, + installed.as_deref(), + "7.14.0", + true + )); fs::remove_dir_all(&dir).ok(); } From 97bd346ff6560c3383a132c14a1866370a0319f0 Mon Sep 17 00:00:00 2001 From: Juho Vainio Date: Tue, 8 Sep 2026 11:49:14 +0000 Subject: [PATCH 06/15] fix(e2e): close review feedback on cache-sharing safety and doc accuracy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - shared_cache_dir()'s doc comment attributed backend savings to it, contradicting isolate_env()'s corrected comment; reword so both agree the backend is covered by shared_runtimes_dir instead. - The shared cache dir holds one mutable, non-atomically-written file (cache/therock/startup-update-check.json). A concurrent scenario could tear it, and load_startup_update_check hard-errors on a malformed file. Disable the check suite-wide instead. - Document (not enforce) that cache-dir sharing is only safe because GPU lanes pin max_concurrent to 1 — already silently violated on the WSL2 lane, whose GPU-capability probe reads false. Enforcing this is tracked as a follow-up (ROCm/rocm-cli#359). Per review from @tomastola and @rominf on #355. Signed-off-by: Juho Vainio --- tests/e2e-cucumber/tests/e2e.rs | 51 +++++++++++++++++++++++---------- 1 file changed, 36 insertions(+), 15 deletions(-) diff --git a/tests/e2e-cucumber/tests/e2e.rs b/tests/e2e-cucumber/tests/e2e.rs index 1c3b856f5..1440831ac 100644 --- a/tests/e2e-cucumber/tests/e2e.rs +++ b/tests/e2e-cucumber/tests/e2e.rs @@ -131,15 +131,15 @@ fn validated_shared_dir(env_var: &str) -> Option { /// A persistent directory shared across scenarios for heavy, immutable artifacts /// (HF model weights, the pip cache, and the CLI's own therock/tool archive -/// download cache — engine backends like `llamacpp:rocm`). Set by CI to a path -/// on the runner's persistent disk; unset for local runs, where every scenario -/// stays fully isolated (nothing shared). +/// download cache). Set by CI to a path on the runner's persistent disk; unset +/// for local runs, where every scenario stays fully isolated (nothing shared). /// -/// Sharing these read-only artifacts avoids re-downloading multi-GB engine -/// backends and model weights per scenario. Only immutable artifacts are -/// shared — service records, config, and the runtimes registry stay isolated -/// per scenario (see [`shared_runtimes_dir`] for the runtimes' own, opt-in, -/// shared tree). +/// Sharing these read-only artifacts avoids re-downloading the therock +/// SDK/tool archives and model weights per scenario. Only immutable artifacts +/// are shared — service records, config, and the runtimes registry stay +/// isolated per scenario (see [`shared_runtimes_dir`] for the runtimes' own, +/// opt-in, shared tree, which is what actually covers engine backends like +/// `llamacpp:rocm`). fn shared_cache_dir() -> Option { validated_shared_dir("E2E_SHARED_CACHE_DIR") } @@ -235,18 +235,28 @@ impl E2eWorld { let root = root.path(); env.push(("ROCM_CLI_CONFIG_DIR", root.join("config").into_os_string())); env.push(("ROCM_CLI_DATA_DIR", root.join("data").into_os_string())); - // The CLI's own therock/tool archive download cache (plus its HTTP/TUF - // metadata cache) holds nothing the suite asserts on — every byte in it - // is content-addressed and re-fetchable (see storage::download_cache_dir / - // tool_download_cache_dir). Route it through the same shared, persistent - // dir as HF_HOME/PIP_CACHE_DIR below instead of this scenario's TempDir, - // so therock SDK/tool archives are downloaded once per runner rather than - // once per scenario. Local runs (no shared dir) keep the old fully-isolated + // The CLI's own therock/tool archive download cache — every archive + // in it is content-addressed and re-fetchable (see + // storage::download_cache_dir / tool_download_cache_dir). Route it + // through the same shared, persistent dir as HF_HOME/PIP_CACHE_DIR + // below instead of this scenario's TempDir, so therock SDK/tool + // archives are downloaded once per runner rather than once per + // scenario. Local runs (no shared dir) keep the old fully-isolated // cache. This does NOT cover the ~3.3GB llamacpp:rocm Lemonade backend // (EAI-8572) — that lives under the shared runtimes tree (see // `use_shared_runtimes`) and was actually fixed by making // `prepare_embeddable` stop wiping that shared tree on every scenario // (rocm-engine-lemonade's `RUNTIME_VERSION_MARKER`). + // + // Two paths under this cache are mutable rather than content- + // addressed: `cache/therock/startup-update-check.json` (a plain, + // non-atomic `fs::write`) and `cache/therock/metadata/*.json` (the + // etag revalidation cache, atomically written). Concurrent scenarios + // now sharing this dir could race the former into a torn file, which + // `load_startup_update_check` treats as a hard error — so disable the + // check suite-wide rather than let a shared, non-atomic write become + // a flaky `rocm` exit code. + env.push(("ROCM_CLI_DISABLE_STARTUP_UPDATE_CHECK", "1".into())); let cache_dir = shared_cache_dir() .map_or_else(|| root.join("cache"), |shared| shared.join("rocm-cli")); env.push(("ROCM_CLI_CACHE_DIR", cache_dir.into_os_string())); @@ -1146,6 +1156,17 @@ async fn main() { // one scenario at a time whenever a GPU is present. The no-GPU mock job keeps // the default parallelism (its scenarios use isolated in-process mock servers // on OS-assigned ports, so they're safe to run concurrently). + // + // This `max_concurrent == 1` is ALSO what makes it safe for a GPU lane to + // point `shared_cache_dir()`/`shared_uv_cache_dir()` at one persistent dir + // (see `isolate_env`): readers extracting an archive and a concurrent + // `remove_file` of that same archive after another scenario's extract + // (`uv.rs`, `therock.rs`) would otherwise race. `cap.has_amd_gpu` is a + // capability *probe*, not the CI lane's own knowledge of whether it set a + // shared cache dir — if a lane exports `E2E_SHARED_CACHE_DIR` while its + // probe reads false (e.g. a missing GPU driver), it would silently run up + // to 64 scenarios against one shared cache dir. Keep the two coupled if + // either changes. let max_concurrent = if cap.has_amd_gpu { 1 } else { 64 }; let summary = E2eWorld::cucumber() .max_concurrent_scenarios(max_concurrent) From 49361001dbc9340bbed63a8718841807d4cd7027 Mon Sep 17 00:00:00 2001 From: Juho Vainio Date: Tue, 8 Sep 2026 11:49:28 +0000 Subject: [PATCH 07/15] fix(lemonade): make the runtime-version marker regression test real MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit runtime_version_marker_survives_without_a_manifest never called prepare_embeddable — it reimplemented the marker read/write inline and only exercised needs_extraction, which adjacent tests already covered. Confirmed by mutation: deleting the marker write from prepare_embeddable still left it green. Extract installed_runtime_version/record_runtime_version helpers and have both prepare_embeddable and the test call them, so reverting either function now fails the test. Also fixes a temp-dir leak/rerun hazard by switching to tempfile::tempdir(). Also move the marker from beside runtime_dir to inside it, so remove_dir_all(runtime_dir) clears the marker as part of the same wipe that removes the tree it describes — "marker present implies extraction completed" now holds by construction instead of resting on the write always being last. Per review from @tomastola and @rominf on #355. Signed-off-by: Juho Vainio --- engines/lemonade/src/lib.rs | 84 ++++++++++++++++++++++--------------- 1 file changed, 50 insertions(+), 34 deletions(-) diff --git a/engines/lemonade/src/lib.rs b/engines/lemonade/src/lib.rs index e171a2d6d..7432b726e 100644 --- a/engines/lemonade/src/lib.rs +++ b/engines/lemonade/src/lib.rs @@ -40,13 +40,16 @@ const DEFAULT_MODEL_REPO_DIR: &str = "models--unsloth--Qwen3-4B-Instruct-2507-GG const DEFAULT_MODEL_GGUF: &str = "Qwen3-4B-Instruct-2507-Q4_K_M.gguf"; const LLAMACPP_RECIPE: &str = "llamacpp"; const ROCM_BACKEND_NAME: &str = "rocm"; -/// Records the extracted embeddable version next to the runtime tree itself -/// (inside `lemonade_root`, not the CLI's own manifest) so the version check -/// survives even when the manifest lives under a data dir that is not shared -/// with the runtime tree (e.g. each e2e scenario's isolated data dir vs. a -/// shared runtime tree). Without this, [`prepare_embeddable`] can never prove -/// the shared tree is already current, wipes it, and re-extracts on every -/// call — destroying any llama.cpp backend installed into it in the process. +/// Records the extracted embeddable version inside the runtime tree itself +/// (not the CLI's own manifest) so the version check survives even when the +/// manifest lives under a data dir that is not shared with the runtime tree +/// (e.g. each e2e scenario's isolated data dir vs. a shared runtime tree). +/// Living inside `runtime_dir` rather than beside it also means a wipe of the +/// tree removes the marker in the same operation — "marker present implies +/// extraction completed" holds by construction. Without this marker, +/// [`prepare_embeddable`] can never prove the shared tree is already current, +/// wipes it, and re-extracts on every call — destroying any llama.cpp backend +/// installed into it in the process. const RUNTIME_VERSION_MARKER: &str = "runtime-version.txt"; #[cfg(feature = "e2e-test-hooks")] const BACKEND_INSTALL_FAILURE_TEST_ENV: &str = "ROCM_E2E_LEMONADE_BACKEND_INSTALL_FAILURE"; @@ -1059,6 +1062,27 @@ fn needs_extraction( reinstall || !server_present || installed != Some(wanted) } +/// The Lemonade runtime version last extracted into `runtime_dir`, if any. +fn installed_runtime_version(runtime_dir: &Path) -> Option { + fs::read_to_string(runtime_dir.join(RUNTIME_VERSION_MARKER)) + .ok() + .map(|s| s.trim().to_owned()) +} + +/// Record `version` as installed into `runtime_dir`. Marker lives inside +/// `runtime_dir` (not beside it) so `remove_dir_all(runtime_dir)` clears it as +/// part of the same wipe that removes the tree it describes — "marker present +/// implies extraction completed" holds by construction, with no separate +/// bookkeeping to keep in sync. +fn record_runtime_version(runtime_dir: &Path, version: &str) -> Result<()> { + fs::write(runtime_dir.join(RUNTIME_VERSION_MARKER), version).with_context(|| { + format!( + "failed to record installed Lemonade runtime version under {}", + runtime_dir.display() + ) + }) +} + fn prepare_embeddable( paths: &AppPaths, env_root: Option<&Path>, @@ -1085,17 +1109,15 @@ fn prepare_embeddable( // version. Without comparing versions the extraction would be skipped and // the old binaries reported as the new version. // - // The version is read from a marker inside `root` (the same shared tree - // as `runtime_dir`), not from the CLI's own manifest: the manifest lives + // The version is read from a marker inside `runtime_dir` (the same shared + // tree it describes), not from the CLI's own manifest: the manifest lives // under `paths` (the per-invocation data dir), which is not guaranteed to // be the same data dir that last extracted into this `runtime_dir` (e.g. // each e2e scenario gets an isolated data dir but shares one runtime // tree). Sourcing the check from the isolated manifest would report // "unknown version" on every call, wipe the shared tree, and re-extract // over it — destroying any llama.cpp backend already installed there. - let installed_version = fs::read_to_string(root.join(RUNTIME_VERSION_MARKER)) - .ok() - .map(|s| s.trim().to_owned()); + let installed_version = installed_runtime_version(&runtime_dir); if needs_extraction( reinstall, installed_version.as_deref(), @@ -1114,12 +1136,7 @@ fn prepare_embeddable( let embeddable_root = find_embeddable_root(&extract_root)?; copy_tree(&embeddable_root, &runtime_dir)?; fs::remove_dir_all(&extract_root).ok(); - fs::write(root.join(RUNTIME_VERSION_MARKER), &version).with_context(|| { - format!( - "failed to record installed Lemonade runtime version under {}", - root.display() - ) - })?; + record_runtime_version(&runtime_dir, &version)?; } drop(archive_guard); let lemond = lemond_path_in(&runtime_dir); @@ -5033,20 +5050,21 @@ mod tests { #[test] fn runtime_version_marker_survives_without_a_manifest() { // The regression this guards: an isolated caller (e.g. a data dir that - // has never seen this `root` before) must still recognize a runtime - // tree extracted by a *different* caller sharing the same `root`, or - // `prepare_embeddable` wipes and re-extracts it on every call. - let dir = - std::env::temp_dir().join(format!("rocm-lemonade-marker-test-{}", std::process::id())); - fs::create_dir_all(&dir).unwrap(); - let marker = dir.join(RUNTIME_VERSION_MARKER); - - assert!(fs::read_to_string(&marker).is_err()); - - fs::write(&marker, "7.13.0\n").unwrap(); - let installed = fs::read_to_string(&marker) - .ok() - .map(|s| s.trim().to_owned()); + // has never seen this runtime tree before) must still recognize a + // runtime tree extracted by a *different* caller sharing the same + // tree, or `prepare_embeddable` wipes and re-extracts it on every + // call. Goes through `installed_runtime_version`/`record_runtime_version` + // (the exact functions `prepare_embeddable` calls) rather than + // reimplementing their read/write, so reverting either production + // function to source the version from the manifest instead fails + // this test. + let dir = tempfile::tempdir().unwrap(); + let runtime_dir = dir.path(); + + assert_eq!(installed_runtime_version(runtime_dir), None); + + record_runtime_version(runtime_dir, "7.13.0").unwrap(); + let installed = installed_runtime_version(runtime_dir); assert_eq!(installed.as_deref(), Some("7.13.0")); assert!(!needs_extraction( false, @@ -5060,8 +5078,6 @@ mod tests { "7.14.0", true )); - - fs::remove_dir_all(&dir).ok(); } #[test] From 91851e953aa7fa7afc0b870da53a6b68603f7726 Mon Sep 17 00:00:00 2001 From: Juho Vainio Date: Tue, 8 Sep 2026 11:49:42 +0000 Subject: [PATCH 08/15] fix(core): recover from a poisoned PROCESS_ENV_TEST_LOCK instead of panicking MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both fix.rs tests guarded by this lock use plain .lock().unwrap(). If either test fails while holding the guard, the mutex poisons and the other test panics with PoisonError instead of its own assertion — one real failure becomes one real plus one misleading. Switch to .unwrap_or_else(PoisonError::into_inner), mirroring the pattern already used at main.rs:19174. Also: the race this lock (added in 090231e8) serializes against is only reachable on the Windows test lane — Linux CI runs cargo nextest (process-per-test, so two tests can never see each other's env write), while Windows runs cargo test --workspace --all-targets (thread-per-test). Noting that here since 090231e8 reads as a general fix without it. Per review from @tomastola and @rominf on #355. Signed-off-by: Juho Vainio --- crates/rocm-core/src/fix.rs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/crates/rocm-core/src/fix.rs b/crates/rocm-core/src/fix.rs index 5750a0411..2a11ba085 100644 --- a/crates/rocm-core/src/fix.rs +++ b/crates/rocm-core/src/fix.rs @@ -1013,7 +1013,9 @@ mod tests { // outright, so it returned nothing here no matter what was planted. // Going through the shared resolver is what makes this pass -- and is // what stops fix-6-path putting 6.2 on PATH when 6.10 is installed. - let _guard = PROCESS_ENV_TEST_LOCK.lock().unwrap(); + let _guard = PROCESS_ENV_TEST_LOCK + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner); let root = std::env::temp_dir().join(format!( "rocm-fix-path-resolver-{}-{:?}", std::process::id(), @@ -1048,7 +1050,9 @@ mod tests { // The old scan accepted any directory whose name started with a digit, // so an empty leftover could be put on PATH. The resolver requires a // marker. - let _guard = PROCESS_ENV_TEST_LOCK.lock().unwrap(); + let _guard = PROCESS_ENV_TEST_LOCK + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner); let root = std::env::temp_dir().join(format!( "rocm-fix-path-empty-{}-{:?}", std::process::id(), From f5fe94de9acf6dc5eaac38c58e523bb83fccb864 Mon Sep 17 00:00:00 2001 From: Juho Vainio Date: Wed, 9 Sep 2026 08:33:52 +0000 Subject: [PATCH 09/15] fix(core): write the startup-update-check record atomically MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit save_startup_update_check used a plain fs::write while the neighboring file in the same cache dir (write_cached_http_entry) already writes through write_file_atomically. Two concurrent `rocm` invocations on one machine could tear the record, and load_startup_update_check hard-errors on a torn file, so `rocm update` would exit non-zero for a real user — not just under the e2e suite's now-shared cache dir. Swap in the existing atomic writer. This also lets e2e drop the ROCM_CLI_DISABLE_STARTUP_UPDATE_CHECK workaround added for the same hazard, restoring update.feature's coverage of the startup_check code path. Per review from @tomastola on #355. Signed-off-by: Juho Vainio --- apps/rocm/src/therock.rs | 10 ++-------- tests/e2e-cucumber/tests/e2e.rs | 14 ++++++-------- 2 files changed, 8 insertions(+), 16 deletions(-) diff --git a/apps/rocm/src/therock.rs b/apps/rocm/src/therock.rs index 6d34ba253..0f7d35c7e 100644 --- a/apps/rocm/src/therock.rs +++ b/apps/rocm/src/therock.rs @@ -774,17 +774,11 @@ fn load_startup_update_check(paths: &AppPaths) -> Result Result<()> { let path = startup_update_check_path(paths); - let parent = path - .parent() - .context("startup update check path has no parent directory")?; - fs::create_dir_all(parent)?; - fs::write( + write_file_atomically( &path, - serde_json::to_vec_pretty(record) + &serde_json::to_vec_pretty(record) .context("failed to serialize startup update check record")?, ) - .with_context(|| format!("failed to write {}", path.display()))?; - Ok(()) } fn install_wheel_runtime( diff --git a/tests/e2e-cucumber/tests/e2e.rs b/tests/e2e-cucumber/tests/e2e.rs index 1440831ac..95b07be53 100644 --- a/tests/e2e-cucumber/tests/e2e.rs +++ b/tests/e2e-cucumber/tests/e2e.rs @@ -249,14 +249,12 @@ impl E2eWorld { // (rocm-engine-lemonade's `RUNTIME_VERSION_MARKER`). // // Two paths under this cache are mutable rather than content- - // addressed: `cache/therock/startup-update-check.json` (a plain, - // non-atomic `fs::write`) and `cache/therock/metadata/*.json` (the - // etag revalidation cache, atomically written). Concurrent scenarios - // now sharing this dir could race the former into a torn file, which - // `load_startup_update_check` treats as a hard error — so disable the - // check suite-wide rather than let a shared, non-atomic write become - // a flaky `rocm` exit code. - env.push(("ROCM_CLI_DISABLE_STARTUP_UPDATE_CHECK", "1".into())); + // addressed: `cache/therock/startup-update-check.json` and + // `cache/therock/metadata/*.json` (the etag revalidation cache). + // Both are now written atomically (`save_startup_update_check` / + // `write_cached_http_entry`), so concurrent scenarios sharing this + // dir can't tear either into a state `load_startup_update_check` + // would hard-error on. let cache_dir = shared_cache_dir() .map_or_else(|| root.join("cache"), |shared| shared.join("rocm-cli")); env.push(("ROCM_CLI_CACHE_DIR", cache_dir.into_os_string())); From e2bae9c8d5cea3b8a7151ec59964cdc0c0b0fbff Mon Sep 17 00:00:00 2001 From: Juho Vainio Date: Wed, 9 Sep 2026 08:34:25 +0000 Subject: [PATCH 10/15] fix(e2e): derive max_concurrent from a configured shared cache dir too MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit max_concurrent serialized scenarios (the invariant cache-dir sharing's safety rests on) only when the GPU-capability probe read true. The WSL2 lane already disagreed with its own export: cap.has_amd_gpu reads false there (no working rocm-smi), while the lane still sets E2E_SHARED_CACHE_DIR — so it ran up to 64 scenarios against one shared cache dir, and only stayed green because it happens to skip the scenarios that would race. Derive the cap from the hazard itself (a shared cache dir is configured) in addition to the GPU probe, so a lane like this becomes serialized-and-slower instead of racing-and-accidentally-green. Per review from @tomastola on #355. Signed-off-by: Juho Vainio --- tests/e2e-cucumber/tests/e2e.rs | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/tests/e2e-cucumber/tests/e2e.rs b/tests/e2e-cucumber/tests/e2e.rs index 95b07be53..46dab8fd5 100644 --- a/tests/e2e-cucumber/tests/e2e.rs +++ b/tests/e2e-cucumber/tests/e2e.rs @@ -1160,12 +1160,16 @@ async fn main() { // (see `isolate_env`): readers extracting an archive and a concurrent // `remove_file` of that same archive after another scenario's extract // (`uv.rs`, `therock.rs`) would otherwise race. `cap.has_amd_gpu` is a - // capability *probe*, not the CI lane's own knowledge of whether it set a - // shared cache dir — if a lane exports `E2E_SHARED_CACHE_DIR` while its - // probe reads false (e.g. a missing GPU driver), it would silently run up - // to 64 scenarios against one shared cache dir. Keep the two coupled if - // either changes. - let max_concurrent = if cap.has_amd_gpu { 1 } else { 64 }; + // capability *probe* that can disagree with what a lane actually exports + // (e.g. a missing GPU driver reads `false` while `E2E_SHARED_CACHE_DIR` is + // still set) — so derive the cap from the hazard itself, a configured + // shared cache dir, rather than only from the probe. A lane that races + // becomes serialized-and-slower instead of racing. + let max_concurrent = if cap.has_amd_gpu || shared_cache_dir().is_some() { + 1 + } else { + 64 + }; let summary = E2eWorld::cucumber() .max_concurrent_scenarios(max_concurrent) // Record the scenario name on the World before each scenario so every From b34d2f40f42dd7c11b147323a9b66a80ff63d4fa Mon Sep 17 00:00:00 2001 From: Juho Vainio Date: Wed, 9 Sep 2026 08:34:50 +0000 Subject: [PATCH 11/15] fix(lemonade): make prepare_embeddable itself the seam under test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit runtime_version_marker_survives_without_a_manifest called only installed_runtime_version/record_runtime_version directly, so it pinned their round-trip but never constrained prepare_embeddable's choice of which source to trust. Reverting installed_version's read back to the pre-fix manifest lookup — the actual historic bug — left all tests green; so did dropping the record_runtime_version call site. Neither mutation was caught, despite an earlier (wrong) claim that they were. Split prepare_embeddable into a thin production wrapper and a prepare_embeddable_with that takes the archive version, its expected sha256, and the download step as parameters, mirroring the existing generic ensure_cached_archive. A new test plants a runtime tree with a matching marker and a sentinel file, calls prepare_embeddable_with against a fresh, manifest-less AppPaths with a stubbed download step, and asserts the sentinel survives. This fails under the manifest-read mutation (confirmed) and passes on the current code. Renamed the original test to make clear it only covers the helpers' round-trip through needs_extraction, not the wiring. Per review from @tomastola on #355. Signed-off-by: Juho Vainio --- engines/lemonade/src/lib.rs | 102 ++++++++++++++++++++++++++++++------ 1 file changed, 85 insertions(+), 17 deletions(-) diff --git a/engines/lemonade/src/lib.rs b/engines/lemonade/src/lib.rs index 7432b726e..3432b9ba8 100644 --- a/engines/lemonade/src/lib.rs +++ b/engines/lemonade/src/lib.rs @@ -1088,8 +1088,33 @@ fn prepare_embeddable( env_root: Option<&Path>, reinstall: bool, ) -> Result { + prepare_embeddable_with( + paths, + env_root, + reinstall, + rocm_deps::LEMONADE_VERSION, + embeddable_archive_sha256(), + download_file, + ) +} + +/// Same as [`prepare_embeddable`], but with the archive version, its expected +/// sha256, and the download step all injectable — the seam a test uses to +/// exercise the real decision of which source `installed_version` trusts, +/// rather than reimplementing that decision inline. +fn prepare_embeddable_with( + paths: &AppPaths, + env_root: Option<&Path>, + reinstall: bool, + version: &str, + expected_sha256: &str, + download: F, +) -> Result +where + F: FnMut(&str, &Path) -> Result<()>, +{ let root = lemonade_root(paths, env_root); - let version = rocm_deps::LEMONADE_VERSION.to_owned(); + let version = version.to_owned(); let archive_name = embeddable_archive_name(&version); let archive_url = embeddable_url(&version); let downloads = root.join("downloads"); @@ -1097,12 +1122,7 @@ fn prepare_embeddable( fs::create_dir_all(&downloads)?; // Held until the archive bytes have been extracted and copied out: the // verified bytes and the extracted bytes must be the same read. - let archive_guard = ensure_cached_archive( - &archive_url, - &archive, - embeddable_archive_sha256(), - download_file, - )?; + let archive_guard = ensure_cached_archive(&archive_url, &archive, expected_sha256, download)?; let runtime_dir = runtime_dir_in(&root); // The runtime directory is not version-scoped, so a bump downloads a new // archive into a tree that already holds `lemond` from the previous @@ -5048,16 +5068,12 @@ mod tests { } #[test] - fn runtime_version_marker_survives_without_a_manifest() { - // The regression this guards: an isolated caller (e.g. a data dir that - // has never seen this runtime tree before) must still recognize a - // runtime tree extracted by a *different* caller sharing the same - // tree, or `prepare_embeddable` wipes and re-extracts it on every - // call. Goes through `installed_runtime_version`/`record_runtime_version` - // (the exact functions `prepare_embeddable` calls) rather than - // reimplementing their read/write, so reverting either production - // function to source the version from the manifest instead fails - // this test. + fn runtime_version_marker_round_trips_through_needs_extraction() { + // Unit coverage for the helpers themselves and their wiring into + // `needs_extraction` — NOT a regression test for the bug this marker + // fixes (see `prepare_embeddable_trusts_the_runtime_tree_over_an_absent_manifest` + // below for that): this never calls `prepare_embeddable`, so it can't + // constrain which source it reads `installed_version` from. let dir = tempfile::tempdir().unwrap(); let runtime_dir = dir.path(); @@ -5080,6 +5096,58 @@ mod tests { )); } + #[test] + fn prepare_embeddable_trusts_the_runtime_tree_over_an_absent_manifest() { + // The actual regression: `prepare_embeddable` must source + // `installed_version` from the marker inside `runtime_dir`, not from + // `paths`' manifest. An isolated caller (fresh `AppPaths`, no + // manifest on disk) sharing a runtime tree that a *different* caller + // already extracted must recognize it as current and skip + // re-extraction. Reverting to a manifest read (the original bug) + // makes every isolated caller miss, wipe, and re-extract — destroying + // any llama.cpp backend already installed there. Calling + // `prepare_embeddable_with` (the real function, seamed for an + // injectable download step) rather than reimplementing its decision + // inline is what makes this test fail under that reversion. + let env_root = tempfile::tempdir().unwrap(); + let paths = AppPaths { + config_dir: env_root.path().join("config"), + data_dir: env_root.path().join("data"), // no manifest ever written here + cache_dir: env_root.path().join("cache"), + }; + let version = "7.13.0"; + let root = lemonade_root(&paths, Some(env_root.path())); + let runtime_dir = runtime_dir_in(&root); + fs::create_dir_all(&runtime_dir).unwrap(); + fs::write(lemond_path_in(&runtime_dir), b"fake lemond").unwrap(); + fs::write(lemonade_path_in(&runtime_dir), b"fake lemonade").unwrap(); + record_runtime_version(&runtime_dir, version).unwrap(); + let sentinel = runtime_dir.join("llamacpp-rocm-backend.sentinel"); + fs::write(&sentinel, b"a backend a prior scenario installed").unwrap(); + + let archive_bytes = b"never parsed unless extraction wrongly triggers"; + let expected_sha256 = format!("{:x}", Sha256::digest(archive_bytes)); + + let manifest = prepare_embeddable_with( + &paths, + Some(env_root.path()), + false, + version, + &expected_sha256, + |_, destination| { + fs::write(destination, archive_bytes)?; + Ok(()) + }, + ) + .unwrap(); + + assert_eq!(manifest.version, version); + assert!( + sentinel.is_file(), + "prepare_embeddable wiped the shared runtime tree instead of recognizing it as current" + ); + } + #[test] fn lemonade_root_uses_requested_engine_root() { let paths = AppPaths { From 07dc8d45ef8cbbf1d18bd2c7a5d85facbf66b2c0 Mon Sep 17 00:00:00 2001 From: Juho Vainio Date: Wed, 9 Sep 2026 10:21:17 +0000 Subject: [PATCH 12/15] fix(e2e): also serialize when a shared runtimes dir is configured MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit max_concurrent only keyed on shared_cache_dir(), missing the other shared mutable tree: two lanes (e2e-selfhosted.yml, nightly.yml, both Windows PowerShell blocks) export E2E_SHARED_RUNTIMES_DIR without E2E_SHARED_CACHE_DIR, so the predicate collapsed back to the bare GPU-capability probe on those lanes — the exact probe-vs-lane disagreement this change exists to stop trusting. Left unserialized, this is the shared runtime tree the PR is titled after: on a version bump, two concurrent scenarios could both enter the extraction branch, one wiping the tree the other is reading. Add shared_runtimes_dir() to the predicate. Left shared_uv_cache_dir() out deliberately — uv's cache is content-addressed and uv does its own locking. Per review from @tomastola on #355. Signed-off-by: Juho Vainio --- tests/e2e-cucumber/tests/e2e.rs | 31 +++++++++++++++++-------------- 1 file changed, 17 insertions(+), 14 deletions(-) diff --git a/tests/e2e-cucumber/tests/e2e.rs b/tests/e2e-cucumber/tests/e2e.rs index 46dab8fd5..fa81680af 100644 --- a/tests/e2e-cucumber/tests/e2e.rs +++ b/tests/e2e-cucumber/tests/e2e.rs @@ -1156,20 +1156,23 @@ async fn main() { // on OS-assigned ports, so they're safe to run concurrently). // // This `max_concurrent == 1` is ALSO what makes it safe for a GPU lane to - // point `shared_cache_dir()`/`shared_uv_cache_dir()` at one persistent dir - // (see `isolate_env`): readers extracting an archive and a concurrent - // `remove_file` of that same archive after another scenario's extract - // (`uv.rs`, `therock.rs`) would otherwise race. `cap.has_amd_gpu` is a - // capability *probe* that can disagree with what a lane actually exports - // (e.g. a missing GPU driver reads `false` while `E2E_SHARED_CACHE_DIR` is - // still set) — so derive the cap from the hazard itself, a configured - // shared cache dir, rather than only from the probe. A lane that races - // becomes serialized-and-slower instead of racing. - let max_concurrent = if cap.has_amd_gpu || shared_cache_dir().is_some() { - 1 - } else { - 64 - }; + // point `shared_cache_dir()`/`shared_runtimes_dir()`/`shared_uv_cache_dir()` + // at one persistent dir (see `isolate_env`): readers extracting an archive + // and a concurrent `remove_file`/`remove_dir_all` of that same archive or + // runtime tree after another scenario's extract (`uv.rs`, `therock.rs`, + // rocm-engine-lemonade's `prepare_embeddable`) would otherwise race. + // `cap.has_amd_gpu` is a capability *probe* that can disagree with what a + // lane actually exports (e.g. a missing GPU driver reads `false` while a + // shared dir env var is still set) — so derive the cap from the hazards + // themselves rather than only from the probe. `shared_uv_cache_dir()` is + // deliberately excluded: uv's cache is content-addressed and uv does its + // own locking. A lane that races becomes serialized-and-slower instead. + let max_concurrent = + if cap.has_amd_gpu || shared_cache_dir().is_some() || shared_runtimes_dir().is_some() { + 1 + } else { + 64 + }; let summary = E2eWorld::cucumber() .max_concurrent_scenarios(max_concurrent) // Record the scenario name on the World before each scenario so every From 631bed121f16dd114ca265dbbb81361abc09394f Mon Sep 17 00:00:00 2001 From: Juho Vainio Date: Wed, 9 Sep 2026 10:21:32 +0000 Subject: [PATCH 13/15] fix(lemonade): cover the write side of the marker regression too prepare_embeddable_trusts_the_runtime_tree_over_an_absent_manifest only covers the read side (installed_version's source). Dropping the record_runtime_version call site in prepare_embeddable's extraction branch is the same defect from the write side -- marker absent after extraction, so the next isolated caller sees no marker, wipes, and re-extracts on every call -- and was uncaught. Add a sibling test using a real, well-formed archive (this crate already builds one for extract_archive_writes_a_well_formed_tree): extract into an empty runtime_dir via prepare_embeddable_with, then assert the marker was recorded. Confirmed it fails under the dropped call site. Per review from @tomastola on #355. Signed-off-by: Juho Vainio --- engines/lemonade/src/lib.rs | 65 +++++++++++++++++++++++++++++++++++++ 1 file changed, 65 insertions(+) diff --git a/engines/lemonade/src/lib.rs b/engines/lemonade/src/lib.rs index 3432b9ba8..ce2f9aafb 100644 --- a/engines/lemonade/src/lib.rs +++ b/engines/lemonade/src/lib.rs @@ -5148,6 +5148,71 @@ mod tests { ); } + #[test] + fn prepare_embeddable_records_the_version_after_a_real_extraction() { + // The write-side sibling of the test above: an isolated caller + // extracting into an empty `runtime_dir` for the first time must + // leave a marker behind, or the *next* isolated caller sharing this + // tree sees no marker, wipes, and re-extracts on every call — the + // same defect the read-side regression guards against, from the + // write side instead. Dropping `record_runtime_version`'s call site + // in `prepare_embeddable` fails this test. + let env_root = tempfile::tempdir().unwrap(); + let paths = AppPaths { + config_dir: env_root.path().join("config"), + data_dir: env_root.path().join("data"), + cache_dir: env_root.path().join("cache"), + }; + let version = "7.13.0"; + + // A real, well-formed archive (unlike the stub above): this test + // exercises the extraction branch, so `extract_archive` must succeed. + let archive_dir = tempfile::tempdir().unwrap(); + let archive = archive_dir.path().join("embeddable.tar.gz"); + { + let file = fs::File::create(&archive).unwrap(); + let encoder = flate2::write::GzEncoder::new(file, flate2::Compression::fast()); + let mut builder = tar::Builder::new(encoder); + for name in [ + platform_binary_name("lemond"), + platform_binary_name("lemonade"), + ] { + let mut header = tar::Header::new_ustar(); + header.set_size(5); + header.set_mode(0o755); + header.set_entry_type(tar::EntryType::Regular); + header.set_path(format!("embeddable/bin/{name}")).unwrap(); + header.set_cksum(); + builder.append(&header, &b"hello"[..]).unwrap(); + } + builder.into_inner().unwrap().finish().unwrap(); + } + let expected_sha256 = format!("{:x}", Sha256::digest(&fs::read(&archive).unwrap())); + + let root = lemonade_root(&paths, Some(env_root.path())); + let runtime_dir = runtime_dir_in(&root); + assert_eq!(installed_runtime_version(&runtime_dir), None); + + prepare_embeddable_with( + &paths, + Some(env_root.path()), + false, + version, + &expected_sha256, + |_, destination| { + fs::copy(&archive, destination)?; + Ok(()) + }, + ) + .unwrap(); + + assert_eq!( + installed_runtime_version(&runtime_dir).as_deref(), + Some(version), + "prepare_embeddable extracted the archive but never recorded the version" + ); + } + #[test] fn lemonade_root_uses_requested_engine_root() { let paths = AppPaths { From 40215a00757cc50247f4306f3caae5a9b149661f Mon Sep 17 00:00:00 2001 From: Juho Vainio Date: Wed, 9 Sep 2026 10:39:51 +0000 Subject: [PATCH 14/15] fix(lemonade): drop a needless borrow flagged by clippy --all-targets Sha256::digest takes impl AsRef<[u8]>, which Vec already satisfies; &fs::read(...).unwrap() borrowed needlessly. My earlier `cargo clippy --lib` runs never compiled #[cfg(test)] code, so this slipped past locally and only surfaced in CI's `--workspace --all-targets`. Signed-off-by: Juho Vainio --- engines/lemonade/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/engines/lemonade/src/lib.rs b/engines/lemonade/src/lib.rs index ce2f9aafb..896500030 100644 --- a/engines/lemonade/src/lib.rs +++ b/engines/lemonade/src/lib.rs @@ -5187,7 +5187,7 @@ mod tests { } builder.into_inner().unwrap().finish().unwrap(); } - let expected_sha256 = format!("{:x}", Sha256::digest(&fs::read(&archive).unwrap())); + let expected_sha256 = format!("{:x}", Sha256::digest(fs::read(&archive).unwrap())); let root = lemonade_root(&paths, Some(env_root.path())); let runtime_dir = runtime_dir_in(&root); From 5c75282bc0b50ff46ce8c1e477d53eaca3098814 Mon Sep 17 00:00:00 2001 From: Juho Vainio Date: Wed, 9 Sep 2026 11:05:47 +0000 Subject: [PATCH 15/15] fix(lemonade): build a platform-matching archive in the write-side test prepare_embeddable_records_the_version_after_a_real_extraction always built a tar.gz, but extract_archive dispatches on the destination's extension, which prepare_embeddable_with derives from the current platform (embeddable_os_arch: .zip on Windows, .tar.gz elsewhere). On Windows CI the destination ends in .zip, so the tar.gz bytes failed zip parsing ("Could not find EOCD") before the assertion ever ran. Build a zip via zip::ZipWriter when runtime_is_windows(), matching the existing zip-building tests in this file. Both branches are runtime `if`s rather than #[cfg], so a single-platform CI run still type-checks both. Signed-off-by: Juho Vainio --- engines/lemonade/src/lib.rs | 29 +++++++++++++++++++++++------ 1 file changed, 23 insertions(+), 6 deletions(-) diff --git a/engines/lemonade/src/lib.rs b/engines/lemonade/src/lib.rs index 896500030..aa527544a 100644 --- a/engines/lemonade/src/lib.rs +++ b/engines/lemonade/src/lib.rs @@ -5167,16 +5167,33 @@ mod tests { // A real, well-formed archive (unlike the stub above): this test // exercises the extraction branch, so `extract_archive` must succeed. + // `extract_archive` dispatches on the destination's extension, which + // `prepare_embeddable_with` derives from the current platform (see + // `embeddable_os_arch`) — so the bytes built here must be in that + // same format, not always a tar.gz. + let (_, extension) = embeddable_os_arch(); let archive_dir = tempfile::tempdir().unwrap(); - let archive = archive_dir.path().join("embeddable.tar.gz"); - { + let archive = archive_dir.path().join(format!("embeddable.{extension}")); + let entry_names = [ + platform_binary_name("lemond"), + platform_binary_name("lemonade"), + ]; + if runtime_is_windows() { + let file = fs::File::create(&archive).unwrap(); + let mut writer = zip::ZipWriter::new(file); + let options = zip::write::SimpleFileOptions::default(); + for name in &entry_names { + writer + .start_file(format!("embeddable/bin/{name}"), options) + .unwrap(); + writer.write_all(b"hello").unwrap(); + } + writer.finish().unwrap(); + } else { let file = fs::File::create(&archive).unwrap(); let encoder = flate2::write::GzEncoder::new(file, flate2::Compression::fast()); let mut builder = tar::Builder::new(encoder); - for name in [ - platform_binary_name("lemond"), - platform_binary_name("lemonade"), - ] { + for name in &entry_names { let mut header = tar::Header::new_ustar(); header.set_size(5); header.set_mode(0o755);