From 6acf749930e91d615667bae729ecdf3776d03074 Mon Sep 17 00:00:00 2001 From: l0ng-ai <24760907+l0ng-ai@users.noreply.github.com> Date: Sun, 16 Aug 2026 07:21:09 +0800 Subject: [PATCH] fix(daemon): sweep the shell-integration dirs a killed daemon leaves MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every zsh pane gets a throwaway ZDOTDIR under the temp directory, four redirector files in it, and the pane's own teardown removes it — stop a server cleanly and it leaves none. A server that is killed never runs that teardown, and nothing ever goes back to look: this machine had 3,850 of them, from months of crashes and `kill -9`s. Same shape as the socket a killed daemon used to leave behind, and the same answer: a later startup is the only thing in a position to notice, so it sweeps beside the endpoint cleanup. Timid on purpose. A directory goes only when the name is exactly ours, the pid in it parses, and that pid is not a live process — so a running daemon's directories are never touched, and a pid since reused by something else just waits for another day. Verified after the sweep: 3,850 down to 14, and every one of those 14 belongs to a live process, the installed tty7.app's daemon among them. `process_alive` was wrong about that, which the test caught. `kill(pid, 0) == 0` is only half the answer: `EPERM` means the process exists and belongs to someone else, and reading that as dead would have had the sweep delete a live owner's directory. Both callers wanted the other reading — one decides whether to clean up after a daemon, the other whether to delete its files. The two tests that bind an endpoint now hold a lock while they do. `set_config_dir` is first-wins, so every test in the process shares one socket path, and the one added here made the pair flaky together while each passed alone. --- crates/tty7-core/src/daemon/server.rs | 5 ++ .../tty7-core/src/daemon/shell_integration.rs | 87 +++++++++++++++++++ crates/tty7-core/src/daemon/spawn.rs | 19 +++- crates/tty7-core/src/daemon/transport.rs | 12 +++ 4 files changed, 119 insertions(+), 4 deletions(-) diff --git a/crates/tty7-core/src/daemon/server.rs b/crates/tty7-core/src/daemon/server.rs index 37156b4b..04187a2f 100644 --- a/crates/tty7-core/src/daemon/server.rs +++ b/crates/tty7-core/src/daemon/server.rs @@ -570,6 +570,11 @@ fn run_with(registry: Arc) -> anyhow::Result<()> { let listener = transport::bind()?; log::info!("daemon listening on {}", transport::endpoint_display()); + // Beside the endpoint above, for the same reason: a daemon that was killed + // could not tidy up after itself, and a later startup is the only thing + // that ever comes back to the temp directory to look. + crate::daemon::shell_integration::sweep_dead_zdotdirs(); + #[cfg(windows)] report_conpty_host(); diff --git a/crates/tty7-core/src/daemon/shell_integration.rs b/crates/tty7-core/src/daemon/shell_integration.rs index 9b0664c7..1723edc7 100644 --- a/crates/tty7-core/src/daemon/shell_integration.rs +++ b/crates/tty7-core/src/daemon/shell_integration.rs @@ -831,6 +831,59 @@ fn throwaway_dir(prefix: &str) -> Option { Some(dir) } +/// The `` a throwaway directory's name carries, if it carries one. +/// +/// `tty7-zdotdir--`, so the owner is the text between the prefix and +/// the first `-` after it. Anything else — the `tty7-zdotdir-wsl` a WSL route +/// writes inside its own directory, a name someone else happened to choose — +/// answers `None` and is left alone. +fn zdotdir_owner(name: &str) -> Option { + name.strip_prefix(ZDOTDIR_PREFIX)? + .split_once('-')? + .0 + .parse() + .ok() +} + +/// Remove the throwaway ZDOTDIRs left by daemons that are gone. +/// +/// A pane's teardown removes its own, so a daemon that stops cleanly leaves +/// none. One that is killed never runs that teardown, and its directories then +/// stay in the temp dir for good — four files each, and nothing ever looks at +/// them again. This machine had 3,850 of them from months of crashes and +/// `kill -9`s, which is the same shape as the socket a killed daemon used to +/// leave: litter only a later startup is in a position to notice. +/// +/// Deliberately timid. It removes a directory only when the name is exactly +/// ours, the pid parses, and that pid is not a live process — so a running +/// daemon's directories are never touched, and a pid that has since been +/// reused just means the directory waits for another day. On a platform with +/// no cheap liveness query [`daemon_process_alive`](crate::daemon::spawn::daemon_process_alive) +/// answers "alive", and this does nothing at all. +pub(crate) fn sweep_dead_zdotdirs() { + let Ok(entries) = std::fs::read_dir(std::env::temp_dir()) else { + return; + }; + let mine = std::process::id(); + let mut swept = 0usize; + for entry in entries.flatten() { + let name = entry.file_name(); + let Some(name) = name.to_str() else { continue }; + let Some(owner) = zdotdir_owner(name) else { + continue; + }; + if owner == mine || crate::daemon::spawn::daemon_process_alive(owner) { + continue; + } + if entry.path().is_dir() && std::fs::remove_dir_all(entry.path()).is_ok() { + swept += 1; + } + } + if swept > 0 { + log::info!("swept {swept} shell-integration directories left by daemons that are gone"); + } +} + fn setup_zsh() -> Option { let dir = throwaway_dir(ZDOTDIR_PREFIX)?; for (name, contents) in zsh_redirectors() { @@ -2764,6 +2817,40 @@ mod tests { String::from_utf16(&units).expect("valid UTF-16LE") } + /// Only our own directories, and only ones with a pid in them. + /// + /// The sweep deletes, so what it declines to match matters more than what + /// it matches. `tty7-zdotdir-wsl` is a real name this module writes, and a + /// pid is the one thing that makes a directory safe to judge. + #[test] + fn only_a_named_pid_makes_a_directory_ours_to_sweep() { + assert_eq!(zdotdir_owner("tty7-zdotdir-4242-0"), Some(4242)); + assert_eq!(zdotdir_owner("tty7-zdotdir-1-17"), Some(1)); + assert_eq!(zdotdir_owner("tty7-zdotdir-wsl"), None); + assert_eq!(zdotdir_owner("tty7-zdotdir-4242"), None, "no seq, no pid"); + assert_eq!(zdotdir_owner("tty7-covtest-4242-0"), None, "another prefix"); + assert_eq!(zdotdir_owner("something-else"), None); + assert_eq!(zdotdir_owner(".hidden"), None); + } + + /// A live owner's directory is left where it is. + #[test] + fn the_sweep_spares_a_living_daemon() { + let mine = + std::env::temp_dir().join(format!("{ZDOTDIR_PREFIX}{}-sweeptest", std::process::id())); + let init = std::env::temp_dir().join(format!("{ZDOTDIR_PREFIX}1-sweeptest")); + std::fs::create_dir_all(&mine).expect("one owned by this very process"); + std::fs::create_dir_all(&init).expect("one owned by pid 1, which outlives everything"); + + sweep_dead_zdotdirs(); + + let (kept_mine, kept_init) = (mine.is_dir(), init.is_dir()); + std::fs::remove_dir_all(&mine).ok(); + std::fs::remove_dir_all(&init).ok(); + assert!(kept_mine, "the sweep took the running process's own"); + assert!(kept_init, "the sweep took a live owner's"); + } + #[test] fn throwaway_dir_is_unique_per_call() { let a = throwaway_dir("tty7-test-").expect("dir a"); diff --git a/crates/tty7-core/src/daemon/spawn.rs b/crates/tty7-core/src/daemon/spawn.rs index ae597029..87df60ad 100644 --- a/crates/tty7-core/src/daemon/spawn.rs +++ b/crates/tty7-core/src/daemon/spawn.rs @@ -174,17 +174,17 @@ fn recorded_daemon_is_dead_with(recorded: Option, alive: impl Fn(u32) -> bo } #[cfg(windows)] -fn daemon_process_alive(pid: u32) -> bool { +pub(crate) fn daemon_process_alive(pid: u32) -> bool { !crate::daemon::winproc::wait_for_exit(pid, Duration::ZERO) } #[cfg(any(target_os = "macos", target_os = "linux"))] -fn daemon_process_alive(pid: u32) -> bool { +pub(crate) fn daemon_process_alive(pid: u32) -> bool { process_alive(pid as libc::pid_t) } #[cfg(not(any(target_os = "macos", target_os = "linux", windows)))] -fn daemon_process_alive(_pid: u32) -> bool { +pub(crate) fn daemon_process_alive(_pid: u32) -> bool { // No cheap liveness query on this platform: assume alive, which keeps // the TCP probe as the authority. true @@ -571,9 +571,20 @@ fn signal_and_await_exit(pid: libc::pid_t, sig: libc::c_int, timeout: Duration) wait_for_recorded_exit(pid as u32, timeout) } +/// Whether `pid` names a process that exists. +/// +/// Signal 0 asks the question without delivering anything, and there are two +/// ways it can answer yes: `0` for a process we may signal, and `EPERM` for +/// one that exists but belongs to someone else. Reading only the first — +/// `kill(pid, 0) == 0` — calls another user's running process dead, which is +/// the wrong direction for every caller here: one decides whether to clean up +/// after a daemon, and the other whether to delete a directory it owns. #[cfg(any(target_os = "macos", target_os = "linux"))] fn process_alive(pid: libc::pid_t) -> bool { - unsafe { libc::kill(pid, 0) == 0 } + if unsafe { libc::kill(pid, 0) } == 0 { + return true; + } + std::io::Error::last_os_error().kind() == std::io::ErrorKind::PermissionDenied } #[cfg(windows)] diff --git a/crates/tty7-core/src/daemon/transport.rs b/crates/tty7-core/src/daemon/transport.rs index 353b5bd1..35032852 100644 --- a/crates/tty7-core/src/daemon/transport.rs +++ b/crates/tty7-core/src/daemon/transport.rs @@ -141,6 +141,16 @@ mod tests { use super::*; use std::path::PathBuf; + /// The endpoint is derived from the config dir, and `set_config_dir` is + /// first-wins, so every test in this process binds the *same* socket path. + /// Two that bind therefore cannot run at once — this is the lock that + /// keeps them apart, as `singleton`'s tests do for the same reason. + static ENDPOINT: std::sync::Mutex<()> = std::sync::Mutex::new(()); + + fn hold_endpoint() -> std::sync::MutexGuard<'static, ()> { + ENDPOINT.lock().unwrap_or_else(|e| e.into_inner()) + } + fn pin_config_dir() { let dir = std::env::temp_dir().join(format!("tty7-covtest-{}", std::process::id())); std::fs::create_dir_all(&dir).ok(); @@ -149,6 +159,7 @@ mod tests { #[test] fn endpoint_lifecycle_bind_connect_and_clear() { + let _endpoint = hold_endpoint(); pin_config_dir(); remove_stale_endpoint(); assert!(!endpoint_exists(), "no endpoint before bind"); @@ -177,6 +188,7 @@ mod tests { /// was load-bearing, and a `kill -9` then left the daemon unable to start. #[test] fn a_leftover_socket_file_blocks_the_next_bind_until_it_is_removed() { + let _endpoint = hold_endpoint(); pin_config_dir(); remove_stale_endpoint();