diff --git a/CHANGELOG.md b/CHANGELOG.md index 6807f1be9..21007aafe 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,11 @@ The format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ### Fixed +- **Worktree sandbox cleanup on plan cancel** (#9230): `plan cancel` now removes + the git worktree branch and directory created during execute, preventing resource + leaks from dangling worktrees. Delegates to `GitWorktreeSandbox.cleanup_stale()` + in the infrastructure layer. + - **`plan apply` merge conflict cleanup** (#7250): When `git merge` fails due to a conflict during `plan apply`, the apply command now reads the actual conflict detail from `CalledProcessError.stdout` (git writes conflict info to stdout, not diff --git a/features/cancel_worktree_cleanup.feature b/features/cancel_worktree_cleanup.feature new file mode 100644 index 000000000..53317d19c --- /dev/null +++ b/features/cancel_worktree_cleanup.feature @@ -0,0 +1,17 @@ +@cancel-worktree-cleanup +Feature: Plan cancel cleans up worktree sandbox (#9230) + Verifies that cancelling a plan after execute removes the + git worktree branch and directory to prevent resource leaks. + + Scenario: _cleanup_sandbox_for_plan removes worktree for cancelled plan for cwc + Given a temp git project with a worktree sandbox for plan "01TESTCANCEL000000000000" for cwc + And a mocked service that resolves the project for cwc + When I call _cleanup_sandbox_for_plan for plan "01TESTCANCEL000000000000" for cwc + Then the branch "cleveragents/plan-01TESTCANCEL000000000000" should not exist for cwc + And the worktree directory should not exist for cwc + + Scenario: _cleanup_sandbox_for_plan is a no-op when no sandbox exists for cwc + Given a temp git project without any worktree for cwc + And a mocked service with no linked resources for cwc + When I call _cleanup_sandbox_for_plan for plan "01TESTNOSANDBOX0000000000" for cwc + Then the call should complete without error for cwc diff --git a/features/steps/cancel_worktree_cleanup_steps.py b/features/steps/cancel_worktree_cleanup_steps.py new file mode 100644 index 000000000..9845bbd3f --- /dev/null +++ b/features/steps/cancel_worktree_cleanup_steps.py @@ -0,0 +1,144 @@ +"""Steps for cancel_worktree_cleanup.feature.""" + +from __future__ import annotations + +import os +import shutil +import subprocess +import tempfile +from pathlib import Path +from unittest.mock import MagicMock, patch + +from behave import given, then, when + + +def _git(args: list[str], cwd: str) -> subprocess.CompletedProcess[str]: + return subprocess.run( + ["git", *args], + cwd=cwd, + capture_output=True, + text=True, + check=True, + timeout=10, + ) + + +@given('a temp git project with a worktree sandbox for plan "{plan_id}" for cwc') +def step_create_project_with_worktree(context: object, plan_id: str) -> None: + d = tempfile.mkdtemp(prefix="cwc-") + context.add_cleanup(shutil.rmtree, d, True) + _git(["init", "-q", "-b", "main"], d) + _git(["config", "user.name", "T"], d) + _git(["config", "user.email", "t@t"], d) + _git(["config", "commit.gpgsign", "false"], d) + Path(d, "file.py").write_text("content\n") + _git(["add", "."], d) + _git(["commit", "-q", "-m", "init"], d) + + branch = f"cleveragents/plan-{plan_id}" + wt_dir = tempfile.mkdtemp(prefix="cwc-wt-") + context.add_cleanup(shutil.rmtree, wt_dir, True) + _git(["worktree", "add", "-b", branch, wt_dir, "HEAD"], d) + + context.cwc_repo = d + context.cwc_wt_dir = wt_dir + context.cwc_plan_id = plan_id + + +@given("a mocked service that resolves the project for cwc") +def step_mock_service(context: object) -> None: + mock_resource = MagicMock() + mock_resource.resource_type_name = "git-checkout" + mock_resource.location = context.cwc_repo + mock_resource.resource_id = "res-cwc-test" + + mock_lr = MagicMock() + mock_lr.resource_id = "res-cwc-test" + + mock_project = MagicMock() + mock_project.linked_resources = [mock_lr] + + mock_plan = MagicMock() + mock_plan.project_links = [MagicMock(project_name="local/cwc-test")] + + mock_service = MagicMock() + mock_service.get_plan.return_value = mock_plan + + mock_project_repo = MagicMock() + mock_project_repo.get.return_value = mock_project + + mock_resource_registry = MagicMock() + mock_resource_registry.show_resource.return_value = mock_resource + + mock_container = MagicMock() + mock_container.namespaced_project_repo.return_value = mock_project_repo + mock_container.resource_registry_service.return_value = mock_resource_registry + + context.cwc_service = mock_service + context.cwc_container = mock_container + + +@given("a temp git project without any worktree for cwc") +def step_create_clean_project(context: object) -> None: + d = tempfile.mkdtemp(prefix="cwc-clean-") + context.add_cleanup(shutil.rmtree, d, True) + _git(["init", "-q", "-b", "main"], d) + _git(["config", "user.name", "T"], d) + _git(["config", "user.email", "t@t"], d) + _git(["config", "commit.gpgsign", "false"], d) + Path(d, "file.py").write_text("content\n") + _git(["add", "."], d) + _git(["commit", "-q", "-m", "init"], d) + context.cwc_repo = d + context.cwc_plan_id = "01TESTNOSANDBOX0000000000" + + +@given("a mocked service with no linked resources for cwc") +def step_mock_service_no_resources(context: object) -> None: + mock_plan = MagicMock() + mock_plan.project_links = [] + + mock_service = MagicMock() + mock_service.get_plan.return_value = mock_plan + + mock_container = MagicMock() + + context.cwc_service = mock_service + context.cwc_container = mock_container + + +@when('I call _cleanup_sandbox_for_plan for plan "{plan_id}" for cwc') +def step_call_cleanup(context: object, plan_id: str) -> None: + from cleveragents.cli.commands.plan import _cleanup_sandbox_for_plan + + with patch( + "cleveragents.application.container.get_container", + return_value=context.cwc_container, + ): + _cleanup_sandbox_for_plan(plan_id, context.cwc_service) + + context.cwc_cleanup_done = True + + +@then('the branch "{branch_name}" should not exist for cwc') +def step_branch_not_exists(context: object, branch_name: str) -> None: + result = subprocess.run( + ["git", "rev-parse", "--verify", f"refs/heads/{branch_name}"], + cwd=context.cwc_repo, + capture_output=True, + check=False, + timeout=10, + ) + assert result.returncode != 0, f"Branch {branch_name} still exists" + + +@then("the worktree directory should not exist for cwc") +def step_worktree_gone(context: object) -> None: + assert not os.path.exists(context.cwc_wt_dir), ( + f"Worktree directory still exists: {context.cwc_wt_dir}" + ) + + +@then("the call should complete without error for cwc") +def step_no_error(context: object) -> None: + assert context.cwc_cleanup_done is True diff --git a/src/cleveragents/cli/commands/plan.py b/src/cleveragents/cli/commands/plan.py index cf4d817a7..a53a2df72 100644 --- a/src/cleveragents/cli/commands/plan.py +++ b/src/cleveragents/cli/commands/plan.py @@ -1349,6 +1349,51 @@ def _get_lifecycle_service(): return container.plan_lifecycle_service() +def _cleanup_sandbox_for_plan( + plan_id: str, + service: PlanLifecycleService, +) -> None: + """Remove any worktree sandbox left by a previous execute. + + Resolves the plan's linked git-checkout resource and delegates + to :meth:`GitWorktreeSandbox.cleanup_stale` in the infrastructure + layer. Silently does nothing if no sandbox is found. + + Used by ``plan cancel`` to prevent resource leaks. + """ + from cleveragents.application.container import get_container + from cleveragents.infrastructure.sandbox.git_worktree import ( + GitWorktreeSandbox, + ) + + container = get_container() + plan = service.get_plan(plan_id) + project_names = [pl.project_name for pl in getattr(plan, "project_links", [])] + + for project_name in project_names: + try: + project = container.namespaced_project_repo().get(project_name) + except Exception: + continue + if project is None: + continue + for lr in getattr(project, "linked_resources", []): + try: + resource = container.resource_registry_service().show_resource( + lr.resource_id, + ) + except Exception: + continue + if ( + resource.resource_type_name not in ("git-checkout", "git") + or not resource.location + ): + continue + + if GitWorktreeSandbox.cleanup_stale(resource.location, plan_id): + return # Cleaned up — done + + def _create_sandbox_for_plan( plan_id: str, service: PlanLifecycleService, @@ -3252,6 +3297,9 @@ def cancel_plan( plan = service.cancel_plan(plan_id, reason=reason) + # Clean up any worktree sandbox left by a previous execute + _cleanup_sandbox_for_plan(plan_id, service) + if fmt != OutputFormat.RICH.value: data = _plan_spec_dict(plan) if reason: diff --git a/src/cleveragents/infrastructure/sandbox/git_worktree.py b/src/cleveragents/infrastructure/sandbox/git_worktree.py index 790baec0a..b5651d849 100644 --- a/src/cleveragents/infrastructure/sandbox/git_worktree.py +++ b/src/cleveragents/infrastructure/sandbox/git_worktree.py @@ -163,6 +163,86 @@ class GitWorktreeSandbox: """Context after creation, ``None`` before ``create``.""" return self._context + # -- class helpers ------------------------------------------------------- + + @classmethod + def cleanup_stale(cls, repo_path: str, plan_id: str) -> bool: + """Remove a stale worktree branch left by a previous execution. + + Idempotent — does nothing if no stale branch exists. + + Args: + repo_path: Absolute path to the git repository root. + plan_id: The plan ULID whose stale branch should be removed. + + Returns: + ``True`` if a stale branch was found and cleaned up, + ``False`` if no stale branch existed. + """ + branch_name = f"cleveragents/plan-{plan_id}" + + try: + _run_git( + ["rev-parse", "--verify", f"refs/heads/{branch_name}"], + cwd=repo_path, + ) + except (subprocess.CalledProcessError, subprocess.TimeoutExpired): + return False + + logger.info( + "Cleaning up stale sandbox branch: branch=%s repo=%s", + branch_name, + repo_path, + ) + + try: + wt_result = _run_git( + ["worktree", "list", "--porcelain"], + cwd=repo_path, + ) + for wt_block in wt_result.stdout.split("\n\n"): + if f"branch refs/heads/{branch_name}" in wt_block: + for line in wt_block.splitlines(): + if line.startswith("worktree "): + wt_path = line.split("worktree ", 1)[1] + try: + _run_git( + ["worktree", "remove", "--force", wt_path], + cwd=repo_path, + ) + except ( + subprocess.CalledProcessError, + subprocess.TimeoutExpired, + ): + logger.warning( + "git worktree remove failed; " + "removing directory manually: %s", + wt_path, + ) + shutil.rmtree(wt_path, ignore_errors=True) + except (subprocess.CalledProcessError, subprocess.TimeoutExpired): + logger.warning( + "Failed to list worktrees for stale cleanup: %s", + branch_name, + ) + + try: + _run_git(["branch", "-D", branch_name], cwd=repo_path) + except (subprocess.CalledProcessError, subprocess.TimeoutExpired): + logger.warning( + "Failed to delete stale branch %s", + branch_name, + ) + + with contextlib.suppress( + subprocess.CalledProcessError, + subprocess.TimeoutExpired, + ): + _run_git(["worktree", "prune"], cwd=repo_path) + + logger.info("Stale sandbox branch cleaned up: branch=%s", branch_name) + return True + # -- protocol methods ---------------------------------------------------- def create(self, plan_id: str) -> SandboxContext: