From f4c088bae8b5c19a6414a954d4a5aa3087056e44 Mon Sep 17 00:00:00 2001 From: khyari hamza Date: Thu, 19 Feb 2026 13:58:33 +0000 Subject: [PATCH] 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) --- .../application/services/cleanup_service.py | 55 +++++++++++----- src/cleveragents/cli/commands/cleanup.py | 62 ++++++++++++++++++- src/cleveragents/config/settings.py | 8 ++- 3 files changed, 105 insertions(+), 20 deletions(-) diff --git a/src/cleveragents/application/services/cleanup_service.py b/src/cleveragents/application/services/cleanup_service.py index d1a5cb312..4e854fd9e 100644 --- a/src/cleveragents/application/services/cleanup_service.py +++ b/src/cleveragents/application/services/cleanup_service.py @@ -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--``) and + copy-on-write (``ca-cow-sandbox--``) formats. + """ name = path.name - if name.startswith("ca-sandbox-"): - # Format: ca-sandbox-- - 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: - + 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 ─────────────────────────────────────────────── diff --git a/src/cleveragents/cli/commands/cleanup.py b/src/cleveragents/cli/commands/cleanup.py index b9773f71a..516a087fe 100644 --- a/src/cleveragents/cli/commands/cleanup.py +++ b/src/cleveragents/cli/commands/cleanup.py @@ -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: diff --git a/src/cleveragents/config/settings.py b/src/cleveragents/config/settings.py index 05fc7effa..bc3446fe1 100644 --- a/src/cleveragents/config/settings.py +++ b/src/cleveragents/config/settings.py @@ -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.", )