feat: return non-zero exit code on activation errors

Use lazy_errors to accumulate errors during activation and return a
non-zero exit code if any errors were encountered, while still applying
as many changes as possible.

Author: jfroche

Cargo.lock

@@ -18,6 +18,7 @@ dbus = "0.9.7"
 env_logger = "0.11.0"
 im = { version = "15.1.0", features = ["serde"] }
 itertools = "0.14.0"
+lazy_errors = "0.10.1"
 log = "0.4.17"
 nix = { version = "0.31.0", features = ["hostname", "user"] }
 regex = "1.11.1"

Cargo.toml

@@ -21,6 +21,7 @@ dbus.workspace = true
 env_logger.workspace = true
 im.workspace = true
 itertools.workspace = true
+lazy_errors.workspace = true
 log.workspace = true
 nix.workspace = true
 regex.workspace = true

crates/system-manager-engine/Cargo.toml

@@ -4,15 +4,34 @@ mod tmp_files;
 pub(crate) mod users;
 
 use anyhow::Result;
+use lazy_errors::prelude::*;
 use serde::{Deserialize, Serialize};
 use std::fs::DirBuilder;
 use std::path::{Path, PathBuf};
-use std::{fs, io, process};
+use std::{fmt, fs, io, process};
 use thiserror::Error;
 
 use crate::activate::etc_files::FileTree;
 use crate::{StorePath, STATE_FILE_NAME, SYSTEM_MANAGER_STATE_DIR};
 
+pub(crate) fn collect_activation_result_err<R, F, M>(
+    res: ActivationResult<R>,
+    err_stash: &mut ErrorStash<F, M>,
+) -> ActivationResult<R>
+where
+    M: fmt::Display,
+    F: FnOnce() -> M,
+{
+    res.map_err(|e| {
+        let ActivationError::WithPartialResult {
+            result: _,
+            ref source,
+        } = e;
+        err_stash.push(source.to_string());
+        e
+    })
+}
+
 #[derive(Error, Debug)]
 pub enum ActivationError<R> {
     #[error("")]
@@ -77,60 +96,68 @@ pub fn activate(store_path: &StorePath, ephemeral: bool) -> Result<()> {
 
     let state_file = &get_state_file()?;
     let old_state = State::from_file(state_file)?;
+    let mut errs = ErrorStash::new(|| "Activation completed with errors");
 
     log::info!("Activating etc files...");
 
-    match etc_files::activate(store_path, old_state.file_tree, ephemeral) {
+    let etc_result = collect_activation_result_err(
+        etc_files::activate(store_path, old_state.file_tree, ephemeral),
+        &mut errs,
+    );
+    if let Err(ref e) = etc_result {
+        log::error!("Error during activation: {e:?}");
+    }
+
+    // Only run daemon reload, userborn, tmpfiles, and services when etc files
+    // were fully applied. Partial etc results mean services may reference
+    // missing config files.
+    let (etc_tree, services) = match etc_result {
         Ok(etc_tree) => {
             log::info!("Restarting sysinit-reactivation.target...");
-            services::restart_sysinit_reactivation_target()?;
+            let sysinit_result = services::restart_sysinit_reactivation_target();
+            if let Err(ref e) = sysinit_result {
+                log::error!("Error restarting sysinit-reactivation.target: {e}");
+            }
+            sysinit_result.or_stash(&mut errs);
 
             // Restart userborn before tmpfiles so users exist when tmpfiles runs
-            if let Err(e) = services::restart_userborn_if_exists() {
+            let userborn_result = services::restart_userborn_if_exists();
+            if let Err(ref e) = userborn_result {
                 log::error!("Error restarting userborn.service: {e}");
             }
+            userborn_result.or_stash(&mut errs);
 
             log::info!("Activating tmp files...");
-            let tmp_result = tmp_files::activate(&etc_tree);
-            if let Err(e) = &tmp_result {
-                log::error!("Error during activation of tmp files");
-                log::error!("{e}");
-            } else {
-                log::debug!("Successfully created tmp files");
+            let tmp_result =
+                collect_activation_result_err(tmp_files::activate(&etc_tree), &mut errs);
+            if let Err(ref e) = tmp_result {
+                log::error!("Error during activation of tmp files: {e}");
             }
 
             log::info!("Activating systemd services...");
-            let final_state = match services::activate(store_path, old_state.services, ephemeral) {
-                Ok(services) => State {
-                    file_tree: etc_tree,
-                    services,
-                },
-                Err(ActivationError::WithPartialResult { result, source }) => {
-                    log::error!("Error during activation: {source:?}");
-                    State {
-                        file_tree: etc_tree,
-                        services: result,
-                    }
-                }
-            };
-            final_state.write_to_file(state_file)?;
-
-            if let Err(e) = tmp_result {
-                return Err(e.into());
+            let svc_result = collect_activation_result_err(
+                services::activate(store_path, old_state.services, ephemeral),
+                &mut errs,
+            );
+            if let Err(ref e) = svc_result {
+                log::error!("Error during activation: {e:?}");
             }
-
-            Ok(())
-        }
-        Err(ActivationError::WithPartialResult { result, source }) => {
-            log::error!("Error during activation: {source:?}");
-            let final_state = State {
-                file_tree: result,
-                ..old_state
+            let services = match svc_result {
+                Ok(s) => s,
+                Err(ActivationError::WithPartialResult { result, .. }) => result,
             };
-            final_state.write_to_file(state_file)?;
-            Ok(())
+            (etc_tree, services)
         }
-    }
+        Err(ActivationError::WithPartialResult { result, .. }) => (result, old_state.services),
+    };
+
+    let final_state = State {
+        file_tree: etc_tree,
+        services,
+    };
+    final_state.write_to_file(state_file).or_stash(&mut errs);
+
+    Ok(Result::<(), _>::from(errs)?)
 }
 
 pub fn prepopulate(store_path: &StorePath, ephemeral: bool) -> Result<()> {
@@ -146,36 +173,47 @@ pub fn prepopulate(store_path: &StorePath, ephemeral: bool) -> Result<()> {
 
     let state_file = &get_state_file()?;
     let old_state = State::from_file(state_file)?;
+    let mut errs = ErrorStash::new(|| "Pre-population completed with errors");
 
     log::info!("Activating etc files...");
 
-    match etc_files::activate(store_path, old_state.file_tree, ephemeral) {
+    let etc_result = collect_activation_result_err(
+        etc_files::activate(store_path, old_state.file_tree, ephemeral),
+        &mut errs,
+    );
+    if let Err(ref e) = etc_result {
+        log::error!("Error during activation: {e:?}");
+    }
+
+    // Only register services when etc files were fully applied, preserving
+    // old service state on etc failure to avoid persisting state from a
+    // partial run.
+    let (etc_tree, services) = match etc_result {
         Ok(etc_tree) => {
             log::info!("Registering systemd services...");
-            match services::get_active_services(store_path, old_state.services) {
-                Ok(services) => State {
-                    file_tree: etc_tree,
-                    services,
-                },
-                Err(ActivationError::WithPartialResult { result, source }) => {
-                    log::error!("Error during activation: {source:?}");
-                    State {
-                        file_tree: etc_tree,
-                        services: result,
-                    }
-                }
-            }
-        }
-        Err(ActivationError::WithPartialResult { result, source }) => {
-            log::error!("Error during activation: {source:?}");
-            State {
-                file_tree: result,
-                ..old_state
+            let svc_result = collect_activation_result_err(
+                services::get_active_services(store_path, old_state.services),
+                &mut errs,
+            );
+            if let Err(ref e) = svc_result {
+                log::error!("Error during activation: {e:?}");
             }
+            let services = match svc_result {
+                Ok(s) => s,
+                Err(ActivationError::WithPartialResult { result, .. }) => result,
+            };
+            (etc_tree, services)
         }
-    }
-    .write_to_file(state_file)?;
-    Ok(())
+        Err(ActivationError::WithPartialResult { result, .. }) => (result, old_state.services),
+    };
+
+    let final_state = State {
+        file_tree: etc_tree,
+        services,
+    };
+    final_state.write_to_file(state_file).or_stash(&mut errs);
+
+    Ok(Result::<(), _>::from(errs)?)
 }
 
 fn run_preactivation_assertions(store_path: &StorePath) -> Result<process::ExitStatus> {
@@ -198,3 +236,38 @@ pub(crate) fn get_state_file() -> Result<PathBuf> {
         .create(SYSTEM_MANAGER_STATE_DIR)?;
     Ok(state_file)
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn empty_stash_returns_ok() {
+        let errs = ErrorStash::new(|| "Activation completed with errors");
+        let result: std::result::Result<(), lazy_errors::prelude::Error> = errs.into();
+        assert!(result.is_ok());
+    }
+
+    #[test]
+    fn single_stashed_error_returns_err() {
+        let mut errs = ErrorStash::new(|| "Activation completed with errors");
+        Err::<(), _>(anyhow::anyhow!("userborn failed")).or_stash(&mut errs);
+        let result: std::result::Result<(), lazy_errors::prelude::Error> = errs.into();
+        let err = result.unwrap_err();
+        let msg = format!("{err:#}");
+        assert!(msg.contains("userborn failed"), "message was: {msg}");
+    }
+
+    #[test]
+    fn multiple_stashed_errors_returns_combined_err() {
+        let mut errs = ErrorStash::new(|| "Deactivation completed with errors");
+        Err::<(), _>(anyhow::anyhow!("userborn failed")).or_stash(&mut errs);
+        Err::<(), _>(anyhow::anyhow!("tmpfiles failed")).or_stash(&mut errs);
+        let result: std::result::Result<(), lazy_errors::prelude::Error> = errs.into();
+        let err = result.unwrap_err();
+        let msg = format!("{err:#}");
+        assert!(msg.contains("Deactivation"), "message was: {msg}");
+        assert!(msg.contains("userborn failed"), "message was: {msg}");
+        assert!(msg.contains("tmpfiles failed"), "message was: {msg}");
+    }
+}