fix(updates): quote MSI INSTALL_ROOT to avoid msiexec 1639 and clear terminal transactions

docs/core-protocol.md

@@ -912,7 +912,11 @@ counts as success.
 On a later launch, the core reconciles these managed outcomes against the native
 package registration and its running version. A known live installer keeps
 `managed-pending` active. A completed installer resolves to `succeeded` or
-`failed`, and the core clears the persisted active transaction. An MSI reboot
+`failed`, and the core clears the persisted active transaction. Terminal helper
+outcomes (`succeeded`, `rolled-back`, `failed`) are likewise reported for that
+launch only; the core clears the persisted transaction after the first
+reconciliation so the same failure dialog does not reappear on every restart.
+An MSI reboot
 warning remains until Windows' per-boot sequence number changes. Sessions
 remain available, but another update must wait for the required reboot so it
 cannot overlap pending Windows file replacements. Reopening the app before

native/opennow-core/src/update_apply/managed.rs

@@ -159,6 +159,19 @@ mod windows {
         Ok(value.strip_prefix("\\\\?\\").unwrap_or(value).to_owned())
     }
 
+    pub(super) fn install_root_argument(target: &Path) -> Result<String, String> {
+        let path = argument_path(target)?;
+        if path.contains('"') || path.contains('\n') || path.contains('\r') {
+            return Err(
+                "Windows Installer requires an installation path without quotes".to_owned(),
+            );
+        }
+        if path.is_empty() || path.len() > 32767 {
+            return Err("Windows Installer requires a bounded installation path".to_owned());
+        }
+        Ok(format!("INSTALL_ROOT=\"{path}\""))
+    }
+
     struct Handle(u32);
     impl Drop for Handle {
         fn drop(&mut self) {
@@ -379,6 +392,25 @@ pub(super) fn windows_managed(target: &Path) -> Result<bool, String> {
     Ok(false)
 }
 
+#[cfg(not(windows))]
+fn msi_install_root_argument(target: &Path) -> Result<String, String> {
+    let value = target
+        .to_str()
+        .ok_or("Windows Installer requires a Unicode installation path")?;
+    let path = if let Some(unc) = value.strip_prefix("\\\\?\\UNC\\") {
+        format!("\\\\{unc}")
+    } else {
+        value.strip_prefix("\\\\?\\").unwrap_or(value).to_owned()
+    };
+    if path.contains('"') || path.contains('\n') || path.contains('\r') {
+        return Err("Windows Installer requires an installation path without quotes".to_owned());
+    }
+    if path.is_empty() || path.len() > 32767 {
+        return Err("Windows Installer requires a bounded installation path".to_owned());
+    }
+    Ok(format!("INSTALL_ROOT=\"{path}\""))
+}
+
 pub(super) fn install(plan: &Plan, directory: &Path) -> Result<(), String> {
     let identity = plan
         .managed_identity
@@ -404,17 +436,40 @@ pub(super) fn install(plan: &Plan, directory: &Path) -> Result<(), String> {
             let mut command = Command::new(Path::new(&system).join("System32/msiexec.exe"));
             command.arg("/i");
             #[cfg(windows)]
-            command.arg(windows::argument_path(&plan.package)?);
+            {
+                let package = windows::argument_path(&plan.package)?;
+                if package.contains('"') {
+                    return Err(
+                        "Windows Installer requires a package path without quotes".to_owned()
+                    );
+                }
+                command.arg(package);
+            }
             #[cfg(not(windows))]
             command.arg(&plan.package);
             command.args(["/passive", "/norestart", "REBOOT=ReallySuppress"]);
+            // msiexec parses the raw command line for PROPERTY=value tokens. A Rust
+            // Command::arg containing spaces would be quoted as a whole
+            // ("INSTALL_ROOT=C:\Program Files\..."), which msiexec rejects with 1639
+            // and a help dialog. Emit INSTALL_ROOT="..." with quotes only around the
+            // value, matching the installer contract test.
             #[cfg(windows)]
-            command.arg(format!(
-                "INSTALL_ROOT={}",
-                windows::argument_path(&plan.target)?
-            ));
+            {
+                use std::os::windows::process::CommandExt;
+                command.raw_arg(windows::install_root_argument(&plan.target)?);
+                let log = windows::argument_path(&directory.join("msiexec.log"))?;
+                if log.contains('"') {
+                    return Err("Windows Installer requires a log path without quotes".to_owned());
+                }
+                command.arg("/l*v");
+                command.arg(log);
+            }
             #[cfg(not(windows))]
-            command.arg(format!("INSTALL_ROOT={}", plan.target.display()));
+            {
+                command.arg(msi_install_root_argument(&plan.target)?);
+                command.arg("/l*v");
+                command.arg(directory.join("msiexec.log"));
+            }
             command
         }
         _ => return Err("Not a managed package update".to_owned()),
@@ -456,6 +511,12 @@ pub(super) fn install(plan: &Plan, directory: &Path) -> Result<(), String> {
     }
     if code != 0 {
         let mut message = installer_failure_message(plan.kind, code);
+        if plan.kind == InstallKind::WindowsMsi {
+            let log = directory.join("msiexec.log");
+            if log.is_file() {
+                message.push_str(&format!("; installer log: {}", log.display()));
+            }
+        }
         if prepare(plan.kind, &plan.package, &plan.target, &plan.version).is_ok()
             && super::canonical_file(&plan.application_executable).is_ok()
         {
@@ -510,6 +571,9 @@ fn installer_failure_message(kind: InstallKind, code: i32) -> String {
         (InstallKind::WindowsMsi, 1602) => {
             "Windows Installer was cancelled. Try installing the update again and complete the installer prompts"
         }
+        (InstallKind::WindowsMsi, 1639) => {
+            "Windows Installer rejected the update command line. Try installing the update again; the installer log beside the update outcome has details"
+        }
         _ => "Native package manager failed",
     };
     format!("{reason} (exit code {code}); update completion is not confirmed")
@@ -599,6 +663,11 @@ mod tests {
                 1602,
                 "Windows Installer was cancelled",
             ),
+            (
+                InstallKind::WindowsMsi,
+                1639,
+                "rejected the update command line",
+            ),
         ] {
             let message = installer_failure_message(kind, code);
             assert!(message.contains(explanation), "{message}");
@@ -611,6 +680,7 @@ mod tests {
     fn installer_failure_codes_are_interpreted_only_for_their_package_manager() {
         for (kind, code) in [
             (InstallKind::DebianPackage, 1602),
+            (InstallKind::DebianPackage, 1639),
             (InstallKind::WindowsMsi, 126),
             (InstallKind::WindowsMsi, 127),
             (InstallKind::DebianPackage, 1),
@@ -625,6 +695,29 @@ mod tests {
         }
     }
 
+    #[test]
+    fn msi_install_root_quotes_only_the_value_for_msiexec() {
+        #[cfg(windows)]
+        let argument =
+            windows::install_root_argument(Path::new(r"C:\Program Files\OpenNOW Nightly")).unwrap();
+        #[cfg(not(windows))]
+        let argument =
+            msi_install_root_argument(Path::new(r"C:\Program Files\OpenNOW Nightly")).unwrap();
+        assert_eq!(
+            argument,
+            r#"INSTALL_ROOT="C:\Program Files\OpenNOW Nightly""#
+        );
+        #[cfg(windows)]
+        let stripped = windows::install_root_argument(Path::new(r"\\?\C:\OpenNOW")).unwrap();
+        #[cfg(not(windows))]
+        let stripped = msi_install_root_argument(Path::new(r"\\?\C:\OpenNOW")).unwrap();
+        assert_eq!(stripped, r#"INSTALL_ROOT="C:\OpenNOW""#);
+        #[cfg(windows)]
+        assert!(windows::install_root_argument(Path::new("C:\\evil\"quote")).is_err());
+        #[cfg(not(windows))]
+        assert!(msi_install_root_argument(Path::new("C:\\evil\"quote")).is_err());
+    }
+
     #[cfg(target_os = "linux")]
     #[test]
     fn deb_ownership_matches_canonical_directories_but_rejects_file_symlinks() {

native/opennow-core/src/updater.rs

@@ -95,11 +95,17 @@ impl UpdaterService {
                         "idle",
                         "Ready to check GitHub Releases".to_owned(),
                     ),
-                    Err(error) => (
-                        None,
-                        "failed",
-                        format!("Could not recover update status: {error}"),
-                    ),
+                    Err(error) => {
+                        // A corrupt transaction must not block every future launch. Report it
+                        // once for this session, then drop the unreadable persistence so the
+                        // next startup begins clean. Diagnostics remain in the message.
+                        let _ = fs::remove_file(staging_dir.join("active-apply.json"));
+                        (
+                            None,
+                            "failed",
+                            format!("Could not recover update status: {error}"),
+                        )
+                    }
                 }
             };
         Ok(Self {
@@ -175,6 +181,7 @@ impl UpdaterService {
             state.transaction = None;
             state.message = "Checking GitHub Releases…".to_owned();
         }
+        let _ = fs::remove_file(self.staging_dir.join("active-apply.json"));
         let releases = self.fetch_releases(RELEASES_URL);
         let releases = match releases {
             Ok(releases) => releases,
@@ -300,6 +307,7 @@ impl UpdaterService {
             state.message = format!("Downloading OpenNOW {}…", available.version);
             available
         };
+        let _ = fs::remove_file(self.staging_dir.join("active-apply.json"));
         match self.download_verified(&available) {
             Ok(downloaded) => {
                 let mut state = self.state.lock().expect("updater state poisoned");
@@ -339,6 +347,7 @@ impl UpdaterService {
                 "Verifying and preparing a complete replacement before shutdown.".to_owned();
             state.transaction = None;
         }
+        let _ = fs::remove_file(self.staging_dir.join("active-apply.json"));
         let result = (|| {
             verify_downloaded_file(&downloaded)?;
             let application = std::env::var_os("OPENNOW_APP_EXECUTABLE")
@@ -371,6 +380,7 @@ impl UpdaterService {
             state.status = "failed";
             state.message = format!("Update preparation failed; OpenNOW remains open: {error}");
             state.transaction = None;
+            let _ = fs::remove_file(self.staging_dir.join("active-apply.json"));
             return Err(error);
         }
         Ok(self.state())
@@ -564,11 +574,15 @@ fn reconcile_transaction(state: &mut State, staging_dir: &Path) {
             state.message =
                 "The update helper outcome is missing or does not match the prepared version."
                     .to_owned();
+            // Terminal: report once for this launch, then drop persistence so the next
+            // startup does not re-show the same failure dialog.
+            clear_finished_transaction(state, staging_dir);
             return;
         }
         Err(error) => {
             state.status = "failed";
             state.message = error;
+            clear_finished_transaction(state, staging_dir);
             return;
         }
     };
@@ -605,14 +619,7 @@ fn reconcile_transaction(state: &mut State, staging_dir: &Path) {
                 state.message = format!("Could not recover native update completion: {error}");
             }
         }
-        state.transaction = None;
-        match fs::remove_file(staging_dir.join("active-apply.json")) {
-            Ok(()) => (),
-            Err(error) if error.kind() == std::io::ErrorKind::NotFound => (),
-            Err(error) => state.message.push_str(&format!(
-                "; could not remove the completed update transaction: {error}"
-            )),
-        }
+        clear_finished_transaction(state, staging_dir);
         return;
     }
     if matches!(
@@ -628,11 +635,13 @@ fn reconcile_transaction(state: &mut State, staging_dir: &Path) {
             Ok(false) => {
                 state.status = "failed";
                 state.message = "The update helper stopped before completing the transaction. OpenNOW will not close automatically.".to_owned();
+                clear_finished_transaction(state, staging_dir);
                 return;
             }
             Err(error) => {
                 state.status = "failed";
                 state.message = format!("Could not confirm update helper ownership: {error}");
+                clear_finished_transaction(state, staging_dir);
                 return;
             }
         }
@@ -649,6 +658,26 @@ fn reconcile_transaction(state: &mut State, staging_dir: &Path) {
         OutcomeStatus::RebootRequired => "reboot-required",
     };
     state.message = outcome.message;
+    if matches!(
+        outcome.status,
+        OutcomeStatus::Completed | OutcomeStatus::RolledBack | OutcomeStatus::Failed
+    ) {
+        // Terminal helper outcomes are reported for this launch only. Clearing the
+        // persisted transaction prevents the same failure dialog on every restart,
+        // matching the managed-recovery behavior documented in core-protocol.md.
+        clear_finished_transaction(state, staging_dir);
+    }
+}
+
+fn clear_finished_transaction(state: &mut State, staging_dir: &Path) {
+    state.transaction = None;
+    match fs::remove_file(staging_dir.join("active-apply.json")) {
+        Ok(()) => (),
+        Err(error) if error.kind() == std::io::ErrorKind::NotFound => (),
+        Err(error) => state.message.push_str(&format!(
+            "; could not remove the completed update transaction: {error}"
+        )),
+    }
 }
 
 fn restore_downloaded_status(state: &mut State) {
@@ -1211,6 +1240,9 @@ mod tests {
             update_apply::OutcomeStatus::Installing,
             update_apply::OutcomeStatus::AwaitingStartup,
         ] {
+            // Terminal failures clear persistence after the first report, so each
+            // stale-helper case needs fresh persistence to exercise the same path.
+            save_prepared_update(&updates, &prepared).unwrap();
             fs::write(
                 &prepared.outcome_path,
                 serde_json::to_vec(&update_apply::UpdateOutcome {
@@ -1228,6 +1260,49 @@ mod tests {
             assert_eq!(updater.state()["status"], "failed");
             assert_eq!(updater.state()["exitRequired"], false);
             assert!(!updater.installation_pending());
+            // The failure is reported for this launch only; the next startup begins
+            // clean instead of re-showing the same dialog.
+            assert!(!updates.join("active-apply.json").exists());
+        }
+    }
+
+    #[test]
+    fn terminal_helper_outcomes_are_reported_once_then_cleared() {
+        for (outcome, expected) in [
+            (update_apply::OutcomeStatus::Failed, "failed"),
+            (update_apply::OutcomeStatus::RolledBack, "rolled-back"),
+            (update_apply::OutcomeStatus::Completed, "succeeded"),
+        ] {
+            let directory = tempfile::tempdir().unwrap();
+            let updates = directory.path().join("updates");
+            fs::create_dir(&updates).unwrap();
+            let prepared = update_apply::PreparedUpdate {
+                plan_path: directory.path().join("plan.json"),
+                outcome_path: directory.path().join("outcome.json"),
+                version: "1.1.0".to_owned(),
+            };
+            save_prepared_update(&updates, &prepared).unwrap();
+            fs::write(
+                &prepared.outcome_path,
+                serde_json::to_vec(&update_apply::UpdateOutcome {
+                    schema_version: 1,
+                    version: prepared.version.clone(),
+                    status: outcome,
+                    message: "Terminal helper result".to_owned(),
+                    installed_version: None,
+                    restarted_process: None,
+                })
+                .unwrap(),
+            )
+            .unwrap();
+            let updater = UpdaterService::new(directory.path()).unwrap();
+            assert_eq!(updater.state()["status"], expected);
+            assert!(!updater.installation_pending());
+            assert!(!updates.join("active-apply.json").exists());
+            let restarted = UpdaterService::new(directory.path()).unwrap();
+            assert_eq!(restarted.state()["status"], "idle");
+            assert_eq!(restarted.state()["canCheck"], true);
+            assert!(!restarted.installation_pending());
         }
     }
 
@@ -1244,6 +1319,11 @@ mod tests {
         assert_eq!(updater.state()["status"], "failed");
         assert_eq!(updater.state()["exitRequired"], false);
         assert_eq!(updater.state()["canCheck"], true);
+        // Corrupt persistence is dropped after the first report so the next launch
+        // starts clean instead of failing forever.
+        assert!(!directory.path().join("updates/active-apply.json").exists());
+        let restarted = UpdaterService::new(directory.path()).unwrap();
+        assert_eq!(restarted.state()["status"], "idle");
     }
 
     #[test]