From 069c981202249c113554c21114f887b5a75264d3 Mon Sep 17 00:00:00 2001 From: Roman Inflianskas Date: Thu, 1 Oct 2026 13:00:41 +0000 Subject: [PATCH] fix(runtimes): keep the rollback target when re-selecting the active runtime Activating the runtime already in use recomputed the rollback target from the runtime being left. There is none in that case, so it cleared the recorded one -- silently taking away the recovery path the previous activation had just offered ("if this causes problems, run `rocm runtimes rollback`"). Nothing the user could see had changed, so nothing signalled that the undo was gone. Deriving the target from the current runtime is right only when the user is actually leaving one. When the requested runtime is already active the activation is a no-op for the registry pointers, so the recorded target is now preserved instead of overwritten. The guard that stopped a runtime being recorded as its own rollback target is kept: in the branch that records a new target, the current runtime differs from the requested one by construction. The shortest command sequence that exposes this is three activations in a row, which is why none of the seven existing lifecycle scenarios caught it -- every one- and two-command sequence conforms. Scenario runtime-lifecycle-08 pins it: it fails before this change and passes after, and needs no GPU or download, so it runs on the mock lane every PR. Signed-off-by: Roman Inflianskas --- apps/rocm/src/main.rs | 35 +++++++++++++++---- .../features/runtime_lifecycle.feature | 15 ++++++++ .../tests/e2e/runtime_lifecycle_steps.rs | 26 ++++++++++++++ 3 files changed, 69 insertions(+), 7 deletions(-) diff --git a/apps/rocm/src/main.rs b/apps/rocm/src/main.rs index ab0264418..846d00e23 100644 --- a/apps/rocm/src/main.rs +++ b/apps/rocm/src/main.rs @@ -8828,14 +8828,35 @@ pub(crate) fn activate_runtime( let manifest = select_runtime_manifest(&manifests, selector)?; validate_runtime_manifest_for_activation(manifest)?; let current = current_runtime_manifest(config, &manifests); - let previous_runtime_key = current + // Re-selecting the runtime already in use is a no-op to the user: they have + // not left anything, so there is no new rollback target — and no reason to + // forget the one the activation that put them here recorded. Deriving the + // target from `current` unconditionally used to clear it, silently removing + // the recovery path that activation had just offered. + let (previous_runtime_key, previous_runtime_id) = if current .as_ref() - .map(|manifest| manifest.runtime_key.clone()) - .filter(|runtime_key| runtime_key != &manifest.runtime_key); - let previous_runtime_id = current - .as_ref() - .map(|manifest| manifest.runtime_id.clone()) - .filter(|_| previous_runtime_key.is_some()); + .is_some_and(|current| current.runtime_key == manifest.runtime_key) + { + let runtime_key = config.previous_runtime_key.clone(); + let runtime_id = runtime_key + .as_deref() + .and_then(|runtime_key| { + manifests + .iter() + .find(|candidate| candidate.runtime_key == runtime_key) + }) + .map(|previous| previous.runtime_id.clone()); + (runtime_key, runtime_id) + } else { + // `current` differs from the requested runtime here by construction, so + // this can never record a runtime as its own rollback target. + ( + current + .as_ref() + .map(|manifest| manifest.runtime_key.clone()), + current.as_ref().map(|manifest| manifest.runtime_id.clone()), + ) + }; config.default_runtime_id = Some(manifest.runtime_id.clone()); config.active_runtime_key = Some(manifest.runtime_key.clone()); diff --git a/tests/e2e-cucumber/features/runtime_lifecycle.feature b/tests/e2e-cucumber/features/runtime_lifecycle.feature index a0d3103b5..d5cdc932c 100644 --- a/tests/e2e-cucumber/features/runtime_lifecycle.feature +++ b/tests/e2e-cucumber/features/runtime_lifecycle.feature @@ -65,3 +65,18 @@ Feature: Runtime lifecycle state machine Then the listing explains the active and rollback markers And the first runtime is marked active And the second runtime is marked as the rollback target + + # Re-selecting the runtime you are already on changes nothing the user can + # see, so it must not quietly take away the recovery path the previous + # activation had just offered ("if this causes problems, run `rocm runtimes + # rollback`"). It used to: the activation recomputed the rollback target from + # the runtime being left, and when there was none it cleared the recorded one + # instead of leaving it alone. The shortest command sequence that exposes this + # is three activations in a row, which is why none of the hand-written + # examples above caught it: every one- and two-command sequence conforms. + @id:runtime-lifecycle-reactivating-keeps-rollback-target + Scenario: runtime-lifecycle-08 - Re-selecting the runtime already in use keeps the rollback target + Given two registered runtimes with the second active after the first + When the user activates the second runtime again + Then the second runtime is still the one in use + And the first runtime is still the rollback target diff --git a/tests/e2e-cucumber/tests/e2e/runtime_lifecycle_steps.rs b/tests/e2e-cucumber/tests/e2e/runtime_lifecycle_steps.rs index 7b38d4a02..4591d4f01 100644 --- a/tests/e2e-cucumber/tests/e2e/runtime_lifecycle_steps.rs +++ b/tests/e2e-cucumber/tests/e2e/runtime_lifecycle_steps.rs @@ -134,6 +134,12 @@ async fn activate_second(world: &mut E2eWorld) { record(world, stdout, stderr, rc); } +#[when("the user activates the second runtime again")] +async fn activate_second_again(world: &mut E2eWorld) { + let (stdout, stderr, rc) = crate::run_rocm(world, &["runtimes", "activate", SECOND_KEY]); + record(world, stdout, stderr, rc); +} + #[when("the user rolls back")] async fn rollback(world: &mut E2eWorld) { let (stdout, stderr, rc) = crate::run_rocm(world, &["runtimes", "rollback"]); @@ -234,6 +240,26 @@ async fn first_active_again(world: &mut E2eWorld) { ); } +#[then("the second runtime is still the one in use")] +async fn second_still_active(world: &mut E2eWorld) { + let listing = crate::run_rocm_ok(world, &["runtimes", "list"]); + assert!( + listing.contains(&format!("* {SECOND_KEY}")), + "expected {SECOND_KEY} to still be the runtime in use, got:\n{listing}" + ); +} + +#[then("the first runtime is still the rollback target")] +async fn first_still_rollback_target(world: &mut E2eWorld) { + let listing = crate::run_rocm_ok(world, &["runtimes", "list"]); + assert!( + listing.contains(&format!("- {FIRST_KEY}")), + "re-selecting the runtime already in use is a no-op for the user, so it must not \ + discard the rollback target the previous activation promised; expected {FIRST_KEY} \ + still marked as the rollback target, got:\n{listing}" + ); +} + #[then("its registry entry is removed")] async fn registry_removed(world: &mut E2eWorld) { let out = ok_output(world);