From d548d0b24ec61f76b5d3373cb078072d67bd1ae4 Mon Sep 17 00:00:00 2001 From: Miguel Palhas Date: Fri, 4 Sep 2026 08:40:01 +0100 Subject: [PATCH] feat(maestro): skip the tmux probe on a detected host reboot Recovery could not tell a daemon restart from a host reboot: both looked like "some sessions were not in tmux list-sessions", so the tmux probe ran unconditionally and the logs said nothing useful when it was actually a reboot. Record /proc/sys/kernel/random/boot_id in the state directory on every recover(). When it differs from the stored value, every session from the previous boot is provably dead, so the tmux probe is skipped and the second recovery pass registers them dormant directly, logging "host rebooted, sessions dormant" with the count. A missing stored value or an unreadable boot id (non-Linux, restricted /proc) falls back to today's probe-based recovery. The boot id source is injected via a BootIdSource trait so tests can simulate a changed or unreadable boot id without rebooting the machine. Refs #164 --- crates/maestro-daemon/src/boot_id.rs | 62 +++++++ crates/maestro-daemon/src/cli.rs | 6 +- crates/maestro-daemon/src/config.rs | 6 + crates/maestro-daemon/src/lib.rs | 1 + crates/maestro-daemon/src/session.rs | 95 ++++++++++- .../maestro-daemon/tests/session_manager.rs | 153 ++++++++++++++++++ 6 files changed, 318 insertions(+), 5 deletions(-) create mode 100644 crates/maestro-daemon/src/boot_id.rs diff --git a/crates/maestro-daemon/src/boot_id.rs b/crates/maestro-daemon/src/boot_id.rs new file mode 100644 index 0000000..9106815 --- /dev/null +++ b/crates/maestro-daemon/src/boot_id.rs @@ -0,0 +1,62 @@ +//! Tells a daemon restart from a host reboot (#164). +//! +//! `/proc/sys/kernel/random/boot_id` is fixed for the life of the kernel and +//! changes on every boot. [`SessionManager::recover`] compares it against the +//! value it stored on the previous run: a difference means every session from +//! before is provably dead, so the tmux probe can be skipped and those +//! sessions registered dormant directly instead of relying on tmux reporting +//! an empty list. + +use std::path::Path; + +/// Where the daemon reads the running kernel's boot id. Behind a trait so +/// tests can simulate a changed (or unreadable) boot id without rebooting the +/// machine. +pub trait BootIdSource: std::fmt::Debug + Send + Sync + 'static { + /// Current boot id, or `None` if it can't be determined (non-Linux, + /// restricted `/proc`, missing or empty file). + fn current(&self) -> Option; +} + +/// Production source: `/proc/sys/kernel/random/boot_id`. +#[derive(Debug, Default)] +pub struct ProcBootId; + +impl BootIdSource for ProcBootId { + fn current(&self) -> Option { + read_boot_id(Path::new("/proc/sys/kernel/random/boot_id")) + } +} + +fn read_boot_id(path: &Path) -> Option { + let contents = std::fs::read_to_string(path).ok()?; + let trimmed = contents.trim(); + (!trimmed.is_empty()).then(|| trimmed.to_string()) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn reads_trimmed_contents() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("boot_id"); + std::fs::write(&path, "abc-123\n").unwrap(); + assert_eq!(read_boot_id(&path).as_deref(), Some("abc-123")); + } + + #[test] + fn missing_file_returns_none() { + let dir = tempfile::tempdir().unwrap(); + assert_eq!(read_boot_id(&dir.path().join("missing")), None); + } + + #[test] + fn empty_file_returns_none() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("boot_id"); + std::fs::write(&path, "\n").unwrap(); + assert_eq!(read_boot_id(&path), None); + } +} diff --git a/crates/maestro-daemon/src/cli.rs b/crates/maestro-daemon/src/cli.rs index c61a5e9..4f6e13e 100644 --- a/crates/maestro-daemon/src/cli.rs +++ b/crates/maestro-daemon/src/cli.rs @@ -83,7 +83,11 @@ async fn run(config_override: Option) -> Result<(), Box> { .with_spawn_history(crate::spawn_history::SpawnHistory::new( config.spawn_history_path(), config.spawn_history_days, - )); + )) + .with_boot_id( + config.boot_id_path(), + std::sync::Arc::new(crate::boot_id::ProcBootId), + ); // Spawned sessions learn where to report back (#103). Derived from the // configured bind address; set before any spawn can happen. manager.set_daemon_url(format!("http://{}", config.bind_addr)); diff --git a/crates/maestro-daemon/src/config.rs b/crates/maestro-daemon/src/config.rs index ef70e73..e78e483 100644 --- a/crates/maestro-daemon/src/config.rs +++ b/crates/maestro-daemon/src/config.rs @@ -112,6 +112,12 @@ impl DaemonConfig { pub fn spawn_history_path(&self) -> PathBuf { self.state_dir.join("spawn-history.jsonl") } + + /// Where recovery records the kernel boot id it saw last (#164): + /// `/boot_id`. + pub fn boot_id_path(&self) -> PathBuf { + self.state_dir.join("boot_id") + } } /// Env-var overrides applied by [`load_config`]. Tests pass explicit diff --git a/crates/maestro-daemon/src/lib.rs b/crates/maestro-daemon/src/lib.rs index 6d67cea..c4f8031 100644 --- a/crates/maestro-daemon/src/lib.rs +++ b/crates/maestro-daemon/src/lib.rs @@ -12,6 +12,7 @@ pub mod attach; /// Naming a session after its first prompt (#131). #[cfg(feature = "haiku-autoname")] pub mod autoname; +pub mod boot_id; pub mod branches; pub mod cli; pub mod config; diff --git a/crates/maestro-daemon/src/session.rs b/crates/maestro-daemon/src/session.rs index 2cf7a0b..cc8492b 100644 --- a/crates/maestro-daemon/src/session.rs +++ b/crates/maestro-daemon/src/session.rs @@ -19,6 +19,7 @@ use maestro_protocol::{ use tokio::sync::{broadcast, Mutex}; use uuid::Uuid; +use crate::boot_id::BootIdSource; use crate::panes::Panes; use crate::pty::{pipe_pane_command, PtyBridge}; use crate::sidecar::{SidecarError, SidecarStore}; @@ -179,6 +180,25 @@ struct Inner { /// client that reconnects never collides with the registration its /// previous socket has not finished releasing. next_viewport: AtomicU64, + /// Boot-id tracking for `recover` (#164). `None` — the default — means + /// reboot detection is unwired, which behaves exactly like a boot id + /// that could never be read: always fall back to probing tmux. + boot_id: Option, +} + +/// Where recovery persists the boot id it last saw, and how it reads the +/// current one. See [`crate::boot_id`]. +struct BootIdConfig { + path: PathBuf, + source: Arc, +} + +impl std::fmt::Debug for BootIdConfig { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.debug_struct("BootIdConfig") + .field("path", &self.path) + .finish_non_exhaustive() + } } impl SessionManager { @@ -221,6 +241,7 @@ impl SessionManager { events, viewports: Mutex::new(Viewports::new()), next_viewport: AtomicU64::new(0), + boot_id: None, }), } } @@ -250,6 +271,21 @@ impl SessionManager { self } + /// Wire boot-id tracking for `recover` (#164). Builder, called once right + /// after construction like the other `with_*` methods. Without it, + /// `recover` always falls back to probing tmux — the same as a boot id + /// that could never be read. + /// + /// # Panics + /// + /// If the handle has already been cloned. + pub fn with_boot_id(mut self, path: PathBuf, source: Arc) -> Self { + let inner = Arc::get_mut(&mut self.inner) + .expect("with_boot_id must be called before the SessionManager is cloned"); + inner.boot_id = Some(BootIdConfig { path, source }); + self + } + /// Set the daemon's own HTTP base URL (#103), exported to every session /// spawned afterwards as `MAESTRO_URL`. Unlike the `with_*` builders this /// is a plain setter: the daemon runner learns its bound address after @@ -665,9 +701,21 @@ impl SessionManager { /// and start a `PtyBridge`. Sessions whose sidecar references a `cwd` /// that no longer exists are still adopted — the spec does not require /// us to repair them. + /// + /// #164: when the kernel's boot id has changed since the last run, the + /// tmux probe below is skipped entirely — a reboot kills the tmux server, + /// so every previous session is already dead and there is nothing tmux + /// could truthfully adopt. [`Self::register_dormant`] then registers all + /// of them directly. pub async fn recover(&self) -> Result { tokio::fs::create_dir_all(&self.inner.fifo_dir).await?; - let live = { + let rebooted = self.check_boot_id().await; + let live = if rebooted { + // #164: every session from the previous boot is provably dead — + // no tmux server survives a reboot — so there is nothing to gain + // from asking it and no session it could honestly report back. + Vec::new() + } else { let tmux = self.inner.tmux.clone(); tmux_blocking(move || tmux.list_sessions()).await? }; @@ -737,10 +785,39 @@ impl SessionManager { .send(CatalogEvent::SessionAdded { session }); adopted += 1; } - self.register_dormant().await; + self.register_dormant(rebooted).await; Ok(adopted) } + /// Compare the running kernel's boot id against the one recovery stored + /// last time (#164), then persist the current one for next time. Returns + /// `true` only when both are known and differ — a host reboot. + /// + /// A missing stored value (first run) or an unreadable current one + /// (non-Linux, restricted `/proc`, unwired in this build) both read as + /// "no reboot", which is today's behaviour: fall back to probing tmux. + async fn check_boot_id(&self) -> bool { + let Some(cfg) = &self.inner.boot_id else { + return false; + }; + let Some(current) = cfg.source.current() else { + return false; + }; + let stored = tokio::fs::read_to_string(&cfg.path) + .await + .ok() + .map(|s| s.trim().to_string()) + .filter(|s| !s.is_empty()); + let rebooted = stored.is_some_and(|s| s != current); + if let Some(parent) = cfg.path.parent() { + let _ = tokio::fs::create_dir_all(parent).await; + } + if let Err(e) = tokio::fs::write(&cfg.path, ¤t).await { + tracing::warn!(error = ?e, path = %cfg.path.display(), "recover: failed to persist boot id"); + } + rebooted + } + /// Second recovery pass (#162): register every sidecar tmux had nothing to /// adopt as a [`SessionStatus::Dormant`] session. /// @@ -761,7 +838,12 @@ impl SessionManager { /// nothing can be resumed in a directory that is gone, so the honest move /// is to delete the sidecar rather than register a record no operator can /// act on. A sidecar whose `cwd` still exists is kept regardless of age. - async fn register_dormant(&self) { + /// + /// `rebooted` (#164) only changes what gets logged: a detected reboot + /// names the count even when it is zero, so the operator sees that + /// recovery told a reboot from a restart instead of a log that stays + /// silent either way. + async fn register_dormant(&self, rebooted: bool) { let stored = match self.inner.sidecar.load_all() { Ok(stored) => stored, Err(e) => { @@ -812,7 +894,12 @@ impl SessionManager { registered.push(session); } } - if !registered.is_empty() { + if rebooted { + tracing::info!( + dormant = registered.len(), + "host rebooted, sessions dormant" + ); + } else if !registered.is_empty() { tracing::info!( dormant = registered.len(), "registered sessions with no live tmux as dormant" diff --git a/crates/maestro-daemon/tests/session_manager.rs b/crates/maestro-daemon/tests/session_manager.rs index e5d8c58..08db711 100644 --- a/crates/maestro-daemon/tests/session_manager.rs +++ b/crates/maestro-daemon/tests/session_manager.rs @@ -297,6 +297,159 @@ async fn recover_keeps_dormant_sidecar_when_cwd_exists() { assert_eq!(snap[0].status, SessionStatus::Dormant); } +/// #164: a fake [`maestro_daemon::boot_id::BootIdSource`] lets tests simulate +/// a changed (or unreadable) boot id without rebooting the machine. +#[derive(Debug)] +struct FakeBootId(Option); + +impl maestro_daemon::boot_id::BootIdSource for FakeBootId { + fn current(&self) -> Option { + self.0.clone() + } +} + +fn revive_with_boot_id( + fx: &DaemonFixture, + boot_id_path: PathBuf, + current: Option<&str>, +) -> maestro_daemon::SessionManager { + maestro_daemon::SessionManager::new( + Arc::new(fx.tmux.tmux.clone()), + maestro_daemon::sidecar::SidecarStore::new(fx.sidecar_dir()), + maestro_daemon::worktree::WorktreeManager::new(None), + fx.fifo_dir(), + ) + .with_boot_id( + boot_id_path, + Arc::new(FakeBootId(current.map(String::from))), + ) +} + +/// #164: a changed boot id means every session from the previous boot is +/// already dead, so recovery must not trust tmux to say so — it skips the +/// probe outright. Proven by leaving the tmux session alive: if the probe +/// ran, it would come back `Running`, not `Dormant`. +#[tokio::test] +async fn recover_skips_tmux_probe_when_boot_id_changed() { + let fx = DaemonFixture::new(); + let cwd = TempDir::new().unwrap(); + let s = fx + .manager + .create(CreateSessionRequest { + cwd: cwd.path().to_path_buf(), + command: "sleep 30".into(), + env: HashMap::new(), + metadata: HashMap::new(), + worktree: None, + }) + .await + .expect("create"); + + let boot_id_path = fx.state_dir.path().join("boot_id"); + std::fs::write(&boot_id_path, "previous-boot").unwrap(); + + // tmux session is still alive — a probe would find it if one ran. + let revived = revive_with_boot_id(&fx, boot_id_path.clone(), Some("current-boot")); + let adopted = revived.recover().await.expect("recover"); + assert_eq!(adopted, 0, "reboot detected: nothing adopted live"); + + let snap = revived.snapshot().await; + assert_eq!(snap.len(), 1); + assert_eq!(snap[0].id, s.id); + assert_eq!(snap[0].status, SessionStatus::Dormant); + + // The current boot id is now the stored one, for the next comparison. + assert_eq!( + std::fs::read_to_string(&boot_id_path).unwrap().trim(), + "current-boot" + ); +} + +/// #164: an unchanged boot id is a plain restart — recovery probes tmux as +/// before and adopts the still-live session `Running`. +#[tokio::test] +async fn recover_probes_tmux_when_boot_id_unchanged() { + let fx = DaemonFixture::new(); + let cwd = TempDir::new().unwrap(); + let s = fx + .manager + .create(CreateSessionRequest { + cwd: cwd.path().to_path_buf(), + command: "sleep 30".into(), + env: HashMap::new(), + metadata: HashMap::new(), + worktree: None, + }) + .await + .expect("create"); + + let boot_id_path = fx.state_dir.path().join("boot_id"); + std::fs::write(&boot_id_path, "same-boot").unwrap(); + + let revived = revive_with_boot_id(&fx, boot_id_path, Some("same-boot")); + let adopted = revived.recover().await.expect("recover"); + assert_eq!(adopted, 1); + let snap = revived.snapshot().await; + assert_eq!(snap[0].id, s.id); + assert_eq!(snap[0].status, SessionStatus::Running); +} + +/// #164: no stored boot id (first run ever, or an older build that never +/// wrote one) means there is nothing to compare against — falls back to +/// today's probe-based recovery rather than assuming a reboot. +#[tokio::test] +async fn recover_falls_back_to_probe_when_no_stored_boot_id() { + let fx = DaemonFixture::new(); + let cwd = TempDir::new().unwrap(); + fx.manager + .create(CreateSessionRequest { + cwd: cwd.path().to_path_buf(), + command: "sleep 30".into(), + env: HashMap::new(), + metadata: HashMap::new(), + worktree: None, + }) + .await + .expect("create"); + + let boot_id_path = fx.state_dir.path().join("boot_id"); + assert!(!boot_id_path.exists()); + + let revived = revive_with_boot_id(&fx, boot_id_path.clone(), Some("current-boot")); + let adopted = revived.recover().await.expect("recover"); + assert_eq!(adopted, 1, "no stored boot id; falls back to probing tmux"); + assert_eq!( + std::fs::read_to_string(&boot_id_path).unwrap().trim(), + "current-boot" + ); +} + +/// #164: an unreadable current boot id (non-Linux, restricted `/proc`) falls +/// back to today's behaviour rather than failing startup or assuming a +/// reboot. +#[tokio::test] +async fn recover_falls_back_to_probe_when_boot_id_unreadable() { + let fx = DaemonFixture::new(); + let cwd = TempDir::new().unwrap(); + fx.manager + .create(CreateSessionRequest { + cwd: cwd.path().to_path_buf(), + command: "sleep 30".into(), + env: HashMap::new(), + metadata: HashMap::new(), + worktree: None, + }) + .await + .expect("create"); + + let boot_id_path = fx.state_dir.path().join("boot_id"); + std::fs::write(&boot_id_path, "stored-boot").unwrap(); + + let revived = revive_with_boot_id(&fx, boot_id_path, None); + let adopted = revived.recover().await.expect("recover"); + assert_eq!(adopted, 1, "boot id unreadable; falls back to probing tmux"); +} + /// #162: dismissing a dormant session is the one thing that can be done to it /// — no tmux call to fail, and the sidecar goes with it so the next recovery /// does not resurrect it.