From 324f06f6394d9fc1d6f1eef35a7af9bebee23560 Mon Sep 17 00:00:00 2001 From: dotCooCoo Date: Sun, 8 Nov 2026 12:04:02 -0800 Subject: [PATCH 1/4] 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 2/4] 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 3/4] 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 4/4] 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.