fix(agent): improve MSI installation handling and Hub Service updates

mamoreau-devolutions · mamoreau-devolutions · commit e31ac917af37 · 2025-11-06T15:29:33.000-05:00
This commit addresses several issues with the updater's MSI installation and Hub Service update process:

1. Strip UTF-8 BOM from update.json parsing

   - Some editors add UTF-8 BOM (0xEF 0xBB 0xBF) which caused parse failures

   - Now strips BOM before JSON deserialization if present

2. Validate MSI exit codes properly

   - Previously only checked if msiexec launched, not if it succeeded

   - Now validates exit codes: 0 = success, 3010/1641 = success with reboot

     (logged as warning since our installers shouldn't require reboot)

   - Other codes properly treated as failures

   - Applied to both install and uninstall operations

3. Hub Service installer parameter improvements

   - Removed P.SERVICESTART parameter for Hub Service (not supported)

   - Added ADDLOCAL parameter generation based on installed services

   - Maps service names to MSI features: PAM, Encryption, Reporting

   - Preserves user's feature selection during updates

4. Enhanced service restart logic for Hub Service

   - Gateway: only restarts services with manual startup that were running

   - Hub Service: restarts all services that were running (since we can't

     control startup mode via installer parameters)

These changes ensure Hub Service updates preserve the installed features and properly handle installation success/failure cases.
diff --git a/devolutions-agent/src/updater/mod.rs b/devolutions-agent/src/updater/mod.rs
@@ -210,8 +210,16 @@ async fn read_update_json(update_file_path: &Utf8Path) -> anyhow::Result<UpdateJ
     let update_json_data = fs::read(update_file_path)
         .await
         .context("failed to read update.json file")?;
+    
+    // Strip UTF-8 BOM if present (some editors add it)
+    let data_without_bom = if update_json_data.starts_with(&[0xEF, 0xBB, 0xBF]) {
+        &update_json_data[3..]
+    } else {
+        &update_json_data
+    };
+    
     let update_json: UpdateJson =
-        serde_json::from_slice(&update_json_data).context("failed to parse update.json file")?;
+        serde_json::from_slice(data_without_bom).context("failed to parse update.json file")?;
 
     Ok(update_json)
 }
diff --git a/devolutions-agent/src/updater/package.rs b/devolutions-agent/src/updater/package.rs
@@ -67,14 +67,46 @@ async fn install_msi(ctx: &UpdaterCtx, path: &Utf8Path, log_path: &Utf8Path) ->
         }
     }
 
-    if msi_install_result.is_err() {
-        return Err(UpdaterError::MsiInstall {
-            product: ctx.product,
-            msi_path: path.to_owned(),
-        });
+    match msi_install_result {
+        Ok(status) => {
+            let exit_code = status.code().unwrap_or(-1);
+            
+            // MSI exit codes:
+            // 0 = Success
+            // 3010 = Success but reboot required (unexpected - our installers shouldn't require reboot)
+            // 1641 = Success and reboot initiated
+            // Other codes = Error
+            match exit_code {
+                0 => {
+                    info!("MSI installation completed successfully");
+                    Ok(())
+                }
+                3010 | 1641 => {
+                    // Our installers should not require a reboot, but if they do, log as warning
+                    // and continue since the installation technically succeeded
+                    warn!(
+                        %exit_code,
+                        "MSI installation completed but unexpectedly requires system reboot"
+                    );
+                    Ok(())
+                }
+                _ => {
+                    error!(%exit_code, "MSI installation failed with exit code");
+                    Err(UpdaterError::MsiInstall {
+                        product: ctx.product,
+                        msi_path: path.to_owned(),
+                    })
+                }
+            }
+        }
+        Err(_) => {
+            error!("Failed to execute msiexec command");
+            Err(UpdaterError::MsiInstall {
+                product: ctx.product,
+                msi_path: path.to_owned(),
+            })
+        }
     }
-
-    Ok(())
 }
 
 async fn uninstall_msi(ctx: &UpdaterCtx, product_code: Uuid, log_path: &Utf8Path) -> Result<(), UpdaterError> {
@@ -101,14 +133,47 @@ async fn uninstall_msi(ctx: &UpdaterCtx, product_code: Uuid, log_path: &Utf8Path
         }
     }
 
-    if msi_uninstall_result.is_err() {
-        return Err(UpdaterError::MsiUninstall {
-            product: ctx.product,
-            product_code,
-        });
+    match msi_uninstall_result {
+        Ok(status) => {
+            let exit_code = status.code().unwrap_or(-1);
+            
+            // MSI exit codes:
+            // 0 = Success
+            // 3010 = Success but reboot required (unexpected - our installers shouldn't require reboot)
+            // 1641 = Success and reboot initiated
+            // Other codes = Error
+            match exit_code {
+                0 => {
+                    info!(%product_code, "MSI uninstallation completed successfully");
+                    Ok(())
+                }
+                3010 | 1641 => {
+                    // Our installers should not require a reboot, but if they do, log as warning
+                    // and continue since the uninstallation technically succeeded
+                    warn!(
+                        %exit_code,
+                        %product_code,
+                        "MSI uninstallation completed but unexpectedly requires system reboot"
+                    );
+                    Ok(())
+                }
+                _ => {
+                    error!(%exit_code, %product_code, "MSI uninstallation failed with exit code");
+                    Err(UpdaterError::MsiUninstall {
+                        product: ctx.product,
+                        product_code,
+                    })
+                }
+            }
+        }
+        Err(_) => {
+            error!(%product_code, "Failed to execute msiexec command");
+            Err(UpdaterError::MsiUninstall {
+                product: ctx.product,
+                product_code,
+            })
+        }
     }
-
-    Ok(())
 }
 
 fn ensure_enough_rights() -> Result<(), UpdaterError> {
diff --git a/devolutions-agent/src/updater/product_actions.rs b/devolutions-agent/src/updater/product_actions.rs
@@ -5,6 +5,10 @@ use crate::updater::{Product, UpdaterError};
 const GATEWAY_SERVICE_NAME: &str = "DevolutionsGateway";
 
 // Hub Service installs up to 3 separate Windows services (depending on selected features)
+// Service Name -> MSI Feature mapping for ADDLOCAL parameter:
+//   "Devolutions Hub PAM Service" -> "PAM"
+//   "Devolutions Hub Encryption Service" -> "Encryption"
+//   "Devolutions Hub Reporting Service" -> "Reporting"
 const HUB_SERVICE_NAMES: &[&str] = &[
     "Devolutions Hub PAM Service",
     "Devolutions Hub Encryption Service",
@@ -99,25 +103,31 @@ impl ServiceUpdateActions {
                 continue;
             }
 
-            // Start service if it was running prior to the update, but service startup
-            // was set to manual.
-            if !state.startup_was_automatic && state.was_running {
-                info!("Starting '{}' service after update", state.name);
-
-                match service_manager.open_service_all_access(state.name) {
-                    Ok(service) => {
+            match service_manager.open_service_all_access(state.name) {
+                Ok(service) => {
+                    // Start service if it was running prior to the update
+                    // For Gateway: only if startup was manual (automatic services will auto-start)
+                    // For Hub Service: always start if it was running, since we can't control
+                    //                  startup mode via P.SERVICESTART parameter
+                    let should_start = match self.product {
+                        Product::Gateway => !state.startup_was_automatic && state.was_running,
+                        Product::HubService => state.was_running,
+                    };
+
+                    if should_start {
+                        info!("Starting '{}' service after update", state.name);
                         service.start()?;
                         info!("Service '{}' started", state.name);
-                    }
-                    Err(e) => {
-                        warn!("Failed to start service '{}' after update: {}", state.name, e);
+                    } else {
+                        debug!(
+                            "Service '{}' doesn't need manual restart (automatic_startup: {}, was_running: {})",
+                            state.name, state.startup_was_automatic, state.was_running
+                        );
                     }
                 }
-            } else {
-                debug!(
-                    "Service '{}' doesn't need manual restart (automatic_startup: {}, was_running: {})",
-                    state.name, state.startup_was_automatic, state.was_running
-                );
+                Err(e) => {
+                    warn!("Failed to access service '{}' after update: {}", state.name, e);
+                }
             }
         }
 
@@ -138,21 +148,43 @@ impl ProductUpdateActions for ServiceUpdateActions {
         // When performing update, we want to make sure the service startup mode is restored to the
         // previous state. (Installer sets Manual by default).
 
-        // For products with a single primary service, check if it should be automatic
-        if self.service_states.len() == 1 && self.service_states[0].startup_was_automatic {
-            info!("Adjusting MSIEXEC parameters for {} service startup mode", self.product);
-            return vec!["P.SERVICESTART=Automatic".to_owned()];
-        }
-
-        // For Hub Service with multiple services, check if any PAM service should be automatic
-        // (The MSI installer controls the main PAM service startup via P.SERVICESTART)
-        if self.product == Product::HubService {
-            if let Some(pam_state) = self.service_states.iter().find(|s| s.exists && s.name.contains("PAM")) {
-                if pam_state.startup_was_automatic {
-                    info!("Adjusting MSIEXEC parameters for Hub PAM service startup mode");
+        match self.product {
+            Product::Gateway => {
+                // Gateway installer supports P.SERVICESTART property
+                if self.service_states.len() == 1 && self.service_states[0].startup_was_automatic {
+                    info!("Adjusting MSIEXEC parameters for Gateway service startup mode");
                     return vec!["P.SERVICESTART=Automatic".to_owned()];
                 }
             }
+            Product::HubService => {
+                // Hub Service installer requires ADDLOCAL parameter to specify which services to install.
+                // Build the list based on currently installed services.
+                let mut features = Vec::new();
+                
+                for state in &self.service_states {
+                    if state.exists {
+                        // Map service name to MSI feature name
+                        let feature = match state.name {
+                            "Devolutions Hub PAM Service" => "PAM",
+                            "Devolutions Hub Encryption Service" => "Encryption",
+                            "Devolutions Hub Reporting Service" => "Reporting",
+                            _ => {
+                                warn!("Unknown Hub Service: {}", state.name);
+                                continue;
+                            }
+                        };
+                        features.push(feature);
+                    }
+                }
+
+                if !features.is_empty() {
+                    let addlocal = format!("ADDLOCAL={}", features.join(","));
+                    info!("Adjusting MSIEXEC parameters for Hub Service features: {}", addlocal);
+                    return vec![addlocal];
+                } else {
+                    warn!("No Hub Service features detected, installer may use defaults");
+                }
+            }
         }
 
         Vec::new()
