mirror of
https://github.com/l0ng-ai/tty7.git
synced 2026-09-21 16:02:20 +00:00
fix(daemon): sweep the shell-integration dirs a killed daemon leaves
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.
This commit is contained in:
@@ -570,6 +570,11 @@ fn run_with(registry: Arc<Registry>) -> 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();
|
||||
|
||||
|
||||
@@ -831,6 +831,59 @@ fn throwaway_dir(prefix: &str) -> Option<PathBuf> {
|
||||
Some(dir)
|
||||
}
|
||||
|
||||
/// The `<pid>` a throwaway directory's name carries, if it carries one.
|
||||
///
|
||||
/// `tty7-zdotdir-<pid>-<seq>`, 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<u32> {
|
||||
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<Injection> {
|
||||
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");
|
||||
|
||||
@@ -174,17 +174,17 @@ fn recorded_daemon_is_dead_with(recorded: Option<u32>, 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)]
|
||||
|
||||
@@ -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();
|
||||
|
||||
|
||||
Reference in New Issue
Block a user