ares-install: stop reporting a successful install as a failure

ares-install/src/install.rs

@@ -19,6 +19,11 @@ pub(crate) trait InstallApp {
 #[derive(Debug)]
 pub enum InstallError {
     Response { error_code: i32, reason: String },
+    /// The install stream ended without ever saying how it went.
+    NoVerdict,
+    /// The connection went down with the install already under way, so what
+    /// the device did with the package is not known.
+    Interrupted { device: String },
     ChecksumMismatch { expected: String, actual: String },
     Luna(LunaError),
     Transfer(TransferError),
@@ -31,6 +36,17 @@ impl Display for InstallError {
             InstallError::Response { error_code, reason } => {
                 write!(f, "{reason} (error {error_code})")
             }
+            InstallError::NoVerdict => write!(
+                f,
+                "the device stopped reporting before it said whether the package installed"
+            ),
+            InstallError::Interrupted { device } => write!(
+                f,
+                "lost the connection to {device} while the install was running. The device may \
+                 have installed the package anyway - `ares-install -d {device} --listfull` \
+                 prints the version that is on it. `--list` alone only names the app, which \
+                 says nothing about which build landed"
+            ),
             InstallError::ChecksumMismatch { expected, actual } => write!(
                 f,
                 "uploaded package is corrupted: expected sha256 {expected}, device has {actual}"
@@ -74,9 +90,8 @@ impl InstallApp for DeviceSession {
             IoError::new(
                 ErrorKind::Other,
                 format!(
-                    "Failed to generate checksum for {}: {:?}",
-                    package.as_ref().to_string_lossy(),
-                    e
+                    "Failed to generate checksum for {}: {e}",
+                    package.as_ref().to_string_lossy()
                 ),
             )
         })?;
@@ -114,7 +129,7 @@ impl InstallApp for DeviceSession {
         pb.set_message("Checking uploaded package");
         let verified = verify_upload(self, &ipk_path, &checksum);
         if let Err(e) = &verified {
-            pb.suspend(|| eprintln!("Upload of {package_display_name} is broken: {e:?}"));
+            pb.suspend(|| eprintln!("Upload of {package_display_name} is broken: {e}"));
         }
 
         let result = verified.and_then(|_| {
@@ -152,22 +167,51 @@ impl InstallApp for DeviceSession {
                         )
                     })
                     .next()
-                    .unwrap_or_else(|| Ok(String::new())),
+                    // Reaching the end of the stream having seen neither
+                    // "installed" nor a failure is not a success. Reporting one
+                    // claims a package is on the device that may well not be.
+                    .unwrap_or(Err(InstallError::NoVerdict)),
                 Err(e) => Err(e.into()),
             }
         });
 
+        // The install request is sent before the device answers it, so a
+        // connection that dies here says nothing about what the device did -
+        // and on some sets the install goes through regardless. Calling that a
+        // failed install is a guess, and the wrong one often enough to matter.
+        let result = result.map_err(|e| match e {
+            InstallError::Luna(LunaError::Session(_)) | InstallError::Io(_)
+                if !self.is_connected() =>
+            {
+                InstallError::Interrupted {
+                    device: self.device.name.clone(),
+                }
+            }
+            other => other,
+        });
+
         if let Ok(package_id) = &result {
             pb.suspend(|| println!("Installed package {}!", package_id));
         }
-        pb.suspend(|| println!("Deleting uploaded package..."));
 
         pb.set_prefix("Cleanup");
         pb.set_message("Deleting uploaded package");
 
-        if let Err(e) = self.rm(&ipk_path) {
+        // Cleaning up over a connection that is already gone only produces a
+        // second, confusing error - and it lands before the one that explains
+        // the run, because this happens on the way out. Say what was left
+        // behind instead, and let the real error through.
+        if self.is_connected() {
+            pb.suspend(|| println!("Deleting uploaded package..."));
+            if let Err(e) = self.rm(&ipk_path) {
+                pb.suspend(|| eprintln!("Failed to delete {ipk_path}: {e}"));
+            }
+        } else {
             pb.suspend(|| {
-                eprintln!("Failed to delete {}: {:?}", ipk_path, e);
+                eprintln!(
+                    "Lost the connection to {}, so {ipk_path} is still on the device.",
+                    self.device.name
+                );
             });
         }
         pb.finish_and_clear();

ares-install/src/main.rs

@@ -5,7 +5,7 @@ use ares_connection_lib::session::NewSession;
 use ares_device_lib::DeviceManager;
 use ares_device_lib::cli::unwrap_or_exit;
 use clap::Parser;
-use install::InstallApp;
+use install::{InstallApp, InstallError};
 use list::ListApps;
 
 use crate::remove::RemoveApp;
@@ -80,6 +80,13 @@ fn main() {
     } else if let Some(package) = cli.package {
         match session.install_app(package) {
             Ok(_) => {}
+            // An interrupted install reports itself: "Failed to install" would
+            // contradict the one thing the message has to say, which is that
+            // nobody knows yet.
+            Err(e @ InstallError::Interrupted { .. }) => {
+                eprintln!("{e}");
+                exit(1);
+            }
             Err(e) => {
                 eprintln!("Failed to install: {e}");
                 exit(1);