From bb1c63d1779077d10d771d96f2e38937817cb398 Mon Sep 17 00:00:00 2001 From: "Andrei G." Date: Sat, 30 May 2026 03:34:22 +0200 Subject: [PATCH] fix(worktree): propagate config errors and ensure cleanup on early exit #4701: handle_worktree_command now propagates config parse errors via `?` with an error message including the config file path, instead of silently falling back to the default config (which has worktree.enabled=false). #4702: add WorktreeCleanupGuard RAII struct in zeph-subagent that ensures wm.remove runs on both normal return and early exit via `?`. Uses Handle::try_current() in Drop to safely spawn the cleanup task; logs an error if no runtime is active without panicking. Note: panic=abort in the release profile means Drop does not run on panics in release builds. Closes #4701, Closes #4702 --- crates/zeph-subagent/src/manager.rs | 145 ++++++++++++++++++++++++++-- src/commands/worktree.rs | 31 +++++- 2 files changed, 168 insertions(+), 8 deletions(-) diff --git a/crates/zeph-subagent/src/manager.rs b/crates/zeph-subagent/src/manager.rs index f059ed332..3c3481d3e 100644 --- a/crates/zeph-subagent/src/manager.rs +++ b/crates/zeph-subagent/src/manager.rs @@ -139,6 +139,44 @@ pub struct SpawnContext { pub inherited_tool_allowlist: Option>, } +/// RAII guard that calls [`DefaultWorktreeManager::remove`] when dropped. +/// +/// Guarantees cleanup on normal return and on early `?` returns from the agent loop. +/// With `panic = "abort"` (release profile) panics terminate the process via `abort(3)` +/// before any `Drop` runs; the OS reclaims resources directly. With `panic = "unwind"` +/// (dev/test profile) `Drop` runs during stack unwinding. +struct WorktreeCleanupGuard { + wm: Arc, + handle: zeph_worktree::WorktreeHandle, + prune: bool, + enabled: bool, +} + +impl Drop for WorktreeCleanupGuard { + fn drop(&mut self) { + if !self.enabled { + return; + } + let wm = Arc::clone(&self.wm); + let h = self.handle.clone(); + let prune = self.prune; + match tokio::runtime::Handle::try_current() { + Ok(rt) => { + rt.spawn(async move { + if let Err(e) = wm.remove(&h, prune).await { + tracing::warn!(error = %e, "failed to remove sub-agent worktree"); + } + }); + } + Err(_) => { + tracing::error!( + "no tokio runtime in WorktreeCleanupGuard::drop — worktree cleanup skipped" + ); + } + } + } +} + /// Wraps an executor to allow file operations on the agent's memory directory. /// /// When the inner executor returns `SandboxViolation` for a file tool call, this @@ -1268,15 +1306,18 @@ impl SubAgentManager { ); let guard = CwdRestoreGuard::new(&handle.path, owned_guard) .map_err(|e| SubAgentError::WorktreeSetup(e.to_string()))?; + let _cleanup = WorktreeCleanupGuard { + wm: Arc::clone(wm), + handle: handle.clone(), + prune: prune_branch_on_remove, + enabled: cleanup_on_completion, + }; + let result = run_agent_loop(agent_loop_args).await; - // guard drops here: cwd restored, mutex released + // CwdRestoreGuard drops here: cwd restored, mutex released. + // WorktreeCleanupGuard drops next: spawns wm.remove via + // Handle::try_current. drop(guard); - // Remove the worktree after the agent completes (INV-2 ordering). - if cleanup_on_completion - && let Err(e) = wm.remove(&handle, prune_branch_on_remove).await - { - tracing::warn!(error = %e, "failed to remove sub-agent worktree"); - } return result; } @@ -5599,3 +5640,93 @@ mod worktree_predicate_tests { ); } } + +/// Regression tests for #4702: `WorktreeCleanupGuard` `enabled` flag and Drop behaviour. +/// +/// Now that `WorktreeCleanupGuard` is a module-level struct, these tests instantiate the +/// real production type. Tests that require `wm.remove` to execute use a `#[tokio::test]` +/// runtime; tests that exercise the `enabled = false` no-op path work without one. +#[cfg(test)] +mod worktree_cleanup_guard_tests { + use std::sync::Arc; + + use tempfile::TempDir; + use zeph_config::WorktreeConfig; + use zeph_worktree::{DefaultGitRunner, DefaultWorktreeManager, WorktreeHandle}; + + use super::WorktreeCleanupGuard; + + fn make_dummy_wm() -> (TempDir, Arc) { + let dir = TempDir::new().unwrap(); + std::fs::create_dir_all(dir.path().join(".git")).unwrap(); + let config = WorktreeConfig { + enabled: true, + root: "worktrees".to_string(), + ..WorktreeConfig::default() + }; + let wm = + DefaultWorktreeManager::new(dir.path().to_path_buf(), config, DefaultGitRunner::new()) + .unwrap(); + (dir, Arc::new(wm)) + } + + fn dummy_handle(dir: &TempDir) -> WorktreeHandle { + WorktreeHandle { + path: dir.path().join("wt"), + branch_name: "agent/test".to_string(), + base_ref_resolved: "HEAD".to_string(), + subagent_id: "test-agent".to_string(), + created_at: std::time::SystemTime::now(), + } + } + + /// `enabled = false`: Drop must be a no-op — no `Handle::try_current` call, no panic + /// even outside a tokio runtime. + #[test] + fn cleanup_skipped_when_disabled() { + let (_dir, wm) = make_dummy_wm(); + let dir2 = TempDir::new().unwrap(); + let handle = dummy_handle(&dir2); + // Drop must not panic even without a tokio runtime. + drop(WorktreeCleanupGuard { + wm, + handle, + prune: false, + enabled: false, + }); + } + + /// `enabled = true` outside a tokio runtime: Drop must log an error and not panic. + /// This covers the `Handle::try_current` → `Err` branch. + #[test] + fn cleanup_logs_error_without_runtime() { + let (_dir, wm) = make_dummy_wm(); + let dir2 = TempDir::new().unwrap(); + let handle = dummy_handle(&dir2); + // No tokio runtime active — must not panic, must log error instead. + drop(WorktreeCleanupGuard { + wm, + handle, + prune: false, + enabled: true, + }); + } + + /// `enabled = true` inside a tokio runtime: Drop spawns `wm.remove`. The task + /// runs and completes without error (the worktree path does not exist, so remove + /// is a no-op or logs a warning — either outcome is acceptable here). + #[tokio::test] + async fn cleanup_spawns_remove_with_runtime() { + let (_dir, wm) = make_dummy_wm(); + let dir2 = TempDir::new().unwrap(); + let handle = dummy_handle(&dir2); + drop(WorktreeCleanupGuard { + wm, + handle, + prune: false, + enabled: true, + }); + // Yield to allow the spawned task to complete. + tokio::task::yield_now().await; + } +} diff --git a/src/commands/worktree.rs b/src/commands/worktree.rs index 4c08a91af..409bb113a 100644 --- a/src/commands/worktree.rs +++ b/src/commands/worktree.rs @@ -19,7 +19,9 @@ pub(crate) async fn handle_worktree_command( config_path: Option<&Path>, ) -> anyhow::Result<()> { let config_file = resolve_config_path(config_path); - let config = zeph_core::config::Config::load(&config_file).unwrap_or_default(); + let config = zeph_core::config::Config::load(&config_file).map_err(|e| { + anyhow::anyhow!("failed to load config from {}: {e}", config_file.display()) + })?; if !config.worktree.enabled { anyhow::bail!( @@ -73,3 +75,30 @@ pub(crate) async fn handle_worktree_command( Ok(()) } + +#[cfg(test)] +mod tests { + use std::io::Write as _; + + /// Regression test for #4701: `handle_worktree_command` must propagate a config parse + /// error instead of silently falling back to `Config::default()`. + #[tokio::test] + async fn invalid_config_returns_error_not_default() { + let mut f = tempfile::NamedTempFile::new().expect("tempfile"); + f.write_all(b"[[[[invalid toml}}}").expect("write"); + let path = f.path().to_owned(); + + let result = + super::handle_worktree_command(crate::cli::WorktreeCommand::List, Some(&path)).await; + + assert!( + result.is_err(), + "expected an error for invalid config, got Ok" + ); + let msg = format!("{:#}", result.unwrap_err()); + assert!( + msg.contains("failed to load config"), + "error must mention config load failure, got: {msg}" + ); + } +}