diff --git a/src-tauri/src/app_update.rs b/src-tauri/src/app_update.rs index 611cc70..c5a6f28 100644 --- a/src-tauri/src/app_update.rs +++ b/src-tauri/src/app_update.rs @@ -971,8 +971,12 @@ pub(crate) async fn download_and_stage_portable_app_update( launch_portable_update_helper(app, &helper_path, &work_dir, &descriptor).await } .await; + // A busy DMG may still be mounted here. Never recursively clean its mount point. if result.is_err() { - let _ = fs::remove_dir_all(&work_dir); + cleanup_macos_update_dmg(&work_dir); + if !work_dir.join("mount").exists() { + let _ = fs::remove_dir_all(&work_dir); + } } result } @@ -2113,7 +2117,13 @@ fn run_macos_update_command(command: &mut Command, action: &str) -> Result<(), S return Ok(()); } let stderr = String::from_utf8_lossy(&output.stderr); - Err(format!("Failed to {action}: {}", stderr.trim())) + let stdout = String::from_utf8_lossy(&output.stdout); + let details = [stderr.trim(), stdout.trim()] + .into_iter() + .filter(|text| !text.is_empty()) + .collect::>() + .join("; "); + Err(format!("Failed to {action} ({}): {details}", output.status)) } #[cfg(target_os = "macos")] @@ -2121,6 +2131,23 @@ pub(crate) fn stage_macos_application_from_dmg( dmg_path: &Path, work_dir: &Path, staged_app: &Path, +) -> Result<(), String> { + stage_macos_application_from_dmg_with_commands( + dmg_path, + work_dir, + staged_app, + run_macos_update_command, + thread::sleep, + ) +} + +#[cfg(any(target_os = "macos", test))] +pub(crate) fn stage_macos_application_from_dmg_with_commands( + dmg_path: &Path, + work_dir: &Path, + staged_app: &Path, + mut run: impl FnMut(&mut Command, &str) -> Result<(), String>, + mut wait: impl FnMut(Duration), ) -> Result<(), String> { let mount_dir = work_dir.join("mount"); fs::create_dir_all(&mount_dir).map_err(|error| format!("Failed to create DMG mount directory: {error}"))?; @@ -2130,7 +2157,7 @@ pub(crate) fn stage_macos_application_from_dmg( .arg(dmg_path) .args(["-nobrowse", "-readonly", "-mountpoint"]) .arg(&mount_dir); - run_macos_update_command(&mut attach, "Mount application update DMG")?; + run(&mut attach, "Mount application update DMG")?; let source_app = mount_dir.join("EasyCLIProxyAPI.app"); let stage_result = (|| -> Result<(), String> { @@ -2144,27 +2171,79 @@ pub(crate) fn stage_macos_application_from_dmg( .map_err(|error| format!("Failed to create application update staging directory: {error}"))?; let mut ditto = Command::new("ditto"); ditto.arg(&source_app).arg(staged_app); - run_macos_update_command(&mut ditto, "Stage new macOS application")?; + run(&mut ditto, "Stage new macOS application")?; let mut codesign = Command::new("codesign"); codesign .args(["--verify", "--deep", "--strict"]) .arg(staged_app); - run_macos_update_command(&mut codesign, "Verify new macOS application signature")?; + run(&mut codesign, "Verify new macOS application signature")?; let mut gatekeeper = Command::new("spctl"); gatekeeper .args(["--assess", "--type", "execute", "--verbose=2"]) .arg(staged_app); - run_macos_update_command(&mut gatekeeper, "Verify new macOS application system trust status") + run(&mut gatekeeper, "Verify new macOS application system trust status") })(); - let mut detach = Command::new("hdiutil"); - detach.arg("detach").arg(&mount_dir); - let detach_result = run_macos_update_command(&mut detach, "Unmount application update DMG"); - match (stage_result, detach_result) { - (Err(error), _) => Err(error), - (Ok(()), Err(error)) => Err(error), - (Ok(()), Ok(())) => Ok(()), + match eject_macos_update_dmg_with_commands(&mount_dir, &mut run, &mut wait) { + Ok(()) => { + let _ = fs::remove_dir(&mount_dir); + } + Err(error) => { + // A successfully staged app no longer depends on the read-only DMG. + // Preserve staging errors, but never fail an update just for cleanup. + eprintln!("Application update DMG cleanup deferred: {error}"); + } } + stage_result +} + +#[cfg(any(target_os = "macos", test))] +pub(crate) fn eject_macos_update_dmg_with_commands( + mount_dir: &Path, + mut run: impl FnMut(&mut Command, &str) -> Result<(), String>, + mut wait: impl FnMut(Duration), +) -> Result<(), String> { + let mut last_error = String::new(); + // Spotlight or security scanning can briefly hold the newly mounted image busy. + for attempt in 0..3 { + if attempt > 0 { + wait(Duration::from_millis(500)); + } + let mut eject = Command::new("diskutil"); + eject.arg("eject").arg(mount_dir); + match run(&mut eject, "Eject application update DMG") { + Ok(()) => return Ok(()), + Err(error) => last_error = error, + } + } + + // Compatibility fallback, limited to this updater's private read-only mount. + let mut detach = Command::new("hdiutil"); + detach.args(["detach", "-force"]).arg(mount_dir); + run(&mut detach, "Force unmount application update DMG") + .map_err(|error| format!("{last_error}; {error}")) +} + +#[cfg(target_os = "macos")] +fn cleanup_macos_update_dmg(work_dir: &Path) { + cleanup_macos_update_dmg_with_commands(work_dir, run_macos_update_command, thread::sleep); +} + +#[cfg(any(target_os = "macos", test))] +pub(crate) fn cleanup_macos_update_dmg_with_commands( + work_dir: &Path, + run: impl FnMut(&mut Command, &str) -> Result<(), String>, + wait: impl FnMut(Duration), +) { + let mount_dir = work_dir.join("mount"); + if mount_dir.exists() { + if let Err(error) = eject_macos_update_dmg_with_commands(&mount_dir, run, wait) { + eprintln!("Application update DMG cleanup deferred: {error}"); + return; + } + let _ = fs::remove_dir(&mount_dir); + } + let _ = fs::remove_file(work_dir.join("update.dmg")); } #[cfg(target_os = "macos")] @@ -2374,9 +2453,8 @@ pub(crate) fn replace_macos_application(descriptor: &MacosUpdateDescriptor) -> R #[cfg(target_os = "macos")] fn cleanup_macos_update_payload(descriptor_path: &Path, descriptor: &MacosUpdateDescriptor) { - let _ = fs::remove_file(descriptor.work_dir.join("update.dmg")); + cleanup_macos_update_dmg(&descriptor.work_dir); let _ = fs::remove_dir_all(descriptor.work_dir.join("staging")); - let _ = fs::remove_dir_all(descriptor.work_dir.join("mount")); let _ = fs::remove_file(&descriptor.ack_path); let _ = fs::remove_file(portable_update_helper_ack_path(&descriptor.work_dir)); let _ = fs::remove_file(descriptor_path); diff --git a/src-tauri/src/tests/app_update.rs b/src-tauri/src/tests/app_update.rs index 9add79c..367f11c 100644 --- a/src-tauri/src/tests/app_update.rs +++ b/src-tauri/src/tests/app_update.rs @@ -51,6 +51,190 @@ fn macos_update_helper_stays_inside_the_signed_app_bundle() { assert_eq!(macos_update_helper_path(¤t_exe), current_exe); } +#[test] +fn macos_dmg_eject_retries_busy_mounts_and_stops_after_success() { + let mount_dir = Path::new("/private/tmp/update with spaces/mount"); + for busy_attempts in 0..3 { + let mut attempts = 0; + let mut waits = Vec::new(); + eject_macos_update_dmg_with_commands( + mount_dir, + |command, _| { + assert_eq!(command.get_program(), "diskutil"); + assert_eq!( + command.get_args().collect::>(), + [std::ffi::OsStr::new("eject"), mount_dir.as_os_str()] + ); + attempts += 1; + if attempts <= busy_attempts { + Err("Resource busy".to_string()) + } else { + Ok(()) + } + }, + |duration| waits.push(duration), + ) + .unwrap(); + assert_eq!(attempts, busy_attempts + 1); + assert_eq!(waits, vec![Duration::from_millis(500); busy_attempts]); + } +} + +#[test] +fn macos_dmg_eject_falls_back_only_for_the_update_mount() { + let mount_dir = Path::new("/private/tmp/update with spaces/mount"); + for force_succeeds in [true, false] { + let mut eject_attempts = 0; + let mut force_attempts = 0; + let result = eject_macos_update_dmg_with_commands( + mount_dir, + |command, _| { + if command.get_program() == "diskutil" { + eject_attempts += 1; + return Err("diskutil: Resource busy".to_string()); + } + assert_eq!(command.get_program(), "hdiutil"); + assert_eq!( + command.get_args().collect::>(), + [ + std::ffi::OsStr::new("detach"), + std::ffi::OsStr::new("-force"), + mount_dir.as_os_str(), + ] + ); + force_attempts += 1; + if force_succeeds { + Ok(()) + } else { + Err("hdiutil: Resource busy".to_string()) + } + }, + |_| {}, + ); + assert_eq!(eject_attempts, 3); + assert_eq!(force_attempts, 1); + if force_succeeds { + result.unwrap(); + } else { + let error = result.unwrap_err(); + assert!(error.contains("diskutil: Resource busy")); + assert!(error.contains("hdiutil: Resource busy")); + } + } +} + +#[test] +fn macos_dmg_staging_survives_cleanup_failure_without_bypassing_verification() { + // Exercise successful staging and each fatal preparation step, even when + // every eject attempt fails with the error reported in issue #301. + for failure in [None, Some("ditto"), Some("codesign"), Some("spctl")] { + let root = agent_test_home("macos-dmg-stage-busy"); + let source_app = root.join("mount").join("EasyCLIProxyAPI.app"); + let staged_app = root.join("staging/EasyCLIProxyAPI.app"); + fs::create_dir_all(&source_app).unwrap(); + fs::write(source_app.join("payload"), b"application").unwrap(); + let mut preparation = Vec::new(); + let mut cleanup_attempts = 0; + let result = stage_macos_application_from_dmg_with_commands( + &root.join("update.dmg"), + &root, + &staged_app, + |command, _| { + let program = command.get_program().to_str().unwrap(); + let args = command.get_args().collect::>(); + if program == "diskutil" || args.first().is_some_and(|arg| *arg == "detach") { + cleanup_attempts += 1; + return Err("Resource busy".to_string()); + } + preparation.push(program.to_string()); + if failure == Some(program) { + return Err(format!("{program} failed")); + } + if program == "ditto" { + assert_eq!(args, [source_app.as_os_str(), staged_app.as_os_str()]); + fs::create_dir_all(&staged_app).unwrap(); + fs::copy(source_app.join("payload"), staged_app.join("payload")).unwrap(); + } else if program == "codesign" || program == "spctl" { + assert_eq!(args.last().copied(), Some(staged_app.as_os_str())); + } + Ok(()) + }, + |_| {}, + ); + assert_eq!(cleanup_attempts, 4); + if let Some(failure) = failure { + assert_eq!(result.unwrap_err(), format!("{failure} failed")); + assert_eq!(preparation.last().map(String::as_str), Some(failure)); + } else { + result.unwrap(); + assert_eq!(preparation, ["hdiutil", "ditto", "codesign", "spctl"]); + assert_eq!(fs::read(staged_app.join("payload")).unwrap(), b"application"); + } + assert!(source_app.join("payload").is_file()); + fs::remove_dir_all(root).unwrap(); + } +} + +#[test] +fn macos_dmg_missing_app_is_rejected_and_still_ejected() { + let root = agent_test_home("macos-dmg-missing-app"); + let mut commands = Vec::new(); + let error = stage_macos_application_from_dmg_with_commands( + &root.join("update.dmg"), + &root, + &root.join("staging/EasyCLIProxyAPI.app"), + |command, _| { + commands.push(command.get_program().to_string_lossy().into_owned()); + Ok(()) + }, + |_| panic!("Successful ejection should not wait"), + ) + .unwrap_err(); + assert!(error.contains("missing EasyCLIProxyAPI.app")); + assert_eq!(commands, ["hdiutil", "diskutil"]); + assert!(!root.join("mount").exists()); + fs::remove_dir_all(root).unwrap(); +} + +#[test] +fn macos_dmg_cleanup_preserves_busy_images_and_retries_later() { + let root = agent_test_home("macos-dmg-cleanup"); + let mount_dir = root.join("mount"); + let archive = root.join("update.dmg"); + let mounted_file = mount_dir.join("mounted-file"); + fs::create_dir_all(&mount_dir).unwrap(); + fs::write(&archive, b"disk image").unwrap(); + fs::write(&mounted_file, b"mounted contents").unwrap(); + + cleanup_macos_update_dmg_with_commands( + &root, + |_, _| Err("Resource busy".to_string()), + |_| {}, + ); + assert_eq!(fs::read(&archive).unwrap(), b"disk image"); + assert_eq!(fs::read(&mounted_file).unwrap(), b"mounted contents"); + + cleanup_macos_update_dmg_with_commands( + &root, + |command, _| { + assert_eq!(command.get_program(), "diskutil"); + // Simulate the mounted contents disappearing after a successful eject. + fs::remove_file(&mounted_file).unwrap(); + Ok(()) + }, + |_| panic!("Successful ejection should not wait"), + ); + assert!(!archive.exists()); + assert!(!mount_dir.exists()); + + cleanup_macos_update_dmg_with_commands( + &root, + |_, _| panic!("An already cleaned image should not be ejected again"), + |_| panic!("An already cleaned image should not wait"), + ); + fs::remove_dir_all(root).unwrap(); +} + fn portable_update_test_asset(version: &str, arch: &str) -> PortableUpdateAsset { let (_, display, suffix) = portable_update_asset_platform().unwrap(); let name = format!("EasyCLIProxyAPI-v{version}-{display}-{arch}.{suffix}");