diff --git a/crates/tty7-core/src/daemon/server.rs b/crates/tty7-core/src/daemon/server.rs index f671874f..37156b4b 100644 --- a/crates/tty7-core/src/daemon/server.rs +++ b/crates/tty7-core/src/daemon/server.rs @@ -543,23 +543,28 @@ fn report_conpty_host() { fn run_with(registry: Arc) -> anyhow::Result<()> { crate::daemon::control::server_started(); - // A stale daemon.port whose recorded daemon is gone cannot belong to a - // live server: skip the probe (which would pay the OS's refusal delay on - // the dead port) and let the bind below overwrite the file. A live - // recorded daemon still gets the connect — the singleton seat is held by - // this process, so it can only be a foreign server worth refusing. - if transport::endpoint_exists() && !crate::daemon::spawn::recorded_daemon_is_dead() { - match transport::connect() { - Ok(_) => { - anyhow::bail!( - "daemon already running at {}", - transport::endpoint_display() - ); - } - Err(_) => { - transport::remove_stale_endpoint(); - } + // A stale endpoint whose recorded daemon is gone cannot belong to a live + // server, so there is nothing worth asking it: skip the probe, which would + // otherwise pay the OS's refusal delay on a dead port. A live recorded + // daemon still gets the connect — the singleton seat is held by this + // process, so anything answering is a foreign server worth refusing. + // + // The removal is not part of that shortcut, though, and used to be: the + // endpoint on unix is a socket *file*, and `bind` on a path that exists + // fails with EADDRINUSE rather than replacing it, where a Windows + // daemon.port is a recorded number the next bind simply overwrites. So the + // one case that most needs the unlink — the recorded daemon gone, which is + // every unclean death: SIGKILL, OOM, a lost machine — was the case that + // skipped it, and the server could not start again until somebody deleted + // a socket out of the temp dir by hand. + if transport::endpoint_exists() { + if !crate::daemon::spawn::recorded_daemon_is_dead() && transport::connect().is_ok() { + anyhow::bail!( + "daemon already running at {}", + transport::endpoint_display() + ); } + transport::remove_stale_endpoint(); } let listener = transport::bind()?; diff --git a/crates/tty7-core/src/daemon/transport.rs b/crates/tty7-core/src/daemon/transport.rs index fdefd3f5..353b5bd1 100644 --- a/crates/tty7-core/src/daemon/transport.rs +++ b/crates/tty7-core/src/daemon/transport.rs @@ -167,6 +167,38 @@ mod tests { assert!(!endpoint_exists(), "endpoint cleared after removal"); } + /// A socket file left behind by a killed daemon blocks the next `bind`. + /// + /// This is the fact `run_with` turns on: a unix endpoint is a *file*, and + /// binding a path that exists fails rather than replacing it, so the + /// startup path has to unlink a dead predecessor's socket itself. Windows + /// records a port instead, which the next bind simply overwrites — that + /// difference is what let the unlink get skipped on the one path where it + /// 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() { + pin_config_dir(); + remove_stale_endpoint(); + + let dead = bind().expect("first bind"); + drop(dead); // as a killed daemon leaves it: file on disk, nobody listening + assert!(endpoint_exists(), "SIGKILL leaves the socket file behind"); + assert!( + connect().is_err(), + "nothing is listening, so the path is provably stale" + ); + + assert!( + bind().is_err(), + "binding an existing socket path fails — this is the whole bug" + ); + + remove_stale_endpoint(); + let fresh = bind().expect("bind succeeds once the stale file is gone"); + drop(fresh); + remove_stale_endpoint(); + } + #[test] fn socket_path_stays_in_config_dir_when_it_fits() { let dir = std::path::PathBuf::from("/tmp/tty7-short");