fix(plan): clean up worktree sandbox on plan cancel
CI / helm (pull_request) Successful in 35s
CI / build (pull_request) Successful in 3m51s
CI / lint (pull_request) Successful in 3m56s
CI / quality (pull_request) Successful in 4m21s
CI / typecheck (pull_request) Successful in 4m32s
CI / security (pull_request) Successful in 4m43s
CI / push-validation (pull_request) Successful in 23s
CI / e2e_tests (pull_request) Successful in 7m51s
CI / integration_tests (pull_request) Successful in 8m32s
CI / unit_tests (pull_request) Successful in 9m6s
CI / docker (pull_request) Successful in 1m29s
CI / coverage (pull_request) Successful in 13m28s
CI / status-check (pull_request) Waiting to run
CI / helm (pull_request) Successful in 35s
CI / build (pull_request) Successful in 3m51s
CI / lint (pull_request) Successful in 3m56s
CI / quality (pull_request) Successful in 4m21s
CI / typecheck (pull_request) Successful in 4m32s
CI / security (pull_request) Successful in 4m43s
CI / push-validation (pull_request) Successful in 23s
CI / e2e_tests (pull_request) Successful in 7m51s
CI / integration_tests (pull_request) Successful in 8m32s
CI / unit_tests (pull_request) Successful in 9m6s
CI / docker (pull_request) Successful in 1m29s
CI / coverage (pull_request) Successful in 13m28s
CI / status-check (pull_request) Waiting to run
When a user cancels a plan after execute, the git worktree branch and directory created during execute are not cleaned up, causing resource leaks (dangling worktrees accumulate over time). Add GitWorktreeSandbox.cleanup_stale() classmethod in the infrastructure layer with idempotent error handling. _cleanup_sandbox_for_plan() in the CLI layer resolves the plan's linked git-checkout resource and delegates to cleanup_stale(). Called after service.cancel_plan() in the cancel CLI handler. ISSUES CLOSED: #9230
This commit is contained in:
@@ -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
|
||||
|
||||
@@ -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
|
||||
@@ -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
|
||||
@@ -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:
|
||||
|
||||
@@ -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:
|
||||
|
||||
Reference in New Issue
Block a user