fix(versions): no separate-prompt fallback for an all-users downgrade; one queue turn per downgrade

Two review findings, both ways for the 'app left uninstalled' trap to come back:
- If the one-prompt elevated session could not start, an all-users downgrade fell back to a
  prompt per step. It now stops before anything is removed and says nothing was changed.
- The step-by-step route (per-user copies) took a separate installer-queue turn per step, so
  another queued install, or a cancel while waiting, could stop the put-back after the copy
  was removed. Remove, install and put-back now run in one turn, with nothing to wait on.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
Jeremy
2026-10-29 18:25:48 +00:00
parent a7491e9d15
commit e77c72aff5
3 changed files with 52 additions and 33 deletions
+33 -29
View File
@@ -2034,7 +2034,8 @@ pub mod windows {
/// Goes back to `chosen` from a newer installed copy, which the app packages won't install
/// over: removes it, installs `chosen` in the same folder and context, and puts `previous`
/// back if that does not finish (see [`msi_downgrade`]). An all-users copy does all three
/// under one permission prompt; a per-user copy needs none, and runs them one by one.
/// under one permission prompt, or not at all; a per-user copy needs none, and runs them one
/// by one in a single turn of the installer queue.
pub(super) fn downgrade_msi(
app: &CraftApp,
chosen: Package<'_>,
@@ -2050,19 +2051,23 @@ pub mod windows {
elevated = downgrade_elevated(app, &chosen, &previous, destination, scope)?;
Ok(())
})?;
if let Some(ended) = elevated {
return msi_downgrade::report(ended, app.name, chosen.version, previous.version);
}
log::warn!("{}: couldn't start the elevated session; going back one step at a time", app.name);
// Separate prompts are no fallback: refusing a later one would leave no app at all.
return match elevated {
Some(ended) => msi_downgrade::report(ended, app.name, chosen.version, previous.version),
None => Err(msi_downgrade::no_session(app.name, chosen.version)),
};
}
let phase = std::cell::RefCell::new(phase);
msi_downgrade::run(msi_downgrade::Steps {
app: app.name,
chosen: chosen.version,
previous: previous.version,
remove: &mut || remove_msi_registrations(app, stop, &mut **phase.borrow_mut()),
install_chosen: &mut || run_msi(chosen.file, destination, scope, stop, &mut **phase.borrow_mut()),
reinstall_previous: &mut || run_msi(previous.file, destination, scope, stop, &mut **phase.borrow_mut()),
// One turn for all three steps: once the copy is removed, nothing waiting in the queue
// (another install, or a cancel while waiting) can stop the put-back.
in_turn(stop, phase, JobPhase::Installing, || {
msi_downgrade::run(msi_downgrade::Steps {
app: app.name,
chosen: chosen.version,
previous: previous.version,
remove: &mut || remove_registrations(app),
install_chosen: &mut || run_direct(chosen.file, destination, scope),
reinstall_previous: &mut || run_direct(previous.file, destination, scope),
})
})
}
@@ -2106,29 +2111,28 @@ pub mod windows {
}
pub fn uninstall_msi(app: &CraftApp, stop: &Stop, phase: &mut dyn FnMut(JobPhase)) -> Result<(), String> {
remove_msi_registrations(app, stop, phase).map_err(|removal| removal.error)
in_turn(stop, phase, JobPhase::Removing, || remove_registrations(app).map_err(|removal| removal.error))
}
/// Removes every Windows Installer registration of `app` in turn. A failure says whether an
/// earlier registration was already removed, so a downgrade knows the app is partly gone.
fn remove_msi_registrations(app: &CraftApp, stop: &Stop, phase: &mut dyn FnMut(JobPhase)) -> Result<(), msi_downgrade::Removal> {
/// Removes every Windows Installer registration of `app` in turn, within the caller's queue
/// turn. A failure says whether an earlier registration was already removed, so a downgrade
/// knows the app is partly gone.
fn remove_registrations(app: &CraftApp) -> Result<(), msi_downgrade::Removal> {
let installs = crate::detect::windows::msi_installs(app);
if installs.is_empty() {
return Err(msi_downgrade::Removal { error: format!("{} isn't registered with the Windows Installer", app.name), partly: false });
}
let mut removed = 0;
let result = in_turn(stop, phase, JobPhase::Removing, || {
for m in &installs {
let status = process::command(msiexec())
.args(["/x", &m.product_code, "/passive", "/norestart"])
.status()
.map_err(|e| format!("couldn't start the Windows Installer ({e})"))?;
explain(status.code())?;
removed += 1;
for (removed, m) in installs.iter().enumerate() {
let status = process::command(msiexec())
.args(["/x", &m.product_code, "/passive", "/norestart"])
.status()
.map_err(|e| format!("couldn't start the Windows Installer ({e})"))
.and_then(|status| explain(status.code()));
if let Err(error) = status {
return Err(msi_downgrade::Removal { error, partly: removed > 0 });
}
Ok(())
});
result.map_err(|error| msi_downgrade::Removal { error, partly: removed > 0 })
}
Ok(())
}
}
+12 -4
View File
@@ -17,8 +17,11 @@
//! The session is given only `msiexec` command lines for the two held, checked packages and the
//! installed copy's product codes, as [`msi_batch`](super::msi_batch) does for Update all.
//!
//! A per-user copy needs no permission, so its steps run one after another ([`run`]); that is
//! also the fallback when the elevated session cannot start.
//! If that session cannot start, an all-users downgrade stops before anything is removed: falling
//! back to one prompt per step would bring the trap back.
//!
//! A per-user copy needs no permission, so its steps run one after another ([`run`]), all in one
//! turn of the installer queue: once the copy is removed, nothing queued can stop the put-back.
#![cfg_attr(not(windows), allow(dead_code))]
@@ -86,6 +89,11 @@ fn partly_removed_not_put_back(app: &str, previous: &str, why: &str, again: &str
format!("{app} {previous} couldn't be fully removed ({why}), and putting it back failed too ({again}). Install {app} again from the launcher.")
}
/// An all-users downgrade whose one-prompt session could not start: nothing was removed.
pub(super) fn no_session(app: &str, chosen: &str) -> String {
format!("couldn't ask Windows for permission to go back to {app} {chosen}, so nothing was changed.")
}
/// A refused permission prompt reads as what happened, not as the cancel code.
fn reason(error: &str) -> String {
if error == CANCELLED { "permission wasn't given".to_string() } else { error.to_string() }
@@ -143,7 +151,7 @@ pub(super) enum Ended {
Done,
/// The one prompt was refused: nothing ran.
Declined,
/// The session never started; the per-user route can still be tried.
/// The session never started; nothing ran.
LaunchFailed,
RemovalFailed(i32),
/// The chosen version failed with this code, and the previous one is back.
@@ -180,7 +188,7 @@ pub(super) fn ended(code: Option<i32>) -> Ended {
}
/// What a finished elevated downgrade means for the person. `LaunchFailed` is not passed here:
/// the caller falls back to [`run`] instead.
/// the caller reports [`no_session`] instead.
pub(super) fn report(ended: Ended, app: &str, chosen: &str, previous: &str) -> Result<(), String> {
// Inside the session permission was already given: a cancel there is the progress window's.
let msiexec = |code: i32| match super::explain(Some(code)).err().unwrap_or_default() {
@@ -124,6 +124,13 @@ fn each_outcome_tells_the_person_what_is_installed_now() {
assert!(odd.contains("Check which version of FilmCraft is installed"), "{odd}");
}
/// An all-users downgrade with no elevated session stops before removing anything, and says so.
#[test]
fn without_the_one_prompt_session_nothing_is_changed() {
let message = no_session("FilmCraft", "0.13.0");
assert!(message.contains("go back to FilmCraft 0.13.0") && message.contains("nothing was changed"), "{message}");
}
#[test]
fn only_a_real_product_code_reaches_the_elevated_session() {
assert_eq!(removal("{3D425FF2-3EB2-4950-9833-95B01B102845}"), Ok("/x {3D425FF2-3EB2-4950-9833-95B01B102845} /passive /norestart".into()));