diff --git a/CHANGELOG.md b/CHANGELOG.md index a804f70..d48437a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -257,6 +257,16 @@ EXPERIMENTAL, so on-disk formats and the CLI may change without notice. ### Fixed +- **`fabric doctor` reports whether the service is ENABLED, not just that its + unit file exists.** It read `service_installed` from the unit file's presence, + so a service disabled during an incident with its unit left in place said + "installed and managed by the OS" — and after a reboot no daemon started. The + CA trust check was fixed for exactly this mistake; the service check was not. + Doctor now queries the manager (`systemctl --user is-enabled`, or whether + launchd has the label loaded) and reads three states: enabled, present but not + enabled (a problem, because a reboot leaves no daemon), and not installed. + Finding 10 of the 2026-08-29 review. + - **`fabric doctor` on a non-default home asks the right daemon which build a peer runs.** Doctor shells out to `fabric exec -- fabric --version` without `--home`, so `fabric --home X doctor` asked the DEFAULT daemon about a diff --git a/src/doctor.rs b/src/doctor.rs index f79d0ee..525aad6 100644 --- a/src/doctor.rs +++ b/src/doctor.rs @@ -139,8 +139,9 @@ pub struct Facts { /// first-run report red, which is the one thing this design was for. pub manages_service: bool, pub daemon_running: bool, - /// `None` when it could not be determined, which is different from `false`. - pub service_installed: Option, + /// Whether the managed service will start on boot, not merely whether its + /// unit file exists. Presence is not enablement. + pub service: ServiceEnablement, pub own_version: String, pub peers: Vec, pub syncs: Vec, @@ -155,7 +156,11 @@ impl Facts { fn never_configured(&self) -> bool { !self.has_identity && self.peers.is_empty() - && !(self.manages_service && self.service_installed == Some(true)) + && !(self.manages_service + && matches!( + self.service, + ServiceEnablement::Enabled | ServiceEnablement::PresentNotEnabled + )) } } @@ -182,18 +187,29 @@ pub fn diagnose(facts: &Facts) -> Vec { "this home is not the managed one, so no OS service applies to it", ) } else { - match facts.service_installed { - Some(true) => Finding::new("service", Verdict::Ok, "installed and managed by the OS"), - Some(false) => Finding::new( + match facts.service { + ServiceEnablement::Enabled => { + Finding::new("service", Verdict::Ok, "installed, enabled, and will start on boot") + } + // The unit file exists but the manager will not start it on boot. This + // is NOT the same as "installed": a reboot leaves no daemon. It reads + // differently from "not installed" because the repair differs. + ServiceEnablement::PresentNotEnabled => Finding::new( + "service", + if fresh { Verdict::Setup } else { Verdict::Problem }, + "the service is installed but not enabled, so it will not start after a reboot", + ) + .with_action("fabric service install"), + ServiceEnablement::NotInstalled => Finding::new( "service", if fresh { Verdict::Setup } else { Verdict::Problem }, "fabric is not installed as a service, so it will not come back after a reboot", ) .with_action("fabric service install"), - None => Finding::new( + ServiceEnablement::Unknown => Finding::new( "service", Verdict::Unknown, - "could not tell whether fabric is installed as a service", + "could not tell whether fabric is enabled as a service", ), } }); @@ -550,7 +566,7 @@ mod tests { has_identity: true, manages_service: true, daemon_running: true, - service_installed: Some(true), + service: ServiceEnablement::Enabled, own_version: "0.2.0+abc".to_string(), peers: vec![PeerFact { label: "hetz".to_string(), @@ -603,7 +619,7 @@ mod tests { has_identity: false, manages_service: true, daemon_running: false, - service_installed: Some(false), + service: ServiceEnablement::NotInstalled, own_version: "0.2.0+abc".to_string(), peers: Vec::new(), syncs: Vec::new(), @@ -649,7 +665,7 @@ mod tests { manages_service: false, // ...but the unit is on disk for the prod home, and this is exactly // what the gatherer sees. - service_installed: Some(true), + service: ServiceEnablement::Enabled, daemon_running: false, own_version: "0.2.0+abc".to_string(), peers: Vec::new(), @@ -679,7 +695,7 @@ mod tests { #[test] fn the_same_gap_on_a_configured_machine_is_a_problem() { let mut facts = configured(); - facts.service_installed = Some(false); + facts.service = ServiceEnablement::NotInstalled; let findings = diagnose(&facts); let service = find(&findings, "service"); assert_eq!(service[0].verdict, Verdict::Problem); @@ -690,7 +706,7 @@ mod tests { #[test] fn a_check_that_could_not_look_says_unknown_and_counts() { let mut facts = configured(); - facts.service_installed = None; + facts.service = ServiceEnablement::Unknown; facts.peers[0].reachable = None; facts.ca.present = true; facts.ca.installed = None; @@ -775,6 +791,38 @@ mod tests { assert_eq!(argv.get(exec_at + 1).map(String::as_str), Some("hetz")); } + /// Finding 10: a unit file left in place by a `disable` is NOT "installed + /// and managed by the OS". It will not start after a reboot, and it reads + /// as a problem with a different repair from "never installed". + #[test] + fn a_present_but_not_enabled_service_is_a_problem_not_ok() { + let mut facts = configured(); + facts.service = ServiceEnablement::PresentNotEnabled; + let findings = diagnose(&facts); + let service = find(&findings, "service"); + assert!( + service + .iter() + .any(|f| f.verdict == Verdict::Problem && f.detail.contains("not enabled")), + "a disabled-but-present service did not read as a problem: {:?}", + service.iter().map(|f| (&f.verdict, &f.detail)).collect::>() + ); + assert!( + !service.iter().any(|f| f.detail.contains("installed, enabled")), + "a disabled service was still called enabled" + ); + + // And the healthy case still reads Ok, so the test is about the state + // and not about the check being broken. + facts.service = ServiceEnablement::Enabled; + let findings = diagnose(&facts); + assert!( + find(&findings, "service") + .iter() + .any(|f| f.verdict == Verdict::Ok && f.detail.contains("will start on boot")) + ); + } + #[test] fn a_denied_sync_and_an_unreachable_one_read_differently() { let mut facts = configured(); @@ -991,20 +1039,11 @@ mod tests { use anyhow::Result; use crate::ca; +use crate::service::ServiceEnablement; use crate::config::FabricHome; use crate::control::{ControlRequest, ControlResponse, PeerReachability, SyncEntryStatus}; /// Where the OS service definition would live on this platform. -fn service_unit_path() -> Result { - #[cfg(target_os = "macos")] - { - crate::service::launch_agent_path() - } - #[cfg(not(target_os = "macos"))] - { - crate::service::systemd_user_unit_path() - } -} /// Collect everything the checks reason about. /// @@ -1019,11 +1058,7 @@ where let has_identity = home.identity_path().exists(); let manages_service = home.is_default_state_root(); - let service_installed = match service_unit_path() { - Ok(path) => Some(path.exists()), - // Could not even work out where it would be. Not "no". - Err(_) => None, - }; + let service = crate::service::service_enablement(); let mut own_version = env!("CARGO_PKG_VERSION").to_string(); let mut daemon_running = false; @@ -1090,7 +1125,7 @@ where has_identity, manages_service, daemon_running, - service_installed, + service, own_version, peers, ca: gather_ca(home, &syncs), diff --git a/src/service.rs b/src/service.rs index a06e5ea..80d2def 100644 --- a/src/service.rs +++ b/src/service.rs @@ -297,6 +297,71 @@ fn resolve_memory_max_mb(home: &FabricHome, requested: Option>) -> R Ok(config.memory_max_mb()) } +/// Whether the managed service will actually start on boot — not merely whether +/// its unit file exists. +/// +/// Presence is not enablement. A service disabled during an incident, with its +/// unit file left in place, read as "installed and managed by the OS" and never +/// came back after a reboot. The CA trust check was fixed for exactly this +/// mistake; the service check was not. Finding 10 of the 2026-08-29 review. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum ServiceEnablement { + /// Enabled: the manager will start it on boot. + Enabled, + /// The unit or plist is present, but the manager will not start it on boot. + PresentNotEnabled, + /// No unit or plist at all. + NotInstalled, + /// Could not determine (the manager could not be queried). + Unknown, +} + +/// Query the OS service manager for the real enablement state. +pub fn service_enablement() -> ServiceEnablement { + match ServiceManager::current() { + Ok(manager) => manager.enablement(), + Err(_) => ServiceEnablement::Unknown, + } +} + +/// Interpret `systemctl --user is-enabled fabric.service`. +/// +/// `present` is whether the unit file exists, `ran` whether the query executed, +/// and `stdout` its first word. Kept free of `cfg` so it is tested on any host. +fn interpret_systemd_is_enabled(present: bool, ran: bool, stdout: &str) -> ServiceEnablement { + if !present { + return ServiceEnablement::NotInstalled; + } + if !ran { + return ServiceEnablement::Unknown; + } + match stdout.split_whitespace().next().unwrap_or("") { + "enabled" | "enabled-runtime" => ServiceEnablement::Enabled, + // The unit exists but will not be started on boot. `static`, `masked`, + // `linked`, `indirect`, `generated`, `transient` all mean "not enabled + // to start" for our purposes, and so does an empty answer with the file + // present. + "disabled" | "static" | "masked" | "linked" | "indirect" | "generated" + | "transient" | "" => ServiceEnablement::PresentNotEnabled, + _ => ServiceEnablement::Unknown, + } +} + +/// Interpret launchd signals: whether the plist is present and whether the +/// label is currently loaded (`launchctl print` succeeds). +fn interpret_launchd(present: bool, loaded: bool) -> ServiceEnablement { + if !present { + return ServiceEnablement::NotInstalled; + } + if loaded { + ServiceEnablement::Enabled + } else { + // The plist is on disk but launchd has not loaded it, so a reboot will + // not start it. This is the disabled-during-an-incident case. + ServiceEnablement::PresentNotEnabled + } +} + enum ServiceManager { #[cfg(target_os = "linux")] SystemdUser, @@ -319,6 +384,36 @@ impl ServiceManager { bail!("fabric service is currently supported on Linux systemd-user and macOS launchd"); } } + + fn enablement(&self) -> ServiceEnablement { + match self { + #[cfg(target_os = "linux")] + ServiceManager::SystemdUser => { + let present = systemd_user_unit_path() + .map(|path| path.exists()) + .unwrap_or(false); + let query = Command::new("systemctl") + .args(["--user", "is-enabled", SERVICE_NAME]) + .output(); + let (ran, stdout) = match &query { + Ok(output) => (true, String::from_utf8_lossy(&output.stdout).into_owned()), + Err(_) => (false, String::new()), + }; + interpret_systemd_is_enabled(present, ran, &stdout) + } + #[cfg(target_os = "macos")] + ServiceManager::LaunchdUser => { + let present = launch_agent_path().map(|path| path.exists()).unwrap_or(false); + let target = launchd_service_target(); + let loaded = Command::new("launchctl") + .args(["print", &target]) + .output() + .map(|output| output.status.success()) + .unwrap_or(false); + interpret_launchd(present, loaded) + } + } + } } #[cfg(target_os = "linux")] @@ -817,6 +912,46 @@ fn xml_escape(value: &str) -> String { #[cfg(test)] mod tests { + use super::{ServiceEnablement, interpret_launchd, interpret_systemd_is_enabled}; + + #[test] + fn systemd_is_enabled_reading_tells_enabled_from_merely_present() { + // The bug this replaces: a disabled service with its unit file present + // read as installed and never came back after a reboot. + assert_eq!( + interpret_systemd_is_enabled(true, true, "enabled\n"), + ServiceEnablement::Enabled + ); + for present_not_enabled in ["disabled", "static", "masked", "linked", ""] { + assert_eq!( + interpret_systemd_is_enabled(true, true, present_not_enabled), + ServiceEnablement::PresentNotEnabled, + "is-enabled={present_not_enabled:?} must not read as enabled" + ); + } + // No unit file at all, whatever the query says. + assert_eq!( + interpret_systemd_is_enabled(false, true, "enabled"), + ServiceEnablement::NotInstalled + ); + // Could not run the query, but the file is there: not a claim either way. + assert_eq!( + interpret_systemd_is_enabled(true, false, ""), + ServiceEnablement::Unknown + ); + } + + #[test] + fn launchd_reading_tells_loaded_from_merely_present() { + assert_eq!(interpret_launchd(true, true), ServiceEnablement::Enabled); + assert_eq!( + interpret_launchd(true, false), + ServiceEnablement::PresentNotEnabled, + "a plist on disk that launchd has not loaded will not start on boot" + ); + assert_eq!(interpret_launchd(false, false), ServiceEnablement::NotInstalled); + } + use super::*; /// The unit must name the binary it was GIVEN, not the one rendering it.