diff --git a/crates/maestro-daemon/src/session.rs b/crates/maestro-daemon/src/session.rs index 9edd43c..910894b 100644 --- a/crates/maestro-daemon/src/session.rs +++ b/crates/maestro-daemon/src/session.rs @@ -441,21 +441,8 @@ impl SessionManager { ), } }; - // The probe runs before anything is torn down: both refusing (#79) - // and recording the retention (#81) must see the tree as it stands, - // and a refused kill leaves the session running as the operator left - // it. let probe = match &worktree { - Some((_, path)) => { - let path = path.clone(); - Some( - tmux_blocking(move || crate::worktree_probe::in_the_way(&path)) - .await - .map_err(|e| { - SessionError::Worktree(crate::worktree::WorktreeError::from(e)) - })?, - ) - } + Some(wt) => Some(probe_worktree(wt).await?), None => None, }; let guarded = probe.as_ref().is_some_and(|p| !p.is_clear()); @@ -463,29 +450,15 @@ impl SessionManager { return Err(SessionError::KillGuarded(probe.expect("guarded ⇒ probed"))); } - // Record the retention BEFORE tearing down: a failed store write then - // leaves the session running and every artifact untouched, instead of - // ending a session whose work outlives it unrecorded. let mut retained = None; if guarded { - let store = - self.inner.retained.clone().ok_or_else(|| { - SessionError::RetainFailed("no retained store is wired".into()) - })?; - let (base_repo, path) = worktree.as_ref().expect("guarded ⇒ worktree"); - let branch = known_branch - .or_else(|| crate::worktree_probe::current_branch(path)) - .unwrap_or_default(); - let entry = { - let store = store.clone(); - let base_repo = base_repo.clone(); - let path = path.clone(); - tmux_blocking(move || { - store.retain(&path, &base_repo, &branch, RetainedReason::ForceKilled) - }) - .await - .map_err(|e| SessionError::RetainFailed(e.to_string()))? - }; + let entry = record_retention( + &self.inner, + worktree.as_ref().expect("guarded ⇒ worktree"), + known_branch, + RetainedReason::ForceKilled, + ) + .await?; retained = Some((entry, probe.expect("guarded ⇒ probed"))); } @@ -671,6 +644,49 @@ where .expect("tmux blocking task panicked") } +/// What would be lost if this session's worktree were removed right now. +/// +/// The single [`crate::worktree_probe::in_the_way`] call site behind both +/// endings of a session — the kill guard (#79) and the natural-exit +/// classifier (#82) — so neither can grow its own answer. Runs on the +/// blocking pool; callers invoke it before anything is torn down, because +/// refusing (#79), recording a retention (#81) and classifying an exit (#82) +/// must all see the tree as it stands. +async fn probe_worktree( + (_, path): &(PathBuf, PathBuf), +) -> Result { + let path = path.clone(); + tmux_blocking(move || crate::worktree_probe::in_the_way(&path)) + .await + .map_err(|e| SessionError::Worktree(crate::worktree::WorktreeError::from(e))) +} + +/// Record a guarded worktree into the retained store with `reason`, BEFORE +/// the caller tears anything down: a failed write then leaves every artifact +/// untouched instead of destroying work nothing records anymore. Shared by +/// the forced kill (`ForceKilled`, #81) and the natural exit +/// (`ExitedWithUnsavedWork`, #82) so the two paths cannot drift apart. +async fn record_retention( + inner: &Arc, + (base_repo, path): &(PathBuf, PathBuf), + known_branch: Option, + reason: RetainedReason, +) -> Result { + let store = inner + .retained + .clone() + .ok_or_else(|| SessionError::RetainFailed("no retained store is wired".into()))?; + // Sessions adopted from a sidecar carry no branch; ask the tree itself. + let branch = known_branch + .or_else(|| crate::worktree_probe::current_branch(path)) + .unwrap_or_default(); + let base_repo = base_repo.clone(); + let path = path.clone(); + tmux_blocking(move || store.retain(&path, &base_repo, &branch, reason)) + .await + .map_err(|e| SessionError::RetainFailed(e.to_string())) +} + /// One pass of the liveness watcher: diff the in-memory session set against /// the tmux-reported one, and reap anything we still track but tmux has lost. async fn run_liveness_pass(inner: &Arc) { @@ -794,17 +810,23 @@ async fn run_activity_pass(inner: &Arc) { } /// Mark a session whose tmux session has disappeared as [`SessionStatus::Dead`] -/// and announce it, keeping everything else intact. +/// and settle its worktree per the natural-exit rule (#82). /// /// The record, its [`PtyBridge`] and its sidecar all survive, so a client can /// still attach and read the replay buffer — the last thing a crashed agent -/// printed. Artifacts are only released when the operator dismisses the session -/// via [`SessionManager::kill`]. +/// printed. The worktree, though, is settled right away: a pristine tree is +/// removed, anything else is retained into the store with reason +/// `exited-with-unsaved-work` (#82) — nobody is waiting for an answer here, +/// so there is nothing to refuse. Once settled, the session drops its +/// worktree reference so a later dismissal neither re-probes nor retains +/// twice. When settling is impossible (probe or store failure) nothing is +/// destroyed: the tree stays attached to the dead record for a later forced +/// dismissal (#81). /// /// Dead sessions do not survive a daemon restart: [`SessionManager::recover`] /// adopts only what tmux still holds. async fn mark_session_dead(inner: &Arc, id: Uuid) { - let updated = { + let flipped = { let mut sessions = inner.sessions.lock().await; match sessions.get_mut(&id) { Some(record) if record.session.status == SessionStatus::Running => { @@ -812,19 +834,111 @@ async fn mark_session_dead(inner: &Arc, id: Uuid) { // Release anyone attached; the scrollback survives for a later // attach. record.bridge.seal(); - Some(record.session.clone()) + let session = record.session.clone(); + let worktree = record + .worktree_base_repo + .clone() + .zip(record.worktree_path.clone()); + let branch = record.worktree_branch.clone(); + Some((session, worktree, branch)) } _ => None, } }; - let Some(session) = updated else { + let Some((mut session, worktree, branch)) = flipped else { return; }; tracing::info!(%id, "session vanished from tmux — marking dead"); + + if let Some(wt) = &worktree { + if settle_worktree_on_exit(inner, id, wt, branch).await { + session.worktree = None; + let mut sessions = inner.sessions.lock().await; + if let Some(record) = sessions.get_mut(&id) { + record.session.worktree = None; + record.worktree_path = None; + record.worktree_base_repo = None; + record.worktree_branch = None; + } + } + } + let _ = inner.sidecar.write(&session); let _ = inner.events.send(CatalogEvent::SessionUpdated { session }); } +/// Apply the exit rule (#82) to a vanished session's worktree: probe what +/// would be lost through the kill's own predicate, then remove a pristine +/// tree or retain anything else under `exited-with-unsaved-work` — recorded +/// before anything is torn down, exactly as the forced kill does (#81). A +/// removal that fails falls back to retaining, since whatever blocked it is +/// by definition something worth keeping. +/// +/// Returns whether the worktree was fully accounted for. When it wasn't, +/// nothing was destroyed and the caller keeps the reference on the record. +async fn settle_worktree_on_exit( + inner: &Arc, + id: Uuid, + (base_repo, path): &(PathBuf, PathBuf), + known_branch: Option, +) -> bool { + let probe = match probe_worktree(&(base_repo.clone(), path.clone())).await { + Ok(probe) => probe, + Err(e) => { + tracing::error!( + %id, + path = %path.display(), + error = ?e, + "natural exit: could not probe the worktree; leaving it untouched" + ); + return false; + } + }; + if probe.is_clear() { + match inner.worktree_mgr.remove(base_repo, path) { + Ok(()) => { + tracing::info!(%id, path = %path.display(), "natural exit: pristine worktree removed"); + return true; + } + Err(e) => { + tracing::warn!( + %id, + path = %path.display(), + error = ?e, + "natural exit: removal failed; falling back to retention" + ); + } + } + } + match record_retention( + inner, + &(base_repo.clone(), path.clone()), + known_branch, + RetainedReason::ExitedWithUnsavedWork, + ) + .await + { + Ok(entry) => { + tracing::info!( + %id, + path = %path.display(), + entry = %entry.id, + "natural exit: worktree retained with unsaved work" + ); + true + } + Err(e) => { + tracing::error!( + %id, + path = %path.display(), + error = ?e, + "natural exit: could not record the retained worktree; leaving it untouched" + ); + false + } + } +} + /// Tear down a session's artifacts after it has already been removed from the /// live map. Shared by [`SessionManager::kill`] and [`reap_dead_session`], /// which previously hand-rolled the same five-step choreography and only diff --git a/crates/maestro-daemon/tests/session_manager.rs b/crates/maestro-daemon/tests/session_manager.rs index 234421c..9d8a19a 100644 --- a/crates/maestro-daemon/tests/session_manager.rs +++ b/crates/maestro-daemon/tests/session_manager.rs @@ -8,7 +8,9 @@ use std::process::Command; use std::sync::Arc; use std::time::Duration; -use common::{init_bare_repo, init_repo, push_all_to_origin, wait_for_async, DaemonFixture}; +use common::{ + init_bare_repo, init_repo, push_all_to_origin, wait_for_async, DaemonFixture, TmuxFixture, +}; use maestro_daemon::SessionError; use maestro_protocol::CreateSessionRequest; use maestro_protocol::{CatalogEvent, SessionStatus, WorktreeSpec}; @@ -668,6 +670,191 @@ async fn killing_an_ignored_only_worktree_succeeds() { assert!(!fx.manager.has(s.id).await); } +/// Natural exit (#82): a session that ends on its own never reaches the kill +/// path, so the watcher applies the kill's rule itself. A worktree holding +/// unsaved work is RETAINED into the store with reason +/// `exited-with-unsaved-work`; the session stays in the catalog marked dead +/// until dismissed — and dismissing it afterwards neither refuses nor +/// retains a second time. +#[tokio::test] +async fn natural_exit_of_a_guarded_worktree_retains_it_in_the_store() { + let fx = DaemonFixture::new(); + let repo = TempDir::new().unwrap(); + init_repo(repo.path()); + let wt_path = repo.path().join("worktrees").join("feat").join("exited"); + + let s = fx + .manager + .create(CreateSessionRequest { + cwd: repo.path().to_path_buf(), + command: "sleep 30".into(), + env: HashMap::new(), + metadata: HashMap::new(), + worktree: Some(WorktreeSpec { + base_repo: repo.path().to_path_buf(), + branch: "feat/exited".into(), + cleanup_on_kill: true, + create_branch: true, + }), + }) + .await + .expect("create"); + std::fs::write(wt_path.join("README"), b"unsaved edits\n").unwrap(); + + // The command finishes / the operator types exit: tmux loses the session. + fx.tmux.tmux.kill_session(s.id).expect("kill tmux directly"); + fx.manager.start_liveness_watcher(Duration::from_millis(50)); + wait_for_async(Duration::from_secs(3), || async { + fx.manager.status(s.id).await == Some(SessionStatus::Dead) + }) + .await; + + // The tree would have blocked a kill, so the exit kept it... + assert!( + wt_path.exists(), + "a dirty worktree must be retained on natural exit" + ); + // ...in the store, under the exit reason. + let all = fx.retained_store().load_all().expect("load_all"); + assert_eq!(all.len(), 1); + assert_eq!(all[0].path, wt_path); + assert_eq!(all[0].branch, "feat/exited"); + assert_eq!( + all[0].reason, + maestro_protocol::RetainedReason::ExitedWithUnsavedWork + ); + + // The session itself stays until dismissed; its worktree reference is + // settled, so dismissal is neither refused nor retained twice. + // (`Dead` is observable a moment before the settle step finishes.) + wait_for_async(Duration::from_secs(3), || async { + fx.manager.snapshot().await[0].worktree.is_none() + }) + .await; + let snap = fx.manager.snapshot().await; + assert_eq!(snap.len(), 1); + assert_eq!(snap[0].worktree, None); + + fx.manager.kill(s.id, false).await.expect("dismiss"); + assert!(!fx.manager.has(s.id).await); + assert!( + wt_path.exists(), + "dismissal must not remove what was retained" + ); + assert_eq!( + fx.retained_store().load_all().unwrap().len(), + 1, + "dismissal must not record a second entry for the same tree" + ); +} + +/// Natural exit (#82), pristine side of the same rule: nothing in the tree, +/// nothing to keep — the worktree is removed and no store entry appears. +#[tokio::test] +async fn natural_exit_of_a_pristine_worktree_removes_it() { + let fx = DaemonFixture::new(); + let repo = TempDir::new().unwrap(); + init_repo(repo.path()); + let wt_path = repo.path().join("worktrees").join("feat").join("clean"); + + let s = fx + .manager + .create(CreateSessionRequest { + cwd: repo.path().to_path_buf(), + command: "sleep 30".into(), + env: HashMap::new(), + metadata: HashMap::new(), + worktree: Some(WorktreeSpec { + base_repo: repo.path().to_path_buf(), + branch: "feat/clean".into(), + cleanup_on_kill: true, + create_branch: true, + }), + }) + .await + .expect("create"); + + // Push everything so the branch has no unpushed commits: fully pristine. + let origin = TempDir::new().unwrap(); + init_bare_repo(origin.path()); + push_all_to_origin(repo.path(), origin.path()); + + fx.tmux.tmux.kill_session(s.id).expect("kill tmux directly"); + fx.manager.start_liveness_watcher(Duration::from_millis(50)); + wait_for_async(Duration::from_secs(3), || async { + fx.manager.status(s.id).await == Some(SessionStatus::Dead) + }) + .await; + + assert!( + !wt_path.exists(), + "a pristine worktree leaves nothing behind" + ); + assert!( + fx.retained_store().load_all().unwrap().is_empty(), + "nothing was retained" + ); + assert!( + fx.manager.has(s.id).await, + "the session stays until dismissed" + ); +} + +/// When nothing can be recorded — here: no retained store wired — the exit +/// destroys nothing. The tree stays on disk AND stays attached to the dead +/// record, where dismissing it later still goes through #81's forced-kill +/// retention. Losing the store must not lose the work. +#[tokio::test] +async fn natural_exit_without_a_retained_store_destroys_nothing() { + let tmux = TmuxFixture::new(); + let state_dir = TempDir::new().unwrap(); + let manager = maestro_daemon::SessionManager::new( + Arc::new(tmux.tmux.clone()), + maestro_daemon::sidecar::SidecarStore::new(state_dir.path().join("sessions")), + maestro_daemon::worktree::WorktreeManager::new(None), + state_dir.path().join("fifos"), + ); + // Deliberately NO with_retained_store. + + let repo = TempDir::new().unwrap(); + init_repo(repo.path()); + let wt_path = repo.path().join("worktrees").join("feat").join("nostore"); + + let s = manager + .create(CreateSessionRequest { + cwd: repo.path().to_path_buf(), + command: "sleep 30".into(), + env: HashMap::new(), + metadata: HashMap::new(), + worktree: Some(WorktreeSpec { + base_repo: repo.path().to_path_buf(), + branch: "feat/nostore".into(), + cleanup_on_kill: true, + create_branch: true, + }), + }) + .await + .expect("create"); + std::fs::write(wt_path.join("README"), b"unsaved edits\n").unwrap(); + + tmux.tmux.kill_session(s.id).expect("kill tmux directly"); + manager.start_liveness_watcher(Duration::from_millis(50)); + wait_for_async(Duration::from_secs(3), || async { + manager.status(s.id).await == Some(SessionStatus::Dead) + }) + .await; + + assert!( + wt_path.exists(), + "nothing may be destroyed when it cannot be recorded" + ); + let snap = manager.snapshot().await; + assert!( + snap[0].worktree.is_some(), + "the tree must stay attached to the record so a later forced kill can retain it" + ); +} + fn run_git_in(dir: &std::path::Path, args: &[&str]) { let out = Command::new("git") .arg("-C") diff --git a/docs/WORKTREES.md b/docs/WORKTREES.md index 2c5bbd8..cdbf0d3 100644 --- a/docs/WORKTREES.md +++ b/docs/WORKTREES.md @@ -97,6 +97,17 @@ Nobody is waiting for an answer here, so there is nothing to refuse; the choice only between removing and retaining. It uses the same predicate as the kill, from the same place, so the two paths cannot drift apart. +The session itself stays in the catalog marked dead until it is dismissed — the +replay buffer of a crashed agent outlives it — but its worktree is settled at +once. Once settled, the session drops its worktree reference, so dismissing it +afterwards neither refuses (there is no guard left to trip) nor records a second +store entry. + +If settling is impossible — the probe fails, or the store cannot take the entry — +nothing is destroyed. The tree stays on disk *and* stays attached to the dead +record, where dismissing it later still goes through `--force`'s +retain-before-teardown path. Losing the store must never mean losing the work. + ## The retained store A `state_dir`-backed record of worktrees the daemon created that no longer have a