fix(ops): address review findings for cleanup commands
- Add best-effort active plan detection from sandbox mtimes to protect running plans from cleanup (B1) - Add ge= validators on all retention settings to prevent unsafe negative/zero values; checkpoint max requires ge=2 (B3, NB6) - Handle CoW sandbox plan_id extraction (ca-cow-sandbox- prefix) so copy-on-write sandboxes are also protected (NB4) - Add OSError guards in sandbox dir iteration for race safety (NB7) - Add TODO markers and CLI notice for unimplemented session cleanup (NB1) - Rebase onto develop-hamza-1 to incorporate SEC5 changes (B2)
This commit is contained in:
khyari hamza
@@ -168,20 +168,35 @@ class CleanupService:
|
||||
if not tmp.exists():
|
||||
return []
|
||||
dirs: list[Path] = []
|
||||
for prefix in ("ca-sandbox-", "ca-cow-sandbox-"):
|
||||
dirs.extend(
|
||||
p for p in tmp.iterdir() if p.is_dir() and p.name.startswith(prefix)
|
||||
)
|
||||
try:
|
||||
entries = list(tmp.iterdir())
|
||||
except OSError:
|
||||
return []
|
||||
for p in entries:
|
||||
try:
|
||||
is_dir = p.is_dir()
|
||||
except OSError:
|
||||
continue
|
||||
if is_dir and any(
|
||||
p.name.startswith(pfx) for pfx in ("ca-sandbox-", "ca-cow-sandbox-")
|
||||
):
|
||||
dirs.append(p)
|
||||
return dirs
|
||||
|
||||
def _extract_plan_id_from_sandbox(self, path: Path) -> str | None:
|
||||
"""Extract plan_id from a sandbox directory name."""
|
||||
"""Extract plan_id from a sandbox directory name.
|
||||
|
||||
Handles both standard (``ca-sandbox-<plan_id>-<random>``) and
|
||||
copy-on-write (``ca-cow-sandbox-<plan_id>-<random>``) formats.
|
||||
"""
|
||||
name = path.name
|
||||
if name.startswith("ca-sandbox-"):
|
||||
# Format: ca-sandbox-<plan_id>-<random>
|
||||
parts = name[len("ca-sandbox-") :].rsplit("-", 1)
|
||||
if len(parts) >= 1:
|
||||
return parts[0]
|
||||
for prefix in ("ca-cow-sandbox-", "ca-sandbox-"):
|
||||
if name.startswith(prefix):
|
||||
# Format: <prefix><plan_id>-<random>
|
||||
remainder = name[len(prefix) :]
|
||||
parts = remainder.rsplit("-", 1)
|
||||
if parts and parts[0]:
|
||||
return parts[0]
|
||||
return None
|
||||
|
||||
def _is_sandbox_stale(self, path: Path) -> bool:
|
||||
@@ -344,14 +359,22 @@ class CleanupService:
|
||||
return inactive
|
||||
|
||||
def _scan_sessions(self, report: CleanupReport) -> None:
|
||||
"""Placeholder scan — actual session scan requires DB access."""
|
||||
# Populated by the CLI command which has DB access.
|
||||
pass
|
||||
"""Placeholder scan — session cleanup requires DB access.
|
||||
|
||||
Session cleanup is not yet implemented in MVP. The
|
||||
``scan_inactive_sessions`` method is available for callers
|
||||
that can provide session dicts from the database.
|
||||
"""
|
||||
# TODO(CONC3): Wire DB session query when Container is
|
||||
# available at CLI startup.
|
||||
|
||||
def _purge_sessions(self, report: CleanupReport) -> None:
|
||||
"""Placeholder purge — actual session purge requires DB access."""
|
||||
# Populated by the CLI command which has DB access.
|
||||
pass
|
||||
"""Placeholder purge — session cleanup requires DB access.
|
||||
|
||||
Session deletion is not yet implemented in MVP.
|
||||
"""
|
||||
# TODO(CONC3): Wire DB session deletion when Container is
|
||||
# available at CLI startup.
|
||||
|
||||
# ── Log cleanup ───────────────────────────────────────────────
|
||||
|
||||
|
||||
@@ -6,6 +6,7 @@ stale sandboxes, checkpoints, sessions, logs, and backups.
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
from pathlib import Path
|
||||
from typing import TYPE_CHECKING, Annotated
|
||||
|
||||
import typer
|
||||
@@ -23,15 +24,64 @@ app = typer.Typer(
|
||||
)
|
||||
|
||||
|
||||
def _get_active_plan_ids() -> frozenset[str]:
|
||||
"""Best-effort query for currently active plan IDs.
|
||||
|
||||
Scans sandbox directory names for embedded plan IDs that have
|
||||
recent modification times (< 1 hour), treating them as likely
|
||||
active. Falls back to an empty set with a warning when the
|
||||
temp directory cannot be read.
|
||||
|
||||
A future version can query the plan lifecycle service directly
|
||||
once the Container is available at CLI startup.
|
||||
"""
|
||||
import tempfile
|
||||
import time
|
||||
|
||||
try:
|
||||
tmp = Path(tempfile.gettempdir())
|
||||
if not tmp.exists():
|
||||
return frozenset()
|
||||
active: set[str] = set()
|
||||
one_hour_ago = time.time() - 3600
|
||||
for p in tmp.iterdir():
|
||||
if not p.is_dir():
|
||||
continue
|
||||
name = p.name
|
||||
plan_id: str | None = None
|
||||
if name.startswith("ca-sandbox-"):
|
||||
parts = name[len("ca-sandbox-") :].rsplit("-", 1)
|
||||
if parts:
|
||||
plan_id = parts[0]
|
||||
elif name.startswith("ca-cow-sandbox-"):
|
||||
parts = name[len("ca-cow-sandbox-") :].rsplit("-", 1)
|
||||
if parts:
|
||||
plan_id = parts[0]
|
||||
if plan_id:
|
||||
try:
|
||||
mtime = p.stat().st_mtime
|
||||
if mtime > one_hour_ago:
|
||||
active.add(plan_id)
|
||||
except OSError:
|
||||
pass
|
||||
return frozenset(active)
|
||||
except OSError:
|
||||
err = get_err_console()
|
||||
err.print(
|
||||
"[yellow]Warning: Could not scan for active plans. "
|
||||
"Running plans will NOT be protected from cleanup.[/yellow]"
|
||||
)
|
||||
return frozenset()
|
||||
|
||||
|
||||
def _get_cleanup_service() -> CleanupService:
|
||||
"""Build a :class:`CleanupService` from current settings."""
|
||||
from cleveragents.application.services.cleanup_service import CleanupService
|
||||
from cleveragents.config.settings import get_settings
|
||||
|
||||
settings = get_settings()
|
||||
# In the future, active_plan_ids would be fetched from the DB.
|
||||
# For MVP, we pass an empty set.
|
||||
return CleanupService(settings=settings, active_plan_ids=frozenset())
|
||||
active_plan_ids = _get_active_plan_ids()
|
||||
return CleanupService(settings=settings, active_plan_ids=active_plan_ids)
|
||||
|
||||
|
||||
@app.command(name="scan")
|
||||
@@ -105,6 +155,12 @@ def purge(
|
||||
console.print("\n[bold]Cleanup Complete[/bold]\n")
|
||||
_print_summary(report)
|
||||
|
||||
if purge_all and report.sessions.removed == 0:
|
||||
console.print(
|
||||
"\n[dim]Note: Session cleanup is not yet implemented. "
|
||||
"Inactive sessions must be removed manually.[/dim]"
|
||||
)
|
||||
|
||||
|
||||
@app.command(name="status")
|
||||
def status() -> None:
|
||||
|
||||
@@ -151,28 +151,34 @@ class Settings(BaseSettings):
|
||||
# Cleanup / retention policies (CONC3)
|
||||
cleanup_sandbox_max_age_hours: int = Field(
|
||||
default=48,
|
||||
ge=1,
|
||||
validation_alias=AliasChoices("CLEVERAGENTS_CLEANUP_SANDBOX_MAX_AGE_HOURS"),
|
||||
description="Max age (hours) before stale sandboxes are eligible for cleanup.",
|
||||
)
|
||||
cleanup_checkpoint_max_per_plan: int = Field(
|
||||
default=50,
|
||||
ge=2,
|
||||
validation_alias=AliasChoices("CLEVERAGENTS_CHECKPOINT_MAX"),
|
||||
description=(
|
||||
"Max checkpoints per plan; oldest pruned first (keep first + most recent)."
|
||||
"Max checkpoints per plan; oldest pruned first (keep first + most recent). "
|
||||
"Minimum 2 to always preserve the first and most recent checkpoints."
|
||||
),
|
||||
)
|
||||
cleanup_session_inactivity_days: int = Field(
|
||||
default=30,
|
||||
ge=1,
|
||||
validation_alias=AliasChoices("CLEVERAGENTS_CLEANUP_SESSION_INACTIVITY_DAYS"),
|
||||
description="Days of inactivity before a session is eligible for cleanup.",
|
||||
)
|
||||
cleanup_log_retention_days: int = Field(
|
||||
default=30,
|
||||
ge=1,
|
||||
validation_alias=AliasChoices("CLEVERAGENTS_LOG_RETENTION_DAYS"),
|
||||
description="Days to retain log files before automatic cleanup.",
|
||||
)
|
||||
cleanup_backup_retention_days: int = Field(
|
||||
default=7,
|
||||
ge=1,
|
||||
validation_alias=AliasChoices("CLEVERAGENTS_BACKUP_RETENTION_DAYS"),
|
||||
description="Days to retain backup snapshots before automatic cleanup.",
|
||||
)
|
||||
|
||||
Reference in New Issue
Block a user