diff --git a/crates/maestro-daemon/src/session.rs b/crates/maestro-daemon/src/session.rs index 0f5769d..910894b 100644 --- a/crates/maestro-daemon/src/session.rs +++ b/crates/maestro-daemon/src/session.rs @@ -810,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 => { @@ -828,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