diff --git a/README.md b/README.md index 59de6a4..ccd95be 100644 --- a/README.md +++ b/README.md @@ -41,7 +41,7 @@ after installing a new toolchain to refresh the snapshot. `xy service stop` lasts until the next login. To disable start-on-login permanently, use `xy service uninstall`. -The daemon writes to `$XDG_STATE_HOME/xy/logs/xy.log`. Failures that happen +The daemon writes to `$XDG_STATE_HOME/xy/logs/daemon.log`. Failures that happen before the daemon starts logging — a missing binary, a malformed plist — are visible only to launchd: diff --git a/crates/xy/src/cli/service.rs b/crates/xy/src/cli/service.rs index e6aae48..8c01ef0 100644 --- a/crates/xy/src/cli/service.rs +++ b/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 { } fn status(paths: &Paths) -> Result { - 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")); + } } diff --git a/crates/xy/src/logging.rs b/crates/xy/src/logging.rs index d1d2f2a..91059d6 100644 --- a/crates/xy/src/logging.rs +++ b/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> { 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")); + } } diff --git a/crates/xy/src/service.rs b/crates/xy/src/service.rs index ac2e663..0a294b3 100644 --- a/crates/xy/src/service.rs +++ b/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, pub path_env: Option, @@ -139,10 +140,11 @@ pub(crate) fn read_pid(pidfile: &Path) -> Option { .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 { +/// 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 { 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 { 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, Option) { @@ -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) { + 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, "Disab").unwrap(); + + let err = enable_plist(&plist_path).unwrap_err().to_string(); + + assert!(err.contains("read plist")); + } } diff --git a/docs/superpowers/plans/2026-07-31-xy-start-on-login.md b/docs/superpowers/plans/2026-07-31-xy-start-on-login.md index a91e852..00dd3ab 100644 --- a/docs/superpowers/plans/2026-07-31-xy-start-on-login.md +++ b/docs/superpowers/plans/2026-07-31-xy-start-on-login.md @@ -158,7 +158,7 @@ git commit -m "feat(logs): impl io::Write for RotatingLogWriter" ### Task 2: Daemon log file -The daemon currently logs only to stderr, which launchd discards. Give it `log_dir/xy.log` using the same rotation as per-server logs, and reorder `main.rs` so paths resolve before the logger is built. +The daemon currently logs only to stderr, which launchd discards. Give it `log_dir/daemon.log` using the same rotation as per-server logs, and reorder `main.rs` so paths resolve before the logger is built. **Files:** - Create: `crates/xy/src/logging.rs` @@ -193,7 +193,7 @@ 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"); } } @@ -224,7 +224,7 @@ const LOG_FILE_MAX_BYTES: u64 = 10 * 1024 * 1024; const LOG_FILE_KEEP: usize = 5; pub(crate) fn daemon_writer(log_dir: &Path) -> std::io::Result> { - let writer = RotatingLogWriter::open(&log_dir.join("xy.log"), LOG_FILE_MAX_BYTES, LOG_FILE_KEEP)?; + let writer = RotatingLogWriter::open(&log_dir.join("daemon.log"), LOG_FILE_MAX_BYTES, LOG_FILE_KEEP)?; Ok(Mutex::new(writer)) } @@ -304,14 +304,14 @@ paths.ensure_dirs().context("create state dirs")?; ```bash cargo build -p xy -rm -f ~/.local/state/xy/logs/xy.log +rm -f ~/.local/state/xy/logs/daemon.log ./target/debug/xy daemon & sleep 2 -cat ~/.local/state/xy/logs/xy.log +cat ~/.local/state/xy/logs/daemon.log kill %1 ``` -Expected: `xy.log` exists and contains a `daemon listening` line. +Expected: `daemon.log` exists and contains a `daemon listening` line. - [ ] **Step 8: Run the full suite, format, lint** @@ -1203,7 +1203,7 @@ after installing a new toolchain to refresh the snapshot. `xy service stop` lasts until the next login. To disable start-on-login permanently, use `xy service uninstall`. -The daemon writes to `$XDG_STATE_HOME/xy/logs/xy.log`. Failures that happen +The daemon writes to `$XDG_STATE_HOME/xy/logs/daemon.log`. Failures that happen before the daemon starts logging — a missing binary, a malformed plist — are visible only to launchd: @@ -1236,7 +1236,7 @@ cargo install --path crates/xy xy service install xy service status # expect: state: running (pid N) xy list # expect: configured servers, reached via the daemon -tail ~/.local/state/xy/logs/xy.log +tail ~/.local/state/xy/logs/daemon.log ``` Then log out and back in, and confirm `xy service status` still reports `running` with a different pid. That last step is the only real proof that start-on-login works, because it is the only one that exercises `RunAtLoad`. diff --git a/docs/superpowers/specs/2026-07-31-xy-start-on-login-design.md b/docs/superpowers/specs/2026-07-31-xy-start-on-login-design.md index b576e4e..1fb05ef 100644 --- a/docs/superpowers/specs/2026-07-31-xy-start-on-login-design.md +++ b/docs/superpowers/specs/2026-07-31-xy-start-on-login-design.md @@ -79,7 +79,7 @@ bypass it with the `contents: Option` escape hatch and hand-author plist XML, the daemon gains its own log file (see below). **Accepted limitation.** Failures occurring before the daemon's logger exists — -missing binary, dyld error, malformed plist — appear in neither `xy.log` nor +missing binary, dyld error, malformed plist — appear in neither `daemon.log` nor `xy service status`. This is worse than first assumed: `LaunchdServiceManager:: status()` returns `ServiceStatus::Stopped(None)` unconditionally (`launchd.rs:288`), so the `Option` reason carried by the enum is always @@ -152,7 +152,7 @@ server config using a relative `working_dir` resolves somewhere predictable. which is too late to open a log file for the logger itself. For the `Cmd::Daemon` arm only, the subscriber writes to `stderr.and(file)` via -`MakeWriterExt`, where the file half is `log_dir/xy.log` backed by the existing +`MakeWriterExt`, where the file half is `log_dir/daemon.log` backed by the existing `xy_supervisor::logs::RotatingLogWriter` (10 MB × 5, the same rotation used for per-server logs). No adapter type is needed: `tracing-subscriber` 0.3 implements `MakeWriter` for `Mutex where W: io::Write` @@ -175,7 +175,7 @@ type already tracks `written` and rotates, but today exposes only `write_line(tag, line)`, which prefixes a tag the daemon's own log does not want. -Result: `~/.local/state/xy/logs/` becomes uniform — `xy.log` for the daemon, +Result: `~/.local/state/xy/logs/` becomes uniform — `daemon.log` for the daemon, `.log` per supervised server, all rotated by the same code. ## launchd mechanics @@ -264,14 +264,16 @@ Sample output: state: running (pid 4821) program: /Users/olsson/.cargo/bin/xy path: /opt/homebrew/bin:… (snapshotted 2026-07-31) - log: ~/.local/state/xy/logs/xy.log + log: ~/.local/state/xy/logs/daemon.log ### Exit codes Reuses the established scheme, minus the codes that cannot apply. `0` success, `1` operational error (launchctl failed, agent missing, permission denied). Code `2` (daemon unreachable) is structurally impossible because these commands -never open the socket; `3` (config invalid) does not arise. +never open the socket. Code `3` is reachable only before dispatch: `main.rs` +returns it when `Paths::resolve()` fails, which happens ahead of every +subcommand including `xy service`. No `xy service` code path returns `3` itself. ## Testing