diff --git a/src-tauri/src/acp/deploy.rs b/src-tauri/src/acp/deploy.rs index 3cb19b79..6ccf8269 100644 --- a/src-tauri/src/acp/deploy.rs +++ b/src-tauri/src/acp/deploy.rs @@ -517,6 +517,10 @@ async fn ensure_cached_binary( let dest = cached_binary_path(app_handle, triple)?; let local_version = deployment_version(); + if let Some(parent) = dest.parent() { + sweep_stale_binaries(parent).await; + } + if let Some(id) = emit_connection_id { emit_status(app_handle, id, "checking", None); } @@ -575,6 +579,137 @@ async fn check_cached_version(path: &std::path::Path) -> Option { } } +/// A unique `.old-` sibling of `path`. Appended to the whole file name rather +/// than built with `with_extension`, which would eat the `.exe`. +fn stale_sibling(path: &std::path::Path) -> Result { + let name = path + .file_name() + .and_then(|name| name.to_str()) + .ok_or_else(|| format!("Cannot derive a sibling name for {}", path.display()))?; + let nanos = std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .map(|elapsed| elapsed.as_nanos()) + .unwrap_or(0); + Ok(path.with_file_name(format!("{}.old-{}", name, nanos))) +} + +/// Move `tmp` into place at `dest`, tolerating a `dest` that a running process still holds open. +/// +/// Windows refuses to overwrite the image of a running process, so an orphaned maestro-server — +/// left by a crash, or by the installer killing the app mid-update — makes the plain rename fail +/// with `ERROR_ACCESS_DENIED`. That stranded the user on the previous binary, which the updated +/// app cannot talk to, and no later attempt could recover because the orphan outlives them all. +/// Renaming a running image *is* permitted, so the old binary is moved aside and the new one +/// takes its name. Deleting the aside copy only succeeds once the process holding it exits, which +/// is why that failing is not an error here — `sweep_stale_binaries` reclaims it on a later run. +/// +/// Deliberately not gated on `cfg(windows)`: on Unix the first rename replaces a running binary +/// happily, so the fallback is simply never reached and one code path is less than two. +async fn install_binary_at(tmp: &std::path::Path, dest: &std::path::Path) -> Result<(), String> { + let blocked = match tokio::fs::rename(tmp, dest).await { + Ok(()) => return Ok(()), + Err(error) => error, + }; + + let aside = stale_sibling(dest)?; + if let Err(error) = tokio::fs::rename(dest, &aside).await { + discard_temp_download(tmp).await; + return Err(format!( + "Failed to install {}: {} (moving the old one aside also failed: {}). \ + A running maestro-server is still holding it — quit Maestro and try again.", + dest.display(), + blocked, + error + )); + } + + match tokio::fs::rename(tmp, dest).await { + Ok(()) => { + if let Err(error) = tokio::fs::remove_file(&aside).await { + log::debug!( + "[deploy] {} is still in use, sweeping it on a later run: {}", + aside.display(), + error + ); + } + Ok(()) + } + Err(error) => { + // Put the old binary back rather than leaving nothing at all at dest. + if let Err(restore) = tokio::fs::rename(&aside, dest).await { + log::warn!( + "[deploy] could not restore {} after a failed install: {}", + dest.display(), + restore + ); + } + discard_temp_download(tmp).await; + Err(format!("Failed to install {}: {}", dest.display(), error)) + } + } +} + +async fn discard_temp_download(tmp: &std::path::Path) { + if let Err(error) = tokio::fs::remove_file(tmp).await { + log::debug!( + "[deploy] could not remove the partial download {}: {}", + tmp.display(), + error + ); + } +} + +/// Remove binaries that [`install_binary_at`] moved aside on an earlier run. +/// +/// Best effort: an `.old-` file whose process is still alive cannot be deleted yet, which is the +/// expected case rather than a problem, so failures are logged at `debug` and the sweep goes on. +/// `.download-tmp` files are deliberately left alone — their names are fixed per triple, so the +/// next download overwrites them, and deleting one here could pull it out from under a concurrent +/// `ensure_cached_binary` for a different triple. +async fn sweep_stale_binaries(dir: &std::path::Path) { + let mut entries = match tokio::fs::read_dir(dir).await { + Ok(entries) => entries, + Err(error) => { + log::debug!( + "[deploy] cannot scan {} for stale binaries: {}", + dir.display(), + error + ); + return; + } + }; + + loop { + match entries.next_entry().await { + Ok(Some(entry)) => { + let file_name = entry.file_name(); + let is_stale = file_name + .to_str() + .is_some_and(|name| name.contains(".old-")); + if !is_stale { + continue; + } + if let Err(error) = tokio::fs::remove_file(entry.path()).await { + log::debug!( + "[deploy] stale binary {} not removed yet: {}", + entry.path().display(), + error + ); + } + } + Ok(None) => break, + Err(error) => { + log::debug!( + "[deploy] stale binary scan of {} ended early: {}", + dir.display(), + error + ); + break; + } + } + } +} + async fn download_server_binary(triple: &str, dest: &std::path::Path) -> Result<(), String> { let version = env!("CARGO_PKG_VERSION"); let filename = asset_filename(triple); @@ -605,9 +740,7 @@ async fn download_server_binary(triple: &str, dest: &std::path::Path) -> Result< tokio::fs::write(&tmp_path, &bytes) .await .map_err(|e| format!("Failed to write binary to temp file: {}", e))?; - tokio::fs::rename(&tmp_path, dest) - .await - .map_err(|e| format!("Failed to install binary: {}", e))?; + install_binary_at(&tmp_path, dest).await?; #[cfg(unix)] { @@ -630,3 +763,119 @@ fn emit_status(app_handle: &AppHandle, connection_id: i32, status: &str, message }, ); } + +#[cfg(test)] +mod tests { + use super::*; + use std::path::{Path, PathBuf}; + + fn write(path: &Path, contents: &str) { + std::fs::write(path, contents).expect("test file should be writable"); + } + + fn siblings_ending_in_old(dir: &Path) -> Vec { + std::fs::read_dir(dir) + .expect("test dir should be readable") + .filter_map(|entry| entry.ok().map(|entry| entry.path())) + .filter(|path| { + path.file_name() + .and_then(|name| name.to_str()) + .is_some_and(|name| name.contains(".old-")) + }) + .collect() + } + + #[tokio::test] + async fn a_free_destination_is_replaced_outright() { + let dir = tempfile::tempdir().expect("tempdir"); + let dest = dir.path().join("maestro-server.exe"); + let tmp = dir.path().join("maestro-server.download-tmp"); + write(&dest, "old"); + write(&tmp, "new"); + + install_binary_at(&tmp, &dest).await.expect("install"); + + assert_eq!(std::fs::read_to_string(&dest).unwrap(), "new"); + assert!(!tmp.exists(), "the temp file should have been consumed"); + assert!( + siblings_ending_in_old(dir.path()).is_empty(), + "no aside copy is needed when the destination is free" + ); + } + + #[tokio::test] + async fn the_sweep_reclaims_aside_copies_and_leaves_the_binary() { + let dir = tempfile::tempdir().expect("tempdir"); + let dest = dir.path().join("maestro-server.exe"); + let stale = stale_sibling(&dest).expect("sibling name"); + let partial = dir.path().join("maestro-server.download-tmp"); + write(&dest, "live"); + write(&stale, "orphaned"); + write(&partial, "half a download"); + + sweep_stale_binaries(dir.path()).await; + + assert!(!stale.exists(), "the aside copy should be gone"); + assert!(dest.exists(), "the live binary must survive the sweep"); + assert!( + partial.exists(), + "a .download-tmp may belong to a concurrent download and is left alone" + ); + } + + /// The bug this whole path exists for: Windows will not let a running process's image be + /// overwritten, so the plain rename fails and the binary can never be updated. `ping.exe` + /// stands in for an orphaned maestro-server — any live process holding the destination + /// reproduces it. + #[cfg(windows)] + #[tokio::test] + async fn a_destination_held_by_a_running_process_is_still_replaced() { + let system_root = match std::env::var("SystemRoot") { + Ok(root) => root, + Err(_) => return, + }; + let source = PathBuf::from(system_root).join("System32").join("PING.EXE"); + if !source.exists() { + return; + } + + let dir = tempfile::tempdir().expect("tempdir"); + let dest = dir.path().join("maestro-server.exe"); + std::fs::copy(&source, &dest).expect("copy ping.exe into place"); + let tmp = dir.path().join("maestro-server.download-tmp"); + write(&tmp, "the new build"); + + let mut holder = tokio::process::Command::new(&dest) + .args(["-n", "30", "127.0.0.1"]) + .stdin(std::process::Stdio::null()) + .stdout(std::process::Stdio::null()) + .stderr(std::process::Stdio::null()) + .kill_on_drop(true) + .no_console_window() + .spawn() + .expect("spawn the process holding the destination"); + + // Without the fallback in install_binary_at this is where the update fails. + assert!( + tokio::fs::rename(&tmp, &dest).await.is_err(), + "a running image must not be overwritable, or this test proves nothing" + ); + + install_binary_at(&tmp, &dest) + .await + .expect("install should route around the lock"); + assert_eq!(std::fs::read_to_string(&dest).unwrap(), "the new build"); + assert_eq!( + siblings_ending_in_old(dir.path()).len(), + 1, + "the held binary should have been moved aside, not deleted" + ); + + holder.kill().await.expect("kill the holder"); + sweep_stale_binaries(dir.path()).await; + assert!( + siblings_ending_in_old(dir.path()).is_empty(), + "the aside copy is reclaimed once nothing holds it" + ); + } +} diff --git a/src-tauri/src/main.rs b/src-tauri/src/main.rs index 223ff3c3..1df5aaf0 100644 --- a/src-tauri/src/main.rs +++ b/src-tauri/src/main.rs @@ -80,6 +80,24 @@ fn resolve_data_dir(app: &tauri::App) -> Result { .map_err(|e| format!("Failed to get app data directory: {}", e)) } +/// Stop the maestro-server processes this instance started, before the app exits. +/// +/// Without this they are orphaned. `kill_on_drop(true)` only fires when the `Child` is dropped +/// inside the runtime, and `handle.exit(0)` drops neither map, so the children outlive us — and a +/// stray server holds the cached `maestro-server.exe` open, which is what makes the next update +/// fail: Windows will not let the image of a running process be overwritten. Clearing the maps is +/// exactly what `release_active_project_lock` does when the user leaves a project; quitting simply +/// never did it, despite that function's own comment claiming both paths. +/// +/// This kills rather than asking the servers to wind down, because a graceful exit is not +/// reachable from here: a server only exits on stdin EOF, the pipe only closes when the last +/// `writer_tx` clone drops, and the shared reader task holds one until *it* sees EOF. Leaving +/// projects has always ended connection servers this way. +async fn stop_connection_servers(state: &Arc) { + state.acp.sessions.lock().await.clear(); + state.acp.connection_servers.lock().await.clear(); +} + /// Setup hook for Tauri initialization fn setup(app: &mut tauri::App) -> Result<(), Box> { let app_data_dir = resolve_data_dir(app)?; @@ -177,6 +195,7 @@ fn main() { } // Give maestro-server time to forward CloseSessionRequest to agents. tokio::time::sleep(std::time::Duration::from_millis(500)).await; + stop_connection_servers(&state).await; handle.exit(0); }); }