diff --git a/crates/cli/src/app.rs b/crates/cli/src/app.rs index d8e8ada0..e9ed09f9 100644 --- a/crates/cli/src/app.rs +++ b/crates/cli/src/app.rs @@ -4396,7 +4396,10 @@ impl App { "agentd" => { // Subcommand dispatch: // - // /agentd restart → daemon.restart (exec self) + // /agentd restart [binary path] + // → daemon.restart; exec the daemon's own binary, + // or the given one (e.g. a freshly-built worktree + // binary). The path is validated daemon-side. // // Other subcommands are reserved for future use // (e.g. `/agentd info` to print build version). The @@ -4405,10 +4408,13 @@ impl App { // observes that as a "daemon disconnected" status // and the user must re-run `agent` to reconnect // (auto-reconnect is follow-up work, see issue #90). - let sub = arg.trim(); + let mut parts = arg.trim().splitn(2, char::is_whitespace); + let sub = parts.next().unwrap_or(""); + let rest = parts.next().unwrap_or("").trim(); match sub { "restart" => { - match self.client.daemon_restart().await { + let exe = (!rest.is_empty()).then(|| rest.to_string()); + match self.client.daemon_restart(exe).await { Ok(r) => self.set_status(format!( "agentd: restart requested (exe={}, pid={}) — reconnect when ready", r.exe, r.pid diff --git a/crates/client/src/lib.rs b/crates/client/src/lib.rs index 8dab1e63..eb7a13d7 100644 --- a/crates/client/src/lib.rs +++ b/crates/client/src/lib.rs @@ -248,9 +248,17 @@ impl Client { /// the recv side with "broken pipe". Callers should treat any /// reply (Ok or `BrokenPipe`-style error) as "restart in flight" /// and re-attempt connect with backoff. - pub async fn daemon_restart(&self) -> Result { - self.request(ipc_method::DAEMON_RESTART, &serde_json::Value::Null) - .await + /// `exe: Some(path)` execs that binary instead of the daemon's own + /// (e.g. a freshly-built one); `None` re-execs in place. + pub async fn daemon_restart( + &self, + exe: Option, + ) -> Result { + self.request( + ipc_method::DAEMON_RESTART, + &agentd_protocol::DaemonRestartParams { exe }, + ) + .await } /// Dev-only: point the daemon's web server at a directory of assets /// (or `None` to revert to embedded). No-op on release daemons. diff --git a/crates/daemon/src/server.rs b/crates/daemon/src/server.rs index 58a3ae53..0f3fa7de 100644 --- a/crates/daemon/src/server.rs +++ b/crates/daemon/src/server.rs @@ -1219,12 +1219,17 @@ async fn dispatch( } m if m == ipc_method::DAEMON_RESTART => { // Hand off to main's `tokio::select!` arm which calls - // `exec()` on the resolved current_exe. The reply - // races the kernel: we send it back here, then the - // IPC socket closes when exec() replaces the process - // image. Clients detect that as a disconnect and - // reconnect. - match manager.request_daemon_restart() { + // `exec()` on the resolved exe (the daemon's own binary, or + // the caller-supplied `exe` path). The reply races the + // kernel: we send it back here, then the IPC socket closes + // when exec() replaces the process image. Clients detect + // that as a disconnect and reconnect. + // Params are optional; older clients send `null` (no exe + // override), which fails to deserialize — treat that as None. + let exe = parse_params::(req.params.clone()) + .ok() + .and_then(|p| p.exe); + match manager.request_daemon_restart(exe.map(std::path::PathBuf::from)) { Ok(cmd) => { // Cloudflared survives the daemon's exec() // because it's spawned in a separate process diff --git a/crates/daemon/src/session.rs b/crates/daemon/src/session.rs index 390b0b27..ca0d9e48 100644 --- a/crates/daemon/src/session.rs +++ b/crates/daemon/src/session.rs @@ -405,6 +405,29 @@ pub fn capture_startup_exe() { } } +/// Validate a caller-supplied restart binary: resolve it to an absolute +/// path (relative paths resolve against the *daemon's* cwd), confirm it +/// exists, is a regular file, and is executable. Returns the canonical +/// path to exec, or an error that's surfaced to the caller so a typo +/// never leaves the daemon trying to exec() a missing binary. +fn validate_restart_exe(path: &std::path::Path) -> Result { + let abs = std::fs::canonicalize(path) + .with_context(|| format!("restart binary not found: {}", path.display()))?; + let meta = std::fs::metadata(&abs) + .with_context(|| format!("cannot stat restart binary: {}", abs.display()))?; + if !meta.is_file() { + anyhow::bail!("restart binary is not a file: {}", abs.display()); + } + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + if meta.permissions().mode() & 0o111 == 0 { + anyhow::bail!("restart binary is not executable: {}", abs.display()); + } + } + Ok(abs) +} + /// Best exe path for re-`exec()` on restart: the startup-captured /// path if available, else `current_exe()` with any trailing /// `" (deleted)"` marker stripped (defensive — the startup capture @@ -588,8 +611,13 @@ impl SessionManager { /// the exe path can't be resolved or the receiver was dropped /// (which shouldn't happen — main holds it for the daemon's /// lifetime). - pub fn request_daemon_restart(&self) -> Result { - let exe = restart_exe_path().context("resolve restart exe")?; + pub fn request_daemon_restart(&self, exe_override: Option) -> Result { + let exe = match exe_override { + // Validate a caller-supplied binary BEFORE tearing the + // daemon down — exec()ing a bad path would never come back. + Some(p) => validate_restart_exe(&p)?, + None => restart_exe_path().context("resolve restart exe")?, + }; let args: Vec = std::env::args().skip(1).collect(); let cmd = RestartCommand { exe, args }; self.restart_tx @@ -2871,6 +2899,32 @@ mod tests { use super::*; use agentd_protocol::{Capabilities, PtySize}; + #[test] + fn validate_restart_exe_accepts_executable_rejects_bad_paths() { + use std::os::unix::fs::PermissionsExt; + let dir = tempfile::tempdir().unwrap(); + + // An executable file → returns the canonical (absolute) path. + let good = dir.path().join("agentd"); + std::fs::write(&good, b"#!/bin/sh\n").unwrap(); + std::fs::set_permissions(&good, std::fs::Permissions::from_mode(0o755)).unwrap(); + let resolved = validate_restart_exe(&good).expect("executable accepted"); + assert!(resolved.is_absolute()); + assert_eq!(resolved, std::fs::canonicalize(&good).unwrap()); + + // Missing path → error. + assert!(validate_restart_exe(&dir.path().join("nope")).is_err()); + + // A directory → error (not a regular file). + assert!(validate_restart_exe(dir.path()).is_err()); + + // A non-executable file → error. + let plain = dir.path().join("plain"); + std::fs::write(&plain, b"data").unwrap(); + std::fs::set_permissions(&plain, std::fs::Permissions::from_mode(0o644)).unwrap(); + assert!(validate_restart_exe(&plain).is_err()); + } + #[test] fn startup_resume_retries_errored_sessions() { assert!(should_resume_on_startup(SessionState::Pending)); diff --git a/crates/e2e/tests/restart.rs b/crates/e2e/tests/restart.rs index 72e47469..06e0e5ce 100644 --- a/crates/e2e/tests/restart.rs +++ b/crates/e2e/tests/restart.rs @@ -47,7 +47,7 @@ async fn restart_reloads_updated_binary() { // Trigger the restart. The reply races the exec() — either we // get it or the socket closes mid-flight; both mean "restart // in progress". - let _ = d.client.daemon_restart().await; + let _ = d.client.daemon_restart(None).await; // New daemon must come back up on the same socket. let client = d @@ -109,7 +109,7 @@ async fn tui_auto_reconnects_after_restart() { .expect("modeline never rendered"); // Restart the daemon out from under the TUI. - let _ = d.client.daemon_restart().await; + let _ = d.client.daemon_restart(None).await; // The TUI sets "reconnected to daemon" only from its // successful-reconnect path (crates/cli/src/app.rs). Seeing @@ -179,7 +179,7 @@ async fn web_client_reconnects_to_same_url_after_restart() { // Restart the daemon. Token + port + password are persisted in // runtime/remote.json and adopted by the new daemon, so the // URL stays valid. - let _ = d.client.daemon_restart().await; + let _ = d.client.daemon_restart(None).await; // The WS drops on the daemon exec(). Best-effort confirm we // observed it leave "open" (it can recover fast, so don't fail diff --git a/crates/protocol/src/lib.rs b/crates/protocol/src/lib.rs index 8df443b0..e6ed1af2 100644 --- a/crates/protocol/src/lib.rs +++ b/crates/protocol/src/lib.rs @@ -1085,6 +1085,17 @@ pub struct RemoteStopResult { pub was_running: bool, } +/// Params for `daemon.restart`. `exe: None` re-execs the daemon's own +/// binary (the upgrade-in-place case); `Some(path)` execs a different +/// binary instead — e.g. a freshly-built one from a worktree. The path +/// is validated (must exist + be executable) before the restart fires, +/// so a typo returns an error rather than bricking the daemon. +#[derive(Debug, Clone, Default, Serialize, Deserialize)] +pub struct DaemonRestartParams { + #[serde(default, skip_serializing_if = "Option::is_none")] + pub exe: Option, +} + /// Result of `daemon.restart`. Echoed back to the caller right /// before the daemon exec()s itself — clients see this reply, then /// observe the IPC socket close as the new process replaces the