diff --git a/crates/tty7-core/src/daemon/mod.rs b/crates/tty7-core/src/daemon/mod.rs index b603ac9b..b89d5a39 100644 --- a/crates/tty7-core/src/daemon/mod.rs +++ b/crates/tty7-core/src/daemon/mod.rs @@ -20,6 +20,12 @@ pub(crate) mod shell_integration; #[cfg(windows)] pub(crate) mod winproc; +/// Windows-only like the updater flow that holds it: the macOS updater swaps +/// the bundle by rename and never stops the daemon, so it has no window in +/// which a fresh daemon could relock anything. +#[cfg(windows)] +pub mod update_guard; + /// The Windows environment refresh new panes get (#333). Only the registry /// reader and the spawn wiring are Windows-only; the merge itself is a pure /// function, so `cfg(test)` keeps the module compiling everywhere and its diff --git a/crates/tty7-core/src/daemon/spawn.rs b/crates/tty7-core/src/daemon/spawn.rs index 0a478246..1bafb9d5 100644 --- a/crates/tty7-core/src/daemon/spawn.rs +++ b/crates/tty7-core/src/daemon/spawn.rs @@ -198,6 +198,18 @@ pub fn ensure_running() -> anyhow::Result<()> { } } + // While an updater is replacing the installation, spawning a daemon would + // relock the very images the installer is clearing — the update would fail + // with "files in use" caused by us. Connecting to a live daemon above is + // fine; only creating a new one waits. + #[cfg(windows)] + if crate::daemon::update_guard::held() { + anyhow::bail!( + "a tty7 update is being installed right now; the daemon will return \ + when the updater relaunches the app" + ); + } + spawn_detached()?; let deadline = Instant::now() + STARTUP_TIMEOUT; diff --git a/crates/tty7-core/src/daemon/update_guard.rs b/crates/tty7-core/src/daemon/update_guard.rs new file mode 100644 index 00000000..99f665a5 --- /dev/null +++ b/crates/tty7-core/src/daemon/update_guard.rs @@ -0,0 +1,129 @@ +//! A file that says "an updater is replacing this installation right now". +//! +//! Between `spawn::stop_for_update` clearing the installed images and the +//! installer finishing, nothing used to stop a `tty7` CLI call — or a +//! manually launched GUI — from spawning a fresh daemon that relocks the very +//! files being replaced. The guard closes that window: the updater holds it +//! for the whole installation and releases it just before relaunching the +//! app, and `spawn::ensure_running` refuses to spawn a daemon while it is +//! held. Only spawning is deferred; connecting to a daemon that is already +//! running stays untouched. +//! +//! The guard names its holder by pid so it can never outlive a crashed +//! updater: a guard whose writer is gone is removed on sight, and a TTL +//! backstops the one coincidence pid-liveness cannot see — the dead writer's +//! pid recycled by an unrelated long-lived process. + +use std::path::{Path, PathBuf}; +use std::time::Duration; + +use crate::core::config; + +/// Far above any real installation's duration (Setup runs in seconds), and +/// the most a recycled writer pid can cost. +const GUARD_TTL: Duration = Duration::from_secs(10 * 60); + +fn path() -> Option { + config::config_path("update.lock") +} + +/// Claims the guard for the calling process. +pub fn hold() { + let Some(path) = path() else { return }; + if let Some(parent) = path.parent() { + let _ = std::fs::create_dir_all(parent); + } + if let Err(error) = std::fs::write(&path, std::process::id().to_string()) { + log::warn!( + "could not write the update guard {}: {error}", + path.display() + ); + } +} + +/// Releases the guard. Harmless when it is not held. +pub fn clear() { + if let Some(path) = path() { + let _ = std::fs::remove_file(path); + } +} + +/// Whether an updater is installing right now. A guard whose writer has +/// exited or that outlived the TTL is stale — the updater clears it on every +/// path that relaunches the app, so a leftover means it died — and is removed +/// here so one crash never costs more than one look. +pub(crate) fn held() -> bool { + let Some(path) = path() else { return false }; + let Ok(contents) = std::fs::read_to_string(&path) else { + return false; + }; + let live = contents + .trim() + .parse::() + .is_ok_and(|pid| process_alive(pid)); + if !live || expired(&path) { + log::info!("removing a stale update guard at {}", path.display()); + let _ = std::fs::remove_file(&path); + return false; + } + true +} + +fn expired(path: &Path) -> bool { + std::fs::metadata(path) + .and_then(|meta| meta.modified()) + .ok() + .and_then(|modified| modified.elapsed().ok()) + .is_some_and(|age| age > GUARD_TTL) +} + +fn process_alive(pid: u32) -> bool { + !crate::daemon::winproc::wait_for_exit(pid, Duration::ZERO) +} + +#[cfg(test)] +mod tests { + use super::*; + + fn pin_config_dir() { + let dir = std::env::temp_dir().join(format!("tty7-covtest-{}", std::process::id())); + std::fs::create_dir_all(&dir).ok(); + config::set_config_dir(dir); + } + + fn exited_pid() -> u32 { + let mut child = std::process::Command::new("cmd") + .args(["/C", "exit"]) + .spawn() + .unwrap(); + let pid = child.id(); + child.wait().unwrap(); + pid + } + + // One test, like the pidfile's: the guard file is process-global state, + // and two tests sharing it would race each other. + #[test] + fn guard_lifecycle_holds_for_a_live_writer_and_sheds_stale_files() { + pin_config_dir(); + clear(); + assert!(!held(), "no guard file, no guard"); + + hold(); + assert!(held(), "this process is alive, so its guard holds"); + clear(); + assert!(!held()); + + std::fs::write(path().unwrap(), exited_pid().to_string()).unwrap(); + assert!(!held(), "a dead writer cannot be installing anything"); + assert!( + !path().unwrap().exists(), + "the stale guard is gone after one look" + ); + + // Garbage is stale by the same rule. + std::fs::write(path().unwrap(), "not-a-pid").unwrap(); + assert!(!held()); + assert!(!path().unwrap().exists()); + } +} diff --git a/src/bin/tty7-updater.rs b/src/bin/tty7-updater.rs index ee1a7779..f2a54b94 100644 --- a/src/bin/tty7-updater.rs +++ b/src/bin/tty7-updater.rs @@ -283,13 +283,28 @@ mod macos { } fn wait_for_exit(pid: u32) { + // The updater is spawned directly by the app it waits for, so while + // that app lives it *is* this process's parent, and the kernel + // reparents us to launchd the moment it exits. Watching getppid() is + // therefore immune to pid reuse, which `kill(pid, 0)` is not: a + // recycled pid keeps answering 0 forever. (Windows solves the same + // race by holding a process handle — see the windows module.) + let pid = pid as libc::pid_t; + if unsafe { libc::getppid() } == pid { + while unsafe { libc::getppid() } == pid { + thread::sleep(PARENT_POLL); + } + return; + } + // Not our parent — a hand-run updater. The polling fallback keeps + // that invocation working, pid-reuse caveat and all. while process_alive(pid) { thread::sleep(PARENT_POLL); } } - fn process_alive(pid: u32) -> bool { - unsafe { libc::kill(pid as libc::pid_t, 0) == 0 } + fn process_alive(pid: libc::pid_t) -> bool { + unsafe { libc::kill(pid, 0) == 0 } } fn remove_path(path: &Path) -> Result<(), String> { @@ -486,6 +501,14 @@ mod windows { const PORTABLE_PAYLOAD_DIR: &str = "portable-payload"; const PORTABLE_MARKER: &str = ".tty7-portable"; const PORTABLE_MARKER_CONTENT: &[u8] = b"portable-v1"; + /// Lives inside a portable-update backup from before the first installed + /// file moves until the replacement is fully in place. A backup found + /// later still carrying it names a replacement that was cut short — + /// power loss, a kill — and an installation that may mix two versions; + /// one without it is a finished update whose backup deletion lost to an + /// antivirus scan. The app reads it at launch: duplicated in + /// src/core/update.rs, like the portable marker above. + const PORTABLE_BACKUP_INCOMPLETE: &str = ".tty7-replace-incomplete"; const MAX_PORTABLE_ENTRIES: usize = 4096; const MAX_PORTABLE_EXPANDED_BYTES: u64 = 1024 * 1024 * 1024; // Everything the release package owns: an entry outside this list is @@ -676,6 +699,10 @@ mod windows { &plan.log, "stopping the tty7 daemon and clearing installed-file locks", ); + // From here until the relaunch, a `tty7` CLI call or a manual launch + // must not spawn a daemon that relocks the files Setup is replacing. + // launch_app releases the guard on every path out of this function. + tty7_core::daemon::update_guard::hold(); if let Err(error) = tty7_core::daemon::spawn::stop_for_update(&plan.install_dir) { return recover_from_failed_update(&plan, error); } @@ -738,6 +765,10 @@ mod windows { &plan.log, "stopping the tty7 daemon before replacing portable files", ); + // Held for the whole replacement, exactly as in `install`; the pid it + // records must be this process's — the payload child below exits + // immediately, and a guard naming a dead writer holds nothing. + tty7_core::daemon::update_guard::hold(); if let Err(error) = stop_daemon_from_payload(&payload, &plan.install_dir) { return recover_without_replacement(&plan.log, &plan.install_dir, &plan.stage, error); } @@ -1158,6 +1189,15 @@ mod windows { return Err(with_relaunch_failure(cause, relaunch_previous(install_dir))); } }; + // Marked incomplete before any installed file moves. Nothing here + // removes the marker on rollback: a rollback that succeeds removes + // the whole backup, and one that fails leaves an installation whose + // state really is suspect. + if let Err(error) = fs::write(backup.join(PORTABLE_BACKUP_INCOMPLETE), b"") { + let cause = format!("marking the update backup {}: {error}", backup.display()); + let _ = remove_path(&backup); + return Err(with_relaunch_failure(cause, relaunch_previous(install_dir))); + } let mut moved = Vec::new(); for root in PORTABLE_MANAGED_ROOTS { @@ -1197,6 +1237,9 @@ mod windows { // The replacement survived its launch grace period. Old managed files // are no longer needed; an antivirus-held backup is harmless and can be // removed manually rather than turning a successful update into rollback. + // The marker leaves first: a backup that outlives this process without + // it is finished business the next launch may discard on its own. + let _ = fs::remove_file(backup.join(PORTABLE_BACKUP_INCOMPLETE)); let _ = remove_path(&backup); Ok(()) } @@ -1338,6 +1381,10 @@ mod windows { } fn launch_app(install_dir: &Path) -> Result<(), String> { + // Every relaunch — success, failure recovery, rollback — is a point + // where the installation is no longer being replaced, so the daemon + // spawn guard ends here, before the app comes up and asks for one. + tty7_core::daemon::update_guard::clear(); let executable = install_dir.join("tty7-app.exe"); let mut child = Command::new(&executable) .stdin(Stdio::null()) @@ -1861,6 +1908,51 @@ mod windows { ); } + #[test] + fn a_backup_carries_the_incomplete_marker_exactly_while_files_move() { + let install = tempfile::tempdir().unwrap(); + let payload = tempfile::tempdir().unwrap(); + fs::write(install.path().join("tty7-app.exe"), b"old app").unwrap(); + fs::write(payload.path().join("tty7-app.exe"), b"new app").unwrap(); + let install_dir = install.path().to_path_buf(); + + replace_portable_and_relaunch( + install.path(), + payload.path(), + move |_| { + let backups: Vec<_> = fs::read_dir(&install_dir) + .unwrap() + .flatten() + .map(|entry| entry.path()) + .filter(|path| { + path.file_name() + .and_then(|name| name.to_str()) + .is_some_and(|name| name.starts_with(".tty7-update-backup-")) + }) + .collect(); + assert_eq!(backups.len(), 1, "one backup during the replacement"); + assert!( + backups[0].join(PORTABLE_BACKUP_INCOMPLETE).is_file(), + "the marker is present while files are moving" + ); + Ok(()) + }, + |_| panic!("the previous version must not relaunch after success"), + ) + .unwrap(); + + let leftovers: Vec<_> = fs::read_dir(install.path()) + .unwrap() + .flatten() + .map(|entry| entry.file_name()) + .collect(); + assert_eq!( + leftovers, + vec![std::ffi::OsString::from("tty7-app.exe")], + "no backup and no marker survive a completed replacement" + ); + } + #[test] fn portable_replacement_rolls_back_when_the_new_app_does_not_start() { let install = tempfile::tempdir().unwrap(); diff --git a/src/core/update.rs b/src/core/update.rs index f7fee426..d728cfeb 100644 --- a/src/core/update.rs +++ b/src/core/update.rs @@ -89,6 +89,14 @@ const WINDOWS_INNO_INSTALL_MARKER: &str = ".tty7-inno-install"; const WINDOWS_PORTABLE_MARKER: &str = ".tty7-portable"; #[cfg(target_os = "windows")] const WINDOWS_PORTABLE_MARKER_CONTENT: &[u8] = b"portable-v1"; +/// What the updater names the directory it moves the old portable files into. +#[cfg(target_os = "windows")] +const WINDOWS_PORTABLE_BACKUP_PREFIX: &str = ".tty7-update-backup-"; +/// Present inside that backup from before the first installed file moves +/// until the replacement is complete — see `PORTABLE_BACKUP_INCOMPLETE` in +/// src/bin/tty7-updater.rs, which this duplicates like the markers above. +#[cfg(target_os = "windows")] +const WINDOWS_PORTABLE_BACKUP_INCOMPLETE: &str = ".tty7-replace-incomplete"; #[derive(Clone, Debug, PartialEq, Eq)] pub struct AvailableUpdate { @@ -188,6 +196,15 @@ pub struct UpdateStatus { impl Global for UpdateStatus {} pub fn spawn_check(cx: &mut App) { + // Before hydration and before the config gate: an interrupted portable + // replacement has to surface whether or not checking is on, and it is + // recorded as a failure so `hydrate_from_disk` below carries it into the + // UI. The scan itself is one `read_dir`; the deletions it hands back ride + // the background sweep. + #[cfg(target_os = "windows")] + let finished_backups = reconcile_portable_backups(); + #[cfg(not(target_os = "windows"))] + let finished_backups: Vec = Vec::new(); // Before the config gate: a package staged by an earlier run, or a failure // from one, has to reach Settings whether or not checking is still on. hydrate_from_disk(cx); @@ -195,7 +212,16 @@ pub fn spawn_check(cx: &mut App) { // waits on the result. let keep = UpdateState::load().pending.map(|pending| pending.stage); cx.background_executor() - .spawn(async move { sweep_orphaned_stages(keep) }) + .spawn(async move { + for backup in finished_backups { + log::info!( + "removing a completed update's leftover backup at {}", + backup.display() + ); + let _ = std::fs::remove_dir_all(&backup); + } + sweep_orphaned_stages(keep) + }) .detach(); if !cx.global::().check_for_updates { return; @@ -971,6 +997,73 @@ fn is_stage_name(name: &str) -> bool { name.starts_with(".tty7-update-") || name.starts_with("tty7-update-") } +/// Deals with `.tty7-update-backup-*` directories a previous portable update +/// left in the installation. +/// +/// One still carrying the incomplete marker means the replacement was cut +/// short — power loss, a kill — and the installed files may mix two versions. +/// That is recorded as an update failure so Settings shows it (and shows it +/// again at every launch until the user restores or deletes the backup: the +/// warning describes a condition, not an event). The backup itself is kept — +/// it holds the only copy of the previous files. +/// +/// One without the marker is a completed update whose backup deletion lost to +/// an antivirus scan; those are returned for the background sweep to remove. +#[cfg(target_os = "windows")] +fn reconcile_portable_backups() -> Vec { + let Some(WindowsUpdateLayout::Portable(dir)) = current_windows_update_layout() else { + return Vec::new(); + }; + let (interrupted, finished) = scan_portable_backups(&dir); + if let Some(backup) = interrupted.first() { + let detail = format!( + "a previous update was interrupted while replacing the installed files, so {} may \ + mix two versions; the files from before that update are preserved at {} — restore \ + them or reinstall, then delete the backup", + dir.display(), + backup.display() + ); + log::warn!("{detail}"); + let mut state = UpdateState::load(); + if state.last_failure.as_ref().map(|failure| &failure.detail) != Some(&detail) { + state.last_failure = Some(FailureRecord { + version: current_version().to_string(), + detail, + }); + state.save(); + } + } + finished +} + +/// Splits the backups under `dir` into (interrupted, finished) by the +/// incomplete marker. Pure directory inspection, so the policy above stays +/// testable without a real installation. +#[cfg(target_os = "windows")] +fn scan_portable_backups(dir: &Path) -> (Vec, Vec) { + let mut interrupted = Vec::new(); + let mut finished = Vec::new(); + let Ok(entries) = std::fs::read_dir(dir) else { + return (interrupted, finished); + }; + for entry in entries.flatten() { + let path = entry.path(); + let named = path + .file_name() + .and_then(|name| name.to_str()) + .is_some_and(|name| name.starts_with(WINDOWS_PORTABLE_BACKUP_PREFIX)); + if !named || !path.is_dir() { + continue; + } + if path.join(WINDOWS_PORTABLE_BACKUP_INCOMPLETE).is_file() { + interrupted.push(path); + } else { + finished.push(path); + } + } + (interrupted, finished) +} + pub(crate) fn localized_update_phase(phase: &UpdatePhase) -> Option { match phase { UpdatePhase::Idle => None, @@ -2039,6 +2132,24 @@ mod tests { ); } + #[cfg(target_os = "windows")] + #[test] + fn portable_backup_scan_separates_interrupted_from_finished() { + let root = tempfile::tempdir().unwrap(); + let interrupted = root.path().join(".tty7-update-backup-cut"); + std::fs::create_dir(&interrupted).unwrap(); + std::fs::write(interrupted.join(WINDOWS_PORTABLE_BACKUP_INCOMPLETE), b"").unwrap(); + let finished = root.path().join(".tty7-update-backup-done"); + std::fs::create_dir(&finished).unwrap(); + // Neither a user's directory nor a stray file may be touched. + std::fs::create_dir(root.path().join("completions")).unwrap(); + std::fs::write(root.path().join(".tty7-update-backup-not-a-dir"), b"file").unwrap(); + + let (got_interrupted, got_finished) = scan_portable_backups(root.path()); + assert_eq!(got_interrupted, vec![interrupted]); + assert_eq!(got_finished, vec![finished]); + } + #[test] fn parses_versions_with_and_without_prefix() { let release = |major, minor, patch| Some((major, minor, patch, true, vec![]));