feat(maestro): apply the kill's worktree rule when a session exits on its own
When the liveness watcher finds the tmux session gone, it now settles the worktree through the same probe_worktree/record_retention helpers the kill uses: pristine trees are removed, anything else is retained as exited-with-unsaved-work before anything is torn down. The dead session drops its worktree reference once settled, so dismissing it neither refuses nor retains twice. If the probe or store write fails, nothing is destroyed and the tree stays attached to the record. WORKTREES.md: written down the settle-then-drop and nothing-destroyed- when-unrecordable rules alongside the existing exit rule.
This commit is contained in:
@@ -810,17 +810,23 @@ async fn run_activity_pass(inner: &Arc<Inner>) {
|
||||
}
|
||||
|
||||
/// 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<Inner>, 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<Inner>, 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<Inner>,
|
||||
id: Uuid,
|
||||
(base_repo, path): &(PathBuf, PathBuf),
|
||||
known_branch: Option<String>,
|
||||
) -> 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
|
||||
|
||||
@@ -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")
|
||||
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user