From 6e4431b11134037dba90e84bf1ac3a7bd3d8a65b Mon Sep 17 00:00:00 2001 From: Jean-Baptiste Giraudeau Date: Fri, 4 Sep 2026 16:23:19 +0200 Subject: [PATCH 1/9] feat: expose --timeout option on cli when applicable. --- crates/system-manager-engine/src/activate.rs | 28 +++++---- .../src/activate/services.rs | 11 ++-- .../system-manager-engine/src/deactivate.rs | 5 +- crates/system-manager-engine/src/main.rs | 17 ++++- crates/system-manager/src/main.rs | 63 +++++++++++++++++-- testFlake/vm-tests/target-host.nix | 5 +- 6 files changed, 100 insertions(+), 29 deletions(-) diff --git a/crates/system-manager-engine/src/activate.rs b/crates/system-manager-engine/src/activate.rs index fce3d8da..d0c19257 100644 --- a/crates/system-manager-engine/src/activate.rs +++ b/crates/system-manager-engine/src/activate.rs @@ -10,6 +10,7 @@ use std::collections::HashSet; use std::fs::DirBuilder; use std::io::Seek; use std::path::{Path, PathBuf}; +use std::time::Duration; use std::{fs, io, process}; use thiserror::Error; @@ -128,7 +129,7 @@ impl StateV1 { } } -pub fn activate(store_path: &StorePath, ephemeral: bool) -> Result<()> { +pub fn activate(store_path: &StorePath, ephemeral: bool, timeout: &Option) -> Result<()> { log::info!("Activating system-manager profile: {store_path}"); if ephemeral { log::info!("Running in ephemeral mode"); @@ -164,21 +165,22 @@ pub fn activate(store_path: &StorePath, ephemeral: bool) -> Result<()> { } 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 { + let final_state = + match services::activate(store_path, old_state.services, ephemeral, timeout) { + Ok(services) => StateV1 { file_tree: etc_tree, - services: result, + 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 { diff --git a/crates/system-manager-engine/src/activate/services.rs b/crates/system-manager-engine/src/activate/services.rs index 3b570329..0810dd92 100644 --- a/crates/system-manager-engine/src/activate/services.rs +++ b/crates/system-manager-engine/src/activate/services.rs @@ -62,6 +62,7 @@ pub fn activate( store_path: &StorePath, old_services: Services, ephemeral: bool, + timeout: &Option, ) -> ServiceActivationResult { verify_systemd_dir(ephemeral) .map_err(|e| ActivationError::with_partial_result(old_services.clone(), e))?; @@ -82,7 +83,6 @@ pub fn activate( let job_monitor = service_manager .monitor_jobs_init() .map_err(|e| ActivationError::with_partial_result(old_services.clone(), e))?; - let timeout = Some(Duration::from_secs(30)); // Stop removed services and any masked services that might still be running // (e.g. distro-provided units). Must happen before daemon-reload so systemd @@ -93,7 +93,7 @@ pub fn activate( &service_manager, &job_monitor, stop_services(&service_manager, units_to_stop), - &timeout, + timeout, ) .map_err(|e| ActivationError::with_partial_result(services.clone(), e))?; @@ -102,7 +102,7 @@ pub fn activate( &job_monitor, reload_or_restart_units(&service_manager, convert_services(&services_to_reload)) + start_units(&service_manager, ["system-manager.target"]), - &timeout, + timeout, ) .map_err(|e| ActivationError::with_partial_result(services.clone(), e))?; @@ -228,7 +228,7 @@ fn verify_systemd_dir(ephemeral: bool) -> anyhow::Result<()> { Ok(()) } -pub fn deactivate(old_services: Services) -> ServiceActivationResult { +pub fn deactivate(old_services: Services, timeout: &Option) -> ServiceActivationResult { log::debug!("{:?}", old_services); restore_ephemeral_system_dir() @@ -247,7 +247,6 @@ pub fn deactivate(old_services: Services) -> ServiceActivationResult { let job_monitor = service_manager .monitor_jobs_init() .map_err(|e| ActivationError::with_partial_result(old_services.clone(), e))?; - let timeout = Some(Duration::from_secs(30)); let mut units_to_stop = convert_services(&stoppable); units_to_stop.push("system-manager.target"); @@ -257,7 +256,7 @@ pub fn deactivate(old_services: Services) -> ServiceActivationResult { &service_manager, &job_monitor, stop_services(&service_manager, units_to_stop), - &timeout, + timeout, ) // We consider all jobs stopped now.. .map_err(|e| ActivationError::with_partial_result(im::HashMap::new(), e))?; diff --git a/crates/system-manager-engine/src/deactivate.rs b/crates/system-manager-engine/src/deactivate.rs index 3b80f2bc..2633fb24 100644 --- a/crates/system-manager-engine/src/deactivate.rs +++ b/crates/system-manager-engine/src/deactivate.rs @@ -1,4 +1,5 @@ use anyhow::Result; +use std::time::Duration; use crate::activate::etc_files; use crate::activate::services; @@ -7,7 +8,7 @@ use crate::activate::{get_state_file, ActivationError, StateV1}; /// Deactivates system-manager by locking managed users, removing etc files, /// and stopping systemd services. -pub fn deactivate() -> Result<()> { +pub fn deactivate(timeout: &Option) -> Result<()> { log::info!("Deactivating system-manager"); let state_file = &get_state_file()?; let old_state = StateV1::from_file(state_file)?; @@ -24,7 +25,7 @@ pub fn deactivate() -> Result<()> { match etc_files::deactivate(old_state.file_tree) { Ok(etc_tree) => { log::info!("Deactivating systemd services..."); - match services::deactivate(old_state.services) { + match services::deactivate(old_state.services, timeout) { Ok(services) => StateV1 { file_tree: etc_tree, services, diff --git a/crates/system-manager-engine/src/main.rs b/crates/system-manager-engine/src/main.rs index 879a7078..b8a9abb3 100644 --- a/crates/system-manager-engine/src/main.rs +++ b/crates/system-manager-engine/src/main.rs @@ -13,6 +13,7 @@ use anyhow::Result; use clap::Parser; use std::process::ExitCode; +use std::time::Duration; use system_manager_engine::{NixOptions, StorePath, PROFILE_DIR}; @@ -64,11 +65,17 @@ enum Action { store_path_arg: StorePathArg, #[command(flatten)] activation_args: ActivationArgs, + /// Set timeout for action, in seconds + #[arg(short, long)] + timeout: Option, }, /// Deactivate the system-manager profile (remove managed configuration) Deactivate { #[command(flatten)] store_path_arg: OptionalStorePathArg, + /// Set timeout for action, in seconds + #[arg(short, long)] + timeout: Option, }, /// Pre-populate files without starting services Prepopulate { @@ -104,17 +111,23 @@ fn go(args: Args) -> Result<()> { Action::Activate { store_path_arg: StorePathArg { store_path }, activation_args: ActivationArgs { ephemeral }, - } => system_manager_engine::activate::activate(&store_path, ephemeral), + timeout, + } => system_manager_engine::activate::activate( + &store_path, + ephemeral, + &timeout.map(Duration::from_secs), + ), Action::Deactivate { store_path_arg: OptionalStorePathArg { store_path }, + timeout, } => { // Log which store path we're using if it was auto-detected if store_path.is_none() { let path = std::path::Path::new(PROFILE_DIR).join("system-manager"); log::info!("No store path provided, using {}", path.display()); } - system_manager_engine::deactivate::deactivate() + system_manager_engine::deactivate::deactivate(&timeout.map(Duration::from_secs)) } Action::Prepopulate { diff --git a/crates/system-manager/src/main.rs b/crates/system-manager/src/main.rs index 910ab327..67a3bd63 100644 --- a/crates/system-manager/src/main.rs +++ b/crates/system-manager/src/main.rs @@ -174,11 +174,34 @@ impl From<&BuildArgs> for NixBuildOptions { } } +#[derive(clap::Args, Debug)] +#[group(multiple = false)] +struct TimeoutArgs { + #[arg(long, action)] + /// If set disable timeout; wait indefinitely until for action to finish + no_timeout: bool, + /// Set timeout for action, in seconds + #[arg(short, long, default_value_t = 30u64)] + timeout: u64, +} + +impl From<&TimeoutArgs> for Option { + fn from(timeout_args: &TimeoutArgs) -> Self { + if timeout_args.no_timeout { + None + } else { + Some(timeout_args.timeout) + } + } +} + #[derive(clap::Args, Debug)] struct ActivationArgs { #[arg(long, action)] /// If true, only write under /run, otherwise write under /etc ephemeral: bool, + #[command(flatten)] + timeout_args: TimeoutArgs, } #[derive(clap::Args, Debug)] @@ -242,6 +265,8 @@ enum Action { optional_store_path_args: OptionalStorePathArg, #[command(flatten)] sudo_args: SudoArgs, + #[command(flatten)] + timeout_args: TimeoutArgs, }, /// Put all files defined by the given generation in place, but do not start services PrePopulate { @@ -304,7 +329,7 @@ fn go(args: Args) -> Result<()> { match action { Action::PrePopulate { store_or_flake_args, - activation_args: ActivationArgs { ephemeral }, + activation_args: ActivationArgs { ephemeral, .. }, sudo_args, } => { let sudo_options = sudo_args.to_sudo_options(legacy_use_remote_sudo)?; @@ -327,6 +352,7 @@ fn go(args: Args) -> Result<()> { Action::Deactivate { optional_store_path_args: OptionalStorePathArg { maybe_store_path }, sudo_args, + timeout_args, } => { let sudo_options = sudo_args.to_sudo_options(legacy_use_remote_sudo)?; deactivate( @@ -335,6 +361,7 @@ fn go(args: Args) -> Result<()> { &sudo_options, &ssh_options, verbose, + &Option::::from(&timeout_args), ) } @@ -403,7 +430,11 @@ fn go(args: Args) -> Result<()> { Action::Switch { build_args, - activation_args: ActivationArgs { ephemeral }, + activation_args: + ActivationArgs { + ephemeral, + timeout_args, + }, sudo_args, } => { let mut nix_build_options = NixBuildOptions::from(&build_args); @@ -424,12 +455,17 @@ fn go(args: Args) -> Result<()> { &sudo_options, &ssh_options, verbose, + &Option::::from(&timeout_args), ) } Action::Activate { store_path, - activation_args: ActivationArgs { ephemeral }, + activation_args: + ActivationArgs { + ephemeral, + timeout_args, + }, sudo_args, } => { let sudo_options = sudo_args.to_sudo_options(legacy_use_remote_sudo)?; @@ -441,6 +477,7 @@ fn go(args: Args) -> Result<()> { &sudo_options, &ssh_options, verbose, + &Option::::from(&timeout_args), ) } } @@ -618,9 +655,17 @@ fn deactivate( sudo_options: &SudoOptions, ssh_options: &[String], verbose: bool, + timeout: &Option, ) -> Result<()> { let store_path = store_path_or_active_profile(maybe_store_path); - invoke_engine_deactivate(&store_path, target_host, sudo_options, ssh_options, verbose) + invoke_engine_deactivate( + &store_path, + target_host, + sudo_options, + ssh_options, + verbose, + timeout, + ) } // --- Engine invocation functions --- @@ -653,6 +698,7 @@ fn invoke_engine_activate( sudo_options: &SudoOptions, ssh_options: &[String], verbose: bool, + timeout: &Option, ) -> Result<()> { let engine_path = store_path.store_path.join("bin").join(ENGINE_BIN); let mut args = vec![ @@ -666,6 +712,10 @@ fn invoke_engine_activate( if verbose { args.push("--verbose".to_string()); } + if timeout.is_some() { + args.push("--timeout".to_string()); + args.push(timeout.unwrap().to_string()); + } invoke_engine(&engine_path, &args, target_host, sudo_options, ssh_options) } @@ -700,6 +750,7 @@ fn invoke_engine_deactivate( sudo_options: &SudoOptions, ssh_options: &[String], verbose: bool, + timeout: &Option, ) -> Result<()> { // For deactivate, we need to find the engine in the profile // If we have a specific store path, use it; otherwise use the active profile @@ -717,6 +768,10 @@ fn invoke_engine_deactivate( if verbose { args.push("--verbose".to_string()); } + if timeout.is_some() { + args.push("--timeout".to_string()); + args.push(timeout.unwrap().to_string()); + } invoke_engine(&engine_path, &args, target_host, sudo_options, ssh_options) } diff --git a/testFlake/vm-tests/target-host.nix b/testFlake/vm-tests/target-host.nix index 3d678887..06172bba 100644 --- a/testFlake/vm-tests/target-host.nix +++ b/testFlake/vm-tests/target-host.nix @@ -95,7 +95,8 @@ forEachImage "target-host" { ["${hostPkgs.openssh}/bin/ssh"] + ssh_opts + ["root@127.0.0.1", "--", "${toplevel}/bin/system-manager-engine", "activate", - "--store-path", "${toplevel}"], + "--store-path", "${toplevel}", + "--timeout", "100"], capture_output=True, text=True, timeout=120 ) print(f"Remote activate stdout: {result.stdout}") @@ -136,7 +137,7 @@ forEachImage "target-host" { result = subprocess.run( ["${hostPkgs.openssh}/bin/ssh"] + ssh_opts + ["root@127.0.0.1", "--", - "${toplevel}/bin/system-manager-engine", "deactivate"], + "${toplevel}/bin/system-manager-engine", "deactivate", "--timeout", "30"], capture_output=True, text=True, timeout=120 ) print(f"Remote deactivate stdout: {result.stdout}") From f9af8fa95a327423d4cf932284b6a5a3fa78a07f Mon Sep 17 00:00:00 2001 From: Jean-Baptiste Giraudeau Date: Mon, 7 Sep 2026 10:38:18 +0200 Subject: [PATCH 2/9] fix: remove useless borrow in formating to make clippy happy. --- crates/system-manager-engine/src/activate.rs | 2 +- crates/system-manager-engine/src/activate/etc_files.rs | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/crates/system-manager-engine/src/activate.rs b/crates/system-manager-engine/src/activate.rs index d0c19257..ab9ecb8d 100644 --- a/crates/system-manager-engine/src/activate.rs +++ b/crates/system-manager-engine/src/activate.rs @@ -103,7 +103,7 @@ impl StateV1 { backup_path.add_extension("v0back"); log::info!( "Create a backup of the v0 state at {}.", - &backup_path.display() + backup_path.display() ); fs::copy(state_file, backup_path)?; Ok(filetree.into()) diff --git a/crates/system-manager-engine/src/activate/etc_files.rs b/crates/system-manager-engine/src/activate/etc_files.rs index 3904d87b..43e77ec8 100644 --- a/crates/system-manager-engine/src/activate/etc_files.rs +++ b/crates/system-manager-engine/src/activate/etc_files.rs @@ -332,13 +332,13 @@ fn create_etc_file( if old_state.contains(&target) { log::debug!( "{} is managed by system-manager. Deleting.", - &target.display() + target.display() ); fs::remove_file(&target).map_err(|e| ActivationError::WithPartialResult { result: state.clone(), source: e.into(), })?; - log::debug!("{} is managed by system-manager.", &target.display()); + log::debug!("{} is managed by system-manager.", target.display()); unix::fs::symlink(file.source.store_path, &target).map_err(|e| { ActivationError::WithPartialResult { result: state.clone(), @@ -422,7 +422,7 @@ fn find_uid(entry: &EtcFile) -> anyhow::Result { || { log::warn!( "Specified user {} not found, defaulting to root", - &entry.user + entry.user ); 0 }, @@ -443,7 +443,7 @@ fn find_gid(entry: &EtcFile) -> anyhow::Result { || { log::warn!( "Specified group {} not found, defaulting to root", - &entry.group + entry.group ); 0 }, From e6348f2042c22ebad7987c0496a78311039a78f1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jean-Fran=C3=A7ois=20Roche?= Date: Sun, 20 Sep 2026 19:21:42 +0200 Subject: [PATCH 3/9] fix(engine): keep the 30s default timeout, 0 means no timeout Before this change the engine's --timeout was an Option with no default, so invoking system-manager-engine directly silently changed behaviour from the previously hardcoded 30s to waiting indefinitely on a hung systemd job. Default to 30s, and read 0 as 'wait indefinitely'. The wrapper now always passes --timeout explicitly (0 for --no-timeout), otherwise omitting the flag would silently fall back to the engine's default instead of disabling the timeout. The target-host VM test no longer needs to pass --timeout explicitly, so it exercises the default path again. --- crates/system-manager-engine/src/main.rs | 35 +++++++++++++++++------- crates/system-manager/src/main.rs | 18 ++++++------ testFlake/vm-tests/target-host.nix | 5 ++-- 3 files changed, 37 insertions(+), 21 deletions(-) diff --git a/crates/system-manager-engine/src/main.rs b/crates/system-manager-engine/src/main.rs index b8a9abb3..0a81c35e 100644 --- a/crates/system-manager-engine/src/main.rs +++ b/crates/system-manager-engine/src/main.rs @@ -42,6 +42,23 @@ struct ActivationArgs { ephemeral: bool, } +#[derive(clap::Args, Debug)] +struct TimeoutArgs { + /// Set the timeout for the action, in seconds. 0 disables the timeout and + /// waits indefinitely for the action to finish + #[arg(short, long, default_value_t = 30u64)] + timeout: u64, +} + +impl From<&TimeoutArgs> for Option { + fn from(timeout_args: &TimeoutArgs) -> Self { + match timeout_args.timeout { + 0 => None, + secs => Some(Duration::from_secs(secs)), + } + } +} + #[derive(clap::Args, Debug)] struct StorePathArg { /// The store path containing the system-manager profile @@ -65,17 +82,15 @@ enum Action { store_path_arg: StorePathArg, #[command(flatten)] activation_args: ActivationArgs, - /// Set timeout for action, in seconds - #[arg(short, long)] - timeout: Option, + #[command(flatten)] + timeout_args: TimeoutArgs, }, /// Deactivate the system-manager profile (remove managed configuration) Deactivate { #[command(flatten)] store_path_arg: OptionalStorePathArg, - /// Set timeout for action, in seconds - #[arg(short, long)] - timeout: Option, + #[command(flatten)] + timeout_args: TimeoutArgs, }, /// Pre-populate files without starting services Prepopulate { @@ -111,23 +126,23 @@ fn go(args: Args) -> Result<()> { Action::Activate { store_path_arg: StorePathArg { store_path }, activation_args: ActivationArgs { ephemeral }, - timeout, + timeout_args, } => system_manager_engine::activate::activate( &store_path, ephemeral, - &timeout.map(Duration::from_secs), + &Option::::from(&timeout_args), ), Action::Deactivate { store_path_arg: OptionalStorePathArg { store_path }, - timeout, + timeout_args, } => { // Log which store path we're using if it was auto-detected if store_path.is_none() { let path = std::path::Path::new(PROFILE_DIR).join("system-manager"); log::info!("No store path provided, using {}", path.display()); } - system_manager_engine::deactivate::deactivate(&timeout.map(Duration::from_secs)) + system_manager_engine::deactivate::deactivate(&Option::::from(&timeout_args)) } Action::Prepopulate { diff --git a/crates/system-manager/src/main.rs b/crates/system-manager/src/main.rs index 67a3bd63..e21625b8 100644 --- a/crates/system-manager/src/main.rs +++ b/crates/system-manager/src/main.rs @@ -670,6 +670,14 @@ fn deactivate( // --- Engine invocation functions --- +/// Always pass the timeout explicitly, so that `--no-timeout` (`None`) is not +/// silently turned back into the engine's own default. The engine reads 0 as +/// "wait indefinitely". +fn push_timeout_arg(args: &mut Vec, timeout: &Option) { + args.push("--timeout".to_string()); + args.push(timeout.unwrap_or(0).to_string()); +} + /// Invoke the engine's register subcommand fn invoke_engine_register( store_path: &StorePath, @@ -712,10 +720,7 @@ fn invoke_engine_activate( if verbose { args.push("--verbose".to_string()); } - if timeout.is_some() { - args.push("--timeout".to_string()); - args.push(timeout.unwrap().to_string()); - } + push_timeout_arg(&mut args, timeout); invoke_engine(&engine_path, &args, target_host, sudo_options, ssh_options) } @@ -768,10 +773,7 @@ fn invoke_engine_deactivate( if verbose { args.push("--verbose".to_string()); } - if timeout.is_some() { - args.push("--timeout".to_string()); - args.push(timeout.unwrap().to_string()); - } + push_timeout_arg(&mut args, timeout); invoke_engine(&engine_path, &args, target_host, sudo_options, ssh_options) } diff --git a/testFlake/vm-tests/target-host.nix b/testFlake/vm-tests/target-host.nix index 06172bba..3d678887 100644 --- a/testFlake/vm-tests/target-host.nix +++ b/testFlake/vm-tests/target-host.nix @@ -95,8 +95,7 @@ forEachImage "target-host" { ["${hostPkgs.openssh}/bin/ssh"] + ssh_opts + ["root@127.0.0.1", "--", "${toplevel}/bin/system-manager-engine", "activate", - "--store-path", "${toplevel}", - "--timeout", "100"], + "--store-path", "${toplevel}"], capture_output=True, text=True, timeout=120 ) print(f"Remote activate stdout: {result.stdout}") @@ -137,7 +136,7 @@ forEachImage "target-host" { result = subprocess.run( ["${hostPkgs.openssh}/bin/ssh"] + ssh_opts + ["root@127.0.0.1", "--", - "${toplevel}/bin/system-manager-engine", "deactivate", "--timeout", "30"], + "${toplevel}/bin/system-manager-engine", "deactivate"], capture_output=True, text=True, timeout=120 ) print(f"Remote deactivate stdout: {result.stdout}") From 1a06c054d1527085486c6041407e7dfb376b0283 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jean-Fran=C3=A7ois=20Roche?= Date: Sun, 20 Sep 2026 19:22:18 +0200 Subject: [PATCH 4/9] fix(cli): stop exposing --timeout on pre-populate TimeoutArgs was flattened into ActivationArgs, which pre-populate also flattens, so 'system-manager pre-populate --timeout 60' was accepted and advertised in --help but then discarded: prepopulate() starts no service and takes no timeout. Attach TimeoutArgs to the switch and activate subcommands directly instead, mirroring the engine where ActivationArgs deliberately carries no timeout. --- crates/system-manager/src/main.rs | 22 +++++++++------------- 1 file changed, 9 insertions(+), 13 deletions(-) diff --git a/crates/system-manager/src/main.rs b/crates/system-manager/src/main.rs index e21625b8..65626c75 100644 --- a/crates/system-manager/src/main.rs +++ b/crates/system-manager/src/main.rs @@ -200,8 +200,6 @@ struct ActivationArgs { #[arg(long, action)] /// If true, only write under /run, otherwise write under /etc ephemeral: bool, - #[command(flatten)] - timeout_args: TimeoutArgs, } #[derive(clap::Args, Debug)] @@ -246,6 +244,8 @@ enum Action { activation_args: ActivationArgs, #[command(flatten)] sudo_args: SudoArgs, + #[command(flatten)] + timeout_args: TimeoutArgs, }, /// Build a new system-manager generation and register it as the active system-manager profile Register { @@ -286,6 +286,8 @@ enum Action { activation_args: ActivationArgs, #[command(flatten)] sudo_args: SudoArgs, + #[command(flatten)] + timeout_args: TimeoutArgs, }, } @@ -329,7 +331,7 @@ fn go(args: Args) -> Result<()> { match action { Action::PrePopulate { store_or_flake_args, - activation_args: ActivationArgs { ephemeral, .. }, + activation_args: ActivationArgs { ephemeral }, sudo_args, } => { let sudo_options = sudo_args.to_sudo_options(legacy_use_remote_sudo)?; @@ -430,12 +432,9 @@ fn go(args: Args) -> Result<()> { Action::Switch { build_args, - activation_args: - ActivationArgs { - ephemeral, - timeout_args, - }, + activation_args: ActivationArgs { ephemeral }, sudo_args, + timeout_args, } => { let mut nix_build_options = NixBuildOptions::from(&build_args); let sudo_options = sudo_args.to_sudo_options(legacy_use_remote_sudo)?; @@ -461,12 +460,9 @@ fn go(args: Args) -> Result<()> { Action::Activate { store_path, - activation_args: - ActivationArgs { - ephemeral, - timeout_args, - }, + activation_args: ActivationArgs { ephemeral }, sudo_args, + timeout_args, } => { let sudo_options = sudo_args.to_sudo_options(legacy_use_remote_sudo)?; copy_closure(&store_path, &target_host, &ssh_options)?; From a4bf73e279490f3b2f9bdd1a3e2fd466c4ea602a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jean-Fran=C3=A7ois=20Roche?= Date: Sun, 20 Sep 2026 19:51:39 +0200 Subject: [PATCH 5/9] test(cli): cover the timeout contract Covers the default (30s), the --no-timeout/--timeout conflict, that pre-populate rejects --timeout, and that --no-timeout reaches the engine as an explicit 0 rather than an omitted flag. --- crates/system-manager/src/main.rs | 67 +++++++++++++++++++++++++++++++ 1 file changed, 67 insertions(+) diff --git a/crates/system-manager/src/main.rs b/crates/system-manager/src/main.rs index 65626c75..4d5d6a8d 100644 --- a/crates/system-manager/src/main.rs +++ b/crates/system-manager/src/main.rs @@ -1084,4 +1084,71 @@ mod tests { assert!(args.ssh_options.is_empty()); } + + fn switch_timeout_args(extra: &[&str]) -> Option { + let argv = ["system-manager", "switch", "--flake", ".#test"] + .into_iter() + .chain(extra.iter().copied()); + let args = Args::try_parse_from(argv).expect("failed to parse args"); + + match args.action { + Action::Switch { timeout_args, .. } => Option::::from(&timeout_args), + other => panic!("expected switch, got {other:?}"), + } + } + + #[test] + fn timeout_defaults_to_30_seconds() { + assert_eq!(switch_timeout_args(&[]), Some(30)); + } + + #[test] + fn no_timeout_conflicts_with_timeout() { + Args::try_parse_from([ + "system-manager", + "switch", + "--flake", + ".#test", + "--no-timeout", + "--timeout", + "5", + ]) + .expect_err("--no-timeout and --timeout should be mutually exclusive"); + } + + #[test] + fn pre_populate_rejects_timeout() { + // pre-populate starts no service, so it must not advertise a timeout + Args::try_parse_from([ + "system-manager", + "pre-populate", + "--flake", + ".#test", + "--timeout", + "5", + ]) + .expect_err("pre-populate should not accept --timeout"); + } + + #[test] + fn no_timeout_is_passed_to_the_engine_as_zero() { + // The engine defaults to 30s, so omitting the flag would silently + // re-enable the timeout instead of disabling it. + let timeout = switch_timeout_args(&["--no-timeout"]); + assert_eq!(timeout, None); + + let mut args = vec!["activate".to_string()]; + push_timeout_arg(&mut args, &timeout); + assert_eq!(args, vec!["activate", "--timeout", "0"]); + } + + #[test] + fn explicit_timeout_is_passed_to_the_engine() { + let timeout = switch_timeout_args(&["--timeout", "90"]); + assert_eq!(timeout, Some(90)); + + let mut args = vec!["activate".to_string()]; + push_timeout_arg(&mut args, &timeout); + assert_eq!(args, vec!["activate", "--timeout", "90"]); + } } From bb8d6b31b40209072dfa310ca457cddb0f619e86 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jean-Fran=C3=A7ois=20Roche?= Date: Sun, 20 Sep 2026 22:42:20 +0200 Subject: [PATCH 6/9] fix(cli): preserve compatibility with older engines --- crates/system-manager/src/main.rs | 79 +++++++++++++++++++------------ 1 file changed, 49 insertions(+), 30 deletions(-) diff --git a/crates/system-manager/src/main.rs b/crates/system-manager/src/main.rs index 4d5d6a8d..c9690889 100644 --- a/crates/system-manager/src/main.rs +++ b/crates/system-manager/src/main.rs @@ -180,17 +180,39 @@ struct TimeoutArgs { #[arg(long, action)] /// If set disable timeout; wait indefinitely until for action to finish no_timeout: bool, - /// Set timeout for action, in seconds - #[arg(short, long, default_value_t = 30u64)] - timeout: u64, + /// Set timeout for action, in seconds. Defaults to the engine's timeout. + #[arg(short, long)] + timeout: Option, } -impl From<&TimeoutArgs> for Option { +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +enum EngineTimeoutArg { + Default, + Disabled, + Seconds(u64), +} + +impl EngineTimeoutArg { + fn push_to(self, args: &mut Vec) { + let value = match self { + Self::Default => return, + Self::Disabled => 0, + Self::Seconds(seconds) => seconds, + }; + + args.push("--timeout".to_string()); + args.push(value.to_string()); + } +} + +impl From<&TimeoutArgs> for EngineTimeoutArg { fn from(timeout_args: &TimeoutArgs) -> Self { if timeout_args.no_timeout { - None + Self::Disabled + } else if let Some(seconds) = timeout_args.timeout { + Self::Seconds(seconds) } else { - Some(timeout_args.timeout) + Self::Default } } } @@ -363,7 +385,7 @@ fn go(args: Args) -> Result<()> { &sudo_options, &ssh_options, verbose, - &Option::::from(&timeout_args), + &EngineTimeoutArg::from(&timeout_args), ) } @@ -454,7 +476,7 @@ fn go(args: Args) -> Result<()> { &sudo_options, &ssh_options, verbose, - &Option::::from(&timeout_args), + &EngineTimeoutArg::from(&timeout_args), ) } @@ -473,7 +495,7 @@ fn go(args: Args) -> Result<()> { &sudo_options, &ssh_options, verbose, - &Option::::from(&timeout_args), + &EngineTimeoutArg::from(&timeout_args), ) } } @@ -651,7 +673,7 @@ fn deactivate( sudo_options: &SudoOptions, ssh_options: &[String], verbose: bool, - timeout: &Option, + timeout: &EngineTimeoutArg, ) -> Result<()> { let store_path = store_path_or_active_profile(maybe_store_path); invoke_engine_deactivate( @@ -666,14 +688,6 @@ fn deactivate( // --- Engine invocation functions --- -/// Always pass the timeout explicitly, so that `--no-timeout` (`None`) is not -/// silently turned back into the engine's own default. The engine reads 0 as -/// "wait indefinitely". -fn push_timeout_arg(args: &mut Vec, timeout: &Option) { - args.push("--timeout".to_string()); - args.push(timeout.unwrap_or(0).to_string()); -} - /// Invoke the engine's register subcommand fn invoke_engine_register( store_path: &StorePath, @@ -702,7 +716,7 @@ fn invoke_engine_activate( sudo_options: &SudoOptions, ssh_options: &[String], verbose: bool, - timeout: &Option, + timeout: &EngineTimeoutArg, ) -> Result<()> { let engine_path = store_path.store_path.join("bin").join(ENGINE_BIN); let mut args = vec![ @@ -716,7 +730,7 @@ fn invoke_engine_activate( if verbose { args.push("--verbose".to_string()); } - push_timeout_arg(&mut args, timeout); + timeout.push_to(&mut args); invoke_engine(&engine_path, &args, target_host, sudo_options, ssh_options) } @@ -751,7 +765,7 @@ fn invoke_engine_deactivate( sudo_options: &SudoOptions, ssh_options: &[String], verbose: bool, - timeout: &Option, + timeout: &EngineTimeoutArg, ) -> Result<()> { // For deactivate, we need to find the engine in the profile // If we have a specific store path, use it; otherwise use the active profile @@ -769,7 +783,7 @@ fn invoke_engine_deactivate( if verbose { args.push("--verbose".to_string()); } - push_timeout_arg(&mut args, timeout); + timeout.push_to(&mut args); invoke_engine(&engine_path, &args, target_host, sudo_options, ssh_options) } @@ -1085,21 +1099,26 @@ mod tests { assert!(args.ssh_options.is_empty()); } - fn switch_timeout_args(extra: &[&str]) -> Option { + fn switch_timeout_args(extra: &[&str]) -> EngineTimeoutArg { let argv = ["system-manager", "switch", "--flake", ".#test"] .into_iter() .chain(extra.iter().copied()); let args = Args::try_parse_from(argv).expect("failed to parse args"); match args.action { - Action::Switch { timeout_args, .. } => Option::::from(&timeout_args), + Action::Switch { timeout_args, .. } => EngineTimeoutArg::from(&timeout_args), other => panic!("expected switch, got {other:?}"), } } #[test] - fn timeout_defaults_to_30_seconds() { - assert_eq!(switch_timeout_args(&[]), Some(30)); + fn omitted_timeout_uses_the_engine_default() { + let timeout = switch_timeout_args(&[]); + assert_eq!(timeout, EngineTimeoutArg::Default); + + let mut args = vec!["activate".to_string()]; + timeout.push_to(&mut args); + assert_eq!(args, vec!["activate"]); } #[test] @@ -1135,20 +1154,20 @@ mod tests { // The engine defaults to 30s, so omitting the flag would silently // re-enable the timeout instead of disabling it. let timeout = switch_timeout_args(&["--no-timeout"]); - assert_eq!(timeout, None); + assert_eq!(timeout, EngineTimeoutArg::Disabled); let mut args = vec!["activate".to_string()]; - push_timeout_arg(&mut args, &timeout); + timeout.push_to(&mut args); assert_eq!(args, vec!["activate", "--timeout", "0"]); } #[test] fn explicit_timeout_is_passed_to_the_engine() { let timeout = switch_timeout_args(&["--timeout", "90"]); - assert_eq!(timeout, Some(90)); + assert_eq!(timeout, EngineTimeoutArg::Seconds(90)); let mut args = vec!["activate".to_string()]; - push_timeout_arg(&mut args, &timeout); + timeout.push_to(&mut args); assert_eq!(args, vec!["activate", "--timeout", "90"]); } } From 143f6eb2e83630a65af177b47645d126bf44888b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jean-Fran=C3=A7ois=20Roche?= Date: Sun, 20 Sep 2026 22:43:17 +0200 Subject: [PATCH 7/9] fix(engine): apply timeout to activation prerequisites --- crates/system-manager-engine/src/activate.rs | 4 ++-- crates/system-manager-engine/src/activate/services.rs | 10 ++++------ 2 files changed, 6 insertions(+), 8 deletions(-) diff --git a/crates/system-manager-engine/src/activate.rs b/crates/system-manager-engine/src/activate.rs index ab9ecb8d..ac6185fb 100644 --- a/crates/system-manager-engine/src/activate.rs +++ b/crates/system-manager-engine/src/activate.rs @@ -148,10 +148,10 @@ pub fn activate(store_path: &StorePath, ephemeral: bool, timeout: &Option { log::info!("Restarting sysinit-reactivation.target..."); - services::restart_sysinit_reactivation_target()?; + services::restart_sysinit_reactivation_target(timeout)?; // Restart userborn before tmpfiles so users exist when tmpfiles runs - if let Err(e) = services::restart_userborn_if_exists() { + if let Err(e) = services::restart_userborn_if_exists(timeout) { log::error!("Error restarting userborn.service: {e}"); } diff --git a/crates/system-manager-engine/src/activate/services.rs b/crates/system-manager-engine/src/activate/services.rs index 0810dd92..34b5248f 100644 --- a/crates/system-manager-engine/src/activate/services.rs +++ b/crates/system-manager-engine/src/activate/services.rs @@ -396,10 +396,9 @@ impl From for String { } } -pub fn restart_sysinit_reactivation_target() -> anyhow::Result<()> { +pub fn restart_sysinit_reactivation_target(timeout: &Option) -> anyhow::Result<()> { let service_manager = systemd::ServiceManager::new_session()?; let job_monitor = service_manager.monitor_jobs_init()?; - let timeout = Some(Duration::from_secs(30)); log::info!("Reloading the systemd daemon..."); service_manager.daemon_reload()?; @@ -410,14 +409,14 @@ pub fn restart_sysinit_reactivation_target() -> anyhow::Result<()> { "restarting", ); - wait_for_jobs(&service_manager, &job_monitor, jobs, &timeout)?; + wait_for_jobs(&service_manager, &job_monitor, jobs, timeout)?; Ok(()) } /// This must be called after daemon-reload so systemd knows about the unit, /// but before tmpfiles activation since tmpfiles may reference users that /// userborn needs to create. -pub fn restart_userborn_if_exists() -> anyhow::Result<()> { +pub fn restart_userborn_if_exists(timeout: &Option) -> anyhow::Result<()> { let service_manager = systemd::ServiceManager::new_session()?; // Check if userborn.service exists by listing units matching the pattern @@ -430,7 +429,6 @@ pub fn restart_userborn_if_exists() -> anyhow::Result<()> { log::info!("Restarting userborn.service to create users before tmpfiles..."); let job_monitor = service_manager.monitor_jobs_init()?; - let timeout = Some(Duration::from_secs(30)); // We use restart rather than start because userborn is a oneshot service // with RemainAfterExit=true. @@ -440,7 +438,7 @@ pub fn restart_userborn_if_exists() -> anyhow::Result<()> { "restarting", ); - wait_for_jobs(&service_manager, &job_monitor, jobs, &timeout)?; + wait_for_jobs(&service_manager, &job_monitor, jobs, timeout)?; log::info!("userborn.service completed"); Ok(()) } From 57bbf6850e1d1d0586415f13b0317a6af853fc38 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jean-Fran=C3=A7ois=20Roche?= Date: Sun, 20 Sep 2026 22:48:05 +0200 Subject: [PATCH 8/9] fix(tests): expose activation timeout controls --- .../test_driver/machine.py | 37 +++++++++++++++++-- nix/lib.nix | 16 +++++++- 2 files changed, 47 insertions(+), 6 deletions(-) diff --git a/lib/container-test-driver/test_driver/machine.py b/lib/container-test-driver/test_driver/machine.py index fe93eb64..cccfff0b 100644 --- a/lib/container-test-driver/test_driver/machine.py +++ b/lib/container-test-driver/test_driver/machine.py @@ -20,6 +20,11 @@ from .utils import CONTAINER_PATH, prepare_machine_root, retry +DEFAULT_ACTIVATION_TIMEOUT = 30 +MINIMUM_ACTIVATION_COMMAND_TIMEOUT = 300 +ACTIVATION_COMMAND_GRACE_PERIOD = 30 + + class TestInfraBackendNix(testinfra.backend.base.BaseBackend): """Testinfra backend that uses the container-test-driver Machine to run commands.""" @@ -482,24 +487,48 @@ def succeed(self, command: str, timeout: int | None = None) -> str: raise RuntimeError(msg) return res.stdout - def activate(self, profile: str | None = None) -> str: + def activate( + self, + profile: str | None = None, + timeout: int | None = DEFAULT_ACTIVATION_TIMEOUT, + ) -> str: """Activate system-manager profile and display the output. Args: profile: Path to system-manager profile. If None, uses the profile passed to the container. + timeout: Maximum time in seconds to wait for systemd jobs. None or + zero disables the timeout. """ if profile is None: profile = str(self.profile) if self.profile else None if profile is None: msg = "No profile specified for activation" raise Error(msg) - - activate_cmd = f"RUST_LOG=debug {profile}/bin/activate" + if timeout is not None and timeout < 0: + msg = "Activation timeout cannot be negative" + raise ValueError(msg) + + timeout_seconds = 0 if timeout is None else timeout + timeout_arg = ( + "" + if timeout_seconds == DEFAULT_ACTIVATION_TIMEOUT + else f" --timeout {timeout_seconds}" + ) + activate_path = shlex.quote(f"{profile}/bin/activate") + activate_cmd = f"RUST_LOG=debug {activate_path}{timeout_arg}" + command_timeout = ( + None + if timeout_seconds == 0 + else max( + MINIMUM_ACTIVATION_COMMAND_TIMEOUT, + timeout_seconds + ACTIVATION_COMMAND_GRACE_PERIOD, + ) + ) print(f"\n{Fore.CYAN}=== Activating system-manager ==={Style.RESET_ALL}") print(f"Profile: {profile}") - res = self.execute(activate_cmd, timeout=300) + res = self.execute(activate_cmd, timeout=command_timeout) # Always show the output if res.stdout.strip(): diff --git a/nix/lib.nix b/nix/lib.nix index c204d962..085e96df 100644 --- a/nix/lib.nix +++ b/nix/lib.nix @@ -218,17 +218,29 @@ let node, profile, action, + arguments ? [ ], }: + let + command = lib.escapeShellArgs ([ "${profile}/bin/${action}" ] ++ arguments); + in '' - ${node}.succeed("RUST_LOG=debug ${profile}/bin/${action} 2>&1 | tee /tmp/output.log") + ${node}.succeed("RUST_LOG=debug ${command} 2>&1 | tee /tmp/output.log") ${node}.succeed("! grep -F 'ERROR' /tmp/output.log") ''; activateProfileSnippet = - { node, profile }: + { + node, + profile, + timeout ? 30, + }: self.mkTestPreamble { inherit node profile; action = "activate"; + arguments = lib.optionals (timeout != 30) [ + "--timeout" + (if timeout == null then "0" else toString timeout) + ]; }; deactivateProfileSnippet = { node, profile }: From 9d2bb6ef6e5e925b441d55e2be0b2b54eab847a2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jean-Fran=C3=A7ois=20Roche?= Date: Sun, 20 Sep 2026 22:50:45 +0200 Subject: [PATCH 9/9] docs: document activation timeout controls --- docs/site/how-to/test-configuration.md | 15 ++++++++++++++- docs/site/reference/cli.md | 12 ++++++++++++ 2 files changed, 26 insertions(+), 1 deletion(-) diff --git a/docs/site/how-to/test-configuration.md b/docs/site/how-to/test-configuration.md index a267adca..1d672767 100644 --- a/docs/site/how-to/test-configuration.md +++ b/docs/site/how-to/test-configuration.md @@ -89,7 +89,7 @@ The test script has access to these symbols: | Method | Description | |--------|-------------| -| `machine.activate()` | Activate system-manager profile and display output | +| `machine.activate(profile=None, timeout=30)` | Activate a system-manager profile and display output | | `machine.succeed(cmd)` | Run command, fail test if exit code != 0 | | `machine.fail(cmd)` | Run command, fail test if exit code == 0 | | `machine.wait_for_unit(unit)` | Wait for systemd unit to be active | @@ -99,6 +99,19 @@ The test script has access to these symbols: | `machine.execute(cmd)` | Run command, return result (does not fail test) | | `machine.systemctl(args)` | Run systemctl with given arguments | +Pass `timeout` to control how long each systemd job batch may run during activation: + +```python +# Allow slow services up to 90 seconds per job batch. +machine.activate(timeout=90) + +# Wait indefinitely for systemd jobs. +machine.activate(timeout=None) +``` + +The command runner automatically allows enough time for the configured activation timeout. +Passing `0` is equivalent to passing `None`. + ### Testinfra assertions The `machine` object provides testinfra assertions for declarative checks on services, files, and users. diff --git a/docs/site/reference/cli.md b/docs/site/reference/cli.md index 130d8149..4c68a55c 100644 --- a/docs/site/reference/cli.md +++ b/docs/site/reference/cli.md @@ -45,6 +45,17 @@ The `switch` subcommand builds and activates your configuration immediately, mak **--sudo**: Specifies that System Manager can use sudo. +**--timeout SECONDS**: Sets how long each systemd job batch may run during activation. +The default is 30 seconds. +Set the value to `0` to wait indefinitely. + +**--no-timeout**: Waits indefinitely for systemd jobs. +This option cannot be combined with `--timeout`. + +```sh +system-manager switch --flake . --sudo --timeout 90 +``` + ### register The `register` subcommand builds and registers a System Manager configuration, but does not activate it. Compare this to `switch`, which does everything register does, but then activates it. @@ -56,6 +67,7 @@ The `build` subcommand builds everything needed for a switch, but does not regis ### deactivate The `deactivate` deactivates System Manager. +It accepts the same `--timeout SECONDS` and `--no-timeout` options as `switch`. ### pre-populate