From 2eadfc5fdf8aae6d76134d10e0af376937730e65 Mon Sep 17 00:00:00 2001 From: Jakub Date: Tue, 21 Jul 2026 14:26:46 +0200 Subject: [PATCH] fix(daemon): restore instance-lock recovery from a wedged daemon MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Starting Okena while a UI-owned daemon held the instance lock without advertising ended in a hard lockout the user had to clear by killing the process by hand: WARN Instance lock owner pid 95630 neither re-advertised nor exited within 8s; reaping it WARN Refusing to reap pid 95630: process is not a verified UI-owned Okena daemon The guard verified ownership by looking for `--ui-owned` in the owner's argv, read via sysinfo after `System::refresh_processes`. That call refreshes only a minimal set of fields and leaves `cmd()` empty — `name` and `exe` are populated, argv is not. The flag was therefore never found, no process ever verified, and the whole reap path was unreachable. Ask for the command line explicitly. That made the post-reap check reachable for the first time, and it was wrong: it failed on "is the lock held by anyone", not "is it still held by the process we just killed". The likeliest reason a daemon wedges un-advertised is a restart in flight — and in that state its replacement is already running, blocked on `--await-pid `, waiting for exactly this kill to acquire the lock. So a successful recovery reported itself as the same lockout, now naming a pid that no longer existed. Wait for the reaped pid to actually exit, fail only if that pid still holds the lock, and give a successor its own window to advertise. Process inspection moves behind `read_process_identity` + a pure `is_ui_owned_okena` predicate so both are testable; the unix test spawns a child and asserts its argv is readable, which fails on the old call with `args: []`. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01Y4Aq3X2s6ftafXzmqaN3A8 --- crates/okena-remote-server/src/local.rs | 227 +++++++++++++++++++----- 1 file changed, 184 insertions(+), 43 deletions(-) diff --git a/crates/okena-remote-server/src/local.rs b/crates/okena-remote-server/src/local.rs index dc1c21680..facb28e2f 100644 --- a/crates/okena-remote-server/src/local.rs +++ b/crates/okena-remote-server/src/local.rs @@ -272,7 +272,7 @@ pub fn remember_current_executable() -> std::io::Result { pub fn spawn_daemon() -> std::io::Result { let exe = remember_current_executable()?; std::process::Command::new(exe) - .args(["--headless", "--ui-owned"]) + .args(["--headless", UI_OWNED_FLAG]) .spawn() } @@ -511,6 +511,11 @@ fn wait_until_ready_replacing(old_pid: u32, timeout: Duration) -> Option= deadline { - if let Some(pid) = instance_lock_owner_pid() { - log::warn!( - "Instance lock owner pid {pid} neither re-advertised nor exited within {LOCK_SETTLE_TIMEOUT:?}; reaping it" - ); - if !instance_lock_is_held() { - break; - } - if !kill_pid_if_ui_owned_okena(pid) { - return Err(format!( - "Instance lock is held by pid {pid}, but it is not a verified UI-owned Okena daemon; refusing to terminate it." - )); - } - // Let the OS release the flock the reaped process held. - std::thread::sleep(Duration::from_millis(200)); - if instance_lock_is_held() { - return Err(format!( - "UI-owned daemon pid {pid} did not release the instance lock after termination." - )); - } + let Some(pid) = instance_lock_owner_pid() else { + break; + }; + if !instance_lock_is_held() { + break; } - break; + if reaps_left == 0 { + log::warn!("Instance lock still held by pid {pid} after reaping; spawning anyway"); + break; + } + log::warn!( + "Instance lock owner pid {pid} neither re-advertised nor exited within {LOCK_SETTLE_TIMEOUT:?}; reaping it" + ); + if !kill_pid_if_ui_owned_okena(pid) { + return Err(format!( + "Instance lock is held by pid {pid}, but it is not a verified UI-owned Okena daemon; refusing to terminate it." + )); + } + reaps_left -= 1; + // Wait for the process itself to go, which is what releases the + // flock — rather than sleeping a fixed guess. + wait_for_pid_exit(pid, REAP_RELEASE_TIMEOUT); + + // Only *this* pid still holding the lock is a failure. Someone + // else holding it is the expected outcome of reaping a daemon + // that wedged mid-restart: its replacement was already spawned + // and blocked on `--await-pid `, so killing the outgoing + // process is exactly the event that lets the successor acquire. + // Treating any holder as failure reported that success as the + // very lockout this branch exists to clear. + if instance_lock_owner_pid() == Some(pid) && instance_lock_is_held() { + return Err(format!( + "UI-owned daemon pid {pid} did not release the instance lock after termination." + )); + } + if !instance_lock_owner_alive() { + log::info!("Instance lock released; spawning a fresh daemon"); + break; + } + // A successor took over — give it the same chance to advertise + // that the process we just reaped had. + log::info!("Instance lock passed to a successor; waiting for it to advertise"); + deadline = Instant::now() + LOCK_SETTLE_TIMEOUT; } } } @@ -752,6 +783,10 @@ const LOCK_SETTLE_TIMEOUT: Duration = Duration::from_secs(8); /// Per-iteration reachability poll while waiting out an un-advertised owner. const LOCK_SETTLE_POLL: Duration = Duration::from_millis(400); +/// How long to wait for a reaped process to actually disappear (and with it the +/// flock it held) before deciding the reap failed. +const REAP_RELEASE_TIMEOUT: Duration = Duration::from_secs(2); + /// Best-effort read of the instance-lock file's recorded owner pid (0/absent → None). fn instance_lock_owner_pid() -> Option { let path = okena_workspace::persistence::instance_lock_path(); @@ -779,37 +814,81 @@ fn instance_lock_is_held() -> bool { } } +/// What we could learn about a live process, as the reap guard needs it. +#[derive(Debug, Default, Clone, PartialEq, Eq)] +struct ProcessIdentity { + name: Option, + exe_file_name: Option, + args: Vec, +} + +/// Read a live process's name, executable and argv. +/// +/// Split out from the guard so the sysinfo refresh contract is testable on its +/// own. `System::refresh_processes` refreshes only a minimal set of fields and +/// leaves `cmd()` **empty** — with it, the `--ui-owned` check below never +/// matched, so no process was ever verified and the reap path was dead code: +/// every un-advertised lock holder became a hard lockout the user had to clear +/// by killing the process by hand. The command line has to be requested. +fn read_process_identity(pid: u32) -> Option { + use sysinfo::{Pid, ProcessRefreshKind, ProcessesToUpdate, System, UpdateKind}; + + let spid = Pid::from_u32(pid); + let mut sys = System::new(); + sys.refresh_processes_specifics( + ProcessesToUpdate::Some(&[spid]), + true, + ProcessRefreshKind::nothing() + .with_cmd(UpdateKind::Always) + .with_exe(UpdateKind::Always), + ); + let proc = sys.process(spid)?; + + Some(ProcessIdentity { + name: proc.name().to_str().map(str::to_string), + exe_file_name: proc + .exe() + .and_then(|path| path.file_name()) + .and_then(|name| name.to_str()) + .map(str::to_string), + args: proc + .cmd() + .iter() + .filter_map(|arg| arg.to_str().map(str::to_string)) + .collect(), + }) +} + +/// Whether a process looks like a daemon this UI spawned and may therefore reap. +fn is_ui_owned_okena(identity: &ProcessIdentity) -> bool { + let is_okena = |name: &String| name.starts_with("okena"); + let named_okena = + identity.name.as_ref().is_some_and(is_okena) || identity.exe_file_name.as_ref().is_some_and(is_okena); + let ui_owned = identity.args.iter().any(|arg| arg == UI_OWNED_FLAG); + named_okena && ui_owned +} + /// Reap only a process that still matches the lockfile and explicit lifecycle mode. fn kill_pid_if_ui_owned_okena(pid: u32) -> bool { if instance_lock_owner_pid() != Some(pid) || !instance_lock_is_held() { return false; } + let Some(identity) = read_process_identity(pid) else { + return false; + }; + if !is_ui_owned_okena(&identity) { + log::warn!( + "Refusing to reap pid {pid}: process is not a verified UI-owned Okena daemon ({identity:?})" + ); + return false; + } + use sysinfo::{Pid, ProcessesToUpdate, System}; let spid = Pid::from_u32(pid); let mut sys = System::new(); sys.refresh_processes(ProcessesToUpdate::Some(&[spid]), true); - if let Some(proc) = sys.process(spid) { - let is_okena = |s: &str| s.starts_with("okena"); - let name_ok = proc.name().to_str().is_some_and(is_okena); - let exe_ok = proc - .exe() - .and_then(|p| p.file_name()) - .and_then(|n| n.to_str()) - .is_some_and(is_okena); - let ui_owned = proc - .cmd() - .iter() - .any(|arg| arg.to_str().is_some_and(|arg| arg == "--ui-owned")); - if (name_ok || exe_ok) && ui_owned { - return proc.kill(); - } else { - log::warn!( - "Refusing to reap pid {pid}: process is not a verified UI-owned Okena daemon" - ); - } - } - false + sys.process(spid).is_some_and(|proc| proc.kill()) } /// Ensure a local daemon is reachable from the user's config dir, with caller- @@ -909,6 +988,68 @@ fn blocking_client_and_url( mod tests { use super::*; + fn identity(name: &str, exe: &str, args: &[&str]) -> ProcessIdentity { + ProcessIdentity { + name: Some(name.to_string()), + exe_file_name: Some(exe.to_string()), + args: args.iter().map(|arg| arg.to_string()).collect(), + } + } + + #[test] + fn ui_owned_okena_requires_both_an_okena_binary_and_the_flag() { + assert!(is_ui_owned_okena(&identity( + "okena", + "okena", + &["--headless", UI_OWNED_FLAG] + ))); + assert!( + is_ui_owned_okena(&identity("okena-daemon", "okena-daemon", &[UI_OWNED_FLAG])), + "the standalone daemon binary counts too", + ); + assert!( + !is_ui_owned_okena(&identity("okena", "okena", &["--headless"])), + "a daemon the user started themselves is not ours to kill", + ); + assert!( + !is_ui_owned_okena(&identity("zsh", "zsh", &[UI_OWNED_FLAG])), + "a recycled pid carrying the flag is still not okena", + ); + assert!( + !is_ui_owned_okena(&ProcessIdentity::default()), + "unknown process — never reap", + ); + } + + /// Pins the sysinfo contract the guard depends on. `refresh_processes` alone + /// leaves `cmd()` empty, which silently made `is_ui_owned_okena` fail for + /// every process: the reap path became unreachable and any un-advertised + /// lock holder turned into a lockout the user had to clear by hand. + #[cfg(unix)] + #[test] + fn read_process_identity_reports_the_command_line() { + let mut child = std::process::Command::new("/bin/sleep") + .arg("30") + .spawn() + .expect("spawn sleep"); + // Give the OS a moment to publish the new process. + std::thread::sleep(std::time::Duration::from_millis(300)); + + let identity = read_process_identity(child.id()); + let _ = child.kill(); + let _ = child.wait(); + + let identity = identity.expect("sysinfo should see a live child process"); + assert!( + !identity.args.is_empty(), + "command line must be populated, got {identity:?}", + ); + assert!( + identity.args.iter().any(|arg| arg == "30"), + "argv should carry the argument we spawned with, got {identity:?}", + ); + } + fn temp_dir() -> std::path::PathBuf { let dir = std::env::temp_dir().join(format!( "okena-local-test-{:?}-{}",