diff --git a/CONTRIBUTORS.md b/CONTRIBUTORS.md index 5ff33cad5..99ce630e9 100644 --- a/CONTRIBUTORS.md +++ b/CONTRIBUTORS.md @@ -101,3 +101,4 @@ Below are some specific details of individual PR contributions. * HAL 9000 has contributed the automated timeline snapshot update (PR #10288): added Schedule Adherence and Daily Snapshot tables for April 18 progress tracking, capturing milestone completion percentages, risk assessments, velocity projections, and ETAs across M3-M10. Includes malformed diff fix ensuring proper newline before table content. * HAL 9000 has contributed advanced context strategies integration tests (#10671, #7574): Behave scenarios with FakeEmbeddings for deterministic testing, Robot Framework E2E tests, and strategy implementation stubs covering semantic search, relevance scoring, adaptive selection, context fusion, YAML configuration, and ContextAssembler integration. * HAL 9000 has contributed the resource and skill management showcase alignment (#4213): updated the CLI tools showcase with consistent counts, explicit save instructions, metadata callouts, and README framing for platform walkthroughs; removed obsolete tdd_issue tags from coverage threshold Robot tests; hardened the Skip If No LLM Keys E2E helper with per-key regex validation and log suppression to prevent credential leakage. +* HAL 9000 has contributed the sandbox dirs cache invalidation fix (PR #11091 / issue #7527): introduced `SandboxDirsCache` to track filesystem paths of sandbox-created directories by plan_id, wired automatic invalidation into all cleanup/purge methods (`cleanup_all`, `cleanup_abandoned`, `clear_sandbox_dirs_cache`, `_cleanup_on_exit_handler`), and added BDD test coverage. diff --git a/features/sandbox_dirs_cache.feature b/features/sandbox_dirs_cache.feature new file mode 100644 index 000000000..fc5c0eae5 --- /dev/null +++ b/features/sandbox_dirs_cache.feature @@ -0,0 +1,63 @@ +@sandbox-dirs-cache +Feature: Sandbox directories cache tracking and invalidation (#7527) + Verifies that the SandboxDirsCache correctly records, tracks, and purges + sandbox directory paths by plan_id. + + Scenario: Record a sandbox directory path for a plan + Given an empty sandbox dirs cache + When I record dir "/tmp/sandboxes/worktree-abc123" for plan "plan-001" + Then the cache should contain one path for plan "plan-001" + And the total tracked plan count should be 1 + + Scenario: Record multiple directories for the same plan + Given an empty sandbox dirs cache + When I record dir "/tmp/sandboxes/worktree-abc" for plan "plan-002" + And I record dir "/tmp/sandboxes/overlay-def456" for plan "plan-002" + Then the cache should contain two paths for plan "plan-002" + And the total tracked path count should be 2 + + Scenario: Record directories for different plans + Given an empty sandbox dirs cache + When I record dir "/tmp/sandboxes/plan-a-dir" for plan "plan-a" + And I record dir "/tmp/sandboxes/plan-b-dir" for plan "plan-b" + Then the cache should contain two distinct plans + And the total tracked plan count should be 2 + + Scenario: Purge a single plan invalidates all its dirs + Given a sandbox dirs cache with one path "/tmp/sandboxes/worktree-x" for plan "plan-purge" + When I purge sandbox dirs for plan "plan-purge" + Then the cache should contain no paths for plan "plan-purge" + And the total tracked plan count should be 0 + + Scenario: Purge non-existent plan returns empty list + Given an empty sandbox dirs cache + When I purge sandbox dirs for plan "nonexistent-plan" + Then the purged path count should be 0 + And no plans should be removed from tracking + + Scenario: Check membership for recorded path + Given a sandbox dirs cache with one path "/tmp/sandboxes/abc" for plan "plan-check" + When I check if dir "/tmp/sandboxes/abc" belongs to plan "plan-check" + Then the result should be True + + Scenario: Check membership for non-recorded path returns False + Given a sandbox dirs cache with one path "/tmp/sandboxes/xyz" for plan "plan-wrong" + When I check if dir "/tmp/sandboxes/missing" belongs to plan "plan-wrong" + Then the result should be False + + Scenario: Check membership after purge returns False + Given a sandbox dirs cache with one path "/tmp/sandboxes/cached-dir" for plan "plan-expired" + When I purge sandbox dirs for plan "plan-expired" + And I check if dir "/tmp/sandboxes/cached-dir" belongs to plan "plan-expired" + Then the result should be False + + Scenario: Clear all entries empties the cache + Given a sandbox dirs cache with paths for plans "plan-clear-1" and "plan-clear-2" + When I clear all entries from the sandbox dirs cache + Then no plans should remain tracked + And total tracked path count should be 0 + + Scenario: Duplicate directory recording is idempotent + Given a sandbox dirs cache with one path "/tmp/sandboxes/uniq-dir" for plan "plan-uniq" + When I record dir "/tmp/sandboxes/uniq-dir" for plan "plan-uniq" again + Then the cache should still contain exactly one path for plan "plan-uniq" diff --git a/features/steps/sandbox_dirs_cache_steps.py b/features/steps/sandbox_dirs_cache_steps.py new file mode 100644 index 000000000..814d10946 --- /dev/null +++ b/features/steps/sandbox_dirs_cache_steps.py @@ -0,0 +1,143 @@ +"""Step definitions for sandbox dirs cache BDD tests (#7527 / PR #10989).""" + +from __future__ import annotations + +from behave import given, then, when + +from cleveragents.infrastructure.sandbox.dirs_cache import SandboxDirsCache + + +def _make_cache() -> SandboxDirsCache: + """Create a fresh cache instance, always starting clean.""" + return SandboxDirsCache() + + +# Each scenario sets context._sdc (SandboxDirsCache) and context._last_* for result tracking. + + +@given("an empty sandbox dirs cache") +def step_given_empty_cache(context): + context._sdc = SandboxDirsCache() + + +@given( + 'a sandbox dirs cache with one path "{dir_path}" for plan "{plan_id}"' +) +def step_given_one_dir(context, dir_path, plan_id): + context._sdc = SandboxDirsCache() + context._sdc.record(plan_id, dir_path) + + +@given( + 'a sandbox dirs cache with paths for plans "{plan_a}" and "{plan_b}"' +) +def step_given_two_plans(context, plan_a, plan_b): + context._sdc = SandboxDirsCache() + context._sdc.record(plan_a, f"/tmp/sandboxes/{plan_a}-dir") + context._sdc.record(plan_b, f"/tmp/sandboxes/{plan_b}-dir") + + +@when('I record dir "{dir_path}" for plan "{plan_id}"') +def step_when_record_one(context, dir_path, plan_id): + context._last_result = None + context._sdc.record(plan_id, dir_path) + + +@when('I record dir "{dir_path}" for plan "{plan_id}" again') +def step_when_record_duplicate(context, dir_path, plan_id): + context._last_result = None + context._sdc.record(plan_id, dir_path) + + +@when('I purge sandbox dirs for plan "{plan_id}"') +def step_when_purge(context, plan_id): + context._purged = context._sdc.purge(plan_id) + + +@when('I check if dir "{dir_path}" belongs to plan "{plan_id}"') +def step_when_check_membership(context, dir_path, plan_id): + context._check_result = context._sdc.get(plan_id, dir_path) + + +@when("I clear all entries from the sandbox dirs cache") +def step_when_clear_all(context): + context._sdc.clear() + + +@then('the cache should contain one path for plan "{plan_id}"') +def step_then_one_path_for_plan(context, plan_id): + actual = len(context._sdc._dirs.get(plan_id, set())) + assert actual == 1, f"Expected 1 path for {plan_id}, got {actual}" + + +@then('the cache should contain two paths for plan "{plan_id}"') +def step_then_two_paths_for_plan(context, plan_id): + actual = len(context._sdc._dirs.get(plan_id, set())) + assert actual == 2, f"Expected 2 paths for {plan_id}, got {actual}" + + +@then("the total tracked plan count should be 1") +def step_then_plan_count_one(context): + assert context._sdc.plan_count == 1 + + +@then("the total tracked path count should be 2") +def step_then_path_count_two(context): + assert context._sdc.dir_count == 2 + + +@then("the cache should contain two distinct plans") +def step_then_two_plans(context): + assert len(context._sdc._dirs) == 2 + + +@then("the total tracked plan count should be 2") +def step_then_plan_count_two(context): + assert context._sdc.plan_count == 2 + + +@then('the cache should contain no paths for plan "{plan_id}"') +def step_then_no_paths_for_plan(context, plan_id): + actual = len(context._sdc._dirs.get(plan_id, set())) + assert actual == 0, f"Expected 0 paths for {plan_id}, got {actual}" + + +@then("the total tracked plan count should be 0") +def step_then_plan_count_zero(context): + assert context._sdc.plan_count == 0 + + +@then("the purged path count should be 0") +def step_then_purged_count_zero(context): + assert len(context._purged) == 0 + + +@then("no plans should be removed from tracking") +def step_then_no_plans_removed(context): + assert context._sdc.plan_count == 0 + + +@then("the result should be True") +def step_then_membership_true(context): + assert context._check_result is True + + +@then("the result should be False") +def step_then_membership_false(context): + assert context._check_result is False + + +@then("no plans should remain tracked") +def step_then_no_plans_remaining(context): + assert context._sdc.plan_count == 0 + + +@then("total tracked path count should be 0") +def step_then_dir_count_zero(context): + assert context._sdc.dir_count == 0 + + +@then('the cache should still contain exactly one path for plan "{plan_id}"') +def step_then_one_path_after_rerecord(context, plan_id): + actual = len(context._sdc._dirs.get(plan_id, set())) + assert actual == 1, f"Expected 1 (idempotent), got {actual}" diff --git a/src/cleveragents/infrastructure/sandbox/__init__.py b/src/cleveragents/infrastructure/sandbox/__init__.py index 1a6545a1a..399e724d3 100644 --- a/src/cleveragents/infrastructure/sandbox/__init__.py +++ b/src/cleveragents/infrastructure/sandbox/__init__.py @@ -23,6 +23,7 @@ from cleveragents.infrastructure.sandbox.checkpoint import ( SandboxCheckpoint, ) from cleveragents.infrastructure.sandbox.copy_on_write import CopyOnWriteSandbox +from cleveragents.infrastructure.sandbox.dirs_cache import SandboxDirsCache from cleveragents.infrastructure.sandbox.factory import SandboxFactory from cleveragents.infrastructure.sandbox.git_worktree import GitWorktreeSandbox from cleveragents.infrastructure.sandbox.manager import SandboxManager @@ -73,6 +74,7 @@ __all__ = [ "SandboxBoundaryError", "SandboxCheckpoint", "SandboxContext", + "SandboxDirsCache", "SandboxError", "SandboxFactory", "SandboxManager", diff --git a/src/cleveragents/infrastructure/sandbox/dirs_cache.py b/src/cleveragents/infrastructure/sandbox/dirs_cache.py new file mode 100644 index 000000000..6fbd7335e --- /dev/null +++ b/src/cleveragents/infrastructure/sandbox/dirs_cache.py @@ -0,0 +1,147 @@ +"""Sandbox directories cache for CleverAgents. + +Tracks filesystem paths of sandbox-created directories indexed by plan_id, +enabling instant invalidation when those sandboxes are purged or cleaned up. +This prevents stale path references from being used after a sandbox's +filesystem artifacts have been removed. + +Stage B3.7 supplementary -- introduced for #7527 (PR #10989). +""" + +from __future__ import annotations + +import logging +import threading +from typing import Any + +logger = logging.getLogger(__name__) + + +class SandboxDirsCache: + """Thread-safe cache of sandbox directory paths, keyed by plan_id. + + When a sandbox is created, its filesystem path is recorded here. + When the sandbox is purged/cleaned up, the corresponding entries + are invalidated so that any subsequent lookup returns ``None`` + rather than pointing to a deleted path. + + Thread-safe: all mutable state is protected by ``_lock`` (an RLock). + + Usage:: + + cache = SandboxDirsCache() + cache.record("plan-001", "/tmp/sandboxes/worktree-abc") + # ... sandbox is purged ... + cache.purge("plan-001") # clears all dirs for this plan + cache.get("plan-001", "worktree-abc") # -> None (invalidated) + """ + + def __init__(self) -> None: + """Initialise the sandbox directories cache. + + The internal structure is a nested mapping of ``(plan_id, dir_path)`` + keys to ``True``. All access is serialised by ``_lock``. + """ + self._dirs: dict[str, set[str]] = {} + self._lock: threading.RLock = threading.RLock() + + def record(self, plan_id: str, dir_path: str) -> None: + """Record a sandbox directory path for a plan. + + Args: + plan_id: Identifier of the plan that owns this sandbox. + dir_path: Absolute filesystem path to the sandbox directory. + + Raises: + ValueError: If ``plan_id`` or ``dir_path`` is empty. + """ + if not plan_id: + raise ValueError("plan_id cannot be empty") + if not dir_path: + raise ValueError("dir_path cannot be empty") + + with self._lock: + self._dirs.setdefault(plan_id, set()).add(dir_path) + logger.debug( + "Recorded sandbox dir %s for plan=%s (total plans: %d)", + dir_path, + plan_id, + len(self._dirs), + ) + + def purge(self, plan_id: str) -> list[str]: + """Remove all recorded directories for a given plan. + + This invalidates every directory path that was previously recorded + under ``plan_id``, ensuring subsequent lookups return ``None``. + + Args: + plan_id: Identifier of the plan whose sandbox dirs should be + invalidated. + + Returns: + List of directory paths that were purged (empty if the plan + had no recorded dirs). + """ + with self._lock: + removed = list(self._dirs.pop(plan_id, set())) + + if removed: + logger.info( + "Purged %d sandbox dir(s) for plan=%s", len(removed), plan_id + ) + + return removed + + def get(self, plan_id: str, dir_path: str) -> bool: + """Check whether a sandbox directory is still recorded for *plan_id*. + + Args: + plan_id: Identifier of the plan to check. + dir_path: Sandbox directory path to look up. + + Returns: + ``True`` if the directory is still recorded (not yet purged). + + Raises: + ValueError: If ``plan_id`` or ``dir_path`` is empty. + """ + if not plan_id: + raise ValueError("plan_id cannot be empty") + if not dir_path: + raise ValueError("dir_path cannot be empty") + + with self._lock: + return dir_path in self._dirs.get(plan_id, set()) + + @property + def plan_count(self) -> int: + """Return the number of distinct plans tracked.""" + with self._lock: + return len(self._dirs) + + @property + def dir_count(self) -> int: + """Return the total number of recorded directory paths.""" + with self._lock: + return sum(len(paths) for paths in self._dirs.values()) + + def clear(self) -> None: + """Remove all tracked plans and their directories.""" + with self._lock: + count = len(self._dirs) + self._dirs.clear() + if count > 0: + logger.info("Cleared %d tracked plan(s) from sandbox dirs cache", count) + + def __contains__(self, args: tuple[str, str]) -> bool: + """Support ``"plan_id", "dir_path"`` tuple membership checks. + + Args: + args: ``(plan_id, dir_path)`` tuple. + + Returns: + ``True`` if recorded as valid for the given plan. + """ + plan_id, dir_path = args + return self.get(plan_id, dir_path) diff --git a/src/cleveragents/infrastructure/sandbox/manager.py b/src/cleveragents/infrastructure/sandbox/manager.py index 032e11f74..b64ac35b7 100644 --- a/src/cleveragents/infrastructure/sandbox/manager.py +++ b/src/cleveragents/infrastructure/sandbox/manager.py @@ -25,6 +25,7 @@ from cleveragents.infrastructure.sandbox.boundary import ( BoundaryCache, NoSandboxBoundaryError, ) +from cleveragents.infrastructure.sandbox.dirs_cache import SandboxDirsCache from cleveragents.infrastructure.sandbox.factory import ( SandboxFactory, SandboxStrategyStr, @@ -91,6 +92,7 @@ class SandboxManager: self._lock: threading.RLock = threading.RLock() self._cleanup_on_exit: bool = cleanup_on_exit self._boundary_cache: BoundaryCache = BoundaryCache() + self._sandbox_dirs_cache: SandboxDirsCache = SandboxDirsCache() if cleanup_on_exit: atexit.register(self._cleanup_on_exit_handler) @@ -178,6 +180,23 @@ class SandboxManager: return sandbox + def record_sandbox_dir(self, plan_id: str, dir_path: str) -> None: + """Record a sandbox directory path for later invalidation. + + When a sandbox creates a new filesystem directory (e.g., a + worktree or overlay), this registers the path so it can be + purged together with all other directories belonging to the + same plan in :meth:`purge_sandbox_dirs`. + + Args: + plan_id: Identifier of the owning plan. + dir_path: Absolute filesystem path of the sandbox directory. + + Raises: + ValueError: If ``plan_id`` or ``dir_path`` is empty. + """ + self._sandbox_dirs_cache.record(plan_id, dir_path) + def get_sandbox(self, plan_id: str, resource_id: str) -> Sandbox | None: """Look up an existing sandbox without creating a new one. @@ -481,6 +500,8 @@ class SandboxManager: ) with self._lock: + # Invalidate cached sandbox directory paths for this plan. + self.purge_sandbox_dirs(plan_id) self._active_sandboxes.pop(plan_id, None) def cleanup_abandoned(self) -> int: @@ -532,6 +553,8 @@ class SandboxManager: ) if all_cleaned and remaining: self._active_sandboxes.pop(plan_id, None) + # Invalidate cached sandbox directory paths. + self.purge_sandbox_dirs(plan_id) if cleaned > 0: logger.info("Cleaned up %d abandoned sandbox(es)", cleaned) @@ -641,6 +664,32 @@ class SandboxManager: """Return the number of cached boundary lookups.""" return self._boundary_cache.size + # -- sandbox dirs cache -------------------------------------------------- + + def purge_sandbox_dirs(self, plan_id: str) -> list[str]: + """Remove all recorded sandbox directories for a plan. + + Args: + plan_id: Identifier of the plan whose sandbox dirs to purge. + + Returns: + List of directory paths that were purged. + """ + return self._sandbox_dirs_cache.purge(plan_id) + + def clear_sandbox_dirs_cache(self) -> None: + """Clear all cached sandbox directory paths. + + Should be called at the start of each plan execution or when the + resource DAG is modified -- mirrors :meth:`clear_boundary_cache`. + """ + self._sandbox_dirs_cache.clear() + + @property + def sandbox_dirs_cache_size(self) -> int: + """Return the number of tracked plans in the sandbox dirs cache.""" + return self._sandbox_dirs_cache.plan_count + # -- internal ------------------------------------------------------------ def _cleanup_on_exit_handler(self) -> None: @@ -650,4 +699,5 @@ class SandboxManager: for plan_id in plan_ids: with contextlib.suppress(Exception): + self.purge_sandbox_dirs(plan_id) self.cleanup_all(plan_id)