test(e2e): activate a runtime explicitly in the shared pre-warm tree

[rominf]

• If this PR fixes a bug, searched tests/e2e-cucumber/expectations.toml for the fixed ticket ID and removed/narrowed any now-stale xfail rows. (No rows covered this failure mode — the scenarios were failing unexpectedly, not xfailing.)

Summary

Every GPU E2E serve scenario has been failing with no active ROCm runtime is configured, while the precondition step that is supposed to guarantee a runtime
still passed.

Root cause. Scenarios that only need a runtime present share one
pre-warmed managed-runtime tree, and used to rely on the CLI auto-selecting the
single runtime in it. That auto-selection is deliberately all-or-nothing: with
more than one ready runtime installed the CLI refuses to guess. Once the
pre-warm learned to install a newer runtime side by side with the old one
(#275), the tree grew past one and auto-selection started returning nothing, so
serve bailed before it ever launched an engine. The precondition only checks
that a runtime is installed, not that one is active, so it stayed green and
the failure surfaced one step later with no explanation.

Fix. Name the runtime instead of relying on the count. The pre-warm's own
activation is recorded in <runtimes>/active.json, which lives inside the
shared tree and so is visible through the symlink even though every scenario
keeps its own config dir. A new resolver reads it back, validates it against the
registry, falls back to the sole manifest, and refuses to guess otherwise —
activating an arbitrary one of several would serve against an unintended ROCm
version and pass, which is worse than failing loudly.

The second commit fixes three more places the multi-runtime tree broke the same
way: the consolidated report could attribute a run to a ROCm version it did not
serve on, the update-freshness scenario asserted on the newest runtime rather
than the active one, and a failed or skipped activation was silent even though
neither precondition assertion can detect it.

Naming a runtime is not the same as naming a usable one (third commit, found
on the GPU lane by the first two). A registry entry can outlive the folder it
points at: a scenario that installs through its own data/runtimes symlink
records its per-scenario temp dir as the install root, and that folder dies with
the scenario. The pre-warm evicts such entries (#316), but its repair is
deliberately non-fatal and skips the tree entirely when runtimes list itself
fails — which a poisoned entry makes it do. So the suite has to expect to meet
one. Selection is now filtered to entries activate would accept, judged by the
same rule the pre-warm's repair uses: an install root inside the shared tree is
sound, one outside it is a corpse. Testing mere existence would be wrong — a
foreign path that happens to exist would have the scenario serve against a
runtime outside the shared tree.

Behaviour change worth naming: on a tree where every registry entry
records an install root outside the tree, the consolidated report now emits no
ROCm version where it previously emitted one. That is the intended trade — an
absent field reads as unknown, a wrong one reads as fact — but the report is
what loses a field, so it should not be discovered from the diff.

Risk: low. Test-harness and docs only; no product code changes.

Test plan

• 15 new unit tests covering the resolution rules (multi-runtime tree, stale
  marker, corrupt marker, empty tree, refuse-to-guess, install roots that left
  the tree, and a tree reached through a symlinked parent). These run in the
  required e2e job. Each was checked to fail against the commit before its
  fix — including confirming that the earlier tests passed with the root check
  deleted, which is why they now build manifests the way the CLI really does.
• Reproduced against the real rocm binary on a two-runtime tree: serve fails
  with the exact error above, rocm runtimes activate <key> (the call the
  harness now makes) clears it, and it still fails from a fresh per-scenario
  config dir until the activation is re-applied — which is precisely the CI
  shape.
• Confirmed on hardware. Before: 7 occurrences of no active ROCm runtime is configured on the GPU lane. After the first two commits: zero — the serves
  now reach vLLM launch and fail on a separate, already-tracked blocker
  (Failed to infer device type, fix(install): always keep the SDK's build of the torch an engine pins #314), which this PR does not claim to fix.
  That same run is what surfaced the dead-install-root case the third commit
  handles.
• The wiring is exercised only by @requires-gpu scenarios on the self-hosted
  e2e-gpu lane, which is advisory rather than required, so that lane is what
  confirms the fix on hardware. The GPU lane will not go green from this PR
  — the remaining failures there belong to fix(install): always keep the SDK's build of the torch an engine pins #314.

[tomastola]

Reviewed at ac1016e. The diagnosis is convincing and refusing to guess between several runtimes is the right default. One thing I'd fix before merge, two smaller ones.

1. activatable_runtime_keys isn't using the same rule as the pre-warm's repair

shared_runtime.rs:86 claims parity:

Judged by the same rule the pre-warm's own repair uses

but assess (xtask/src/e2e_prewarm.rs:184-189) matches against two roots:

let mut roots = vec![runtimes_dir.to_path_buf()];
if let Ok(resolved) = runtimes_dir.canonicalize()
    && resolved != *runtimes_dir
{
    roots.push(resolved);
}

with the reason spelled out directly above it — "so a caller that passes a path through a symlinked parent still matches the roots the CLI wrote." shared_runtime.rs:107 keeps only the first:

Path::new(root).starts_with(runtimes_dir)

That second arm isn't decorative here, because the two sides are resolved-vs-as-given by construction:

• install_root in the manifest is canonicalized before it is written — that is exactly what #317, this branch's merge base, changed.
• runtimes_dir is E2E_SHARED_RUNTIMES_DIR used verbatim: validated_shared_dir (tests/e2e-cucumber/tests/e2e.rs:106) checks absolute-and-no-.. and never resolves.

So if the tree is ever reached through a symlinked component of $RUNNER_WORKSPACE — or, on Windows, one differing by 8.3 shortening or the verbatim prefix, both of which #317's own commit message calls out — starts_with fails for every healthy entry. activatable_runtime_keys comes back empty, runtime_key_to_activate returns None, and activate_shared_runtime takes the eprintln branch and returns. That is the bug this PR fixes, restored through a different door, and silently: it's best-effort by design, so the only trace is one stderr line and the serve fails with no active ROCm runtime is configured again.

To be clear about what I did and didn't establish: I have not shown this fires on the current runners — $RUNNER_WORKSPACE/e2e-prewarm/data/runtimes is very likely already canonical on all five lanes. The narrower claim is that the doc comment states an equivalence that isn't there, and the arm it's missing is the one the pre-warm added on purpose.

Worth noting runtime_steps.rs:37-52 — in a file this PR already edits — carries linked_runtimes_target, whose whole docstring is about resolving a harness path "so it can be compared against a path the CLI resolved", \\?\ strip included. Same comparison, third implementation, two of the three handling it.

2. active_runtime_install_root's rationale is now unreachable (capability.rs:210-217)

The retained comment justifies deriving <runtimes_dir>/wheel/<key> over the manifest field like this:

That field records the absolute path where the runtime was first installed — on Strix a per-scenario temp dir that no longer exists by report time

After this change such an entry can't reach that code: runtime_key_to_activate filters an out-of-tree install_root out first, so the fallback on line 231 can only ever yield an in-tree path (or None). The fallback still earns its keep for a pre-wheel/ in-tree layout — but the example the comment turns on is now impossible, and it's the load-bearing half of the explanation.

A behaviour change hides in the same place that the description doesn't mention: on a tree where every entry is out-of-tree — precisely the shape that comment describes — the report now emits no version where it previously emitted one. "Absent beats wrong" is the right principle and I'd keep it; it just deserves a line in the PR body, since the consolidated report is what loses a field.

3. The freshness fallback fires in a case its comment excludes (runtime_steps.rs:316, :325-328)

let line = active
    .as_deref()
    .and_then(|key| runtime_lines().find(|line| line.split_whitespace().nth(1) == Some(key)))
    .or_else(|| runtime_lines().next());

The comment says it "falls back to the first line when nothing is active". It also falls back when something is active but has no matching runtime <key> line — and then asserts on a runtime the run didn't use, which is the misattribution this change exists to remove. By the PR's own standard that a clear failure beats a silently mismatched pass, I'd panic on known-active-but-unmatched and keep the fallback strictly for active == None.

Minor

active_runtime_key (:290) takes &mut E2eWorld but only calls run_rocm, which takes &E2eWorld (e2e.rs:605). Dropping the mut lets assert_update_reports_freshness keep its original world.cli_output.as_deref().unwrap_or("") and drops the clone plus re-borrow at :307-308.

Things I liked

• Scenario 5 has always been titled "the active runtime's freshness"; until now the assertion read whichever line came first. Good to see the test catch up with its own name.
• skips_a_runtime_whose_install_root_left_the_tree documenting the regression from the first attempt at the fix.
• Filtering on "root is inside the tree" rather than "root exists" is the right predicate, and the reason given — a foreign path that happens to exist is worse than none — is the correct one.

[rominf]

Verified on hardware (GPU lane, mi300x-0, head ac1016ee). Signature counts across the three runs:

signature	before	after 1st fix	after 2nd fix
no active ROCm runtime is configured	7	0	0
install root is missing	0	9	0
Failed to infer device type	0	5	5
unexpected failures	4	4	3

The counts alone were misleading — before and after the first fix both read "4 unexpected failures" for entirely different reasons. The decisive evidence is this line, which was <unset> in every previous run:

active_runtime_key: release-wheel-multi-arch-7-14-0-f955517fdca7f54a

The suite now selects and activates a runtime, and picks the sound one: the registry still lists a runtime whose install root left the tree, and selection correctly steps over it. No shared runtime: diagnostics were emitted, so every activate succeeded rather than falling back. serve-hf-checkpoint-inference recovered and now passes.

The GPU lane is still red and this PR does not fix that. The three remaining failures (bench-load-real-serve, chat-end-to-end-local-model, serve-vllm-inference) are all Failed to infer device type — #314's territory. I checked for any other error signature in the log; there is none.

Other lanes on this head: Strix Halo Windows, WSL2 and rad3 R9700 all pass. Strix Halo Ubuntu was cancelled at its 35-minute timeout-minutes cap, which is pre-existing rather than related to this change — the same lane hits the same cap on main (runs 33113059131 and 33081012739 both cancelled at exactly 35 minutes, while 33126563205 passed at 25). That lane looks worth a separate look, since it is running close enough to its cap to fail intermittently.

All 16 required checks are green.

[volen-silo]

Reviewed at ac1016e, independently and before reading @tomastola's review — we converged on three of the same items (the starts_with canonicalization gap, the .or_else fallback at runtime_steps.rs:325, and the needless &mut on active_runtime_key). No point restating them. Their framing of the first one is sharper than mine: anchoring it to the parity claim in the shared_runtime.rs:86 doc comment, and to the fact that assess added the second root deliberately, makes the case better than "these two paths are resolved differently" does.

Approving on the read that the residual risk is latent rather than live — but the test gap below bears directly on it, so it's worth folding in before merge.

What I can add on the questions the diff raises

Coverage is complete. All three use_shared_runtimes() call sites enumerated: runtime_steps.rs:127, :151, :282. The first two are patched; the third (user_checks_for_updates) is reached only from runtime_setup.feature:47, preceded in the same scenario by Given a managed runtime is active at line 46, so it inherits activation. No Background: sections in any feature file, so there is no fourth path. Of the 14 shared-tree scenarios, every one that performs a real serve reaches a patched step — the GPU serve steps in serving_steps.rs never opt into the shared tree themselves and always sit behind an activation Given.

No concurrency hazard. e2e.rs:1116 pins max_concurrent_scenarios to 1 whenever a GPU is present, and the shared tree only exists on GPU hosts — so there is no concurrent writer to active.json. Scenarios that activate different keys all work in their own isolated registries and never call use_shared_runtimes().

The pre-warm can't be confused by this. Worth stating explicitly since it's the obvious worry when a scenario starts writing to a shared tree: nothing in the pre-warm reads active.json. decide() parses rocm update stdout, and assess() / repair_poisoned_runtimes() parse runtimes list keyed on install_root: — both iterate every registered runtime regardless of which is active. A scenario overwriting the marker cannot affect the #275 refresh logic.

The expectations claim in the checklist holds. No row covers this failure mode, so these reconcile as unexpected_fail — as stated. The XPASS hazard is also clear: the only rows touching real GPU serve paths are the lemonade ones, already flaky = true, and the affected scenarios still fail post-fix (now on #314), so no stale_xpass is created either way.

One finding I didn't see raised

The three new tests never exercise the filter they look like they protect

manifest() (shared_runtime.rs:146-148) writes a bare "{}" — no install_root. So names_the_active_runtime_when_several_are_installed, falls_back_to_the_sole_manifest_without_a_marker and refuses_to_guess_between_several_without_a_marker all take the "no recorded root, keep it" branch at lines 102-106. All three would pass identically if line 107 were deleted.

That shape is also unproducible by the real CLI: install_root: PathBuf is non-optional in InstalledRuntimeManifest (therock.rs:255), and load_runtime_manifests silently skips any registry file that fails to deserialize.

This matters for the canonicalization item specifically — whatever fix that takes, no existing test would catch a regression in it. Switching those three to the realistic installed() helper covers the intended path; one case whose in-tree root is reached via a symlinked alias would pin the fix itself.

A second reason the capability.rs fallback is load-bearing

Separate from the point about the Strix temp-dir example being unreachable: the manifest-install_root fallback is also the only correct path for tarball-format runtimes. MANAGED_RUNTIME_FORMATS is ["wheel", "tarball"] (runtime.rs:445), a tarball root is <tree>/tarball/<key>, and the derived <tree>/wheel/<key> never matches it — the pre-warm's own fixture models exactly this shape. So the comment describing it as being for trees "that predate the wheel layout" undersells it twice over, and reads like an invitation to delete it as dead code.

Tradeoff worth recording rather than changing

activate_shared_runtime (e2e.rs:404) swallows a nonzero exit. The obvious objection is that a Given silently failing its precondition is the anti-pattern this PR diagnoses. But it holds up better than I expected: a failed activate bails before writing the config, so the scenario proceeds with no active key and validate_engine_selection_runtime falls through to single_ready_runtime_key — and because the harness's filter is weaker than "ready", the common divergence case (several registered, one genuinely ready) self-heals. Defensible as-is; asserting the precondition in setup_active_runtime would keep the benefit while removing the blind spot.

Liked

• Downstream safety of the new None was actually verified rather than assumed — PlatformVersions carries skip_serializing_if, both render sites guard on emptiness, and grid_heading_omits_absent_versions pins it. No consumer unwraps.
• Three commits, each self-contained, and the third exists because the first two were run on hardware and surfaced a case that hadn't been modelled — with the PR body saying so.
• Stating plainly that the lane will not go green from this PR. Easier to write a green-sounding summary; this is more useful.

[rominf]

Thanks both — all four findings verified against source before acting, and all four held. Pushed as 957e2541 and f7d7d936.

1. starts_with parity (@tomastola, @volen-silo). Confirmed and fixed. You were right that the doc claimed an equivalence that wasn't there, and right about the mechanism: install_root is canonicalized before it's written, validated_shared_dir deliberately doesn't resolve, so the two sides are resolved-vs-as-given by construction. I've matched assess's two roots and added a third for the \\?\ prefix, for the reason linked_runtimes_target already documents in this same file.

Your note that you hadn't shown it fires on the current runners is the right characterisation and I'm not claiming otherwise — $RUNNER_WORKSPACE/e2e-prewarm/data/runtimes is very likely canonical on all five lanes today. What made it worth fixing anyway is the failure mode: it fails closed and silent, disqualifying every healthy entry at once and surfacing as exactly the error this PR exists to remove.

2. The three tests never exercised the filter (@volen-silo). This one was the most useful finding in either review, and I checked it empirically rather than by reading: stubbing the root check to true left all three green. Only my three new tests failed. Switched them to the realistic helper — after the change, four tests fail when the filter is removed instead of three. Your point that the bare-{} shape is unproducible by the real CLI is also correct and is now recorded in the helper's doc, so nobody restores it thinking it's a valid case.

I also took the suggestion of pinning the canonicalization fix itself: resolves_a_tree_reached_through_a_symlinked_parent fails against ac1016ee and passes now. Its sibling checks the tolerance didn't over-apply and re-admit the corpses.

3. Freshness fallback (@tomastola, @volen-silo). Agreed, and it's the PR's own standard turned on itself. Now asserts on the active runtime's line or fails naming the key; the fallback is strictly active == None.

4. capability.rs rationale (@tomastola) + the tarball point (@volen-silo). Both correct, and the second is the more consequential: MANAGED_RUNTIME_FORMATS is ["wheel", "tarball"] and a tarball root is <tree>/tarball/<key>, which the derived wheel path can never match — I verified the layout rather than take it on trust. The comment described it as a pre-wheel/-layout fallback, which reads as an invitation to delete live code. Rewritten, along with dropping the now-unreachable temp-dir example. The behaviour change @tomastola flagged (no version at all where every entry is out-of-tree) is now stated in the PR body.

Minor. &mut dropped; the clone and re-borrow went with it.

On activate_shared_runtime swallowing a nonzero exit — @volen-silo, I've left this as-is and taken your analysis at face value on the self-healing path, having re-checked that the harness's filter really is weaker than "ready". Asserting the precondition in setup_active_runtime is the better end state and I agree with the reasoning, but it changes what a Given does to scenarios beyond this fix, so I'd rather not fold it in silently here. Happy to do it in this PR if you'd prefer that to a follow-up.

Also flagging something neither review could have seen: Strix Halo Ubuntu was cancelled at its 35-minute cap on the last run. That's pre-existing — the same lane hits the same cap on main — but it's running close enough to the limit to fail intermittently and probably wants its own issue.

CI is running; I'll report what the GPU lane actually shows rather than assume it's unaffected.

[rominf]

Rebased onto main (92556755 → c8021409), force-pushed. Since both of you have already reviewed, here's exactly what moved.

The rebase is a no-op for review purposes. Six commits replayed with zero conflicts and no file overlap between the two sides — git diff main...HEAD is byte-identical to before apart from the new base. Nothing to re-read.

Why rebase at all: the Read the Docs check was red, and it was never about this branch's content. The docs site landed in main via #311, after this branch forked, so the head simply had no .readthedocs.yaml for RTD to build. The correlation across open PRs is exact — every PR based after #311 is green, every PR based before it is red, including several unrelated ones. Picking up the config is the only thing that fixes it. (RTD isn't a required check, so this was never merge-blocking; the in-repo Sphinx docs build (-W) job is separate and correctly skips, as this PR touches no docs.)

One real bug, mine, fixed in c8021409. windows-build-and-test failed on the symlink test I added last push. Windows hands TempDir out in 8.3 short form (RUNNER~1\…) while the CLI records a canonicalized root, and the two never starts_with-match — so the fixture was building a manifest the real CLI cannot produce and the entry got disqualified. The defect was in my test, not the filter. Fixture now canonicalizes, matching what rocm install sdk writes. Worth noting this is a second Windows path trap in the same file, distinct from the \\?\ verbatim prefix strip_verbatim_prefix already handles — fixing one didn't cover the other. windows-build-and-test passed on the pre-rebase head with the fix in.

GPU lane result on the pre-rebase head (92556755/f7d7d936, run 33174994368) — this is the evidence the fix works, measured by signature rather than failure count:

signature	before	now
no active ROCm runtime is configured	7	0
install root is missing	9 (my first attempt)	0
Failed to infer device type	5	5

active_runtime_key: release-wheel-gfx94x-dcgpu-7-13-0 is set, selection_source: config_active_runtime_key on every serve, and zero shared runtime: diagnostics — every activate succeeded. 84 scenarios, 76 passed.

I enumerated all 8 remaining failures rather than assume: every one is downstream of the vLLM RuntimeError: Failed to infer device type, which is the separate issue PR #314 addresses. None carry this PR's signatures.

Separately — I chased the Strix Halo Ubuntu cancellation I flagged earlier and it is not a slow lane, so raising the cap would be the wrong fix. Each serve scenario re-downloads the 3.3 GB llama.cpp backend, because lemonade_root() falls back to paths.engine_dir() off the per-scenario ROCM_CLI_DATA_DIR — which dies with the scenario. The harness shares runtimes, HF weights and the uv cache, but the engine tree was never given the same treatment. Three re-downloads in one run, five in another; that is where the 35 minutes goes. Tracked separately, not in scope here.

CI is re-running on the rebased head; the self-hosted run is currently held behind the superseded one on the concurrency group. I'll confirm when it reports.

[tomastola]

Re-reviewed at c802140 after the latest changes. The canonical/as-given path comparison now matches the pre-warm rule, including symlinked parents and Windows path spellings; the fixtures exercise realistic canonical install roots; the freshness assertion fails rather than falling back to an unintended runtime; and the reporting comments/body accurately capture the absent-version tradeoff.

No remaining code findings from my review. The red Windows unit test is outside this diff and passed on the immediately preceding main run; it should be rerun. The MI300X GPU lane reaching the separately tracked device-inference blocker is consistent with the PR's stated boundary rather than a regression in this fix.

[rominf]

CI settled on the rebased head c8021409. All 16 required checks green, and Read the Docs is green now that the branch carries .readthedocs.yaml.

Two reds appeared after the rebase and I chased both rather than waving them through.

windows-build-and-test — a pre-existing flake, not this branch. It failed on fix::tests::the_path_fix_finds_the_install_the_rest_of_the_cli_found in rocm-core, a crate this PR does not touch. The cause is a genuine race, and it is worth knowing about:

crates/rocm-core/src/fix.rs has two tests that both set_var("ROCM_PATH", …) around a newest_rocm_install_dir() call with no mutual exclusion. One plants a real install and expects its path back; the other plants an empty dir and expects "". Interleaved, the first reads the second's empty directory — and the observed left: "" is precisely the sibling's expected value, which is what makes this a confirmed race rather than a plausible story.

It surfaces only on Windows because ci.yml uses two different runners: the Ubuntu lane runs cargo nextest run (process per test, cannot race), while windows-build-and-test runs cargo test --workspace --all-targets (threads in one process). nextest's isolation masks it everywhere else. That's a property of the runner, not of Windows.

Verified rather than assumed: the same test passed on main in the same hour, and a re-run of the job passed as the control. Tracked separately with a suggested fix (serialize on a Mutex, or temp_env) — out of scope here.

Incidentally this is the second RUNNER~1 failure this week and the two are unrelated: mine last push was an 8.3 short-path mismatch in a fixture, this one is an env-var race. Same string in the error, different bug.

E2E tests (GPU) — unchanged by the rebase and advisory. Byte-identical to the pre-rebase run: 84 scenarios, 76 passed, the same 8 failures, active_runtime_key set, zero no active ROCm runtime is configured and zero install root is missing. All 8 remain downstream of the vLLM Failed to infer device type error that PR #314 addresses; I re-enumerated them on this head rather than carrying the earlier conclusion forward.

Ready for re-review. I'm not merging — that's your call, and the two open items from my last comment (the starts_with canonicalization and the test-gap fix) are still deliberately unresolved for you to confirm.