mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-06-11 08:42:11 +00:00
fix(installer): never brick the install when a self-update swap fails
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.
This commit is contained in:
parent
c349eca823
commit
d3b1e43005
1 changed files with 132 additions and 21 deletions
|
|
@ -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);
|
||||
}
|
||||
}
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue