refactor(bottles): two-phase cleanup parallel to prepare/launch
test / run tests/run_tests.py (pull_request) Successful in 13s
test / run tests/run_tests.py (pull_request) Successful in 13s
cmd_cleanup used to only sweep running containers via `docker ps`,
missing stopped pipelock sidecars and orphaned networks entirely. On
my host the new version surfaced ~10 stranded networks left behind by
SIGKILLed sessions — the kind of thing the old command implied it was
handling.
New shape, symmetric with start:
- BottleCleanupPlan (abstract, in bottles/__init__.py) with `print` +
`empty` abstract members.
- DockerBottleCleanupPlan (concrete, in bottles/docker.py) carrying
the resolved tuples of containers and networks.
- BottlePlatform gains abstract prepare_cleanup() + cleanup(plan).
DockerBottlePlatform implements both:
- prepare_cleanup: docker ps -a + docker network ls, both
filtered to ^claude-bottle-, sorted for stable output.
- cleanup: docker rm -f containers first (they hold the network
attachment), then docker network rm.
- cmd_cleanup is now ~25 lines: prepare → print → y/N → cleanup.
This commit is contained in:
@@ -1,7 +1,7 @@
|
||||
"""Per-platform bottle factories.
|
||||
|
||||
A bottle is a running, isolated environment with claude inside. Each
|
||||
platform exposes two functions:
|
||||
platform exposes four methods:
|
||||
|
||||
prepare(spec, stage_dir=...) -> BottlePlan
|
||||
Resolves names, validates host-side prerequisites, and writes
|
||||
@@ -12,6 +12,13 @@ platform exposes two functions:
|
||||
Brings up the container (or VM, or remote machine), provisions
|
||||
it, yields a Bottle handle, and tears everything down on exit.
|
||||
|
||||
prepare_cleanup() -> BottleCleanupPlan
|
||||
Enumerates orphaned resources left behind by previous bottles
|
||||
(containers, networks, ...). Idempotent; no side effects.
|
||||
|
||||
cleanup(plan) -> None
|
||||
Actually removes everything described by the cleanup plan.
|
||||
|
||||
Selection is driven by CLAUDE_BOTTLE_PLATFORM (default "docker"). Per
|
||||
PRD 0003 the manifest does not carry a platform field; the host
|
||||
environment picks.
|
||||
@@ -58,6 +65,23 @@ class BottlePlan(ABC):
|
||||
"""Render the y/N preflight summary to stderr."""
|
||||
|
||||
|
||||
@dataclass(frozen=True)
|
||||
class BottleCleanupPlan(ABC):
|
||||
"""Base output of a platform's prepare_cleanup step. Concrete
|
||||
subclasses (e.g. DockerBottleCleanupPlan) carry platform-specific
|
||||
lists of resources to be removed and implement `print` + `empty`."""
|
||||
|
||||
@abstractmethod
|
||||
def print(self) -> None:
|
||||
"""Render the cleanup y/N summary to stderr."""
|
||||
|
||||
@property
|
||||
@abstractmethod
|
||||
def empty(self) -> bool:
|
||||
"""True iff there is nothing to clean up; the CLI uses this to
|
||||
short-circuit before showing the y/N."""
|
||||
|
||||
|
||||
class Bottle(Protocol):
|
||||
"""Handle to a running bottle. Yielded by a platform's launch step.
|
||||
|
||||
@@ -89,6 +113,15 @@ class BottlePlatform(ABC):
|
||||
def launch(self, plan: BottlePlan) -> AbstractContextManager[Bottle]:
|
||||
"""Build/run the bottle and yield a handle; tear down on exit."""
|
||||
|
||||
@abstractmethod
|
||||
def prepare_cleanup(self) -> BottleCleanupPlan:
|
||||
"""Enumerate orphaned resources from previous bottles. No side
|
||||
effects; safe to call before the y/N."""
|
||||
|
||||
@abstractmethod
|
||||
def cleanup(self, plan: BottleCleanupPlan) -> None:
|
||||
"""Remove everything described by the cleanup plan."""
|
||||
|
||||
|
||||
# Import concrete platform classes AFTER the base types are defined, so
|
||||
# each platform module can pull BottleSpec / BottlePlan / BottlePlatform
|
||||
@@ -114,6 +147,7 @@ def get_bottle_platform() -> BottlePlatform:
|
||||
|
||||
__all__ = [
|
||||
"Bottle",
|
||||
"BottleCleanupPlan",
|
||||
"BottlePlan",
|
||||
"BottlePlatform",
|
||||
"BottleSpec",
|
||||
|
||||
@@ -34,7 +34,7 @@ from .. import skills as skills_mod
|
||||
from .. import ssh as ssh_mod
|
||||
from ..env_resolve import env_resolve
|
||||
from ..log import die, info
|
||||
from . import BottlePlan, BottlePlatform, BottleSpec
|
||||
from . import BottleCleanupPlan, BottlePlan, BottlePlatform, BottleSpec
|
||||
|
||||
|
||||
# Where the repo root lives, for `docker build` context. Computed once.
|
||||
@@ -120,6 +120,31 @@ class DockerBottlePlan(BottlePlan):
|
||||
print(file=sys.stderr)
|
||||
|
||||
|
||||
# --- Cleanup plan ----------------------------------------------------------
|
||||
|
||||
|
||||
@dataclass(frozen=True)
|
||||
class DockerBottleCleanupPlan(BottleCleanupPlan):
|
||||
"""Resources DockerBottlePlatform.cleanup will remove. Produced by
|
||||
`prepare_cleanup` from a snapshot of `docker ps -a` + `docker
|
||||
network ls`; sorted so the y/N output is stable."""
|
||||
|
||||
containers: tuple[str, ...]
|
||||
networks: tuple[str, ...]
|
||||
|
||||
@property
|
||||
def empty(self) -> bool:
|
||||
return not self.containers and not self.networks
|
||||
|
||||
def print(self) -> None:
|
||||
print(file=sys.stderr)
|
||||
for name in self.containers:
|
||||
info(f"container: {name}")
|
||||
for name in self.networks:
|
||||
info(f"network: {name}")
|
||||
print(file=sys.stderr)
|
||||
|
||||
|
||||
# --- Bottle handle ---------------------------------------------------------
|
||||
|
||||
|
||||
@@ -438,3 +463,67 @@ class DockerBottlePlatform(BottlePlatform):
|
||||
)
|
||||
|
||||
return in_container_prompt_path if agent.prompt else None
|
||||
|
||||
# --- Cleanup ---
|
||||
|
||||
def prepare_cleanup(self) -> DockerBottleCleanupPlan:
|
||||
"""Enumerate all claude-bottle-prefixed containers (running or
|
||||
stopped) and networks. No removals — caller confirms first."""
|
||||
docker_mod.require_docker()
|
||||
|
||||
# `docker ps -a --filter name=...` uses regex matching; anchor at
|
||||
# the start so we don't pick up containers that merely contain
|
||||
# "claude-bottle-" mid-name.
|
||||
cr = subprocess.run(
|
||||
[
|
||||
"docker", "ps", "-a",
|
||||
"--filter", "name=^claude-bottle-",
|
||||
"--format", "{{.Names}}",
|
||||
],
|
||||
capture_output=True,
|
||||
text=True,
|
||||
)
|
||||
containers = tuple(sorted(
|
||||
line for line in (cr.stdout or "").splitlines() if line
|
||||
))
|
||||
|
||||
# `docker network ls --filter name=...` uses substring matching.
|
||||
# "claude-bottle-" is specific enough that false positives are
|
||||
# not a concern.
|
||||
nr = subprocess.run(
|
||||
[
|
||||
"docker", "network", "ls",
|
||||
"--filter", "name=claude-bottle-",
|
||||
"--format", "{{.Name}}",
|
||||
],
|
||||
capture_output=True,
|
||||
text=True,
|
||||
)
|
||||
networks = tuple(sorted(
|
||||
line for line in (nr.stdout or "").splitlines() if line
|
||||
))
|
||||
|
||||
return DockerBottleCleanupPlan(containers=containers, networks=networks)
|
||||
|
||||
def cleanup(self, plan: BottleCleanupPlan) -> None:
|
||||
"""Remove the containers and networks listed in the plan.
|
||||
Containers first; networks would refuse to delete while
|
||||
containers are still attached."""
|
||||
assert isinstance(plan, DockerBottleCleanupPlan), (
|
||||
f"DockerBottlePlatform.cleanup expects DockerBottleCleanupPlan, "
|
||||
f"got {type(plan).__name__}"
|
||||
)
|
||||
for name in plan.containers:
|
||||
info(f"removing container {name}")
|
||||
subprocess.run(
|
||||
["docker", "rm", "-f", name],
|
||||
stdout=subprocess.DEVNULL,
|
||||
stderr=subprocess.DEVNULL,
|
||||
)
|
||||
for name in plan.networks:
|
||||
info(f"removing network {name}")
|
||||
subprocess.run(
|
||||
["docker", "network", "rm", name],
|
||||
stdout=subprocess.DEVNULL,
|
||||
stderr=subprocess.DEVNULL,
|
||||
)
|
||||
|
||||
@@ -1,42 +1,31 @@
|
||||
"""cleanup: stop and remove all active claude-bottle containers."""
|
||||
"""cleanup: stop and remove all orphaned claude-bottle resources
|
||||
(containers + networks) left behind by previous bottles."""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import subprocess
|
||||
import sys
|
||||
|
||||
from .. import docker as docker_mod
|
||||
from ..bottles import get_bottle_platform
|
||||
from ..log import info
|
||||
from ._common import read_tty_line
|
||||
|
||||
|
||||
def cmd_cleanup(_argv: list[str]) -> int:
|
||||
docker_mod.require_docker()
|
||||
result = subprocess.run(
|
||||
["docker", "ps", "--filter", "name=^claude-bottle-", "--format", "{{.Names}}"],
|
||||
capture_output=True,
|
||||
text=True,
|
||||
)
|
||||
containers = (result.stdout or "").strip()
|
||||
if not containers:
|
||||
info("no active claude-bottle containers")
|
||||
platform = get_bottle_platform()
|
||||
plan = platform.prepare_cleanup()
|
||||
|
||||
if plan.empty:
|
||||
info("no claude-bottle resources to clean up")
|
||||
return 0
|
||||
print(file=sys.stderr)
|
||||
for name in containers.splitlines():
|
||||
info(f"found: {name}")
|
||||
print(file=sys.stderr)
|
||||
|
||||
plan.print()
|
||||
sys.stderr.write("claude-bottle: remove all of the above? [y/N] ")
|
||||
sys.stderr.flush()
|
||||
reply = read_tty_line()
|
||||
if reply not in ("y", "Y", "yes", "YES"):
|
||||
info("aborted")
|
||||
return 0
|
||||
for name in containers.splitlines():
|
||||
info(f"removing {name}")
|
||||
subprocess.run(
|
||||
["docker", "rm", "-f", name],
|
||||
stdout=subprocess.DEVNULL,
|
||||
stderr=subprocess.DEVNULL,
|
||||
)
|
||||
|
||||
platform.cleanup(plan)
|
||||
info("done")
|
||||
return 0
|
||||
|
||||
Reference in New Issue
Block a user