From 65b2646d3b63c36e5ab2138363d124f6a021dcad Mon Sep 17 00:00:00 2001 From: Tim Date: Wed, 20 May 2026 12:12:58 +0200 Subject: [PATCH] fix: centralize tmux lifecycle cleanup --- src/main.rs | 9 -- src/tui/app.rs | 13 --- src/use_cases.rs | 74 +++++++++------ tests/supervisor_tmux_test.rs | 170 +++++++++++++++++++++++++++++++++- 4 files changed, 216 insertions(+), 50 deletions(-) diff --git a/src/main.rs b/src/main.rs index 2dd7570..8b05da7 100644 --- a/src/main.rs +++ b/src/main.rs @@ -483,15 +483,6 @@ fn cmd_archive_task(config: &Config, task_id: &str, save: bool) -> Result<()> { anyhow::bail!("Task '{}' is already archived", task.meta.task_id()); } - // Kill tmux sessions (best-effort) - for repo in &task.meta.repos { - let _ = Tmux::kill_session(&repo.tmux_session); - } - if task.meta.is_multi_repo() { - let parent_session = Config::tmux_session_name(&task.meta.name, &task.meta.branch_name); - let _ = Tmux::kill_session(&parent_session); - } - let display_id = task.meta.task_id(); use_cases::archive_task(config, &mut task, save)?; diff --git a/src/tui/app.rs b/src/tui/app.rs index 2b97ebb..155b8e2 100644 --- a/src/tui/app.rs +++ b/src/tui/app.rs @@ -1826,19 +1826,6 @@ impl App { tracing::info!(task_id = %task_id, saved, "TUI: archive task requested"); self.log_output(format!("Archiving task {}...", task_id)); - // Kill tmux sessions for all repos (side effect) - if task.meta.has_repos() { - for repo in &task.meta.repos { - let _ = Tmux::kill_session(&repo.tmux_session); - } - } - // Also kill the parent-dir session (used for repo-inspector in multi-repo tasks) - if task.meta.is_multi_repo() { - let parent_session = Config::tmux_session_name(&task.meta.name, &task.meta.branch_name); - let _ = Tmux::kill_session(&parent_session); - } - self.log_output(" Killed tmux session(s)".to_string()); - // Delegate business logic to use_cases use_cases::archive_task(&self.config, &mut task, saved)?; self.log_output(" Archived task".to_string()); diff --git a/src/use_cases.rs b/src/use_cases.rs index d6c5d4d..2b91ff9 100644 --- a/src/use_cases.rs +++ b/src/use_cases.rs @@ -289,14 +289,14 @@ pub fn create_multi_repo_task( /// up when the task is permanently deleted (see `permanently_delete_archived_task`). /// /// This is the pure business logic behind archiving from the task list. -/// It archives and stops attached agents, but does NOT kill task tmux sessions — -/// that's a side effect handled by the caller. +/// It archives and stops attached agents and task tmux sessions. /// It does NOT remove the task directory — the directory is kept as the archive. pub fn archive_task(config: &Config, task: &mut Task, saved: bool) -> Result<()> { let task_id = task.meta.task_id(); tracing::info!(task_id = %task_id, saved, "archiving task"); archive_agents_attached_to_task(config, &task_id)?; + kill_task_tmux_sessions(task); // Remove worktrees (branches are kept for later reference) let parent_dir = task.meta.parent_dir.as_deref(); @@ -313,6 +313,31 @@ pub fn archive_task(config: &Config, task: &mut Task, saved: bool) -> Result<()> Ok(()) } +fn kill_task_tmux_sessions(task: &Task) { + for repo in &task.meta.repos { + if let Err(e) = Tmux::kill_session(&repo.tmux_session) { + tracing::warn!( + task_id = %task.meta.task_id(), + session = %repo.tmux_session, + error = %e, + "failed to kill task repo tmux session" + ); + } + } + + if task.meta.is_multi_repo() { + let parent_session = Config::tmux_session_name(&task.meta.name, &task.meta.branch_name); + if let Err(e) = Tmux::kill_session(&parent_session) { + tracing::warn!( + task_id = %task.meta.task_id(), + session = %parent_session, + error = %e, + "failed to kill task parent tmux session" + ); + } + } +} + fn archive_agents_attached_to_task(config: &Config, task_id: &str) -> Result<()> { let attached_agents: Vec<_> = AgentRecord::list_all(config)? .into_iter() @@ -345,6 +370,7 @@ pub fn permanently_delete_archived_task(config: &Config, task: Task) -> Result<( tracing::info!(task_id = %task_id, "permanently deleting archived task"); archive_agents_attached_to_task(config, &task_id)?; + kill_task_tmux_sessions(&task); // Delete branches for all repos (best-effort) let parent_dir = task.meta.parent_dir.as_deref(); @@ -360,13 +386,13 @@ pub fn permanently_delete_archived_task(config: &Config, task: Task) -> Result<( /// Fully delete a task: remove worktrees, delete branches, and remove the task /// directory. This is the "nuclear option" — everything is gone immediately. /// -/// Like `archive_task`, this does NOT kill task tmux sessions — the caller handles that. -/// Attached agents are archived and their canonical tmux sessions are stopped. +/// Attached agents are archived and task/agent tmux sessions are stopped. pub fn fully_delete_task(config: &Config, task: Task) -> Result<()> { let task_id = task.meta.task_id(); tracing::info!(task_id = %task_id, "fully deleting task"); archive_agents_attached_to_task(config, &task_id)?; + kill_task_tmux_sessions(&task); // Remove worktrees (best-effort) let parent_dir = task.meta.parent_dir.as_deref(); @@ -1810,7 +1836,7 @@ pub fn list_unassigned_tasks(config: &Config) -> Result> { .collect()) } -/// Delete a project: archive all its tasks, kill PM session, remove project directory. +/// Delete a project: archive all its tasks and agents, kill PM session, remove project directory. pub fn delete_project(config: &Config, project_name: &str) -> Result<()> { // Verify the project exists let _project = Project::load_by_name(config, project_name)?; @@ -1822,26 +1848,16 @@ pub fn delete_project(config: &Config, project_name: &str) -> Result<()> { if task.meta.archived_at.is_some() { continue; } - // Kill tmux sessions for repos (best-effort) - if task.meta.has_repos() { - for repo in &task.meta.repos { - let _ = Tmux::kill_session(&repo.tmux_session); - } - } - // Kill multi-repo parent session if applicable - if task.meta.is_multi_repo() { - let parent_session = Config::tmux_session_name(&task.meta.name, &task.meta.branch_name); - let _ = Tmux::kill_session(&parent_session); - } archive_task(config, &mut task, false)?; archived_count += 1; } - // Archive all non-archived agents (researchers and reviewers). + // Archive running agents and kill any already-archived agent sessions still present. let agents = AgentRecord::list_for_project(config, project_name)?; let mut agent_archived_count = 0; for agent in &agents { if agent.meta.status == AgentStatus::Archived { + kill_agent_tmux_session(agent); continue; } if let Err(e) = archive_agent(config, &agent.meta.project, &agent.meta.name) { @@ -3158,6 +3174,19 @@ pub(crate) fn agent_tmux_session_for_record(agent: &AgentRecord) -> String { agent_tmux_session(&agent.meta) } +fn kill_agent_tmux_session(agent: &AgentRecord) { + let session_name = agent_tmux_session(&agent.meta); + if let Err(e) = Tmux::kill_session(&session_name) { + tracing::warn!( + project = %agent.meta.project, + agent = %agent.meta.name, + session = %session_name, + error = %e, + "failed to kill agent tmux session" + ); + } +} + fn agent_window_kind(kind: &AgentKind) -> &'static str { match kind { AgentKind::Engineer => "engineer", @@ -4005,11 +4034,7 @@ pub fn archive_agent(config: &Config, project: &str, name: &str) -> Result<()> { } } - let session_name = agent_tmux_session(&agent.meta); - if Tmux::session_exists(&session_name) { - tracing::info!(session = &session_name, "killing agent tmux session"); - Tmux::kill_session(&session_name)?; - } + kill_agent_tmux_session(&agent); agent.meta.status = AgentStatus::Archived; agent.meta.attachment = AgentAttachment::Unattached; @@ -4030,10 +4055,7 @@ pub fn permanently_delete_archived_agent(config: &Config, agent: AgentRecord) -> ); } - let session_name = agent_tmux_session(&agent.meta); - if let Err(e) = Tmux::kill_session(&session_name) { - tracing::warn!(session = %session_name, error = %e, "failed to kill agent tmux session during permanent delete"); - } + kill_agent_tmux_session(&agent); cleanup_agent_worktrees(config, &agent); diff --git a/tests/supervisor_tmux_test.rs b/tests/supervisor_tmux_test.rs index 2c5d4fa..739dadb 100644 --- a/tests/supervisor_tmux_test.rs +++ b/tests/supervisor_tmux_test.rs @@ -1,10 +1,11 @@ mod helpers; +use agman::agent_model::{AgentAttachment, AgentStatus}; use agman::config::Config; use agman::supervisor; use agman::tmux::Tmux; use agman::use_cases; -use helpers::{create_test_researcher, create_test_task, test_config}; +use helpers::{create_test_project, create_test_researcher, create_test_task, test_config}; use std::process::Command; use std::time::{SystemTime, UNIX_EPOCH}; @@ -22,9 +23,170 @@ impl Drop for TmuxCleanup { } } +#[test] +fn archive_task_kills_task_sessions_and_attached_agent_sessions() { + if !tmux_available() { + eprintln!("skipping tmux lifecycle smoke: tmux binary is unavailable"); + return; + } + + let tmp = tempfile::tempdir().unwrap(); + let config = test_config(&tmp); + let unique = unique_test_name(); + let project = format!("repo-{unique}"); + let branch = format!("branch-{unique}"); + let mut task = create_test_task(&config, &project, &branch); + let task_id = task.meta.task_id(); + let researcher = create_test_researcher(&config, &project, &format!("research-{unique}")); + use_cases::attach_agent_to_task(&config, &project, &researcher.meta.name, &task_id, None) + .unwrap(); + let engineer = use_cases::attached_engineer_for_task(&config, &task_id).unwrap(); + let task_session = task.meta.primary_repo().tmux_session.clone(); + let engineer_session = Config::engineer_tmux_session(&project, &engineer.meta.name); + let researcher_session = Config::researcher_tmux_session(&project, &researcher.meta.name); + let _cleanup = TmuxCleanup { + sessions: vec![ + task_session.clone(), + engineer_session.clone(), + researcher_session.clone(), + ], + }; + create_tmux_session(&task_session, tmp.path()); + create_tmux_session(&engineer_session, tmp.path()); + create_tmux_session(&researcher_session, tmp.path()); + + use_cases::archive_task(&config, &mut task, false).unwrap(); + + assert!(!Tmux::session_exists(&task_session)); + assert!(!Tmux::session_exists(&engineer_session)); + assert!(!Tmux::session_exists(&researcher_session)); +} + +#[test] +fn archive_agent_kills_canonical_session_and_unlinks_task_window() { + if !tmux_available() { + eprintln!("skipping tmux lifecycle smoke: tmux binary is unavailable"); + return; + } + + let tmp = tempfile::tempdir().unwrap(); + let config = test_config(&tmp); + let unique = unique_test_name(); + let project = format!("repo-{unique}"); + let branch = format!("branch-{unique}"); + let task = create_test_task(&config, &project, &branch); + let task_id = task.meta.task_id(); + let researcher = create_test_researcher(&config, &project, &format!("research-{unique}")); + use_cases::attach_agent_to_task(&config, &project, &researcher.meta.name, &task_id, None) + .unwrap(); + let task_session = task.meta.primary_repo().tmux_session.clone(); + let researcher_session = Config::researcher_tmux_session(&project, &researcher.meta.name); + let window_name = + Tmux::linked_agent_window_name("researcher", &researcher.meta.name, &researcher_session); + let _cleanup = TmuxCleanup { + sessions: vec![task_session.clone(), researcher_session.clone()], + }; + create_tmux_session(&task_session, tmp.path()); + create_tmux_session(&researcher_session, tmp.path()); + Tmux::link_agent_window(&task_session, &researcher_session, &window_name).unwrap(); + assert!(window_names(&task_session).contains(&window_name)); + + use_cases::archive_agent(&config, &project, &researcher.meta.name).unwrap(); + + assert!(Tmux::session_exists(&task_session)); + assert!(!Tmux::session_exists(&researcher_session)); + assert!(!window_names(&task_session).contains(&window_name)); +} + +#[test] +fn delete_project_kills_pm_task_and_agent_sessions() { + if !tmux_available() { + eprintln!("skipping tmux lifecycle smoke: tmux binary is unavailable"); + return; + } + + let tmp = tempfile::tempdir().unwrap(); + let config = test_config(&tmp); + let unique = unique_test_name(); + let project = format!("repo-{unique}"); + let branch = format!("branch-{unique}"); + create_test_project(&config, &project); + let task = create_test_task(&config, &project, &branch); + let task_id = task.meta.task_id(); + let engineer = use_cases::attached_engineer_for_task(&config, &task_id).unwrap(); + let running_agent = create_test_researcher(&config, &project, &format!("running-{unique}")); + let mut archived_agent = + create_test_researcher(&config, &project, &format!("archived-{unique}")); + archived_agent.meta.status = AgentStatus::Archived; + archived_agent.meta.attachment = AgentAttachment::Unattached; + archived_agent.save_meta().unwrap(); + + let pm_session = Config::pm_tmux_session(&project); + let task_session = task.meta.primary_repo().tmux_session.clone(); + let engineer_session = Config::engineer_tmux_session(&project, &engineer.meta.name); + let running_session = Config::researcher_tmux_session(&project, &running_agent.meta.name); + let archived_session = Config::researcher_tmux_session(&project, &archived_agent.meta.name); + let unrelated_session = format!("agman-unrelated-{unique}"); + let _cleanup = TmuxCleanup { + sessions: vec![ + pm_session.clone(), + task_session.clone(), + engineer_session.clone(), + running_session.clone(), + archived_session.clone(), + unrelated_session.clone(), + ], + }; + for session in [ + &pm_session, + &task_session, + &engineer_session, + &running_session, + &archived_session, + &unrelated_session, + ] { + create_tmux_session(session, tmp.path()); + } + + use_cases::delete_project(&config, &project).unwrap(); + + assert!(!Tmux::session_exists(&pm_session)); + assert!(!Tmux::session_exists(&task_session)); + assert!(!Tmux::session_exists(&engineer_session)); + assert!(!Tmux::session_exists(&running_session)); + assert!(!Tmux::session_exists(&archived_session)); + assert!(Tmux::session_exists(&unrelated_session)); +} + +#[test] +fn permanently_delete_archived_task_kills_leftover_task_session() { + if !tmux_available() { + eprintln!("skipping tmux lifecycle smoke: tmux binary is unavailable"); + return; + } + + let tmp = tempfile::tempdir().unwrap(); + let config = test_config(&tmp); + let unique = unique_test_name(); + let project = format!("repo-{unique}"); + let branch = format!("branch-{unique}"); + let mut task = create_test_task(&config, &project, &branch); + task.meta.archived_at = Some(chrono::Utc::now()); + task.save_meta().unwrap(); + let task_session = task.meta.primary_repo().tmux_session.clone(); + let _cleanup = TmuxCleanup { + sessions: vec![task_session.clone()], + }; + create_tmux_session(&task_session, tmp.path()); + + use_cases::permanently_delete_archived_task(&config, task).unwrap(); + + assert!(!Tmux::session_exists(&task_session)); +} + #[test] fn ensure_task_tmux_backfills_attached_agent_windows_after_creation_and_recreation() { - if Command::new("tmux").arg("-V").output().is_err() { + if !tmux_available() { eprintln!("skipping tmux backfill smoke: tmux binary is unavailable"); return; } @@ -167,6 +329,10 @@ fn unique_test_name() -> String { format!("{}-{nanos}", std::process::id()) } +fn tmux_available() -> bool { + Command::new("tmux").arg("-V").output().is_ok() +} + fn create_tmux_session(session: &str, cwd: &std::path::Path) { let output = Command::new("tmux") .args(["new-session", "-d", "-s", session, "-n", "main", "-c"])