From 85de86be60dfb6350be052e9deb81632af6af5c0 Mon Sep 17 00:00:00 2001 From: dotCooCoo Date: Sat, 7 Nov 2026 16:14:17 -0800 Subject: [PATCH 01/25] craft-update: start the Windows update helper when the app closes, and never restart a running app On Windows the update helper was started as soon as the download was ready, then waited one minute for the app to exit. An app that held unsaved work, or a user who did not close it, outlasted that wait; the helper then started the old exe again with no arguments, which opened a second window of the old version and installed nothing. - self_update::prepare_update does what update did but returns a Restart. On Windows the Restart records the release as installed and starts the helper; on macOS and Linux it does nothing, since the restart is already scheduled. update calls prepare_update and starts the Restart at once, so its callers see no change. - Updater starts the Restart when it returns Action::Close. If the helper can't start, the app stays open and the prompt shows the error. - The helper does not start any exe when the app is still running after its wait. A step that fails after the app exited still starts the old exe, as before. --- crates/update/src/self_update.rs | 291 ++++++++++++++++++++++++++----- crates/update/src/updater.rs | 94 ++++++++-- 2 files changed, 327 insertions(+), 58 deletions(-) diff --git a/crates/update/src/self_update.rs b/crates/update/src/self_update.rs index 256081ce..d9bde551 100644 --- a/crates/update/src/self_update.rs +++ b/crates/update/src/self_update.rs @@ -244,11 +244,12 @@ fn msi_step(app: &App, exe: &Path) -> Result { crate::msi_migrate::windows::step(app, exe) } -/// Whether the helper starts the app after the update step: always after a successful one, and -/// after a failed one only while its exe is still there. +/// Whether the helper starts the app after the update step: never while the old app is still +/// running (a second copy of it would open), always after a successful step, and after a failed +/// one only while its exe is still there. #[cfg_attr(not(windows), allow(dead_code))] -fn restarts(step_succeeded: bool, exe_exists: bool) -> bool { - step_succeeded || exe_exists +fn restarts(app_exited: bool, step_succeeded: bool, exe_exists: bool) -> bool { + app_exited && (step_succeeded || exe_exists) } /// The exe to start after an MSI update of a registered copy: the one its registrations name now @@ -371,6 +372,58 @@ pub fn update( last: &dyn Fn() -> Result<(), String>, running_version: Option<&str>, ) -> Result<(), String> { + prepare_update(app, install, release, platform, http, paths, stop, phase, last, running_version)?.start() +} + +/// What remains of an update after [`prepare_update`]: starting the Windows helper, which waits +/// for this process to exit and then installs the update. On macOS and Linux the update is +/// already in place and its restart already scheduled, so [`Restart::start`] does nothing. +#[must_use = "on Windows the update is only installed once the restart is started"] +pub struct Restart { + helper: Option Result<(), String> + Send>>, +} + +impl Restart { + /// Nothing is left to start. + #[cfg_attr(all(windows, not(test)), allow(dead_code))] + pub(crate) fn scheduled() -> Self { + Self { helper: None } + } + + /// `start` runs when the app closes. + #[cfg_attr(not(any(windows, test)), allow(dead_code))] + pub(crate) fn later(start: impl FnOnce() -> Result<(), String> + Send + 'static) -> Self { + Self { helper: Some(Box::new(start)) } + } + + /// Starts the helper. Call it when the app closes: the helper waits only a minute for the + /// app to exit, and installs nothing when the app is still running after that. + pub fn start(self) -> Result<(), String> { + self.helper.map_or(Ok(()), |start| start()) + } +} + +impl std::fmt::Debug for Restart { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.debug_struct("Restart").field("pending", &self.helper.is_some()).finish() + } +} + +/// [`update`] without starting the Windows helper: the caller starts the returned [`Restart`] +/// when the app closes. On macOS and Linux the result is the same as [`update`]'s. +#[allow(clippy::too_many_arguments)] +pub fn prepare_update( + app: &App, + install: &Install, + release: &Release, + platform: Platform, + http: &Http, + paths: &Paths, + stop: &Stop, + phase: &mut dyn FnMut(JobPhase), + last: &dyn Fn() -> Result<(), String>, + running_version: Option<&str>, +) -> Result { let cur_v = running_version.and_then(|v| current_version(app, v)); if let (Some(rel_v), Some(cur_v)) = (release.version.as_ref(), cur_v.as_ref()) && !version::offers_update(rel_v, cur_v) @@ -390,11 +443,13 @@ pub fn update( // The restart ends this process: nothing else may be under way. last()?; phase(JobPhase::Installing); - // Written before the swap starts: the Windows helper runs alongside this process and clears - // it when it fails, which a write after scheduling could undo. + // Written before the swap starts. On Windows the helper does the swap, so the record is + // written when it starts (see `windows_helper`): it clears the record when it fails, which a + // write after it started could undo. + #[cfg(not(windows))] mark_installed(paths, release); let pid = std::process::id(); - let scheduled = (|| -> Result<(), String> { + let scheduled = (|| -> Result { match install { #[cfg(target_os = "macos")] Install::MacApp(bundle) => { @@ -402,20 +457,20 @@ pub fn update( let placed = install::macos::install_dmg(app, &file, Some(&existing), None, false, paths, &install::macos::Expect::release(release)); let _ = std::fs::remove_file(&file); let placed = placed?; - relaunch_after_exit(app, pid, &[Path::new("/usr/bin/open").as_os_str(), placed.as_os_str()]) + relaunch_after_exit(app, pid, &[Path::new("/usr/bin/open").as_os_str(), placed.as_os_str()]).map(|()| Restart::scheduled()) } #[cfg(target_os = "linux")] Install::AppImage(path) => { replace_appimage(&file, path)?; let _ = std::fs::remove_file(&file); - relaunch_after_exit(app, pid, &[path.as_os_str()]) + relaunch_after_exit(app, pid, &[path.as_os_str()]).map(|()| Restart::scheduled()) } #[cfg(target_os = "linux")] Install::Package(kind, exe) => { let result = install::linux::install_package(*kind, &file, None, stop, phase); let _ = std::fs::remove_file(&file); result?; - relaunch_after_exit(app, pid, &[exe.as_os_str()]) + relaunch_after_exit(app, pid, &[exe.as_os_str()]).map(|()| Restart::scheduled()) } #[cfg(target_os = "linux")] Install::Flatpak => { @@ -429,26 +484,26 @@ pub fn update( let result = install::linux::update_flatpak_bundle(&file, &app.bundle_id(), system, stop, phase); let _ = std::fs::remove_file(&file); result?; - relaunch_flatpak_after_exit(app) + relaunch_flatpak_after_exit(app).map(|()| Restart::scheduled()) } #[cfg(windows)] - Install::Msi(exe) => windows_helper(app, paths, pid, exe, &msi_finish(&file)?), + Install::Msi(exe) => windows_helper(app, paths, release, pid, exe, &msi_finish(&file)?), #[cfg(windows)] Install::Portable(exe) => { exe.parent().ok_or_else(|| format!("{} isn't in a folder", app.name))?; - windows_helper(app, paths, pid, exe, &Finish::Portable { zip: file, tmp: paths.cache.join(PORTABLE_STAGING) }) + windows_helper(app, paths, release, pid, exe, &Finish::Portable { zip: file, tmp: paths.cache.join(PORTABLE_STAGING) }) } #[cfg(windows)] Install::Standalone(exe) => { exe.parent().ok_or_else(|| format!("{} isn't in a folder", app.name))?; - windows_helper(app, paths, pid, exe, &Finish::Portable { zip: file, tmp: paths.cache.join(PORTABLE_STAGING) }) + windows_helper(app, paths, release, pid, exe, &Finish::Portable { zip: file, tmp: paths.cache.join(PORTABLE_STAGING) }) } #[cfg(unix)] Install::Standalone(exe) => { let result = replace_standalone(app, paths, &file, exe); let _ = std::fs::remove_file(&file); result?; - relaunch_after_exit(app, pid, &[exe.as_os_str()]) + relaunch_after_exit(app, pid, &[exe.as_os_str()]).map(|()| Restart::scheduled()) } #[allow(unreachable_patterns)] _ => { @@ -469,7 +524,7 @@ fn mark_installed(paths: &Paths, release: &Release) { /// Removes the record of [`mark_installed`] when the swap couldn't be scheduled, so the update /// stays on offer. -fn unmark_unless_scheduled(paths: &Paths, scheduled: Result<(), String>) -> Result<(), String> { +fn unmark_unless_scheduled(paths: &Paths, scheduled: Result) -> Result { if scheduled.is_err() { let _ = std::fs::remove_file(marker(paths)); } @@ -759,11 +814,12 @@ const HELPER_DIR: &str = "launcher-update-helper"; #[cfg_attr(not(windows), allow(dead_code))] const PORTABLE_STAGING: &str = "launcher-update"; -/// Copies this program into the cache and starts the copy as the update helper, which waits for -/// `pid` to exit, does `finish` and starts `exe`. Run from the cache, the helper doesn't hold the -/// installed exe that the step replaces. +/// Copies this program into the cache as the update helper, which waits for `pid` to exit, does +/// `finish` and starts `exe`. Run from the cache, the helper doesn't hold the installed exe that +/// the step replaces. The returned [`Restart`] records `release` as installed and starts the +/// helper. #[cfg(windows)] -fn windows_helper(app: &App, paths: &Paths, pid: u32, exe: &Path, finish: &Finish) -> Result<(), String> { +fn windows_helper(app: &App, paths: &Paths, release: &Release, pid: u32, exe: &Path, finish: &Finish) -> Result { let program = std::env::current_exe().map_err(|e| format!("couldn't find this program ({e})"))?; let dir = paths.cache.join(HELPER_DIR); // The previous update's copy, when it's still there. @@ -774,7 +830,12 @@ fn windows_helper(app: &App, paths: &Paths, pid: u32, exe: &Path, finish: &Finis let mut cmd = process::command(&helper); cmd.args(helper_args(pid, exe, finish)); clear_pretend_version(&mut cmd, app); - process::spawn_detached(cmd).map_err(|e| format!("couldn't schedule the update ({e})")) + let (paths, release) = (paths.clone(), release.clone()); + Ok(Restart::later(move || { + mark_installed(&paths, &release); + let started = process::spawn_detached(cmd).map_err(|e| format!("couldn't schedule the update ({e})")); + unmark_unless_scheduled(&paths, started) + })) } /// Finishes an update when this program was started as the update helper; `args` are its @@ -799,23 +860,48 @@ pub fn finish(app: &App, args: &[OsString]) -> Option> { // Read before the step: the update may move or replace the old exe. let registered = matches!(step, Finish::Msi(_)) && matches!(crate::msi_copy::windows::find(app, &exe), Some(crate::msi_copy::MsiCopy::Registered { .. })); - let done = run_step(app, &me, pid, &exe, &step, std::time::Duration::from_secs(60)); + let wait = |pid| process::wait_for_exit(pid, std::time::Duration::from_secs(60)); + let start = |exe: &Path| { + signed_like(app, &me, exe) + .and_then(|()| process::spawn_detached(std::process::Command::new(exe)).map_err(|e| format!("couldn't start {} ({e})", exe.display()))) + }; + Some(finish_step(app, &me, &paths, pid, exe, &step, registered, &wait, &start)) +} + +/// The helper's work once its checks have passed: waits for `pid` with `wait`, does `step`, and +/// starts the app with `start` when [`restarts`] allows it. Every failure is recorded for the +/// app's next start. +#[cfg(windows)] +#[allow(clippy::too_many_arguments)] +fn finish_step( + app: &App, + reference: &Path, + paths: &Paths, + pid: u32, + exe: PathBuf, + step: &Finish, + registered: bool, + wait: &dyn Fn(u32) -> bool, + start: &dyn Fn(&Path) -> Result<(), String>, +) -> Result<(), String> { + let stepped = step_after_wait(app, reference, pid, &exe, step, wait); + let done = stepped.result; if let Err(e) = &done { - record_failure(&paths, e); + record_failure(paths, e); } // The new app, or the old one when the step failed. let exe = if done.is_ok() { exe_after_msi(app, registered, &exe) } else { exe }; - // A failed move can leave no program to start; its recorded error already says how to get - // the app back, and a restart error must not replace it. - if !restarts(done.is_ok(), exe.is_file()) { - return Some(done); + // An app that is still running gets no second copy. A failed move can leave no program to + // start; its recorded error already says how to get the app back, and a restart error must + // not replace it. + if !restarts(stepped.exited, done.is_ok(), exe.is_file()) { + return done; } - let started = signed_like(app, &me, &exe) - .and_then(|()| process::spawn_detached(std::process::Command::new(&exe)).map_err(|e| format!("couldn't start {} ({e})", exe.display()))); + let started = start(&exe); if let Err(e) = &started { - record_failure(&paths, e); + record_failure(paths, e); } - Some(done.and(started)) + done.and(started) } /// Finishes an update on non-Windows platforms (no-op helper). @@ -990,14 +1076,29 @@ mod wait_watch { } } -/// Waits up to `wait` for `pid` to exit, does `step` once it has, and removes the download either -/// way. The MSI, and every program in the portable zip, must be signed like `reference`. -#[cfg(windows)] +/// [`step_after_wait`], waiting up to `wait` for `pid` to exit. +#[cfg(all(windows, test))] fn run_step(app: &App, reference: &Path, pid: u32, exe: &Path, step: &Finish, wait: std::time::Duration) -> Result<(), String> { + step_after_wait(app, reference, pid, exe, step, &|pid| process::wait_for_exit(pid, wait)).result +} + +/// What [`step_after_wait`] came to. +#[cfg(windows)] +struct Stepped { + /// The old app had exited when the wait returned. + exited: bool, + result: Result<(), String>, +} + +/// Calls `wait`, which says whether `pid` exited in time, does `step` once it has, and removes +/// the download either way. The MSI, and every program in the portable zip, must be signed like +/// `reference`. +#[cfg(windows)] +fn step_after_wait(app: &App, reference: &Path, pid: u32, exe: &Path, step: &Finish, wait: &dyn Fn(u32) -> bool) -> Stepped { #[cfg(test)] wait_watch::entered(pid); // The test hook needs the result, so it is stored rather than consumed by the branch. - let waited = process::wait_for_exit(pid, wait); + let waited = wait(pid); #[cfg(test)] wait_watch::returned(pid, waited); let result = if waited { @@ -1025,7 +1126,7 @@ fn run_step(app: &App, reference: &Path, pid: u32, exe: &Path, step: &Finish, wa let _ = std::fs::remove_file(zip); } } - result + Stepped { exited: waited, result } } #[cfg(test)] @@ -1434,7 +1535,7 @@ mod tests { std::fs::create_dir_all(&paths.data).unwrap(); let release = Release { tag: "v0.2.0".into(), ..Release::default() }; mark_installed(&paths, &release); - assert!(unmark_unless_scheduled(&paths, Err("couldn't schedule the update".into())).is_err()); + assert!(unmark_unless_scheduled(&paths, Err::<(), _>("couldn't schedule the update".into())).is_err()); assert!(!just_updated_to(&paths, &release)); mark_installed(&paths, &release); assert!(unmark_unless_scheduled(&paths, Ok(())).is_ok()); @@ -1986,10 +2087,118 @@ mod tests { #[test] fn a_failed_step_that_removed_the_app_starts_nothing_and_keeps_its_message() { - assert!(restarts(true, true)); - assert!(restarts(true, false)); - assert!(restarts(false, true)); - assert!(!restarts(false, false)); + assert!(restarts(true, true, true)); + assert!(restarts(true, true, false)); + assert!(restarts(true, false, true)); + assert!(!restarts(true, false, false)); + } + + /// An app still running after the helper's wait never gets a second copy, whatever the step + /// and the exe would allow. + #[test] + fn an_app_still_running_is_never_started_again() { + for (step_succeeded, exe_exists) in [(true, true), (true, false), (false, true), (false, false)] { + assert!(!restarts(false, step_succeeded, exe_exists), "{step_succeeded} {exe_exists}"); + } + } + + /// A helper whose wait ends with the old app still running installs nothing, starts no + /// program, records why, and removes the download. + #[cfg(windows)] + #[test] + fn the_helper_starts_nothing_while_the_old_app_runs() { + let dir = tempfile::tempdir().unwrap(); + let paths = Paths::under(dir.path()); + let exe = dir.path().join("artcraft-launcher.exe"); + std::fs::write(&exe, "0.1.0").unwrap(); + let zip = dir.path().join("artcraft-launcher-0.2.0-windows-x64-portable.zip"); + std::fs::write(&zip, "zip").unwrap(); + let step = Finish::Portable { zip: zip.clone(), tmp: dir.path().join(PORTABLE_STAGING) }; + let started = std::sync::Mutex::new(Vec::::new()); + let start = |exe: &Path| { + started.lock().unwrap().push(exe.to_path_buf()); + Ok(()) + }; + let never_exits = |_pid| false; + let error = finish_step(&crate::suite::LAUNCHER, &system_program("cmd.exe"), &paths, 4242, exe.clone(), &step, false, &never_exits, &start) + .unwrap_err(); + assert!(error.contains("still running"), "{error}"); + assert_eq!(*started.lock().unwrap(), Vec::::new(), "no copy of the app may start while it still runs"); + assert_eq!(std::fs::read_to_string(&exe).unwrap(), "0.1.0"); + assert!(!zip.exists(), "the download is removed"); + assert_eq!(take_failure(&paths).as_deref(), Some(error.as_str())); + } + + /// A step that fails after the old app exited starts the old app again, as it was. + #[cfg(windows)] + #[test] + fn the_helper_starts_the_old_app_after_a_failed_step() { + let dir = tempfile::tempdir().unwrap(); + let paths = Paths::under(dir.path()); + let exe = dir.path().join("artcraft-launcher.exe"); + std::fs::write(&exe, "0.1.0").unwrap(); + // Not a zip, so the step fails. + let zip = dir.path().join("artcraft-launcher-0.2.0-windows-x64-portable.zip"); + std::fs::write(&zip, "not a zip").unwrap(); + let step = Finish::Portable { zip, tmp: dir.path().join(PORTABLE_STAGING) }; + let started = std::sync::Mutex::new(Vec::::new()); + let start = |exe: &Path| { + started.lock().unwrap().push(exe.to_path_buf()); + Ok(()) + }; + let exited = |_pid| true; + assert!(finish_step(&crate::suite::LAUNCHER, &system_program("cmd.exe"), &paths, 4242, exe.clone(), &step, false, &exited, &start).is_err()); + assert_eq!(*started.lock().unwrap(), vec![exe.clone()]); + assert_eq!(std::fs::read_to_string(&exe).unwrap(), "0.1.0"); + } + + /// A successful step starts the new app once. + #[cfg(windows)] + #[test] + fn the_helper_starts_the_new_app_after_the_update() { + use std::io::Write; + let dir = tempfile::tempdir().unwrap(); + let paths = Paths::under(dir.path()); + let folder = dir.path().join("ArtCraft Launcher"); + std::fs::create_dir_all(&folder).unwrap(); + let exe = folder.join("artcraft-launcher.exe"); + std::fs::write(&exe, "0.1.0").unwrap(); + let zip = dir.path().join("artcraft-launcher-0.2.0-windows-x64-portable.zip"); + let mut writer = zip::ZipWriter::new(std::fs::File::create(&zip).unwrap()); + let signed_exe = std::fs::read(system_program("cmd.exe")).unwrap(); + writer.start_file("artcraft-launcher-0.2.0-windows-x64-portable/artcraft-launcher.exe", zip::write::SimpleFileOptions::default()).unwrap(); + writer.write_all(&signed_exe).unwrap(); + writer.finish().unwrap(); + let step = Finish::Portable { zip, tmp: dir.path().join(PORTABLE_STAGING) }; + let started = std::sync::Mutex::new(Vec::<(PathBuf, Vec)>::new()); + let start = |exe: &Path| { + started.lock().unwrap().push((exe.to_path_buf(), std::fs::read(exe).unwrap())); + Ok(()) + }; + let exited = |_pid| true; + assert_eq!( + finish_step(&crate::suite::LAUNCHER, &system_program("cmd.exe"), &paths, 4242, exe.clone(), &step, false, &exited, &start), + Ok(()) + ); + assert_eq!(*started.lock().unwrap(), vec![(exe, signed_exe)], "the new app starts once"); + assert_eq!(take_failure(&paths), None); + } + + /// A prepared Windows restart does nothing until it is started, and runs once when it is. + #[test] + fn a_prepared_restart_runs_only_when_started() { + use std::sync::Arc; + use std::sync::atomic::{AtomicUsize, Ordering}; + let runs = Arc::new(AtomicUsize::new(0)); + let counter = Arc::clone(&runs); + let restart = Restart::later(move || { + counter.fetch_add(1, Ordering::SeqCst); + Ok(()) + }); + assert_eq!(runs.load(Ordering::SeqCst), 0, "nothing starts before the app closes"); + assert_eq!(restart.start(), Ok(())); + assert_eq!(runs.load(Ordering::SeqCst), 1); + assert_eq!(Restart::scheduled().start(), Ok(())); } #[test] diff --git a/crates/update/src/updater.rs b/crates/update/src/updater.rs index 9211f257..b3c9548f 100644 --- a/crates/update/src/updater.rs +++ b/crates/update/src/updater.rs @@ -15,6 +15,7 @@ use std::time::Duration; use crate::dialog::{StartupPrompt, StartupPromptResponse}; use crate::pins::{Choice, Pins}; use crate::release::Release; +use crate::self_update::Restart; use crate::translate::{Note, Translate, english}; use crate::{App, Http, Paths, Platform, Stop, check_for_update, self_update}; @@ -42,7 +43,8 @@ pub enum Action { /// Open this page in the browser. OpenUrl(String), /// Close the app through its normal close path (which asks about unsaved work): the update is - /// installed and a helper restarts the app once it has exited. + /// installed and a helper restarts the app once it has exited. On Windows the helper, which + /// installs the update, starts when this is returned. Close, } @@ -89,9 +91,11 @@ pub struct Updater { requested: bool, started: bool, check: Option>, - install: Option<(Receiver>, Release)>, + install: Option<(Receiver>, Release)>, prompt: Option, - restart: bool, + /// A finished installation waiting for the app to close: what still has to start then, and + /// the release, to offer again if it can't. + restart: Option<(Restart, Release)>, /// An update is installed and waits for the app to exit; nothing starts a second one. installed: bool, /// The version-choice file to read in place of [`Pins::default_path`] (tests). @@ -116,7 +120,7 @@ impl Updater { check: None, install: None, prompt: None, - restart: false, + restart: None, installed: false, pins_path: None, managed: None, @@ -308,9 +312,9 @@ impl Updater { if let Some(outcome) = outcome { let release = self.install.take().map(|(_, release)| release); match outcome { - Ok(()) => { + Ok(restart) => { self.installed = true; - self.restart = true; + self.restart = release.map(|release| (restart, release)); let note = self.installed_note(); if let Some(prompt) = &mut self.prompt { prompt.is_installing = false; @@ -351,10 +355,24 @@ impl Updater { /// and a close that the user then cancels would leave it waiting for an exit that never comes. pub fn show_if(&mut self, ctx: &egui::Context, can_start: bool) -> Action { // The close waits for the app to hold no unsaved work: the user may have edited while the - // update installed, and a close they cancel is asked again once the work is saved. - if self.restart && can_start { - self.restart = false; - return Action::Close; + // update installed. The Windows helper starts only now, since it waits just a minute for + // the app to exit. + if can_start && let Some((restart, release)) = self.restart.take() { + match restart.start() { + Ok(()) => return Action::Close, + Err(e) => { + log::warn!("{} self-update failed: {e}", self.app.name); + // Nothing will install it: the update is on offer again. + self.installed = false; + if self.prompt.is_none() { + self.prompt = Some(self.prompt_for(release)); + } + if let Some(prompt) = &mut self.prompt { + prompt.is_installing = false; + prompt.set_message(e); + } + } + } } let Some(prompt) = &mut self.prompt else { return Action::None }; match prompt.show(ctx) { @@ -421,7 +439,7 @@ impl Updater { } } -fn install(app: &'static App, installed: &self_update::Install, mut release: Release, version: &str) -> Result<(), String> { +fn install(app: &'static App, installed: &self_update::Install, mut release: Release, version: &str) -> Result { let http = Http::new(&format!("{}/Update", app.name)).map_err(|e| format!("could not initialize HTTP: {e}"))?; if release.assets.is_empty() && let Some(full) = check_for_update(app, &http, version).map_err(|e| format!("could not fetch release assets: {e}"))? @@ -430,7 +448,7 @@ fn install(app: &'static App, installed: &self_update::Install, mut release: Rel } let paths = Paths::for_app(app).map_err(|e| format!("could not find paths: {e}"))?; let platform = Platform::current().ok_or("unsupported platform for self update")?; - self_update::update(app, installed, &release, platform, &http, &paths, &Stop::default(), &mut |_| {}, &|| Ok(()), Some(version)) + self_update::prepare_update(app, installed, &release, platform, &http, &paths, &Stop::default(), &mut |_| {}, &|| Ok(()), Some(version)) } /// Download in the prompt: the release page, when it is a safe web link (an app may build its @@ -642,7 +660,7 @@ mod tests { fn a_dropped_install_worker_is_an_error() { let ctx = egui::Context::default(); let mut u = updater(Ok(None)); - let (tx, rx) = channel::>(); + let (tx, rx) = channel::>(); u.install = Some((rx, newer())); drop(tx); u.tick(&ctx); @@ -701,7 +719,7 @@ mod tests { let mut u = updater(Ok(Some(newer()))); let (tx, rx) = channel(); u.install = Some((rx, newer())); - assert!(tx.send(Ok(())).is_ok()); + assert!(tx.send(Ok(Restart::scheduled())).is_ok()); u.tick(&ctx); assert_eq!(u.show(&ctx), Action::Close); u.prompt = None; @@ -742,7 +760,7 @@ mod tests { // An installed update names the app, in the language of the moment. let (tx, rx) = channel(); u.install = Some((rx, newer())); - assert!(tx.send(Ok(())).is_ok()); + assert!(tx.send(Ok(Restart::scheduled())).is_ok()); u.tick(&ctx); assert!(message(&u).starts_with("FR The update is installed. Close PhotoCraft"), "{}", message(&u)); FRENCH.store(false, Ordering::Relaxed); @@ -763,7 +781,7 @@ mod tests { u.offer(newer()); let (tx, rx) = channel(); u.install = Some((rx, newer())); - assert!(tx.send(Ok(())).is_ok()); + assert!(tx.send(Ok(Restart::scheduled())).is_ok()); u.tick(&ctx); assert_eq!(frame(&mut u, &ctx, false), Action::None, "unsaved work holds the close back"); assert_eq!(frame(&mut u, &ctx, true), Action::Close); @@ -776,11 +794,53 @@ mod tests { let mut u = updater(Ok(None)); let (tx, rx) = channel(); u.install = Some((rx, newer())); - assert!(tx.send(Ok(())).is_ok()); + assert!(tx.send(Ok(Restart::scheduled())).is_ok()); u.tick(&ctx); assert_eq!(u.show(&ctx), Action::Close); assert_eq!(u.show(&ctx), Action::None); } + + /// A finished installation that still has to start its helper starts it only when the app + /// may close, and only once. + #[test] + fn the_helper_starts_once_when_the_app_closes() { + use std::sync::atomic::{AtomicUsize, Ordering}; + let ctx = egui::Context::default(); + let mut u = updater(Ok(None)); + u.offer(newer()); + let starts = Arc::new(AtomicUsize::new(0)); + let counter = Arc::clone(&starts); + let (tx, rx) = channel(); + u.install = Some((rx, newer())); + let restart = Restart::later(move || { + counter.fetch_add(1, Ordering::SeqCst); + Ok(()) + }); + assert!(tx.send(Ok(restart)).is_ok()); + u.tick(&ctx); + assert_eq!(starts.load(Ordering::SeqCst), 0, "the helper waits for the close"); + assert_eq!(frame(&mut u, &ctx, false), Action::None); + assert_eq!(starts.load(Ordering::SeqCst), 0, "unsaved work holds the helper back"); + assert_eq!(frame(&mut u, &ctx, true), Action::Close); + assert_eq!(starts.load(Ordering::SeqCst), 1); + assert_eq!(frame(&mut u, &ctx, true), Action::None); + assert_eq!(starts.load(Ordering::SeqCst), 1, "the helper starts once"); + } + + /// A helper that can't start keeps the app open and offers the update again with the reason. + #[test] + fn a_helper_that_cannot_start_keeps_the_app_open() { + let ctx = egui::Context::default(); + let mut u = updater(Ok(None)); + let (tx, rx) = channel(); + u.install = Some((rx, newer())); + assert!(tx.send(Ok(Restart::later(|| Err("couldn't schedule the update (denied)".into())))).is_ok()); + u.tick(&ctx); + assert_eq!(frame(&mut u, &ctx, true), Action::None); + let prompt = u.prompt.as_ref().map(|p| (p.is_installing, p.status_message.clone())); + assert_eq!(prompt, Some((false, Some("couldn't schedule the update (denied)".into())))); + assert!(!u.installed, "the update is on offer again"); + } } #[cfg(test)] From df66c35b07871335ee0f6927d38243be206e91e3 Mon Sep 17 00:00:00 2001 From: dotCooCoo Date: Sat, 7 Nov 2026 16:19:03 -0800 Subject: [PATCH 02/25] craft-update: keep the app running when a helper thread cannot start spawn_detached, run_command and open_url_with_fallback started their reaper, pipe-reader and opener threads with std::thread::spawn, which panics when the thread cannot be created. The Windows update helper now starts from the UI thread when the app closes, so such a panic would end the app. Each thread now starts through std::thread::Builder: a process that has already started keeps running and the failed wait is logged, run_command stops the helper and returns an error, and a failed opener thread is logged. --- crates/update/src/process.rs | 32 +++++++++++++++++++++++++------- 1 file changed, 25 insertions(+), 7 deletions(-) diff --git a/crates/update/src/process.rs b/crates/update/src/process.rs index 1074cec4..5aa6d42e 100644 --- a/crates/update/src/process.rs +++ b/crates/update/src/process.rs @@ -62,7 +62,15 @@ pub fn run_command(mut cmd: Command, program: &str, timeout: Duration) -> Result let mut child = cmd.stdin(Stdio::null()).stdout(Stdio::piped()).stderr(Stdio::piped()).spawn().map_err(|e| format!("couldn't start {program} ({e})"))?; // Read both pipes while it runs, so a chatty helper can't block on a full one. - let (stdout, stderr) = (read_all(child.stdout.take()), read_all(child.stderr.take())); + let readers = read_all(child.stdout.take()).and_then(|stdout| Ok((stdout, read_all(child.stderr.take())?))); + let (stdout, stderr) = match readers { + Ok(readers) => readers, + Err(e) => { + let _ = child.kill(); + let _ = child.wait(); + return Err(format!("couldn't read the output of {program} ({e})")); + } + }; let start = Instant::now(); let status = loop { match child.try_wait() { @@ -93,8 +101,8 @@ pub fn run_command(mut cmd: Command, program: &str, timeout: Duration) -> Result } /// Captures at most 8 MB of a child's pipe and drains the rest on its own thread. -fn read_all(pipe: Option) -> std::thread::JoinHandle> { - std::thread::spawn(move || { +fn read_all(pipe: Option) -> std::io::Result>> { + std::thread::Builder::new().name("helper-output".into()).spawn(move || { let mut out = Vec::new(); if let Some(mut pipe) = pipe { let _ = std::io::Read::read_to_end(&mut std::io::Read::take(&mut pipe, 8 << 20), &mut out); @@ -106,12 +114,16 @@ fn read_all(pipe: Option) -> std::thread:: }) } -/// Starts `cmd` without waiting for it, and reaps it when it exits. +/// Starts `cmd` without waiting for it, and reaps it when it exits. The process keeps running +/// when no thread can be started to reap it; only the wait is skipped. pub fn spawn_detached(mut cmd: Command) -> Result<(), String> { let mut child = cmd.stdin(Stdio::null()).stdout(Stdio::null()).stderr(Stdio::null()).spawn().map_err(|e| format!("couldn't start it ({e})"))?; - std::thread::spawn(move || { + let reaper = std::thread::Builder::new().name("reap-child".into()).spawn(move || { let _ = child.wait(); }); + if let Err(e) = reaper { + log::warn!("couldn't start a thread to wait for a started process ({e})"); + } Ok(()) } @@ -224,12 +236,15 @@ fn opener(target: &std::ffi::OsStr) -> Result { pub fn open_url_with_fallback(primary_url: &str, fallback_url: &str) { let primary = primary_url.to_string(); let fallback = fallback_url.to_string(); - std::thread::spawn(move || { + let opener = std::thread::Builder::new().name("open-url".into()).spawn(move || { if !open_and_wait_success(&primary, Duration::from_millis(1500)) { log::warn!("couldn't open {primary}; falling back to {fallback}"); let _ = open(std::ffi::OsStr::new(&fallback)); } }); + if let Err(e) = opener { + log::warn!("couldn't start a thread to open {primary_url} ({e})"); + } } /// Starts the default system opener for `target` and returns `true` unless it fails to start @@ -262,9 +277,12 @@ fn open_and_wait_success(target: &str, timeout: Duration) -> bool { Ok(Some(status)) => return status.success(), Ok(None) if start.elapsed() < timeout => std::thread::sleep(Duration::from_millis(25)), Ok(None) => { - std::thread::spawn(move || { + let reaper = std::thread::Builder::new().name("reap-child".into()).spawn(move || { let _ = child.wait(); }); + if let Err(e) = reaper { + log::warn!("couldn't start a thread to wait for the opener of {target} ({e})"); + } return true; } Err(_) => return false, From f5f45b090c5feaa86a57b4c21153416ff174d9ea Mon Sep 17 00:00:00 2001 From: dotCooCoo Date: Sat, 7 Nov 2026 16:23:44 -0800 Subject: [PATCH 03/25] craft-update: start a waiting update helper when the app exits without Close The Windows helper of a finished installation started only when show_if returned Action::Close. An app that exited another way (the window's close button, a quit while unsaved work held the close back, an OS shutdown) dropped it, so the update never installed and the download stayed in the cache. Dropping the Updater now starts a helper that is still waiting, once, including one from an installation that finished after the last tick. Updater::start_pending_update does the same for an app that ends the process in on_exit without dropping the Updater; Action::Close's doc comment names it. --- crates/update/src/updater.rs | 90 +++++++++++++++++++++++++++++++++++- 1 file changed, 89 insertions(+), 1 deletion(-) diff --git a/crates/update/src/updater.rs b/crates/update/src/updater.rs index b3c9548f..7096805e 100644 --- a/crates/update/src/updater.rs +++ b/crates/update/src/updater.rs @@ -44,7 +44,10 @@ pub enum Action { OpenUrl(String), /// Close the app through its normal close path (which asks about unsaved work): the update is /// installed and a helper restarts the app once it has exited. On Windows the helper, which - /// installs the update, starts when this is returned. + /// installs the update, starts when this is returned. When the app exits another way (the + /// window's close button, a quit with unsaved work, an OS shutdown), dropping the [`Updater`] + /// starts it. An app that ends the process in `on_exit` without dropping the `Updater` calls + /// [`Updater::start_pending_update`] there first. Close, } @@ -394,6 +397,26 @@ impl Updater { } } + /// Starts the helper of an installed update that is still waiting for the app to close, so + /// that the update installs once the app has exited. Call it when the app exits by a path + /// that did not come through [`Action::Close`]; dropping the `Updater` calls it too. Later + /// calls do nothing. An installation still running is left alone. + pub fn start_pending_update(&mut self) { + // An installation that finished since the last `tick`. + if let Some((rx, release)) = &self.install + && let Ok(Ok(restart)) = rx.try_recv() + { + self.restart = Some((restart, release.clone())); + self.install = None; + self.installed = true; + } + if let Some((restart, _)) = self.restart.take() + && let Err(e) = restart.start() + { + log::warn!("{} self-update failed: {e}", self.app.name); + } + } + fn request_install(&mut self, ctx: &egui::Context, can_start: bool) { if can_start { self.start_install(ctx); @@ -439,6 +462,14 @@ impl Updater { } } +impl Drop for Updater { + /// The app is exiting: an installed update that is still waiting starts its helper, which + /// waits for this process to exit before it installs. + fn drop(&mut self) { + self.start_pending_update(); + } +} + fn install(app: &'static App, installed: &self_update::Install, mut release: Release, version: &str) -> Result { let http = Http::new(&format!("{}/Update", app.name)).map_err(|e| format!("could not initialize HTTP: {e}"))?; if release.assets.is_empty() @@ -827,6 +858,63 @@ mod tests { assert_eq!(starts.load(Ordering::SeqCst), 1, "the helper starts once"); } + /// A counter and a helper that adds one to it each time it starts. + fn counted_restart() -> (Arc, Restart) { + let starts = Arc::new(std::sync::atomic::AtomicUsize::new(0)); + let counter = Arc::clone(&starts); + let restart = Restart::later(move || { + counter.fetch_add(1, std::sync::atomic::Ordering::SeqCst); + Ok(()) + }); + (starts, restart) + } + + /// An app that exits without [`Action::Close`] still starts the waiting helper, once, when + /// the updater is dropped. + #[test] + fn dropping_the_updater_starts_a_waiting_helper_once() { + use std::sync::atomic::Ordering; + let ctx = egui::Context::default(); + let mut u = updater(Ok(None)); + let (starts, restart) = counted_restart(); + let (tx, rx) = channel(); + u.install = Some((rx, newer())); + assert!(tx.send(Ok(restart)).is_ok()); + u.tick(&ctx); + assert_eq!(frame(&mut u, &ctx, false), Action::None, "unsaved work holds the close back"); + assert_eq!(starts.load(Ordering::SeqCst), 0); + drop(u); + assert_eq!(starts.load(Ordering::SeqCst), 1); + + // Picked up from the worker without a `tick` first. + let mut u = updater(Ok(None)); + let (starts, restart) = counted_restart(); + let (tx, rx) = channel(); + u.install = Some((rx, newer())); + assert!(tx.send(Ok(restart)).is_ok()); + u.start_pending_update(); + assert_eq!(starts.load(Ordering::SeqCst), 1); + u.start_pending_update(); + drop(u); + assert_eq!(starts.load(Ordering::SeqCst), 1, "the helper starts once"); + } + + /// A helper started at [`Action::Close`] does not start again when the updater is dropped. + #[test] + fn a_helper_started_at_close_does_not_start_again_on_drop() { + use std::sync::atomic::Ordering; + let ctx = egui::Context::default(); + let mut u = updater(Ok(None)); + let (starts, restart) = counted_restart(); + let (tx, rx) = channel(); + u.install = Some((rx, newer())); + assert!(tx.send(Ok(restart)).is_ok()); + u.tick(&ctx); + assert_eq!(frame(&mut u, &ctx, true), Action::Close); + drop(u); + assert_eq!(starts.load(Ordering::SeqCst), 1); + } + /// A helper that can't start keeps the app open and offers the update again with the reason. #[test] fn a_helper_that_cannot_start_keeps_the_app_open() { From 0e856027c5790396906a651dfaf5d042f6571628 Mon Sep 17 00:00:00 2001 From: dotCooCoo Date: Sat, 7 Nov 2026 16:27:51 -0800 Subject: [PATCH 04/25] craft-update: log an install failure picked up at exit and clear the finished job --- crates/update/src/updater.rs | 25 ++++++++++++++++++++++--- 1 file changed, 22 insertions(+), 3 deletions(-) diff --git a/crates/update/src/updater.rs b/crates/update/src/updater.rs index 7096805e..5b90061d 100644 --- a/crates/update/src/updater.rs +++ b/crates/update/src/updater.rs @@ -404,11 +404,17 @@ impl Updater { pub fn start_pending_update(&mut self) { // An installation that finished since the last `tick`. if let Some((rx, release)) = &self.install - && let Ok(Ok(restart)) = rx.try_recv() + && let Ok(outcome) = rx.try_recv() { - self.restart = Some((restart, release.clone())); + match outcome { + Ok(restart) => { + self.restart = Some((restart, release.clone())); + self.installed = true; + } + Err(Failure::Installer(e)) => log::warn!("{} self-update failed: {e}", self.app.name), + Err(Failure::Updater(note)) => log::warn!("{} self-update failed: {}", self.app.name, note.translated(english)), + } self.install = None; - self.installed = true; } if let Some((restart, _)) = self.restart.take() && let Err(e) = restart.start() @@ -899,6 +905,19 @@ mod tests { assert_eq!(starts.load(Ordering::SeqCst), 1, "the helper starts once"); } + /// A failed installation picked up at exit leaves nothing installed and nothing waiting. + #[test] + fn a_failed_install_picked_up_at_exit_leaves_nothing_waiting() { + let mut u = updater(Ok(None)); + let (tx, rx) = channel::>(); + u.install = Some((rx, newer())); + assert!(tx.send(Err("couldn't download the update".into())).is_ok()); + u.start_pending_update(); + assert!(u.install.is_none(), "the finished job is cleared"); + assert!(!u.installed); + assert!(u.restart.is_none()); + } + /// A helper started at [`Action::Close`] does not start again when the updater is dropped. #[test] fn a_helper_started_at_close_does_not_start_again_on_drop() { From da46f3ca20fc9dc02f45844e3e6cb0426841173a Mon Sep 17 00:00:00 2001 From: dotCooCoo Date: Sat, 7 Nov 2026 16:34:01 -0800 Subject: [PATCH 05/25] craft-update: an install that finishes after the updater is dropped starts its helper --- crates/update/src/updater.rs | 61 +++++++++++++++++++++++++++++++++--- 1 file changed, 57 insertions(+), 4 deletions(-) diff --git a/crates/update/src/updater.rs b/crates/update/src/updater.rs index 5b90061d..0368cb86 100644 --- a/crates/update/src/updater.rs +++ b/crates/update/src/updater.rs @@ -8,8 +8,8 @@ //! [`Updater::managed`] says why. use std::path::PathBuf; -use std::sync::Arc; -use std::sync::mpsc::{Receiver, TryRecvError, channel}; +use std::sync::mpsc::{Receiver, SendError, Sender, TryRecvError, channel}; +use std::sync::{Arc, Mutex, PoisonError}; use std::time::Duration; use crate::dialog::{StartupPrompt, StartupPromptResponse}; @@ -101,6 +101,9 @@ pub struct Updater { restart: Option<(Restart, Release)>, /// An update is installed and waits for the app to exit; nothing starts a second one. installed: bool, + /// Set when the `Updater` is dropped. An install worker sends its outcome while holding it, + /// and starts a finished installation's helper itself once it is set. + dropped: Arc>, /// The version-choice file to read in place of [`Pins::default_path`] (tests). pins_path: Option, /// The launcher's choice for this app when it is not stable, as of the last check or offer. @@ -125,6 +128,7 @@ impl Updater { prompt: None, restart: None, installed: false, + dropped: Arc::new(Mutex::new(false)), pins_path: None, managed: None, translate: english, @@ -400,7 +404,8 @@ impl Updater { /// Starts the helper of an installed update that is still waiting for the app to close, so /// that the update installs once the app has exited. Call it when the app exits by a path /// that did not come through [`Action::Close`]; dropping the `Updater` calls it too. Later - /// calls do nothing. An installation still running is left alone. + /// calls do nothing. An installation still running when the `Updater` is dropped starts its + /// helper itself when it finishes. pub fn start_pending_update(&mut self) { // An installation that finished since the last `tick`. if let Some((rx, release)) = &self.install @@ -452,10 +457,11 @@ impl Updater { let (tx, rx) = channel(); let (app, version, ctx, release) = (self.app, self.version.clone(), ctx.clone(), prompt.release.clone()); let worker_release = release.clone(); + let dropped = Arc::clone(&self.dropped); let spawned = std::thread::Builder::new().name("self-update".into()).spawn(move || { let run = || install(app, &installed, worker_release, &version).map_err(Failure::Installer); let outcome = std::panic::catch_unwind(std::panic::AssertUnwindSafe(run)).unwrap_or_else(|_| Err(Failure::panicked())); - let _ = tx.send(outcome); + deliver(app, &tx, &dropped, outcome); ctx.request_repaint(); }); match spawned { @@ -472,10 +478,29 @@ impl Drop for Updater { /// The app is exiting: an installed update that is still waiting starts its helper, which /// waits for this process to exit before it installs. fn drop(&mut self) { + // Held while the channel is read: a worker sent its outcome before, and it is read here, + // or it sends after and starts its helper itself. + let dropped = Arc::clone(&self.dropped); + let mut dropped = dropped.lock().unwrap_or_else(PoisonError::into_inner); + *dropped = true; self.start_pending_update(); } } +/// Sends the install worker's outcome to the [`Updater`]. When the `Updater` has been dropped +/// (the app is exiting), the worker starts a finished installation's helper itself, since +/// nothing will start it at a close. +fn deliver(app: &App, tx: &Sender>, dropped: &Mutex, outcome: Result) { + let gone = dropped.lock().unwrap_or_else(PoisonError::into_inner); + let unsent = if *gone { Some(outcome) } else { tx.send(outcome).err().map(|SendError(outcome)| outcome) }; + drop(gone); + if let Some(Ok(restart)) = unsent + && let Err(e) = restart.start() + { + log::warn!("{} self-update failed: {e}", app.name); + } +} + fn install(app: &'static App, installed: &self_update::Install, mut release: Release, version: &str) -> Result { let http = Http::new(&format!("{}/Update", app.name)).map_err(|e| format!("could not initialize HTTP: {e}"))?; if release.assets.is_empty() @@ -905,6 +930,34 @@ mod tests { assert_eq!(starts.load(Ordering::SeqCst), 1, "the helper starts once"); } + /// An installation that finishes after the updater was dropped starts its helper once; one + /// the updater receives waits for the close. + #[test] + fn an_install_finishing_after_the_updater_is_dropped_starts_its_helper() { + use std::sync::atomic::Ordering; + let mut u = updater(Ok(None)); + let dropped = Arc::clone(&u.dropped); + let (starts, restart) = counted_restart(); + let (tx, rx) = channel(); + u.install = Some((rx, newer())); + drop(u); + assert_eq!(starts.load(Ordering::SeqCst), 0); + deliver(&crate::suite::LAUNCHER, &tx, &dropped, Ok(restart)); + assert_eq!(starts.load(Ordering::SeqCst), 1); + + // Sent while the drop has read the channel but not yet released the receiver. + let (starts, restart) = counted_restart(); + let (tx, _rx) = channel(); + deliver(&crate::suite::LAUNCHER, &tx, &Mutex::new(true), Ok(restart)); + assert_eq!(starts.load(Ordering::SeqCst), 1, "a receiver about to be dropped is not sent to"); + + let (starts, restart) = counted_restart(); + let (tx, rx) = channel(); + deliver(&crate::suite::LAUNCHER, &tx, &Mutex::new(false), Ok(restart)); + assert_eq!(starts.load(Ordering::SeqCst), 0, "the updater starts it at the close"); + assert!(rx.try_recv().is_ok_and(|outcome| outcome.is_ok())); + } + /// A failed installation picked up at exit leaves nothing installed and nothing waiting. #[test] fn a_failed_install_picked_up_at_exit_leaves_nothing_waiting() { From cbd9abfdc30e5e531bb8748f27131525dd275fd5 Mon Sep 17 00:00:00 2001 From: dotCooCoo Date: Sat, 7 Nov 2026 17:11:33 -0800 Subject: [PATCH 06/25] craft-update: say that the drop starts the helper only when the app drops the Updater --- crates/update/src/updater.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/crates/update/src/updater.rs b/crates/update/src/updater.rs index 0368cb86..c31c8f24 100644 --- a/crates/update/src/updater.rs +++ b/crates/update/src/updater.rs @@ -44,9 +44,9 @@ pub enum Action { OpenUrl(String), /// Close the app through its normal close path (which asks about unsaved work): the update is /// installed and a helper restarts the app once it has exited. On Windows the helper, which - /// installs the update, starts when this is returned. When the app exits another way (the - /// window's close button, a quit with unsaved work, an OS shutdown), dropping the [`Updater`] - /// starts it. An app that ends the process in `on_exit` without dropping the `Updater` calls + /// installs the update, starts when this is returned. When the app exits another way, such as + /// the window's close button or a quit with unsaved work, and drops the [`Updater`] as it exits, + /// the drop starts it. An app that ends the process in `on_exit` without dropping the `Updater` calls /// [`Updater::start_pending_update`] there first. Close, } From 878d4e712c33d0a4fc9a7780516257320e0c7a13 Mon Sep 17 00:00:00 2001 From: dotCooCoo Date: Sat, 7 Nov 2026 17:31:45 -0800 Subject: [PATCH 07/25] craft-update: tell the user what happened to an update MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A requested check while an update downloads or waits for the app to close now reports Installing or Ready instead of leaving the result window blank, and an app the launcher manages reports Managed instead of calling its version the newest. On Windows a prepared update says "Update ready. Close {app} to install it." rather than that it is installed. The helper records the version it installed just before it starts the new app, and the first start of that version reports Updated. A recorded helper failure is reported once at the next start (InstallFailed, with Check Again), in the prompt when the update is offered again, and to a requested check while the old app still runs. A failure wins over a stale record of a finished update, and a new update clears both records. Records are read up to 4 KiB. Action::Close is now a request: the Windows helper starts only at the exit (dropping the Updater or start_pending_update in on_exit). A close the user cancels keeps the update ready, and Restart to Update in the prompt or the result window (Updater::restart_to_update) asks for the close again. A helper that cannot start at the exit is recorded and reported at the next start. New strings for app catalogs: "Installing update…", "Update ready. Close {app} to install it.", "Restart to Update", "The update could not be installed: {reason}", "{app} updated to {version}." and "{app} {version} follows the version chosen in the launcher, so it does not update itself." --- crates/update/src/dialog.rs | 126 +++++-- crates/update/src/self_update.rs | 208 ++++++++++- crates/update/src/updater.rs | 462 +++++++++++++++++++++---- crates/update/src/updater_pin_tests.rs | 8 +- 4 files changed, 701 insertions(+), 103 deletions(-) diff --git a/crates/update/src/dialog.rs b/crates/update/src/dialog.rs index 1b1759d2..5f471d47 100644 --- a/crates/update/src/dialog.rs +++ b/crates/update/src/dialog.rs @@ -35,6 +35,9 @@ pub struct StartupPrompt { /// [`StartupPrompt::set_note`] and [`StartupPrompt::set_message`] keep only one. pub status_note: Option, pub is_installing: bool, + /// The update is ready and waits for the app to close: the main button reads "Restart to + /// Update" and answers [`StartupPromptResponse::UpdateAndRestart`]. + pub is_ready: bool, /// Puts the card's words in the interface language ([`crate::translate`]); English by default. pub translate: Translate, } @@ -52,6 +55,7 @@ impl StartupPrompt { status_message: None, status_note: None, is_installing: false, + is_ready: false, translate: english, } } @@ -190,13 +194,13 @@ impl StartupPrompt { response = StartupPromptResponse::Later; } } else { - if self.is_self_updating { - let update_btn = egui::Button::new( - RichText::new(button(t, "Update and Restart")).size(10.5).strong().monospace().color(on_fill), - ) - .fill(fill) - .corner_radius(egui::CornerRadius::ZERO) - .min_size(Vec2::new(140.0, 26.0)); + if self.is_self_updating || self.is_ready { + let label = if self.is_ready { "Restart to Update" } else { "Update and Restart" }; + let update_btn = + egui::Button::new(RichText::new(button(t, label)).size(10.5).strong().monospace().color(on_fill)) + .fill(fill) + .corner_radius(egui::CornerRadius::ZERO) + .min_size(Vec2::new(140.0, 26.0)); if ui.add(update_btn).clicked() { response = StartupPromptResponse::UpdateAndRestart; } @@ -352,18 +356,24 @@ pub enum CheckResultResponse { None, /// OK, the close button, Esc or a click outside: the app stops showing the dialog. Close, - /// Failed state only: the app starts another requested check. + /// Failed states only: the app starts another requested check. CheckAgain, + /// Ready state only: the app calls `Updater::restart_to_update`, which asks it to close. + RestartToUpdate, } /// The result of a manual "Check for Updates", in the same card as [`StartupPrompt`]: a spinner -/// while checking, "newest version" when up to date, the error and a retry when it failed. +/// while checking, "newest version" when up to date, the error and a retry when it failed, an +/// update installing or ready, and the launcher's choice when it manages the version. It also +/// shows what the last update came to at the first start after it. /// /// An app shows it while a requested check is in view: open it when the user starts a check -/// (`Updater::check_now`), call [`show`](Self::show) every frame with `updater.status()` until it -/// returns [`CheckResultResponse::Close`], then stop calling it. On -/// [`CheckResultResponse::CheckAgain`] start another requested check and keep showing it. An -/// available update goes to [`StartupPrompt`] instead; the app closes this dialog when the prompt opens. +/// (`Updater::check_now`) or when `updater.status()` is not [`Status::Idle`] after the check at +/// start, call [`show`](Self::show) every frame with `updater.status()` until it returns +/// [`CheckResultResponse::Close`], then call `Updater::dismiss_status` and stop calling it. On +/// [`CheckResultResponse::CheckAgain`] start another requested check and keep showing it; on +/// [`CheckResultResponse::RestartToUpdate`] call `Updater::restart_to_update`. An available update +/// goes to [`StartupPrompt`] instead; the app closes this dialog when the prompt opens. #[derive(Clone, Copy, Debug)] pub struct CheckResultDialog { app: &'static App, @@ -384,14 +394,15 @@ impl CheckResultDialog { self } - /// Draws the modal card for `status` and returns what the user chose. [`Status::Idle`] draws - /// nothing and returns [`CheckResultResponse::None`]. + /// Draws the card for `status` and returns what the user chose. [`Status::Idle`] draws + /// nothing and returns [`CheckResultResponse::None`]. The card is modal except for + /// [`Status::Updated`], a notice that leaves the app usable behind it. pub fn show(&self, ctx: &egui::Context, status: &Status) -> CheckResultResponse { if matches!(status, Status::Idle) { return CheckResultResponse::None; } let mut response = CheckResultResponse::None; - if backdrop(ctx) { + if !matches!(status, Status::Updated(_)) && backdrop(ctx) { response = CheckResultResponse::Close; } if ctx.input_mut(|i| i.consume_key(egui::Modifiers::NONE, egui::Key::Escape)) { @@ -417,6 +428,7 @@ impl CheckResultDialog { } ui.add_space(8.0); + let alert_color = Color32::from_rgb(225, 95, 30); match status { Status::Checking => { ui.horizontal(|ui| { @@ -424,6 +436,31 @@ impl CheckResultDialog { ui.label(RichText::new(t("Checking for updates…", &[])).size(12.5).color(text_primary)); }); } + Status::Installing => { + ui.horizontal(|ui| { + ui.spinner(); + ui.label(RichText::new(t("Installing update…", &[])).size(12.5).color(text_primary)); + }); + } + Status::Ready => { + let text = t(crate::updater::READY_TEXT, &[("app", self.app.name)]); + ui.label(RichText::new(text).size(12.5).color(text_primary)); + } + Status::Managed => { + let text = t( + "{app} {version} follows the version chosen in the launcher, so it does not update itself.", + &[("app", self.app.name), ("version", self.running_version)], + ); + ui.label(RichText::new(text).size(12.5).color(text_muted)); + } + Status::Updated(version) => { + let text = t("{app} updated to {version}.", &[("app", self.app.name), ("version", version)]); + ui.label(RichText::new(text).size(12.5).color(text_primary)); + } + Status::InstallFailed(reason) => { + let text = t("The update could not be installed: {reason}", &[("reason", reason)]); + ui.add(egui::Label::new(RichText::new(text).size(12.5).color(alert_color)).selectable(true)); + } Status::UpToDate => { let text = t( "{app} {version} is the newest version available.", @@ -432,7 +469,6 @@ impl CheckResultDialog { ui.label(RichText::new(text).size(12.5).color(text_muted)); } Status::Failed(error) => { - let alert_color = Color32::from_rgb(225, 95, 30); ui.label(RichText::new(t("Could not check for updates.", &[])).size(12.5).strong().color(alert_color)); ui.add(egui::Label::new(RichText::new(error).size(12.0).color(text_muted)).selectable(true)); ui.label( @@ -458,7 +494,7 @@ impl CheckResultDialog { if ui.add(ok).clicked() { response = CheckResultResponse::Close; } - if matches!(status, Status::Failed(_)) { + if matches!(status, Status::Failed(_) | Status::InstallFailed(_)) { let again = egui::Button::new(RichText::new(button(t, "Check Again")).size(10.5).monospace().color(text_primary)) .corner_radius(egui::CornerRadius::ZERO) .min_size(Vec2::new(100.0, 26.0)); @@ -466,6 +502,15 @@ impl CheckResultDialog { response = CheckResultResponse::CheckAgain; } } + if matches!(status, Status::Ready) { + let restart = + egui::Button::new(RichText::new(button(t, "Restart to Update")).size(10.5).monospace().color(text_primary)) + .corner_radius(egui::CornerRadius::ZERO) + .min_size(Vec2::new(130.0, 26.0)); + if ui.add(restart).clicked() { + response = CheckResultResponse::RestartToUpdate; + } + } }); } }); @@ -573,6 +618,7 @@ mod tests { status_message: None, status_note: None, is_installing: false, + is_ready: false, translate: english, } } @@ -1049,6 +1095,26 @@ mod tests { let (_, idle) = run(&ctx, &Status::Idle, Vec::new()); assert!(painted_text(&idle).is_empty()); + // An update under way, the launcher's choice, and what the last update came to. + let ready = crate::translate::fill(crate::updater::READY_TEXT, &[("app", "PhotoCraft")]); + let backdrop = Color32::from_black_alpha(150); + for (status, wants, modal) in [ + (Status::Installing, vec!["Installing update…", "OK"], true), + (Status::Ready, vec![ready.as_str(), "RESTART TO UPDATE", "OK"], true), + (Status::Managed, vec!["PhotoCraft 0.1.0 follows the version chosen in the launcher, so it does not update itself.", "OK"], true), + (Status::Updated("0.2.0".into()), vec!["PhotoCraft updated to 0.2.0.", "OK"], false), + (Status::InstallFailed("disk full".into()), vec!["The update could not be installed: disk full", "CHECK AGAIN", "OK"], true), + ] { + let (response, output) = run(&ctx, &status, Vec::new()); + let text = painted_text(&output); + for want in wants { + assert!(text.contains(want), "{status:?}: {want} missing in {text}"); + } + assert!(!text.contains("newest version"), "{status:?} is not called newest: {text}"); + assert_eq!(response, CheckResultResponse::None); + assert_eq!(rect_fills(&output).iter().any(|(_, fill)| *fill == backdrop), modal, "{status:?} modal"); + } + let escape = Event::Key { key: Key::Escape, physical_key: None, pressed: true, repeat: false, modifiers: Modifiers::NONE }; assert_eq!(run(&dark_context(), &Status::UpToDate, vec![escape]).0, CheckResultResponse::Close); } @@ -1070,19 +1136,20 @@ mod tests { #[test] fn the_prompt_draws_its_words_through_the_apps_translator() { - for (self_updating, installing) in [(false, false), (true, false), (true, true)] { + for (self_updating, installing, ready) in [(false, false, false), (true, false, false), (true, true, false), (true, false, true)] { let ctx = dark_context(); - let mut prompt = StartupPrompt { is_installing: installing, ..prompt_for(self_updating) }.with_translator(pseudo); + let mut prompt = StartupPrompt { is_installing: installing, is_ready: ready, ..prompt_for(self_updating) }.with_translator(pseudo); frame(&ctx, &mut prompt, Vec::new()); let (_, output) = frame(&ctx, &mut prompt, Vec::new()); let painted = painted_text(&output); assert_all_translated(&painted, &["P", "PhotoCraft"]); // Values reach the translation; buttons are looked up in title case and drawn in capitals. assert!(painted.contains("[0.1.0, 0.2.0 | v]") && painted.contains("[ArtCraft | by ]"), "{painted}"); - let button = match (self_updating, installing) { - (_, true) => "[ | DISMISS]", - (true, false) => "[ | UPDATE AND RESTART]", - (false, false) => "[ | DOWNLOAD]", + let button = match (self_updating, installing, ready) { + (_, true, _) => "[ | DISMISS]", + (_, false, true) => "[ | RESTART TO UPDATE]", + (true, false, false) => "[ | UPDATE AND RESTART]", + (false, false, false) => "[ | DOWNLOAD]", }; assert!(painted.contains(button) && painted.contains("[ | UPDATE]"), "{painted}"); } @@ -1091,7 +1158,16 @@ mod tests { #[test] fn the_result_dialog_draws_its_words_through_the_apps_translator() { let dialog = CheckResultDialog::new(&PHOTOCRAFT, "0.1.0").with_translator(pseudo); - for status in [Status::Checking, Status::UpToDate, Status::Failed("no network".into())] { + for status in [ + Status::Checking, + Status::UpToDate, + Status::Failed("no network".into()), + Status::Installing, + Status::Ready, + Status::Managed, + Status::Updated("0.2.0".into()), + Status::InstallFailed("no network".into()), + ] { let ctx = dark_context(); let mut output = None; for _ in 0..2 { diff --git a/crates/update/src/self_update.rs b/crates/update/src/self_update.rs index d9bde551..63d69eab 100644 --- a/crates/update/src/self_update.rs +++ b/crates/update/src/self_update.rs @@ -512,16 +512,91 @@ pub fn prepare_update( } } })(); - unmark_unless_scheduled(paths, scheduled) + let scheduled = unmark_unless_scheduled(paths, scheduled); + // On macOS and Linux the update is in place once it is scheduled. + #[cfg(not(windows))] + if scheduled.is_ok() { + record_done(paths, &release.tag); + } + scheduled } -/// Records `release` as just installed, so it isn't offered again for [`JUST_UPDATED_FOR`]. +/// Records `release` as just installed, so it isn't offered again for [`JUST_UPDATED_FOR`]. The +/// outcome records of an earlier update are removed: this update writes its own. fn mark_installed(paths: &Paths, release: &Release) { + let _ = std::fs::remove_file(failure_file(paths)); + let _ = std::fs::remove_file(done_file(paths)); if let Err(e) = crate::paths::write_atomic(&marker(paths), format!("{}\n{}\n", release.tag, crate::now()).as_bytes()) { log::warn!("couldn't record the self-update: {e}"); } } +/// Where a finished update records the version it installed, for the new version's first start. +fn done_file(paths: &Paths) -> PathBuf { + paths.data.join("self-update-done.txt") +} + +/// Records that the update to `tag` has been installed. +fn record_done(paths: &Paths, tag: &str) { + if let Err(e) = crate::paths::write_atomic(&done_file(paths), tag.as_bytes()) { + log::warn!("couldn't record the finished self-update: {e}"); + } +} + +/// The version the scheduled update installs, from the record [`mark_installed`] wrote. +#[cfg_attr(not(windows), allow(dead_code))] +fn scheduled_tag(paths: &Paths) -> Option { + read_record(&marker(paths)).and_then(|text| text.lines().next().map(str::to_owned)).filter(|tag| !tag.trim().is_empty()) +} + +/// The most of an outcome record that is read: a reason longer than this is cut short. +const RECORD_LIMIT: usize = 4096; + +/// The text of the record at `path`, at most [`RECORD_LIMIT`] bytes of it. `None` when it is +/// missing, unreadable or not a plain file. +fn read_record(path: &Path) -> Option { + use std::io::Read; + if !std::fs::metadata(path).is_ok_and(|m| m.is_file()) { + return None; + } + let mut bytes = Vec::new(); + std::fs::File::open(path).ok()?.take(RECORD_LIMIT as u64 + 1).read_to_end(&mut bytes).ok()?; + let cut = bytes.len() > RECORD_LIMIT; + bytes.truncate(RECORD_LIMIT); + let mut text = String::from_utf8_lossy(&bytes).into_owned(); + if cut { + // A multi-byte character split at the limit decodes as U+FFFD; drop it. + text = text.trim_end_matches('\u{FFFD}').to_owned(); + text.push('…'); + } + Some(text) +} + +/// What the last self-update came to, read once by the next start ([`take_outcome`]). +#[derive(Clone, Debug, PartialEq, Eq)] +pub enum Outcome { + /// The update installed this version, which is the one running now. + Installed(Version), + /// The update could not be installed, for this reason. + Failed(String), +} + +/// What the last self-update came to, once: the records are removed as they are read. A +/// recorded failure wins over a record of a finished update. A finished update counts only when +/// the version it installed is `running`, the version this copy counts as. +pub fn take_outcome(paths: &Paths, running: &str) -> Option { + let failure = take_failure(paths); + let done = read_record(&done_file(paths)); + let _ = std::fs::remove_file(done_file(paths)); + if let Some(reason) = failure { + let reason = reason.trim(); + let reason = if reason.is_empty() { "no reason was recorded" } else { reason }; + return Some(Outcome::Failed(reason.to_owned())); + } + let installed = version::parse(done?.trim())?; + (Some(&installed) == version::parse(running).as_ref()).then_some(Outcome::Installed(installed)) +} + /// Removes the record of [`mark_installed`] when the swap couldn't be scheduled, so the update /// stays on offer. fn unmark_unless_scheduled(paths: &Paths, scheduled: Result) -> Result { @@ -897,6 +972,12 @@ fn finish_step( if !restarts(stepped.exited, done.is_ok(), exe.is_file()) { return done; } + // Written before the new app starts, so its first start finds it. A failed start removes it. + if done.is_ok() + && let Some(tag) = scheduled_tag(paths) + { + record_done(paths, &tag); + } let started = start(&exe); if let Err(e) = &started { record_failure(paths, e); @@ -915,20 +996,25 @@ fn failure_file(paths: &Paths) -> PathBuf { paths.logs().join("self-update-error.txt") } -/// Records a failed update: the next check offers the update again, and the next start logs `error`. -#[cfg_attr(not(windows), allow(dead_code))] -fn record_failure(paths: &Paths, error: &str) { +/// Records a failed update: the next check offers the update again, and the next start shows +/// `error`. A record of the update as finished is removed. +#[cfg_attr(not(any(windows, feature = "egui")), allow(dead_code))] +pub(crate) fn record_failure(paths: &Paths, error: &str) { let _ = std::fs::remove_file(marker(paths)); + let _ = std::fs::remove_file(done_file(paths)); let _ = std::fs::create_dir_all(paths.logs()); let _ = crate::paths::write_atomic(&failure_file(paths), error.as_bytes()); } -/// The error of the last update, if it failed, once. +/// The error of the last update, if it failed, once. A long error is cut short at +/// [`RECORD_LIMIT`] bytes. pub fn take_failure(paths: &Paths) -> Option { let file = failure_file(paths); - let error = std::fs::read_to_string(&file).ok()?; - let _ = std::fs::remove_file(&file); - Some(error) + let error = read_record(&file); + if std::fs::metadata(&file).is_ok_and(|m| m.is_file()) { + let _ = std::fs::remove_file(&file); + } + error } /// Whether `exe` is named like this app's own executable (case-insensitive, any of its ids). @@ -1562,6 +1648,55 @@ mod tests { assert_eq!(take_failure(&paths), None); } + /// The outcome is read once; a failure wins over a finished update, which counts only for the + /// version it installed. + #[test] + fn the_outcome_of_an_update_is_read_once() { + let dir = tempfile::tempdir().unwrap(); + let paths = Paths::under(dir.path()); + assert_eq!(take_outcome(&paths, "0.2.0"), None); + record_done(&paths, "v0.2.0"); + assert_eq!(take_outcome(&paths, "0.2.0"), Some(Outcome::Installed(Version::new(0, 2, 0)))); + assert_eq!(take_outcome(&paths, "0.2.0"), None, "read once"); + + record_done(&paths, "v0.2.0"); + assert_eq!(take_outcome(&paths, "0.1.0"), None, "the old version still runs"); + assert!(!done_file(&paths).exists(), "a record that doesn't match is removed too"); + + record_done(&paths, "v0.2.0"); + record_failure(&paths, "the Windows Installer stopped with error 1603"); + assert!(!done_file(&paths).exists(), "a failure removes the record of a finished update"); + record_done(&paths, "v0.2.0"); + assert_eq!(take_outcome(&paths, "0.2.0"), Some(Outcome::Failed("the Windows Installer stopped with error 1603".into()))); + assert_eq!(take_outcome(&paths, "0.2.0"), None); + + // Damaged records: no version, an empty reason, a reason longer than the limit. + crate::paths::write_atomic(&done_file(&paths), b"\xff\xfe not a version").unwrap(); + assert_eq!(take_outcome(&paths, "0.2.0"), None); + record_failure(&paths, " \n"); + assert_eq!(take_outcome(&paths, "0.2.0"), Some(Outcome::Failed("no reason was recorded".into()))); + record_failure(&paths, &"é".repeat(RECORD_LIMIT)); + let Some(Outcome::Failed(reason)) = take_outcome(&paths, "0.2.0") else { panic!("a long reason is still a failure") }; + assert!(reason.len() <= RECORD_LIMIT + '…'.len_utf8() && reason.ends_with("é…"), "{}", reason.len()); + // A folder where a record goes is not read, and not removed. + std::fs::create_dir_all(failure_file(&paths)).unwrap(); + std::fs::create_dir_all(done_file(&paths)).unwrap(); + assert_eq!(take_outcome(&paths, "0.2.0"), None); + assert!(failure_file(&paths).is_dir()); + } + + /// Scheduling a new update clears the outcome records of an earlier one, so a stale failure + /// is not reported after it. + #[test] + fn a_new_update_clears_the_last_ones_outcome() { + let dir = tempfile::tempdir().unwrap(); + let paths = Paths::under(dir.path()); + record_failure(&paths, "the Windows Installer stopped with error 1603"); + mark_installed(&paths, &Release { tag: "v0.3.0".into(), ..Release::default() }); + assert_eq!(take_failure(&paths), None); + assert_eq!(scheduled_tag(&paths).as_deref(), Some("v0.3.0")); + } + /// A launcher that doesn't quit keeps its files, and the download is still removed. #[cfg(windows)] #[test] @@ -2176,12 +2311,65 @@ mod tests { Ok(()) }; let exited = |_pid| true; + mark_installed(&paths, &Release { tag: "v0.2.0".into(), ..Release::default() }); assert_eq!( finish_step(&crate::suite::LAUNCHER, &system_program("cmd.exe"), &paths, 4242, exe.clone(), &step, false, &exited, &start), Ok(()) ); assert_eq!(*started.lock().unwrap(), vec![(exe, signed_exe)], "the new app starts once"); - assert_eq!(take_failure(&paths), None); + assert_eq!(take_outcome(&paths, "0.2.0"), Some(Outcome::Installed(Version::new(0, 2, 0))), "the new version's first start says so"); + } + + /// A portable update of the launcher to 0.2.0 in `dir`: its exe and the step. + #[cfg(windows)] + fn portable_update(dir: &Path) -> (PathBuf, Finish) { + use std::io::Write; + let folder = dir.join("ArtCraft Launcher"); + std::fs::create_dir_all(&folder).unwrap(); + let exe = folder.join("artcraft-launcher.exe"); + std::fs::write(&exe, "0.1.0").unwrap(); + let zip = dir.join("artcraft-launcher-0.2.0-windows-x64-portable.zip"); + let mut writer = zip::ZipWriter::new(std::fs::File::create(&zip).unwrap()); + writer.start_file("artcraft-launcher-0.2.0-windows-x64-portable/artcraft-launcher.exe", zip::write::SimpleFileOptions::default()).unwrap(); + writer.write_all(&std::fs::read(system_program("cmd.exe")).unwrap()).unwrap(); + writer.finish().unwrap(); + (exe, Finish::Portable { zip, tmp: dir.join(PORTABLE_STAGING) }) + } + + /// A finished step whose new app can't be started is a failure at the next start, never a + /// finished update. + #[cfg(windows)] + #[test] + fn a_failed_relaunch_is_not_reported_as_a_finished_update() { + let dir = tempfile::tempdir().unwrap(); + let paths = Paths::under(dir.path()); + let (exe, step) = portable_update(dir.path()); + mark_installed(&paths, &Release { tag: "v0.2.0".into(), ..Release::default() }); + let start = |_: &Path| Err("couldn't start the launcher (denied)".to_owned()); + let exited = |_pid| true; + let result = finish_step(&crate::suite::LAUNCHER, &system_program("cmd.exe"), &paths, 4242, exe, &step, false, &exited, &start); + assert_eq!(result, Err("couldn't start the launcher (denied)".into())); + assert_eq!(take_outcome(&paths, "0.2.0"), Some(Outcome::Failed("couldn't start the launcher (denied)".into()))); + } + + /// A helper that times out or whose step fails records no finished update. + #[cfg(windows)] + #[test] + fn a_timed_out_or_failed_step_is_not_reported_as_a_finished_update() { + for (exits, zip_ok) in [(false, true), (true, false)] { + let dir = tempfile::tempdir().unwrap(); + let paths = Paths::under(dir.path()); + let (exe, step) = portable_update(dir.path()); + if !zip_ok && let Finish::Portable { zip, .. } = &step { + std::fs::write(zip, "not a zip").unwrap(); + } + mark_installed(&paths, &Release { tag: "v0.2.0".into(), ..Release::default() }); + let start = |_: &Path| Ok(()); + let wait = |_pid| exits; + assert!(finish_step(&crate::suite::LAUNCHER, &system_program("cmd.exe"), &paths, 4242, exe, &step, false, &wait, &start).is_err()); + assert!(!done_file(&paths).exists(), "exits {exits}, zip {zip_ok}"); + assert!(matches!(take_outcome(&paths, "0.2.0"), Some(Outcome::Failed(_))), "exits {exits}, zip {zip_ok}"); + } } /// A prepared Windows restart does nothing until it is started, and runs once when it is. diff --git a/crates/update/src/updater.rs b/crates/update/src/updater.rs index c31c8f24..a91fbcd6 100644 --- a/crates/update/src/updater.rs +++ b/crates/update/src/updater.rs @@ -4,8 +4,12 @@ //! handling of failures, dropped workers, repaints and quitting. //! //! An app the launcher keeps on another version (see [`crate::pins`]) offers nothing itself: the -//! check reads the shared choice first, and a requested check reports [`Status::UpToDate`] while -//! [`Updater::managed`] says why. +//! check reads the shared choice first, and a requested check reports [`Status::Managed`] while +//! [`Updater::managed`] says which choice it is. +//! +//! An update ends with the app closing. On Windows the update installs only after the app has +//! exited, so the updater says it is ready rather than installed, and the first start afterwards +//! reports what came of it: [`Status::Updated`] or [`Status::InstallFailed`]. use std::path::PathBuf; use std::sync::mpsc::{Receiver, SendError, Sender, TryRecvError, channel}; @@ -15,7 +19,7 @@ use std::time::Duration; use crate::dialog::{StartupPrompt, StartupPromptResponse}; use crate::pins::{Choice, Pins}; use crate::release::Release; -use crate::self_update::Restart; +use crate::self_update::{Outcome, Restart}; use crate::translate::{Note, Translate, english}; use crate::{App, Http, Paths, Platform, Stop, check_for_update, self_update}; @@ -23,7 +27,8 @@ use crate::{App, Http, Paths, Platform, Stop, check_for_update, self_update}; /// with one; without it the check asks GitHub. pub type CheckSource = Arc Result, String> + Send + Sync>; -/// What a requested check came to, for a window that answers Help ▸ Check for Updates. +/// What a requested check came to, for a window that answers Help ▸ Check for Updates, and what +/// the last update came to, at the first start after it. #[derive(Clone, Debug, PartialEq, Eq)] pub enum Status { /// Nothing to report. @@ -34,20 +39,39 @@ pub enum Status { UpToDate, /// A requested check failed. Failed(String), + /// A requested check found an update downloading and installing; the startup prompt shows + /// its progress. + Installing, + /// A requested check found an update ready and waiting for the app to close + /// ([`READY_TEXT`]). [`Updater::restart_to_update`] asks the app to close. + Ready, + /// A requested check found that the launcher keeps this app on a version of its choosing + /// ([`Updater::managed`]), so it does not update itself. + Managed, + /// This start follows an update that installed this version. + Updated(String), + /// The last update could not be installed, for this reason. + InstallFailed(String), } +/// What [`Status::Ready`] says: on Windows the update installs after the app exits; elsewhere it +/// is already in place and takes effect when the app restarts. +pub const READY_TEXT: &str = + if cfg!(windows) { "Update ready. Close {app} to install it." } else { "The update is installed. Close {app} to finish; it restarts by itself." }; + /// What the app should do after [`Updater::show`]. #[derive(Clone, Debug, PartialEq, Eq)] pub enum Action { None, /// Open this page in the browser. OpenUrl(String), - /// Close the app through its normal close path (which asks about unsaved work): the update is - /// installed and a helper restarts the app once it has exited. On Windows the helper, which - /// installs the update, starts when this is returned. When the app exits another way, such as - /// the window's close button or a quit with unsaved work, and drops the [`Updater`] as it exits, - /// the drop starts it. An app that ends the process in `on_exit` without dropping the `Updater` calls - /// [`Updater::start_pending_update`] there first. + /// Close the app through its normal close path (which asks about unsaved work): an update is + /// ready and a helper installs it and restarts the app once the app has exited. This is a + /// request: the user may cancel the close, and the update then stays ready, with + /// [`Status::Ready`] for a requested check and Restart to Update in the prompt. On Windows + /// the helper starts only at the exit: when the app drops the [`Updater`] as it exits, or + /// calls [`Updater::start_pending_update`] in `on_exit`, which an app that ends the process + /// there without dropping the `Updater` must do. Close, } @@ -96,11 +120,17 @@ pub struct Updater { check: Option>, install: Option<(Receiver>, Release)>, prompt: Option, - /// A finished installation waiting for the app to close: what still has to start then, and - /// the release, to offer again if it can't. + /// A finished installation waiting for the app to exit: what still has to start then, and + /// the release it installs. restart: Option<(Restart, Release)>, - /// An update is installed and waits for the app to exit; nothing starts a second one. + /// An update is ready and waits for the app to exit; nothing starts a second one. installed: bool, + /// [`Self::show_if`] returns [`Action::Close`] at its next frame that may close: once when an + /// installation finishes, and again for each [`Self::restart_to_update`]. + close_wanted: bool, + /// The folders that hold the outcome records ([`self_update::take_outcome`]) in place of the + /// app's own (tests). + paths: Option, /// Set when the `Updater` is dropped. An install worker sends its outcome while holding it, /// and starts a finished installation's helper itself once it is set. dropped: Arc>, @@ -128,6 +158,8 @@ impl Updater { prompt: None, restart: None, installed: false, + close_wanted: false, + paths: None, dropped: Arc::new(Mutex::new(false)), pins_path: None, managed: None, @@ -152,9 +184,9 @@ impl Updater { StartupPrompt::new(self.app, release, self.version.clone()).with_translator(self.translate) } - /// "The update is installed…", translated when the prompt draws. + /// [`READY_TEXT`], translated when the prompt draws. fn installed_note(&self) -> Note { - Note::new("The update is installed. Close {app} to finish; it restarts by itself.", &[("app", self.app.name)]) + Note::new(READY_TEXT, &[("app", self.app.name)]) } /// Replaces the network with `source` (tests). @@ -163,6 +195,33 @@ impl Updater { self } + /// Reads and writes the update's outcome records under `paths` instead of the app's own + /// folders (tests). + pub fn with_paths(mut self, paths: Paths) -> Self { + self.paths = Some(paths); + self + } + + /// The folders of the outcome records: [`Self::with_paths`]'s, else the app's own. Unit tests + /// never read or write the app's own records. + fn outcome_paths(&self) -> Option { + self.paths.clone().or_else(|| if cfg!(test) { None } else { Paths::for_app(self.app).ok() }) + } + + /// What the last update came to, once: [`Status::Updated`] when it installed the version + /// running now, [`Status::InstallFailed`] with the reason when it failed. + fn read_outcome(&mut self) { + let Some(paths) = self.outcome_paths() else { return }; + match self_update::take_outcome(&paths, &self.version) { + Some(Outcome::Installed(version)) => self.status = Status::Updated(version.to_string()), + Some(Outcome::Failed(reason)) => { + log::warn!("{} self-update failed: {reason}", self.app.name); + self.status = Status::InstallFailed(reason); + } + None => {} + } + } + /// Reads the version choices from `path` instead of the launcher's file (tests). pub fn with_pins_path(mut self, path: impl Into) -> Self { self.pins_path = Some(path.into()); @@ -194,20 +253,34 @@ impl Updater { self.managed.is_some() } - /// The check at start, once. Later calls do nothing. + /// The check at start, once. Later calls do nothing. It first reads what the last update came + /// to, once: [`Self::status`] is then [`Status::Updated`] or [`Status::InstallFailed`], and + /// the app shows its [`CheckResultDialog`](crate::dialog::CheckResultDialog) while the status + /// is not [`Status::Idle`]. The check itself stays quiet unless it finds a release. pub fn check_at_start(&mut self, ctx: &egui::Context) { if !std::mem::replace(&mut self.started, true) { + self.read_outcome(); self.begin(ctx, false); } } /// Help ▸ Check for Updates: a check whose outcome [`Self::status`] reports. A check already - /// running becomes a requested one; nothing starts while an update installs. + /// running becomes a requested one. While an update installs or waits for the app to close, + /// nothing starts and the status says so ([`Status::Installing`], [`Status::Ready`]). pub fn check_now(&mut self, ctx: &egui::Context) { self.started = true; self.begin(ctx, true); } + /// Asks the app to close so that a ready update installs: the next [`Self::show_if`] that may + /// close returns [`Action::Close`]. Restart to Update calls it after the user canceled an + /// earlier close. Does nothing when no update is ready. + pub fn restart_to_update(&mut self) { + if self.restart.is_some() { + self.close_wanted = true; + } + } + /// Opens the startup prompt for a release the app found itself. pub fn offer(&mut self, release: Release) { if self.install.is_none() && !self.installed && !self.read_choice() { @@ -233,6 +306,9 @@ impl Updater { fn begin(&mut self, ctx: &egui::Context, requested: bool) { if self.install.is_some() || self.installed { + if requested { + self.status = self.pending_status(); + } return; } if requested { @@ -265,28 +341,57 @@ impl Updater { } } + /// The answer to a requested check while an update installs or waits for the app to exit. A + /// helper that already started while the app kept running may have failed: its recorded + /// reason is the answer then, and the update is on offer again. + fn pending_status(&mut self) -> Status { + if self.install.is_some() { + return Status::Installing; + } + if self.restart.is_none() + && let Some(reason) = self.outcome_paths().and_then(|paths| self_update::take_failure(&paths)) + { + log::warn!("{} self-update failed: {reason}", self.app.name); + self.installed = false; + return Status::InstallFailed(reason.trim().to_owned()); + } + Status::Ready + } + + /// Sets the outcome of a check. The check at start leaves the status as it is: nothing, or + /// the outcome of the last update ([`Self::read_outcome`]). fn finish_check(&mut self, answer: Checked) { let requested = std::mem::take(&mut self.requested); // The launcher may have pinned or unpinned the app while the check ran: whatever the - // answer, an app it manages offers nothing and reports nothing to do. + // answer, an app it manages offers nothing and says so. if self.read_choice() { if let Err(e) = &answer { log::info!("{} update check failed ({e}), but the launcher manages its version", self.app.name); } - self.status = if requested { Status::UpToDate } else { Status::Idle }; + if requested { + self.status = Status::Managed; + } return; } match answer { Ok(Some(release)) => { - self.status = Status::Idle; if self.prompt.is_none() && self.install.is_none() && !self.installed { - self.prompt = Some(self.prompt_for(release)); + let mut prompt = self.prompt_for(release); + // A failed update offered again says why the last attempt failed. + if let Status::InstallFailed(reason) = &self.status { + prompt.set_note(Note::new("The update could not be installed: {reason}", &[("reason", reason)])); + } + self.prompt = Some(prompt); } + self.status = Status::Idle; } - Ok(None) => self.status = if requested { Status::UpToDate } else { Status::Idle }, + Ok(None) if requested => self.status = Status::UpToDate, + Ok(None) => {} Err(e) => { log::warn!("{} update check failed: {e}", self.app.name); - self.status = if requested { Status::Failed(e) } else { Status::Idle }; + if requested { + self.status = Status::Failed(e); + } } } } @@ -322,17 +427,26 @@ impl Updater { Ok(restart) => { self.installed = true; self.restart = release.map(|release| (restart, release)); + self.close_wanted = true; let note = self.installed_note(); if let Some(prompt) = &mut self.prompt { prompt.is_installing = false; + prompt.is_ready = true; prompt.set_note(note); } + if matches!(self.status, Status::Installing) { + self.status = Status::Ready; + } } Err(failure) => { match &failure { Failure::Installer(e) => log::warn!("{} self-update failed: {e}", self.app.name), Failure::Updater(note) => log::warn!("{} self-update failed: {}", self.app.name, note.translated(english)), } + // The prompt says why; a result window showing the installation closes. + if matches!(self.status, Status::Installing) { + self.status = Status::Idle; + } // The prompt may have been dismissed meanwhile: bring it back with the reason. if self.prompt.is_none() && let Some(release) = release @@ -357,29 +471,15 @@ impl Updater { self.show_if(ctx, true) } - /// Like [`Self::show`], but an installation only starts when `can_start` is true. An app passes - /// `false` while it holds unsaved work: the restart helper is scheduled before the app closes, - /// and a close that the user then cancels would leave it waiting for an exit that never comes. + /// Like [`Self::show`], but an installation only starts, and a ready update only asks the app + /// to close, when `can_start` is true. An app passes `false` while it holds unsaved work. pub fn show_if(&mut self, ctx: &egui::Context, can_start: bool) -> Action { // The close waits for the app to hold no unsaved work: the user may have edited while the - // update installed. The Windows helper starts only now, since it waits just a minute for - // the app to exit. - if can_start && let Some((restart, release)) = self.restart.take() { - match restart.start() { - Ok(()) => return Action::Close, - Err(e) => { - log::warn!("{} self-update failed: {e}", self.app.name); - // Nothing will install it: the update is on offer again. - self.installed = false; - if self.prompt.is_none() { - self.prompt = Some(self.prompt_for(release)); - } - if let Some(prompt) = &mut self.prompt { - prompt.is_installing = false; - prompt.set_message(e); - } - } - } + // update installed. It is only asked for: the Windows helper, which waits just a minute + // for the app to exit, starts at the exit itself (`start_pending_update`), so a close the + // user cancels leaves the update ready. + if can_start && self.restart.is_some() && std::mem::take(&mut self.close_wanted) { + return Action::Close; } let Some(prompt) = &mut self.prompt else { return Action::None }; match prompt.show(ctx) { @@ -401,11 +501,12 @@ impl Updater { } } - /// Starts the helper of an installed update that is still waiting for the app to close, so - /// that the update installs once the app has exited. Call it when the app exits by a path - /// that did not come through [`Action::Close`]; dropping the `Updater` calls it too. Later - /// calls do nothing. An installation still running when the `Updater` is dropped starts its - /// helper itself when it finishes. + /// Starts the helper of a ready update, so that the update installs once the app has exited. + /// Call it once the app's exit is certain (a close nothing can cancel any more, in `on_exit`), + /// whether or not the exit came through [`Action::Close`]; dropping the `Updater` calls it + /// too. Later calls do nothing. An installation still running when the `Updater` is dropped + /// starts its helper itself when it finishes. A helper that cannot start is recorded, and the + /// next start reports it ([`Status::InstallFailed`]). pub fn start_pending_update(&mut self) { // An installation that finished since the last `tick`. if let Some((rx, release)) = &self.install @@ -425,6 +526,9 @@ impl Updater { && let Err(e) = restart.start() { log::warn!("{} self-update failed: {e}", self.app.name); + if let Some(paths) = self.outcome_paths() { + self_update::record_failure(&paths, &e); + } } } @@ -444,7 +548,11 @@ impl Updater { let installed_note = self.installed_note(); let Some(prompt) = &mut self.prompt else { return }; if self.installed { + // Restart to update: the app is asked to close again. prompt.set_note(installed_note); + if self.restart.is_some() { + self.close_wanted = true; + } return; } let Some(installed) = self_update::current(self.app) else { @@ -819,14 +927,35 @@ mod tests { assert!(message(&u).starts_with("Save or close"), "{}", message(&u)); FRENCH.store(true, Ordering::Relaxed); assert!(message(&u).starts_with("FR Save or close"), "{}", message(&u)); - // An installed update names the app, in the language of the moment. + // A ready update names the app, in the language of the moment. let (tx, rx) = channel(); u.install = Some((rx, newer())); assert!(tx.send(Ok(Restart::scheduled())).is_ok()); u.tick(&ctx); - assert!(message(&u).starts_with("FR The update is installed. Close PhotoCraft"), "{}", message(&u)); + let ready = crate::translate::fill(READY_TEXT, &[("app", "PhotoCraft")]); + assert_eq!(message(&u), format!("FR {ready}")); FRENCH.store(false, Ordering::Relaxed); - assert!(message(&u).starts_with("The update is installed."), "{}", message(&u)); + assert_eq!(message(&u), ready); + } + + /// On Windows the update installs after the app exits, so a finished preparation says it is + /// ready, never that it is installed. + #[test] + fn a_prepared_update_says_when_it_installs() { + if cfg!(windows) { + assert_eq!(READY_TEXT, "Update ready. Close {app} to install it."); + } else { + assert!(READY_TEXT.starts_with("The update is installed."), "{READY_TEXT}"); + } + let ctx = egui::Context::default(); + let mut u = updater(Ok(None)); + u.offer(newer()); + let (tx, rx) = channel(); + u.install = Some((rx, newer())); + assert!(tx.send(Ok(Restart::scheduled())).is_ok()); + u.tick(&ctx); + let prompt = u.prompt.as_ref().map(|p| (p.is_installing, p.is_ready, p.message())); + assert_eq!(prompt, Some((false, true, Some(crate::translate::fill(READY_TEXT, &[("app", "PhotoCraft")]))))); } /// One frame with the updater's prompt drawn. @@ -862,10 +991,10 @@ mod tests { assert_eq!(u.show(&ctx), Action::None); } - /// A finished installation that still has to start its helper starts it only when the app - /// may close, and only once. + /// A finished installation that still has to start its helper asks the app to close only + /// when it may, and starts the helper only at the exit, once. #[test] - fn the_helper_starts_once_when_the_app_closes() { + fn the_helper_starts_once_when_the_app_exits() { use std::sync::atomic::{AtomicUsize, Ordering}; let ctx = egui::Context::default(); let mut u = updater(Ok(None)); @@ -884,11 +1013,62 @@ mod tests { assert_eq!(frame(&mut u, &ctx, false), Action::None); assert_eq!(starts.load(Ordering::SeqCst), 0, "unsaved work holds the helper back"); assert_eq!(frame(&mut u, &ctx, true), Action::Close); - assert_eq!(starts.load(Ordering::SeqCst), 1); + assert_eq!(starts.load(Ordering::SeqCst), 0, "a close is a request; the helper waits for the exit"); assert_eq!(frame(&mut u, &ctx, true), Action::None); + u.start_pending_update(); + assert_eq!(starts.load(Ordering::SeqCst), 1); + u.start_pending_update(); + drop(u); assert_eq!(starts.load(Ordering::SeqCst), 1, "the helper starts once"); } + /// A close the user cancels starts no helper and keeps the update ready: a requested check + /// says so, and Restart to Update asks for the close again, from the result window or from + /// the prompt. + #[test] + fn a_canceled_close_keeps_the_update_ready() { + use std::sync::atomic::Ordering; + let ctx = egui::Context::default(); + let mut u = updater(Ok(None)); + u.offer(newer()); + let (starts, restart) = counted_restart(); + let (tx, rx) = channel(); + u.install = Some((rx, newer())); + assert!(tx.send(Ok(restart)).is_ok()); + u.tick(&ctx); + assert_eq!(frame(&mut u, &ctx, true), Action::Close); + // The app's close was canceled: it keeps running and drawing. + for _ in 0..3 { + assert_eq!(frame(&mut u, &ctx, true), Action::None, "a canceled close is not asked for again by itself"); + } + assert_eq!(starts.load(Ordering::SeqCst), 0, "no helper starts while the app runs"); + u.check_now(&ctx); + assert_eq!(u.status(), &Status::Ready); + assert!(u.check.is_none(), "no check runs while an update is ready"); + assert!(u.prompt.as_ref().is_some_and(|p| p.is_ready), "the prompt offers Restart to Update"); + + u.restart_to_update(); + assert_eq!(frame(&mut u, &ctx, false), Action::None, "unsaved work still holds the close back"); + assert_eq!(frame(&mut u, &ctx, true), Action::Close); + assert_eq!(frame(&mut u, &ctx, true), Action::None); + // The prompt's button does the same. + u.request_install(&ctx, true); + assert_eq!(frame(&mut u, &ctx, true), Action::Close); + assert_eq!(starts.load(Ordering::SeqCst), 0); + + drop(u); + assert_eq!(starts.load(Ordering::SeqCst), 1, "the exit starts the helper once"); + } + + /// Restart to Update does nothing when no update is ready. + #[test] + fn restart_to_update_needs_a_ready_update() { + let ctx = egui::Context::default(); + let mut u = updater(Ok(None)); + u.restart_to_update(); + assert_eq!(frame(&mut u, &ctx, true), Action::None); + } + /// A counter and a helper that adds one to it each time it starts. fn counted_restart() -> (Arc, Restart) { let starts = Arc::new(std::sync::atomic::AtomicUsize::new(0)); @@ -971,9 +1151,9 @@ mod tests { assert!(u.restart.is_none()); } - /// A helper started at [`Action::Close`] does not start again when the updater is dropped. + /// After [`Action::Close`], the app's exit (the updater dropped) starts the helper once. #[test] - fn a_helper_started_at_close_does_not_start_again_on_drop() { + fn the_exit_after_a_close_starts_the_helper_once() { use std::sync::atomic::Ordering; let ctx = egui::Context::default(); let mut u = updater(Ok(None)); @@ -983,23 +1163,177 @@ mod tests { assert!(tx.send(Ok(restart)).is_ok()); u.tick(&ctx); assert_eq!(frame(&mut u, &ctx, true), Action::Close); + assert_eq!(frame(&mut u, &ctx, true), Action::None, "the close is asked for once"); + assert_eq!(starts.load(Ordering::SeqCst), 0); drop(u); assert_eq!(starts.load(Ordering::SeqCst), 1); } - /// A helper that can't start keeps the app open and offers the update again with the reason. + /// Outcome records in a fresh folder, and an updater reading them that runs `version`. + fn with_records(version: &str) -> (tempfile::TempDir, Paths, Updater) { + let dir = tempfile::tempdir().unwrap(); + let paths = Paths::under(dir.path()); + let u = reading(&paths, version); + (dir, paths, u) + } + + /// An updater running `version` that reads the outcome records under `paths`. + fn reading(paths: &Paths, version: &str) -> Updater { + Updater::new(&PHOTOCRAFT, version).with_source(Arc::new(|| Ok(None))).with_pins_path(no_pins()).with_paths(paths.clone()) + } + + /// The check at start, answered with `answer` without waiting on its thread. + fn start_answered(u: &mut Updater, ctx: &egui::Context, answer: Checked) { + u.check_at_start(ctx); + let (tx, rx) = channel(); + u.check = Some(rx); + assert!(tx.send(answer).is_ok()); + u.tick(ctx); + } + + /// A helper that can't start at the exit is recorded, and the next start says why, once. #[test] - fn a_helper_that_cannot_start_keeps_the_app_open() { + fn a_helper_that_cannot_start_at_the_exit_is_reported_at_the_next_start() { let ctx = egui::Context::default(); - let mut u = updater(Ok(None)); + let (_dir, paths, mut u) = with_records("0.1.0"); let (tx, rx) = channel(); u.install = Some((rx, newer())); assert!(tx.send(Ok(Restart::later(|| Err("couldn't schedule the update (denied)".into())))).is_ok()); u.tick(&ctx); - assert_eq!(frame(&mut u, &ctx, true), Action::None); - let prompt = u.prompt.as_ref().map(|p| (p.is_installing, p.status_message.clone())); - assert_eq!(prompt, Some((false, Some("couldn't schedule the update (denied)".into())))); + assert_eq!(frame(&mut u, &ctx, true), Action::Close); + drop(u); + + let mut next = reading(&paths, "0.1.0"); + start_answered(&mut next, &ctx, Ok(None)); + assert_eq!(next.status(), &Status::InstallFailed("couldn't schedule the update (denied)".into())); + let mut later = reading(&paths, "0.1.0"); + start_answered(&mut later, &ctx, Ok(None)); + assert_eq!(later.status(), &Status::Idle, "a failure is reported once"); + } + + /// The first start after a helper failure shows the reason once, and the quiet check at start + /// does not replace it. A failure wins over a stale record of a finished update. + #[test] + fn a_recorded_failure_is_shown_once_at_the_next_start() { + let ctx = egui::Context::default(); + let (_dir, paths, mut u) = with_records("0.1.0"); + self_update::record_failure(&paths, "the Windows Installer stopped with error 1603"); + crate::paths::write_atomic(&paths.data.join("self-update-done.txt"), b"v0.1.0").unwrap(); + start_answered(&mut u, &ctx, Ok(None)); + assert_eq!(u.status(), &Status::InstallFailed("the Windows Installer stopped with error 1603".into())); + u.dismiss_status(); + assert_eq!(u.status(), &Status::Idle); + + let mut next = reading(&paths, "0.1.0"); + start_answered(&mut next, &ctx, Ok(None)); + assert_eq!(next.status(), &Status::Idle, "neither record is read twice"); + } + + /// A failed update that the check at start offers again says in its prompt why it failed. + #[test] + fn a_failed_update_offered_again_says_why_it_failed() { + let ctx = egui::Context::default(); + let (_dir, paths, mut u) = with_records("0.1.0"); + self_update::record_failure(&paths, "disk full"); + start_answered(&mut u, &ctx, Ok(Some(newer()))); + assert_eq!(u.status(), &Status::Idle, "the prompt carries the reason"); + assert_eq!(u.prompt.as_ref().and_then(StartupPrompt::message).as_deref(), Some("The update could not be installed: disk full")); + } + + /// An unreadable record (a folder where the file goes) shows nothing and breaks nothing. + #[test] + fn an_unreadable_record_shows_nothing() { + let ctx = egui::Context::default(); + let (_dir, paths, mut u) = with_records("0.2.0"); + std::fs::create_dir_all(paths.logs().join("self-update-error.txt")).unwrap(); + std::fs::create_dir_all(paths.data.join("self-update-done.txt")).unwrap(); + start_answered(&mut u, &ctx, Ok(None)); + assert_eq!(u.status(), &Status::Idle); + } + + /// The new version's first start says it was updated, once, and only when the finished + /// update installed the version running now. + #[test] + fn a_finished_update_is_announced_once_by_the_version_it_installed() { + let ctx = egui::Context::default(); + let (_dir, paths, mut u) = with_records("0.2.0"); + crate::paths::write_atomic(&paths.data.join("self-update-done.txt"), b"v0.2.0").unwrap(); + start_answered(&mut u, &ctx, Ok(None)); + assert_eq!(u.status(), &Status::Updated("0.2.0".into())); + let mut next = reading(&paths, "0.2.0"); + start_answered(&mut next, &ctx, Ok(None)); + assert_eq!(next.status(), &Status::Idle, "announced once"); + + // The old version started again (the update didn't take): no claim, and the record goes. + crate::paths::write_atomic(&paths.data.join("self-update-done.txt"), b"v0.3.0").unwrap(); + let mut old = reading(&paths, "0.2.0"); + start_answered(&mut old, &ctx, Ok(None)); + assert_eq!(old.status(), &Status::Idle); + assert!(!paths.data.join("self-update-done.txt").exists()); + } + + /// A requested check while an update downloads says it is installing, and while it waits for + /// the close says it is ready, after dismissal and on repeated clicks too, and starts nothing. + #[test] + fn a_requested_check_during_an_update_says_where_it_is() { + let ctx = egui::Context::default(); + let mut u = updater(Ok(Some(newer()))); + u.offer(newer()); + let (tx, rx) = channel(); + u.install = Some((rx, newer())); + for _ in 0..2 { + u.check_now(&ctx); + assert_eq!(u.status(), &Status::Installing); + assert!(u.check.is_none(), "no check starts"); + } + u.dismiss_status(); + u.check_now(&ctx); + assert_eq!(u.status(), &Status::Installing, "asked again after dismissal"); + assert!(tx.send(Ok(Restart::scheduled())).is_ok()); + u.tick(&ctx); + assert_eq!(u.status(), &Status::Ready, "the open window follows the update"); + u.dismiss_status(); + for _ in 0..2 { + u.check_now(&ctx); + assert_eq!(u.status(), &Status::Ready); + assert!(u.check.is_none() && u.install.is_none(), "nothing starts"); + } + } + + /// A failed installation closes a result window showing it; the prompt says why. + #[test] + fn a_failed_install_ends_the_installing_status() { + let ctx = egui::Context::default(); + let mut u = updater(Ok(None)); + let (tx, rx) = channel(); + u.install = Some((rx, newer())); + u.check_now(&ctx); + assert_eq!(u.status(), &Status::Installing); + assert!(tx.send(Err("disk full".into())).is_ok()); + u.tick(&ctx); + assert_eq!(u.status(), &Status::Idle); + assert_eq!(u.prompt.as_ref().and_then(|p| p.status_message.clone()).as_deref(), Some("disk full")); + } + + /// A helper that started while the app kept running and then failed is found by the next + /// requested check, which offers the update again. + #[test] + fn a_requested_check_finds_a_helper_failure_while_the_app_still_runs() { + let ctx = egui::Context::default(); + let (_dir, paths, mut u) = with_records("0.1.0"); + let (tx, rx) = channel(); + u.install = Some((rx, newer())); + assert!(tx.send(Ok(Restart::scheduled())).is_ok()); + u.tick(&ctx); + u.start_pending_update(); + u.check_now(&ctx); + assert_eq!(u.status(), &Status::Ready, "the helper is still waiting"); + self_update::record_failure(&paths, "ArtCraft Launcher is still running"); + u.check_now(&ctx); + assert_eq!(u.status(), &Status::InstallFailed("ArtCraft Launcher is still running".into())); assert!(!u.installed, "the update is on offer again"); + u.check_now(&ctx); + assert_eq!(u.status(), &Status::Checking, "Check Again runs a check"); } } diff --git a/crates/update/src/updater_pin_tests.rs b/crates/update/src/updater_pin_tests.rs index 2a4f58f1..8a4b73d7 100644 --- a/crates/update/src/updater_pin_tests.rs +++ b/crates/update/src/updater_pin_tests.rs @@ -30,7 +30,7 @@ fn a_pinned_app_is_not_prompted_and_the_network_is_not_asked() { assert_eq!(u.status(), &Status::Idle); u.check_now(&ctx); settle(&mut u, &ctx); - assert_eq!(u.status(), &Status::UpToDate, "{track:?}"); + assert_eq!(u.status(), &Status::Managed, "{track:?}"); assert_eq!(u.managed(), Some(&Choice::Managed(track.clone()))); u.offer(newer()); assert!(!u.prompt_open(), "an offer is held too: {track:?}"); @@ -109,7 +109,7 @@ fn a_check_after_the_pin_closes_a_prompt_opened_before_it() { u.check_now(&ctx); settle(&mut u, &ctx); assert!(!u.prompt_open()); - assert_eq!(u.status(), &Status::UpToDate); + assert_eq!(u.status(), &Status::Managed); } #[test] @@ -140,7 +140,7 @@ fn a_pin_made_while_the_check_runs_still_holds_its_answer() { u.check_now(&ctx); settle(&mut u, &ctx); assert!(!u.prompt_open()); - assert_eq!(u.status(), &Status::UpToDate); + assert_eq!(u.status(), &Status::Managed); } #[test] @@ -160,7 +160,7 @@ fn every_answer_after_a_pin_made_mid_check_reports_the_launchers_choice() { let mut u = Updater::new(&PHOTOCRAFT, "0.13.0").with_source(source).with_pins_path(&path); u.check_now(&ctx); settle(&mut u, &ctx); - assert_eq!(u.status(), &Status::UpToDate); + assert_eq!(u.status(), &Status::Managed); assert_eq!(u.managed(), Some(&Choice::Managed(crate::pins::VersionTrack::Nightly))); } } From 8ccbe9b2ca8fd7b3aab0aef35ee8cb819e3c575a Mon Sep 17 00:00:00 2001 From: dotCooCoo Date: Sat, 7 Nov 2026 17:37:59 -0800 Subject: [PATCH 08/25] craft-update: record every update failure at the exit and clear a stale ready prompt An installation that finishes after the Updater is dropped now records a helper that cannot start, and a failed installation, so the next start reports it. start_pending_update records an installation failure it picks up at the exit too. When a requested check finds that a started helper failed, the open prompt no longer offers Restart to Update: it says why the update could not be installed, and no close is asked for. --- crates/update/src/updater.rs | 114 +++++++++++++++++++++++++++++------ 1 file changed, 94 insertions(+), 20 deletions(-) diff --git a/crates/update/src/updater.rs b/crates/update/src/updater.rs index a91fbcd6..14809ded 100644 --- a/crates/update/src/updater.rs +++ b/crates/update/src/updater.rs @@ -95,6 +95,23 @@ impl Failure { fn dropped() -> Self { Self::Updater(Note::new("The update stopped unexpectedly.", &[])) } + + /// The reason in English, for the log and for the record the next start reads. + fn reason(&self) -> String { + match self { + Self::Installer(e) => e.clone(), + Self::Updater(note) => note.translated(english), + } + } +} + +/// Logs a failed update and records it under `paths`, so that the next start reports it +/// ([`Status::InstallFailed`]). Used at the exit, where no prompt can show the reason. +fn record(app: &App, paths: Option<&Paths>, reason: &str) { + log::warn!("{} self-update failed: {reason}", app.name); + if let Some(paths) = paths { + self_update::record_failure(paths, reason); + } } impl From for Failure { @@ -343,7 +360,8 @@ impl Updater { /// The answer to a requested check while an update installs or waits for the app to exit. A /// helper that already started while the app kept running may have failed: its recorded - /// reason is the answer then, and the update is on offer again. + /// reason is the answer then, the update is on offer again, and an open prompt says why + /// instead of offering Restart to Update. fn pending_status(&mut self) -> Status { if self.install.is_some() { return Status::Installing; @@ -352,8 +370,15 @@ impl Updater { && let Some(reason) = self.outcome_paths().and_then(|paths| self_update::take_failure(&paths)) { log::warn!("{} self-update failed: {reason}", self.app.name); + let reason = reason.trim().to_owned(); self.installed = false; - return Status::InstallFailed(reason.trim().to_owned()); + self.close_wanted = false; + if let Some(prompt) = &mut self.prompt { + prompt.is_installing = false; + prompt.is_ready = false; + prompt.set_note(Note::new("The update could not be installed: {reason}", &[("reason", &reason)])); + } + return Status::InstallFailed(reason); } Status::Ready } @@ -505,8 +530,9 @@ impl Updater { /// Call it once the app's exit is certain (a close nothing can cancel any more, in `on_exit`), /// whether or not the exit came through [`Action::Close`]; dropping the `Updater` calls it /// too. Later calls do nothing. An installation still running when the `Updater` is dropped - /// starts its helper itself when it finishes. A helper that cannot start is recorded, and the - /// next start reports it ([`Status::InstallFailed`]). + /// starts its helper itself when it finishes. A helper that cannot start, or an installation + /// that failed and was not reported yet, is recorded, and the next start reports it + /// ([`Status::InstallFailed`]). pub fn start_pending_update(&mut self) { // An installation that finished since the last `tick`. if let Some((rx, release)) = &self.install @@ -517,18 +543,14 @@ impl Updater { self.restart = Some((restart, release.clone())); self.installed = true; } - Err(Failure::Installer(e)) => log::warn!("{} self-update failed: {e}", self.app.name), - Err(Failure::Updater(note)) => log::warn!("{} self-update failed: {}", self.app.name, note.translated(english)), + Err(failure) => record(self.app, self.outcome_paths().as_ref(), &failure.reason()), } self.install = None; } if let Some((restart, _)) = self.restart.take() && let Err(e) = restart.start() { - log::warn!("{} self-update failed: {e}", self.app.name); - if let Some(paths) = self.outcome_paths() { - self_update::record_failure(&paths, &e); - } + record(self.app, self.outcome_paths().as_ref(), &e); } } @@ -546,6 +568,7 @@ impl Updater { return; } let installed_note = self.installed_note(); + let paths = self.outcome_paths(); let Some(prompt) = &mut self.prompt else { return }; if self.installed { // Restart to update: the app is asked to close again. @@ -569,7 +592,7 @@ impl Updater { let spawned = std::thread::Builder::new().name("self-update".into()).spawn(move || { let run = || install(app, &installed, worker_release, &version).map_err(Failure::Installer); let outcome = std::panic::catch_unwind(std::panic::AssertUnwindSafe(run)).unwrap_or_else(|_| Err(Failure::panicked())); - deliver(app, &tx, &dropped, outcome); + deliver(app, &tx, &dropped, paths.as_ref(), outcome); ctx.request_repaint(); }); match spawned { @@ -597,15 +620,20 @@ impl Drop for Updater { /// Sends the install worker's outcome to the [`Updater`]. When the `Updater` has been dropped /// (the app is exiting), the worker starts a finished installation's helper itself, since -/// nothing will start it at a close. -fn deliver(app: &App, tx: &Sender>, dropped: &Mutex, outcome: Result) { +/// nothing will start it at a close. A helper that cannot start, or a failed installation, is +/// then recorded under `paths` for the next start. +fn deliver(app: &App, tx: &Sender>, dropped: &Mutex, paths: Option<&Paths>, outcome: Result) { let gone = dropped.lock().unwrap_or_else(PoisonError::into_inner); let unsent = if *gone { Some(outcome) } else { tx.send(outcome).err().map(|SendError(outcome)| outcome) }; drop(gone); - if let Some(Ok(restart)) = unsent - && let Err(e) = restart.start() - { - log::warn!("{} self-update failed: {e}", app.name); + match unsent { + Some(Ok(restart)) => { + if let Err(e) = restart.start() { + record(app, paths, &e); + } + } + Some(Err(failure)) => record(app, paths, &failure.reason()), + None => {} } } @@ -1122,18 +1150,18 @@ mod tests { u.install = Some((rx, newer())); drop(u); assert_eq!(starts.load(Ordering::SeqCst), 0); - deliver(&crate::suite::LAUNCHER, &tx, &dropped, Ok(restart)); + deliver(&crate::suite::LAUNCHER, &tx, &dropped, None, Ok(restart)); assert_eq!(starts.load(Ordering::SeqCst), 1); // Sent while the drop has read the channel but not yet released the receiver. let (starts, restart) = counted_restart(); let (tx, _rx) = channel(); - deliver(&crate::suite::LAUNCHER, &tx, &Mutex::new(true), Ok(restart)); + deliver(&crate::suite::LAUNCHER, &tx, &Mutex::new(true), None, Ok(restart)); assert_eq!(starts.load(Ordering::SeqCst), 1, "a receiver about to be dropped is not sent to"); let (starts, restart) = counted_restart(); let (tx, rx) = channel(); - deliver(&crate::suite::LAUNCHER, &tx, &Mutex::new(false), Ok(restart)); + deliver(&crate::suite::LAUNCHER, &tx, &Mutex::new(false), None, Ok(restart)); assert_eq!(starts.load(Ordering::SeqCst), 0, "the updater starts it at the close"); assert!(rx.try_recv().is_ok_and(|outcome| outcome.is_ok())); } @@ -1321,6 +1349,7 @@ mod tests { fn a_requested_check_finds_a_helper_failure_while_the_app_still_runs() { let ctx = egui::Context::default(); let (_dir, paths, mut u) = with_records("0.1.0"); + u.offer(newer()); let (tx, rx) = channel(); u.install = Some((rx, newer())); assert!(tx.send(Ok(Restart::scheduled())).is_ok()); @@ -1328,13 +1357,58 @@ mod tests { u.start_pending_update(); u.check_now(&ctx); assert_eq!(u.status(), &Status::Ready, "the helper is still waiting"); + assert!(u.prompt.as_ref().is_some_and(|p| p.is_ready), "the prompt offers Restart to Update"); self_update::record_failure(&paths, "ArtCraft Launcher is still running"); u.check_now(&ctx); assert_eq!(u.status(), &Status::InstallFailed("ArtCraft Launcher is still running".into())); assert!(!u.installed, "the update is on offer again"); + let prompt = u.prompt.as_ref().map(|p| (p.is_installing, p.is_ready, p.message())); + let reason = "The update could not be installed: ArtCraft Launcher is still running".to_owned(); + assert_eq!(prompt, Some((false, false, Some(reason))), "the prompt no longer offers Restart to Update"); + assert!(!u.close_wanted, "no close is asked for"); + assert_eq!(frame(&mut u, &ctx, true), Action::None); u.check_now(&ctx); assert_eq!(u.status(), &Status::Checking, "Check Again runs a check"); } + + /// An installation that finishes after the updater was dropped and whose helper cannot start, + /// or that failed, is recorded, and the next start says why. + #[test] + fn an_install_failing_after_the_updater_is_dropped_is_reported_at_the_next_start() { + let ctx = egui::Context::default(); + let failures: [(Result, &str); 3] = [ + (Ok(Restart::later(|| Err("couldn't schedule the update (denied)".into()))), "couldn't schedule the update (denied)"), + (Err("couldn't download the update".into()), "couldn't download the update"), + (Err(Failure::panicked()), "The update failed unexpectedly."), + ]; + for (outcome, want) in failures { + let (_dir, paths, mut u) = with_records("0.1.0"); + let dropped = Arc::clone(&u.dropped); + let (tx, rx) = channel(); + u.install = Some((rx, newer())); + drop(u); + deliver(&PHOTOCRAFT, &tx, &dropped, Some(&paths), outcome); + let mut next = reading(&paths, "0.1.0"); + start_answered(&mut next, &ctx, Ok(None)); + assert_eq!(next.status(), &Status::InstallFailed(want.into()), "{want}"); + } + } + + /// A failed installation picked up at the exit is recorded, and the next start says why. + #[test] + fn a_failed_install_picked_up_at_exit_is_reported_at_the_next_start() { + let ctx = egui::Context::default(); + for (failure, want) in [(Failure::from("disk full"), "disk full"), (Failure::dropped(), "The update stopped unexpectedly.")] { + let (_dir, paths, mut u) = with_records("0.1.0"); + let (tx, rx) = channel::>(); + u.install = Some((rx, newer())); + assert!(tx.send(Err(failure)).is_ok()); + drop(u); + let mut next = reading(&paths, "0.1.0"); + start_answered(&mut next, &ctx, Ok(None)); + assert_eq!(next.status(), &Status::InstallFailed(want.into()), "{want}"); + } + } } #[cfg(test)] From 4c29fc7cd57cf8bf3fbe6dc6c7da4611242b018f Mon Sep 17 00:00:00 2001 From: dotCooCoo Date: Sat, 7 Nov 2026 18:05:12 -0800 Subject: [PATCH 09/25] craft-update: Check Again after a failed update still says why it failed A failed update's reason is kept until a prompt that offers the update again shows it, so a dismissed failure followed by Check Again still explains the failure. A prompt that already showed the reason does not repeat it. --- crates/update/src/updater.rs | 58 ++++++++++++++++++++++++++++++++---- 1 file changed, 52 insertions(+), 6 deletions(-) diff --git a/crates/update/src/updater.rs b/crates/update/src/updater.rs index 14809ded..e7f1d788 100644 --- a/crates/update/src/updater.rs +++ b/crates/update/src/updater.rs @@ -148,6 +148,9 @@ pub struct Updater { /// The folders that hold the outcome records ([`self_update::take_outcome`]) in place of the /// app's own (tests). paths: Option, + /// Why the last update could not be installed, until a prompt that offers the update again + /// shows it. + failed_reason: Option, /// Set when the `Updater` is dropped. An install worker sends its outcome while holding it, /// and starts a finished installation's helper itself once it is set. dropped: Arc>, @@ -177,6 +180,7 @@ impl Updater { installed: false, close_wanted: false, paths: None, + failed_reason: None, dropped: Arc::new(Mutex::new(false)), pins_path: None, managed: None, @@ -233,6 +237,7 @@ impl Updater { Some(Outcome::Installed(version)) => self.status = Status::Updated(version.to_string()), Some(Outcome::Failed(reason)) => { log::warn!("{} self-update failed: {reason}", self.app.name); + self.failed_reason = Some(reason.clone()); self.status = Status::InstallFailed(reason); } None => {} @@ -373,10 +378,14 @@ impl Updater { let reason = reason.trim().to_owned(); self.installed = false; self.close_wanted = false; - if let Some(prompt) = &mut self.prompt { - prompt.is_installing = false; - prompt.is_ready = false; - prompt.set_note(Note::new("The update could not be installed: {reason}", &[("reason", &reason)])); + match &mut self.prompt { + Some(prompt) => { + prompt.is_installing = false; + prompt.is_ready = false; + prompt.set_note(Note::new("The update could not be installed: {reason}", &[("reason", &reason)])); + } + // The next prompt that offers the update says why. + None => self.failed_reason = Some(reason.clone()), } return Status::InstallFailed(reason); } @@ -403,8 +412,8 @@ impl Updater { if self.prompt.is_none() && self.install.is_none() && !self.installed { let mut prompt = self.prompt_for(release); // A failed update offered again says why the last attempt failed. - if let Status::InstallFailed(reason) = &self.status { - prompt.set_note(Note::new("The update could not be installed: {reason}", &[("reason", reason)])); + if let Some(reason) = self.failed_reason.take() { + prompt.set_note(Note::new("The update could not be installed: {reason}", &[("reason", &reason)])); } self.prompt = Some(prompt); } @@ -585,6 +594,7 @@ impl Updater { }; prompt.is_installing = true; prompt.clear_message(); + self.failed_reason = None; let (tx, rx) = channel(); let (app, version, ctx, release) = (self.app, self.version.clone(), ctx.clone(), prompt.release.clone()); let worker_release = release.clone(); @@ -1268,6 +1278,34 @@ mod tests { assert_eq!(u.prompt.as_ref().and_then(StartupPrompt::message).as_deref(), Some("The update could not be installed: disk full")); } + /// Check Again after a failed update, with the failure already dismissed, still says in the + /// prompt that offers the update why it failed, and only that prompt does. + #[test] + fn check_again_after_a_failed_update_says_why_it_failed() { + let ctx = egui::Context::default(); + let (_dir, paths, mut u) = with_records("0.1.0"); + self_update::record_failure(&paths, "disk full"); + start_answered(&mut u, &ctx, Ok(None)); + assert_eq!(u.status(), &Status::InstallFailed("disk full".into())); + u.dismiss_status(); + u.check_now(&ctx); + assert_eq!(u.status(), &Status::Checking); + let (tx, rx) = channel(); + u.check = Some(rx); + assert!(tx.send(Ok(Some(newer()))).is_ok()); + u.tick(&ctx); + assert_eq!(u.prompt.as_ref().and_then(StartupPrompt::message).as_deref(), Some("The update could not be installed: disk full")); + + u.prompt = None; + u.check_now(&ctx); + let (tx, rx) = channel(); + u.check = Some(rx); + assert!(tx.send(Ok(Some(newer()))).is_ok()); + u.tick(&ctx); + assert!(u.prompt.is_some()); + assert_eq!(u.prompt.as_ref().and_then(StartupPrompt::message), None, "the reason is shown once"); + } + /// An unreadable record (a folder where the file goes) shows nothing and breaks nothing. #[test] fn an_unreadable_record_shows_nothing() { @@ -1369,6 +1407,14 @@ mod tests { assert_eq!(frame(&mut u, &ctx, true), Action::None); u.check_now(&ctx); assert_eq!(u.status(), &Status::Checking, "Check Again runs a check"); + // The prompt showed the reason: a later prompt does not repeat it. + u.prompt = None; + let (tx, rx) = channel(); + u.check = Some(rx); + assert!(tx.send(Ok(Some(newer()))).is_ok()); + u.tick(&ctx); + assert!(u.prompt.is_some()); + assert_eq!(u.prompt.as_ref().and_then(StartupPrompt::message), None, "the reason is shown once"); } /// An installation that finishes after the updater was dropped and whose helper cannot start, From 7bde3a7c1ecfd0c7368e7820715485f9fd11f722 Mon Sep 17 00:00:00 2001 From: dotCooCoo Date: Sat, 7 Nov 2026 18:09:51 -0800 Subject: [PATCH 10/25] craft-update: record an install worker that ends without an outcome at the exit start_pending_update now treats a disconnected install worker as a failed update, as tick does, so the next start says the update stopped unexpectedly. --- crates/update/src/updater.rs | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/crates/update/src/updater.rs b/crates/update/src/updater.rs index e7f1d788..a19209ed 100644 --- a/crates/update/src/updater.rs +++ b/crates/update/src/updater.rs @@ -543,9 +543,13 @@ impl Updater { /// that failed and was not reported yet, is recorded, and the next start reports it /// ([`Status::InstallFailed`]). pub fn start_pending_update(&mut self) { - // An installation that finished since the last `tick`. + // An installation that finished since the last `tick`, or whose worker ended without a word. if let Some((rx, release)) = &self.install - && let Ok(outcome) = rx.try_recv() + && let Some(outcome) = match rx.try_recv() { + Ok(outcome) => Some(outcome), + Err(TryRecvError::Empty) => None, + Err(TryRecvError::Disconnected) => Some(Err(Failure::dropped())), + } { match outcome { Ok(restart) => { @@ -1454,6 +1458,15 @@ mod tests { start_answered(&mut next, &ctx, Ok(None)); assert_eq!(next.status(), &Status::InstallFailed(want.into()), "{want}"); } + // A worker that ended without sending anything. + let (_dir, paths, mut u) = with_records("0.1.0"); + let (tx, rx) = channel::>(); + u.install = Some((rx, newer())); + drop(tx); + drop(u); + let mut next = reading(&paths, "0.1.0"); + start_answered(&mut next, &ctx, Ok(None)); + assert_eq!(next.status(), &Status::InstallFailed("The update stopped unexpectedly.".into())); } } From a1471f2cf354bc739e47be065cbf14c66e3aefc5 Mon Sep 17 00:00:00 2001 From: dotCooCoo Date: Sat, 7 Nov 2026 18:34:39 -0800 Subject: [PATCH 11/25] craft-update: show_if alone decides whether Restart to Update closes the app restart_to_update only asks for the close; show_if returns Action::Close only while an update is ready, so the test of a call with no update ready fails when that check is removed. The test of a folder in place of an outcome record is removed: Windows refuses to open a folder, so it passed with the plain-file check removed. --- crates/update/src/updater.rs | 17 ++--------------- 1 file changed, 2 insertions(+), 15 deletions(-) diff --git a/crates/update/src/updater.rs b/crates/update/src/updater.rs index a19209ed..c8019b10 100644 --- a/crates/update/src/updater.rs +++ b/crates/update/src/updater.rs @@ -296,11 +296,9 @@ impl Updater { /// Asks the app to close so that a ready update installs: the next [`Self::show_if`] that may /// close returns [`Action::Close`]. Restart to Update calls it after the user canceled an - /// earlier close. Does nothing when no update is ready. + /// earlier close. No close is asked for while no update is ready. pub fn restart_to_update(&mut self) { - if self.restart.is_some() { - self.close_wanted = true; - } + self.close_wanted = true; } /// Opens the startup prompt for a release the app found itself. @@ -1310,17 +1308,6 @@ mod tests { assert_eq!(u.prompt.as_ref().and_then(StartupPrompt::message), None, "the reason is shown once"); } - /// An unreadable record (a folder where the file goes) shows nothing and breaks nothing. - #[test] - fn an_unreadable_record_shows_nothing() { - let ctx = egui::Context::default(); - let (_dir, paths, mut u) = with_records("0.2.0"); - std::fs::create_dir_all(paths.logs().join("self-update-error.txt")).unwrap(); - std::fs::create_dir_all(paths.data.join("self-update-done.txt")).unwrap(); - start_answered(&mut u, &ctx, Ok(None)); - assert_eq!(u.status(), &Status::Idle); - } - /// The new version's first start says it was updated, once, and only when the finished /// update installed the version running now. #[test] From 164dcf6a6d1452ad924c973f9f69e1886b09e9c2 Mon Sep 17 00:00:00 2001 From: dotCooCoo Date: Sat, 7 Nov 2026 18:41:49 -0800 Subject: [PATCH 12/25] craft-update: an install still running at start_pending_update starts its own helper start_pending_update now marks the Updater as exiting and lets go of an install that is still running, so the worker starts its helper itself when it finishes (or records its failure) instead of sending its outcome into a channel nobody reads. Before, only dropping the Updater did this, so an app that called start_pending_update in on_exit and kept the Updater alive lost the helper of an install that finished afterwards. Drop now calls start_pending_update. The docs of Action::Close and start_pending_update say that an app which ends the process in on_exit loses an install still in progress. --- crates/update/src/updater.rs | 51 +++++++++++++++++++++++++++--------- 1 file changed, 39 insertions(+), 12 deletions(-) diff --git a/crates/update/src/updater.rs b/crates/update/src/updater.rs index c8019b10..fe4bf914 100644 --- a/crates/update/src/updater.rs +++ b/crates/update/src/updater.rs @@ -71,7 +71,8 @@ pub enum Action { /// [`Status::Ready`] for a requested check and Restart to Update in the prompt. On Windows /// the helper starts only at the exit: when the app drops the [`Updater`] as it exits, or /// calls [`Updater::start_pending_update`] in `on_exit`, which an app that ends the process - /// there without dropping the `Updater` must do. + /// there without dropping the `Updater` must do. An installation still in progress when + /// such an app ends the process is lost, since its worker thread ends with the process. Close, } @@ -536,13 +537,21 @@ impl Updater { /// Starts the helper of a ready update, so that the update installs once the app has exited. /// Call it once the app's exit is certain (a close nothing can cancel any more, in `on_exit`), /// whether or not the exit came through [`Action::Close`]; dropping the `Updater` calls it - /// too. Later calls do nothing. An installation still running when the `Updater` is dropped - /// starts its helper itself when it finishes. A helper that cannot start, or an installation - /// that failed and was not reported yet, is recorded, and the next start reports it + /// too. Later calls do nothing. An installation still running at the first call starts its + /// helper itself when it finishes, as long as the process is still running then; an app + /// that ends the process in `on_exit` loses an installation still in progress, since its + /// worker thread ends with the process. A helper that cannot start, or an installation that + /// failed and was not reported yet, is recorded, and the next start reports it /// ([`Status::InstallFailed`]). pub fn start_pending_update(&mut self) { + // Held while the channel is read: a worker sent its outcome before, and it is read here, + // or it sends after and starts its helper (or records its failure) itself. + let dropped = Arc::clone(&self.dropped); + let mut dropped = dropped.lock().unwrap_or_else(PoisonError::into_inner); + *dropped = true; // An installation that finished since the last `tick`, or whose worker ended without a word. - if let Some((rx, release)) = &self.install + // One still running is let go: it now reports through `deliver` alone. + if let Some((rx, release)) = self.install.take() && let Some(outcome) = match rx.try_recv() { Ok(outcome) => Some(outcome), Err(TryRecvError::Empty) => None, @@ -551,12 +560,11 @@ impl Updater { { match outcome { Ok(restart) => { - self.restart = Some((restart, release.clone())); + self.restart = Some((restart, release)); self.installed = true; } Err(failure) => record(self.app, self.outcome_paths().as_ref(), &failure.reason()), } - self.install = None; } if let Some((restart, _)) = self.restart.take() && let Err(e) = restart.start() @@ -621,11 +629,6 @@ impl Drop for Updater { /// The app is exiting: an installed update that is still waiting starts its helper, which /// waits for this process to exit before it installs. fn drop(&mut self) { - // Held while the channel is read: a worker sent its outcome before, and it is read here, - // or it sends after and starts its helper itself. - let dropped = Arc::clone(&self.dropped); - let mut dropped = dropped.lock().unwrap_or_else(PoisonError::into_inner); - *dropped = true; self.start_pending_update(); } } @@ -1178,6 +1181,30 @@ mod tests { assert!(rx.try_recv().is_ok_and(|outcome| outcome.is_ok())); } + /// An installation still running when the app calls `start_pending_update` in `on_exit`, and + /// that finishes before the updater is dropped, starts its helper once; the worker's end is + /// not then recorded as a failure. + #[test] + fn an_install_finishing_after_start_pending_update_starts_its_helper_once() { + use std::sync::atomic::Ordering; + let ctx = egui::Context::default(); + let (_dir, paths, mut u) = with_records("0.1.0"); + let dropped = Arc::clone(&u.dropped); + let (starts, restart) = counted_restart(); + let (tx, rx) = channel(); + u.install = Some((rx, newer())); + u.start_pending_update(); + assert_eq!(starts.load(Ordering::SeqCst), 0, "nothing has finished yet"); + deliver(&PHOTOCRAFT, &tx, &dropped, Some(&paths), Ok(restart)); + drop(tx); + assert_eq!(starts.load(Ordering::SeqCst), 1, "the worker starts the helper itself"); + drop(u); + assert_eq!(starts.load(Ordering::SeqCst), 1, "the helper starts once"); + let mut next = reading(&paths, "0.1.0"); + start_answered(&mut next, &ctx, Ok(None)); + assert_eq!(next.status(), &Status::Idle, "a helper that started is not a failure"); + } + /// A failed installation picked up at exit leaves nothing installed and nothing waiting. #[test] fn a_failed_install_picked_up_at_exit_leaves_nothing_waiting() { From 53fbd5b6369f10530fbcc37d38ba0f6076985a69 Mon Sep 17 00:00:00 2001 From: dotCooCoo Date: Sat, 7 Nov 2026 19:44:54 -0800 Subject: [PATCH 13/25] craft-update: a retry after a failed helper waits for the exit and says why A new install worker after start_pending_update gets a fresh handoff flag, so a retry after a helper that failed while the app kept running reports through tick and starts its helper at the exit, instead of at once. Updater::offer carries the reason of a failed update into its prompt, as a check does. The outcome records are read only from plain files: a link is not followed, as is_plain_file already does for the files it moves. The tests wait for a check's answer on its channel instead of polling with sleeps. --- crates/update/src/self_update.rs | 31 ++++++++-- crates/update/src/updater.rs | 101 ++++++++++++++++++++++++++----- 2 files changed, 114 insertions(+), 18 deletions(-) diff --git a/crates/update/src/self_update.rs b/crates/update/src/self_update.rs index 63d69eab..619f6577 100644 --- a/crates/update/src/self_update.rs +++ b/crates/update/src/self_update.rs @@ -553,10 +553,10 @@ fn scheduled_tag(paths: &Paths) -> Option { const RECORD_LIMIT: usize = 4096; /// The text of the record at `path`, at most [`RECORD_LIMIT`] bytes of it. `None` when it is -/// missing, unreadable or not a plain file. +/// missing, unreadable or not a plain file (a link is not followed). fn read_record(path: &Path) -> Option { use std::io::Read; - if !std::fs::metadata(path).is_ok_and(|m| m.is_file()) { + if !is_plain_file(path) { return None; } let mut bytes = Vec::new(); @@ -717,7 +717,6 @@ fn stage(from: &Path, to: &Path, executable: bool) -> Result { } /// A regular file, not a link to one. -#[cfg(unix)] fn is_plain_file(path: &Path) -> bool { std::fs::symlink_metadata(path).is_ok_and(|m| m.file_type().is_file()) } @@ -1011,7 +1010,7 @@ pub(crate) fn record_failure(paths: &Paths, error: &str) { pub fn take_failure(paths: &Paths) -> Option { let file = failure_file(paths); let error = read_record(&file); - if std::fs::metadata(&file).is_ok_and(|m| m.is_file()) { + if is_plain_file(&file) { let _ = std::fs::remove_file(&file); } error @@ -1685,6 +1684,30 @@ mod tests { assert!(failure_file(&paths).is_dir()); } + /// A link where a record goes is not followed: neither record is read through it, and the + /// file it points to stays. Skipped where the system refuses to create the link (Windows + /// without the privilege for symbolic links). + #[test] + fn a_record_that_is_a_link_is_not_read() { + let dir = tempfile::tempdir().unwrap(); + let paths = Paths::under(dir.path()); + std::fs::create_dir_all(paths.logs()).unwrap(); + std::fs::create_dir_all(&paths.data).unwrap(); + let elsewhere = dir.path().join("elsewhere.txt"); + std::fs::write(&elsewhere, "v0.2.0").unwrap(); + #[cfg(unix)] + let link = |to: &Path, at: &Path| std::os::unix::fs::symlink(to, at); + #[cfg(windows)] + let link = |to: &Path, at: &Path| std::os::windows::fs::symlink_file(to, at); + if let Err(e) = link(&elsewhere, &failure_file(&paths)).and_then(|()| link(&elsewhere, &done_file(&paths))) { + eprintln!("skipped: couldn't create a symbolic link ({e})"); + return; + } + assert_eq!(take_failure(&paths), None); + assert_eq!(take_outcome(&paths, "0.2.0"), None); + assert_eq!(std::fs::read_to_string(&elsewhere).unwrap(), "v0.2.0"); + } + /// Scheduling a new update clears the outcome records of an earlier one, so a stale failure /// is not reported after it. #[test] diff --git a/crates/update/src/updater.rs b/crates/update/src/updater.rs index fe4bf914..0e3bb20d 100644 --- a/crates/update/src/updater.rs +++ b/crates/update/src/updater.rs @@ -206,6 +206,16 @@ impl Updater { StartupPrompt::new(self.app, release, self.version.clone()).with_translator(self.translate) } + /// Opens the prompt offering `release`. A failed update offered again says why the last + /// attempt failed, once. + fn open_offer(&mut self, release: Release) { + let mut prompt = self.prompt_for(release); + if let Some(reason) = self.failed_reason.take() { + prompt.set_note(Note::new("The update could not be installed: {reason}", &[("reason", &reason)])); + } + self.prompt = Some(prompt); + } + /// [`READY_TEXT`], translated when the prompt draws. fn installed_note(&self) -> Note { Note::new(READY_TEXT, &[("app", self.app.name)]) @@ -302,10 +312,11 @@ impl Updater { self.close_wanted = true; } - /// Opens the startup prompt for a release the app found itself. + /// Opens the startup prompt for a release the app found itself. After a failed update the + /// prompt says why it failed, like the one a check opens. pub fn offer(&mut self, release: Release) { if self.install.is_none() && !self.installed && !self.read_choice() { - self.prompt = Some(self.prompt_for(release)); + self.open_offer(release); } } @@ -409,12 +420,7 @@ impl Updater { match answer { Ok(Some(release)) => { if self.prompt.is_none() && self.install.is_none() && !self.installed { - let mut prompt = self.prompt_for(release); - // A failed update offered again says why the last attempt failed. - if let Some(reason) = self.failed_reason.take() { - prompt.set_note(Note::new("The update could not be installed: {reason}", &[("reason", &reason)])); - } - self.prompt = Some(prompt); + self.open_offer(release); } self.status = Status::Idle; } @@ -608,7 +614,7 @@ impl Updater { let (tx, rx) = channel(); let (app, version, ctx, release) = (self.app, self.version.clone(), ctx.clone(), prompt.release.clone()); let worker_release = release.clone(); - let dropped = Arc::clone(&self.dropped); + let dropped = handoff(&mut self.dropped); let spawned = std::thread::Builder::new().name("self-update".into()).spawn(move || { let run = || install(app, &installed, worker_release, &version).map_err(Failure::Installer); let outcome = std::panic::catch_unwind(std::panic::AssertUnwindSafe(run)).unwrap_or_else(|_| Err(Failure::panicked())); @@ -633,6 +639,17 @@ impl Drop for Updater { } } +/// The flag a new install worker sends its outcome under (the `Updater`'s `dropped`). After +/// [`Updater::start_pending_update`] set it while the app kept running (a helper that then failed +/// puts the update on offer again), a new flag replaces it, so that the new worker reports +/// through `tick` instead of starting its helper at once. A worker let go before keeps the old one. +fn handoff(dropped: &mut Arc>) -> Arc> { + if *dropped.lock().unwrap_or_else(PoisonError::into_inner) { + *dropped = Arc::new(Mutex::new(false)); + } + Arc::clone(dropped) +} + /// Sends the install worker's outcome to the [`Updater`]. When the `Updater` has been dropped /// (the app is exiting), the worker starts a finished installation's helper itself, since /// nothing will start it at a close. A helper that cannot start, or a failed installation, is @@ -691,13 +708,19 @@ mod tests { std::env::temp_dir().join("craft-update-tests-no-such-dir").join("versions.json") } - /// Ticks until the running check has reported. + /// Waits for the running check's answer on its channel (its thread always sends one, or ends + /// and closes the channel), then hands it to `tick` as a frame would. pub(super) fn settle(updater: &mut Updater, ctx: &egui::Context) { - let deadline = std::time::Instant::now() + Duration::from_secs(10); - while updater.check.is_some() && std::time::Instant::now() < deadline { - updater.tick(ctx); - std::thread::sleep(Duration::from_millis(5)); + let Some(rx) = updater.check.take() else { return }; + let (tx, answer) = channel(); + if let Ok(checked) = rx.recv() { + assert!(tx.send(checked).is_ok()); } + // Without an answer `tx` is dropped here, and `tick` sees a check that stopped. + drop(tx); + updater.check = Some(answer); + updater.tick(ctx); + assert!(updater.check.is_none(), "the check has reported"); } /// #391: Download opens only a safe web link, whoever built the `Release`. @@ -1435,6 +1458,56 @@ mod tests { assert_eq!(u.prompt.as_ref().and_then(StartupPrompt::message), None, "the reason is shown once"); } + /// A retry after a helper failed while the app kept running reports through `tick` and waits + /// for the exit, like the first installation, instead of starting its helper at once. + #[test] + fn a_retry_after_a_helper_failure_while_the_app_runs_waits_for_the_exit() { + use std::sync::atomic::Ordering; + let ctx = egui::Context::default(); + let (_dir, paths, mut u) = with_records("0.1.0"); + u.offer(newer()); + let (tx, rx) = channel(); + u.install = Some((rx, newer())); + assert!(tx.send(Ok(Restart::scheduled())).is_ok()); + u.tick(&ctx); + u.start_pending_update(); + self_update::record_failure(&paths, "ArtCraft Launcher is still running"); + u.check_now(&ctx); + assert_eq!(u.status(), &Status::InstallFailed("ArtCraft Launcher is still running".into())); + + // The retry's worker, as `start_install` starts it. + let dropped = handoff(&mut u.dropped); + let (starts, restart) = counted_restart(); + let (tx, rx) = channel(); + u.install = Some((rx, newer())); + deliver(&PHOTOCRAFT, &tx, &dropped, Some(&paths), Ok(restart)); + assert_eq!(starts.load(Ordering::SeqCst), 0, "the helper waits for the exit"); + u.tick(&ctx); + assert!(u.installed && u.restart.is_some(), "tick picks up the retry"); + assert_eq!(frame(&mut u, &ctx, true), Action::Close); + drop(u); + assert_eq!(starts.load(Ordering::SeqCst), 1, "the exit starts the helper once"); + let mut next = reading(&paths, "0.1.0"); + start_answered(&mut next, &ctx, Ok(None)); + assert_eq!(next.status(), &Status::Idle, "the retry is not recorded as a failure"); + } + + /// A release the app offers itself after a failed update says in its prompt why it failed, + /// once. + #[test] + fn an_offer_after_a_failed_update_says_why_it_failed() { + let ctx = egui::Context::default(); + let (_dir, paths, mut u) = with_records("0.1.0"); + self_update::record_failure(&paths, "disk full"); + start_answered(&mut u, &ctx, Ok(None)); + assert_eq!(u.status(), &Status::InstallFailed("disk full".into())); + u.offer(newer()); + assert_eq!(u.prompt.as_ref().and_then(StartupPrompt::message).as_deref(), Some("The update could not be installed: disk full")); + u.prompt = None; + u.offer(newer()); + assert_eq!(u.prompt.as_ref().and_then(StartupPrompt::message), None, "the reason is shown once"); + } + /// An installation that finishes after the updater was dropped and whose helper cannot start, /// or that failed, is recorded, and the next start says why. #[test] From 75eacfc2a8cb7025f4a8a27d7f873d3b713faefe Mon Sep 17 00:00:00 2001 From: dotCooCoo Date: Sat, 7 Nov 2026 19:50:28 -0800 Subject: [PATCH 14/25] craft-update: an offer after a failed update clears the failure status Updater::offer opens a prompt that carries the failed update's reason, and now sets the status to Idle, as a check that finds a release does. An app that shows CheckResultDialog while the status is not Idle no longer shows the failure card and the prompt at once. --- crates/update/src/updater.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/crates/update/src/updater.rs b/crates/update/src/updater.rs index 0e3bb20d..1cfbe62e 100644 --- a/crates/update/src/updater.rs +++ b/crates/update/src/updater.rs @@ -313,10 +313,12 @@ impl Updater { } /// Opens the startup prompt for a release the app found itself. After a failed update the - /// prompt says why it failed, like the one a check opens. + /// prompt says why it failed, like the one a check opens. Opening the prompt sets + /// [`Self::status`] to [`Status::Idle`]. pub fn offer(&mut self, release: Release) { if self.install.is_none() && !self.installed && !self.read_choice() { self.open_offer(release); + self.status = Status::Idle; } } @@ -1503,6 +1505,7 @@ mod tests { assert_eq!(u.status(), &Status::InstallFailed("disk full".into())); u.offer(newer()); assert_eq!(u.prompt.as_ref().and_then(StartupPrompt::message).as_deref(), Some("The update could not be installed: disk full")); + assert_eq!(u.status(), &Status::Idle, "the prompt carries the reason"); u.prompt = None; u.offer(newer()); assert_eq!(u.prompt.as_ref().and_then(StartupPrompt::message), None, "the reason is shown once"); From 10d1cbb4abc56000194594a52ed651f400246e5f Mon Sep 17 00:00:00 2001 From: dotCooCoo Date: Sat, 7 Nov 2026 19:58:28 -0800 Subject: [PATCH 15/25] craft-update: a check that answers during an installation reports the installation A requested check still running when the user starts the installation from the prompt answered with its own result once it finished, which replaced Installing or Ready with UpToDate, the check error or nothing. Its answer is now logged, and a requested check reports Installing, Ready or the recorded helper failure, as a check started during the installation does. --- crates/update/src/updater.rs | 50 ++++++++++++++++++++++++++++++++++++ 1 file changed, 50 insertions(+) diff --git a/crates/update/src/updater.rs b/crates/update/src/updater.rs index 1cfbe62e..da174874 100644 --- a/crates/update/src/updater.rs +++ b/crates/update/src/updater.rs @@ -408,6 +408,19 @@ impl Updater { /// the outcome of the last update ([`Self::read_outcome`]). fn finish_check(&mut self, answer: Checked) { let requested = std::mem::take(&mut self.requested); + // A check that was running when an installation started answers with the installation's + // state: the update installing, ready, or the recorded helper failure. + if self.install.is_some() || self.installed { + match &answer { + Ok(Some(release)) => log::info!("{} update check found {} while an update installs", self.app.name, release.tag), + Ok(None) => log::info!("{} update check found nothing newer while an update installs", self.app.name), + Err(e) => log::warn!("{} update check failed while an update installs: {e}", self.app.name), + } + if requested { + self.status = self.pending_status(); + } + return; + } // The launcher may have pinned or unpinned the app while the check ran: whatever the // answer, an app it manages offers nothing and says so. if self.read_choice() { @@ -1409,6 +1422,43 @@ mod tests { } } + /// A requested check that answers after an installation started reports the installation, + /// not its own answer, whether the update is still installing or already waits for the close. + #[test] + fn a_check_answering_during_an_update_reports_the_update() { + let ctx = egui::Context::default(); + for answer in [Ok(None), Err("offline".to_owned()), Ok(Some(newer()))] { + let mut u = updater(Ok(None)); + u.offer(newer()); + u.check_now(&ctx); + assert_eq!(u.status(), &Status::Checking); + let (check_tx, check_rx) = channel(); + u.check = Some(check_rx); + let (tx, rx) = channel(); + u.install = Some((rx, newer())); + assert!(check_tx.send(answer.clone()).is_ok()); + u.tick(&ctx); + assert_eq!(u.status(), &Status::Installing, "a check answering {answer:?} while installing"); + assert!(tx.send(Ok(Restart::scheduled())).is_ok()); + u.tick(&ctx); + assert_eq!(u.status(), &Status::Ready, "after {answer:?}"); + + let mut u = updater(Ok(None)); + u.offer(newer()); + u.check_now(&ctx); + let (check_tx, check_rx) = channel(); + u.check = Some(check_rx); + let (tx, rx) = channel(); + u.install = Some((rx, newer())); + assert!(tx.send(Ok(Restart::scheduled())).is_ok()); + u.tick(&ctx); + assert!(u.check.is_some(), "the check is still running"); + assert!(check_tx.send(answer.clone()).is_ok()); + u.tick(&ctx); + assert_eq!(u.status(), &Status::Ready, "a check answering {answer:?} after the install finished"); + } + } + /// A failed installation closes a result window showing it; the prompt says why. #[test] fn a_failed_install_ends_the_installing_status() { From 8ecf7fa15d18db22e5b7e2540efaff660a3bf1f4 Mon Sep 17 00:00:00 2001 From: dotCooCoo Date: Sat, 7 Nov 2026 20:34:29 -0800 Subject: [PATCH 16/25] craft-update: self-update tests wait on handshakes instead of time The Windows helper tests hand step_after_wait a wait that reports when the helper reaches it and blocks until the test has reaped the stand-in launcher, so the old-file check no longer depends on a 500 ms sleep or a 2 s observation window. The test-only wait_watch hooks are removed. The launcher-that-keeps-running test asks the real wait with no timeout instead of bounding the elapsed time. The Unix restart test puts a sleep script on the helper's PATH that reports each pause through a named pipe, and the started command reports whether the old process was still running. A thread that waits for the helper to exit writes to the same pipe, so a helper that dies early fails the test instead of hanging it. --- crates/update/src/self_update.rs | 255 +++++++++++++++---------------- 1 file changed, 119 insertions(+), 136 deletions(-) diff --git a/crates/update/src/self_update.rs b/crates/update/src/self_update.rs index 619f6577..3b4d9cf8 100644 --- a/crates/update/src/self_update.rs +++ b/crates/update/src/self_update.rs @@ -1112,55 +1112,6 @@ fn check_helper(app: &App, me: &Path, paths: &Paths, exe: &Path, step: &Finish) if allowed { Ok(()) } else { Err(refused()) } } -/// Lets a Windows test observe both sides of `run_step`'s wait for the old launcher. Compiled only -/// for tests, so production behavior is unchanged. -#[cfg(all(test, windows))] -mod wait_watch { - use std::sync::mpsc::{self, Receiver, Sender}; - use std::sync::{Mutex, PoisonError}; - - /// A test's two observations of one wait, keyed by the launcher's pid. - pub(super) struct Channels { - /// Sent just before `run_step` calls the wait. - pub(super) entered: Receiver<()>, - /// Sent right after the wait returns, before any extraction, and carries the wait's result. - pub(super) returned: Receiver, - } - - struct Watch { - pid: u32, - entered: Option>, - returned: Option>, - } - - static WATCH: Mutex> = Mutex::new(None); - - pub(super) fn watch(pid: u32) -> Channels { - let (entered, entered_rx) = mpsc::channel(); - let (returned, returned_rx) = mpsc::channel(); - *WATCH.lock().unwrap_or_else(PoisonError::into_inner) = Some(Watch { pid, entered: Some(entered), returned: Some(returned) }); - Channels { entered: entered_rx, returned: returned_rx } - } - - /// Notes that `run_step` is about to wait. Only the first call for a watched pid is kept. - pub(super) fn entered(pid: u32) { - let mut watch = WATCH.lock().unwrap_or_else(PoisonError::into_inner); - let sender = watch.as_mut().filter(|watch| watch.pid == pid).and_then(|watch| watch.entered.take()); - if let Some(sender) = sender { - let _ = sender.send(()); - } - } - - /// Notes the wait's result as soon as it returns; the step hasn't run yet. - pub(super) fn returned(pid: u32, waited: bool) { - let mut watch = WATCH.lock().unwrap_or_else(PoisonError::into_inner); - let sender = watch.as_mut().filter(|watch| watch.pid == pid).and_then(|watch| watch.returned.take()); - if let Some(sender) = sender { - let _ = sender.send(waited); - } - } -} - /// [`step_after_wait`], waiting up to `wait` for `pid` to exit. #[cfg(all(windows, test))] fn run_step(app: &App, reference: &Path, pid: u32, exe: &Path, step: &Finish, wait: std::time::Duration) -> Result<(), String> { @@ -1180,12 +1131,7 @@ struct Stepped { /// `reference`. #[cfg(windows)] fn step_after_wait(app: &App, reference: &Path, pid: u32, exe: &Path, step: &Finish, wait: &dyn Fn(u32) -> bool) -> Stepped { - #[cfg(test)] - wait_watch::entered(pid); - // The test hook needs the result, so it is stored rather than consumed by the branch. let waited = wait(pid); - #[cfg(test)] - wait_watch::returned(pid, waited); let result = if waited { match step { Finish::Msi(msi) => { @@ -1364,18 +1310,23 @@ mod tests { let tmp = dir.path().join("cache").join("launcher-update"); let step = Finish::Portable { zip: zip.clone(), tmp: tmp.clone() }; - let mut old = std::process::Command::new("ping").args(["-n", "3", "127.0.0.1"]).stdout(std::process::Stdio::null()).spawn().unwrap(); + let mut old = BlockedLauncher::start(); let exe = folder.join("artcraft-launcher.exe"); - let helper = { - let (exe, step, pid) = (exe.clone(), step.clone(), old.id()); - std::thread::spawn(move || { - run_step(&crate::suite::LAUNCHER, &system_program("cmd.exe"), pid, &exe, &step, std::time::Duration::from_secs(30)) - }) - }; - std::thread::sleep(std::time::Duration::from_millis(500)); - assert_eq!(std::fs::read_to_string(&exe).unwrap(), "0.1.0", "replaced while the old launcher was still running"); - old.wait().unwrap(); - helper.join().unwrap().unwrap(); + std::thread::scope(|scope| { + // Inside the scope, so a failed assertion drops the gate and releases the helper + // before the scope joins it. + let (gate, wait) = WaitGate::new(); + let (helper_exe, pid) = (exe.clone(), old.id()); + let helper = scope.spawn(move || step_after_wait(&crate::suite::LAUNCHER, &system_program("cmd.exe"), pid, &helper_exe, &step, &wait)); + gate.entered(); + old.assert_running("while the helper was waiting"); + assert_eq!(std::fs::read(&exe).unwrap(), b"0.1.0", "replaced while the old launcher was still running"); + old.finish().expect("the stand-in launcher couldn't be reaped"); + gate.release(); + let stepped = helper.join().unwrap(); + assert!(stepped.exited, "the wait reported that the launcher was still running although it had exited"); + stepped.result.unwrap(); + }); assert_eq!(std::fs::read(&exe).unwrap(), signed_exe); assert_eq!(std::fs::read_to_string(folder.join("ArtCraftLauncherData").join("settings.json")).unwrap(), "{}"); assert!(!folder.join("artcraft-launcher-0.2.0-windows-x64-portable").exists(), "the zip's folder must not be nested in the launcher's"); @@ -1434,20 +1385,52 @@ mod tests { } } + /// A wait for [`step_after_wait`] that the test controls: it reports when the helper has + /// reached its wait, blocks until the test releases it, and then asks the real + /// [`process::wait_for_exit`] whether the launcher has exited, without waiting. + #[cfg(windows)] + struct WaitGate { + entered: std::sync::mpsc::Receiver<()>, + release: std::sync::mpsc::Sender<()>, + } + + #[cfg(windows)] + impl WaitGate { + /// The gate and the wait to hand to the helper. The wait holds the only sender of + /// `entered`, so a helper that ends without reaching its wait fails [`WaitGate::entered`] + /// instead of blocking it, and dropping the gate releases the wait. + fn new() -> (Self, impl Fn(u32) -> bool + Send) { + let (entered_tx, entered) = std::sync::mpsc::channel(); + let (release, released) = std::sync::mpsc::channel::<()>(); + let released = std::sync::Mutex::new(released); + let wait = move |pid| { + let _ = entered_tx.send(()); + let _ = released.lock().map(|released| released.recv()); + process::wait_for_exit(pid, std::time::Duration::ZERO) + }; + (Self { entered, release }, wait) + } + + /// Blocks until the helper has reached its wait. + fn entered(&self) { + self.entered.recv().expect("the helper never reached its wait"); + } + + /// Lets the helper's wait return. + fn release(&self) { + self.release.send(()).expect("the helper stopped before its wait was released"); + } + } + /// A launcher without `portable.txt` is a standalone installation, and the helper still /// replaces its own executable after it exits. The test holds a stand-in launcher alive - /// through a pipe and observes both sides of the helper's wait: the first assertion runs only - /// after the helper announces it is about to wait. A wait that returns while the stand-in is alive - /// fails the test before any extraction can replace the executable. - /// - /// The "still blocked" check is a bounded observation, not proof: a helper descheduled for the - /// whole window could hide a missing wait until the release below. A premature timeout longer - /// than this observation window is also outside this test's coverage. + /// through a pipe and holds the helper inside its wait until the stand-in has exited, so the + /// executable must still be the old one while the helper waits, and the real + /// [`process::wait_for_exit`] must report the live stand-in as running and the exited one as gone. #[cfg(windows)] #[test] fn a_standalone_update_replaces_the_launcher_after_it_exits() { use std::io::Write; - use std::sync::mpsc::RecvTimeoutError; let dir = tempfile::tempdir().unwrap(); let folder = dir.path().join("ArtCraft Launcher"); std::fs::create_dir_all(folder.join("ArtCraftLauncherData")).unwrap(); @@ -1468,49 +1451,23 @@ mod tests { let tmp = dir.path().join("cache").join(PORTABLE_STAGING); let step = Finish::Portable { zip: zip.clone(), tmp: tmp.clone() }; - // Timeouts only bound the test; each failure names the observation that never arrived. - let entered_timeout = std::time::Duration::from_secs(10); - let blocked_window = std::time::Duration::from_secs(2); - let returned_timeout = std::time::Duration::from_secs(30); - + let mut old = BlockedLauncher::start(); + let pid = old.id(); std::thread::scope(|scope| { - let mut old = BlockedLauncher::start(); - let pid = old.id(); - let watch = wait_watch::watch(pid); + // Inside the scope, so a failed assertion drops the gate and releases the helper + // before the scope joins it. + let (gate, wait) = WaitGate::new(); let helper_exe = exe.clone(); - let step = step.clone(); - let helper = scope.spawn(move || { - run_step(&crate::suite::LAUNCHER, &system_program("cmd.exe"), pid, &helper_exe, &step, std::time::Duration::from_secs(30)) - }); - match watch.entered.recv_timeout(entered_timeout) { - Ok(()) => {} - Err(RecvTimeoutError::Timeout) => { - panic!("the helper didn't reach its wait within {entered_timeout:?} (helper already finished: {})", helper.is_finished()) - } - Err(RecvTimeoutError::Disconnected) => panic!("the entered channel disconnected before the helper reached its wait"), - } - old.assert_running("before the first replacement check"); - assert_eq!(std::fs::read_to_string(&exe).unwrap(), "0.1.0", "replaced while the old launcher was still running"); - - // A correct wait can't return while the stand-in holds the pipe. - match watch.returned.recv_timeout(blocked_window) { - Err(RecvTimeoutError::Timeout) => {} - Ok(waited) => panic!("run_step left its wait while the old launcher was still running (waited: {waited})"), - Err(RecvTimeoutError::Disconnected) => panic!("the returned channel disconnected while the old launcher was still running"), - } - old.assert_running("after the wait was expected to stay blocked"); - assert_eq!(std::fs::read_to_string(&exe).unwrap(), "0.1.0", "replaced while the old launcher was still running"); - + let helper = scope.spawn(move || step_after_wait(&crate::suite::LAUNCHER, &system_program("cmd.exe"), pid, &helper_exe, &step, &wait)); + gate.entered(); + old.assert_running("while the helper was waiting"); + assert_eq!(std::fs::read(&exe).unwrap(), b"0.1.0", "replaced while the old launcher was still running"); + assert!(!process::wait_for_exit(pid, std::time::Duration::ZERO), "the wait reported the running launcher as exited"); old.finish().expect("the stand-in launcher couldn't be reaped"); - match watch.returned.recv_timeout(returned_timeout) { - Ok(waited) => assert!(waited, "the wait reported that the launcher was still running although it had exited"), - Err(RecvTimeoutError::Timeout) => panic!( - "the helper didn't leave its wait within {returned_timeout:?} after the launcher exited (helper already finished: {})", - helper.is_finished() - ), - Err(RecvTimeoutError::Disconnected) => panic!("the returned channel disconnected before reporting the wait's result"), - } - helper.join().unwrap().unwrap(); + gate.release(); + let stepped = helper.join().unwrap(); + assert!(stepped.exited, "the wait reported that the launcher was still running although it had exited"); + stepped.result.unwrap(); }); assert_eq!(std::fs::read(&exe).unwrap(), signed_exe); assert_eq!(std::fs::read_to_string(folder.join("ArtCraftLauncherData").join("settings.json")).unwrap(), "{}"); @@ -1730,19 +1687,11 @@ mod tests { let msi = dir.path().join("artcraft-launcher-0.2.0-windows-x64.msi"); std::fs::write(&msi, "not a package").unwrap(); let running = std::process::id(); - let started = std::time::Instant::now(); - // The test process itself never exits within the wait, so the package is never run. - let error = run_step( - &crate::suite::LAUNCHER, - &system_program("cmd.exe"), - running, - &exe, - &Finish::Msi(msi.clone()), - std::time::Duration::from_millis(300), - ) - .unwrap_err(); + // The test process itself is still running when the wait asks, so the package is never run. + let error = + run_step(&crate::suite::LAUNCHER, &system_program("cmd.exe"), running, &exe, &Finish::Msi(msi.clone()), std::time::Duration::ZERO) + .unwrap_err(); assert!(error.contains("still running"), "{error}"); - assert!(started.elapsed() < std::time::Duration::from_secs(30)); assert_eq!(std::fs::read_to_string(&exe).unwrap(), "0.1.0"); assert!(!msi.exists()); } @@ -1977,22 +1926,56 @@ mod tests { } } - /// The helper waits for the old launcher to exit before it starts the new one. + /// The helper waits for the old launcher to exit before it starts the new one. The helper's + /// `sleep` is a script on `PATH` that reports each pause through a named pipe and pauses until + /// the test releases it, and the started command reports whether the old launcher was still + /// running. The test holds both pipes open for reading and writing, so no open blocks, and a + /// thread that waits for the helper to exit writes `exited`, so every read gets a line. #[cfg(unix)] #[test] fn the_restart_waits_for_the_old_process_to_exit() { + use std::io::{BufRead, Write}; + use std::os::unix::fs::PermissionsExt; let dir = tempfile::tempdir().unwrap(); - let marker = dir.path().join("started"); - let mut old = std::process::Command::new("sleep").arg("1").spawn().unwrap(); - relaunch_after_exit(&crate::suite::LAUNCHER, old.id(), &[std::ffi::OsStr::new("touch"), marker.as_os_str()]).unwrap(); - std::thread::sleep(std::time::Duration::from_millis(400)); - assert!(!marker.exists(), "started while the old launcher was still running"); - old.wait().unwrap(); - let deadline = std::time::Instant::now() + std::time::Duration::from_secs(5); - while !marker.exists() && std::time::Instant::now() < deadline { - std::thread::sleep(std::time::Duration::from_millis(50)); + let (events, release) = (dir.path().join("events"), dir.path().join("release")); + for fifo in [&events, &release] { + assert!(std::process::Command::new("mkfifo").arg(fifo).status().unwrap().success()); } - assert!(marker.exists(), "never started after the old launcher exited"); + let bin = dir.path().join("bin"); + std::fs::create_dir(&bin).unwrap(); + let sleep = bin.join("sleep"); + std::fs::write(&sleep, "#!/bin/sh\nprintf 'waiting\\n' > \"$CRAFT_TEST_EVENTS\"\nread _ < \"$CRAFT_TEST_RELEASE\"\n").unwrap(); + std::fs::set_permissions(&sleep, std::fs::Permissions::from_mode(0o755)).unwrap(); + let mut path = std::ffi::OsString::from(bin.as_os_str()); + if let Some(rest) = std::env::var_os("PATH") { + path.push(":"); + path.push(rest); + } + + let mut old = std::process::Command::new("cat").stdin(std::process::Stdio::piped()).stdout(std::process::Stdio::null()).spawn().unwrap(); + let pid = old.id().to_string(); + let started = r#"if kill -0 "$1" 2>/dev/null; then s=alive; else s=gone; fi; printf 'started %s\n' "$s" > "$CRAFT_TEST_EVENTS""#; + let command = [std::ffi::OsStr::new("/bin/sh"), "-c".as_ref(), started.as_ref(), "sh".as_ref(), pid.as_ref()]; + let open = |fifo: &Path| std::fs::OpenOptions::new().read(true).write(true).open(fifo).unwrap(); + let (events_pipe, mut release_pipe) = (open(&events), open(&release)); + let mut exited = events_pipe.try_clone().unwrap(); + let mut lines = std::io::BufReader::new(events_pipe).lines(); + let mut next = move || lines.next().unwrap().unwrap(); + + let mut helper = helper_command(&crate::suite::LAUNCHER, old.id(), &command); + helper.env("PATH", path).env("CRAFT_TEST_EVENTS", &events).env("CRAFT_TEST_RELEASE", &release); + let mut helper = helper.stdin(std::process::Stdio::null()).stdout(std::process::Stdio::null()).spawn().unwrap(); + std::thread::spawn(move || { + let _ = helper.wait(); + let _ = exited.write_all(b"exited\n"); + }); + + assert_eq!(next(), "waiting", "the helper didn't wait while the old launcher was still running"); + drop(old.stdin.take()); + old.wait().unwrap(); + release_pipe.write_all(b"go\n").unwrap(); + assert_eq!(next(), "started gone", "didn't start once the old launcher had exited"); + assert_eq!(next(), "exited"); } /// A real disk image: the new `ArtCraft Launcher.app` replaces the old one in place. From 4671e213a21ca27ebd7a9b373b2b8af528cdd4f6 Mon Sep 17 00:00:00 2001 From: dotCooCoo Date: Sat, 7 Nov 2026 21:53:25 -0800 Subject: [PATCH 17/25] craft-update: restart helpers start at the exit and outcomes reach the new app - On macOS and Linux the restart helper starts at the exit too (Restart::start), instead of when the update is prepared. A close the user cancels no longer leaves a helper that starts a second copy after its one-minute wait, or a Flatpak helper that gives up before Restart to Update. - The record of a finished update names the release's version instead of its tag, so a release whose tag is not a version is still reported. - A standalone Windows copy updated from the portable zip becomes portable. The helper now writes the outcome and the record of the update to the folders beside it, which the new app reads. - When the check at start finds a release for a prompt the app already opened with Updater::offer, that prompt shows why the last update failed. - Errors and failure reasons reach 4.5:1 contrast on the card and in the prompt's message box in both themes, and a long one scrolls inside a 160 px area so the card's buttons stay on screen. --- crates/update/src/dialog.rs | 89 +++++++++++-- crates/update/src/self_update.rs | 206 +++++++++++++++++++++++++------ crates/update/src/updater.rs | 37 ++++-- 3 files changed, 279 insertions(+), 53 deletions(-) diff --git a/crates/update/src/dialog.rs b/crates/update/src/dialog.rs index 5f471d47..5db9e1c8 100644 --- a/crates/update/src/dialog.rs +++ b/crates/update/src/dialog.rs @@ -106,7 +106,7 @@ impl StartupPrompt { } let pal = Palette::new(ctx, self.app); - let Palette { is_dark, card_bg, border_color, text_primary, text_muted, fill, on_fill } = pal; + let Palette { card_bg, border_color, text_primary, text_muted, fill, on_fill, alert, alert_bg, .. } = pal; let t = self.translate; let version = self.release.version_label(); @@ -130,16 +130,16 @@ impl StartupPrompt { ui.add_space(8.0); if let Some(msg) = self.message() { - let alert_bg = if is_dark { Color32::from_rgb(50, 30, 20) } else { Color32::from_rgb(255, 242, 235) }; - let alert_color = Color32::from_rgb(225, 95, 30); egui::Frame::new() .fill(alert_bg) - .stroke(egui::Stroke::new(1.0, alert_color)) + .stroke(egui::Stroke::new(1.0, alert)) .corner_radius(egui::CornerRadius::ZERO) .inner_margin(egui::Margin::symmetric(10, 6)) .show(ui, |ui| { ui.set_width(ui.available_width()); - ui.label(RichText::new(msg).size(12.0).color(alert_color)); + bounded(ui, "craft_update.prompt_message", |ui| { + ui.label(RichText::new(msg).size(12.0).color(alert)); + }); }); ui.add_space(4.0); } @@ -247,6 +247,10 @@ struct Palette { fill: Color32, /// Text on `fill`. on_fill: Color32, + /// Errors and failure reasons, on the card and on `alert_bg`. + alert: Color32, + /// The box around a message in the prompt. + alert_bg: Color32, } impl Palette { @@ -261,6 +265,8 @@ impl Palette { text_muted: if is_dark { Color32::from_rgb(150, 150, 160) } else { Color32::from_rgb(105, 105, 115) }, fill, on_fill: ink_on(fill), + alert: if is_dark { Color32::from_rgb(225, 95, 30) } else { Color32::from_rgb(184, 66, 8) }, + alert_bg: if is_dark { Color32::from_rgb(44, 26, 18) } else { Color32::from_rgb(255, 242, 235) }, } } } @@ -410,7 +416,7 @@ impl CheckResultDialog { } let pal = Palette::new(ctx, self.app); - let Palette { card_bg, border_color, text_primary, text_muted, fill, on_fill, .. } = pal; + let Palette { card_bg, border_color, text_primary, text_muted, fill, on_fill, alert, .. } = pal; let t = self.translate; egui::Area::new(egui::Id::unique("craft_update.check_result")).anchor(Align2::CENTER_CENTER, Vec2::ZERO).order(egui::Order::Foreground).show( ctx, @@ -428,7 +434,6 @@ impl CheckResultDialog { } ui.add_space(8.0); - let alert_color = Color32::from_rgb(225, 95, 30); match status { Status::Checking => { ui.horizontal(|ui| { @@ -459,7 +464,9 @@ impl CheckResultDialog { } Status::InstallFailed(reason) => { let text = t("The update could not be installed: {reason}", &[("reason", reason)]); - ui.add(egui::Label::new(RichText::new(text).size(12.5).color(alert_color)).selectable(true)); + bounded(ui, "craft_update.install_failure", |ui| { + ui.add(egui::Label::new(RichText::new(text).size(12.5).color(alert)).selectable(true)); + }); } Status::UpToDate => { let text = t( @@ -469,8 +476,10 @@ impl CheckResultDialog { ui.label(RichText::new(text).size(12.5).color(text_muted)); } Status::Failed(error) => { - ui.label(RichText::new(t("Could not check for updates.", &[])).size(12.5).strong().color(alert_color)); - ui.add(egui::Label::new(RichText::new(error).size(12.0).color(text_muted)).selectable(true)); + ui.label(RichText::new(t("Could not check for updates.", &[])).size(12.5).strong().color(alert)); + bounded(ui, "craft_update.check_failure", |ui| { + ui.add(egui::Label::new(RichText::new(error).size(12.0).color(text_muted)).selectable(true)); + }); ui.label( RichText::new(t("All releases are listed at {url}", &[("url", &self.app.releases_url())])) .size(11.5) @@ -520,6 +529,16 @@ impl CheckResultDialog { } } +/// The tallest an error or failure reason draws in a card; a longer one scrolls, so the card's +/// buttons stay on screen. +const REASON_MAX_HEIGHT: f32 = 160.0; + +/// Draws `add_contents` (an error or a failure reason) at most [`REASON_MAX_HEIGHT`] tall, +/// scrolling the rest. +fn bounded(ui: &mut egui::Ui, id: &str, add_contents: impl FnOnce(&mut egui::Ui)) { + egui::ScrollArea::vertical().id_salt(id).max_height(REASON_MAX_HEIGHT).auto_shrink([false, true]).show(ui, add_contents); +} + /// Backgrounds of an unfilled button (LATER, DISMISS) when hovered and when pressed: a step from /// the card, so its text keeps the contrast it has at rest. fn button_states(is_dark: bool) -> (Color32, Color32) { @@ -592,7 +611,7 @@ mod tests { use egui::{Context, Event, FullOutput, Key, Modifiers, RawInput, Rect}; const DARK_CARD: Color32 = Color32::from_rgb(26, 26, 28); - const DARK_STATUS: Color32 = Color32::from_rgb(50, 30, 20); + const DARK_STATUS: Color32 = Color32::from_rgb(44, 26, 18); fn screen() -> Rect { Rect::from_min_size(egui::Pos2::ZERO, egui::vec2(640.0, 480.0)) @@ -906,6 +925,54 @@ mod tests { } } + /// Errors and failure reasons reach WCAG AA's 4.5:1 on the card and in the prompt's message + /// box, in both themes. + #[test] + fn failure_text_reaches_aa_in_both_themes() { + for theme in [egui::ThemePreference::Dark, egui::ThemePreference::Light] { + let ctx = Context::default(); + ctx.set_theme(theme); + let pal = Palette::new(&ctx, &PHOTOCRAFT); + for (surface, bg) in [("card", pal.card_bg), ("message box", pal.alert_bg)] { + let ratio = contrast(pal.alert, bg); + assert!(ratio >= 4.5, "{theme:?} {surface}: {ratio:.2}:1"); + } + } + } + + /// A long error or failure reason scrolls inside the card, so the card's buttons stay on + /// screen. + #[test] + fn a_long_reason_keeps_the_buttons_on_screen() { + let long = "the installer reported an error. ".repeat(120); + let on_screen = |output: &FullOutput, label: &str| text_pos(output, label).is_some_and(|pos| screen().contains(pos)); + let dialog = CheckResultDialog::new(&PHOTOCRAFT, "0.1.0"); + for status in [Status::InstallFailed(long.clone()), Status::Failed(long.clone())] { + let ctx = dark_context(); + let mut output = None; + // The first frame of an area only sizes it; the second paints. + for _ in 0..2 { + let mut out = ctx.run_ui(RawInput { screen_rect: Some(screen()), ..RawInput::default() }, |ui| { + dialog.show(ui.ctx(), &status); + }); + out.textures_delta.clear(); + output = Some(out); + } + let output = output.unwrap(); + for label in ["OK", "CHECK AGAIN"] { + assert!(on_screen(&output, label), "{label} is off screen for a long {status:?}"); + } + } + let ctx = dark_context(); + let mut prompt = prompt_for(true); + prompt.set_message(long); + frame(&ctx, &mut prompt, Vec::new()); + let (_, output) = frame(&ctx, &mut prompt, Vec::new()); + for label in ["UPDATE AND RESTART", "LATER"] { + assert!(on_screen(&output, label), "{label} is off screen under a long message"); + } + } + /// Where the frame painted `text` (inside its galley, so a pointer there is over it). fn text_pos(output: &FullOutput, text: &str) -> Option { fn find(shape: &egui::Shape, text: &str) -> Option { diff --git a/crates/update/src/self_update.rs b/crates/update/src/self_update.rs index 3b4d9cf8..d044ff6e 100644 --- a/crates/update/src/self_update.rs +++ b/crates/update/src/self_update.rs @@ -375,29 +375,31 @@ pub fn update( prepare_update(app, install, release, platform, http, paths, stop, phase, last, running_version)?.start() } -/// What remains of an update after [`prepare_update`]: starting the Windows helper, which waits -/// for this process to exit and then installs the update. On macOS and Linux the update is -/// already in place and its restart already scheduled, so [`Restart::start`] does nothing. -#[must_use = "on Windows the update is only installed once the restart is started"] +/// What remains of an update after [`prepare_update`]: starting the helper, which waits for this +/// process to exit. On Windows the helper then installs the update and starts the new version; +/// on macOS and Linux the update is already in place and the helper starts the new version. +#[must_use = "the update only installs or restarts once the restart is started"] pub struct Restart { helper: Option Result<(), String> + Send>>, } impl Restart { /// Nothing is left to start. - #[cfg_attr(all(windows, not(test)), allow(dead_code))] + #[cfg_attr(not(test), allow(dead_code))] pub(crate) fn scheduled() -> Self { Self { helper: None } } /// `start` runs when the app closes. - #[cfg_attr(not(any(windows, test)), allow(dead_code))] + #[cfg_attr(not(any(windows, unix, test)), allow(dead_code))] pub(crate) fn later(start: impl FnOnce() -> Result<(), String> + Send + 'static) -> Self { Self { helper: Some(Box::new(start)) } } - /// Starts the helper. Call it when the app closes: the helper waits only a minute for the - /// app to exit, and installs nothing when the app is still running after that. + /// Starts the helper. Call it when the app's exit is certain: the helper waits only a minute + /// for the app to exit. On Windows it installs nothing when the app is still running after + /// that; on macOS and Linux the Flatpak helper starts nothing then, and the other helpers + /// start the new version anyway. pub fn start(self) -> Result<(), String> { self.helper.map_or(Ok(()), |start| start()) } @@ -409,8 +411,8 @@ impl std::fmt::Debug for Restart { } } -/// [`update`] without starting the Windows helper: the caller starts the returned [`Restart`] -/// when the app closes. On macOS and Linux the result is the same as [`update`]'s. +/// [`update`] without starting the helper: the caller starts the returned [`Restart`] when the +/// app's exit is certain. On macOS and Linux the update is in place when this returns. #[allow(clippy::too_many_arguments)] pub fn prepare_update( app: &App, @@ -457,20 +459,20 @@ pub fn prepare_update( let placed = install::macos::install_dmg(app, &file, Some(&existing), None, false, paths, &install::macos::Expect::release(release)); let _ = std::fs::remove_file(&file); let placed = placed?; - relaunch_after_exit(app, pid, &[Path::new("/usr/bin/open").as_os_str(), placed.as_os_str()]).map(|()| Restart::scheduled()) + Ok(relaunch_after_exit(app, pid, &[Path::new("/usr/bin/open").as_os_str(), placed.as_os_str()])) } #[cfg(target_os = "linux")] Install::AppImage(path) => { replace_appimage(&file, path)?; let _ = std::fs::remove_file(&file); - relaunch_after_exit(app, pid, &[path.as_os_str()]).map(|()| Restart::scheduled()) + Ok(relaunch_after_exit(app, pid, &[path.as_os_str()])) } #[cfg(target_os = "linux")] Install::Package(kind, exe) => { let result = install::linux::install_package(*kind, &file, None, stop, phase); let _ = std::fs::remove_file(&file); result?; - relaunch_after_exit(app, pid, &[exe.as_os_str()]).map(|()| Restart::scheduled()) + Ok(relaunch_after_exit(app, pid, &[exe.as_os_str()])) } #[cfg(target_os = "linux")] Install::Flatpak => { @@ -484,7 +486,7 @@ pub fn prepare_update( let result = install::linux::update_flatpak_bundle(&file, &app.bundle_id(), system, stop, phase); let _ = std::fs::remove_file(&file); result?; - relaunch_flatpak_after_exit(app).map(|()| Restart::scheduled()) + relaunch_flatpak_after_exit(app) } #[cfg(windows)] Install::Msi(exe) => windows_helper(app, paths, release, pid, exe, &msi_finish(&file)?), @@ -503,7 +505,7 @@ pub fn prepare_update( let result = replace_standalone(app, paths, &file, exe); let _ = std::fs::remove_file(&file); result?; - relaunch_after_exit(app, pid, &[exe.as_os_str()]).map(|()| Restart::scheduled()) + Ok(relaunch_after_exit(app, pid, &[exe.as_os_str()])) } #[allow(unreachable_patterns)] _ => { @@ -513,20 +515,22 @@ pub fn prepare_update( } })(); let scheduled = unmark_unless_scheduled(paths, scheduled); - // On macOS and Linux the update is in place once it is scheduled. + // On macOS and Linux the update is in place once it is prepared. #[cfg(not(windows))] if scheduled.is_ok() { - record_done(paths, &release.tag); + record_done(paths, &release.version_label()); } scheduled } -/// Records `release` as just installed, so it isn't offered again for [`JUST_UPDATED_FOR`]. The -/// outcome records of an earlier update are removed: this update writes its own. +/// Records `release` as just installed, so it isn't offered again for [`JUST_UPDATED_FOR`]: its +/// tag, the time, and the version it installs. The outcome records of an earlier update are +/// removed: this update writes its own. fn mark_installed(paths: &Paths, release: &Release) { let _ = std::fs::remove_file(failure_file(paths)); let _ = std::fs::remove_file(done_file(paths)); - if let Err(e) = crate::paths::write_atomic(&marker(paths), format!("{}\n{}\n", release.tag, crate::now()).as_bytes()) { + let text = format!("{}\n{}\n{}\n", release.tag, crate::now(), release.version_label()); + if let Err(e) = crate::paths::write_atomic(&marker(paths), text.as_bytes()) { log::warn!("couldn't record the self-update: {e}"); } } @@ -536,17 +540,22 @@ fn done_file(paths: &Paths) -> PathBuf { paths.data.join("self-update-done.txt") } -/// Records that the update to `tag` has been installed. -fn record_done(paths: &Paths, tag: &str) { - if let Err(e) = crate::paths::write_atomic(&done_file(paths), tag.as_bytes()) { +/// Records that the update to `version` has been installed. +fn record_done(paths: &Paths, version: &str) { + if let Err(e) = crate::paths::write_atomic(&done_file(paths), version.as_bytes()) { log::warn!("couldn't record the finished self-update: {e}"); } } -/// The version the scheduled update installs, from the record [`mark_installed`] wrote. +/// The version the scheduled update installs, from the record [`mark_installed`] wrote: its third +/// line, or the tag on its first line in a record without one. #[cfg_attr(not(windows), allow(dead_code))] -fn scheduled_tag(paths: &Paths) -> Option { - read_record(&marker(paths)).and_then(|text| text.lines().next().map(str::to_owned)).filter(|tag| !tag.trim().is_empty()) +fn scheduled_version(paths: &Paths) -> Option { + let text = read_record(&marker(paths))?; + let mut lines = text.lines(); + let tag = lines.next(); + let version = lines.nth(1).filter(|version| !version.trim().is_empty()).or(tag); + version.map(str::trim).filter(|version| !version.is_empty()).map(str::to_owned) } /// The most of an outcome record that is read: a reason longer than this is cut short. @@ -741,10 +750,13 @@ fn plain_files(dir: &Path) -> Result, String> { Ok(files) } -/// Starts `sh`, which waits for `pid` to exit and then runs `command`. Detached, so it outlives us. +/// A [`Restart`] that starts `sh`, which waits for `pid` to exit and then runs `command`. +/// Detached, so it outlives us. Nothing starts before the app's exit is certain: a close the user +/// cancels leaves no helper counting down its wait. #[cfg(unix)] -fn relaunch_after_exit(app: &App, pid: u32, command: &[&std::ffi::OsStr]) -> Result<(), String> { - process::spawn_detached(helper_command(app, pid, command)).map_err(|e| format!("couldn't schedule the restart ({e})")) +fn relaunch_after_exit(app: &App, pid: u32, command: &[&std::ffi::OsStr]) -> Restart { + let cmd = helper_command(app, pid, command); + Restart::later(move || process::spawn_detached(cmd).map_err(|e| format!("couldn't schedule the restart ({e})"))) } #[cfg(unix)] @@ -798,9 +810,10 @@ fn flatpak_host_pid() -> Option { parse_flatpak_host_pid(&out, &instance_id) } -/// Starts a host helper that waits for this Flatpak instance to exit, then relaunches it via Flatpak on the host. +/// A [`Restart`] that starts a host helper, which waits for this Flatpak instance to exit, then +/// relaunches it via Flatpak on the host. Nothing starts before the app's exit is certain. #[cfg(target_os = "linux")] -fn relaunch_flatpak_after_exit(app: &App) -> Result<(), String> { +fn relaunch_flatpak_after_exit(app: &App) -> Result { let host_pid = flatpak_host_pid(); let instance_id = flatpak_instance_id(); // Each wait clause polls until the old process is gone, then execs the relaunch command. @@ -819,7 +832,8 @@ fn relaunch_flatpak_after_exit(app: &App) -> Result<(), String> { (None, None) => return Err(format!("{} is updated; quit and reopen it to start the new version", app.name)), }; let script = format!("({wait_clause}) &"); - process::spawn_detached(flatpak_restart_command(app, &script, &target_arg)).map_err(|e| format!("couldn't schedule the Flatpak restart ({e})")) + let cmd = flatpak_restart_command(app, &script, &target_arg); + Ok(Restart::later(move || process::spawn_detached(cmd).map_err(|e| format!("couldn't schedule the Flatpak restart ({e})")))) } #[cfg(target_os = "linux")] @@ -958,6 +972,7 @@ fn finish_step( wait: &dyn Fn(u32) -> bool, start: &dyn Fn(&Path) -> Result<(), String>, ) -> Result<(), String> { + let portable_before = has_portable_marker(&exe); let stepped = step_after_wait(app, reference, pid, &exe, step, wait); let done = stepped.result; if let Err(e) = &done { @@ -965,6 +980,14 @@ fn finish_step( } // The new app, or the old one when the step failed. let exe = if done.is_ok() { exe_after_msi(app, registered, &exe) } else { exe }; + // A standalone copy updated from the portable zip gains `portable.txt` and keeps its folders + // beside it from now on: the records the new app reads go there. + let version = scheduled_version(paths); + let moved = (done.is_ok() && has_portable_marker(&exe) != portable_before).then(|| Paths::for_exe(app, &exe).ok()).flatten(); + if let Some(after) = &moved { + move_marker(paths, after); + } + let paths = moved.as_ref().unwrap_or(paths); // An app that is still running gets no second copy. A failed move can leave no program to // start; its recorded error already says how to get the app back, and a restart error must // not replace it. @@ -973,9 +996,9 @@ fn finish_step( } // Written before the new app starts, so its first start finds it. A failed start removes it. if done.is_ok() - && let Some(tag) = scheduled_tag(paths) + && let Some(version) = &version { - record_done(paths, &tag); + record_done(paths, version); } let started = start(&exe); if let Err(e) = &started { @@ -984,6 +1007,26 @@ fn finish_step( done.and(started) } +/// Whether `exe` has `portable.txt` beside it, which makes [`Paths::for_exe`] keep its folders +/// there. +#[cfg(windows)] +fn has_portable_marker(exe: &Path) -> bool { + exe.parent().is_some_and(|dir| dir.join("portable.txt").is_file()) +} + +/// Moves the record of the scheduled update from `from` to `to`, the folders the updated app +/// reads, and removes outcome records left in `to` by an earlier update. +#[cfg(windows)] +fn move_marker(from: &Paths, to: &Paths) { + let _ = std::fs::remove_file(failure_file(to)); + let _ = std::fs::remove_file(done_file(to)); + if let Some(text) = read_record(&marker(from)) + && crate::paths::write_atomic(&marker(to), text.as_bytes()).is_ok() + { + let _ = std::fs::remove_file(marker(from)); + } +} + /// Finishes an update on non-Windows platforms (no-op helper). #[cfg(not(windows))] pub fn finish(_app: &App, _args: &[OsString]) -> Option> { @@ -1674,7 +1717,24 @@ mod tests { record_failure(&paths, "the Windows Installer stopped with error 1603"); mark_installed(&paths, &Release { tag: "v0.3.0".into(), ..Release::default() }); assert_eq!(take_failure(&paths), None); - assert_eq!(scheduled_tag(&paths).as_deref(), Some("v0.3.0")); + assert_eq!(scheduled_version(&paths).as_deref(), Some("v0.3.0")); + } + + /// A release whose tag is not its version (an app may build its own `Release`) is recorded + /// by its version, so the new version's first start still reports the update. + #[test] + fn a_finished_update_is_recorded_by_its_version_not_its_tag() { + let dir = tempfile::tempdir().unwrap(); + let paths = Paths::under(dir.path()); + let release = Release { tag: "stable".into(), version: version::parse("0.2.0"), ..Release::default() }; + mark_installed(&paths, &release); + assert!(just_updated_to(&paths, &release), "the tag still guards against offering it again"); + assert_eq!(scheduled_version(&paths).as_deref(), Some("0.2.0")); + record_done(&paths, &scheduled_version(&paths).unwrap()); + assert_eq!(take_outcome(&paths, "0.2.0"), Some(Outcome::Installed(Version::new(0, 2, 0)))); + // A record without the version line, from before it was written, names the tag. + crate::paths::write_atomic(&marker(&paths), format!("v0.3.0\n{}\n", crate::now()).as_bytes()).unwrap(); + assert_eq!(scheduled_version(&paths).as_deref(), Some("v0.3.0")); } /// A launcher that doesn't quit keeps its files, and the download is still removed. @@ -1926,6 +1986,28 @@ mod tests { } } + /// The restart helper starts only when the restart is started, at the app's exit: a close the + /// user cancels leaves no helper counting down its one-minute wait. + #[cfg(unix)] + #[test] + fn the_restart_helper_starts_only_when_the_restart_is_started() { + use std::io::BufRead; + let dir = tempfile::tempdir().unwrap(); + let events = dir.path().join("events"); + assert!(std::process::Command::new("mkfifo").arg(&events).status().unwrap().success()); + // Open for reading and writing, so neither this open nor the helper's blocks. + let pipe = std::fs::OpenOptions::new().read(true).write(true).open(&events).unwrap(); + let mut lines = std::io::BufReader::new(pipe).lines(); + let mut gone = std::process::Command::new("true").spawn().unwrap(); + gone.wait().unwrap(); + let started = r#"printf 'started\n' > "$1""#; + let command = [std::ffi::OsStr::new("/bin/sh"), "-c".as_ref(), started.as_ref(), "sh".as_ref(), events.as_os_str()]; + let restart = relaunch_after_exit(&crate::suite::LAUNCHER, gone.id(), &command); + assert_eq!(format!("{restart:?}"), "Restart { pending: true }", "the helper must wait for the restart to be started"); + assert_eq!(restart.start(), Ok(())); + assert_eq!(lines.next().unwrap().unwrap(), "started"); + } + /// The helper waits for the old launcher to exit before it starts the new one. The helper's /// `sleep` is a script on `PATH` that reports each pause through a named pipe and pauses until /// the test releases it, and the started command reports whether the old launcher was still @@ -2326,6 +2408,60 @@ mod tests { assert_eq!(take_outcome(&paths, "0.2.0"), Some(Outcome::Installed(Version::new(0, 2, 0))), "the new version's first start says so"); } + /// A standalone copy updated from the portable zip becomes portable: the outcome and the + /// record of the update go to the folders beside it, which the new app reads. + #[cfg(windows)] + #[test] + fn a_standalone_copy_that_becomes_portable_finds_the_outcome_beside_it() { + use std::io::Write; + let dir = tempfile::tempdir().unwrap(); + // The per-user folders the old, standalone copy used. + let paths = Paths::under(&dir.path().join("per-user")); + let folder = dir.path().join("ArtCraft Launcher"); + std::fs::create_dir_all(&folder).unwrap(); + let exe = folder.join("artcraft-launcher.exe"); + std::fs::write(&exe, "0.1.0").unwrap(); + let zip = dir.path().join("artcraft-launcher-0.2.0-windows-x64-portable.zip"); + let mut writer = zip::ZipWriter::new(std::fs::File::create(&zip).unwrap()); + writer.start_file("artcraft-launcher-0.2.0-windows-x64-portable/artcraft-launcher.exe", zip::write::SimpleFileOptions::default()).unwrap(); + writer.write_all(&std::fs::read(system_program("cmd.exe")).unwrap()).unwrap(); + writer.start_file("artcraft-launcher-0.2.0-windows-x64-portable/portable.txt", zip::write::SimpleFileOptions::default()).unwrap(); + writer.finish().unwrap(); + let step = Finish::Portable { zip, tmp: dir.path().join(PORTABLE_STAGING) }; + let release = Release { tag: "v0.2.0".into(), version: version::parse("0.2.0"), ..Release::default() }; + mark_installed(&paths, &release); + let exited = |_pid| true; + let start = |_: &Path| Ok(()); + assert_eq!( + finish_step(&crate::suite::LAUNCHER, &system_program("cmd.exe"), &paths, 4242, exe.clone(), &step, false, &exited, &start), + Ok(()) + ); + let after = Paths::for_exe(&crate::suite::LAUNCHER, &exe).unwrap(); + assert_eq!(after.data, folder.join("ArtCraftLauncherData").join("data")); + assert!(just_updated_to(&after, &release), "the new app does not offer the same update again"); + assert_eq!(take_outcome(&after, "0.2.0"), Some(Outcome::Installed(Version::new(0, 2, 0)))); + assert_eq!(take_outcome(&paths, "0.2.0"), None, "nothing is left where the new app no longer looks"); + + // A new app that cannot start is reported there too. + let dir = tempfile::tempdir().unwrap(); + let paths = Paths::under(&dir.path().join("per-user")); + let (exe, step) = portable_update(dir.path()); + if let Finish::Portable { zip, .. } = &step { + let mut writer = zip::ZipWriter::new(std::fs::File::create(zip).unwrap()); + writer + .start_file("artcraft-launcher-0.2.0-windows-x64-portable/artcraft-launcher.exe", zip::write::SimpleFileOptions::default()) + .unwrap(); + writer.write_all(&std::fs::read(system_program("cmd.exe")).unwrap()).unwrap(); + writer.start_file("artcraft-launcher-0.2.0-windows-x64-portable/portable.txt", zip::write::SimpleFileOptions::default()).unwrap(); + writer.finish().unwrap(); + } + mark_installed(&paths, &release); + let denied = |_: &Path| Err("couldn't start the launcher (denied)".to_owned()); + assert!(finish_step(&crate::suite::LAUNCHER, &system_program("cmd.exe"), &paths, 4242, exe.clone(), &step, false, &exited, &denied).is_err()); + let after = Paths::for_exe(&crate::suite::LAUNCHER, &exe).unwrap(); + assert_eq!(take_outcome(&after, "0.2.0"), Some(Outcome::Failed("couldn't start the launcher (denied)".into()))); + } + /// A portable update of the launcher to 0.2.0 in `dir`: its exe and the step. #[cfg(windows)] fn portable_update(dir: &Path) -> (PathBuf, Finish) { diff --git a/crates/update/src/updater.rs b/crates/update/src/updater.rs index da174874..196c862e 100644 --- a/crates/update/src/updater.rs +++ b/crates/update/src/updater.rs @@ -68,8 +68,8 @@ pub enum Action { /// Close the app through its normal close path (which asks about unsaved work): an update is /// ready and a helper installs it and restarts the app once the app has exited. This is a /// request: the user may cancel the close, and the update then stays ready, with - /// [`Status::Ready`] for a requested check and Restart to Update in the prompt. On Windows - /// the helper starts only at the exit: when the app drops the [`Updater`] as it exits, or + /// [`Status::Ready`] for a requested check and Restart to Update in the prompt. The helper + /// starts only at the exit: when the app drops the [`Updater`] as it exits, or /// calls [`Updater::start_pending_update`] in `on_exit`, which an app that ends the process /// there without dropping the `Updater` must do. An installation still in progress when /// such an app ends the process is lost, since its worker thread ends with the process. @@ -434,8 +434,14 @@ impl Updater { } match answer { Ok(Some(release)) => { - if self.prompt.is_none() && self.install.is_none() && !self.installed { - self.open_offer(release); + match &mut self.prompt { + None => self.open_offer(release), + // A prompt the app opened before (`offer`) says why the last update failed. + Some(prompt) => { + if let Some(reason) = self.failed_reason.take() { + prompt.set_note(Note::new("The update could not be installed: {reason}", &[("reason", &reason)])); + } + } } self.status = Status::Idle; } @@ -529,9 +535,9 @@ impl Updater { /// to close, when `can_start` is true. An app passes `false` while it holds unsaved work. pub fn show_if(&mut self, ctx: &egui::Context, can_start: bool) -> Action { // The close waits for the app to hold no unsaved work: the user may have edited while the - // update installed. It is only asked for: the Windows helper, which waits just a minute - // for the app to exit, starts at the exit itself (`start_pending_update`), so a close the - // user cancels leaves the update ready. + // update installed. It is only asked for: the helper, which waits just a minute for the + // app to exit, starts at the exit itself (`start_pending_update`), so a close the user + // cancels leaves the update ready. if can_start && self.restart.is_some() && std::mem::take(&mut self.close_wanted) { return Action::Close; } @@ -1561,6 +1567,23 @@ mod tests { assert_eq!(u.prompt.as_ref().and_then(StartupPrompt::message), None, "the reason is shown once"); } + /// An offer the app made before the check at start keeps its prompt; when the check finds the + /// release too, that prompt says why the last update failed, once. + #[test] + fn an_offer_before_the_check_at_start_says_why_the_last_update_failed() { + let ctx = egui::Context::default(); + let (_dir, paths, mut u) = with_records("0.1.0"); + self_update::record_failure(&paths, "disk full"); + u.offer(newer()); + assert_eq!(u.prompt.as_ref().and_then(StartupPrompt::message), None, "the failure is not read before the check at start"); + start_answered(&mut u, &ctx, Ok(Some(newer()))); + assert_eq!(u.status(), &Status::Idle, "the prompt carries the reason"); + assert_eq!(u.prompt.as_ref().and_then(StartupPrompt::message).as_deref(), Some("The update could not be installed: disk full")); + u.prompt = None; + u.offer(newer()); + assert_eq!(u.prompt.as_ref().and_then(StartupPrompt::message), None, "the reason is shown once"); + } + /// An installation that finishes after the updater was dropped and whose helper cannot start, /// or that failed, is recorded, and the next start says why. #[test] From 01e0322721839f16d8b4d36e1637499820b7e0f9 Mon Sep 17 00:00:00 2001 From: dotCooCoo Date: Sat, 7 Nov 2026 22:43:46 -0800 Subject: [PATCH 18/25] craft-update: the restart-helper test fails when the helper exits without starting the command The test kept the events pipe open for reading and writing, so a helper that exited before writing `started` left the read waiting forever. The helper is now started through `relaunch_with` with a spawner the test controls: it starts the helper the way `spawn_detached` does, and a thread that waits for the helper to exit writes `exited`, so every read gets a line. The test also checks that nothing is spawned before `Restart::start`. --- crates/update/src/self_update.rs | 44 ++++++++++++++++++++++++++++---- 1 file changed, 39 insertions(+), 5 deletions(-) diff --git a/crates/update/src/self_update.rs b/crates/update/src/self_update.rs index d044ff6e..40b157dc 100644 --- a/crates/update/src/self_update.rs +++ b/crates/update/src/self_update.rs @@ -755,8 +755,19 @@ fn plain_files(dir: &Path) -> Result, String> { /// cancels leaves no helper counting down its wait. #[cfg(unix)] fn relaunch_after_exit(app: &App, pid: u32, command: &[&std::ffi::OsStr]) -> Restart { + relaunch_with(app, pid, command, process::spawn_detached) +} + +/// [`relaunch_after_exit`], with `spawn` starting the helper. +#[cfg(unix)] +fn relaunch_with( + app: &App, + pid: u32, + command: &[&std::ffi::OsStr], + spawn: impl FnOnce(std::process::Command) -> Result<(), String> + Send + 'static, +) -> Restart { let cmd = helper_command(app, pid, command); - Restart::later(move || process::spawn_detached(cmd).map_err(|e| format!("couldn't schedule the restart ({e})"))) + Restart::later(move || spawn(cmd).map_err(|e| format!("couldn't schedule the restart ({e})"))) } #[cfg(unix)] @@ -1987,25 +1998,48 @@ mod tests { } /// The restart helper starts only when the restart is started, at the app's exit: a close the - /// user cancels leaves no helper counting down its one-minute wait. + /// user cancels leaves no helper counting down its one-minute wait. The test starts the helper + /// the way `spawn_detached` does and keeps it, and a thread that waits for the helper to exit + /// writes `exited`, so a helper that exits without writing `started` fails the test. #[cfg(unix)] #[test] fn the_restart_helper_starts_only_when_the_restart_is_started() { - use std::io::BufRead; + use std::io::{BufRead, Write}; + use std::sync::Arc; + use std::sync::atomic::{AtomicBool, Ordering}; let dir = tempfile::tempdir().unwrap(); let events = dir.path().join("events"); assert!(std::process::Command::new("mkfifo").arg(&events).status().unwrap().success()); // Open for reading and writing, so neither this open nor the helper's blocks. let pipe = std::fs::OpenOptions::new().read(true).write(true).open(&events).unwrap(); + let mut exited = pipe.try_clone().unwrap(); let mut lines = std::io::BufReader::new(pipe).lines(); let mut gone = std::process::Command::new("true").spawn().unwrap(); gone.wait().unwrap(); let started = r#"printf 'started\n' > "$1""#; let command = [std::ffi::OsStr::new("/bin/sh"), "-c".as_ref(), started.as_ref(), "sh".as_ref(), events.as_os_str()]; - let restart = relaunch_after_exit(&crate::suite::LAUNCHER, gone.id(), &command); + let spawned = Arc::new(AtomicBool::new(false)); + let flag = Arc::clone(&spawned); + let spawn = move |mut cmd: std::process::Command| -> Result<(), String> { + flag.store(true, Ordering::SeqCst); + let mut helper = cmd + .stdin(std::process::Stdio::null()) + .stdout(std::process::Stdio::null()) + .stderr(std::process::Stdio::null()) + .spawn() + .map_err(|e| e.to_string())?; + std::thread::spawn(move || { + let _ = helper.wait(); + let _ = exited.write_all(b"exited\n"); + }); + Ok(()) + }; + let restart = relaunch_with(&crate::suite::LAUNCHER, gone.id(), &command, spawn); + assert!(!spawned.load(Ordering::SeqCst), "the helper must wait for the restart to be started"); assert_eq!(format!("{restart:?}"), "Restart { pending: true }", "the helper must wait for the restart to be started"); assert_eq!(restart.start(), Ok(())); - assert_eq!(lines.next().unwrap().unwrap(), "started"); + assert_eq!(lines.next().unwrap().unwrap(), "started", "the helper exited without starting the command"); + assert_eq!(lines.next().unwrap().unwrap(), "exited"); } /// The helper waits for the old launcher to exit before it starts the new one. The helper's From 324f06f6394d9fc1d6f1eef35a7af9bebee23560 Mon Sep 17 00:00:00 2001 From: dotCooCoo Date: Sun, 8 Nov 2026 12:04:02 -0800 Subject: [PATCH 19/25] craft-update: wait later for a started process when no thread can wait for it When spawn_detached or the system opener cannot start a thread to wait for the process it started, the process is kept and waited for by a later call once it has exited, so on Unix it does not stay in the process table. --- crates/update/src/process.rs | 73 ++++++++++++++++++++++++++++-------- 1 file changed, 58 insertions(+), 15 deletions(-) diff --git a/crates/update/src/process.rs b/crates/update/src/process.rs index 5aa6d42e..71489934 100644 --- a/crates/update/src/process.rs +++ b/crates/update/src/process.rs @@ -2,7 +2,8 @@ use std::ffi::OsStr; use std::path::{Path, PathBuf}; -use std::process::{Command, Stdio}; +use std::process::{Child, Command, Stdio}; +use std::sync::{Arc, Mutex, PoisonError}; use std::time::{Duration, Instant}; /// A `Command` that won't flash a console window on Windows, and clears AppImage-injected @@ -115,18 +116,47 @@ fn read_all(pipe: Option) -> std::io::Resu } /// Starts `cmd` without waiting for it, and reaps it when it exits. The process keeps running -/// when no thread can be started to reap it; only the wait is skipped. +/// when no thread can be started to reap it; it is then reaped by a later call once it exits. pub fn spawn_detached(mut cmd: Command) -> Result<(), String> { - let mut child = cmd.stdin(Stdio::null()).stdout(Stdio::null()).stderr(Stdio::null()).spawn().map_err(|e| format!("couldn't start it ({e})"))?; - let reaper = std::thread::Builder::new().name("reap-child".into()).spawn(move || { - let _ = child.wait(); - }); - if let Err(e) = reaper { - log::warn!("couldn't start a thread to wait for a started process ({e})"); - } + let child = cmd.stdin(Stdio::null()).stdout(Stdio::null()).stderr(Stdio::null()).spawn().map_err(|e| format!("couldn't start it ({e})"))?; + reap_in_background(child, "a started process"); Ok(()) } +/// Started processes that no thread waits for, because none could be started. On Unix a process +/// that exits stays in the process table until it is waited for. +static UNREAPED: Mutex> = Mutex::new(Vec::new()); + +/// Waits for `child` on its own thread. When no thread can be started, `child` is kept in +/// [`UNREAPED`] and waited for by a later call once it has exited. +fn reap_in_background(child: Child, what: &str) { + reap_in_background_with(child, what, |wait| std::thread::Builder::new().name("reap-child".into()).spawn(wait).map(drop)); +} + +/// [`reap_in_background`] with `start_thread` in place of starting a thread (tests). +fn reap_in_background_with(child: Child, what: &str, start_thread: impl FnOnce(Box) -> std::io::Result<()>) { + reap_exited(); + let slot = Arc::new(Mutex::new(Some(child))); + let theirs = Arc::clone(&slot); + let wait = Box::new(move || { + let child = theirs.lock().unwrap_or_else(PoisonError::into_inner).take(); + if let Some(mut child) = child { + let _ = child.wait(); + } + }); + if let Err(e) = start_thread(wait) { + log::warn!("couldn't start a thread to wait for {what} ({e})"); + if let Some(child) = slot.lock().unwrap_or_else(PoisonError::into_inner).take() { + UNREAPED.lock().unwrap_or_else(PoisonError::into_inner).push(child); + } + } +} + +/// Waits for the processes in [`UNREAPED`] that have exited, without blocking on the others. +fn reap_exited() { + UNREAPED.lock().unwrap_or_else(PoisonError::into_inner).retain_mut(|child| matches!(child.try_wait(), Ok(None))); +} + /// Waits up to `timeout` for the process `pid` to exit, and says whether it has. A process that /// can't be opened, because it's gone or never was, counts as exited. #[cfg(windows)] @@ -277,12 +307,7 @@ fn open_and_wait_success(target: &str, timeout: Duration) -> bool { Ok(Some(status)) => return status.success(), Ok(None) if start.elapsed() < timeout => std::thread::sleep(Duration::from_millis(25)), Ok(None) => { - let reaper = std::thread::Builder::new().name("reap-child".into()).spawn(move || { - let _ = child.wait(); - }); - if let Err(e) = reaper { - log::warn!("couldn't start a thread to wait for the opener of {target} ({e})"); - } + reap_in_background(child, &format!("the opener of {target}")); return true; } Err(_) => return false, @@ -430,6 +455,24 @@ mod tests { assert_eq!(is_running(Path::new(&requested)), expected, "{requested}"); } } + + /// A process that no thread can wait for is kept, and waited for once it has exited, so it + /// does not stay in the process table. + #[test] + fn a_process_without_a_waiting_thread_is_reaped_later() { + let mut child = command("cat").stdin(Stdio::piped()).stdout(Stdio::null()).stderr(Stdio::null()).spawn().unwrap(); + let stdin = child.stdin.take().unwrap(); + let pid = child.id(); + let kept = || UNREAPED.lock().unwrap_or_else(PoisonError::into_inner).iter().any(|c| c.id() == pid); + reap_in_background_with(child, "a test process", |_wait| Err(std::io::Error::other("no threads"))); + assert!(kept(), "a running process with no thread to wait for it is kept"); + // `cat` exits when its input closes. + drop(stdin); + while kept() { + reap_exited(); + std::thread::yield_now(); + } + } } #[cfg(test)] From 075ad12afc2c72a16be2f162819353ef2963d711 Mon Sep 17 00:00:00 2001 From: dotCooCoo Date: Sun, 8 Nov 2026 12:04:02 -0800 Subject: [PATCH 20/25] craft-update: keep the update step's error when the helper cannot restart the app When the update helper's step fails and starting the old app again fails too, the error recorded for the next start names both, the step's first. Before, the restart error replaced the step's. --- crates/update/src/self_update.rs | 48 ++++++++++++++++++++++++++++++-- 1 file changed, 46 insertions(+), 2 deletions(-) diff --git a/crates/update/src/self_update.rs b/crates/update/src/self_update.rs index d9bde551..e378fe7c 100644 --- a/crates/update/src/self_update.rs +++ b/crates/update/src/self_update.rs @@ -898,10 +898,23 @@ fn finish_step( return done; } let started = start(&exe); - if let Err(e) = &started { + let restarted = started.is_ok(); + let result = with_restart(done, started); + if !restarted && let Err(e) = &result { record_failure(paths, e); } - done.and(started) + result +} + +/// The helper's result once it has tried to start the app: when both the update step and the +/// start fail, the error names both, the step's first. +#[cfg_attr(not(windows), allow(dead_code))] +fn with_restart(done: Result<(), String>, started: Result<(), String>) -> Result<(), String> { + match (done, started) { + (done, Ok(())) => done, + (Ok(()), Err(restart)) => Err(restart), + (Err(update), Err(restart)) => Err(format!("{update}; {restart}")), + } } /// Finishes an update on non-Windows platforms (no-op helper). @@ -2152,6 +2165,37 @@ mod tests { assert_eq!(std::fs::read_to_string(&exe).unwrap(), "0.1.0"); } + /// When the update step fails and the old app can't be started again either, the error kept + /// for the next start names both. + #[cfg(windows)] + #[test] + fn a_failed_step_and_a_failed_restart_are_both_recorded() { + let dir = tempfile::tempdir().unwrap(); + let paths = Paths::under(dir.path()); + let exe = dir.path().join("artcraft-launcher.exe"); + std::fs::write(&exe, "0.1.0").unwrap(); + // Not a zip, so the step fails. + let zip = dir.path().join("artcraft-launcher-0.2.0-windows-x64-portable.zip"); + std::fs::write(&zip, "not a zip").unwrap(); + let step = Finish::Portable { zip, tmp: dir.path().join(PORTABLE_STAGING) }; + let start = |_exe: &Path| Err("couldn't start artcraft-launcher.exe (denied)".to_string()); + let exited = |_pid| true; + let error = + finish_step(&crate::suite::LAUNCHER, &system_program("cmd.exe"), &paths, 4242, exe.clone(), &step, false, &exited, &start).unwrap_err(); + assert!(error.ends_with("; couldn't start artcraft-launcher.exe (denied)"), "{error}"); + assert!(error.len() > "; couldn't start artcraft-launcher.exe (denied)".len(), "the step's error comes first: {error}"); + assert_eq!(take_failure(&paths).as_deref(), Some(error.as_str())); + } + + /// The helper's result keeps the update step's error when the start fails too. + #[test] + fn a_failed_restart_keeps_the_step_error() { + assert_eq!(with_restart(Ok(()), Ok(())), Ok(())); + assert_eq!(with_restart(Err("step".into()), Ok(())), Err("step".into())); + assert_eq!(with_restart(Ok(()), Err("start".into())), Err("start".into())); + assert_eq!(with_restart(Err("step".into()), Err("start".into())), Err("step; start".into())); + } + /// A successful step starts the new app once. #[cfg(windows)] #[test] From 658136392bf098c6a52cc2415099e8c2024719f2 Mon Sep 17 00:00:00 2001 From: dotCooCoo Date: Sun, 8 Nov 2026 12:04:02 -0800 Subject: [PATCH 21/25] craft-update: start the helper of an install that finishes after start_pending_update An app that calls start_pending_update in on_exit without dropping the Updater now gets the same handoff as the drop: an install that finishes after the call starts its helper itself instead of sending its outcome to an Updater that will not read it. --- crates/update/src/updater.rs | 59 ++++++++++++++++++++++++------------ 1 file changed, 39 insertions(+), 20 deletions(-) diff --git a/crates/update/src/updater.rs b/crates/update/src/updater.rs index c31c8f24..8f3ad1b0 100644 --- a/crates/update/src/updater.rs +++ b/crates/update/src/updater.rs @@ -101,9 +101,10 @@ pub struct Updater { restart: Option<(Restart, Release)>, /// An update is installed and waits for the app to exit; nothing starts a second one. installed: bool, - /// Set when the `Updater` is dropped. An install worker sends its outcome while holding it, - /// and starts a finished installation's helper itself once it is set. - dropped: Arc>, + /// Set when the app exits: the `Updater` is dropped or [`Updater::start_pending_update`] is + /// called. An install worker sends its outcome while holding it, and starts a finished + /// installation's helper itself once it is set. + exiting: Arc>, /// The version-choice file to read in place of [`Pins::default_path`] (tests). pins_path: Option, /// The launcher's choice for this app when it is not stable, as of the last check or offer. @@ -128,7 +129,7 @@ impl Updater { prompt: None, restart: None, installed: false, - dropped: Arc::new(Mutex::new(false)), + exiting: Arc::new(Mutex::new(false)), pins_path: None, managed: None, translate: english, @@ -404,9 +405,14 @@ impl Updater { /// Starts the helper of an installed update that is still waiting for the app to close, so /// that the update installs once the app has exited. Call it when the app exits by a path /// that did not come through [`Action::Close`]; dropping the `Updater` calls it too. Later - /// calls do nothing. An installation still running when the `Updater` is dropped starts its - /// helper itself when it finishes. + /// calls do nothing. An installation still running at the first call starts its helper itself + /// when it finishes. pub fn start_pending_update(&mut self) { + // Held while the channel is read: a worker sent its outcome before, and it is read here, + // or it sends after and starts its helper itself. + let exiting = Arc::clone(&self.exiting); + let mut exiting = exiting.lock().unwrap_or_else(PoisonError::into_inner); + *exiting = true; // An installation that finished since the last `tick`. if let Some((rx, release)) = &self.install && let Ok(outcome) = rx.try_recv() @@ -457,11 +463,11 @@ impl Updater { let (tx, rx) = channel(); let (app, version, ctx, release) = (self.app, self.version.clone(), ctx.clone(), prompt.release.clone()); let worker_release = release.clone(); - let dropped = Arc::clone(&self.dropped); + let exiting = Arc::clone(&self.exiting); let spawned = std::thread::Builder::new().name("self-update".into()).spawn(move || { let run = || install(app, &installed, worker_release, &version).map_err(Failure::Installer); let outcome = std::panic::catch_unwind(std::panic::AssertUnwindSafe(run)).unwrap_or_else(|_| Err(Failure::panicked())); - deliver(app, &tx, &dropped, outcome); + deliver(app, &tx, &exiting, outcome); ctx.request_repaint(); }); match spawned { @@ -478,20 +484,15 @@ impl Drop for Updater { /// The app is exiting: an installed update that is still waiting starts its helper, which /// waits for this process to exit before it installs. fn drop(&mut self) { - // Held while the channel is read: a worker sent its outcome before, and it is read here, - // or it sends after and starts its helper itself. - let dropped = Arc::clone(&self.dropped); - let mut dropped = dropped.lock().unwrap_or_else(PoisonError::into_inner); - *dropped = true; self.start_pending_update(); } } -/// Sends the install worker's outcome to the [`Updater`]. When the `Updater` has been dropped -/// (the app is exiting), the worker starts a finished installation's helper itself, since -/// nothing will start it at a close. -fn deliver(app: &App, tx: &Sender>, dropped: &Mutex, outcome: Result) { - let gone = dropped.lock().unwrap_or_else(PoisonError::into_inner); +/// Sends the install worker's outcome to the [`Updater`]. When the app is exiting (the `Updater` +/// was dropped or [`Updater::start_pending_update`] was called), the worker starts a finished +/// installation's helper itself, since nothing will start it at a close. +fn deliver(app: &App, tx: &Sender>, exiting: &Mutex, outcome: Result) { + let gone = exiting.lock().unwrap_or_else(PoisonError::into_inner); let unsent = if *gone { Some(outcome) } else { tx.send(outcome).err().map(|SendError(outcome)| outcome) }; drop(gone); if let Some(Ok(restart)) = unsent @@ -936,13 +937,13 @@ mod tests { fn an_install_finishing_after_the_updater_is_dropped_starts_its_helper() { use std::sync::atomic::Ordering; let mut u = updater(Ok(None)); - let dropped = Arc::clone(&u.dropped); + let exiting = Arc::clone(&u.exiting); let (starts, restart) = counted_restart(); let (tx, rx) = channel(); u.install = Some((rx, newer())); drop(u); assert_eq!(starts.load(Ordering::SeqCst), 0); - deliver(&crate::suite::LAUNCHER, &tx, &dropped, Ok(restart)); + deliver(&crate::suite::LAUNCHER, &tx, &exiting, Ok(restart)); assert_eq!(starts.load(Ordering::SeqCst), 1); // Sent while the drop has read the channel but not yet released the receiver. @@ -958,6 +959,24 @@ mod tests { assert!(rx.try_recv().is_ok_and(|outcome| outcome.is_ok())); } + /// An installation that finishes after an app called [`Updater::start_pending_update`] at + /// exit, without dropping the updater, starts its helper once. + #[test] + fn an_install_finishing_after_start_pending_update_starts_its_helper() { + use std::sync::atomic::Ordering; + let mut u = updater(Ok(None)); + let exiting = Arc::clone(&u.exiting); + let (starts, restart) = counted_restart(); + let (tx, rx) = channel(); + u.install = Some((rx, newer())); + u.start_pending_update(); + assert_eq!(starts.load(Ordering::SeqCst), 0, "nothing has finished yet"); + deliver(&crate::suite::LAUNCHER, &tx, &exiting, Ok(restart)); + assert_eq!(starts.load(Ordering::SeqCst), 1); + drop(u); + assert_eq!(starts.load(Ordering::SeqCst), 1, "the helper starts once"); + } + /// A failed installation picked up at exit leaves nothing installed and nothing waiting. #[test] fn a_failed_install_picked_up_at_exit_leaves_nothing_waiting() { From 8f17421a0a1291c786394804c8219938c3d40d3c Mon Sep 17 00:00:00 2001 From: dotCooCoo Date: Sun, 8 Nov 2026 12:25:32 -0800 Subject: [PATCH 22/25] craft-update: say when a kept process is reaped and that start_pending_update is for exit A process that no thread could wait for is reaped when a later call starts another process after it has exited. start_pending_update is called only as the app exits: afterwards a finishing installation starts its helper itself instead of reporting through tick, and one still running when the process ends installs nothing. The reaping test waits for the process to exit through its output and fails instead of looping when reaping does not happen. --- crates/update/src/process.rs | 28 ++++++++++++++++++++++------ crates/update/src/updater.rs | 5 +++-- 2 files changed, 25 insertions(+), 8 deletions(-) diff --git a/crates/update/src/process.rs b/crates/update/src/process.rs index 71489934..d76811f9 100644 --- a/crates/update/src/process.rs +++ b/crates/update/src/process.rs @@ -116,7 +116,8 @@ fn read_all(pipe: Option) -> std::io::Resu } /// Starts `cmd` without waiting for it, and reaps it when it exits. The process keeps running -/// when no thread can be started to reap it; it is then reaped by a later call once it exits. +/// when no thread can be started to reap it. It is then reaped when a later call starts another +/// process after it has exited; until then, on Unix, it stays in the process table. pub fn spawn_detached(mut cmd: Command) -> Result<(), String> { let child = cmd.stdin(Stdio::null()).stdout(Stdio::null()).stderr(Stdio::null()).spawn().map_err(|e| format!("couldn't start it ({e})"))?; reap_in_background(child, "a started process"); @@ -128,7 +129,8 @@ pub fn spawn_detached(mut cmd: Command) -> Result<(), String> { static UNREAPED: Mutex> = Mutex::new(Vec::new()); /// Waits for `child` on its own thread. When no thread can be started, `child` is kept in -/// [`UNREAPED`] and waited for by a later call once it has exited. +/// [`UNREAPED`]. Each call first waits for the kept processes that have exited, so a kept process +/// is waited for at the first call after it exits. fn reap_in_background(child: Child, what: &str) { reap_in_background_with(child, what, |wait| std::thread::Builder::new().name("reap-child".into()).spawn(wait).map(drop)); } @@ -460,18 +462,32 @@ mod tests { /// does not stay in the process table. #[test] fn a_process_without_a_waiting_thread_is_reaped_later() { - let mut child = command("cat").stdin(Stdio::piped()).stdout(Stdio::null()).stderr(Stdio::null()).spawn().unwrap(); + let mut child = command("cat").stdin(Stdio::piped()).stdout(Stdio::piped()).stderr(Stdio::null()).spawn().unwrap(); let stdin = child.stdin.take().unwrap(); + let mut stdout = child.stdout.take().unwrap(); let pid = child.id(); let kept = || UNREAPED.lock().unwrap_or_else(PoisonError::into_inner).iter().any(|c| c.id() == pid); reap_in_background_with(child, "a test process", |_wait| Err(std::io::Error::other("no threads"))); assert!(kept(), "a running process with no thread to wait for it is kept"); - // `cat` exits when its input closes. + // `cat` exits when its input closes, and its output closes as it exits. drop(stdin); - while kept() { - reap_exited(); + let _ = std::io::copy(&mut stdout, &mut std::io::sink()); + // `ps` shows an exited process that nothing has waited for in state Z, and nothing once it + // has been waited for (another test's call may do that first). + let state = || { + let out = std::process::Command::new("ps").args(["-o", "stat=", "-p", &pid.to_string()]).output().unwrap(); + String::from_utf8_lossy(&out.stdout).trim().to_owned() + }; + loop { + let now = state(); + if now.is_empty() || now.starts_with('Z') { + break; + } std::thread::yield_now(); } + reap_exited(); + assert!(!kept(), "an exited process is waited for by the next call"); + assert_eq!(state(), "", "the waited-for process has left the process table"); } } diff --git a/crates/update/src/updater.rs b/crates/update/src/updater.rs index 8f3ad1b0..6ea4348b 100644 --- a/crates/update/src/updater.rs +++ b/crates/update/src/updater.rs @@ -405,8 +405,9 @@ impl Updater { /// Starts the helper of an installed update that is still waiting for the app to close, so /// that the update installs once the app has exited. Call it when the app exits by a path /// that did not come through [`Action::Close`]; dropping the `Updater` calls it too. Later - /// calls do nothing. An installation still running at the first call starts its helper itself - /// when it finishes. + /// calls do nothing. Call it only as the app exits: after the call, an installation that + /// finishes starts its helper itself and no longer reports through [`Updater::tick`]. An + /// installation still running when the process ends does not finish, and installs nothing. pub fn start_pending_update(&mut self) { // Held while the channel is read: a worker sent its outcome before, and it is read here, // or it sends after and starts its helper itself. From 5eb0bca0b185908d9c7b4503574480fc0a1b477a Mon Sep 17 00:00:00 2001 From: dotCooCoo Date: Sun, 8 Nov 2026 13:17:41 -0800 Subject: [PATCH 23/25] craft-update: start a detached process only from the thread that waits for it On Unix, spawn_detached starts the waiting thread first, and that thread starts the process and waits for it. When no thread can be started, the call returns an error and the process is not started, so no exited process is left unreaped. On Windows, where an exited process needs no wait, the process starts on the calling thread and no thread waits for it. The list of processes kept for a later call, and the reaping it needed, are removed. When the opener of open_url_with_fallback is still running after its timeout, the open-url thread waits for it. An opener whose state can't be read is stopped and waited for. The tests run spawn_detached's waiting-thread path and check that the waiting thread gets the process's exit status, that the process is not started when no thread can start, and that a program that cannot start reports it. They no longer need ps. --- crates/update/src/process.rs | 181 +++++++++++++++++++++-------------- 1 file changed, 108 insertions(+), 73 deletions(-) diff --git a/crates/update/src/process.rs b/crates/update/src/process.rs index d76811f9..23656b9f 100644 --- a/crates/update/src/process.rs +++ b/crates/update/src/process.rs @@ -2,8 +2,8 @@ use std::ffi::OsStr; use std::path::{Path, PathBuf}; -use std::process::{Child, Command, Stdio}; -use std::sync::{Arc, Mutex, PoisonError}; +use std::process::{Child, Command, ExitStatus, Stdio}; +use std::sync::mpsc; use std::time::{Duration, Instant}; /// A `Command` that won't flash a console window on Windows, and clears AppImage-injected @@ -115,48 +115,61 @@ fn read_all(pipe: Option) -> std::io::Resu }) } -/// Starts `cmd` without waiting for it, and reaps it when it exits. The process keeps running -/// when no thread can be started to reap it. It is then reaped when a later call starts another -/// process after it has exited; until then, on Unix, it stays in the process table. +/// Starts `cmd` without waiting for it. +/// +/// On Unix the process is reaped when it exits: a thread that waits for the process starts +/// first and starts the process itself. When no thread can be started, the call returns an error +/// and does not start the process. On Windows the process starts on the calling thread, and no +/// thread waits for it. pub fn spawn_detached(mut cmd: Command) -> Result<(), String> { - let child = cmd.stdin(Stdio::null()).stdout(Stdio::null()).stderr(Stdio::null()).spawn().map_err(|e| format!("couldn't start it ({e})"))?; - reap_in_background(child, "a started process"); - Ok(()) + cmd.stdin(Stdio::null()).stdout(Stdio::null()).stderr(Stdio::null()); + if cfg!(windows) { + return cmd.spawn().map(drop).map_err(|e| format!("couldn't start it ({e})")); + } + spawn_detached_with( + move || cmd.spawn(), + |wait| { + std::thread::Builder::new() + .name("reap-child".into()) + .spawn(move || { + let _ = wait(); + }) + .map(drop) + }, + ) } -/// Started processes that no thread waits for, because none could be started. On Unix a process -/// that exits stays in the process table until it is waited for. -static UNREAPED: Mutex> = Mutex::new(Vec::new()); - -/// Waits for `child` on its own thread. When no thread can be started, `child` is kept in -/// [`UNREAPED`]. Each call first waits for the kept processes that have exited, so a kept process -/// is waited for at the first call after it exits. -fn reap_in_background(child: Child, what: &str) { - reap_in_background_with(child, what, |wait| std::thread::Builder::new().name("reap-child".into()).spawn(wait).map(drop)); -} - -/// [`reap_in_background`] with `start_thread` in place of starting a thread (tests). -fn reap_in_background_with(child: Child, what: &str, start_thread: impl FnOnce(Box) -> std::io::Result<()>) { - reap_exited(); - let slot = Arc::new(Mutex::new(Some(child))); - let theirs = Arc::clone(&slot); - let wait = Box::new(move || { - let child = theirs.lock().unwrap_or_else(PoisonError::into_inner).take(); - if let Some(mut child) = child { - let _ = child.wait(); - } - }); - if let Err(e) = start_thread(wait) { - log::warn!("couldn't start a thread to wait for {what} ({e})"); - if let Some(child) = slot.lock().unwrap_or_else(PoisonError::into_inner).take() { - UNREAPED.lock().unwrap_or_else(PoisonError::into_inner).push(child); - } +/// The thread that [`spawn_detached`] starts on Unix: `start_thread` starts a thread that runs +/// the closure it is given, and that closure calls `spawn` and then waits for the process. +/// `spawn` is called on that thread only, so when `start_thread` fails nothing is started. +#[cfg_attr(windows, allow(dead_code))] +fn spawn_detached_with( + spawn: impl FnOnce() -> std::io::Result + Send + 'static, + start_thread: impl FnOnce(Box Option + Send>) -> std::io::Result<()>, +) -> Result<(), String> { + let (started, start_result) = mpsc::channel(); + start_thread(Box::new(move || start_and_wait(spawn, &started))).map_err(|e| format!("couldn't start a thread to wait for it ({e})"))?; + match start_result.recv() { + Ok(Ok(())) => Ok(()), + Ok(Err(e)) => Err(format!("couldn't start it ({e})")), + Err(_) => Err("couldn't start it (its thread ended before starting it)".into()), } } -/// Waits for the processes in [`UNREAPED`] that have exited, without blocking on the others. -fn reap_exited() { - UNREAPED.lock().unwrap_or_else(PoisonError::into_inner).retain_mut(|child| matches!(child.try_wait(), Ok(None))); +/// Calls `spawn`, sends whether that worked on `started`, and waits for the process to exit. +/// Returns its exit status, or `None` when it didn't start or couldn't be waited for. +#[cfg_attr(windows, allow(dead_code))] +fn start_and_wait(spawn: impl FnOnce() -> std::io::Result, started: &mpsc::Sender>) -> Option { + match spawn() { + Ok(mut child) => { + let _ = started.send(Ok(())); + child.wait().ok() + } + Err(e) => { + let _ = started.send(Err(e)); + None + } + } } /// Waits up to `timeout` for the process `pid` to exit, and says whether it has. A process that @@ -281,7 +294,9 @@ pub fn open_url_with_fallback(primary_url: &str, fallback_url: &str) { /// Starts the default system opener for `target` and returns `true` unless it fails to start /// or exits in error within `timeout`. An opener still running after `timeout` counts as -/// success and is reaped in the background. +/// success, and the call waits for it to exit before returning. An opener whose state can't be +/// read is stopped and waited for, and counts as a failure. [`open_url_with_fallback`] calls this +/// on its own `open-url` thread. /// /// `explorer` exits with an error status even when it opens the target, so on Windows only a /// failure to start counts; the scheme check covers the rest. @@ -309,10 +324,14 @@ fn open_and_wait_success(target: &str, timeout: Duration) -> bool { Ok(Some(status)) => return status.success(), Ok(None) if start.elapsed() < timeout => std::thread::sleep(Duration::from_millis(25)), Ok(None) => { - reap_in_background(child, &format!("the opener of {target}")); + let _ = child.wait(); return true; } - Err(_) => return false, + Err(_) => { + let _ = child.kill(); + let _ = child.wait(); + return false; + } } } } @@ -457,38 +476,6 @@ mod tests { assert_eq!(is_running(Path::new(&requested)), expected, "{requested}"); } } - - /// A process that no thread can wait for is kept, and waited for once it has exited, so it - /// does not stay in the process table. - #[test] - fn a_process_without_a_waiting_thread_is_reaped_later() { - let mut child = command("cat").stdin(Stdio::piped()).stdout(Stdio::piped()).stderr(Stdio::null()).spawn().unwrap(); - let stdin = child.stdin.take().unwrap(); - let mut stdout = child.stdout.take().unwrap(); - let pid = child.id(); - let kept = || UNREAPED.lock().unwrap_or_else(PoisonError::into_inner).iter().any(|c| c.id() == pid); - reap_in_background_with(child, "a test process", |_wait| Err(std::io::Error::other("no threads"))); - assert!(kept(), "a running process with no thread to wait for it is kept"); - // `cat` exits when its input closes, and its output closes as it exits. - drop(stdin); - let _ = std::io::copy(&mut stdout, &mut std::io::sink()); - // `ps` shows an exited process that nothing has waited for in state Z, and nothing once it - // has been waited for (another test's call may do that first). - let state = || { - let out = std::process::Command::new("ps").args(["-o", "stat=", "-p", &pid.to_string()]).output().unwrap(); - String::from_utf8_lossy(&out.stdout).trim().to_owned() - }; - loop { - let now = state(); - if now.is_empty() || now.starts_with('Z') { - break; - } - std::thread::yield_now(); - } - reap_exited(); - assert!(!kept(), "an exited process is waited for by the next call"); - assert_eq!(state(), "", "the waited-for process has left the process table"); - } } #[cfg(test)] @@ -549,6 +536,54 @@ mod run_tests { assert!(start.elapsed() < Duration::from_secs(5)); } + /// The thread that starts a detached process waits for it, so the process is reaped when it + /// exits: only a wait gives its exit status. + #[test] + fn a_detached_process_is_waited_for() { + let (program, args) = shell("exit 7"); + let mut cmd = command(program); + cmd.args(args); + let (status_sent, status) = mpsc::channel(); + let result = spawn_detached_with( + move || cmd.spawn(), + move |wait| { + std::thread::Builder::new() + .spawn(move || { + let _ = status_sent.send(wait()); + }) + .map(drop) + }, + ); + assert_eq!(result, Ok(())); + assert_eq!(status.recv().ok().flatten().and_then(|status| status.code()), Some(7), "the waiting thread gets the exit status"); + } + + /// When no thread can be started to wait for a process, the process is not started and the + /// call says why. + #[test] + fn no_process_starts_without_a_thread_to_wait_for_it() { + let (program, args) = shell("exit 0"); + let mut cmd = command(program); + cmd.args(args); + let spawned = std::sync::Arc::new(std::sync::atomic::AtomicBool::new(false)); + let spawn_called = std::sync::Arc::clone(&spawned); + let result = spawn_detached_with( + move || { + spawn_called.store(true, std::sync::atomic::Ordering::SeqCst); + cmd.spawn() + }, + |_wait| Err(std::io::Error::other("no threads")), + ); + assert_eq!(result, Err("couldn't start a thread to wait for it (no threads)".into())); + assert!(!spawned.load(std::sync::atomic::Ordering::SeqCst), "the process was started"); + } + + #[test] + fn a_detached_program_that_cannot_start_says_so() { + let result = spawn_detached(command("craft-update-test-no-such-program")); + assert!(matches!(&result, Err(e) if e.starts_with("couldn't start it (")), "{result:?}"); + } + #[test] fn a_silent_failure_and_a_success() { let (program, args) = shell("exit 4"); From f89c6686df2006f12318d76bfc39b1845b156adf Mon Sep 17 00:00:00 2001 From: dotCooCoo Date: Sun, 8 Nov 2026 13:52:56 -0800 Subject: [PATCH 24/25] craft-update: keep link addresses out of the opener's warnings The warnings that open_url_with_fallback and the system opener log no longer include the address being opened, so a link carrying a token or personal details does not reach the application's log. The reasons from the system stay in the messages. --- crates/update/src/process.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/crates/update/src/process.rs b/crates/update/src/process.rs index 23656b9f..e39eb378 100644 --- a/crates/update/src/process.rs +++ b/crates/update/src/process.rs @@ -283,12 +283,12 @@ pub fn open_url_with_fallback(primary_url: &str, fallback_url: &str) { let fallback = fallback_url.to_string(); let opener = std::thread::Builder::new().name("open-url".into()).spawn(move || { if !open_and_wait_success(&primary, Duration::from_millis(1500)) { - log::warn!("couldn't open {primary}; falling back to {fallback}"); + log::warn!("couldn't open a link; opening its fallback"); let _ = open(std::ffi::OsStr::new(&fallback)); } }); if let Err(e) = opener { - log::warn!("couldn't start a thread to open {primary_url} ({e})"); + log::warn!("couldn't start a thread to open a link ({e})"); } } @@ -304,7 +304,7 @@ fn open_and_wait_success(target: &str, timeout: Duration) -> bool { let mut cmd = match opener(std::ffi::OsStr::new(target)) { Ok(cmd) => cmd, Err(error) => { - log::warn!("couldn't resolve the system opener for {target}: {error}"); + log::warn!("couldn't resolve the system opener: {error}"); return false; } }; @@ -314,7 +314,7 @@ fn open_and_wait_success(target: &str, timeout: Duration) -> bool { let mut child = match cmd.stdin(Stdio::null()).stdout(Stdio::null()).stderr(Stdio::null()).spawn() { Ok(c) => c, Err(e) => { - log::warn!("couldn't start the opener for {target}: {e}"); + log::warn!("couldn't start the system opener: {e}"); return false; } }; From c365d75c48b0e47f4b43cdb6592d0cdc39d2d52e Mon Sep 17 00:00:00 2001 From: dotCooCoo Date: Sun, 8 Nov 2026 14:22:08 -0800 Subject: [PATCH 25/25] craft-update: the restart-helper test's comment matches spawn_detached spawn_detached now starts the process on its waiting thread, and the test starts the helper on the calling thread. The comment names only what the two share, standard streams set to null. --- crates/update/src/self_update.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/crates/update/src/self_update.rs b/crates/update/src/self_update.rs index 66bfbae4..01ec41da 100644 --- a/crates/update/src/self_update.rs +++ b/crates/update/src/self_update.rs @@ -2012,8 +2012,9 @@ mod tests { /// The restart helper starts only when the restart is started, at the app's exit: a close the /// user cancels leaves no helper counting down its one-minute wait. The test starts the helper - /// the way `spawn_detached` does and keeps it, and a thread that waits for the helper to exit - /// writes `exited`, so a helper that exits without writing `started` fails the test. + /// with its standard streams set to null, as `spawn_detached` does, and a thread that waits + /// for the helper to exit writes `exited`, so a helper that exits without writing `started` + /// fails the test. #[cfg(unix)] #[test] fn the_restart_helper_starts_only_when_the_restart_is_started() {