From 9d38414a24d257de2a87670c4e0a90b1791da46a Mon Sep 17 00:00:00 2001 From: Nathan Herald Date: Tue, 25 Aug 2026 11:57:35 +0200 Subject: [PATCH] service: install must be safe to re-run Two defects, both live on main, both found while planning `fabric update`. That command re-renders the unit on every run, so each of these would have gone from rare to constant. ONE. A RE-INSTALL SILENTLY DROPPED A CONFIGURED MEMORY CEILING. `allow_shell` and `allow_exec` survive a re-install because `resolve_allow_shell` and `resolve_allow_exec` read them back out of `config.toml` when the option is `None`. `memory_max_mb` had no such path. It lived only in the rendered plist or unit, and `render_*` emits it only when `Some`, so ANY install that did not name it removed a ceiling an operator had set earlier, without saying anything. It is now persisted beside the other two, with `resolve_memory_max_mb` in the same shape as its siblings. The option becomes a real tri-state, because a ceiling is itself optional: nothing said keeps what is persisted, `Some(None)` clears it, `Some(Some)` sets it. Persisting a value with no way to remove it would have made the first ceiling anybody set permanent, so `--no-memory-max-mb` is added alongside, mirroring `--no-allow-shell`. The install summary now prints the RESOLVED ceiling rather than what the caller passed. Those differ exactly when a persisted value was kept, which is the case the line exists to show. TWO. THE LINUX RESTART KILLED ITS OWN CALLER. `install_systemd_user` ended with `systemctl --user restart fabric.service`. `fabric exec` runs its session inside `fabric.service`'s cgroup, so `fabric exec hetz -- fabric service install` restarts the cgroup the caller lives in and dies partway through. Reachable today with a command any of us might run. The restart is now handed to systemd to run on its own, outside the caller's cgroup, three seconds out so the caller returns first. No `--unit` name is passed on purpose: systemd names the transient unit itself, so two updates close together cannot collide on a name that already exists. The install now says `restart scheduled`, because that is what it is. Claiming a restart that has not happened would make a failed start look like a successful install. BOTH TESTS WERE WATCHED FAILING, against a stub that reproduced the real behaviour rather than against nothing: a re-install that never mentioned the ceiling removed it left: None, right: Some(512) the restart is issued in place, so it kills its own caller `SERVICE_NAME` loses its `cfg(target_os = "linux")` gate so the restart argv and its test build everywhere. A Linux-only string cannot be tested from a Mac, and this is the shape whose regression takes a remote machine down. The restart test pins the COMMAND SHAPE, not the effect. The failure mode is that the caller dies, so a test that waited for the effect would be the thing that got killed. Agent: Silber.fabric --- src/config.rs | 17 ++++++ src/main.rs | 18 ++++++- src/service.rs | 139 ++++++++++++++++++++++++++++++++++++++++++++++--- 3 files changed, 164 insertions(+), 10 deletions(-) diff --git a/src/config.rs b/src/config.rs index 9c2e57b..c1d3907 100644 --- a/src/config.rs +++ b/src/config.rs @@ -493,6 +493,14 @@ pub struct FabricConfig { allow_shell: Option, #[serde(default)] allow_exec: Option, + /// The memory ceiling the last install asked for, in MiB. + /// + /// Persisted for the same reason the two allow flags are: the rendered + /// plist or unit is not a place to remember an operator's choice, because + /// the next re-render starts from whatever the caller passed and silently + /// drops what it does not mention. + #[serde(default, skip_serializing_if = "Option::is_none")] + memory_max_mb: Option, #[serde(default)] server_sessions: ServerSessionConfig, #[serde(default, skip_serializing_if = "Vec::is_empty")] @@ -565,6 +573,10 @@ impl FabricConfig { self.allow_exec } + pub fn memory_max_mb(&self) -> Option { + self.memory_max_mb + } + pub fn server_sessions(&self) -> &ServerSessionConfig { &self.server_sessions } @@ -577,6 +589,11 @@ impl FabricConfig { self.allow_exec = Some(allow_exec); } + /// `None` clears the ceiling, which is what `--no-memory-max-mb` asks for. + pub fn set_memory_max_mb(&mut self, memory_max_mb: Option) { + self.memory_max_mb = memory_max_mb; + } + pub fn exposes(&self) -> &[PersistedExpose] { &self.exposes } diff --git a/src/main.rs b/src/main.rs index 2a04f5f..fcbed4e 100644 --- a/src/main.rs +++ b/src/main.rs @@ -243,8 +243,12 @@ enum ServiceCommands { /// Memory ceiling applied by systemd/launchd, in MiB. Unset by default: /// a healthy working set depends on how much this node syncs, so Fabric /// declares no ceiling unless an operator measures one and asks for it. - #[arg(long)] + /// Once set it is remembered, so a later install that omits it keeps it. + #[arg(long, conflicts_with = "no_memory_max_mb")] memory_max_mb: Option, + /// Remove a previously persisted memory ceiling. + #[arg(long)] + no_memory_max_mb: bool, }, /// Show native service-manager status. Status, @@ -643,13 +647,14 @@ async fn main() -> Result<()> { allow_exec, no_allow_exec, memory_max_mb, + no_memory_max_mb, } => { service::install( &home, ServiceInstallOptions { allow_shell: allow_override(allow_shell, no_allow_shell), allow_exec: allow_override(allow_exec, no_allow_exec), - memory_max_mb, + memory_max_mb: memory_override(memory_max_mb, no_memory_max_mb), }, )?; } @@ -1515,6 +1520,15 @@ fn joined_or_dash(values: &[String]) -> String { /// Resolve an enable/disable flag pair into a tri-state override: `Some(true)` to /// enable, `Some(false)` to explicitly disable, `None` to leave the persisted /// value untouched. Shared by the shell and exec allow flags. +/// The same tri-state as `allow_override`, for a value that is itself optional. +/// Nothing said keeps the persisted ceiling; `--no-memory-max-mb` clears it. +fn memory_override(value: Option, clear: bool) -> Option> { + if clear { + return Some(None); + } + value.map(Some) +} + fn allow_override(enable: bool, disable: bool) -> Option { if enable { Some(true) diff --git a/src/service.rs b/src/service.rs index f4ae6a9..9494975 100644 --- a/src/service.rs +++ b/src/service.rs @@ -10,7 +10,9 @@ use anyhow::{Context, Result, bail}; use crate::config::{FabricConfig, FabricHome}; -#[cfg(target_os = "linux")] +/// Not `cfg`-gated, so the restart argv below and its test build on every +/// platform. A Linux-only string cannot be tested from a Mac, and this is the +/// exact shape whose regression takes a remote machine down. const SERVICE_NAME: &str = "fabric.service"; const LAUNCHD_LABEL: &str = "com.compoundingtech.fabric"; /// How long to wait for launchd to fully unload a booted-out service before @@ -30,9 +32,10 @@ const LAUNCHD_BOOTSTRAP_MAX_ATTEMPTS: usize = 5; pub struct ServiceInstallOptions { pub allow_shell: Option, pub allow_exec: Option, - /// Operator-declared memory ceiling. `None` means no ceiling is written into - /// the generated unit or plist at all. - pub memory_max_mb: Option, + /// Operator-declared memory ceiling, tri-state because a ceiling is itself + /// optional. `None` means the caller never mentioned it, so keep whatever is + /// persisted. `Some(None)` clears it. `Some(Some(mb))` sets it. + pub memory_max_mb: Option>, } #[derive(Debug, Clone)] @@ -112,7 +115,8 @@ pub fn install(home: &FabricHome, options: ServiceInstallOptions) -> Result<()> home.prepare()?; let allow_shell = resolve_allow_shell(home, options.allow_shell)?; let allow_exec = resolve_allow_exec(home, options.allow_exec)?; - let spec = ServiceSpec::current(home, allow_shell, allow_exec, options.memory_max_mb)?; + let memory_max_mb = resolve_memory_max_mb(home, options.memory_max_mb)?; + let spec = ServiceSpec::current(home, allow_shell, allow_exec, memory_max_mb)?; match ServiceManager::current()? { #[cfg(target_os = "linux")] ServiceManager::SystemdUser => install_systemd_user(&spec)?, @@ -131,10 +135,12 @@ pub fn install(home: &FabricHome, options: ServiceInstallOptions) -> Result<()> println!("home\t{}", home.root().display()); println!("allow-shell\t{allow_shell}"); println!("allow-exec\t{allow_exec}"); + // Report the RESOLVED ceiling, not what the caller passed. They differ + // whenever the caller said nothing and a persisted ceiling was kept, which + // is precisely the case this line exists to make visible. println!( "memory-max-mb\t{}", - options - .memory_max_mb + memory_max_mb .map(|mb| mb.to_string()) .unwrap_or_else(|| "unset".to_string()) ); @@ -219,6 +225,49 @@ fn resolve_allow_exec(home: &FabricHome, requested: Option) -> Result (&'static str, Vec) { + ( + "systemd-run", + vec![ + "--user".into(), + // Long enough that the caller returns before its cgroup goes away, + // short enough that an operator is not left waiting on it. + "--on-active=3".into(), + "systemctl".into(), + "--user".into(), + "restart".into(), + SERVICE_NAME.into(), + ], + ) +} + +/// Resolve the memory ceiling, in the same shape as the two allow flags above. +/// +/// The tri-state matters. A caller that says nothing must KEEP the persisted +/// ceiling; only an explicit `--no-memory-max-mb` removes one. Before this +/// existed the ceiling lived solely in the rendered unit, so every re-render +/// that did not name it threw it away, silently. +fn resolve_memory_max_mb(home: &FabricHome, requested: Option>) -> Result> { + let mut config = FabricConfig::load(home)?; + if let Some(memory_max_mb) = requested { + config.set_memory_max_mb(memory_max_mb); + config.save(home)?; + return Ok(memory_max_mb); + } + Ok(config.memory_max_mb()) +} + enum ServiceManager { #[cfg(target_os = "linux")] SystemdUser, @@ -255,8 +304,13 @@ fn install_systemd_user(spec: &ServiceSpec) -> Result<()> { run_command("systemctl", &["--user", "daemon-reload"])?; run_command("systemctl", &["--user", "enable", SERVICE_NAME])?; - run_command("systemctl", &["--user", "restart", SERVICE_NAME])?; + let (program, args) = systemd_restart_argv(); + let args: Vec<&str> = args.iter().map(String::as_str).collect(); + run_command(program, &args)?; println!("unit\t{}", unit_path.display()); + // Say scheduled, because it is. Claiming a restart that has not happened yet + // would make a failed start look like a successful install. + println!("restart\tscheduled"); Ok(()) } @@ -736,6 +790,75 @@ fn xml_escape(value: &str) -> String { mod tests { use super::*; + /// A restart issued from inside the service's own cgroup kills the process + /// issuing it. + /// + /// `fabric exec` runs its session inside `fabric.service`, so + /// `fabric exec hetz -- fabric service install` restarts the very cgroup the + /// caller lives in and dies partway through. That is reachable today with a + /// command any of us might run. + /// + /// The restart therefore has to be handed to systemd to run on its own, + /// outside the caller's cgroup. This pins the shape rather than the effect, + /// because the failure mode is that the CALLER dies — a test that waited for + /// the effect would be the thing that got killed. + #[test] + fn the_linux_service_restart_is_detached_from_the_caller() { + let (program, args) = systemd_restart_argv(); + assert_eq!( + program, "systemd-run", + "the restart is issued in place, so it kills its own caller" + ); + assert!( + args.iter().any(|arg| arg == "--user"), + "a user service restart must stay in the user manager: {args:?}" + ); + assert!( + args.iter().any(|arg| arg.starts_with("--on-active=")), + "the restart must be scheduled, so the caller can return first: {args:?}" + ); + let args: Vec<&str> = args.iter().map(String::as_str).collect(); + assert!( + args.windows(4) + .any(|w| w == ["systemctl", "--user", "restart", SERVICE_NAME]), + "whatever wrapping it gains, it must still restart {SERVICE_NAME}: {args:?}" + ); + } + + /// A ceiling an operator set once must survive a re-install that does not + /// mention it. + /// + /// `allow_shell` and `allow_exec` already survive, because they round trip + /// through `config.toml`. `memory_max_mb` did not: it lived only in the + /// rendered plist or unit, and `render_*` emits it only when `Some`. So any + /// re-install without `--memory-max-mb` removed a ceiling somebody set + /// earlier, silently. + /// + /// `fabric update` re-renders the unit on every run, so this would have + /// fired constantly rather than rarely. + #[test] + fn a_memory_ceiling_survives_a_reinstall_that_does_not_mention_it() -> Result<()> { + let dir = tempfile::tempdir()?; + let home = FabricHome::new(dir.path()); + home.prepare()?; + + // An operator sets a ceiling once. + assert_eq!( + resolve_memory_max_mb(&home, Some(Some(512)))?, + Some(512), + "the ceiling the operator asked for was not the one applied" + ); + + // A later install says nothing about memory. It must not remove it. + assert_eq!( + resolve_memory_max_mb(&home, None)?, + Some(512), + "a re-install that never mentioned the ceiling removed it" + ); + Ok(()) + } + + #[test] fn bootout_no_such_process_is_ignorable_but_real_errors_surface() { // Fresh install / already-stopped service — nothing to unload — suppress.