From e8f869a16c90d35a964fb9ed10c0c8bbd6a490ca Mon Sep 17 00:00:00 2001 From: Nic-dorman Date: Wed, 21 Oct 2026 10:47:41 +0100 Subject: [PATCH 1/6] fix(core): don't mistake a stale daemon PID file for a running daemon (V2-1655) A daemon that dies without cleaning up (a reboot, an OOM kill, SIGKILL) leaves daemon.pid and daemon.port behind. Once the PID is reused by an unrelated process, `daemon info` and `daemon start` saw a live PID and treated it as the daemon: `info` reported running while `status` (which asks the daemon over HTTP) said not running, and `start` refused to start a new daemon, so the hint `status` prints led nowhere. `info` and `start` now classify the PID file through one helper that also requires the process to be identifiable as the daemon, the check `stop` already applied before signalling it. `start` removes stale files before spawning; otherwise its port poll would read the dead daemon's port straight back. macOS now checks the executable name via proc_pidpath instead of trusting any live PID. The `info` help text and README no longer claim its output is always JSON: it has been human-readable unless --json is passed since 920f872, which is the convention for every command. Adds unit tests for the reused-PID case and an e2e skill step (5.5). Closes V2-1655 Co-Authored-By: Claude Opus 5.5 --- .../skills/e2e-node-management-test/SKILL.md | 23 +++ CHANGELOG.md | 11 ++ README.md | 2 +- ant-cli/src/commands/node/daemon.rs | 2 +- ant-core/src/node/daemon/client.rs | 175 +++++++++++++++--- 5 files changed, 190 insertions(+), 23 deletions(-) diff --git a/.claude/skills/e2e-node-management-test/SKILL.md b/.claude/skills/e2e-node-management-test/SKILL.md index 2b479c60..28dc2e20 100644 --- a/.claude/skills/e2e-node-management-test/SKILL.md +++ b/.claude/skills/e2e-node-management-test/SKILL.md @@ -104,6 +104,28 @@ ant node daemon start --json Re-run `ant node daemon info --json` and update the saved `api_base` URL (the port will have changed). +**Step 5.5 — Stale PID file recovery:** + +A daemon that dies without cleaning up (a reboot, an OOM kill) leaves `daemon.pid` and +`daemon.port` behind, and its PID can be reused by an unrelated process. Simulate that by +recording the shell's own PID and a dead port as the daemon's. Run everything as **one** shell +command so the shell (the "unrelated process") is still alive while `ant` runs: + +```bash +# Linux: DATA_DIR="${XDG_DATA_HOME:-$HOME/.local/share}/ant" +# macOS: DATA_DIR="$HOME/Library/Application Support/ant" +# Windows: DATA_DIR="$APPDATA/ant", and write $(cat /proc/$$/winpid) instead of $$ +ant node daemon stop --json +echo $$ > "$DATA_DIR/daemon.pid"; echo 1 > "$DATA_DIR/daemon.port" +ant node daemon info --json; ant node daemon status --json; ant node daemon start --json +``` + +Verify: `info` and `status` both report `running: false`, and `start` reports +`already_running: false` with a `port` that is present and not `1`. A stale port file must not be +read back as the new daemon's port. + +Re-run `ant node daemon info --json` and update the saved `api_base` URL. + ### Phase 6: Node management **Step 6.1 — Add nodes:** @@ -516,6 +538,7 @@ Phase 5: Daemon Lifecycle [PASS] 5.2 Daemon status [PASS] 5.3 Daemon info [PASS] 5.4 Pinned-port flag round-trip + [PASS] 5.5 Stale PID file recovery Phase 6: Node Management [PASS] 6.1 Add 3 nodes diff --git a/CHANGELOG.md b/CHANGELOG.md index a876f85d..ea71cfff 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -59,6 +59,17 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 metadata describing a file that was no longer there, so the next `npm update` silently rolled the user back. +### Fixed +- `ant node daemon info` and `ant node daemon start` no longer mistake a stale PID file for a + running daemon (V2-1655). A daemon that dies without cleaning up (a reboot, an OOM kill) + leaves its PID and port files behind, and once the PID is reused by an unrelated process, + `info` reported the dead daemon as running while `status` said it was not, and `start` + refused to start a new one. Both now require the process to be identifiable as the daemon, + the check `stop` already applied, and `start` removes the stale files before starting a new + daemon. On macOS the check now reads the executable's name, where it previously trusted any + live PID. The `info` help text and README no longer claim its output is always JSON: it is + human-readable unless `--json` is passed, like every other command. + ## [0.3.6] - 2026-09-01 ### Fixed diff --git a/README.md b/README.md index 2ade88cd..4068f3f2 100644 --- a/README.md +++ b/README.md @@ -523,7 +523,7 @@ Daemon is running #### `ant node daemon info` -Output connection details as JSON (always JSON, regardless of `--json` flag). AI agents use this to discover the daemon's REST API. +Show connection details: PID, port, console URL and REST API base. AI agents use `ant node daemon info --json` to discover the daemon's REST API. ```json { diff --git a/ant-cli/src/commands/node/daemon.rs b/ant-cli/src/commands/node/daemon.rs index ed60df86..e572ff88 100644 --- a/ant-cli/src/commands/node/daemon.rs +++ b/ant-cli/src/commands/node/daemon.rs @@ -48,7 +48,7 @@ pub enum DaemonCommand { Stop, /// Show whether the daemon is running and summary stats Status, - /// Output connection details for programmatic use (always JSON) + /// Show connection details: PID, port, console and API URLs (add --json for scripts) Info, /// Run the daemon in the foreground (used internally) #[command(hide = true)] diff --git a/ant-core/src/node/daemon/client.rs b/ant-core/src/node/daemon/client.rs index c108e611..f441c557 100644 --- a/ant-core/src/node/daemon/client.rs +++ b/ant-core/src/node/daemon/client.rs @@ -106,8 +106,7 @@ pub async fn stop(config: &DaemonConfig) -> Result { /// If the daemon is already running, returns a result with `already_running: true`. /// Otherwise, spawns the daemon and polls for the port file to confirm startup. pub async fn start(config: &DaemonConfig) -> Result { - // Check if daemon is already running - if let Some(pid) = check_running(&config.pid_file_path) { + if let Some(pid) = running_daemon_or_clear_stale(config) { let port = read_port_file(&config.port_file_path); return Ok(DaemonStartResult { already_running: true, @@ -421,19 +420,15 @@ pub async fn stop_all_nodes(config: &DaemonConfig) -> Result { /// Get daemon connection info for programmatic use. /// -/// Reads PID and port files and checks if the process is alive. +/// Reads the PID and port files. `running` is true only if the recorded PID is a live daemon +/// process (see [`recorded_daemon`]); `pid` and `port` are reported as recorded either way. pub fn info(config: &DaemonConfig) -> DaemonInfo { - let pid = std::fs::read_to_string(&config.pid_file_path) - .ok() - .and_then(|s| s.trim().parse::().ok()); - + let recorded = recorded_daemon(&config.pid_file_path); let port = read_port_file(&config.port_file_path); - let running = pid.is_some_and(is_process_alive); - DaemonInfo { - running, - pid, + running: matches!(recorded, RecordedDaemon::Live(_)), + pid: recorded.pid(), port, api_base: port.map(|p| format!("http://127.0.0.1:{p}/api/v1")), } @@ -466,7 +461,7 @@ pub async fn run(config: DaemonConfig) -> Result<()> { /// Validate that a PID belongs to an ant daemon process by checking its /// command line. This guards against PID reuse after a daemon crash. -#[cfg(unix)] +#[cfg(all(unix, not(target_os = "macos")))] fn validate_daemon_process(pid: u32) -> bool { let cmdline_path = format!("/proc/{pid}/cmdline"); match std::fs::read(&cmdline_path) { @@ -489,14 +484,37 @@ fn validate_daemon_process(pid: u32) -> bool { exe_matches && has_daemon_arg } Err(_) => { - // On non-Linux Unix (macOS), /proc doesn't exist. Fall back to - // trusting the PID file since there's no cheap way to inspect - // the command line without shelling out. + // Without /proc there's no cheap way to inspect the command line + // without shelling out, so fall back to trusting the PID file. true } } } +/// Validate that a PID belongs to an ant daemon process by checking its +/// executable name. macOS has no /proc, and reading another process's +/// arguments needs `sysctl(KERN_PROCARGS2)` parsing, so this checks the +/// basename only, as the Windows version does. +#[cfg(target_os = "macos")] +fn validate_daemon_process(pid: u32) -> bool { + let Some(pid) = pid_to_i32(pid) else { + return false; + }; + let mut buf = [0u8; libc::PROC_PIDPATHINFO_MAXSIZE as usize]; + let len = unsafe { libc::proc_pidpath(pid, buf.as_mut_ptr().cast(), buf.len() as u32) }; + let Ok(len) = usize::try_from(len) else { + return false; + }; + if len == 0 { + return false; + } + let path = String::from_utf8_lossy(&buf[..len]); + std::path::Path::new(path.as_ref()) + .file_name() + .and_then(|name| name.to_str()) + .is_some_and(|name| name == "ant") +} + #[cfg(windows)] fn validate_daemon_process(pid: u32) -> bool { use windows_sys::Win32::Foundation::CloseHandle; @@ -565,13 +583,56 @@ fn read_pid_file(path: &Path) -> Result { .map_err(|_| Error::DaemonNotRunning) } -/// Check if a daemon is running. Returns the PID if so. -fn check_running(pid_file: &Path) -> Option { - let pid = read_pid_file(pid_file).ok()?; - if is_process_alive(pid) { - Some(pid) +/// What the PID file says about the daemon. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum RecordedDaemon { + /// No readable PID file. + Absent, + /// The recorded PID is a live daemon process. + Live(u32), + /// The daemon exited without removing its files (a reboot, an OOM kill, SIGKILL). The + /// recorded PID is now dead, or belongs to an unrelated process. + Stale(u32), +} + +impl RecordedDaemon { + fn pid(self) -> Option { + match self { + RecordedDaemon::Absent => None, + RecordedDaemon::Live(pid) | RecordedDaemon::Stale(pid) => Some(pid), + } + } +} + +/// Classify the daemon recorded in `pid_file`. +/// +/// A live PID is not enough on its own: the files outlive an unclean exit, and after a reboot the +/// PID is often reused by an unrelated process. The process must also be identifiable as the +/// daemon, the same test `stop` applies before signalling it. +fn recorded_daemon(pid_file: &Path) -> RecordedDaemon { + let Ok(pid) = read_pid_file(pid_file) else { + return RecordedDaemon::Absent; + }; + if is_process_alive(pid) && validate_daemon_process(pid) { + RecordedDaemon::Live(pid) } else { - None + RecordedDaemon::Stale(pid) + } +} + +/// The PID of the running daemon, if there is one. +/// +/// Otherwise removes any PID and port files a previous daemon left behind, so the next daemon +/// starts clean: the port poll in [`start`] would otherwise read the dead daemon's port straight +/// back. +fn running_daemon_or_clear_stale(config: &DaemonConfig) -> Option { + match recorded_daemon(&config.pid_file_path) { + RecordedDaemon::Live(pid) => Some(pid), + RecordedDaemon::Absent | RecordedDaemon::Stale(_) => { + let _ = std::fs::remove_file(&config.pid_file_path); + let _ = std::fs::remove_file(&config.port_file_path); + None + } } } @@ -734,4 +795,76 @@ mod tests { let args = daemon_run_args(&config); assert_eq!(args, vec!["node", "daemon", "run", "--port", "0"]); } + + fn config_in(dir: &Path) -> DaemonConfig { + DaemonConfig { + pid_file_path: dir.join("daemon.pid"), + port_file_path: dir.join("daemon.port"), + ..DaemonConfig::default() + } + } + + /// The files a daemon leaves behind when it dies without cleaning up, after its PID has been + /// reused: the test process stands in for the unrelated process now holding it. + fn write_files_with_reused_pid(config: &DaemonConfig) { + std::fs::write(&config.pid_file_path, std::process::id().to_string()).unwrap(); + std::fs::write(&config.port_file_path, "39007").unwrap(); + } + + #[test] + fn info_without_a_pid_file_is_not_running() { + let tmp = tempfile::tempdir().unwrap(); + let info = info(&config_in(tmp.path())); + assert!(!info.running); + assert_eq!(info.pid, None); + } + + // The fallback for other Unixes trusts any live PID, so these hold only where the process can + // be identified. + #[cfg(any(target_os = "linux", target_os = "macos", windows))] + #[test] + fn validate_rejects_a_live_process_that_is_not_the_daemon() { + assert!(!validate_daemon_process(std::process::id())); + } + + #[cfg(any(target_os = "linux", target_os = "macos", windows))] + #[test] + fn info_with_a_reused_pid_is_not_running() { + let tmp = tempfile::tempdir().unwrap(); + let config = config_in(tmp.path()); + write_files_with_reused_pid(&config); + + let info = info(&config); + assert!( + !info.running, + "a reused PID must not be reported as the daemon" + ); + assert_eq!(info.pid, Some(std::process::id())); + assert_eq!(info.port, Some(39007)); + } + + #[cfg(any(target_os = "linux", target_os = "macos", windows))] + #[test] + fn start_clears_files_left_by_a_dead_daemon() { + let tmp = tempfile::tempdir().unwrap(); + let config = config_in(tmp.path()); + write_files_with_reused_pid(&config); + + assert_eq!(running_daemon_or_clear_stale(&config), None); + assert!(!config.pid_file_path.exists()); + assert!( + !config.port_file_path.exists(), + "a stale port file would be read back as the new daemon's port" + ); + } + + #[test] + fn start_clears_a_port_file_without_a_pid_file() { + let tmp = tempfile::tempdir().unwrap(); + let config = config_in(tmp.path()); + std::fs::write(&config.port_file_path, "39007").unwrap(); + + assert_eq!(running_daemon_or_clear_stale(&config), None); + assert!(!config.port_file_path.exists()); + } } From da95adb5cd9a718f02547e078d1d5bf51bfd93b0 Mon Sep 17 00:00:00 2001 From: Nic-dorman Date: Wed, 21 Oct 2026 14:24:18 +0100 Subject: [PATCH 2/6] docs(core): don't link the public `info` docs to a private helper rustdoc's private_intra_doc_links lint fails the Documentation job under -D warnings. Describe the check inline instead of linking to `recorded_daemon`. Co-Authored-By: Claude Opus 5.5 --- ant-core/src/node/daemon/client.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/ant-core/src/node/daemon/client.rs b/ant-core/src/node/daemon/client.rs index f441c557..c2b523d6 100644 --- a/ant-core/src/node/daemon/client.rs +++ b/ant-core/src/node/daemon/client.rs @@ -420,8 +420,9 @@ pub async fn stop_all_nodes(config: &DaemonConfig) -> Result { /// Get daemon connection info for programmatic use. /// -/// Reads the PID and port files. `running` is true only if the recorded PID is a live daemon -/// process (see [`recorded_daemon`]); `pid` and `port` are reported as recorded either way. +/// Reads the PID and port files. `running` is true only if the recorded PID is a live process +/// identifiable as the daemon, since a PID left behind by a daemon that died may have been +/// reused; `pid` and `port` are reported as recorded either way. pub fn info(config: &DaemonConfig) -> DaemonInfo { let recorded = recorded_daemon(&config.pid_file_path); let port = read_port_file(&config.port_file_path); From dfc7c6193d851485d08d52165fa44a754414d062 Mon Sep 17 00:00:00 2001 From: Nic-dorman Date: Fri, 23 Oct 2026 22:00:21 +0100 Subject: [PATCH 3/6] fix(core): require `daemon run` in the daemon's arguments on macOS and Windows too Review of #235: the macOS and Windows checks accepted any live process whose executable is named `ant`. A stale PID reused by another `ant` command, such as a long `ant file upload`, was still reported as the running daemon, and `start` still refused to start. Each platform now requires the process to be `ant` running `daemon run`, the arguments `daemon start` passes to the detached child. macOS and Windows read the arguments through `sysinfo`, which ant-core already uses for node adoption. The executable-name checks stay as they were: they work for another user's process and for an elevated one. Where the arguments can't be read, `sysinfo` reports none, and the name check alone decides. Linux keeps its /proc reading but uses the same rule, which is stricter than the previous "any `daemon` argument". New tests copy the test binary as `ant` and run it as `node daemon run` (accepted), as `file upload` (rejected), and as `node daemon status` behind a reused PID (`info` reports it not running). Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 8 +- ant-core/src/node/daemon/client.rs | 187 +++++++++++++++++++++++++++-- 2 files changed, 181 insertions(+), 14 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ea71cfff..08e8b152 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -66,9 +66,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 `info` reported the dead daemon as running while `status` said it was not, and `start` refused to start a new one. Both now require the process to be identifiable as the daemon, the check `stop` already applied, and `start` removes the stale files before starting a new - daemon. On macOS the check now reads the executable's name, where it previously trusted any - live PID. The `info` help text and README no longer claim its output is always JSON: it is - human-readable unless `--json` is passed, like every other command. + daemon. On every platform the process must be `ant` running `daemon run`, so another `ant` + command holding the reused PID is not taken for the daemon either. Previously macOS trusted any + live PID and Windows checked only the executable's name. The `info` help text and README no + longer claim its output is always JSON: it is human-readable unless `--json` is passed, like + every other command. ## [0.3.6] - 2026-09-01 diff --git a/ant-core/src/node/daemon/client.rs b/ant-core/src/node/daemon/client.rs index c2b523d6..d009ffa5 100644 --- a/ant-core/src/node/daemon/client.rs +++ b/ant-core/src/node/daemon/client.rs @@ -468,9 +468,10 @@ fn validate_daemon_process(pid: u32) -> bool { match std::fs::read(&cmdline_path) { Ok(raw) => { // /proc/PID/cmdline uses null bytes as separators. - // Check that the executable basename ends with "ant" and one - // of the arguments is "daemon". This avoids false positives - // from processes like "rant" or "phantom-daemon". + // Check that the executable basename is exactly "ant" and the + // arguments run the daemon. This avoids false positives from + // processes like "rant" or "phantom-daemon", and from other `ant` + // commands that reuse the PID. let args: Vec = raw .split(|&b| b == 0) .filter(|s| !s.is_empty()) @@ -481,8 +482,7 @@ fn validate_daemon_process(pid: u32) -> bool { .and_then(|exe| std::path::Path::new(exe).file_name()) .and_then(|name| name.to_str()) .is_some_and(|name| name == "ant" || name == "ant.exe"); - let has_daemon_arg = args.iter().any(|a| a == "daemon"); - exe_matches && has_daemon_arg + exe_matches && runs_the_daemon(&args) } Err(_) => { // Without /proc there's no cheap way to inspect the command line @@ -492,12 +492,50 @@ fn validate_daemon_process(pid: u32) -> bool { } } -/// Validate that a PID belongs to an ant daemon process by checking its -/// executable name. macOS has no /proc, and reading another process's -/// arguments needs `sysctl(KERN_PROCARGS2)` parsing, so this checks the -/// basename only, as the Windows version does. -#[cfg(target_os = "macos")] +/// Whether a command line runs the daemon: `daemon run`, as [`daemon_run_args`] passes to the +/// detached child (and as a foreground `ant node daemon run` has). Other `ant` commands, including +/// `daemon start` and `daemon status`, do not match. +fn runs_the_daemon>(args: &[S]) -> bool { + args.windows(2) + .any(|pair| pair[0].as_ref() == "daemon" && pair[1].as_ref() == "run") +} + +/// Validate that a PID belongs to an ant daemon process: its executable is `ant` and its arguments +/// run the daemon. +#[cfg(any(target_os = "macos", windows))] fn validate_daemon_process(pid: u32) -> bool { + executable_is_ant(pid) && arguments_allow_daemon(pid) +} + +/// Whether the process's arguments are the daemon's. `sysinfo` reads them (`KERN_PROCARGS2` on +/// macOS, the process environment block on Windows). +/// +/// The arguments of a process owned by another user, or of an elevated process on Windows, can't +/// be read, and `sysinfo` then reports none. Every real process has at least `argv[0]`, so an +/// empty list means "unreadable", and the executable-name check alone decides. Calling a live +/// daemon stale would make `start` launch a second one. +#[cfg(any(target_os = "macos", windows))] +fn arguments_allow_daemon(pid: u32) -> bool { + use sysinfo::{Pid, ProcessRefreshKind, ProcessesToUpdate, System, UpdateKind}; + + let pid = Pid::from_u32(pid); + let mut system = System::new(); + system.refresh_processes_specifics( + ProcessesToUpdate::Some(&[pid]), + true, + ProcessRefreshKind::new().with_cmd(UpdateKind::Always), + ); + let Some(process) = system.process(pid) else { + return false; + }; + let args = process.cmd(); + args.is_empty() || runs_the_daemon(args) +} + +/// Whether the process's executable is named `ant`. `proc_pidpath` works for any user's process, +/// so this holds even where the arguments can't be read. +#[cfg(target_os = "macos")] +fn executable_is_ant(pid: u32) -> bool { let Some(pid) = pid_to_i32(pid) else { return false; }; @@ -516,8 +554,10 @@ fn validate_daemon_process(pid: u32) -> bool { .is_some_and(|name| name == "ant") } +/// Whether the process's executable is named `ant`. `PROCESS_QUERY_LIMITED_INFORMATION` is granted +/// even for elevated processes, so this holds even where the arguments can't be read. #[cfg(windows)] -fn validate_daemon_process(pid: u32) -> bool { +fn executable_is_ant(pid: u32) -> bool { use windows_sys::Win32::Foundation::CloseHandle; use windows_sys::Win32::System::Threading::{ OpenProcess, QueryFullProcessImageNameW, PROCESS_QUERY_LIMITED_INFORMATION, @@ -828,6 +868,131 @@ mod tests { assert!(!validate_daemon_process(std::process::id())); } + #[test] + fn only_daemon_run_is_the_daemon_command() { + assert!(runs_the_daemon(&["/usr/bin/ant", "node", "daemon", "run"])); + assert!(runs_the_daemon(&[ + "ant.exe", "node", "daemon", "run", "--port", "8765" + ])); + for args in [ + &["ant", "node", "daemon", "start"][..], + &["ant", "node", "daemon", "status"], + &["ant", "file", "upload", "daemon"], + &["ant", "run", "daemon"], + &["ant"], + &[], + ] { + assert!(!runs_the_daemon(args), "{args:?} is not the daemon"); + } + } + + /// Test-name filter that selects [`sleep_as_an_ant_named_process`] in a copy of this binary. + #[cfg(any(target_os = "linux", target_os = "macos", windows))] + const SLEEPER: &str = "node::daemon::client::tests::sleep_as_an_ant_named_process"; + #[cfg(any(target_os = "linux", target_os = "macos", windows))] + const SLEEP_ENV: &str = "ANT_CORE_TEST_SLEEP_AS_ANT"; + + /// The body of [`AntNamedProcess`], not a test: ignored, and a no-op unless its environment + /// variable is set, so an `--ignored` run returns at once. + #[cfg(any(target_os = "linux", target_os = "macos", windows))] + #[test] + #[ignore = "helper process for AntNamedProcess"] + fn sleep_as_an_ant_named_process() { + if std::env::var_os(SLEEP_ENV).is_some() { + std::thread::sleep(Duration::from_secs(120)); + } + } + + /// A live process whose executable is named `ant`, running `args`: a copy of this test binary + /// that sleeps until dropped. libtest reads `args` as more test-name filters, which with + /// `--exact` select no other test. + #[cfg(any(target_os = "linux", target_os = "macos", windows))] + struct AntNamedProcess { + child: std::process::Child, + _dir: tempfile::TempDir, + } + + #[cfg(any(target_os = "linux", target_os = "macos", windows))] + impl AntNamedProcess { + fn spawn(args: &[&str]) -> Self { + let dir = tempfile::tempdir().unwrap(); + let exe = dir + .path() + .join(if cfg!(windows) { "ant.exe" } else { "ant" }); + std::fs::copy(std::env::current_exe().unwrap(), &exe).unwrap(); + let child = std::process::Command::new(&exe) + .args(args) + .args(["--exact", "--ignored", SLEEPER]) + .env(SLEEP_ENV, "1") + .stdin(std::process::Stdio::null()) + .stdout(std::process::Stdio::null()) + .stderr(std::process::Stdio::null()) + .spawn() + .unwrap(); + Self { child, _dir: dir } + } + + fn pid(&self) -> u32 { + self.child.id() + } + + fn assert_still_running(&mut self) { + assert!( + self.child.try_wait().unwrap().is_none(), + "the ant-named helper process exited early" + ); + } + } + + #[cfg(any(target_os = "linux", target_os = "macos", windows))] + impl Drop for AntNamedProcess { + fn drop(&mut self) { + let _ = self.child.kill(); + let _ = self.child.wait(); + } + } + + #[cfg(any(target_os = "linux", target_os = "macos", windows))] + #[test] + fn validate_accepts_an_ant_process_running_the_daemon() { + let mut process = AntNamedProcess::spawn(&["node", "daemon", "run"]); + let pid = process.pid(); + // The child is visible once spawn returns; the retry only absorbs a slow process table. + let accepted = (0..50).any(|_| { + validate_daemon_process(pid) || { + std::thread::sleep(Duration::from_millis(100)); + false + } + }); + process.assert_still_running(); + assert!(accepted, "an `ant node daemon run` process is the daemon"); + } + + #[cfg(any(target_os = "linux", target_os = "macos", windows))] + #[test] + fn validate_rejects_another_ant_command_holding_the_pid() { + let mut process = AntNamedProcess::spawn(&["file", "upload", "photo.jpg"]); + assert!(!validate_daemon_process(process.pid())); + process.assert_still_running(); + } + + #[cfg(any(target_os = "linux", target_os = "macos", windows))] + #[test] + fn info_with_a_pid_reused_by_another_ant_command_is_not_running() { + let mut process = AntNamedProcess::spawn(&["node", "daemon", "status"]); + let tmp = tempfile::tempdir().unwrap(); + let config = config_in(tmp.path()); + std::fs::write(&config.pid_file_path, process.pid().to_string()).unwrap(); + + let info = info(&config); + process.assert_still_running(); + assert!( + !info.running, + "another `ant` command holding a reused PID must not be reported as the daemon" + ); + assert_eq!(info.pid, Some(process.pid())); + } + #[cfg(any(target_os = "linux", target_os = "macos", windows))] #[test] fn info_with_a_reused_pid_is_not_running() { From 01d5640ecf29c4cae2ba5c1520f5bc7282725621 Mon Sep 17 00:00:00 2001 From: Nic-dorman Date: Mon, 26 Oct 2026 09:38:01 +0000 Subject: [PATCH 4/6] fix(core): confirm the daemon by its exact command or its own API, and fail on stale-file cleanup errors Second review of #235: - `daemon run` anywhere in the arguments was accepted, so `ant file upload daemon run` holding a reused PID passed. Only `node daemon run` straight after the executable, as `start` spawns it, now confirms the daemon. - Unreadable arguments fell back to the executable name. They are now "unconfirmed", like any other `ant` command line, and such a process counts as the daemon only if the daemon's API on the recorded port reports its PID. That also keeps a foreground daemon started behind global options recognised, without copying the CLI's option list into ant-core. - Errors removing the stale PID and port files were ignored, so the port poll could read the dead daemon's port back. `start` now fails instead. `start` also spawns the daemon only from the `ant` binary, and never from a unit-test build. Any other harness would read `node daemon run` as test-name filters and re-run the tests that spawned it. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 13 +- ant-core/src/node/daemon/client.rs | 503 +++++++++++++++++++++++------ 2 files changed, 414 insertions(+), 102 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 08e8b152..f4fa3470 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -66,11 +66,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 `info` reported the dead daemon as running while `status` said it was not, and `start` refused to start a new one. Both now require the process to be identifiable as the daemon, the check `stop` already applied, and `start` removes the stale files before starting a new - daemon. On every platform the process must be `ant` running `daemon run`, so another `ant` - command holding the reused PID is not taken for the daemon either. Previously macOS trusted any - live PID and Windows checked only the executable's name. The `info` help text and README no - longer claim its output is always JSON: it is human-readable unless `--json` is passed, like - every other command. + daemon. On every platform the process must be `ant` running `ant node daemon run`, so another + `ant` command holding the reused PID is not taken for the daemon either. Where a process's + arguments can't be read (another user's, an elevated one on Windows) or don't show that, the + daemon's own API on the recorded port must report the PID. Previously macOS trusted any live + PID and Windows checked only the executable's name. `start` now fails instead of carrying on + when a stale file can't be removed, and it only spawns the daemon from the `ant` binary. The + `info` help text and README no longer claim its output is always JSON: it is human-readable + unless `--json` is passed, like every other command. ## [0.3.6] - 2026-09-01 diff --git a/ant-core/src/node/daemon/client.rs b/ant-core/src/node/daemon/client.rs index d009ffa5..b351a7f6 100644 --- a/ant-core/src/node/daemon/client.rs +++ b/ant-core/src/node/daemon/client.rs @@ -67,7 +67,7 @@ pub async fn stop(config: &DaemonConfig) -> Result { return Ok(DaemonStopResult { pid }); } - if !validate_daemon_process(pid) { + if !is_confirmed_daemon(config, pid) { // PID is alive but isn't our daemon — clean up stale files without killing let _ = std::fs::remove_file(&config.pid_file_path); let _ = std::fs::remove_file(&config.port_file_path); @@ -106,7 +106,7 @@ pub async fn stop(config: &DaemonConfig) -> Result { /// If the daemon is already running, returns a result with `already_running: true`. /// Otherwise, spawns the daemon and polls for the port file to confirm startup. pub async fn start(config: &DaemonConfig) -> Result { - if let Some(pid) = running_daemon_or_clear_stale(config) { + if let Some(pid) = running_daemon_or_clear_stale(config)? { let port = read_port_file(&config.port_file_path); return Ok(DaemonStartResult { already_running: true, @@ -115,9 +115,28 @@ pub async fn start(config: &DaemonConfig) -> Result { }); } + // In ant-core's own unit tests the current executable is the test harness. It would read + // `node daemon run` as test-name filters and re-run the tests that got here, each spawning + // again. A unit-test build never spawns it. + if cfg!(test) { + return Err(Error::ProcessSpawn( + "a unit-test build of ant-core does not spawn the daemon".to_string(), + )); + } + // Get the path to the current executable let exe = std::env::current_exe() .map_err(|e| Error::ProcessSpawn(format!("Failed to get current executable: {e}")))?; + // The daemon is recognised as an `ant` executable running `node daemon run` + // (`process_identity`), so only the `ant` binary can start one that `info` and `start` will + // recognise. This also covers test harnesses outside ant-core's unit tests, such as + // integration tests, which would read `node daemon run` as test-name filters. + if !is_ant_executable_name(&exe) { + return Err(Error::ProcessSpawn(format!( + "only the `ant` binary can start the daemon, not {}", + exe.display() + ))); + } let exe_str = exe .to_str() .ok_or_else(|| Error::ProcessSpawn("Executable path is not valid UTF-8".to_string()))?; @@ -421,10 +440,11 @@ pub async fn stop_all_nodes(config: &DaemonConfig) -> Result { /// Get daemon connection info for programmatic use. /// /// Reads the PID and port files. `running` is true only if the recorded PID is a live process -/// identifiable as the daemon, since a PID left behind by a daemon that died may have been -/// reused; `pid` and `port` are reported as recorded either way. +/// confirmed as the daemon, since a PID left behind by a daemon that died may have been reused. +/// The process confirms itself by running `ant node daemon run`; failing that, the daemon's API on +/// the recorded port must report the PID. `pid` and `port` are reported as recorded either way. pub fn info(config: &DaemonConfig) -> DaemonInfo { - let recorded = recorded_daemon(&config.pid_file_path); + let recorded = recorded_daemon(config); let port = read_port_file(&config.port_file_path); DaemonInfo { @@ -460,62 +480,84 @@ pub async fn run(config: DaemonConfig) -> Result<()> { Ok(()) } -/// Validate that a PID belongs to an ant daemon process by checking its -/// command line. This guards against PID reuse after a daemon crash. -#[cfg(all(unix, not(target_os = "macos")))] -fn validate_daemon_process(pid: u32) -> bool { - let cmdline_path = format!("/proc/{pid}/cmdline"); - match std::fs::read(&cmdline_path) { - Ok(raw) => { - // /proc/PID/cmdline uses null bytes as separators. - // Check that the executable basename is exactly "ant" and the - // arguments run the daemon. This avoids false positives from - // processes like "rant" or "phantom-daemon", and from other `ant` - // commands that reuse the PID. - let args: Vec = raw - .split(|&b| b == 0) - .filter(|s| !s.is_empty()) - .map(|s| String::from_utf8_lossy(s).to_string()) - .collect(); - let exe_matches = args - .first() - .and_then(|exe| std::path::Path::new(exe).file_name()) - .and_then(|name| name.to_str()) - .is_some_and(|name| name == "ant" || name == "ant.exe"); - exe_matches && runs_the_daemon(&args) - } - Err(_) => { - // Without /proc there's no cheap way to inspect the command line - // without shelling out, so fall back to trusting the PID file. - true - } +/// What a live process's own details show about whether it is the daemon. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum ProcessIdentity { + /// An `ant` executable running exactly the detached daemon's command line. + Daemon, + /// Not an `ant` executable, so not the daemon. + Other, + /// An `ant` executable whose arguments don't show that it is the daemon. They are another + /// command line, such as another `ant` command or a foreground daemon started behind global + /// options, or they can't be read: a process owned by another user, an elevated one on + /// Windows, or no `/proc`. [`is_confirmed_daemon`] asks the daemon's API instead. + Unconfirmed, +} + +/// Classify a process from whether its executable is `ant` and from its arguments (`argv[0]` +/// first), `None` when they can't be read. Unreadable arguments are never taken as proof. +fn identity_from>( + executable_is_ant: bool, + args: Option<&[S]>, +) -> ProcessIdentity { + if !executable_is_ant { + ProcessIdentity::Other + } else if args.is_some_and(is_daemon_command) { + ProcessIdentity::Daemon + } else { + ProcessIdentity::Unconfirmed } } -/// Whether a command line runs the daemon: `daemon run`, as [`daemon_run_args`] passes to the -/// detached child (and as a foreground `ant node daemon run` has). Other `ant` commands, including -/// `daemon start` and `daemon status`, do not match. -fn runs_the_daemon>(args: &[S]) -> bool { - args.windows(2) - .any(|pair| pair[0].as_ref() == "daemon" && pair[1].as_ref() == "run") +/// Whether `args` (`argv[0]` first) are the detached daemon's command line: `node daemon run` +/// straight after the executable, as [`start`] spawns it with [`daemon_run_args`]. `daemon run` +/// anywhere else, as in `ant file upload daemon run`, is not proof. +fn is_daemon_command>(args: &[S]) -> bool { + matches!( + args.get(1..4), + Some([node, daemon, run]) + if node.as_ref() == "node" && daemon.as_ref() == "daemon" && run.as_ref() == "run" + ) +} + +/// Identify a process from `/proc/PID/cmdline`, which holds its NUL-separated arguments. +#[cfg(all(unix, not(target_os = "macos")))] +fn process_identity(pid: u32) -> ProcessIdentity { + let Ok(raw) = std::fs::read(format!("/proc/{pid}/cmdline")) else { + // No /proc (other Unixes) or a hidepid mount: nothing to go on. + return ProcessIdentity::Unconfirmed; + }; + let args: Vec = raw + .split(|&b| b == 0) + .filter(|s| !s.is_empty()) + .map(|s| String::from_utf8_lossy(s).into_owned()) + .collect(); + let executable_is_ant = args + .first() + .is_some_and(|exe| is_ant_executable_name(Path::new(exe))); + identity_from(executable_is_ant, Some(args.as_slice())) } -/// Validate that a PID belongs to an ant daemon process: its executable is `ant` and its arguments -/// run the daemon. +/// Whether an executable path names the `ant` binary. The basename must be exactly `ant` or +/// `ant.exe`, which rules out processes like `rant`. +fn is_ant_executable_name(exe: &Path) -> bool { + exe.file_name() + .and_then(|name| name.to_str()) + .is_some_and(|name| name == "ant" || name == "ant.exe") +} + +/// Identify a process from its executable name and its arguments. #[cfg(any(target_os = "macos", windows))] -fn validate_daemon_process(pid: u32) -> bool { - executable_is_ant(pid) && arguments_allow_daemon(pid) +fn process_identity(pid: u32) -> ProcessIdentity { + identity_from(executable_is_ant(pid), process_args(pid).as_deref()) } -/// Whether the process's arguments are the daemon's. `sysinfo` reads them (`KERN_PROCARGS2` on -/// macOS, the process environment block on Windows). -/// -/// The arguments of a process owned by another user, or of an elevated process on Windows, can't -/// be read, and `sysinfo` then reports none. Every real process has at least `argv[0]`, so an -/// empty list means "unreadable", and the executable-name check alone decides. Calling a live -/// daemon stale would make `start` launch a second one. +/// The process's arguments, read through `sysinfo` (`KERN_PROCARGS2` on macOS, the process +/// environment block on Windows), or `None` if they can't be read. `sysinfo` reports none for a +/// process owned by another user, or for an elevated one on Windows; every real process has at +/// least `argv[0]`. #[cfg(any(target_os = "macos", windows))] -fn arguments_allow_daemon(pid: u32) -> bool { +fn process_args(pid: u32) -> Option> { use sysinfo::{Pid, ProcessRefreshKind, ProcessesToUpdate, System, UpdateKind}; let pid = Pid::from_u32(pid); @@ -525,15 +567,66 @@ fn arguments_allow_daemon(pid: u32) -> bool { true, ProcessRefreshKind::new().with_cmd(UpdateKind::Always), ); - let Some(process) = system.process(pid) else { + let args = system.process(pid)?.cmd(); + (!args.is_empty()).then(|| args.to_vec()) +} + +/// Whether the live process `pid` is the daemon that `config`'s files record. +/// +/// The process's own details settle it when they can (see [`ProcessIdentity`]). An +/// [`Unconfirmed`](ProcessIdentity::Unconfirmed) `ant` process counts only if the daemon's API, on +/// the recorded port, reports that PID. +fn is_confirmed_daemon(config: &DaemonConfig, pid: u32) -> bool { + match process_identity(pid) { + ProcessIdentity::Daemon => true, + ProcessIdentity::Other => false, + ProcessIdentity::Unconfirmed => daemon_api_reports_pid(&config.port_file_path, pid), + } +} + +/// Whether the daemon API on the port recorded in `port_file` reports `pid` as its PID +/// (`GET /api/v1/status`). +/// +/// A dead daemon's port has nothing listening on it, or something that isn't the daemon. This is a +/// blocking local request with one-second timeouts, made only for an unconfirmed process. +fn daemon_api_reports_pid(port_file: &Path, pid: u32) -> bool { + use std::io::{Read, Write}; + + let Some(port) = read_port_file(port_file) else { + return false; + }; + let timeout = Duration::from_secs(1); + let addr = std::net::SocketAddr::from((std::net::Ipv4Addr::LOCALHOST, port)); + let Ok(mut stream) = std::net::TcpStream::connect_timeout(&addr, timeout) else { + return false; + }; + let request = format!( + "GET /api/v1/status HTTP/1.1\r\nHost: 127.0.0.1:{port}\r\nConnection: close\r\n\r\n" + ); + let mut response = Vec::new(); + stream.set_read_timeout(Some(timeout)).is_ok() + && stream.set_write_timeout(Some(timeout)).is_ok() + && stream.write_all(request.as_bytes()).is_ok() + && stream.take(64 * 1024).read_to_end(&mut response).is_ok() + && status_response_reports_pid(&response, pid) +} + +/// Whether a raw HTTP response is a `200` whose [`DaemonStatus`] body reports `pid`. +fn status_response_reports_pid(response: &[u8], pid: u32) -> bool { + let Some(end_of_head) = response.windows(4).position(|w| w == b"\r\n\r\n") else { return false; }; - let args = process.cmd(); - args.is_empty() || runs_the_daemon(args) + let status_code = std::str::from_utf8(&response[..end_of_head]) + .ok() + .and_then(|head| head.lines().next()) + .and_then(|status_line| status_line.split_whitespace().nth(1)); + status_code == Some("200") + && serde_json::from_slice::(&response[end_of_head + 4..]) + .is_ok_and(|status| status.pid == Some(pid)) } /// Whether the process's executable is named `ant`. `proc_pidpath` works for any user's process, -/// so this holds even where the arguments can't be read. +/// even where the arguments can't be read. #[cfg(target_os = "macos")] fn executable_is_ant(pid: u32) -> bool { let Some(pid) = pid_to_i32(pid) else { @@ -555,7 +648,7 @@ fn executable_is_ant(pid: u32) -> bool { } /// Whether the process's executable is named `ant`. `PROCESS_QUERY_LIMITED_INFORMATION` is granted -/// even for elevated processes, so this holds even where the arguments can't be read. +/// even for an elevated process, whose arguments can't be read. #[cfg(windows)] fn executable_is_ant(pid: u32) -> bool { use windows_sys::Win32::Foundation::CloseHandle; @@ -645,16 +738,16 @@ impl RecordedDaemon { } } -/// Classify the daemon recorded in `pid_file`. +/// Classify the daemon recorded in `config`'s PID file. /// /// A live PID is not enough on its own: the files outlive an unclean exit, and after a reboot the -/// PID is often reused by an unrelated process. The process must also be identifiable as the -/// daemon, the same test `stop` applies before signalling it. -fn recorded_daemon(pid_file: &Path) -> RecordedDaemon { - let Ok(pid) = read_pid_file(pid_file) else { +/// PID is often reused by an unrelated process. The process must also be confirmed as the daemon +/// ([`is_confirmed_daemon`]), the same test `stop` applies before signalling it. +fn recorded_daemon(config: &DaemonConfig) -> RecordedDaemon { + let Ok(pid) = read_pid_file(&config.pid_file_path) else { return RecordedDaemon::Absent; }; - if is_process_alive(pid) && validate_daemon_process(pid) { + if is_process_alive(pid) && is_confirmed_daemon(config, pid) { RecordedDaemon::Live(pid) } else { RecordedDaemon::Stale(pid) @@ -665,18 +758,29 @@ fn recorded_daemon(pid_file: &Path) -> RecordedDaemon { /// /// Otherwise removes any PID and port files a previous daemon left behind, so the next daemon /// starts clean: the port poll in [`start`] would otherwise read the dead daemon's port straight -/// back. -fn running_daemon_or_clear_stale(config: &DaemonConfig) -> Option { - match recorded_daemon(&config.pid_file_path) { - RecordedDaemon::Live(pid) => Some(pid), +/// back. That is also why a file that can't be removed is an error rather than ignored. +fn running_daemon_or_clear_stale(config: &DaemonConfig) -> Result> { + match recorded_daemon(config) { + RecordedDaemon::Live(pid) => Ok(Some(pid)), RecordedDaemon::Absent | RecordedDaemon::Stale(_) => { - let _ = std::fs::remove_file(&config.pid_file_path); - let _ = std::fs::remove_file(&config.port_file_path); - None + remove_stale_file(&config.pid_file_path)?; + remove_stale_file(&config.port_file_path)?; + Ok(None) } } } +/// Remove a file a previous daemon left behind. A file that is already gone is fine. +fn remove_stale_file(path: &Path) -> Result<()> { + match std::fs::remove_file(path) { + Err(e) if e.kind() != std::io::ErrorKind::NotFound => Err(Error::ProcessSpawn(format!( + "could not remove the stale daemon file {}: {e}", + path.display() + ))), + _ => Ok(()), + } +} + #[cfg(unix)] fn pid_to_i32(pid: u32) -> Option { i32::try_from(pid).ok().filter(|&p| p > 0) @@ -860,32 +964,158 @@ mod tests { assert_eq!(info.pid, None); } - // The fallback for other Unixes trusts any live PID, so these hold only where the process can - // be identified. + // Other Unixes have no /proc to read, so there every process is unconfirmed. #[cfg(any(target_os = "linux", target_os = "macos", windows))] #[test] - fn validate_rejects_a_live_process_that_is_not_the_daemon() { - assert!(!validate_daemon_process(std::process::id())); + fn a_process_that_is_not_ant_is_not_the_daemon() { + assert_eq!(process_identity(std::process::id()), ProcessIdentity::Other); } #[test] - fn only_daemon_run_is_the_daemon_command() { - assert!(runs_the_daemon(&["/usr/bin/ant", "node", "daemon", "run"])); - assert!(runs_the_daemon(&[ + fn only_node_daemon_run_after_the_executable_is_the_daemon_command() { + assert!(is_daemon_command(&[ + "/usr/bin/ant", + "node", + "daemon", + "run" + ])); + assert!(is_daemon_command(&[ "ant.exe", "node", "daemon", "run", "--port", "8765" ])); for args in [ - &["ant", "node", "daemon", "start"][..], + // The review's example: `daemon run` as arguments to another command. + &["ant", "file", "upload", "daemon", "run"][..], + // A foreground daemon behind a global option: left to the daemon's API. + &["ant", "--json", "node", "daemon", "run"], + &["ant", "node", "daemon", "start"], &["ant", "node", "daemon", "status"], - &["ant", "file", "upload", "daemon"], - &["ant", "run", "daemon"], + &["ant", "node", "daemon"], &["ant"], &[], ] { - assert!(!runs_the_daemon(args), "{args:?} is not the daemon"); + assert!(!is_daemon_command(args), "{args:?} is not proof"); } } + /// `start` refuses to spawn from any other executable, which is what keeps a test harness + /// from spawning itself as `node daemon run`. + #[test] + fn only_the_ant_binary_is_an_ant_executable() { + // Forward slashes, which every platform's `Path` splits on. + for exe in ["/usr/local/bin/ant", "ant", "ant.exe", "Autonomi/ant.exe"] { + assert!(is_ant_executable_name(Path::new(exe)), "{exe}"); + } + for exe in [ + "target/debug/deps/ant_core-ea7d791ec7076b25", + "target/debug/deps/ant_core-ea7d791ec7076b25.exe", + "/usr/bin/rant", + "/usr/bin/ant-node", + "", + ] { + assert!(!is_ant_executable_name(Path::new(exe)), "{exe}"); + } + assert!( + !is_ant_executable_name(&std::env::current_exe().unwrap()), + "this test binary must never pass for `ant`" + ); + } + + #[test] + fn unreadable_arguments_never_confirm_the_daemon() { + let daemon = Some(&["ant", "node", "daemon", "run"][..]); + let unreadable: Option<&[&str]> = None; + assert_eq!(identity_from(true, daemon), ProcessIdentity::Daemon); + assert_eq!( + identity_from(true, unreadable), + ProcessIdentity::Unconfirmed + ); + assert_eq!( + identity_from(true, Some(&["ant", "file", "upload", "daemon", "run"][..])), + ProcessIdentity::Unconfirmed + ); + assert_eq!(identity_from(false, daemon), ProcessIdentity::Other); + assert_eq!(identity_from(false, unreadable), ProcessIdentity::Other); + } + + /// A one-shot stand-in for the daemon's API: answers a single request with a `DaemonStatus` + /// reporting `pid`. + fn serve_status_once(pid: u32) -> (u16, std::thread::JoinHandle<()>) { + use std::io::{Read, Write}; + + let listener = std::net::TcpListener::bind((Ipv4Addr::LOCALHOST, 0)).unwrap(); + let port = listener.local_addr().unwrap().port(); + let server = std::thread::spawn(move || { + let (mut stream, _) = listener.accept().unwrap(); + let mut request = [0u8; 1024]; + let _ = stream.read(&mut request); + let body = serde_json::to_string(&DaemonStatus { + running: true, + pid: Some(pid), + port: Some(port), + uptime_secs: Some(1), + nodes_total: 0, + nodes_running: 0, + nodes_stopped: 0, + nodes_errored: 0, + }) + .unwrap(); + write!( + stream, + "HTTP/1.1 200 OK\r\ncontent-type: application/json\r\n\ + content-length: {}\r\nconnection: close\r\n\r\n{body}", + body.len() + ) + .unwrap(); + }); + (port, server) + } + + /// A port with nothing listening on it, as a dead daemon's port usually is. + fn unused_port() -> u16 { + std::net::TcpListener::bind((Ipv4Addr::LOCALHOST, 0)) + .unwrap() + .local_addr() + .unwrap() + .port() + } + + #[test] + fn the_daemon_api_must_report_the_same_pid() { + let tmp = tempfile::tempdir().unwrap(); + let port_file = tmp.path().join("daemon.port"); + assert!(!daemon_api_reports_pid(&port_file, 42), "no port file"); + + std::fs::write(&port_file, unused_port().to_string()).unwrap(); + assert!(!daemon_api_reports_pid(&port_file, 42), "nothing listening"); + + let (port, server) = serve_status_once(42); + std::fs::write(&port_file, port.to_string()).unwrap(); + assert!(daemon_api_reports_pid(&port_file, 42)); + server.join().unwrap(); + + let (port, server) = serve_status_once(43); + std::fs::write(&port_file, port.to_string()).unwrap(); + assert!( + !daemon_api_reports_pid(&port_file, 42), + "another daemon's PID" + ); + server.join().unwrap(); + } + + #[test] + fn only_a_200_with_the_pid_confirms_the_daemon() { + let ok = |body: &str| format!("HTTP/1.1 200 OK\r\n\r\n{body}"); + let status = r#"{"running":true,"pid":42,"port":1,"uptime_secs":1,"nodes_total":0,"nodes_running":0,"nodes_stopped":0,"nodes_errored":0}"#; + assert!(status_response_reports_pid(ok(status).as_bytes(), 42)); + assert!(!status_response_reports_pid(ok(status).as_bytes(), 7)); + assert!(!status_response_reports_pid( + format!("HTTP/1.1 404 Not Found\r\n\r\n{status}").as_bytes(), + 42 + )); + assert!(!status_response_reports_pid(ok("not json").as_bytes(), 42)); + assert!(!status_response_reports_pid(b"garbage", 42)); + } + /// Test-name filter that selects [`sleep_as_an_ant_named_process`] in a copy of this binary. #[cfg(any(target_os = "linux", target_os = "macos", windows))] const SLEEPER: &str = "node::daemon::client::tests::sleep_as_an_ant_named_process"; @@ -920,16 +1150,41 @@ mod tests { .path() .join(if cfg!(windows) { "ant.exe" } else { "ant" }); std::fs::copy(std::env::current_exe().unwrap(), &exe).unwrap(); - let child = std::process::Command::new(&exe) + let mut command = std::process::Command::new(&exe); + command .args(args) .args(["--exact", "--ignored", SLEEPER]) .env(SLEEP_ENV, "1") .stdin(std::process::Stdio::null()) .stdout(std::process::Stdio::null()) - .stderr(std::process::Stdio::null()) - .spawn() - .unwrap(); - Self { child, _dir: dir } + .stderr(std::process::Stdio::null()); + // On Linux, executing a file that was just written fails with ETXTBSY while a process + // forked by another test thread still holds the copy's write descriptor. That clears + // once the other child execs, so retry briefly. + let mut attempts = 0; + let child = loop { + match command.spawn() { + Err(e) + if e.kind() == std::io::ErrorKind::ExecutableFileBusy && attempts < 50 => + { + attempts += 1; + std::thread::sleep(Duration::from_millis(20)); + } + result => break result.unwrap(), + } + }; + let process = Self { child, _dir: dir }; + // `spawn` returns once the exec has begun, but on Linux /proc only shows the new + // arguments a moment later: until then the process reads as `Other`. Hand tests a + // process that already shows as `ant`. + let shows_as_ant = (0..250).any(|_| { + process_identity(process.pid()) != ProcessIdentity::Other || { + std::thread::sleep(Duration::from_millis(20)); + false + } + }); + assert!(shows_as_ant, "the ant-named helper never showed as `ant`"); + process } fn pid(&self) -> u32 { @@ -954,25 +1209,58 @@ mod tests { #[cfg(any(target_os = "linux", target_os = "macos", windows))] #[test] - fn validate_accepts_an_ant_process_running_the_daemon() { + fn an_ant_process_running_node_daemon_run_is_the_daemon() { let mut process = AntNamedProcess::spawn(&["node", "daemon", "run"]); let pid = process.pid(); - // The child is visible once spawn returns; the retry only absorbs a slow process table. - let accepted = (0..50).any(|_| { - validate_daemon_process(pid) || { - std::thread::sleep(Duration::from_millis(100)); + // The arguments can still be unreadable for a moment after the exec, which reads as + // `Unconfirmed`; the retry only absorbs that. + let identified = (0..250).any(|_| { + process_identity(pid) == ProcessIdentity::Daemon || { + std::thread::sleep(Duration::from_millis(20)); false } }); process.assert_still_running(); - assert!(accepted, "an `ant node daemon run` process is the daemon"); + assert!(identified, "an `ant node daemon run` process is the daemon"); + } + + #[cfg(any(target_os = "linux", target_os = "macos", windows))] + #[test] + fn another_ant_command_holding_the_pid_is_not_the_daemon() { + // `daemon run` as another command's arguments, the review's example. + let mut process = AntNamedProcess::spawn(&["file", "upload", "daemon", "run"]); + let pid = process.pid(); + let tmp = tempfile::tempdir().unwrap(); + let config = config_in(tmp.path()); + std::fs::write(&config.port_file_path, unused_port().to_string()).unwrap(); + + assert_eq!(process_identity(pid), ProcessIdentity::Unconfirmed); + assert!(!is_confirmed_daemon(&config, pid)); + process.assert_still_running(); } #[cfg(any(target_os = "linux", target_os = "macos", windows))] #[test] - fn validate_rejects_another_ant_command_holding_the_pid() { - let mut process = AntNamedProcess::spawn(&["file", "upload", "photo.jpg"]); - assert!(!validate_daemon_process(process.pid())); + fn an_unconfirmed_ant_process_is_the_daemon_only_if_its_api_says_so() { + let mut process = AntNamedProcess::spawn(&["file", "upload", "daemon", "run"]); + let pid = process.pid(); + let tmp = tempfile::tempdir().unwrap(); + let config = config_in(tmp.path()); + std::fs::write(&config.pid_file_path, pid.to_string()).unwrap(); + + std::fs::write(&config.port_file_path, unused_port().to_string()).unwrap(); + assert!( + !info(&config).running, + "nothing answers on the recorded port" + ); + + let (port, server) = serve_status_once(pid); + std::fs::write(&config.port_file_path, port.to_string()).unwrap(); + assert!( + info(&config).running, + "the daemon's own API reports this PID" + ); + server.join().unwrap(); process.assert_still_running(); } @@ -1016,7 +1304,7 @@ mod tests { let config = config_in(tmp.path()); write_files_with_reused_pid(&config); - assert_eq!(running_daemon_or_clear_stale(&config), None); + assert_eq!(running_daemon_or_clear_stale(&config).unwrap(), None); assert!(!config.pid_file_path.exists()); assert!( !config.port_file_path.exists(), @@ -1030,7 +1318,28 @@ mod tests { let config = config_in(tmp.path()); std::fs::write(&config.port_file_path, "39007").unwrap(); - assert_eq!(running_daemon_or_clear_stale(&config), None); + assert_eq!(running_daemon_or_clear_stale(&config).unwrap(), None); assert!(!config.port_file_path.exists()); } + + /// A stale port file that can't be removed: a directory in its place makes `remove_file` fail + /// on every platform. + fn config_with_an_unremovable_port_file(dir: &Path) -> DaemonConfig { + let config = config_in(dir); + std::fs::create_dir(&config.port_file_path).unwrap(); + config + } + + #[test] + fn a_stale_file_that_cannot_be_removed_is_an_error() { + let tmp = tempfile::tempdir().unwrap(); + let config = config_with_an_unremovable_port_file(tmp.path()); + + let err = running_daemon_or_clear_stale(&config).unwrap_err(); + assert!( + err.to_string() + .contains("could not remove the stale daemon file"), + "{err}" + ); + } } From 7695a7006a10c66e11a058b96b4515c0062e0e0c Mon Sep 17 00:00:00 2001 From: Nic-dorman Date: Mon, 26 Oct 2026 15:13:37 +0000 Subject: [PATCH 5/6] fix(core): leave a daemon that can't be verified alone, look for its API on IPv6 too, and match `ANT.EXE` Third review of #235: - An unanswered daemon API counted as "not the daemon", so a live daemon whose API was briefly slow could lose its files to `start` or `stop`, and a second daemon could start. Confirmation now has three outcomes. Only an API answering with another PID rules the process out. With no answer, `start` and `stop` return an actionable error and remove or signal nothing, and `info` reports it as not running. - The API was only looked for on 127.0.0.1. It is now tried on the configured listen address (unspecified mapped to the loopback of its family), then on 127.0.0.1 and ::1. - On Windows the `ant` name check was case-sensitive, so `ANT.EXE` was rejected. It is now case-insensitive there, for `start` and for process identification. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 12 +- ant-core/src/node/daemon/client.rs | 437 ++++++++++++++++++++++------- 2 files changed, 348 insertions(+), 101 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f4fa3470..a2419ede 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -69,11 +69,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 daemon. On every platform the process must be `ant` running `ant node daemon run`, so another `ant` command holding the reused PID is not taken for the daemon either. Where a process's arguments can't be read (another user's, an elevated one on Windows) or don't show that, the - daemon's own API on the recorded port must report the PID. Previously macOS trusted any live - PID and Windows checked only the executable's name. `start` now fails instead of carrying on - when a stale file can't be removed, and it only spawns the daemon from the `ant` binary. The - `info` help text and README no longer claim its output is always JSON: it is human-readable - unless `--json` is passed, like every other command. + daemon's own API on the recorded port decides, on the configured listen address or either + loopback (IPv4 or IPv6). If that API doesn't answer, `start` and `stop` say they can't tell and + leave the files alone, rather than risk orphaning a live daemon. Previously macOS trusted any + live PID and Windows checked only the executable's name, case-sensitively. `start` now fails + instead of carrying on when a stale file can't be removed, and it only spawns the daemon from + the `ant` binary. The `info` help text and README no longer claim its output is always JSON: + it is human-readable unless `--json` is passed, like every other command. ## [0.3.6] - 2026-09-01 diff --git a/ant-core/src/node/daemon/client.rs b/ant-core/src/node/daemon/client.rs index b351a7f6..fabcb835 100644 --- a/ant-core/src/node/daemon/client.rs +++ b/ant-core/src/node/daemon/client.rs @@ -67,14 +67,28 @@ pub async fn stop(config: &DaemonConfig) -> Result { return Ok(DaemonStopResult { pid }); } - if !is_confirmed_daemon(config, pid) { - // PID is alive but isn't our daemon — clean up stale files without killing - let _ = std::fs::remove_file(&config.pid_file_path); - let _ = std::fs::remove_file(&config.port_file_path); - return Err(Error::DaemonStopFailed(format!( - "PID {pid} is alive but does not appear to be the ant daemon (possible PID reuse). \ - Stale PID file removed." - ))); + match confirm_daemon(config, pid) { + Confirmation::Daemon => {} + Confirmation::NotDaemon => { + // PID is alive but isn't our daemon — clean up stale files without killing + let _ = std::fs::remove_file(&config.pid_file_path); + let _ = std::fs::remove_file(&config.port_file_path); + return Err(Error::DaemonStopFailed(format!( + "PID {pid} is alive but does not appear to be the ant daemon (possible PID \ + reuse). Stale PID file removed." + ))); + } + // Neither signal a process that may not be the daemon, nor delete the files of one that + // may be. + Confirmation::Inconclusive(reason) => { + return Err(Error::DaemonStopFailed(format!( + "cannot tell whether PID {pid} is the ant daemon: {reason}. Nothing was \ + signalled or removed. If it is the daemon, stop that process directly; if not, \ + delete {} and {}", + config.pid_file_path.display(), + config.port_file_path.display() + ))); + } } send_terminate(pid); @@ -442,7 +456,9 @@ pub async fn stop_all_nodes(config: &DaemonConfig) -> Result { /// Reads the PID and port files. `running` is true only if the recorded PID is a live process /// confirmed as the daemon, since a PID left behind by a daemon that died may have been reused. /// The process confirms itself by running `ant node daemon run`; failing that, the daemon's API on -/// the recorded port must report the PID. `pid` and `port` are reported as recorded either way. +/// the recorded port must report the PID. A process that can't be confirmed either way is not +/// reported as running, but nothing is changed. `pid` and `port` are reported as recorded either +/// way. pub fn info(config: &DaemonConfig) -> DaemonInfo { let recorded = recorded_daemon(config); let port = read_port_file(&config.port_file_path); @@ -490,7 +506,7 @@ enum ProcessIdentity { /// An `ant` executable whose arguments don't show that it is the daemon. They are another /// command line, such as another `ant` command or a foreground daemon started behind global /// options, or they can't be read: a process owned by another user, an elevated one on - /// Windows, or no `/proc`. [`is_confirmed_daemon`] asks the daemon's API instead. + /// Windows, or no `/proc`. [`confirm_daemon`] asks the daemon's API instead. Unconfirmed, } @@ -538,12 +554,19 @@ fn process_identity(pid: u32) -> ProcessIdentity { identity_from(executable_is_ant, Some(args.as_slice())) } -/// Whether an executable path names the `ant` binary. The basename must be exactly `ant` or -/// `ant.exe`, which rules out processes like `rant`. +/// Whether an executable path names the `ant` binary. The basename must be `ant` or `ant.exe`, +/// which rules out processes like `rant`. Windows file names are case-insensitive, so there +/// `ANT.EXE` and `Ant.exe` count too. fn is_ant_executable_name(exe: &Path) -> bool { exe.file_name() .and_then(|name| name.to_str()) - .is_some_and(|name| name == "ant" || name == "ant.exe") + .is_some_and(|name| { + if cfg!(windows) { + name.eq_ignore_ascii_case("ant") || name.eq_ignore_ascii_case("ant.exe") + } else { + name == "ant" || name == "ant.exe" + } + }) } /// Identify a process from its executable name and its arguments. @@ -571,58 +594,121 @@ fn process_args(pid: u32) -> Option> { (!args.is_empty()).then(|| args.to_vec()) } +/// What can be said about whether a live process is the daemon that `config`'s files record. +#[derive(Debug, Clone, PartialEq, Eq)] +enum Confirmation { + /// It is the daemon. + Daemon, + /// It is not: its executable isn't `ant`, or the daemon's API answers with another PID. + NotDaemon, + /// It can't be told either way: an `ant` process whose arguments don't show it is the daemon, + /// and no daemon API answered. The text says why. Nothing may be deleted or signalled on this + /// basis, since a live daemon whose API was briefly slow would lose its files. + Inconclusive(String), +} + /// Whether the live process `pid` is the daemon that `config`'s files record. /// -/// The process's own details settle it when they can (see [`ProcessIdentity`]). An -/// [`Unconfirmed`](ProcessIdentity::Unconfirmed) `ant` process counts only if the daemon's API, on -/// the recorded port, reports that PID. -fn is_confirmed_daemon(config: &DaemonConfig, pid: u32) -> bool { +/// The process's own details settle it when they can (see [`ProcessIdentity`]). For an +/// [`Unconfirmed`](ProcessIdentity::Unconfirmed) `ant` process, the daemon's API on the recorded +/// port decides: our PID is the daemon, another PID is not, and no answer is inconclusive. +fn confirm_daemon(config: &DaemonConfig, pid: u32) -> Confirmation { match process_identity(pid) { - ProcessIdentity::Daemon => true, - ProcessIdentity::Other => false, - ProcessIdentity::Unconfirmed => daemon_api_reports_pid(&config.port_file_path, pid), + ProcessIdentity::Daemon => Confirmation::Daemon, + ProcessIdentity::Other => Confirmation::NotDaemon, + ProcessIdentity::Unconfirmed => match daemon_api_pid(config) { + Ok(api_pid) if api_pid == pid => Confirmation::Daemon, + Ok(_) => Confirmation::NotDaemon, + Err(reason) => Confirmation::Inconclusive(format!( + "it is an `ant` process whose arguments don't show it is the daemon, and {reason}" + )), + }, } } -/// Whether the daemon API on the port recorded in `port_file` reports `pid` as its PID -/// (`GET /api/v1/status`). +/// The PID the daemon's API reports (`GET /api/v1/status`) on the port in `config`'s port file, +/// or why no API answered. /// -/// A dead daemon's port has nothing listening on it, or something that isn't the daemon. This is a -/// blocking local request with one-second timeouts, made only for an unconfirmed process. -fn daemon_api_reports_pid(port_file: &Path, pid: u32) -> bool { - use std::io::{Read, Write}; +/// Tries the address the daemon was configured to listen on, then both loopbacks, since the +/// daemon may have been started with another `--listen-addr` than this call's config. These are +/// blocking local requests with one-second timeouts, made only for an unconfirmed process. +fn daemon_api_pid(config: &DaemonConfig) -> std::result::Result { + let Some(port) = read_port_file(&config.port_file_path) else { + return Err(format!( + "{} holds no port to ask its API on", + config.port_file_path.display() + )); + }; + let mut failures = Vec::new(); + for ip in api_addresses(config.listen_addr) { + let addr = std::net::SocketAddr::new(ip, port); + match query_status_pid(addr) { + Ok(pid) => return Ok(pid), + Err(e) => failures.push(format!("{addr}: {e}")), + } + } + Err(format!("no daemon API answered ({})", failures.join("; "))) +} - let Some(port) = read_port_file(port_file) else { - return false; +/// The addresses to look for the daemon's API on: the configured listen address, with an +/// unspecified one (`0.0.0.0`, `::`) replaced by the loopback of its family, then both loopbacks. +fn api_addresses(listen_addr: std::net::IpAddr) -> Vec { + use std::net::{IpAddr, Ipv4Addr, Ipv6Addr}; + + let configured = match listen_addr { + IpAddr::V4(ip) if ip.is_unspecified() => IpAddr::V4(Ipv4Addr::LOCALHOST), + IpAddr::V6(ip) if ip.is_unspecified() => IpAddr::V6(Ipv6Addr::LOCALHOST), + ip => ip, }; + let mut addresses = vec![configured]; + for loopback in [ + IpAddr::V4(Ipv4Addr::LOCALHOST), + IpAddr::V6(Ipv6Addr::LOCALHOST), + ] { + if !addresses.contains(&loopback) { + addresses.push(loopback); + } + } + addresses +} + +/// Ask the daemon API at `addr` for its status and return the PID it reports. +fn query_status_pid(addr: std::net::SocketAddr) -> std::result::Result { + use std::io::{Read, Write}; + let timeout = Duration::from_secs(1); - let addr = std::net::SocketAddr::from((std::net::Ipv4Addr::LOCALHOST, port)); - let Ok(mut stream) = std::net::TcpStream::connect_timeout(&addr, timeout) else { - return false; - }; - let request = format!( - "GET /api/v1/status HTTP/1.1\r\nHost: 127.0.0.1:{port}\r\nConnection: close\r\n\r\n" - ); + let mut stream = std::net::TcpStream::connect_timeout(&addr, timeout) + .map_err(|e| format!("could not connect: {e}"))?; + stream + .set_read_timeout(Some(timeout)) + .and_then(|()| stream.set_write_timeout(Some(timeout))) + .map_err(|e| e.to_string())?; + let request = + format!("GET /api/v1/status HTTP/1.1\r\nHost: {addr}\r\nConnection: close\r\n\r\n"); + stream + .write_all(request.as_bytes()) + .map_err(|e| format!("request failed: {e}"))?; let mut response = Vec::new(); - stream.set_read_timeout(Some(timeout)).is_ok() - && stream.set_write_timeout(Some(timeout)).is_ok() - && stream.write_all(request.as_bytes()).is_ok() - && stream.take(64 * 1024).read_to_end(&mut response).is_ok() - && status_response_reports_pid(&response, pid) + stream + .take(64 * 1024) + .read_to_end(&mut response) + .map_err(|e| format!("no response: {e}"))?; + status_response_pid(&response).ok_or_else(|| "the answer was not a daemon status".to_string()) } -/// Whether a raw HTTP response is a `200` whose [`DaemonStatus`] body reports `pid`. -fn status_response_reports_pid(response: &[u8], pid: u32) -> bool { - let Some(end_of_head) = response.windows(4).position(|w| w == b"\r\n\r\n") else { - return false; - }; +/// The PID in a raw HTTP response that is a `200` carrying a [`DaemonStatus`]. +fn status_response_pid(response: &[u8]) -> Option { + let end_of_head = response.windows(4).position(|w| w == b"\r\n\r\n")?; let status_code = std::str::from_utf8(&response[..end_of_head]) .ok() .and_then(|head| head.lines().next()) .and_then(|status_line| status_line.split_whitespace().nth(1)); - status_code == Some("200") - && serde_json::from_slice::(&response[end_of_head + 4..]) - .is_ok_and(|status| status.pid == Some(pid)) + if status_code != Some("200") { + return None; + } + serde_json::from_slice::(&response[end_of_head + 4..]) + .ok()? + .pid } /// Whether the process's executable is named `ant`. `proc_pidpath` works for any user's process, @@ -641,10 +727,7 @@ fn executable_is_ant(pid: u32) -> bool { return false; } let path = String::from_utf8_lossy(&buf[..len]); - std::path::Path::new(path.as_ref()) - .file_name() - .and_then(|name| name.to_str()) - .is_some_and(|name| name == "ant") + is_ant_executable_name(Path::new(path.as_ref())) } /// Whether the process's executable is named `ant`. `PROCESS_QUERY_LIMITED_INFORMATION` is granted @@ -670,11 +753,7 @@ fn executable_is_ant(pid: u32) -> bool { return false; } let path = String::from_utf16_lossy(&buf[..size as usize]); - // Check the executable basename, not just substring - std::path::Path::new(&path) - .file_stem() - .and_then(|s| s.to_str()) - .is_some_and(|name| name == "ant") + is_ant_executable_name(Path::new(&path)) } } @@ -718,7 +797,7 @@ fn read_pid_file(path: &Path) -> Result { } /// What the PID file says about the daemon. -#[derive(Debug, Clone, Copy, PartialEq, Eq)] +#[derive(Debug, Clone, PartialEq, Eq)] enum RecordedDaemon { /// No readable PID file. Absent, @@ -727,13 +806,18 @@ enum RecordedDaemon { /// The daemon exited without removing its files (a reboot, an OOM kill, SIGKILL). The /// recorded PID is now dead, or belongs to an unrelated process. Stale(u32), + /// The recorded PID is a live process that can't be confirmed or ruled out as the daemon + /// ([`Confirmation::Inconclusive`]). Its files must be left alone. + Unverified { pid: u32, reason: String }, } impl RecordedDaemon { - fn pid(self) -> Option { + fn pid(&self) -> Option { match self { RecordedDaemon::Absent => None, - RecordedDaemon::Live(pid) | RecordedDaemon::Stale(pid) => Some(pid), + RecordedDaemon::Live(pid) + | RecordedDaemon::Stale(pid) + | RecordedDaemon::Unverified { pid, .. } => Some(*pid), } } } @@ -742,15 +826,18 @@ impl RecordedDaemon { /// /// A live PID is not enough on its own: the files outlive an unclean exit, and after a reboot the /// PID is often reused by an unrelated process. The process must also be confirmed as the daemon -/// ([`is_confirmed_daemon`]), the same test `stop` applies before signalling it. +/// ([`confirm_daemon`]), the same test `stop` applies before signalling it. fn recorded_daemon(config: &DaemonConfig) -> RecordedDaemon { let Ok(pid) = read_pid_file(&config.pid_file_path) else { return RecordedDaemon::Absent; }; - if is_process_alive(pid) && is_confirmed_daemon(config, pid) { - RecordedDaemon::Live(pid) - } else { - RecordedDaemon::Stale(pid) + if !is_process_alive(pid) { + return RecordedDaemon::Stale(pid); + } + match confirm_daemon(config, pid) { + Confirmation::Daemon => RecordedDaemon::Live(pid), + Confirmation::NotDaemon => RecordedDaemon::Stale(pid), + Confirmation::Inconclusive(reason) => RecordedDaemon::Unverified { pid, reason }, } } @@ -759,9 +846,18 @@ fn recorded_daemon(config: &DaemonConfig) -> RecordedDaemon { /// Otherwise removes any PID and port files a previous daemon left behind, so the next daemon /// starts clean: the port poll in [`start`] would otherwise read the dead daemon's port straight /// back. That is also why a file that can't be removed is an error rather than ignored. +/// +/// A recorded process that can't be told apart from the daemon is an error too, with its files +/// left in place: deleting them could orphan a live daemon and let a second one start. fn running_daemon_or_clear_stale(config: &DaemonConfig) -> Result> { match recorded_daemon(config) { RecordedDaemon::Live(pid) => Ok(Some(pid)), + RecordedDaemon::Unverified { pid, reason } => Err(Error::ProcessSpawn(format!( + "cannot tell whether PID {pid} is the running daemon: {reason}. Nothing was removed. \ + If no daemon is running, stop that process or delete {} and {}, then start again", + config.pid_file_path.display(), + config.port_file_path.display() + ))), RecordedDaemon::Absent | RecordedDaemon::Stale(_) => { remove_stale_file(&config.pid_file_path)?; remove_stale_file(&config.port_file_path)?; @@ -1018,6 +1114,14 @@ mod tests { !is_ant_executable_name(&std::env::current_exe().unwrap()), "this test binary must never pass for `ant`" ); + // File names are case-insensitive on Windows only. + for exe in ["ANT.EXE", "Ant.exe", "Autonomi/ANT"] { + assert_eq!( + is_ant_executable_name(Path::new(exe)), + cfg!(windows), + "{exe}" + ); + } } #[test] @@ -1037,12 +1141,21 @@ mod tests { assert_eq!(identity_from(false, unreadable), ProcessIdentity::Other); } - /// A one-shot stand-in for the daemon's API: answers a single request with a `DaemonStatus` - /// reporting `pid`. + /// A one-shot stand-in for the daemon's API on `127.0.0.1`: answers a single request with a + /// `DaemonStatus` reporting `pid`. fn serve_status_once(pid: u32) -> (u16, std::thread::JoinHandle<()>) { + serve_status_once_on(std::net::IpAddr::V4(Ipv4Addr::LOCALHOST), pid) + .expect("bind 127.0.0.1") + } + + /// [`serve_status_once`] on `ip`, or `None` where it can't be bound (a host without IPv6). + fn serve_status_once_on( + ip: std::net::IpAddr, + pid: u32, + ) -> Option<(u16, std::thread::JoinHandle<()>)> { use std::io::{Read, Write}; - let listener = std::net::TcpListener::bind((Ipv4Addr::LOCALHOST, 0)).unwrap(); + let listener = std::net::TcpListener::bind((ip, 0)).ok()?; let port = listener.local_addr().unwrap().port(); let server = std::thread::spawn(move || { let (mut stream, _) = listener.accept().unwrap(); @@ -1067,7 +1180,7 @@ mod tests { ) .unwrap(); }); - (port, server) + Some((port, server)) } /// A port with nothing listening on it, as a dead daemon's port usually is. @@ -1079,41 +1192,88 @@ mod tests { .port() } + /// A port that accepts connections but never answers, as a hung daemon's would. + fn silent_port() -> (u16, std::net::TcpListener) { + let listener = std::net::TcpListener::bind((Ipv4Addr::LOCALHOST, 0)).unwrap(); + (listener.local_addr().unwrap().port(), listener) + } + #[test] - fn the_daemon_api_must_report_the_same_pid() { + fn the_daemon_api_reports_its_pid_or_why_it_did_not() { let tmp = tempfile::tempdir().unwrap(); - let port_file = tmp.path().join("daemon.port"); - assert!(!daemon_api_reports_pid(&port_file, 42), "no port file"); + let config = config_in(tmp.path()); + assert!(daemon_api_pid(&config).is_err(), "no port file"); + + std::fs::write(&config.port_file_path, unused_port().to_string()).unwrap(); + assert!(daemon_api_pid(&config).is_err(), "nothing listening"); - std::fs::write(&port_file, unused_port().to_string()).unwrap(); - assert!(!daemon_api_reports_pid(&port_file, 42), "nothing listening"); + let (port, _listener) = silent_port(); + std::fs::write(&config.port_file_path, port.to_string()).unwrap(); + assert!( + daemon_api_pid(&config).is_err(), + "no answer before the timeout" + ); let (port, server) = serve_status_once(42); - std::fs::write(&port_file, port.to_string()).unwrap(); - assert!(daemon_api_reports_pid(&port_file, 42)); + std::fs::write(&config.port_file_path, port.to_string()).unwrap(); + assert_eq!(daemon_api_pid(&config), Ok(42)); server.join().unwrap(); + } - let (port, server) = serve_status_once(43); - std::fs::write(&port_file, port.to_string()).unwrap(); - assert!( - !daemon_api_reports_pid(&port_file, 42), - "another daemon's PID" + #[test] + fn the_daemon_api_is_found_on_ipv6_too() { + use std::net::{IpAddr, Ipv6Addr}; + + let ipv6 = IpAddr::V6(Ipv6Addr::LOCALHOST); + let tmp = tempfile::tempdir().unwrap(); + // Configured for `::1`, as `daemon run --listen-addr ::1`, and with the default config, + // as a call that doesn't know which address the daemon was started on. + for listen_addr in [ipv6, DaemonConfig::default().listen_addr] { + let config = DaemonConfig { + listen_addr, + ..config_in(tmp.path()) + }; + let Some((port, server)) = serve_status_once_on(ipv6, 42) else { + eprintln!("skipped: this host can't bind ::1"); + return; + }; + std::fs::write(&config.port_file_path, port.to_string()).unwrap(); + assert_eq!(daemon_api_pid(&config), Ok(42), "configured {listen_addr}"); + server.join().unwrap(); + } + } + + #[test] + fn the_api_is_looked_for_on_the_listen_address_then_both_loopbacks() { + use std::net::{IpAddr, Ipv6Addr}; + + let v4 = IpAddr::V4(Ipv4Addr::LOCALHOST); + let v6 = IpAddr::V6(Ipv6Addr::LOCALHOST); + let lan = IpAddr::V4(Ipv4Addr::new(192, 168, 1, 5)); + assert_eq!(api_addresses(v4), vec![v4, v6]); + assert_eq!( + api_addresses(IpAddr::V4(Ipv4Addr::UNSPECIFIED)), + vec![v4, v6] ); - server.join().unwrap(); + assert_eq!(api_addresses(v6), vec![v6, v4]); + assert_eq!( + api_addresses(IpAddr::V6(Ipv6Addr::UNSPECIFIED)), + vec![v6, v4] + ); + assert_eq!(api_addresses(lan), vec![lan, v4, v6]); } #[test] - fn only_a_200_with_the_pid_confirms_the_daemon() { + fn only_a_200_daemon_status_carries_a_pid() { let ok = |body: &str| format!("HTTP/1.1 200 OK\r\n\r\n{body}"); let status = r#"{"running":true,"pid":42,"port":1,"uptime_secs":1,"nodes_total":0,"nodes_running":0,"nodes_stopped":0,"nodes_errored":0}"#; - assert!(status_response_reports_pid(ok(status).as_bytes(), 42)); - assert!(!status_response_reports_pid(ok(status).as_bytes(), 7)); - assert!(!status_response_reports_pid( - format!("HTTP/1.1 404 Not Found\r\n\r\n{status}").as_bytes(), - 42 - )); - assert!(!status_response_reports_pid(ok("not json").as_bytes(), 42)); - assert!(!status_response_reports_pid(b"garbage", 42)); + assert_eq!(status_response_pid(ok(status).as_bytes()), Some(42)); + assert_eq!( + status_response_pid(format!("HTTP/1.1 404 Not Found\r\n\r\n{status}").as_bytes()), + None + ); + assert_eq!(status_response_pid(ok("not json").as_bytes()), None); + assert_eq!(status_response_pid(b"garbage"), None); } /// Test-name filter that selects [`sleep_as_an_ant_named_process`] in a copy of this binary. @@ -1145,10 +1305,13 @@ mod tests { #[cfg(any(target_os = "linux", target_os = "macos", windows))] impl AntNamedProcess { fn spawn(args: &[&str]) -> Self { + Self::spawn_as(if cfg!(windows) { "ant.exe" } else { "ant" }, args) + } + + /// [`spawn`](Self::spawn) with the copy named `file_name`. + fn spawn_as(file_name: &str, args: &[&str]) -> Self { let dir = tempfile::tempdir().unwrap(); - let exe = dir - .path() - .join(if cfg!(windows) { "ant.exe" } else { "ant" }); + let exe = dir.path().join(file_name); std::fs::copy(std::env::current_exe().unwrap(), &exe).unwrap(); let mut command = std::process::Command::new(&exe); command @@ -1226,7 +1389,7 @@ mod tests { #[cfg(any(target_os = "linux", target_os = "macos", windows))] #[test] - fn another_ant_command_holding_the_pid_is_not_the_daemon() { + fn another_ant_command_holding_the_pid_is_never_confirmed_by_its_arguments() { // `daemon run` as another command's arguments, the review's example. let mut process = AntNamedProcess::spawn(&["file", "upload", "daemon", "run"]); let pid = process.pid(); @@ -1235,10 +1398,92 @@ mod tests { std::fs::write(&config.port_file_path, unused_port().to_string()).unwrap(); assert_eq!(process_identity(pid), ProcessIdentity::Unconfirmed); - assert!(!is_confirmed_daemon(&config, pid)); + assert!(matches!( + confirm_daemon(&config, pid), + Confirmation::Inconclusive(_) + )); + process.assert_still_running(); + } + + /// An `ant` process that can't be confirmed, recorded with a port whose API is unavailable or + /// silent: a live daemon whose API was briefly slow looks exactly like this. + #[cfg(any(target_os = "linux", target_os = "macos", windows))] + fn unverified_daemon_files(process: &AntNamedProcess, dir: &Path, port: u16) -> DaemonConfig { + let config = config_in(dir); + std::fs::write(&config.pid_file_path, process.pid().to_string()).unwrap(); + std::fs::write(&config.port_file_path, port.to_string()).unwrap(); + config + } + + #[cfg(any(target_os = "linux", target_os = "macos", windows))] + #[test] + fn start_keeps_the_files_of_a_daemon_it_cannot_verify() { + let mut process = AntNamedProcess::spawn(&["file", "upload", "daemon", "run"]); + let (silent, _listener) = silent_port(); + for (port, api) in [(unused_port(), "unavailable"), (silent, "timed out")] { + let tmp = tempfile::tempdir().unwrap(); + let config = unverified_daemon_files(&process, tmp.path(), port); + + let err = running_daemon_or_clear_stale(&config).unwrap_err(); + assert!( + err.to_string().contains("cannot tell whether PID"), + "{api}: {err}" + ); + assert!(config.pid_file_path.exists(), "{api}: PID file removed"); + assert!(config.port_file_path.exists(), "{api}: port file removed"); + assert!(!info(&config).running, "{api}"); + } + process.assert_still_running(); + } + + #[cfg(any(target_os = "linux", target_os = "macos", windows))] + #[tokio::test] + async fn stop_neither_signals_nor_clears_a_daemon_it_cannot_verify() { + let mut process = AntNamedProcess::spawn(&["file", "upload", "daemon", "run"]); + let tmp = tempfile::tempdir().unwrap(); + let config = unverified_daemon_files(&process, tmp.path(), unused_port()); + + let err = stop(&config).await.unwrap_err(); + assert!(err.to_string().contains("cannot tell whether PID"), "{err}"); + process.assert_still_running(); + assert!(config.pid_file_path.exists()); + assert!(config.port_file_path.exists()); + } + + #[cfg(any(target_os = "linux", target_os = "macos", windows))] + #[test] + fn another_daemon_answering_on_the_port_rules_the_process_out() { + let mut process = AntNamedProcess::spawn(&["file", "upload", "daemon", "run"]); + let (port, server) = serve_status_once(process.pid() + 1); + let tmp = tempfile::tempdir().unwrap(); + let config = unverified_daemon_files(&process, tmp.path(), port); + + assert_eq!(running_daemon_or_clear_stale(&config).unwrap(), None); + assert!(!config.pid_file_path.exists()); + assert!(!config.port_file_path.exists()); + server.join().unwrap(); process.assert_still_running(); } + #[cfg(windows)] + #[test] + fn an_upper_or_mixed_case_ant_exe_is_ant_on_windows() { + for file_name in ["ANT.EXE", "Ant.exe"] { + let mut process = AntNamedProcess::spawn_as(file_name, &["node", "daemon", "run"]); + let identified = (0..250).any(|_| { + process_identity(process.pid()) == ProcessIdentity::Daemon || { + std::thread::sleep(Duration::from_millis(20)); + false + } + }); + process.assert_still_running(); + assert!( + identified, + "{file_name} running `node daemon run` is the daemon" + ); + } + } + #[cfg(any(target_os = "linux", target_os = "macos", windows))] #[test] fn an_unconfirmed_ant_process_is_the_daemon_only_if_its_api_says_so() { From 7be1709756ffe324e2e617a29e24d7985f14ff59 Mon Sep 17 00:00:00 2001 From: Nic-dorman Date: Mon, 26 Oct 2026 17:45:20 +0000 Subject: [PATCH 6/6] fix(core): don't rule out a daemon whose executable or API answer is unknown, and bound the status query - macOS and Windows: an executable that can't be looked up is unknown, not "not ant", so the process goes to the API check instead of losing its files. - The API check tries every candidate address until one reports the recorded PID. Another daemon answering on the same port is no longer proof that the recorded process isn't ours; it is inconclusive. - Each status query has one 1 s deadline across connect, request and response, so a trickling listener can't hold `info`, `start` or `stop`. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 9 +- ant-core/src/node/daemon/client.rs | 323 ++++++++++++++++++++++------- 2 files changed, 248 insertions(+), 84 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a2419ede..309844c0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -68,10 +68,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 the check `stop` already applied, and `start` removes the stale files before starting a new daemon. On every platform the process must be `ant` running `ant node daemon run`, so another `ant` command holding the reused PID is not taken for the daemon either. Where a process's - arguments can't be read (another user's, an elevated one on Windows) or don't show that, the - daemon's own API on the recorded port decides, on the configured listen address or either - loopback (IPv4 or IPv6). If that API doesn't answer, `start` and `stop` say they can't tell and - leave the files alone, rather than risk orphaning a live daemon. Previously macOS trusted any + arguments or executable can't be read (another user's, an elevated or protected one on + Windows) or don't show that, it counts as the daemon only if the daemon's own API on the + recorded port reports its PID, on the configured listen address or either loopback (IPv4 or + IPv6), each asked with a one-second deadline. Otherwise `start` and `stop` say they can't tell + and leave the files alone, rather than risk orphaning a live daemon. Previously macOS trusted any live PID and Windows checked only the executable's name, case-sensitively. `start` now fails instead of carrying on when a stale file can't be removed, and it only spawns the daemon from the `ant` binary. The `info` help text and README no longer claim its output is always JSON: diff --git a/ant-core/src/node/daemon/client.rs b/ant-core/src/node/daemon/client.rs index fabcb835..f5e5d4be 100644 --- a/ant-core/src/node/daemon/client.rs +++ b/ant-core/src/node/daemon/client.rs @@ -506,22 +506,22 @@ enum ProcessIdentity { /// An `ant` executable whose arguments don't show that it is the daemon. They are another /// command line, such as another `ant` command or a foreground daemon started behind global /// options, or they can't be read: a process owned by another user, an elevated one on - /// Windows, or no `/proc`. [`confirm_daemon`] asks the daemon's API instead. + /// Windows, or no `/proc`. Also a process whose executable can't be looked up, which may be + /// `ant`. [`confirm_daemon`] asks the daemon's API instead. Unconfirmed, } -/// Classify a process from whether its executable is `ant` and from its arguments (`argv[0]` -/// first), `None` when they can't be read. Unreadable arguments are never taken as proof. +/// Classify a process from whether its executable is `ant` (`None` when it can't be looked up) +/// and from its arguments (`argv[0]` first, `None` when they can't be read). Only an executable +/// known not to be `ant` rules a process out, and unreadable arguments are never taken as proof. fn identity_from>( - executable_is_ant: bool, + executable_is_ant: Option, args: Option<&[S]>, ) -> ProcessIdentity { - if !executable_is_ant { - ProcessIdentity::Other - } else if args.is_some_and(is_daemon_command) { - ProcessIdentity::Daemon - } else { - ProcessIdentity::Unconfirmed + match executable_is_ant { + Some(false) => ProcessIdentity::Other, + Some(true) if args.is_some_and(is_daemon_command) => ProcessIdentity::Daemon, + _ => ProcessIdentity::Unconfirmed, } } @@ -551,7 +551,7 @@ fn process_identity(pid: u32) -> ProcessIdentity { let executable_is_ant = args .first() .is_some_and(|exe| is_ant_executable_name(Path::new(exe))); - identity_from(executable_is_ant, Some(args.as_slice())) + identity_from(Some(executable_is_ant), Some(args.as_slice())) } /// Whether an executable path names the `ant` binary. The basename must be `ant` or `ant.exe`, @@ -599,55 +599,63 @@ fn process_args(pid: u32) -> Option> { enum Confirmation { /// It is the daemon. Daemon, - /// It is not: its executable isn't `ant`, or the daemon's API answers with another PID. + /// It is not: its executable isn't `ant`. NotDaemon, - /// It can't be told either way: an `ant` process whose arguments don't show it is the daemon, - /// and no daemon API answered. The text says why. Nothing may be deleted or signalled on this - /// basis, since a live daemon whose API was briefly slow would lose its files. + /// It can't be told either way: a process that may be `ant` and whose arguments don't show it + /// is the daemon, and no daemon API reported its PID. The text says why. Nothing may be + /// deleted or signalled on this basis: a live daemon would lose its files if its API was + /// briefly slow, or if another daemon on the same port answered first. Inconclusive(String), } /// Whether the live process `pid` is the daemon that `config`'s files record. /// -/// The process's own details settle it when they can (see [`ProcessIdentity`]). For an -/// [`Unconfirmed`](ProcessIdentity::Unconfirmed) `ant` process, the daemon's API on the recorded -/// port decides: our PID is the daemon, another PID is not, and no answer is inconclusive. +/// The process's own details settle it when they can (see [`ProcessIdentity`]). An +/// [`Unconfirmed`](ProcessIdentity::Unconfirmed) process is the daemon only if the daemon's API +/// on the recorded port reports its PID. Anything else is inconclusive: another PID there may be +/// a different daemon on the same port, which says nothing about this process. fn confirm_daemon(config: &DaemonConfig, pid: u32) -> Confirmation { match process_identity(pid) { ProcessIdentity::Daemon => Confirmation::Daemon, ProcessIdentity::Other => Confirmation::NotDaemon, - ProcessIdentity::Unconfirmed => match daemon_api_pid(config) { - Ok(api_pid) if api_pid == pid => Confirmation::Daemon, - Ok(_) => Confirmation::NotDaemon, + ProcessIdentity::Unconfirmed => match daemon_api_reports(config, pid) { + Ok(()) => Confirmation::Daemon, Err(reason) => Confirmation::Inconclusive(format!( - "it is an `ant` process whose arguments don't show it is the daemon, and {reason}" + "it may be an `ant` process, its arguments don't show it is the daemon, and \ + {reason}" )), }, } } -/// The PID the daemon's API reports (`GET /api/v1/status`) on the port in `config`'s port file, -/// or why no API answered. +/// Whether the daemon's API (`GET /api/v1/status`) on the port in `config`'s port file reports +/// `pid`, or what it said instead. /// /// Tries the address the daemon was configured to listen on, then both loopbacks, since the -/// daemon may have been started with another `--listen-addr` than this call's config. These are -/// blocking local requests with one-second timeouts, made only for an unconfirmed process. -fn daemon_api_pid(config: &DaemonConfig) -> std::result::Result { +/// daemon may have been started with another `--listen-addr` than this call's config. Every +/// address is tried until one reports `pid`, since another daemon may hold the same port on +/// another address. These are blocking local requests of at most [`STATUS_QUERY_DEADLINE`] each, +/// made only for an unconfirmed process. +fn daemon_api_reports(config: &DaemonConfig, pid: u32) -> std::result::Result<(), String> { let Some(port) = read_port_file(&config.port_file_path) else { return Err(format!( "{} holds no port to ask its API on", config.port_file_path.display() )); }; - let mut failures = Vec::new(); + let mut answers = Vec::new(); for ip in api_addresses(config.listen_addr) { let addr = std::net::SocketAddr::new(ip, port); match query_status_pid(addr) { - Ok(pid) => return Ok(pid), - Err(e) => failures.push(format!("{addr}: {e}")), + Ok(api_pid) if api_pid == pid => return Ok(()), + Ok(api_pid) => answers.push(format!("{addr} answered for PID {api_pid}")), + Err(e) => answers.push(format!("{addr}: {e}")), } } - Err(format!("no daemon API answered ({})", failures.join("; "))) + Err(format!( + "no daemon API reported PID {pid} ({})", + answers.join("; ") + )) } /// The addresses to look for the daemon's API on: the configured listen address, with an @@ -672,16 +680,36 @@ fn api_addresses(listen_addr: std::net::IpAddr) -> Vec { addresses } +/// How long one status query may take, from connecting to the end of the answer. +const STATUS_QUERY_DEADLINE: Duration = Duration::from_secs(1); + +/// The most of an answer a status query reads. A daemon status is far smaller. +const STATUS_RESPONSE_LIMIT: usize = 64 * 1024; + /// Ask the daemon API at `addr` for its status and return the PID it reports. +/// +/// The whole exchange must finish within [`STATUS_QUERY_DEADLINE`]. Each read may wait only for +/// what is left of it, so a listener that trickles its answer can't hold the caller past it. fn query_status_pid(addr: std::net::SocketAddr) -> std::result::Result { - use std::io::{Read, Write}; - - let timeout = Duration::from_secs(1); - let mut stream = std::net::TcpStream::connect_timeout(&addr, timeout) + use std::io::{ErrorKind, Read, Write}; + + let deadline = std::time::Instant::now() + STATUS_QUERY_DEADLINE; + let out_of_time = || { + format!( + "no complete answer within {} ms", + STATUS_QUERY_DEADLINE.as_millis() + ) + }; + let time_left = || { + deadline + .checked_duration_since(std::time::Instant::now()) + .filter(|left| !left.is_zero()) + .ok_or_else(out_of_time) + }; + let mut stream = std::net::TcpStream::connect_timeout(&addr, STATUS_QUERY_DEADLINE) .map_err(|e| format!("could not connect: {e}"))?; stream - .set_read_timeout(Some(timeout)) - .and_then(|()| stream.set_write_timeout(Some(timeout))) + .set_write_timeout(Some(time_left()?)) .map_err(|e| e.to_string())?; let request = format!("GET /api/v1/status HTTP/1.1\r\nHost: {addr}\r\nConnection: close\r\n\r\n"); @@ -689,10 +717,27 @@ fn query_status_pid(addr: std::net::SocketAddr) -> std::result::Result break, + Ok(n) => response.extend_from_slice(&chunk[..n]), + Err(e) if e.kind() == ErrorKind::Interrupted => {} + // The read timeout is what was left of the deadline. + Err(e) if matches!(e.kind(), ErrorKind::WouldBlock | ErrorKind::TimedOut) => { + return Err(out_of_time()); + } + Err(e) => return Err(format!("no response: {e}")), + } + if response.len() > STATUS_RESPONSE_LIMIT { + return Err(format!( + "the answer was longer than {STATUS_RESPONSE_LIMIT} bytes" + )); + } + } status_response_pid(&response).ok_or_else(|| "the answer was not a daemon status".to_string()) } @@ -711,29 +756,23 @@ fn status_response_pid(response: &[u8]) -> Option { .pid } -/// Whether the process's executable is named `ant`. `proc_pidpath` works for any user's process, -/// even where the arguments can't be read. +/// Whether the process's executable is named `ant`, or `None` if it can't be looked up. +/// `proc_pidpath` works for any user's process, even where the arguments can't be read. #[cfg(target_os = "macos")] -fn executable_is_ant(pid: u32) -> bool { - let Some(pid) = pid_to_i32(pid) else { - return false; - }; +fn executable_is_ant(pid: u32) -> Option { + let pid = pid_to_i32(pid)?; let mut buf = [0u8; libc::PROC_PIDPATHINFO_MAXSIZE as usize]; let len = unsafe { libc::proc_pidpath(pid, buf.as_mut_ptr().cast(), buf.len() as u32) }; - let Ok(len) = usize::try_from(len) else { - return false; - }; - if len == 0 { - return false; - } + let len = usize::try_from(len).ok().filter(|&len| len > 0)?; let path = String::from_utf8_lossy(&buf[..len]); - is_ant_executable_name(Path::new(path.as_ref())) + Some(is_ant_executable_name(Path::new(path.as_ref()))) } -/// Whether the process's executable is named `ant`. `PROCESS_QUERY_LIMITED_INFORMATION` is granted -/// even for an elevated process, whose arguments can't be read. +/// Whether the process's executable is named `ant`, or `None` if it can't be looked up. +/// `PROCESS_QUERY_LIMITED_INFORMATION` is granted even for an elevated process, whose arguments +/// can't be read, but not for a protected one. #[cfg(windows)] -fn executable_is_ant(pid: u32) -> bool { +fn executable_is_ant(pid: u32) -> Option { use windows_sys::Win32::Foundation::CloseHandle; use windows_sys::Win32::System::Threading::{ OpenProcess, QueryFullProcessImageNameW, PROCESS_QUERY_LIMITED_INFORMATION, @@ -742,7 +781,7 @@ fn executable_is_ant(pid: u32) -> bool { unsafe { let handle = OpenProcess(PROCESS_QUERY_LIMITED_INFORMATION, 0, pid); if handle.is_null() { - return false; + return None; } let mut buf = [0u16; 1024]; let mut size = buf.len() as u32; @@ -750,10 +789,10 @@ fn executable_is_ant(pid: u32) -> bool { CloseHandle(handle); if success == 0 { - return false; + return None; } let path = String::from_utf16_lossy(&buf[..size as usize]); - is_ant_executable_name(Path::new(&path)) + Some(is_ant_executable_name(Path::new(&path))) } } @@ -1128,17 +1167,56 @@ mod tests { fn unreadable_arguments_never_confirm_the_daemon() { let daemon = Some(&["ant", "node", "daemon", "run"][..]); let unreadable: Option<&[&str]> = None; - assert_eq!(identity_from(true, daemon), ProcessIdentity::Daemon); + assert_eq!(identity_from(Some(true), daemon), ProcessIdentity::Daemon); assert_eq!( - identity_from(true, unreadable), + identity_from(Some(true), unreadable), ProcessIdentity::Unconfirmed ); assert_eq!( - identity_from(true, Some(&["ant", "file", "upload", "daemon", "run"][..])), + identity_from( + Some(true), + Some(&["ant", "file", "upload", "daemon", "run"][..]) + ), ProcessIdentity::Unconfirmed ); - assert_eq!(identity_from(false, daemon), ProcessIdentity::Other); - assert_eq!(identity_from(false, unreadable), ProcessIdentity::Other); + assert_eq!(identity_from(Some(false), daemon), ProcessIdentity::Other); + assert_eq!( + identity_from(Some(false), unreadable), + ProcessIdentity::Other + ); + } + + #[test] + fn an_executable_that_cannot_be_looked_up_is_never_ruled_out() { + let daemon = Some(&["ant", "node", "daemon", "run"][..]); + let unreadable: Option<&[&str]> = None; + assert_eq!(identity_from(None, daemon), ProcessIdentity::Unconfirmed); + assert_eq!( + identity_from(None, unreadable), + ProcessIdentity::Unconfirmed + ); + } + + /// A PID no process holds: beyond macOS's PID range and any Windows process table. + #[cfg(any(target_os = "macos", windows))] + const NO_SUCH_PID: u32 = 0x3FFF_FFFC; + + #[cfg(any(target_os = "macos", windows))] + #[test] + fn a_failed_executable_lookup_leaves_the_daemon_unconfirmed() { + assert_eq!(executable_is_ant(std::process::id()), Some(false)); + assert_eq!(executable_is_ant(NO_SUCH_PID), None); + assert_eq!(process_identity(NO_SUCH_PID), ProcessIdentity::Unconfirmed); + + let tmp = tempfile::tempdir().unwrap(); + let config = config_in(tmp.path()); + assert!( + matches!( + confirm_daemon(&config, NO_SUCH_PID), + Confirmation::Inconclusive(_) + ), + "a process that can't be looked up is never taken as not the daemon" + ); } /// A one-shot stand-in for the daemon's API on `127.0.0.1`: answers a single request with a @@ -1153,11 +1231,20 @@ mod tests { ip: std::net::IpAddr, pid: u32, ) -> Option<(u16, std::thread::JoinHandle<()>)> { + let listener = std::net::TcpListener::bind((ip, 0)).ok()?; + let port = listener.local_addr().unwrap().port(); + Some((port, serve_status_once_with(listener, pid))) + } + + /// [`serve_status_once`] on an already bound `listener`. + fn serve_status_once_with( + listener: std::net::TcpListener, + pid: u32, + ) -> std::thread::JoinHandle<()> { use std::io::{Read, Write}; - let listener = std::net::TcpListener::bind((ip, 0)).ok()?; let port = listener.local_addr().unwrap().port(); - let server = std::thread::spawn(move || { + std::thread::spawn(move || { let (mut stream, _) = listener.accept().unwrap(); let mut request = [0u8; 1024]; let _ = stream.read(&mut request); @@ -1179,8 +1266,7 @@ mod tests { body.len() ) .unwrap(); - }); - Some((port, server)) + }) } /// A port with nothing listening on it, as a dead daemon's port usually is. @@ -1202,24 +1288,59 @@ mod tests { fn the_daemon_api_reports_its_pid_or_why_it_did_not() { let tmp = tempfile::tempdir().unwrap(); let config = config_in(tmp.path()); - assert!(daemon_api_pid(&config).is_err(), "no port file"); + assert!(daemon_api_reports(&config, 42).is_err(), "no port file"); std::fs::write(&config.port_file_path, unused_port().to_string()).unwrap(); - assert!(daemon_api_pid(&config).is_err(), "nothing listening"); + assert!( + daemon_api_reports(&config, 42).is_err(), + "nothing listening" + ); let (port, _listener) = silent_port(); std::fs::write(&config.port_file_path, port.to_string()).unwrap(); assert!( - daemon_api_pid(&config).is_err(), - "no answer before the timeout" + daemon_api_reports(&config, 42).is_err(), + "no answer before the deadline" ); + let (port, server) = serve_status_once(7); + std::fs::write(&config.port_file_path, port.to_string()).unwrap(); + let answer = daemon_api_reports(&config, 42).unwrap_err(); + assert!(answer.contains("answered for PID 7"), "{answer}"); + server.join().unwrap(); + let (port, server) = serve_status_once(42); std::fs::write(&config.port_file_path, port.to_string()).unwrap(); - assert_eq!(daemon_api_pid(&config), Ok(42)); + assert_eq!(daemon_api_reports(&config, 42), Ok(())); server.join().unwrap(); } + #[test] + fn a_trickled_answer_is_cut_off_at_the_deadline() { + use std::io::{Read, Write}; + + let listener = std::net::TcpListener::bind((Ipv4Addr::LOCALHOST, 0)).unwrap(); + let addr = listener.local_addr().unwrap(); + // A byte every 100 ms: no single read waits long, and the size cap is far off. + std::thread::spawn(move || { + let (mut stream, _) = listener.accept().unwrap(); + let mut request = [0u8; 1024]; + let _ = stream.read(&mut request); + for _ in 0..100 { + if stream.write_all(b"H").is_err() { + break; + } + std::thread::sleep(Duration::from_millis(100)); + } + }); + + let started = std::time::Instant::now(); + let answer = query_status_pid(addr).unwrap_err(); + let took = started.elapsed(); + assert!(answer.contains("no complete answer"), "{answer}"); + assert!(took < STATUS_QUERY_DEADLINE * 3, "took {took:?}"); + } + #[test] fn the_daemon_api_is_found_on_ipv6_too() { use std::net::{IpAddr, Ipv6Addr}; @@ -1238,11 +1359,47 @@ mod tests { return; }; std::fs::write(&config.port_file_path, port.to_string()).unwrap(); - assert_eq!(daemon_api_pid(&config), Ok(42), "configured {listen_addr}"); + assert_eq!( + daemon_api_reports(&config, 42), + Ok(()), + "configured {listen_addr}" + ); server.join().unwrap(); } } + #[test] + fn a_daemon_on_ipv6_is_found_behind_another_on_ipv4_at_the_same_port() { + use std::net::{IpAddr, Ipv6Addr, TcpListener}; + + // Ours on `[::1]:port`, and another daemon on `127.0.0.1:port`, which is asked first. + let Some((port, ipv6, ipv4)) = (0..20).find_map(|_| { + let ipv6 = TcpListener::bind((Ipv6Addr::LOCALHOST, 0)).ok()?; + let port = ipv6.local_addr().unwrap().port(); + let ipv4 = TcpListener::bind((Ipv4Addr::LOCALHOST, port)).ok()?; + Some((port, ipv6, ipv4)) + }) else { + eprintln!("skipped: this host can't bind ::1 and 127.0.0.1 to one port"); + return; + }; + let ours = serve_status_once_with(ipv6, 42); + let other = serve_status_once_with(ipv4, 7); + let tmp = tempfile::tempdir().unwrap(); + let config = config_in(tmp.path()); + assert_eq!( + api_addresses(config.listen_addr), + [ + IpAddr::V4(Ipv4Addr::LOCALHOST), + IpAddr::V6(Ipv6Addr::LOCALHOST) + ] + ); + std::fs::write(&config.port_file_path, port.to_string()).unwrap(); + + assert_eq!(daemon_api_reports(&config, 42), Ok(())); + other.join().unwrap(); + ours.join().unwrap(); + } + #[test] fn the_api_is_looked_for_on_the_listen_address_then_both_loopbacks() { use std::net::{IpAddr, Ipv6Addr}; @@ -1452,15 +1609,21 @@ mod tests { #[cfg(any(target_os = "linux", target_os = "macos", windows))] #[test] - fn another_daemon_answering_on_the_port_rules_the_process_out() { + fn another_daemon_answering_on_the_port_does_not_rule_the_process_out() { let mut process = AntNamedProcess::spawn(&["file", "upload", "daemon", "run"]); - let (port, server) = serve_status_once(process.pid() + 1); + let other = process.pid() + 1; + let (port, server) = serve_status_once(other); let tmp = tempfile::tempdir().unwrap(); let config = unverified_daemon_files(&process, tmp.path(), port); - assert_eq!(running_daemon_or_clear_stale(&config).unwrap(), None); - assert!(!config.pid_file_path.exists()); - assert!(!config.port_file_path.exists()); + let err = running_daemon_or_clear_stale(&config).unwrap_err(); + assert!( + err.to_string() + .contains(&format!("answered for PID {other}")), + "{err}" + ); + assert!(config.pid_file_path.exists()); + assert!(config.port_file_path.exists()); server.join().unwrap(); process.assert_still_running(); }