Skip to content

Commit 5f099d0

Browse files
jfrochejfroche
committed
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.
1 parent 53b3bf3 commit 5f099d0

9 files changed

Lines changed: 222 additions & 128 deletions

File tree

‎Cargo.lock‎

Lines changed: 7 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎Cargo.toml‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ dbus = "0.9.7"
1818
env_logger = "0.11.0"
1919
im = { version = "15.1.0", features = ["serde"] }
2020
itertools = "0.14.0"
21+
lazy_errors = "0.10.1"
2122
log = "0.4.17"
2223
nix = { version = "0.31.0", features = ["hostname", "user"] }
2324
regex = "1.11.1"

‎crates/system-manager-engine/Cargo.toml‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ dbus.workspace = true
2121
env_logger.workspace = true
2222
im.workspace = true
2323
itertools.workspace = true
24+
lazy_errors.workspace = true
2425
log.workspace = true
2526
nix.workspace = true
2627
regex.workspace = true

‎crates/system-manager-engine/src/activate.rs‎

Lines changed: 137 additions & 75 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ mod tmp_files;
44
pub(crate) mod users;
55

66
use anyhow::Result;
7+
use lazy_errors::prelude::*;
78
use serde::{Deserialize, Serialize};
89
use std::fs::DirBuilder;
910
use std::path::{Path, PathBuf};
@@ -13,6 +14,14 @@ use thiserror::Error;
1314
use crate::activate::etc_files::FileTree;
1415
use crate::{StorePath, STATE_FILE_NAME, SYSTEM_MANAGER_STATE_DIR};
1516

17+
/// Separates the partial result from the error in an ActivationResult,
18+
pub(crate) fn split_activation_result<R>(result: ActivationResult<R>) -> (R, anyhow::Result<()>) {
19+
match result {
20+
Ok(r) => (r, Ok(())),
21+
Err(ActivationError::WithPartialResult { result, source }) => (result, Err(source)),
22+
}
23+
}
24+
1625
#[derive(Error, Debug)]
1726
pub enum ActivationError<R> {
1827
#[error("")]
@@ -77,60 +86,68 @@ pub fn activate(store_path: &StorePath, ephemeral: bool) -> Result<()> {
7786

7887
let state_file = &get_state_file()?;
7988
let old_state = State::from_file(state_file)?;
89+
let mut errs = ErrorStash::new(|| "Activation completed with errors");
8090

8191
log::info!("Activating etc files...");
8292

83-
match etc_files::activate(store_path, old_state.file_tree, ephemeral) {
84-
Ok(etc_tree) => {
85-
log::info!("Restarting sysinit-reactivation.target...");
86-
services::restart_sysinit_reactivation_target()?;
87-
88-
// Restart userborn before tmpfiles so users exist when tmpfiles runs
89-
if let Err(e) = services::restart_userborn_if_exists() {
90-
log::error!("Error restarting userborn.service: {e}");
91-
}
92-
93-
log::info!("Activating tmp files...");
94-
let tmp_result = tmp_files::activate(&etc_tree);
95-
if let Err(e) = &tmp_result {
96-
log::error!("Error during activation of tmp files");
97-
log::error!("{e}");
98-
} else {
99-
log::debug!("Successfully created tmp files");
100-
}
101-
102-
log::info!("Activating systemd services...");
103-
let final_state = match services::activate(store_path, old_state.services, ephemeral) {
104-
Ok(services) => State {
105-
file_tree: etc_tree,
106-
services,
107-
},
108-
Err(ActivationError::WithPartialResult { result, source }) => {
109-
log::error!("Error during activation: {source:?}");
110-
State {
111-
file_tree: etc_tree,
112-
services: result,
113-
}
114-
}
115-
};
116-
final_state.write_to_file(state_file)?;
117-
118-
if let Err(e) = tmp_result {
119-
return Err(e.into());
120-
}
121-
122-
Ok(())
93+
let (etc_tree, etc_result) = split_activation_result(etc_files::activate(
94+
store_path,
95+
old_state.file_tree,
96+
ephemeral,
97+
));
98+
let etc_ok = etc_result.is_ok();
99+
if let Err(ref e) = etc_result {
100+
log::error!("Error during activation: {e:?}");
101+
}
102+
etc_result.or_stash(&mut errs);
103+
104+
// Only run daemon reload, userborn, tmpfiles, and services when etc files
105+
// were fully applied. Partial etc results mean services may reference
106+
// missing config files.
107+
let services = if etc_ok {
108+
log::info!("Restarting sysinit-reactivation.target...");
109+
let sysinit_result = services::restart_sysinit_reactivation_target();
110+
if let Err(ref e) = sysinit_result {
111+
log::error!("Error restarting sysinit-reactivation.target: {e}");
123112
}
124-
Err(ActivationError::WithPartialResult { result, source }) => {
125-
log::error!("Error during activation: {source:?}");
126-
let final_state = State {
127-
file_tree: result,
128-
..old_state
129-
};
130-
final_state.write_to_file(state_file)?;
131-
Ok(())
113+
sysinit_result.or_stash(&mut errs);
114+
115+
// Restart userborn before tmpfiles so users exist when tmpfiles runs
116+
let userborn_result = services::restart_userborn_if_exists();
117+
if let Err(ref e) = userborn_result {
118+
log::error!("Error restarting userborn.service: {e}");
132119
}
133-
}
120+
userborn_result.or_stash(&mut errs);
121+
122+
log::info!("Activating tmp files...");
123+
let ((), tmp_result) = split_activation_result(tmp_files::activate(&etc_tree));
124+
if let Err(ref e) = tmp_result {
125+
log::error!("Error during activation of tmp files: {e}");
126+
}
127+
tmp_result.or_stash(&mut errs);
128+
129+
log::info!("Activating systemd services...");
130+
let (services, svc_result) = split_activation_result(services::activate(
131+
store_path,
132+
old_state.services,
133+
ephemeral,
134+
));
135+
if let Err(ref e) = svc_result {
136+
log::error!("Error during activation: {e:?}");
137+
}
138+
svc_result.or_stash(&mut errs);
139+
services
140+
} else {
141+
old_state.services
142+
};
143+
144+
let final_state = State {
145+
file_tree: etc_tree,
146+
services,
147+
};
148+
final_state.write_to_file(state_file).or_stash(&mut errs);
149+
150+
Ok(Result::<(), _>::from(errs)?)
134151
}
135152

136153
pub fn prepopulate(store_path: &StorePath, ephemeral: bool) -> Result<()> {
@@ -146,36 +163,46 @@ pub fn prepopulate(store_path: &StorePath, ephemeral: bool) -> Result<()> {
146163

147164
let state_file = &get_state_file()?;
148165
let old_state = State::from_file(state_file)?;
166+
let mut errs = ErrorStash::new(|| "Pre-population completed with errors");
149167

150168
log::info!("Activating etc files...");
151169

152-
match etc_files::activate(store_path, old_state.file_tree, ephemeral) {
153-
Ok(etc_tree) => {
154-
log::info!("Registering systemd services...");
155-
match services::get_active_services(store_path, old_state.services) {
156-
Ok(services) => State {
157-
file_tree: etc_tree,
158-
services,
159-
},
160-
Err(ActivationError::WithPartialResult { result, source }) => {
161-
log::error!("Error during activation: {source:?}");
162-
State {
163-
file_tree: etc_tree,
164-
services: result,
165-
}
166-
}
167-
}
168-
}
169-
Err(ActivationError::WithPartialResult { result, source }) => {
170-
log::error!("Error during activation: {source:?}");
171-
State {
172-
file_tree: result,
173-
..old_state
174-
}
175-
}
170+
let (etc_tree, etc_result) = split_activation_result(etc_files::activate(
171+
store_path,
172+
old_state.file_tree,
173+
ephemeral,
174+
));
175+
let etc_ok = etc_result.is_ok();
176+
if let Err(ref e) = etc_result {
177+
log::error!("Error during activation: {e:?}");
176178
}
177-
.write_to_file(state_file)?;
178-
Ok(())
179+
etc_result.or_stash(&mut errs);
180+
181+
// Only register services when etc files were fully applied, preserving
182+
// old service state on etc failure to avoid persisting state from a
183+
// partial run.
184+
let services = if etc_ok {
185+
log::info!("Registering systemd services...");
186+
let (services, svc_result) = split_activation_result(services::get_active_services(
187+
store_path,
188+
old_state.services,
189+
));
190+
if let Err(ref e) = svc_result {
191+
log::error!("Error during activation: {e:?}");
192+
}
193+
svc_result.or_stash(&mut errs);
194+
services
195+
} else {
196+
old_state.services
197+
};
198+
199+
let final_state = State {
200+
file_tree: etc_tree,
201+
services,
202+
};
203+
final_state.write_to_file(state_file).or_stash(&mut errs);
204+
205+
Ok(Result::<(), _>::from(errs)?)
179206
}
180207

181208
fn run_preactivation_assertions(store_path: &StorePath) -> Result<process::ExitStatus> {
@@ -198,3 +225,38 @@ pub(crate) fn get_state_file() -> Result<PathBuf> {
198225
.create(SYSTEM_MANAGER_STATE_DIR)?;
199226
Ok(state_file)
200227
}
228+
229+
#[cfg(test)]
230+
mod tests {
231+
use super::*;
232+
233+
#[test]
234+
fn empty_stash_returns_ok() {
235+
let errs = ErrorStash::new(|| "Activation completed with errors");
236+
let result: std::result::Result<(), lazy_errors::prelude::Error> = errs.into();
237+
assert!(result.is_ok());
238+
}
239+
240+
#[test]
241+
fn single_stashed_error_returns_err() {
242+
let mut errs = ErrorStash::new(|| "Activation completed with errors");
243+
Err::<(), _>(anyhow::anyhow!("userborn failed")).or_stash(&mut errs);
244+
let result: std::result::Result<(), lazy_errors::prelude::Error> = errs.into();
245+
let err = result.unwrap_err();
246+
let msg = format!("{err:#}");
247+
assert!(msg.contains("userborn failed"), "message was: {msg}");
248+
}
249+
250+
#[test]
251+
fn multiple_stashed_errors_returns_combined_err() {
252+
let mut errs = ErrorStash::new(|| "Deactivation completed with errors");
253+
Err::<(), _>(anyhow::anyhow!("userborn failed")).or_stash(&mut errs);
254+
Err::<(), _>(anyhow::anyhow!("tmpfiles failed")).or_stash(&mut errs);
255+
let result: std::result::Result<(), lazy_errors::prelude::Error> = errs.into();
256+
let err = result.unwrap_err();
257+
let msg = format!("{err:#}");
258+
assert!(msg.contains("Deactivation"), "message was: {msg}");
259+
assert!(msg.contains("userborn failed"), "message was: {msg}");
260+
assert!(msg.contains("tmpfiles failed"), "message was: {msg}");
261+
}
262+
}

0 commit comments

Comments
 (0)