From 1ae8e8ec9d9b98e95f5b23f7a19a20fd204b3f38 Mon Sep 17 00:00:00 2001 From: dave Date: Wed, 29 Apr 2026 10:28:18 +0000 Subject: [PATCH] huskies: merge 841 --- server/src/startup/tick_loop.rs | 29 ++- server/src/worktree/mod.rs | 2 + server/src/worktree/sweep.rs | 446 ++++++++++++++++++++++++++++++++ 3 files changed, 472 insertions(+), 5 deletions(-) create mode 100644 server/src/worktree/sweep.rs diff --git a/server/src/startup/tick_loop.rs b/server/src/startup/tick_loop.rs index 73803d70..2f7e9f72 100644 --- a/server/src/startup/tick_loop.rs +++ b/server/src/startup/tick_loop.rs @@ -81,20 +81,28 @@ pub(crate) fn spawn_event_bridges( /// Spawn the unified 1-second background tick loop. /// -/// Fires due timers, runs the agent watchdog every 30 ticks, and promotes -/// done→archived items every `sweep_interval_secs` ticks. +/// Fires due timers, runs the agent watchdog every 30 ticks, promotes +/// done→archived items every `sweep_interval_secs` ticks, and removes +/// orphaned worktrees every `worktree_sweep_interval_secs` ticks (default +/// 1200, i.e. 20 minutes). pub(crate) fn spawn_tick_loop( agents: Arc, timer_store: Arc, root: Option, ) { - let sweep_cfg = root + let project_cfg = root .as_ref() - .and_then(|r| config::ProjectConfig::load(r).ok()) - .map(|c| c.watcher) + .and_then(|r| config::ProjectConfig::load(r).ok()); + let sweep_cfg = project_cfg + .as_ref() + .map(|c| c.watcher.clone()) .unwrap_or_default(); let sweep_every = sweep_cfg.sweep_interval_secs.max(1); let done_retention = std::time::Duration::from_secs(sweep_cfg.done_retention_secs); + // Capture config for the worktree sweep (read once at startup). + let worktree_sweep_config = project_cfg.unwrap_or_default(); + // Worktree orphan sweep: every 20 minutes by default. + let worktree_sweep_every: u64 = 1200; let pending_count = timer_store.list().len(); crate::slog!("[tick] Unified tick loop started; {pending_count} pending timer(s)"); @@ -132,6 +140,17 @@ pub(crate) fn spawn_tick_loop( if tick_count.is_multiple_of(sweep_every) { io::watcher::sweep_done_to_archived(done_retention); } + + // Worktree orphan sweep: remove worktrees for done/archived/absent stories. + if tick_count.is_multiple_of(worktree_sweep_every) + && let Some(ref r) = root + { + let removed = + crate::worktree::sweep_orphaned_worktrees(r, &worktree_sweep_config).await; + if removed > 0 { + crate::slog!("[worktree-sweep] Removed {removed} orphaned worktree(s)."); + } + } } }); } diff --git a/server/src/worktree/mod.rs b/server/src/worktree/mod.rs index 80f937c4..a9817536 100644 --- a/server/src/worktree/mod.rs +++ b/server/src/worktree/mod.rs @@ -4,10 +4,12 @@ use std::path::{Path, PathBuf}; mod create; mod git; mod remove; +mod sweep; pub use create::create_worktree; pub use git::{migrate_slug_paths, prune_worktree_sync}; pub use remove::remove_worktree_by_story_id; +pub use sweep::sweep_orphaned_worktrees; #[derive(Debug, Clone)] pub struct WorktreeInfo { diff --git a/server/src/worktree/sweep.rs b/server/src/worktree/sweep.rs new file mode 100644 index 00000000..464e1c26 --- /dev/null +++ b/server/src/worktree/sweep.rs @@ -0,0 +1,446 @@ +//! Periodic orphan sweep — removes worktrees whose stories are done, archived, +//! or absent from the CRDT. + +use crate::config::ProjectConfig; +use crate::pipeline_state::{Stage, read_typed}; +use std::path::Path; + +use super::{list_worktrees, remove_worktree_by_story_id}; + +/// Returns `true` if a worktree for the given pipeline stage should be removed +/// by the orphan sweep. +/// +/// A worktree is swept when its story is `Done`, `Archived`, or not present in +/// the CRDT at all (i.e. `stage` is `None`). Active stages (`Backlog`, +/// `Coding`, `Qa`, `Merge`) are left alone. +pub fn worktree_should_be_swept(stage: Option<&Stage>) -> bool { + match stage { + None => true, + Some(Stage::Done { .. }) | Some(Stage::Archived { .. }) => true, + Some(_) => false, + } +} + +/// Remove orphaned worktrees whose stories are done, archived, or absent from +/// the CRDT. +/// +/// Walks `.huskies/worktrees/`, checks each story's stage via `lookup`, and +/// calls [`remove_worktree_by_story_id`] for any that should be swept. +/// Failures are logged individually; the sweep continues regardless. +/// +/// Returns the number of worktrees successfully removed. +pub async fn sweep_orphaned_worktrees(project_root: &Path, config: &ProjectConfig) -> usize { + sweep_with_lookup(project_root, config, |story_id| { + read_typed(story_id).ok().flatten().map(|item| item.stage) + }) + .await +} + +/// Internal sweep implementation that accepts a custom CRDT lookup function. +/// +/// Accepts a `lookup` closure `fn(&str) -> Option` that returns the +/// stage for a given story ID, or `None` if the story is not in the CRDT. +/// This indirection makes the sweep testable without a real CRDT. +pub(crate) async fn sweep_with_lookup( + project_root: &Path, + config: &ProjectConfig, + lookup: F, +) -> usize +where + F: Fn(&str) -> Option, +{ + let entries = match list_worktrees(project_root) { + Ok(e) => e, + Err(err) => { + crate::slog_error!("[worktree-sweep] Failed to list worktrees: {err}"); + return 0; + } + }; + + let mut removed = 0usize; + for entry in entries { + let stage = lookup(&entry.story_id); + if !worktree_should_be_swept(stage.as_ref()) { + continue; + } + + crate::slog!( + "[worktree-sweep] Removing orphaned worktree for '{}' (stage: {})", + entry.story_id, + stage + .as_ref() + .map(|s| format!("{:?}", s)) + .unwrap_or_else(|| "not in CRDT".to_string()) + ); + + match remove_worktree_by_story_id(project_root, &entry.story_id, config).await { + Ok(()) => removed += 1, + Err(err) => { + crate::slog_error!( + "[worktree-sweep] Failed to remove worktree for '{}': {err}", + entry.story_id + ); + } + } + } + + removed +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::config::WatcherConfig; + use chrono::Utc; + use std::fs; + use std::num::NonZeroU32; + use std::path::PathBuf; + use std::process::Command; + use tempfile::TempDir; + + fn init_git_repo(dir: &Path) { + Command::new("git") + .args(["init"]) + .current_dir(dir) + .output() + .expect("git init"); + Command::new("git") + .args(["commit", "--allow-empty", "-m", "init"]) + .current_dir(dir) + .output() + .expect("git commit"); + } + + fn empty_config() -> ProjectConfig { + ProjectConfig { + component: vec![], + agent: vec![], + watcher: WatcherConfig::default(), + default_qa: "server".to_string(), + default_coder_model: None, + max_coders: None, + max_retries: 2, + base_branch: None, + rate_limit_notifications: true, + web_ui_status_consumer: true, + matrix_status_consumer: true, + slack_status_consumer: true, + discord_status_consumer: true, + whatsapp_status_consumer: true, + timezone: None, + rendezvous: None, + trusted_keys: Vec::new(), + crdt_require_token: false, + crdt_tokens: Vec::new(), + max_mesh_peers: 3, + gateway_url: None, + gateway_project: None, + } + } + + fn done_stage() -> Stage { + Stage::Done { + merged_at: Utc::now(), + merge_commit: crate::pipeline_state::GitSha("abc123".to_string()), + } + } + + fn archived_stage() -> Stage { + Stage::Archived { + archived_at: Utc::now(), + reason: crate::pipeline_state::ArchiveReason::Completed, + } + } + + // ── worktree_should_be_swept unit tests ───────────────────────────────── + + #[test] + fn should_sweep_when_not_in_crdt() { + assert!(worktree_should_be_swept(None)); + } + + #[test] + fn should_sweep_done() { + assert!(worktree_should_be_swept(Some(&done_stage()))); + } + + #[test] + fn should_sweep_archived() { + assert!(worktree_should_be_swept(Some(&archived_stage()))); + } + + #[test] + fn should_not_sweep_backlog() { + assert!(!worktree_should_be_swept(Some(&Stage::Backlog))); + } + + #[test] + fn should_not_sweep_coding() { + assert!(!worktree_should_be_swept(Some(&Stage::Coding))); + } + + #[test] + fn should_not_sweep_qa() { + assert!(!worktree_should_be_swept(Some(&Stage::Qa))); + } + + #[test] + fn should_not_sweep_merge() { + let stage = Stage::Merge { + feature_branch: crate::pipeline_state::BranchName("feature/x".to_string()), + commits_ahead: NonZeroU32::new(1).unwrap(), + }; + assert!(!worktree_should_be_swept(Some(&stage))); + } + + // ── Integration tests: sweep_with_lookup ──────────────────────────────── + + fn setup_project_with_worktree(story_id: &str) -> (TempDir, PathBuf) { + let tmp = TempDir::new().unwrap(); + let project_root = tmp.path().join("project"); + fs::create_dir_all(&project_root).unwrap(); + init_git_repo(&project_root); + + // Create a bare worktree directory (simulates a worktree without full git checkout) + let worktrees_dir = project_root.join(".huskies").join("worktrees"); + fs::create_dir_all(worktrees_dir.join(story_id)).unwrap(); + + (tmp, project_root) + } + + #[tokio::test] + async fn sweep_removes_done_worktree() { + let story_id = "100_done_story"; + let (_tmp, project_root) = setup_project_with_worktree(story_id); + let wt_dir = project_root + .join(".huskies") + .join("worktrees") + .join(story_id); + assert!(wt_dir.exists(), "worktree should exist before sweep"); + + // We can't remove via git worktree because it's not a real git worktree, + // so mock the removal: create a worktree directory and confirm sweep detects it. + // Instead, test that the sweep logic identifies the worktree as removable. + let config = empty_config(); + let removed = sweep_with_lookup(&project_root, &config, |id| { + if id == story_id { + Some(done_stage()) + } else { + None + } + }) + .await; + + // The worktree dir exists but is not a real git worktree, so remove_worktree_by_story_id + // will fail (not a git worktree). We can't assert removed == 1, but we can verify + // it was attempted (removed == 0 due to error is fine; what matters is no panic and + // the sweep continued). + // For a stronger test, use a real git worktree below. + let _ = removed; // sweep ran without panic + } + + #[tokio::test] + async fn sweep_removes_real_done_worktree() { + let tmp = TempDir::new().unwrap(); + let project_root = tmp.path().join("project"); + fs::create_dir_all(&project_root).unwrap(); + init_git_repo(&project_root); + + let story_id = "101_done_real"; + let config = empty_config(); + + // Create a real git worktree. + super::super::create::create_worktree(&project_root, story_id, &config, 3001) + .await + .unwrap(); + + let wt_dir = project_root + .join(".huskies") + .join("worktrees") + .join(story_id); + assert!(wt_dir.exists(), "worktree must exist before sweep"); + + let removed = sweep_with_lookup(&project_root, &config, |id| { + if id == story_id { + Some(done_stage()) + } else { + None + } + }) + .await; + + assert_eq!(removed, 1, "sweep should remove the done worktree"); + assert!( + !wt_dir.exists(), + "worktree directory should be gone after sweep" + ); + } + + #[tokio::test] + async fn sweep_does_not_remove_current_worktree() { + let tmp = TempDir::new().unwrap(); + let project_root = tmp.path().join("project"); + fs::create_dir_all(&project_root).unwrap(); + init_git_repo(&project_root); + + let story_id = "102_current_story"; + let config = empty_config(); + + super::super::create::create_worktree(&project_root, story_id, &config, 3001) + .await + .unwrap(); + + let wt_dir = project_root + .join(".huskies") + .join("worktrees") + .join(story_id); + assert!(wt_dir.exists()); + + let removed = sweep_with_lookup(&project_root, &config, |id| { + if id == story_id { + Some(Stage::Coding) + } else { + None + } + }) + .await; + + assert_eq!(removed, 0, "sweep must not remove current/coding worktrees"); + assert!(wt_dir.exists(), "worktree directory must still exist"); + } + + #[tokio::test] + async fn sweep_does_not_remove_qa_worktree() { + let tmp = TempDir::new().unwrap(); + let project_root = tmp.path().join("project"); + fs::create_dir_all(&project_root).unwrap(); + init_git_repo(&project_root); + + let story_id = "103_qa_story"; + let config = empty_config(); + + super::super::create::create_worktree(&project_root, story_id, &config, 3001) + .await + .unwrap(); + + let wt_dir = project_root + .join(".huskies") + .join("worktrees") + .join(story_id); + + let removed = sweep_with_lookup(&project_root, &config, |id| { + if id == story_id { + Some(Stage::Qa) + } else { + None + } + }) + .await; + + assert_eq!(removed, 0, "sweep must not remove qa worktrees"); + assert!(wt_dir.exists()); + } + + #[tokio::test] + async fn sweep_does_not_remove_merge_worktree() { + let tmp = TempDir::new().unwrap(); + let project_root = tmp.path().join("project"); + fs::create_dir_all(&project_root).unwrap(); + init_git_repo(&project_root); + + let story_id = "104_merge_story"; + let config = empty_config(); + + super::super::create::create_worktree(&project_root, story_id, &config, 3001) + .await + .unwrap(); + + let wt_dir = project_root + .join(".huskies") + .join("worktrees") + .join(story_id); + + let removed = sweep_with_lookup(&project_root, &config, |id| { + if id == story_id { + Some(Stage::Merge { + feature_branch: crate::pipeline_state::BranchName( + "feature/story-104_merge_story".to_string(), + ), + commits_ahead: NonZeroU32::new(1).unwrap(), + }) + } else { + None + } + }) + .await; + + assert_eq!(removed, 0, "sweep must not remove merge worktrees"); + assert!(wt_dir.exists()); + } + + #[tokio::test] + async fn sweep_removes_worktree_not_in_crdt() { + let tmp = TempDir::new().unwrap(); + let project_root = tmp.path().join("project"); + fs::create_dir_all(&project_root).unwrap(); + init_git_repo(&project_root); + + let story_id = "105_absent_story"; + let config = empty_config(); + + super::super::create::create_worktree(&project_root, story_id, &config, 3001) + .await + .unwrap(); + + let wt_dir = project_root + .join(".huskies") + .join("worktrees") + .join(story_id); + assert!(wt_dir.exists()); + + // lookup returns None → story not in CRDT + let removed = sweep_with_lookup(&project_root, &config, |_| None).await; + + assert_eq!( + removed, 1, + "sweep should remove worktree with no CRDT entry" + ); + assert!(!wt_dir.exists()); + } + + #[tokio::test] + async fn sweep_continues_on_individual_failure() { + // One worktree that's a bare directory (not a real git worktree — removal fails) + // and one real worktree. The sweep should skip the failed one and remove the real one. + let tmp = TempDir::new().unwrap(); + let project_root = tmp.path().join("project"); + fs::create_dir_all(&project_root).unwrap(); + init_git_repo(&project_root); + + let bad_id = "106_bad_wt"; + let good_id = "107_good_wt"; + let config = empty_config(); + + // Bad: bare directory, not a real git worktree. + fs::create_dir_all(project_root.join(".huskies").join("worktrees").join(bad_id)).unwrap(); + + // Good: real worktree. + super::super::create::create_worktree(&project_root, good_id, &config, 3001) + .await + .unwrap(); + + let removed = sweep_with_lookup(&project_root, &config, |_| None).await; + + // The good one was removed; bad one failed but sweep continued. + assert!( + removed >= 1, + "at least the real worktree should be removed; got {removed}" + ); + + let good_wt = project_root + .join(".huskies") + .join("worktrees") + .join(good_id); + assert!(!good_wt.exists(), "real worktree should be gone"); + } +}