fix(e2e,lemonade): share the therock/tool cache and fix the shared runtime tree getting wiped per scenario

[juhovainio]

Summary

E2E scenarios weren't sharing caches as intended, so each one re-downloaded the same large artifacts instead of reusing what a prior scenario had already fetched. This PR shares the CLI's own therock/tool cache like the other caches, and fixes a deeper bug that was silently wiping the shared Lemonade runtime tree (and the ~3.3GB llamacpp:rocm backend inside it) on every scenario.

Fixes EAI-8572.

What's addressed

• Route ROCM_CLI_CACHE_DIR (therock SDK/tool archive cache) through the shared, persistent E2E cache dir, same as HF_HOME/PIP_CACHE_DIR. Local runs keep the old fully-isolated cache.
• Fix prepare_embeddable() re-extracting and wiping the shared Lemonade runtime tree on every scenario, because its version check read a manifest under each scenario's isolated data dir (always a miss). Now tracked via a marker file (runtime-version.txt) inside the runtime tree itself. This is what was actually causing the llamacpp:rocm backend to re-download every scenario — credit to @tomastola's review for tracing it to this path.
• Correct the isolate_env() doc comment, which previously credited ROCM_CLI_CACHE_DIR for the backend savings (it never covered that path); the shared_cache_dir() doc comment had drifted the other way and contradicted it, so reworded that too (per @tomastola and @rominf).
• Split prepare_embeddable() into a thin wrapper and a prepare_embeddable_with() seam (version/sha256/download step all injectable, mirroring the existing generic ensure_cached_archive), and added a test that calls the real function against a planted runtime tree + a fresh, manifest-less AppPaths. The earlier rewrite of runtime_version_marker_survives_without_a_manifest still only exercised the helpers directly, so it didn't actually constrain which source prepare_embeddable trusts — @tomastola re-ran the mutation experiment against the real bug (reverting the version read to the old manifest lookup) and it stayed green despite my earlier claim otherwise. The new seam-based test does fail under that mutation (verified again, this time correctly).
• Moved the runtime-version.txt 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, closing the same-version-reinstall/mid-copy-kill window @tomastola and @rominf both raised).
• save_startup_update_check now writes through the existing write_file_atomically instead of a plain fs::write — @rominf found the shared e2e cache dir made a pre-existing tear hazard reachable, but @tomastola pointed out the same tear is possible for any user with two concurrent rocm invocations on one machine (rocm update would exit non-zero on a torn file). Fixing the write in production rather than disabling the check in e2e closes it for real users too, and restores update.feature's e2e coverage of that code path.
• max_concurrent (the cap that makes cache-dir sharing safe) is now derived from whether a shared cache dir is actually configured, not only from the GPU-capability probe — @rominf found the WSL2 lane already disagreeing with itself (probe reads false, no working rocm-smi, while the lane still exports a shared cache dir) and @tomastola suggested deriving the cap from the hazard directly so that lane becomes serialized-and-slower instead of racing-and-accidentally-green.
• Fixed the poisoned-mutex cascade in fix.rs's PROCESS_ENV_TEST_LOCK tests (.unwrap_or_else(PoisonError::into_inner), mirroring the existing pattern in main.rs) and clarified in that commit's message that the race it serializes against is reachable only on the Windows test lane (Linux CI runs cargo nextest, process-per-test) — per @tomastola/@rominf.

Not addressed (follow-ups)

Tracked in #359 rather than here, per @tomastola's request that these stay discoverable after merge:

• Stale tests/e2e-cucumber/README.md:100 doc row.
• Pre-warm (xtask/src/e2e_prewarm.rs:723) doesn't point at the shared cache, so the first scenario still pays the therock/tool cold cost.
• rocm uninstall / build_downloads_plan wipe hazard against a shared cache dir.
• Unbounded growth across the (now three) shared caches — capacity, not correctness.

Test plan

• cargo check -p e2e-cucumber --tests
• cargo fmt --all -- --check
• cargo clippy -p e2e-cucumber --tests -- -D warnings
• cargo test -p rocm-engine-lemonade --lib (96 passed, incl. the seam-based prepare_embeddable_trusts_the_runtime_tree_over_an_absent_manifest regression test — verified via mutation against the actual historic bug, not just the helpers)
• cargo clippy -p rocm-engine-lemonade --lib -- -D warnings
• cargo clippy -p rocm --bin rocm -- -D warnings / cargo test -p rocm --bin rocm therock:: (71 passed)
• cargo clippy -p rocm-core --lib -- -D warnings
• cargo test -p rocm-core --lib fix:: — the two tests touched by the PoisonError change pass individually; the full-module run has one pre-existing failure on dev boxes with a real /opt/rocm install (reproduced identically on pre-fix HEAD, unrelated to this change)
• CI E2E run shows a single cold download of the llamacpp:rocm backend per job instead of one per scenario

[tomastola]

Reviewed at dfb0a61. The mechanism here is sound and low-risk — my concern is about what it targets, not how it's built.

The change likely doesn't fix the reported problem

I tried to find the ~3.3GB llamacpp:rocm backend under ROCM_CLI_CACHE_DIR and couldn't:

• Lemonade's embeddable archive goes to lemonade_root(paths, env_root).join("downloads") (engines/lemonade/src/lib.rs:1064). lemonade_root (:1310) resolves to <env_root>/lemonade or paths.engine_dir("lemonade"), and engine_dir is data_dir/engines/<engine> (crates/rocm-core/src/lib.rs:1668). Never cache_dir.
• The llamacpp:rocm backend itself is installed by Lemonade's own CLI — run_lemonade_backend_install shells out to lemonade … backends install llamacpp:rocm (engines/lemonade/src/lib.rs:2786). apply_lemonade_process_environment passes no cache variable, and there's no LEMONADE_CACHE_DIR plumbing anywhere in the tree.
• engines/lemonade/src/lib.rs touches paths.cache_dir in exactly one place — line 3201 — and only as a read-only HF-hub search root.

What actually persists under cache_dir, as far as I can tell:

Path	Contents	Size
cache/therock/<tarball>	TheRock SDK tarball — deleted immediately after extraction (extract_tarball_and_discard_archive, apps/rocm/src/therock.rs:2569-2578)	never accumulates
cache/therock/metadata/*	channel index body + signature + etag entry	KBs
cache/tools/uv/<version>/<asset>	the uv binary archive (crates/rocm-core/src/uv.rs:415-419)	~tens of MB

Managed Python doesn't land there either — it's uv python install into uv's own dirs (apps/rocm/src/therock.rs:3414).

A likelier cause of the per-scenario re-download: the engines tree hangs off the runtime install root — env_root_for_runtime returns manifest.install_root.join("engines") (apps/rocm/src/main.rs:3882). That tree is shared only for scenarios opting into use_shared_runtimes() / E2E_SHARED_RUNTIMES_DIR; the ones that don't opt in reinstall the engine, backend and all. If that's what's happening, the lever is the runtimes axis (or ROCM_CLI_ENGINE_ENVS_ROOT, which engine_envs_root already honours at crates/rocm-core/src/lib.rs:1681), not ROCM_CLI_CACHE_DIR.

Could you confirm the on-runner path of the 3.3GB artifact before merging? A find $RUNNER_WORKSPACE -size +1G during a job settles it. If it lands under e2e-shared/rocm-cli, my trace is wrong and this is good to go.

Comment accuracy

Independent of the above: the new comment at tests/e2e-cucumber/tests/e2e.rs:238-245 states the change means "the ~3.3GB llamacpp:rocm backend archive is downloaded once per runner rather than once per scenario," and cites storage::download_cache_dir / tool_download_cache_dir as support. Those are reporting/cleanup accessors (apps/rocm/src/storage.rs:369-376) — tool_download_cache_dir isn't a writer at all; the only writer into cache/tools is the uv download. A comment naming a specific artifact and size is what the next maintainer will trust instead of re-deriving, so it's worth getting exact either way. Same for the shared_cache_dir doc-comment edit ("multi-GB engine backends").

Stale doc

tests/e2e-cucumber/README.md:100 still says:

| `ROCM_CLI_CACHE_DIR` | (temp dir) | Isolated cache directory per scenario |

Which is no longer true under CI. (The E2E_SHARED_* vars aren't in that table at all — pre-existing gap, not this PR's to close.)

Smaller notes

• The inline note in default() (e2e.rs:201-202) — "State-free content-addressed caches (HF weights, pip, uv) are always shared, in isolate_cmd" — is now incomplete; the CLI cache joins that list. Same block still unconditionally creates the per-scenario cache subdir (:191), now dead when a shared dir is set.
• shared_cache_dir() is called twice per isolate_env() (:246 and :256), each doing an env read plus create_dir_all, and isolate_env() runs for every spawned rocm. One let shared = shared_cache_dir(); reused for HF_HOME/PIP_CACHE_DIR would be tidier.
• The pre-warm points ROCM_CLI_CACHE_DIR at a different tree (xtask/src/e2e_prewarm.rs:723 → prewarm_dir.join("cache")), so the shared cache is never pre-populated and the first scenario still pays cold cost.
• Latent trap worth a guard comment: rocm uninstall without --keep-cache removes paths.cache_dir wholesale (apps/rocm/src/main.rs:17569), and build_downloads_plan (apps/rocm/src/storage.rs:715-720) clears cache/therock + cache/tools. Nothing reaches either through isolate_env() today — the lifecycle uninstall scenarios correctly use their own smoke_cache dirs (lifecycle_steps.rs:894, :952) — but a future scenario that did would silently wipe the shared cache mid-job.
• Concurrency safety is incidental rather than stated: max_concurrent = if cap.has_amd_gpu { 1 } else { 64 } (e2e.rs:1145), and every lane exporting E2E_SHARED_CACHE_DIR is a GPU lane, so scenarios are serialized where it matters. If capability detection ever returns false on a lane that still exports the var, 64 scenarios would share one cache dir. A clause tying the sharing to the concurrency cap would keep a future change to either from quietly breaking the other.

Tradeoff worth naming

Persistent shared state means partial state survives failures. cancel-in-progress: true can kill a job mid-download, and a truncated file now outlives the job instead of dying with the TempDir. The blast radius is genuinely small — the metadata path does atomic publish with signature verification and etag revalidation, everything else is small and re-fetchable — but the failure mode moves from impossible to rare-and-self-healing, and the description reads as though it were free.

Re #352

Different mechanism, no collision — separate subtrees under $RUNNER_WORKSPACE, separate files. Two things carry over:

1. Capacity isn't a concern from this PR (tens of MB on top of #352's ~23GB move, with the headroom #352 measured). But neither PR adds pruning, so the work volume now accumulates three unbounded caches — probably a follow-up ticket rather than anything to change here.
2. #352's verification lesson applies directly to the unchecked test-plan box. A green GPU lane proves nothing about install-time behavior when the pre-warm tree is warm — install sdk never runs, so uv is never invoked. The same reuse likely means the uv-archive and metadata downloads this PR affects don't get exercised either, so "a single cold download per job" may not be observable. Another reason to pin down where the 3.3GB actually lives first.

Things that read well

• Keeping the runtimes registry isolated while sharing the cache is the right cut, and the doc comment says so explicitly with a pointer to the opt-in mechanism — that's the distinction most likely to be got wrong here.
• map_or_else is lazy on both arms, and the clippy follow-up landed on the better form rather than an #[allow].
• Local runs keep full isolation, so the blast radius really is CI-only.

I read source but didn't run cargo check/clippy myself, so the test-plan claims are unverified on my side.

[juhovainio]

Thanks for the detailed trace — you were right on every point, and it saved a lot of time here.

Confirmed via job logs, then root-caused

Pulled the full log for job 101790452022 (e2e-gpu-strix-ubuntu, run 34137029982). Confirmed directly:

• The embeddable archive download is correctly cache-hit every time (Using verified cached … lemonade-embeddable-11.5.1-ubuntu-x64.tar.gz at prewarm, bench-04, chat-05, chat-06 — identical path each time, matching the shared runtimes tree).
• llamacpp:rocm is nonetheless fully reinstalled on every scenario (bench-04 15:40:34Z, chat-05 15:48:56Z, chat-06 15:57:25Z — fresh Installing Lemonade llamacpp:rocm backend..., never Using installed...).

So your trace was exactly right: ROCM_CLI_CACHE_DIR is never in this path, and the engines tree hangs off the shared runtime root as you described. Chasing why the backend specifically didn't survive despite that shared root led to prepare_embeddable() (engines/lemonade/src/lib.rs):

let installed_version = read_manifest(paths)   // reads under paths.data_dir — per-scenario isolated
    .ok()
    .filter(|manifest| manifest.runtime_dir == runtime_dir)
    .map(|manifest| manifest.version);
if needs_extraction(reinstall, installed_version.as_deref(), &version, lemond_path_in(&runtime_dir).is_file()) {
    if runtime_dir.exists() {
        fs::remove_dir_all(&runtime_dir)?;   // <- wipes the SHARED tree
    }
    ...
}

read_manifest reads from paths.engine_manifests_dir(...), which is under the CLI's own (per-scenario, isolated) data dir — never the shared env_root/runtime_dir that the embeddable archive itself lives under. So on every scenario after the first, this manifest read misses, needs_extraction() returns true, and the code deletes and re-extracts the shared runtime_dir — destroying the llamacpp:rocm backend a prior scenario had just installed into it, every single time. The archive download cache-hits because it's keyed independently; the extraction step isn't.

Pushed the fix in b8f9a5e0: the installed-version check now reads a marker (runtime-version.txt) written next to the runtime tree itself (inside lemonade_root), so it survives regardless of which data dir asks. Added a regression test (runtime_version_marker_survives_without_a_manifest) and reran the full rocm-engine-lemonade suite (95/95 pass) + clippy clean.

Comment accuracy / stale doc / smaller notes

Fixed the isolate_env() comment to stop crediting ROCM_CLI_CACHE_DIR for the backend savings and point at the actual mechanism instead. Left the rest of your smaller notes (stale README row, prewarm not pre-populating the shared cache dir, the uninstall/build_downloads_plan wipe hazard, the concurrency-cap coupling) as follow-ups rather than folding them into this PR — noted them in the description. Happy to split those into a tracked follow-up if you'd rather they not linger unassigned.

Test plan

Re: the unchecked GPU-lane box — same caveat you raised for #352 applies here too, now doubly so: a warm pre-warm tree means neither the original ROCM_CLI_CACHE_DIR change nor this backend fix are exercised end-to-end by a green run alone. I don't have a way to force a cold pre-warm from here; flagging that the CI checkbox will need a genuinely cold run (or on-runner inspection) to actually verify, not just a green job.

[tomastola]

Reviewed at 34a4d78. The root cause here is convincing and the fix is the right shape — colocating the version record with the tree it describes is the correct structural answer, and runtime_dir_in(root) = root/runtime makes the dropped manifest.runtime_dir == runtime_dir filter redundant by construction rather than merely unnecessary. Three things I'd want changed before merge, one of which I was able to demonstrate rather than just assert.

1. The shared_cache_dir doc comment now says the opposite of what this PR established

isolate_env() got the correction, but the doc comment ~200 lines above it moved the wrong way (tests/e2e-cucumber/tests/e2e.rs:132-142):

/// 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`). ...
///
/// Sharing these read-only artifacts avoids re-downloading multi-GB engine
/// backends and model weights per scenario.

Both the parenthetical and "avoids re-downloading multi-GB engine backends" attribute the backend to this directory — the precise claim the PR disproves. It now directly contradicts the new comment at :245-249, which says "This does NOT cover the ~3.3GB llamacpp:rocm Lemonade backend." Two comments in one file disagreeing about the same path is worse than the original single wrong comment, because the next reader has to work out which one lost the argument.

Suggest reverting this hunk to something like "avoids re-downloading the therock SDK/tool archives per scenario" and leaving backends entirely to the shared_runtimes_dir doc.

2. The regression test does not exercise the regression

runtime_version_marker_survives_without_a_manifest never calls prepare_embeddable. It writes a file, reads it back with a hand-copy of the production expression, and calls needs_extraction — which the adjacent existing tests already cover. The only production symbols it touches are the RUNTIME_VERSION_MARKER constant and needs_extraction.

I checked this rather than assuming it. On a scratch worktree at 34a4d78 I deleted the production marker write from prepare_embeddable:

-        fs::write(root.join(RUNTIME_VERSION_MARKER), &version).with_context(|| { ... })?;
+        // MUTATION EXPERIMENT: production marker write deliberately removed.

and re-ran it:

running 1 test
test tests::runtime_version_marker_survives_without_a_manifest ... ok
test result: ok. 1 passed; 0 failed; ...

So the exact regression this PR fixes can be fully reintroduced with the test still green. That matters more than usual here, because the PR description cites this test as the guard for the whole change.

The cheap way to bind it to production code is to lift the two halves into named helpers the test can call:

fn installed_runtime_version(root: &Path) -> Option<String> {
    fs::read_to_string(root.join(RUNTIME_VERSION_MARKER)).ok().map(|s| s.trim().to_owned())
}
fn record_runtime_version(root: &Path, version: &str) -> Result<()> { ... }

Then record_runtime_version(root, "7.13.0") followed by assert_eq!(installed_runtime_version(root).as_deref(), Some("7.13.0")) actually fails if either side is removed or reverted to read_manifest. Same test length, and it survives the mutation above.

Minor, same test: the crate already depends on tempfile and uses tempfile::tempdir() at :6208; the pid-named std::env::temp_dir() dir here leaks on panic and skips the cleanup line.

3. The marker can outlive the tree it certifies

The marker lands at root/runtime-version.txt, a sibling of runtime_dir = root/runtime. The extraction branch does:

remove_dir_all(&runtime_dir)?   // marker still on disk, still claiming the old version
extract_archive(...)?           // any ? here, or a job cancellation, exits with the tree gone
copy_tree(...)?
fs::write(marker, &version)?    // only now does the marker match reality

Interrupt anywhere between the wipe and the write and the marker survives describing a tree that no longer exists. Usually harmless — a version bump means installed != wanted, so it re-extracts. The window is same-version: --reinstall, or a retry after a partial copy. Then installed == wanted and the only remaining guard is lemond_path_in(&runtime_dir).is_file(), so a copy_tree that died after placing lemond but before finishing the rest leaves a truncated tree that the marker certifies as complete. cancel-in-progress: true on the e2e workflows makes mid-extraction kills a routine event rather than a hypothetical.

Cleanest fix is to put the marker inside runtime_dir instead of beside it. Then remove_dir_all(&runtime_dir) deletes it as part of the wipe, and "marker present ⟹ copy_tree completed" holds by construction with no extra code — the same structural argument the PR already makes for colocating it with the tree, carried one directory further. If there's a reason it has to stay outside, fs::remove_file(&marker).ok() immediately before the wipe closes the same window.

4. Scope

090231e ("serialize fix.rs tests that mutate process-global ROCM_PATH") is still riding along in crates/rocm-core/src/fix.rs. It's unrelated to both halves of the title, and it's the kind of fix that's easy to justify folding in to get a branch green — but it'll be invisible in this PR's history later. Worth either splitting or naming in the description.

5. On the test plan and what CI actually shows

Your comment says the CI checkbox "will need a genuinely cold run (or on-runner inspection) to actually verify, not just a green job" — but the box is checked in the description. Those should agree; I'd uncheck it and keep your caveat, since an unverified checked box is the one that misleads a future bisect.

That said, there is more signal available than a pass/fail. Comparing the last clean pre-fix e2e run (090231e, run 34139015141) against the post-fix run (34a4d78, run 34156110331):

Lane	Pre-fix	Post-fix	Δ
rad3 R9700	26m45s	13m26s	−13m19s
E2E tests (GPU)	14m23s	13m51s	−32s
Strix Halo, Ubuntu	25m27s	29m15s	+3m48s

The R9700 lane halving is close to what you'd predict from eliminating repeated llamacpp:rocm installs, and it's the strongest evidence in the PR that the fix does what it claims — better than the checkbox. But Strix Halo Ubuntu, the lane you pulled the original diagnostic logs from, got slower. That's the one I'd want explained before merge. n=1 per lane on self-hosted runners with uncontrolled warm-cache state, so none of this is conclusive; a single grep -c 'Installing Lemonade llamacpp:rocm backend' on the post-fix Strix Halo log would settle it outright, and would legitimately check the box.

Re: your offer on the deferred follow-ups

Yes — a tracked follow-up rather than the description list, please. The four items (stale README row, pre-warm not pointing at the shared cache, the uninstall/build_downloads_plan wipe hazard, the concurrency-cap coupling) stop being discoverable the moment this merges. The wipe hazard in particular is a trap for whoever next writes a scenario that shells out to rocm uninstall without --keep-cache.

Verified locally at 34a4d78

• cargo clippy -p rocm-engine-lemonade --lib -- -D warnings — clean
• cargo fmt --all -- --check — clean
• cargo test -p rocm-engine-lemonade --lib — 95 passed, 0 failed

So the local boxes hold up. I did not run the e2e suite.

Things that read well

• Reading the version from the tree instead of the caller's data dir is the right cut, and the comment explains why the manifest was the wrong source rather than just noting that it was — that's the part a future reader would otherwise re-break.
• Keeping the lemond.is_file() term in needs_extraction means an engine-uninstall that removes runtime_dir and leaves the marker still re-extracts correctly. Worth keeping in mind if the marker moves inside runtime_dir per note 3 — it stays correct there too, just for a different reason.
• The RUNTIME_VERSION_MARKER doc comment describes the failure mode, not the datatype. That's the useful kind.

[juhovainio]

E2E CI speedup measurements

Compared job durations on identical self-hosted hardware, before vs. after this fix (same PR branch, few hours apart for most jobs; Ubuntu's "before" pulled from a separate main-branch run since it's the only prior run that landed on the same box).

Job	Runner (machine)	Before	After	Δ
rad3 R9700	r9700-0 (github-runner-0)	26m45s	13m26s	-50%
Strix Halo, Windows	strix-halo-windows (X2-LA01-S05-D04)	23m19s	13m9s	-44%
GPU	mi300x-0 (github-runner-0)	14m23s	13m51s	-4%
Strix Halo, Ubuntu	strix-halo-ubuntu-2 (CORSAIR-AI-WORKSTATION-300)	>34m26s¹	29m15s	>-15%
Strix Halo, WSL2	strix-halo-wsl (X2-LA01-S05-D04)	1m32s	1m20s	-13%
Wall-clock, whole self-hosted group²	—	~43m42s	~29m16s	-33%

¹ From run 34104408080 (main, workflow_dispatch), the only prior run that happened to land on strix-halo-ubuntu-2. That run hit the job's timeout-minutes: 35 cap (e2e-selfhosted.yml) and was killed at 34m26s, so this is a lower bound, not the actual pre-fix duration — the real number (and the real speedup) is larger.

² Bounded by the rad3/Windows/GPU/WSL2 group only (run 34139015141 → run 34156110331), since Ubuntu's before/after come from separate runs and can't be composited into one wall-clock window. Windows was the critical path in both, so this total isn't affected by the Ubuntu caveat.

Every same-machine pair moved in the same direction — biggest wins on rad3 R9700/Windows (~45-50%, the jobs paying repeatedly for the llamacpp:rocm reinstall), a same-or-larger win on Ubuntu (previously timing out at 35min, now finishing comfortably at 29m15s), smaller wins on WSL2, negligible on GPU.

[rominf]

Reviewed at 34a4d78. The root cause is right and the CI numbers back it up — a runtime-version.txt next to the tree it describes is the correct structural answer, and I agree with @tomastola that runtime_dir_in(root) = root/runtime makes the dropped manifest.runtime_dir filter redundant by construction. I checked the crash-window, concurrency, and orphan-marker angles independently and they all hold up (details at the bottom), so my comments are narrow.

Two of my three points overlap with @tomastola's second review; I'm only restating them because I got a stronger result on one and a different conclusion on the other.

1. The regression test — confirmed vacuous, by a stricter experiment

Same conclusion as @tomastola's note 2, reached a different way. Rather than deleting the marker write, I reverted the entire production file to origin/main — the pre-fix read_manifest(paths) version, bug fully restored — grafted only the new test and the RUNTIME_VERSION_MARKER constant onto it, and ran:

test tests::runtime_version_marker_survives_without_a_manifest ... ok
test result: ok. 1 passed; 0 failed; 94 filtered out

So the test passes against the unfixed code. It never calls prepare_embeddable; it exercises fs::write + fs::read_to_string + needs_extraction, and needs_extraction is byte-identical to main (I diffed it) and already fully covered by a_version_change_forces_re_extraction_over_the_existing_runtime directly above it, including the (false, Some(pin), pin, true) and version-bump arms this test re-asserts.

That matters more than a normal weak-test note because the PR description lists this test as the guard for the whole change, and #4 in the test plan cites it by name. @tomastola's helper-extraction suggestion is the right shape and would survive both mutations.

There's also a rerun hazard I reproduced: assert!(fs::read_to_string(&marker).is_err()) at :5046 is the first assertion, and cleanup at :5064 is after the asserts. I forced a panic after the fs::write, then re-ran the unmodified test — it failed on that first assertion against the leftover directory. The crate already uses tempfile::tempdir() elsewhere; that fixes the leak and the pid-only naming in one move.

2. Where I disagree: put the marker inside runtime_dir, but the stated reason doesn't hold

I agree with @tomastola's recommendation in note 3 and not with the failure mode given for it. I tried to construct the truncated-tree scenario and could not:

• The marker write at :1117 is unconditionally last — after copy_tree returns Ok. A kill anywhere during copy_tree leaves the marker absent or still holding the old version, never the new one.
• Same-version --reinstall doesn't open the window either: reinstall || !server_present || installed != Some(wanted) short-circuits on reinstall at :1059, so the stale marker is never consulted.
• A retry after a partial copy hits installed_version = old-or-absent → re-extract.

So "marker present ⟹ copy_tree completed" already holds today. The real cost of the sibling location is the opposite and milder one: after a mid-extract kill the tree is gone but the marker survives, and correctness then rests entirely on the lemond_path_in(&runtime_dir).is_file() term — a second, independent invariant a future edit could drop without any test noticing. Moving the marker inside runtime_dir is still worth doing, because it collapses two invariants into one that remove_dir_all maintains for free. I'd just not describe the current code as having a live truncated-tree bug.

3. New: the cache dir is now shared, and it holds mutable state

The comment at :240 says every byte under ROCM_CLI_CACHE_DIR "is content-addressed and re-fetchable." Two files aren't:

• cache/therock/startup-update-check.json — written by save_startup_update_check (apps/rocm/src/therock.rs:775-787) with a plain non-atomic fs::write, and cached for 12h (STARTUP_UPDATE_CHECK_INTERVAL_MS, :36). Every rocm invocation refreshes it (main.rs:1262, :1278, :1659).
• cache/therock/metadata/*.json — the etag/last-modified revalidation cache. On a 304 the CLI serves the cached body as the channel index (therock.rs:2095-2101). Same path, different bytes over time. This one is written atomically, so it's the staleness that's the issue, not tearing.

Concretely, with a shared dir: the first scenario to run writes the update record, and every later scenario in that job — and every job on that runner for the next 12 hours — takes the cached branch and skips the check. Nothing asserts on it today (I checked; refresh_startup_update_check_quietly at main.rs:2103 discards the result, and there's no startup banner), so this is not a live failure. But the non-atomic write is a real torn-file risk, and load_startup_update_check hard-errors on a malformed file, which propagates through render_update_report to a non-zero rocm update — that would trip assert_eq!(rc, 0) in update_steps.rs.

The cheapest fix is one line in isolate_env(): set ROCM_CLI_DISABLE_STARTUP_UPDATE_CHECK=1. It's set nowhere in the repo today (only its own definition at therock.rs:696). Alternatively save_startup_update_check could use the write_file_atomically already defined in that same module at :2368. Either way I'd soften the comment — "the mutable entries are atomically written or benign to share" is weaker but true.

4. New: what actually makes the shared cache safe is one lane's capability probe

max_concurrent = if cap.has_amd_gpu { 1 } else { 64 } (e2e.rs:1149). @tomastola flagged in the first review that this coupling is incidental; it's worth knowing it is already false on a live lane, not hypothetical.

From this PR's own WSL2 job (101852715026):

##[warning]rocm-smi not found in this WSL distro — GPU scenarios will resolve to skip
... "has_amd_gpu": false ...
"cache_dir": "/home/administrator/actions-runner/_work/rocm-cli/e2e-shared/rocm-cli"

The WSL preflight is advisory and exit 0s on that path (e2e-selfhosted.yml:741-743, :765-768), the lane exports E2E_SHARED_CACHE_DIR at :785, and host_has_usable_gpu_with_mask returns false because driver_status is wsl_rocdxg_missing (capability.rs:454-462). So that job ran up to 64 concurrent scenarios against one shared cache dir. It passed — 72 passed, 3 xfail, 0 unexpected — because it skips the heavy scenarios, so this is a latent hazard, not a failure. (The 3 failures are the expected xfails, not a regression from this PR.)

Two writers make it more than theoretical if that lane ever gets a working driver: crates/rocm-core/src/uv.rs:472 and apps/rocm/src/therock.rs:2569-2578 both remove_file the shared archive after their own extract finishes, which can pull it out from under a concurrent process still reading it. A debug_assert/clause tying sharing to the cap, per @tomastola's original note, would catch this at the point either side changes.

On the measurements

The speedup table is good evidence and better than the checkbox. One detail strengthens it further than stated: Strix Halo Windows exports only E2E_SHARED_RUNTIMES_DIR, not E2E_SHARED_CACHE_DIR (e2e-selfhosted.yml, Windows run step) — so its −44% is attributable to the marker fix alone, with the cache-dir change controlled out. That's the cleanest single data point in the set.

I'd still uncheck the CI box in the description, per your own caveat and @tomastola's note 5 — a grep -c 'Installing Lemonade llamacpp:rocm backend' on a post-fix log would let you check it honestly.

Checked and did not hold up

Worth recording so nobody re-derives them: the dropped runtime_dir filter (redundant — 1:1 by construction); orphan marker after tree deletion (server_present covers it, and no deletion path orphans it — there is no rocm engines uninstall); concurrency on the shared tree (archive_guard is acquired at :1076 and dropped at :1124, so the cross-process std file lock covers the whole extract+copy+marker region); and the shared-cache wipe hazard via rocm uninstall — both uninstall steps use their own smoke_cache dirs (lifecycle_steps.rs:906-943) and are @lifecycle-tagged onto GitHub-hosted jobs that never export the shared var.

Agreed with @tomastola on splitting or at least naming 090231e (the fix.rs ROCM_PATH serialization) in the description — one note on it: that lane matters, since Linux runs cargo nextest (process-per-test, race impossible) while Windows runs cargo test --workspace --all-targets (ci.yml:357, thread-per-test), so the required Windows lane is the only place the original race was reachable. Both .lock().unwrap() sites will cascade a poison panic over the real assertion if either test fails there; unwrap_or_else(PoisonError::into_inner) — the pattern at main.rs:19174 — avoids that.

feedback addressed

[tomastola]

Re-reviewed at 91851e9. All three findings from my last pass are addressed, two of them structurally rather than by patching the symptom — the marker move and the helper extraction are both better than what I suggested. Thanks also for opening #359; that's exactly the "stays discoverable after merge" I was after.

One finding survives, and I think it survives for a reason worth spelling out. The new startup-update-check mitigation also has a better version available a few lines away.

---

1. The regression test still doesn't cover the regression

Calling the production helpers instead of hand-copying their bodies is a real improvement — it turns the test from a tautology into something that constrains real code. But I don't think it constrains the code that broke.

You wrote:

Verified via the same mutation experiment (deleted the marker write from prepare_embeddable) — the rewritten test now fails as expected.

I ran that exact mutation and it doesn't fail. Commenting out line 1139, the marker write in prepare_embeddable:

// record_runtime_version(&runtime_dir, &version)?;

test tests::runtime_version_marker_survives_without_a_manifest ... ok
test result: ok. 95 passed; 0 failed

I suspect what actually got mutated was the fs::write inside record_runtime_version's body — that one does fail. Three mutations, all run at 91851e9:

Mutation	Test
Delete the fs::write inside record_runtime_version (helper body)	FAILS ✅
Delete the record_runtime_version(…) call in prepare_embeddable (:1139)	passes ❌
Revert installed_runtime_version(…) at :1120 to the pre-fix manifest read — the original bug	passes ❌

The third is the one that matters. Restoring the pre-fix read:

// let installed_version = installed_runtime_version(&runtime_dir);
let installed_version = read_manifest(paths)
    .ok()
    .filter(|m| m.runtime_dir == runtime_dir)
    .map(|m| m.version);

fully reintroduces the defect — every isolated caller misses, wipes the shared tree, re-extracts, and the 3.3GB backend dies on every scenario again — and all 95 tests stay green.

The pattern behind all three rows: the test pins the helpers' behaviour, but nothing asserts that prepare_embeddable uses them. The bug was never inside the helpers — it was in which source that function trusted. So the mutation that passes the bar and the mutation that reproduces the bug are different mutations.

The doc comment's claim is narrower than it reads, for the same reason:

reverting either production function to source the version from the manifest instead fails this test

installed_runtime_version(&Path) has no AppPaths in scope, so it can't read a manifest without a signature change. The mutation it defends against can't plausibly occur; the one that did occur is uncovered.

A real test is reachable, using an idiom this repo already has. prepare_embeddable hardcodes download_file into ensure_cached_archive (:1101), which is the only thing standing between a test and that function. Add a #[cfg(test)] seam mirroring write_cached_http_entry_with (apps/rocm/src/therock.rs:2046), then:

1. plant a runtime_dir containing fake lemonade/lemond, a correct marker, and a sentinel file,
2. call prepare_embeddable with a stubbed archive step and a fresh AppPaths (no manifest),
3. assert the sentinel still exists.

That fails under rows 2 and 3 above, and it's the assertion the PR title is really making.

If that's more than you want here, that's a reasonable call — put it in #359. But then I'd ask you to drop the "mutation-verified" wording from the test plan, because it's a claim the next person to touch this will rely on, and right now deleting the wiring passes.

2. save_startup_update_check should use the atomic writer already in that file

First: the mitigation is justified. I checked @rominf's finding rather than take it on trust — the WSL2 lane really does export E2E_SHARED_CACHE_DIR (.github/workflows/e2e-selfhosted.yml:785) and runs cargo xtask e2e with no GPU preflight, so a probe reading false genuinely puts 64 concurrent scenarios on one shared cache dir. And ROCM_CLI_DISABLE_STARTUP_UPDATE_CHECK is a real switch (apps/rocm/src/therock.rs:696), not a no-op.

But the tear isn't only a test problem. save_startup_update_check (apps/rocm/src/therock.rs:775) uses a plain fs::write, while write_cached_http_entry (:2038) writes the neighbouring file in the same cache directory through write_file_atomically (:2368) — a helper in the same module, already covered by its own tests. Two concurrent rocm invocations on one machine (two terminals, a script, a shell hook) can tear that file today, and the read path propagates:

// therock.rs:521, inside render_update_report
if let Some(record) = load_startup_update_check(paths)? {

so a torn file makes rocm update exit non-zero for a user, not just in CI.

Swapping that one fs::write for write_file_atomically fixes it for users and makes the harness env var unnecessary rather than load-bearing. It's about the same size as the comment currently explaining the workaround.

Also worth weighing: with the check disabled suite-wide, the startup_check: branch at :521-527 is never taken in e2e — and update.feature is precisely the scenario that would otherwise cover it. Small, but it trades real coverage for the workaround.

3. Concurrency coupling: documented, but already violated

The new comment is accurate and I'd keep it. My hesitation is that it documents an invariant a lane in this same repo breaks right now, and the safety argument for cache sharing rests on that invariant. I take the point that a hard panic could break the WSL2 lane's currently-green build — but the enforcement doesn't have to panic:

let max_concurrent = if cap.has_amd_gpu || shared_cache_dir().is_some() { 1 } else { 64 };

That derives the cap from the thing that actually creates the hazard (a shared dir) rather than from a probe that can disagree with it, and it makes the WSL2 lane safe instead of accidentally-safe — no failure mode, just a slower lane. Given the lane is currently green only because it happens to skip the racy scenarios, I'd lean toward doing it here. Your call; not a blocker, and #359 is a fine home for it.

---

Confirmed good

Marker move (:1062-1082). Now sound by construction, and I checked the surroundings rather than just the diff: it's written after copy_tree and removed by the same remove_dir_all(&runtime_dir) that clears the tree, so the "marker implies extraction completed" invariant can't drift. Nothing enumerates the top level of runtime_dir — every consumer joins a subpath — and install-lemond.log (:1165) already lives there, so the location is idiomatic rather than novel. This closes the window I raised.

Only leftover: a runner holding a pre-fix tree keeps a stale root/runtime-version.txt beside it forever. It self-heals with one extra extract and costs a few bytes. Not worth code.

fix.rs poison recovery. Correct, and the reason is worth recording: both tests capture previous and restore ROCM_PATH before their assert!, so a failing assert poisons the lock with the environment already clean — into_inner cannot hand the next test dirty global state. That ordering is what makes the recovery safe, so it should survive any future reshuffle of those tests. Agreed on keeping it in this PR at 2 lines.

Doc comments. Both now agree, and shared_cache_dir correctly points at shared_runtimes_dir as the thing that actually covers llamacpp:rocm. The contradiction is gone.

CI

E2E tests (GPU) is red and it isn't yours — preflight failed after 90s with rocm-smi never returned within its timeout (driver wedged or GPU absent), and both the toolchain and suite steps were skipped, so nothing from this PR ran. There's an in-flight ci/pin-gpu-lanes-to-mi300x branch addressing that lane. It is a required check, so it'll still need a green re-run once the hardware is back.

E2E tests (rad3 R9700) passed in 13m46s, consistent with the improvement in your measurements.

Verified locally at 91851e9

• cargo fmt --all -- --check — clean
• cargo clippy -p rocm-engine-lemonade -p rocm-core --all-targets — clean
• cargo test -p rocm-engine-lemonade -p rocm-core — 95 + 329 pass

Recommendation: settle item 1 (either the seam-based test or dropping the mutation-verified claim), and I'd take item 2 here since it's a user-facing robustness gap with the helper already in scope. Everything else is resolved or fine to defer to #359.

[juhovainio]

Thanks for re-running the mutation — you're right, and I appreciate you checking rather than trusting the earlier claim.

1. Regression test

Fixed in b34d2f4: split prepare_embeddable into a thin wrapper and prepare_embeddable_with (version/sha256/download step all injectable, same shape as the existing generic ensure_cached_archive). The new prepare_embeddable_trusts_the_runtime_tree_over_an_absent_manifest test plants a runtime tree with a matching marker + a sentinel file, calls the real function against a fresh manifest-less AppPaths with a stubbed download step, and asserts the sentinel survives.

Re-ran all three of your mutations against it:

• Revert the version read to the manifest lookup (the actual bug): fails ✅ (extraction wrongly triggers, tries to parse the stub bytes as an archive, errors before the sentinel check even runs)
• Drop the record_runtime_version call site: doesn't apply to this test — it never reaches the extraction branch, since the marker already matches. That mutation stays uncovered; didn't add coverage for it since it's not the bug that shipped and wasn't part of what you asked for.
• Delete the fs::write inside record_runtime_version's body: still fails (unchanged from before).

2. save_startup_update_check atomic write

Fixed in f5fe94d: swapped the fs::write for write_file_atomically (already in that module). Dropped ROCM_CLI_DISABLE_STARTUP_UPDATE_CHECK from isolate_env() now that the underlying tear is fixed, so update.feature regains its e2e coverage of the startup_check branch.

3. Concurrency coupling

Applied your one-liner in e2bae9c: max_concurrent now also serializes whenever shared_cache_dir().is_some(), independent of the GPU probe. Updated #359 to mark that item done.

All green locally: cargo test -p rocm-engine-lemonade --lib (96 passed), cargo test -p rocm --bin rocm therock:: (71 passed), clippy clean on all three crates, cargo fmt --all -- --check clean.

[tomastola]

Re-reviewed at b34d2f40. All three findings from the previous round are addressed, and I verified each independently rather than taking the summary at face value. Two minor residuals below, neither blocking.

1. Regression test — fixed, and it really does catch the bug now ✅

prepare_embeddable_with is the right seam: it injects version/sha256/download and leaves the decision under test untouched, matching the shape of the existing ensure_cached_archive. I re-ran the mutations against prepare_embeddable_trusts_the_runtime_tree_over_an_absent_manifest:

Mutation	Result
Revert installed_version at lib.rs:1140 to a manifest read (the shipped bug)	FAILS ✅
Delete the fs::write in record_runtime_version's body	FAILS ✅
Drop the record_runtime_version call site at lib.rs:1159	passes (see 4)

That first row is the one that mattered, and it now fails. Worth noting the failure arrives in two layers: extraction wrongly triggers and dies on invalid gzip header before the sentinel assert is reached, but if the stub is ever replaced with a valid archive the remove_dir_all would destroy the sentinel and the assert catches it there instead. Both paths are covered, so the test doesn't depend on the stub staying malformed.

Also good: the old test was renamed to runtime_version_marker_round_trips_through_needs_extraction and its doc comment now says outright that it is not the regression test. That's the honest fix.

2. Atomic write — fixed ✅

save_startup_update_check now uses write_file_atomically. I checked the dropped fs::create_dir_all(parent): stage_file_for_atomic_publish_with (therock.rs:2415-2424) does its own create_dir_all, so removing the explicit call is safe and not a first-run regression.

Dropping ROCM_CLI_DISABLE_STARTUP_UPDATE_CHECK from isolate_env() is also correct, and orphans nothing — the var predates this PR (it's on main, from #307), so it stays a legitimate user-facing escape hatch. The coverage claim holds too: with it set, maybe_refresh_startup_update_check_at early-returns Ok(None) (therock.rs:669), so the whole startup_check branch was dead in e2e; update.feature's rocm update now exercises it again.

3. Concurrency coupling — applied, but only half the hazard ⚠️

The one-liner landed. The predicate keys on shared_cache_dir() alone, though, and the other shared mutable tree isn't covered:

let max_concurrent = if cap.has_amd_gpu || shared_cache_dir().is_some() { 1 } else { 64 };

Two lanes export E2E_SHARED_RUNTIMES_DIR without E2E_SHARED_CACHE_DIR — e2e-selfhosted.yml:603 and nightly.yml:719, both the Windows PowerShell blocks. On those, shared_cache_dir() is None, so the predicate collapses back to the bare cap.has_amd_gpu probe — exactly the probe-vs-lane disagreement this change was meant to stop trusting. The tree left unserialized there is the shared runtime tree, i.e. the one this PR is titled after: on a version bump the marker won't match, and two concurrent scenarios can both enter the extraction branch where one fs::remove_dir_all(&runtime_dir) runs under the other's read.

Suggested:

let max_concurrent = if cap.has_amd_gpu
    || shared_cache_dir().is_some()
    || shared_runtimes_dir().is_some()
{
    1
} else {
    64
};

I'd leave shared_uv_cache_dir() out — uv's cache is content-addressed and uv does its own locking.

4. The uncovered mutation — your call, not blocking

You flagged this yourself, which I appreciate. I'll just register that the symptom of dropping the record_runtime_version call site is identical to the bug that shipped (marker absent → needs_extraction true → wipe and re-extract on every call), so it's the same defect from the write side rather than a different one.

It's cheap to close now that the seam exists: the test module already builds real embeddable/bin/lemond archives (see extract_archive_writes_a_well_formed_tree, lib.rs:5579), so a sibling test starting with no marker, injecting one of those, and asserting the marker exists afterward would cover it. Fine to defer or skip — noting it so the gap is a decision rather than an oversight.

CI

E2E tests (rad3 R9700) is now green at 13m59s — the preflight failure I diagnosed as infrastructure last round has cleared, which confirms it wasn't this PR. Four lanes (MI300X, Strix Halo Ubuntu/Windows/WSL2) still pending at the time of writing.

Local verification at b34d2f40

• cargo fmt --all -- --check — clean
• cargo clippy -p rocm-engine-lemonade -p rocm -p e2e-cucumber --all-targets — clean
• cargo test -p rocm-engine-lemonade --lib — 96 passed
• cargo test -p rocm --bin rocm therock:: — 71 passed, 1 ignored

Findings 1 and 2 are resolved. Finding 3's remaining half is a one-line change; I'd take it in this PR since it's the same class of gap and the runtime tree is what the PR exists to protect.

[juhovainio]

3. Concurrency coupling — closed for real this time

Good catch — the predicate was checking the wrong tree on those two Windows lanes. Fixed in 07dc8d4: added shared_runtimes_dir() to the condition, so both shared mutable trees now derive the cap, not just the cache dir. Left shared_uv_cache_dir() out per your note (content-addressed, uv's own locking).

4. Write-side mutation — closed

Fixed in 631bed1: added prepare_embeddable_records_the_version_after_a_real_extraction, using a real well-formed archive built the same way extract_archive_writes_a_well_formed_tree already does in this file. Confirmed it fails when the record_runtime_version call site is dropped.

All green: cargo test -p rocm-engine-lemonade --lib (97 passed), cargo clippy -p rocm-engine-lemonade -p e2e-cucumber --all-targets -- -D warnings, cargo fmt --all -- --check.

[tomastola]

Approving at 5c75282b. Both residuals from the last round are closed, and I verified each by mutation rather than by reading the diff.

Verified

Mutation	Result
Revert installed_version to the manifest read (the shipped bug)	FAILS ✅
Drop the record_runtime_version call site	FAILS ✅ (was passing last round)
Delete the fs::write in record_runtime_version's body	FAILS ✅

All three sides of the marker are now pinned by tests that drive the real prepare_embeddable, and the read-side mutation still fails after the new commits — so the earlier coverage didn't regress while the write-side gap was being closed.

max_concurrent now derives from both shared mutable trees (shared_cache_dir() and shared_runtimes_dir()), with shared_uv_cache_dir() deliberately excluded and the reasoning written into the comment. That closes the gap on the two Windows lanes that export only the runtimes dir.

Nice detail in 5c75282b: building a zip on Windows and a tar.gz elsewhere, keyed off the same embeddable_os_arch() the production path uses, keeps the write-side test real on every platform instead of quietly degrading to a no-op on one.

Local at 5c75282b: cargo fmt --all -- --check clean, cargo clippy -p rocm-engine-lemonade -p e2e-cucumber --all-targets -- -D warnings clean, 97 lemonade tests pass.

One caveat on the green checks

At the time of this approval the self-hosted GPU lanes have not run against this head — the E2E self-hosted workflow is queued for 5c75282b, so its jobs aren't registered as checks yet and the all-green list reflects only the hosted lanes. The E2E tests lane that passed is the mock one; it exports none of the shared-dir env vars, so it exercises none of the behaviour this PR changes. The green rad3 R9700 result was against b34d2f40, four commits back.

My approval covers the code, which I've verified directly. It isn't evidence that the lanes pass. Worth letting the self-hosted group report before merging, particularly Strix Halo Windows and WSL2 — those are the lanes the runtimes-dir predicate change actually targets.