From 53fbd5b6369f10530fbcc37d38ba0f6076985a69 Mon Sep 17 00:00:00 2001 From: dotCooCoo Date: Sat, 7 Nov 2026 19:44:54 -0800 Subject: [PATCH] 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]