craft-update: an install that finishes after the updater is dropped starts its helper

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<Mutex<bool>>,
     /// The version-choice file to read in place of [`Pins::default_path`] (tests).
     pins_path: Option<PathBuf>,
     /// 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<Result<Restart, Failure>>, dropped: &Mutex<bool>, outcome: Result<Restart, Failure>) {
+    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<Restart, String> {
     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() {