From d3b1e4300519e232385fab6d2cac1fffc49a5c99 Mon Sep 17 00:00:00 2001 From: kshitijk4poor <82637225+kshitijk4poor@users.noreply.github.com> Date: Wed, 3 Jun 2026 23:35:54 +0530 Subject: [PATCH] fix(installer): never brick the install when a self-update swap fails MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The macOS self-update bundle swap (install_macos_app_update, added in #38296) could leave the user with NO app installed. If moving the existing /Applications/Hermes.app aside failed, the code deleted the running app outright and set moved_old=false; if the subsequent move of the freshly built bundle into place then also failed, the rollback was gated on moved_old (now false) and skipped — leaving the target deleted with no replacement. Extract the swap into swap_in_new_bundle() with a strict invariant: on ANY failure path the target is left pointing at a working bundle (either the original, rolled back, or untouched) and is never deleted with no replacement. Also clean up the staged .hermes-update-new copy on the failure paths instead of orphaning it. Add unit tests covering the happy path, the rollback-on-install-failure path, and the catastrophic both-moves-fail path. The catastrophic-path test was verified to FAIL against the old code ("original app must NOT be deleted on failure") and pass against the fix. --- .../src-tauri/src/update.rs | 153 +++++++++++++++--- 1 file changed, 132 insertions(+), 21 deletions(-) diff --git a/apps/bootstrap-installer/src-tauri/src/update.rs b/apps/bootstrap-installer/src-tauri/src/update.rs index ea9bb5d2b4c..c84f18965bb 100644 --- a/apps/bootstrap-installer/src-tauri/src/update.rs +++ b/apps/bootstrap-installer/src-tauri/src/update.rs @@ -629,27 +629,10 @@ async fn install_macos_app_update( )); } - let moved_old = if target_app.exists() { - match tokio::fs::rename(target_app, &old).await { - Ok(()) => true, - Err(_) => { - remove_dir_if_exists(target_app).await; - false - } - } - } else { - false - }; - if let Err(err) = tokio::fs::rename(&tmp, target_app).await { - if moved_old { - let _ = tokio::fs::rename(&old, target_app).await; - } - return Err(anyhow!( - "installing updated app at {}: {err}", - target_app.display() - )); - } - remove_dir_if_exists(&old).await; + // Atomic-as-possible swap with rollback. Extracted so the invariant + // (target is never left deleted-with-no-replacement) can be unit-tested + // without ditto / a real .app bundle. + swap_in_new_bundle(&tmp, target_app, &old).await?; let _ = Command::new("/usr/bin/xattr") .arg("-dr") @@ -662,6 +645,42 @@ async fn install_macos_app_update( Ok(target_app.to_path_buf()) } +/// Move a freshly-staged bundle (`tmp`) into place at `target`, parking any +/// existing bundle at `old` so the move can succeed (macOS `rename` won't +/// overwrite a non-empty directory). +/// +/// Invariant: on ANY failure path, `target` is left pointing at a working +/// bundle — either the original (rolled back from `old`) or untouched — and we +/// never delete the running app with no replacement in place. The staged `tmp` +/// copy is cleaned up on failure. +async fn swap_in_new_bundle(tmp: &Path, target: &Path, old: &Path) -> Result<()> { + let moved_old = if target.exists() { + if let Err(err) = tokio::fs::rename(target, old).await { + // Could not move the existing app aside. Leave it untouched and + // bail — a failed update must not brick the install. + remove_dir_if_exists(tmp).await; + return Err(anyhow!( + "could not move existing app aside at {} (leaving it in place): {err}", + target.display() + )); + } + true + } else { + false + }; + if let Err(err) = tokio::fs::rename(tmp, target).await { + // Restore the original app from the backup so the user keeps a working + // install, and clean up the staged copy. + if moved_old { + let _ = tokio::fs::rename(old, target).await; + } + remove_dir_if_exists(tmp).await; + return Err(anyhow!("installing updated app at {}: {err}", target.display())); + } + remove_dir_if_exists(old).await; + Ok(()) +} + #[cfg(not(target_os = "macos"))] async fn install_macos_app_update( _app: &AppHandle, @@ -795,4 +814,96 @@ mod tests { ); assert_eq!(target_app_from_args(["--target-app", "/tmp/not-an-app"]), None); } + + // Helpers for the swap tests: make a throwaway dir tree we can rename. + fn unique_tmp_dir(tag: &str) -> PathBuf { + let base = std::env::temp_dir().join(format!( + "hermes-swap-test-{tag}-{}-{}", + std::process::id(), + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap() + .as_nanos() + )); + std::fs::create_dir_all(&base).unwrap(); + base + } + + fn write_marker(dir: &Path, contents: &str) { + std::fs::create_dir_all(dir).unwrap(); + std::fs::write(dir.join("marker.txt"), contents).unwrap(); + } + + #[tokio::test] + async fn swap_installs_new_bundle_and_cleans_up() { + let base = unique_tmp_dir("ok"); + let target = base.join("Hermes.app"); + let tmp = base.join("Hermes.app.hermes-update-new"); + let old = base.join("Hermes.app.hermes-update-old"); + write_marker(&target, "OLD"); + write_marker(&tmp, "NEW"); + + swap_in_new_bundle(&tmp, &target, &old).await.unwrap(); + + // New bundle is now at target; staging + backup dirs are gone. + assert_eq!( + std::fs::read_to_string(target.join("marker.txt")).unwrap(), + "NEW" + ); + assert!(!tmp.exists(), "staged copy should be cleaned up"); + assert!(!old.exists(), "backup should be cleaned up on success"); + let _ = std::fs::remove_dir_all(&base); + } + + #[tokio::test] + async fn swap_failure_never_leaves_target_missing() { + // Regression guard for the catastrophic path: the move-aside of the + // existing app fails AND the staged bundle can't be installed. The + // buggy version deleted `target` when move-aside failed and then + // skipped rollback, bricking the install. The fixed version must leave + // the original app intact on disk. + // + // Trigger both failures deterministically: + // - `old` is a NON-EMPTY dir -> rename(target, old) fails + // - `tmp` does not exist -> rename(tmp, target) fails + let base = unique_tmp_dir("fail"); + let target = base.join("Hermes.app"); + let tmp = base.join("Hermes.app.hermes-update-new"); // intentionally absent + let old = base.join("Hermes.app.hermes-update-old"); + write_marker(&target, "OLD"); + write_marker(&old, "OCCUPIED"); // non-empty => rename(target,old) fails + + let result = swap_in_new_bundle(&tmp, &target, &old).await; + + assert!(result.is_err(), "swap should fail when neither move can complete"); + assert!(target.exists(), "original app must NOT be deleted on failure"); + assert_eq!( + std::fs::read_to_string(target.join("marker.txt")).unwrap(), + "OLD", + "original app contents must be intact after a failed swap" + ); + let _ = std::fs::remove_dir_all(&base); + } + + #[tokio::test] + async fn swap_rolls_back_when_install_step_fails() { + // Move-aside succeeds but installing the staged bundle fails (tmp + // absent). The original must be rolled back from `old` to `target`. + let base = unique_tmp_dir("rollback"); + let target = base.join("Hermes.app"); + let tmp = base.join("Hermes.app.hermes-update-new"); // absent + let old = base.join("Hermes.app.hermes-update-old"); + write_marker(&target, "OLD"); + + let result = swap_in_new_bundle(&tmp, &target, &old).await; + + assert!(result.is_err()); + assert!(target.exists(), "original must be restored after failed install"); + assert_eq!( + std::fs::read_to_string(target.join("marker.txt")).unwrap(), + "OLD" + ); + assert!(!old.exists(), "backup should be rolled back, not left behind"); + let _ = std::fs::remove_dir_all(&base); + } }