Skip to content

feat: return non-zero exit code on activation errors #378

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
jfroche wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat: return non-zero exit code on activation errors #378

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ env_logger = "0.11.0"
glob = "0.3.1"
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"
Expand Down
1 change: 1 addition & 0 deletions crates/system-manager-engine/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ env_logger.workspace = true
glob.workspace = true
im.workspace = true
itertools.workspace = true
lazy_errors.workspace = true
log.workspace = true
nix.workspace = true
regex.workspace = true
Expand Down
206 changes: 141 additions & 65 deletions crates/system-manager-engine/src/activate.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,18 +4,37 @@ mod tmp_files;
pub(crate) mod users;

use anyhow::{anyhow, Result};
use lazy_errors::prelude::*;
use serde::{Deserialize, Serialize};
use serde_json::error::Category;
use std::collections::HashSet;
use std::fs::DirBuilder;
use std::io::Seek;
use std::path::{Path, PathBuf};
use std::{fs, io, process};
use std::{fmt, fs, io, process};
use thiserror::Error;

use crate::activate::etc_files::etc_tree::StateV0;
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("")]
Expand Down Expand Up @@ -141,63 +160,75 @@ pub fn activate(store_path: &StorePath, ephemeral: bool) -> Result<()> {

let state_file = &get_state_file()?;
let old_state = StateV1::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
Copy link
Copy Markdown
Contributor

@picnoir picnoir Apr 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That being said, the machine is in a broken state and the services will likely break on reboot.

We should probably log a scary message here for now.

Long term, we probably want to make sure the files can be all copied before trying to write them on the disk to prevent this situation from happening.

Alternatively, we could backup the previous files during the activation and roll back everything if something goes wrong.

// 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}");
} else {
log::info!("Successfully restarted sysinit-reactivation.target");
}
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.files);
if let Err(e) = &tmp_result {
log::error!("Error during activation of tmp files");
log::error!("{e}");
let tmp_result =
collect_activation_result_err(tmp_files::activate(&etc_tree.files), &mut errs);
if let Err(ref e) = tmp_result {
log::error!("Error during activation of tmp files: {e}");
} else {
log::debug!("Successfully created tmp files");
log::info!("Successfully created tmp files");
}
Comment thread
jfroche marked this conversation as resolved.

log::info!("Activating systemd services...");
let final_state = match services::activate(store_path, old_state.services, ephemeral) {
Ok(services) => StateV1 {
file_tree: etc_tree,
services,
version: 1,
},
Err(ActivationError::WithPartialResult { result, source }) => {
log::error!("Error during activation: {source:?}");
StateV1 {
file_tree: etc_tree,
services: result,
version: 1,
}
}
};
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:?}");
} else {
log::info!("Successfully activated systemd services");
}

Ok(())
}
Err(ActivationError::WithPartialResult { result, source }) => {
log::error!("Error during activation: {source:?}");
log::debug!("Resulting file tree: {:?}", result);
let final_state = StateV1 {
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 = StateV1 {
file_tree: etc_tree,
services,
version: 1,
};
final_state.write_to_file(state_file).or_stash(&mut errs);

Ok(Result::<(), _>::from(errs)?)
}

pub fn prepopulate(store_path: &StorePath, ephemeral: bool) -> Result<()> {
Expand All @@ -213,38 +244,48 @@ pub fn prepopulate(store_path: &StorePath, ephemeral: bool) -> Result<()> {

let state_file = &get_state_file()?;
let old_state = StateV1::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
Copy link
Copy Markdown
Contributor

@picnoir picnoir Apr 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same. This will break upon reboot. Not sure what the right semantic would be.

We probably need to create an issue and seriously think about the overall activation success/failure semantics and how to mitigate a failure.

// 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) => StateV1 {
file_tree: etc_tree,
services,
version: 1,
},
Err(ActivationError::WithPartialResult { result, source }) => {
log::error!("Error during activation: {source:?}");
StateV1 {
file_tree: etc_tree,
services: result,
version: 1,
}
}
}
}
Err(ActivationError::WithPartialResult { result, source }) => {
log::error!("Error during activation: {source:?}");
StateV1 {
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 = StateV1 {
file_tree: etc_tree,
services,
version: 1,
};
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> {
Expand All @@ -267,3 +308,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}");
}
}
45 changes: 36 additions & 9 deletions crates/system-manager-engine/src/activate/etc_files.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,15 +124,34 @@ pub fn activate(
.collect();
entries.append(&mut non_static_entries);
// Create dirs and link/copy entries
new_state = create_etc_files(entries, new_state.clone(), &old_state, &etc_dir)?;
let mut errors = Vec::new();
new_state = create_etc_files(
entries,
new_state.clone(),
&old_state,
&etc_dir,
&mut errors,
);
// Delete unecessary files
let files_to_delete: HashSet<PathBuf> = old_state
.files
.difference(&new_state.files)
.map(|f| f.to_owned())
.collect();
new_state = delete_paths(&files_to_delete, new_state);
Ok(new_state)
if errors.is_empty() {
Ok(new_state)
} else {
let messages: Vec<String> = errors.iter().map(|e| format!("{e:#}")).collect();
Err(ActivationError::with_partial_result(
new_state,
anyhow::anyhow!(
"{} etc file error(s):\n{}",
messages.len(),
messages.join("\n")
),
))
}
}

pub fn deactivate(old_state: EtcFilesState) -> EtcActivationResult {
Expand Down Expand Up @@ -282,19 +301,24 @@ fn create_etc_files(
mut state: EtcFilesState,
old_state: &EtcFilesState,
etc_dir: &Path,
) -> EtcActivationResult {
errors: &mut Vec<anyhow::Error>,
) -> EtcFilesState {
files.sort_by(|a, b| a.target.cmp(&b.target));
for file in files {
let target = file.target.clone();
state = match create_etc_file(file, state, old_state, etc_dir) {
Ok(state) => state,
Err(ActivationError::WithPartialResult { result, source }) => {
log::warn!("Can't link/copy {} to : {}", target.display(), source);
log::error!(
"Error while creating file in {}: {source:?}",
target.display()
);
errors.push(source);
result
}
}
}
Ok(state)
state
}

/// Create a single etc file.
Expand Down Expand Up @@ -358,10 +382,13 @@ fn create_etc_file(
);
state = backup_and_link(&target, &file.source.store_path, state)?;
} else {
log::warn!(
"Error while creating file in /etc: Unmanaged path already exists in filesystem, please remove it and run system-manager again: {}\nSet replaceExisting if you're willing to override it.",
target.display()
);
return Err(ActivationError::with_partial_result(
state,
anyhow::anyhow!(
"Unmanaged path already exists in filesystem, please remove it or use replaceExisting and run system-manager again: {}.",
target.display()
),
));
}
} else {
// Target do not exist on the filesystem
Expand Down
Loading