Fix stale daemon after upgrade causing silent CDP failures (#1134)
After upgrading agent-browser, the old daemon process keeps running. ensure_daemon() only checks socket connectivity, not version, so the new CLI silently reuses the old daemon — causing broken CDP behavior with no error or warning. Add a version sidecar file (.version) written by the daemon on startup. ensure_daemon() now compares it against the CLI's compiled version and automatically kills/restarts on mismatch. Missing version files (from pre-fix or Node.js-era daemons) are treated as mismatches so the first upgrade to this version also benefits. Fixes #1127 Co-authored-by: ctate <366502+ctate@users.noreply.github.com>
This commit is contained in:
+147
-3
@@ -118,10 +118,16 @@ fn get_pid_path(session: &str) -> PathBuf {
|
||||
get_socket_dir().join(format!("{}.pid", session))
|
||||
}
|
||||
|
||||
fn get_version_path(session: &str) -> PathBuf {
|
||||
get_socket_dir().join(format!("{}.version", session))
|
||||
}
|
||||
|
||||
/// Clean up stale socket and PID files for a session
|
||||
pub fn cleanup_stale_files(session: &str) {
|
||||
let pid_path = get_pid_path(session);
|
||||
let _ = fs::remove_file(&pid_path);
|
||||
let version_path = get_version_path(session);
|
||||
let _ = fs::remove_file(&version_path);
|
||||
let stream_path = get_socket_dir().join(format!("{}.stream", session));
|
||||
let _ = fs::remove_file(&stream_path);
|
||||
|
||||
@@ -306,6 +312,68 @@ fn apply_daemon_env(cmd: &mut Command, session: &str, opts: &DaemonOptions) {
|
||||
}
|
||||
}
|
||||
|
||||
/// Check if the running daemon's version matches this CLI binary.
|
||||
/// Returns false when the version file is missing — an unversioned daemon
|
||||
/// is most likely a stale leftover from before version tracking was added
|
||||
/// (or from the Node.js era), and silently reusing it is the exact bug
|
||||
/// this check exists to prevent. The one-time cost of an unnecessary
|
||||
/// restart on the first upgrade is preferable to silent failures.
|
||||
fn daemon_version_matches(session: &str) -> bool {
|
||||
let version_path = get_version_path(session);
|
||||
match fs::read_to_string(&version_path) {
|
||||
Ok(v) => v.trim() == env!("CARGO_PKG_VERSION"),
|
||||
Err(_) => false,
|
||||
}
|
||||
}
|
||||
|
||||
/// Kill a running daemon by reading its PID file and sending a kill signal.
|
||||
fn kill_stale_daemon(session: &str) {
|
||||
// Remove the socket first so no new connections reach the old daemon
|
||||
#[cfg(unix)]
|
||||
{
|
||||
let socket_path = get_socket_path(session);
|
||||
let _ = fs::remove_file(&socket_path);
|
||||
}
|
||||
|
||||
let pid_path = get_pid_path(session);
|
||||
if let Ok(pid_str) = fs::read_to_string(&pid_path) {
|
||||
if let Ok(pid) = pid_str.trim().parse::<u32>() {
|
||||
#[cfg(unix)]
|
||||
{
|
||||
unsafe {
|
||||
libc::kill(pid as i32, libc::SIGTERM);
|
||||
}
|
||||
// Wait up to 1s for graceful shutdown, then force-kill
|
||||
for _ in 0..10 {
|
||||
thread::sleep(Duration::from_millis(100));
|
||||
if unsafe { libc::kill(pid as i32, 0) } != 0 {
|
||||
break;
|
||||
}
|
||||
}
|
||||
// Force-kill if still alive
|
||||
if unsafe { libc::kill(pid as i32, 0) } == 0 {
|
||||
unsafe {
|
||||
libc::kill(pid as i32, libc::SIGKILL);
|
||||
}
|
||||
thread::sleep(Duration::from_millis(100));
|
||||
}
|
||||
}
|
||||
#[cfg(windows)]
|
||||
{
|
||||
let _ = Command::new("taskkill")
|
||||
.args(["/PID", &pid.to_string(), "/F"])
|
||||
.stdout(Stdio::null())
|
||||
.stderr(Stdio::null())
|
||||
.status();
|
||||
thread::sleep(Duration::from_millis(500));
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// Clean up leftover files regardless
|
||||
cleanup_stale_files(session);
|
||||
}
|
||||
|
||||
pub fn ensure_daemon(session: &str, opts: &DaemonOptions) -> Result<DaemonResult, String> {
|
||||
// Socket connectivity is the sole liveness check — no PID check — so
|
||||
// callers in a different PID namespace (e.g. unshare) can still reuse
|
||||
@@ -316,9 +384,20 @@ pub fn ensure_daemon(session: &str, opts: &DaemonOptions) -> Result<DaemonResult
|
||||
// (daemon has a 100ms shutdown delay, so we wait longer)
|
||||
thread::sleep(Duration::from_millis(150));
|
||||
if daemon_ready(session) {
|
||||
return Ok(DaemonResult {
|
||||
already_running: true,
|
||||
});
|
||||
// Check version: if the running daemon is from a different CLI
|
||||
// version (e.g. after an upgrade), kill it and start a fresh one.
|
||||
if !daemon_version_matches(session) {
|
||||
eprintln!(
|
||||
"{} Daemon version mismatch detected, restarting...",
|
||||
crate::color::warning_indicator()
|
||||
);
|
||||
kill_stale_daemon(session);
|
||||
// Fall through to spawn a new daemon below
|
||||
} else {
|
||||
return Ok(DaemonResult {
|
||||
already_running: true,
|
||||
});
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -754,4 +833,69 @@ mod tests {
|
||||
assert_eq!(get_port_for_session("work"), 51184);
|
||||
assert_eq!(get_port_for_session(""), 49152);
|
||||
}
|
||||
|
||||
// === Daemon Version Mismatch Detection Tests ===
|
||||
|
||||
#[test]
|
||||
fn test_daemon_version_matches_same_version() {
|
||||
let dir = std::env::temp_dir().join("ab-test-version-match");
|
||||
let _ = fs::create_dir_all(&dir);
|
||||
let _guard = EnvGuard::new(&["AGENT_BROWSER_SOCKET_DIR", "XDG_RUNTIME_DIR"]);
|
||||
_guard.set("AGENT_BROWSER_SOCKET_DIR", dir.to_str().unwrap());
|
||||
|
||||
let version_path = dir.join("test-session.version");
|
||||
let _ = fs::write(&version_path, env!("CARGO_PKG_VERSION"));
|
||||
|
||||
assert!(daemon_version_matches("test-session"));
|
||||
|
||||
let _ = fs::remove_file(&version_path);
|
||||
let _ = fs::remove_dir(&dir);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_daemon_version_matches_different_version() {
|
||||
let dir = std::env::temp_dir().join("ab-test-version-mismatch");
|
||||
let _ = fs::create_dir_all(&dir);
|
||||
let _guard = EnvGuard::new(&["AGENT_BROWSER_SOCKET_DIR", "XDG_RUNTIME_DIR"]);
|
||||
_guard.set("AGENT_BROWSER_SOCKET_DIR", dir.to_str().unwrap());
|
||||
|
||||
let version_path = dir.join("test-session.version");
|
||||
let _ = fs::write(&version_path, "0.0.0-old");
|
||||
|
||||
assert!(!daemon_version_matches("test-session"));
|
||||
|
||||
let _ = fs::remove_file(&version_path);
|
||||
let _ = fs::remove_dir(&dir);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_daemon_version_matches_no_file() {
|
||||
let dir = std::env::temp_dir().join("ab-test-version-nofile");
|
||||
let _ = fs::create_dir_all(&dir);
|
||||
let _guard = EnvGuard::new(&["AGENT_BROWSER_SOCKET_DIR", "XDG_RUNTIME_DIR"]);
|
||||
_guard.set("AGENT_BROWSER_SOCKET_DIR", dir.to_str().unwrap());
|
||||
|
||||
// No version file: treated as mismatch so stale pre-version-tracking
|
||||
// daemons (including Node.js era) are always restarted.
|
||||
assert!(!daemon_version_matches("test-session"));
|
||||
|
||||
let _ = fs::remove_dir(&dir);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_cleanup_stale_files_removes_version() {
|
||||
let dir = std::env::temp_dir().join("ab-test-cleanup-version");
|
||||
let _ = fs::create_dir_all(&dir);
|
||||
let _guard = EnvGuard::new(&["AGENT_BROWSER_SOCKET_DIR", "XDG_RUNTIME_DIR"]);
|
||||
_guard.set("AGENT_BROWSER_SOCKET_DIR", dir.to_str().unwrap());
|
||||
|
||||
let version_path = dir.join("test-session.version");
|
||||
let _ = fs::write(&version_path, "0.1.0");
|
||||
assert!(version_path.exists());
|
||||
|
||||
cleanup_stale_files("test-session");
|
||||
assert!(!version_path.exists());
|
||||
|
||||
let _ = fs::remove_dir(&dir);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -62,6 +62,9 @@ pub async fn run_daemon(session: &str) {
|
||||
let pid_path = socket_dir.join(format!("{}.pid", session));
|
||||
let _ = fs::write(&pid_path, process::id().to_string());
|
||||
|
||||
let version_path = socket_dir.join(format!("{}.version", session));
|
||||
let _ = fs::write(&version_path, env!("CARGO_PKG_VERSION"));
|
||||
|
||||
// On Unix the daemon listens on a Unix domain socket; on Windows it uses
|
||||
// TCP, so there is no .sock file — only a .port file written by the server.
|
||||
let socket_path = socket_dir.join(format!("{}.sock", session));
|
||||
@@ -134,6 +137,7 @@ pub async fn run_daemon(session: &str) {
|
||||
let _ = fs::remove_file(socket_dir.join(format!("{}.port", session)));
|
||||
}
|
||||
let _ = fs::remove_file(&pid_path);
|
||||
let _ = fs::remove_file(&version_path);
|
||||
let _ = fs::remove_file(&stream_path);
|
||||
let _ = fs::remove_file(socket_dir.join(format!("{}.engine", session)));
|
||||
let _ = fs::remove_file(socket_dir.join(format!("{}.provider", session)));
|
||||
|
||||
Reference in New Issue
Block a user