fix(service): strip Disabled before load, verify it, and honour the plist

service-manager 0.11 writes Disabled: true into every plist carrying
KeepAlive, and our RestartPolicy::Always guarantees KeepAlive, so every
agent we installed was born disabled. launchctl load honours the key while
still exiting 0, so install reported success on an agent that would never
start — not then and not at the next login.

service::start now removes the Disabled key itself before loading, via a
pure enable_plist() that rewrites nothing when the key is absent and
preserves every other key, including the EnvironmentVariables PATH
snapshot. That makes start self-healing for plists left disabled by an
earlier build. The crate's own start() is still not used, since without
Disabled it degrades to launchctl start, which fails on an unloaded job.

Because launchctl load exits 0 on failure, start also checks a
post-condition: it asks launchd whether the job now exists and reports a
diagnostic if it does not. stop keeps no such check, since a benign unload
of an already-stopped job also prints a failure while exiting 0.

status now treats the plist on disk as the definition of installed, as the
spec says: a plist that exists but is not loaded reports stopped with its
program, PATH and snapshot date intact instead of collapsing to
not-installed with every field cleared. That is precisely the state the
Disabled bug left users in, so it is the state status most needs to
describe.

status also gains the log path the spec always listed, and the daemon's own
log is renamed xy.log -> daemon.log so a supervised server named xy cannot
share a file, and two rotation counters, with the daemon.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EGntTHCW3sEPy1VBRopNNp

crates/xy/src/cli/service.rs

@@ -1,3 +1,4 @@
+use crate::logging;
 use crate::paths::Paths;
 use crate::service::{self, AgentSpec, AgentState, AgentStatus, DEFAULT_LABEL};
 use anyhow::Result;
@@ -28,6 +29,8 @@ pub(crate) fn render_status(status: &AgentStatus) -> String {
         }
     }
 
+    out.push_str(&format!("  log:     {}\n", status.log.display()));
+
     out
 }
 
@@ -125,7 +128,9 @@ fn toggle(action: fn(&str) -> Result<()>, verb: &str) -> Result<i32> {
 }
 
 fn status(paths: &Paths) -> Result<i32> {
-    let status = service::status(DEFAULT_LABEL, &paths.pidfile)?;
+    let log = logging::daemon_log_path(&paths.log_dir);
+
+    let status = service::status(DEFAULT_LABEL, &paths.pidfile, &log)?;
 
     print!("{}", render_status(&status));
 
@@ -141,6 +146,7 @@ mod tests {
         AgentStatus {
             label: "se.aceofba.xy".to_string(),
             plist: PathBuf::from("/Users/me/Library/LaunchAgents/se.aceofba.xy.plist"),
+            log: PathBuf::from("/Users/me/.local/state/xy/logs/daemon.log"),
             state: AgentState::Running,
             program: Some(PathBuf::from("/Users/me/.cargo/bin/xy")),
             path_env: Some("/opt/homebrew/bin:/usr/bin".to_string()),
@@ -196,4 +202,24 @@ mod tests {
         assert!(!out.contains("program:"));
         assert!(!out.contains("path:"));
     }
+
+    #[test]
+    fn render_status_shows_the_daemon_log_path() {
+        let out = render_status(&running_status());
+
+        assert!(out.contains("  log:     /Users/me/.local/state/xy/logs/daemon.log\n"));
+    }
+
+    #[test]
+    fn render_status_shows_the_log_path_even_when_not_installed() {
+        let mut status = running_status();
+        status.state = AgentState::NotInstalled;
+        status.program = None;
+        status.path_env = None;
+        status.pid = None;
+
+        let out = render_status(&status);
+
+        assert!(out.contains("log:     /Users/me/.local/state/xy/logs/daemon.log"));
+    }
 }

crates/xy/src/logging.rs

@@ -1,13 +1,19 @@
-use std::path::Path;
+use std::path::{Path, PathBuf};
 use std::sync::Mutex;
 use xy_supervisor::logs::RotatingLogWriter;
 
 const LOG_FILE_MAX_BYTES: u64 = 10 * 1024 * 1024;
 const LOG_FILE_KEEP: usize = 5;
 
+/// Distinct from `{server_name}.log` so a server named `xy` cannot collide with
+/// the daemon's own log and give two writers independent rotation counters.
+pub(crate) fn daemon_log_path(log_dir: &Path) -> PathBuf {
+    log_dir.join("daemon.log")
+}
+
 pub(crate) fn daemon_writer(log_dir: &Path) -> std::io::Result<Mutex<RotatingLogWriter>> {
     let writer =
-        RotatingLogWriter::open(&log_dir.join("xy.log"), LOG_FILE_MAX_BYTES, LOG_FILE_KEEP)?;
+        RotatingLogWriter::open(&daemon_log_path(log_dir), LOG_FILE_MAX_BYTES, LOG_FILE_KEEP)?;
 
     Ok(Mutex::new(writer))
 }
@@ -17,7 +23,7 @@ mod tests {
     use super::*;
 
     #[test]
-    fn daemon_writer_creates_and_appends_to_xy_log() {
+    fn daemon_writer_creates_and_appends_to_daemon_log() {
         use std::io::Write;
 
         let tmp = tempfile::tempdir().unwrap();
@@ -26,7 +32,15 @@ mod tests {
 
         writer.lock().unwrap().write_all(b"line\n").unwrap();
 
-        let contents = std::fs::read_to_string(tmp.path().join("xy.log")).unwrap();
+        let contents = std::fs::read_to_string(tmp.path().join("daemon.log")).unwrap();
         assert_eq!(contents, "line\n");
     }
+
+    #[test]
+    fn daemon_log_path_does_not_collide_with_a_server_named_xy() {
+        let path = daemon_log_path(Path::new("/state/xy/logs"));
+
+        assert_eq!(path, PathBuf::from("/state/xy/logs/daemon.log"));
+        assert_ne!(path, PathBuf::from("/state/xy/logs/xy.log"));
+    }
 }

crates/xy/src/service.rs

@@ -124,6 +124,7 @@ pub(crate) enum AgentState {
 pub(crate) struct AgentStatus {
     pub label: String,
     pub plist: PathBuf,
+    pub log: PathBuf,
     pub state: AgentState,
     pub program: Option<PathBuf>,
     pub path_env: Option<String>,
@@ -139,10 +140,11 @@ pub(crate) fn read_pid(pidfile: &Path) -> Option<u32> {
         .ok()
 }
 
-fn not_installed_status(label: &str, plist: PathBuf) -> AgentStatus {
+fn not_installed_status(label: &str, plist: PathBuf, log: PathBuf) -> AgentStatus {
     AgentStatus {
         label: label.to_string(),
         plist,
+        log,
         state: AgentState::NotInstalled,
         program: None,
         path_env: None,
@@ -151,11 +153,13 @@ fn not_installed_status(label: &str, plist: PathBuf) -> AgentStatus {
     }
 }
 
-fn build_status(label: &str, plist: PathBuf, pidfile: &Path, state: AgentState) -> AgentStatus {
-    if state == AgentState::NotInstalled {
-        return not_installed_status(label, plist);
-    }
-
+fn build_status(
+    label: &str,
+    plist: PathBuf,
+    log: PathBuf,
+    pidfile: &Path,
+    state: AgentState,
+) -> AgentStatus {
     let snapshotted = std::fs::metadata(&plist)
         .and_then(|meta| meta.modified())
         .ok();
@@ -171,6 +175,7 @@ fn build_status(label: &str, plist: PathBuf, pidfile: &Path, state: AgentState)
     AgentStatus {
         label: label.to_string(),
         plist,
+        log,
         state,
         program,
         path_env,
@@ -179,22 +184,41 @@ fn build_status(label: &str, plist: PathBuf, pidfile: &Path, state: AgentState)
     }
 }
 
-pub(crate) fn status(label: &str, pidfile: &Path) -> Result<AgentStatus> {
+/// The plist on disk is the authority on "installed"; the crate's verdict only
+/// distinguishes running from stopped. A plist that exists but is not loaded
+/// makes `launchctl print` answer `NotInstalled`, which must not erase the
+/// plist-derived facts a user needs to debug that exact state.
+fn installed_state(reported: &ServiceStatus) -> AgentState {
+    match reported {
+        ServiceStatus::Running => AgentState::Running,
+        ServiceStatus::Stopped(_) | ServiceStatus::NotInstalled => AgentState::Stopped,
+    }
+}
+
+pub(crate) fn status(label: &str, pidfile: &Path, log: &Path) -> Result<AgentStatus> {
     let plist = plist_path_for(label)?;
 
     if !plist.exists() {
-        return Ok(not_installed_status(label, plist));
+        return Ok(not_installed_status(label, plist, log.to_path_buf()));
     }
 
+    let reported = query_state(label)?;
+
+    Ok(build_status(
+        label,
+        plist,
+        log.to_path_buf(),
+        pidfile,
+        installed_state(&reported),
+    ))
+}
+
+fn query_state(label: &str) -> Result<ServiceStatus> {
     let parsed: ServiceLabel = label.parse().context("parse service label")?;
 
-    let state = match manager().status(ServiceStatusCtx { label: parsed })? {
-        ServiceStatus::Running => AgentState::Running,
-        ServiceStatus::Stopped(_) => AgentState::Stopped,
-        ServiceStatus::NotInstalled => AgentState::NotInstalled,
-    };
-
-    Ok(build_status(label, plist, pidfile, state))
+    manager()
+        .status(ServiceStatusCtx { label: parsed })
+        .context("query launchd for agent status")
 }
 
 fn read_plist_fields(plist: &Path) -> (Option<PathBuf>, Option<String>) {
@@ -247,8 +271,44 @@ fn launchctl_error(verb: &str, stdout: &[u8], stderr: &[u8]) -> anyhow::Error {
     anyhow::anyhow!("launchctl {verb} failed: {}", detail.join("; "))
 }
 
+/// `service-manager` writes `Disabled: true` into every plist carrying
+/// `KeepAlive`, and `launchctl load` honours it while still exiting 0. Removing
+/// the key is what actually makes the agent loadable, so do it on every start —
+/// that also heals a plist left disabled by an earlier install.
+fn enable_plist(plist: &Path) -> Result<()> {
+    let mut value = plist::Value::from_file(plist)
+        .with_context(|| format!("read plist {}", plist.display()))?;
+
+    let Some(dict) = value.as_dictionary_mut() else {
+        return Ok(());
+    };
+
+    if dict.remove("Disabled").is_none() {
+        return Ok(());
+    }
+
+    value
+        .to_file_xml(plist)
+        .with_context(|| format!("rewrite plist {}", plist.display()))
+}
+
 pub(crate) fn start(label: &str) -> Result<()> {
-    launchctl("load", &plist_path_for(label)?)
+    let plist = plist_path_for(label)?;
+
+    enable_plist(&plist)?;
+
+    launchctl("load", &plist)?;
+
+    // `launchctl load` reports its own failures only on stderr and still exits
+    // 0, so the load is confirmed by asking launchd whether the job now exists.
+    if query_state(label)? == ServiceStatus::NotInstalled {
+        anyhow::bail!(
+            "launchctl load left {label} unregistered; inspect {} and `launchctl print gui/$UID/{label}`",
+            plist.display()
+        );
+    }
+
+    Ok(())
 }
 
 pub(crate) fn stop(label: &str) -> Result<()> {
@@ -345,13 +405,30 @@ mod tests {
     fn status_reports_not_installed_when_plist_is_absent() {
         let tmp = tempfile::tempdir().unwrap();
 
-        let status = status("se.aceofba.xy-absent", &tmp.path().join("xy.pid")).unwrap();
+        let status = status(
+            "se.aceofba.xy-absent",
+            &tmp.path().join("xy.pid"),
+            &tmp.path().join("daemon.log"),
+        )
+        .unwrap();
 
         assert!(matches!(status.state, AgentState::NotInstalled));
         assert!(status.program.is_none());
+        assert!(status.path_env.is_none());
+        assert!(status.snapshotted.is_none());
         assert!(status.pid.is_none());
     }
 
+    #[test]
+    fn status_carries_the_log_path_through() {
+        let tmp = tempfile::tempdir().unwrap();
+        let log = tmp.path().join("daemon.log");
+
+        let status = status("se.aceofba.xy-absent", &tmp.path().join("xy.pid"), &log).unwrap();
+
+        assert_eq!(status.log, log);
+    }
+
     #[test]
     fn read_pid_parses_a_pidfile() {
         let tmp = tempfile::tempdir().unwrap();
@@ -450,7 +527,7 @@ mod tests {
     }
 
     #[test]
-    fn not_installed_state_clears_all_optional_fields_even_when_plist_exists() {
+    fn an_unloaded_plist_is_stopped_not_missing_and_keeps_its_facts() {
         let tmp = tempfile::tempdir().unwrap();
         let plist_path = tmp.path().join("agent.plist");
 
@@ -459,16 +536,155 @@ mod tests {
         let pidfile = tmp.path().join("xy.pid");
         std::fs::write(&pidfile, "4821").unwrap();
 
+        let state = installed_state(&ServiceStatus::NotInstalled);
+
+        assert_eq!(state, AgentState::Stopped);
+
         let status = build_status(
             "se.aceofba.xy-ghost",
             plist_path,
+            tmp.path().join("daemon.log"),
             &pidfile,
-            AgentState::NotInstalled,
+            state,
         );
 
-        assert!(status.program.is_none());
-        assert!(status.path_env.is_none());
-        assert!(status.snapshotted.is_none());
+        assert_eq!(status.program, Some(PathBuf::from("/usr/local/bin/xy")));
+        assert_eq!(status.path_env, Some("/usr/bin".to_string()));
+        assert!(status.snapshotted.is_some());
         assert!(status.pid.is_none());
     }
+
+    #[test]
+    fn installed_state_reports_running_only_for_a_running_job() {
+        assert_eq!(
+            installed_state(&ServiceStatus::Running),
+            AgentState::Running
+        );
+        assert_eq!(
+            installed_state(&ServiceStatus::Stopped(None)),
+            AgentState::Stopped
+        );
+    }
+
+    fn write_disabled_agent_plist(path: &Path, disabled: Option<bool>) {
+        let mut env = plist::Dictionary::new();
+        env.insert(
+            "PATH".to_string(),
+            plist::Value::String("/opt/homebrew/bin:/usr/bin".to_string()),
+        );
+
+        let mut dict = plist::Dictionary::new();
+        dict.insert(
+            "Label".to_string(),
+            plist::Value::String("se.x".to_string()),
+        );
+        dict.insert(
+            "ProgramArguments".to_string(),
+            plist::Value::Array(vec![
+                plist::Value::String("/usr/local/bin/xy".to_string()),
+                plist::Value::String("daemon".to_string()),
+            ]),
+        );
+        dict.insert(
+            "EnvironmentVariables".to_string(),
+            plist::Value::Dictionary(env),
+        );
+        dict.insert("KeepAlive".to_string(), plist::Value::Boolean(true));
+        dict.insert("RunAtLoad".to_string(), plist::Value::Boolean(true));
+
+        if let Some(disabled) = disabled {
+            dict.insert("Disabled".to_string(), plist::Value::Boolean(disabled));
+        }
+
+        plist::Value::Dictionary(dict).to_file_xml(path).unwrap();
+    }
+
+    fn plist_dict(path: &Path) -> plist::Dictionary {
+        plist::Value::from_file(path)
+            .unwrap()
+            .into_dictionary()
+            .unwrap()
+    }
+
+    #[test]
+    fn enable_plist_removes_disabled_and_preserves_every_other_key() {
+        let tmp = tempfile::tempdir().unwrap();
+        let plist_path = tmp.path().join("agent.plist");
+
+        write_disabled_agent_plist(&plist_path, Some(true));
+
+        enable_plist(&plist_path).unwrap();
+
+        let dict = plist_dict(&plist_path);
+
+        assert!(!dict.contains_key("Disabled"));
+        assert_eq!(dict.get("Label").unwrap().as_string(), Some("se.x"));
+        assert_eq!(dict.get("KeepAlive").unwrap().as_boolean(), Some(true));
+        assert_eq!(dict.get("RunAtLoad").unwrap().as_boolean(), Some(true));
+
+        let args: Vec<&str> = dict
+            .get("ProgramArguments")
+            .and_then(plist::Value::as_array)
+            .unwrap()
+            .iter()
+            .filter_map(plist::Value::as_string)
+            .collect();
+
+        assert_eq!(args, vec!["/usr/local/bin/xy", "daemon"]);
+
+        let path_env = dict
+            .get("EnvironmentVariables")
+            .and_then(plist::Value::as_dictionary)
+            .and_then(|env| env.get("PATH"))
+            .and_then(plist::Value::as_string);
+
+        assert_eq!(path_env, Some("/opt/homebrew/bin:/usr/bin"));
+    }
+
+    #[test]
+    fn enable_plist_is_a_no_op_when_disabled_is_absent() {
+        let tmp = tempfile::tempdir().unwrap();
+        let plist_path = tmp.path().join("agent.plist");
+
+        write_disabled_agent_plist(&plist_path, None);
+
+        let before = std::fs::read(&plist_path).unwrap();
+
+        enable_plist(&plist_path).unwrap();
+
+        let after = std::fs::read(&plist_path).unwrap();
+
+        assert_eq!(before, after);
+    }
+
+    #[test]
+    fn enable_plist_is_idempotent() {
+        let tmp = tempfile::tempdir().unwrap();
+        let plist_path = tmp.path().join("agent.plist");
+
+        write_disabled_agent_plist(&plist_path, Some(true));
+
+        enable_plist(&plist_path).unwrap();
+
+        let once = std::fs::read(&plist_path).unwrap();
+
+        enable_plist(&plist_path).unwrap();
+
+        let twice = std::fs::read(&plist_path).unwrap();
+
+        assert_eq!(once, twice);
+        assert!(!plist_dict(&plist_path).contains_key("Disabled"));
+    }
+
+    #[test]
+    fn enable_plist_errors_on_an_unreadable_plist() {
+        let tmp = tempfile::tempdir().unwrap();
+        let plist_path = tmp.path().join("broken.plist");
+
+        std::fs::write(&plist_path, "<plist><dict><key>Disab").unwrap();
+
+        let err = enable_plist(&plist_path).unwrap_err().to_string();
+
+        assert!(err.contains("read plist"));
+    }
 }