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);