diff --git a/CHANGELOG.md b/CHANGELOG.md index c16904dc4..00f6335c6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,12 @@ The format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ## [Unreleased] ### Fixed +- **CleanupService sandbox cache stale after purge** (#7527): `_purge_sandboxes()` + now invalidates `_sandbox_dirs_cache` after deleting directories so that a + subsequent `scan()` call on the same instance re-reads the filesystem instead + of returning already-deleted paths as stale items. Added BDD regression tests + (`features/cleanup_service_cache_invalidation.feature`) to verify correct cache + lifecycle across scan→purge→scan workflows. - **Cross-actor subgraph cycle detection reads actor_ref field** (#1431): Fixed `_detect_subgraph_cycles()`, `_map_node()`, and the `compile_actor()` main loop diff --git a/CONTRIBUTORS.md b/CONTRIBUTORS.md index 51815111f..b5b7b8326 100644 --- a/CONTRIBUTORS.md +++ b/CONTRIBUTORS.md @@ -8,6 +8,8 @@ * Luis Mendes * Rui Hu +* HAL 9000 has contributed automated bug fixes, cache invalidation improvements, and BDD test coverage for the CleanupService sandbox lifecycle (PR #8257, fixes #7527). + # Details Below are some of the specific details of various contributions. diff --git a/features/cleanup_service_cache_invalidation.feature b/features/cleanup_service_cache_invalidation.feature new file mode 100644 index 000000000..32c667a87 --- /dev/null +++ b/features/cleanup_service_cache_invalidation.feature @@ -0,0 +1,31 @@ +@unit @regression +Feature: CleanupService sandbox cache invalidation after purge (#7527) + As a platform operator running CleanupService in a daemon context + I want the sandbox directory cache to be invalidated after purge() + So that a subsequent scan() reflects the actual filesystem state + and does not report already-deleted paths as stale items + + Scenario: cache is None after _purge_sandboxes completes + Given cache invalidation has a CleanupService with a pre-populated sandbox cache + When cache invalidation calls _purge_sandboxes + Then cache invalidation sandbox dirs cache should be None + + Scenario: scan after purge does not return the previously created sandbox directories + Given cache invalidation has a CleanupService with stale sandbox directories on disk + When cache invalidation calls scan then purge then scan again + Then cache invalidation second scan should not contain the previously created sandbox directories + + Scenario: scan after purge does not return previously cached paths + Given cache invalidation has a CleanupService with a pre-populated sandbox cache + When cache invalidation calls purge then scan + Then cache invalidation scan result should not contain the pre-cached paths + + Scenario: cache is repopulated on next _get_sandbox_dirs call after purge + Given cache invalidation has a CleanupService with a pre-populated sandbox cache + When cache invalidation calls _purge_sandboxes then _get_sandbox_dirs + Then cache invalidation cache should be repopulated from filesystem + + Scenario: purge with no stale sandboxes still invalidates cache + Given cache invalidation has a CleanupService with a fresh non-stale sandbox cache + When cache invalidation calls _purge_sandboxes + Then cache invalidation sandbox dirs cache should be None diff --git a/features/steps/cleanup_service_cache_invalidation_steps.py b/features/steps/cleanup_service_cache_invalidation_steps.py new file mode 100644 index 000000000..a2c3246c6 --- /dev/null +++ b/features/steps/cleanup_service_cache_invalidation_steps.py @@ -0,0 +1,174 @@ +"""Step definitions for CleanupService sandbox cache invalidation tests (#7527). + +Verifies that ``_purge_sandboxes()`` invalidates ``_sandbox_dirs_cache`` +so that a subsequent ``scan()`` call re-reads the filesystem instead of +returning already-deleted paths. + +All tests use real filesystem operations — no mocks, no patches. +""" + +from __future__ import annotations + +import os +import shutil +import tempfile +import time +import uuid +from pathlib import Path + +from behave import given, then, when +from behave.runner import Context + +from cleveragents.application.services.cleanup_service import ( + CleanupReport, + CleanupService, +) +from cleveragents.config.settings import Settings + + +def _make_settings(**overrides: object) -> Settings: + s = Settings() + for key, value in overrides.items(): + object.__setattr__(s, key, value) + return s + + +_STALE_MAX_AGE_HOURS = 1 +_STALE_MTIME_OFFSET = 999_999 + + +def _unique_sandbox_name(prefix: str = "ca-sandbox-test") -> str: + return f"{prefix}-{uuid.uuid4().hex[:12]}" + + +def _make_real_stale_sandbox(name: str) -> Path: + tmp = Path(tempfile.gettempdir()) + sandbox = tmp / name + sandbox.mkdir(exist_ok=True) + old_time = time.time() - _STALE_MTIME_OFFSET + os.utime(str(sandbox), (old_time, old_time)) + return sandbox + + +def _make_real_fresh_sandbox(name: str) -> Path: + tmp = Path(tempfile.gettempdir()) + sandbox = tmp / name + sandbox.mkdir(exist_ok=True) + return sandbox + + +def _register_cleanup(context: Context, path: Path) -> None: + if not hasattr(context, "_cleanup_handlers"): + context._cleanup_handlers = [] + def _remove() -> None: + if path.exists(): + shutil.rmtree(str(path), ignore_errors=True) + context._cleanup_handlers.append(_remove) + + +@given("cache invalidation has a CleanupService with a pre-populated sandbox cache") +def step_cache_inv_prepopulated(context: Context) -> None: + context.cache_inv_settings = _make_settings(cleanup_sandbox_max_age_hours=_STALE_MAX_AGE_HOURS) + context.cache_inv_service = CleanupService(context.cache_inv_settings) + na = _unique_sandbox_name("ca-sandbox-plan-aaa") + nb = _unique_sandbox_name("ca-sandbox-plan-bbb") + da = _make_real_stale_sandbox(na) + db = _make_real_stale_sandbox(nb) + _register_cleanup(context, da) + _register_cleanup(context, db) + context.cache_inv_service._sandbox_dirs_cache = [da, db] + context.cache_inv_pre_cached_paths = [da, db] + + +@given("cache invalidation has a CleanupService with stale sandbox directories on disk") +def step_cache_inv_real_stale(context: Context) -> None: + na = _unique_sandbox_name("ca-sandbox-planA") + nb = _unique_sandbox_name("ca-sandbox-planB") + da = _make_real_stale_sandbox(na) + db = _make_real_stale_sandbox(nb) + _register_cleanup(context, da) + _register_cleanup(context, db) + context.cache_inv_settings = _make_settings(cleanup_sandbox_max_age_hours=_STALE_MAX_AGE_HOURS) + context.cache_inv_service = CleanupService(context.cache_inv_settings) + context.cache_inv_real_dirs = [da, db] + + +@given("cache invalidation has a CleanupService with a fresh non-stale sandbox cache") +def step_cache_inv_fresh(context: Context) -> None: + context.cache_inv_settings = _make_settings() + context.cache_inv_service = CleanupService(context.cache_inv_settings) + nm = _unique_sandbox_name("ca-sandbox-plan-fresh") + fd = _make_real_fresh_sandbox(nm) + _register_cleanup(context, fd) + context.cache_inv_service._sandbox_dirs_cache = [fd] + context.cache_inv_fresh_dir = fd + + +@when("cache invalidation calls _purge_sandboxes") +def step_cache_inv_purge(context: Context) -> None: + report = CleanupReport(dry_run=False) + context.cache_inv_service._purge_sandboxes(report) + context.cache_inv_report = report + + +@when("cache invalidation calls scan then purge then scan again") +def step_cache_inv_scan_purge_scan(context: Context) -> None: + svc = context.cache_inv_service + svc._sandbox_dirs_cache = list(context.cache_inv_real_dirs) + context.cache_inv_first_scan = svc.scan() + context.cache_inv_purge_report = svc.purge() + context.cache_inv_second_scan = svc.scan() + + +@when("cache invalidation calls purge then scan") +def step_cache_inv_purge_scan(context: Context) -> None: + svc = context.cache_inv_service + context.cache_inv_purge_report = svc.purge() + context.cache_inv_scan_after_purge = svc.scan() + + +@when("cache invalidation calls _purge_sandboxes then _get_sandbox_dirs") +def step_cache_inv_purge_get_dirs(context: Context) -> None: + svc = context.cache_inv_service + report = CleanupReport(dry_run=False) + svc._purge_sandboxes(report) + nm = _unique_sandbox_name("ca-sandbox-plan-new") + nd = _make_real_stale_sandbox(nm) + _register_cleanup(context, nd) + context.cache_inv_new_dir = nd + context.cache_inv_repopulated = svc._get_sandbox_dirs() + + +@then("cache invalidation sandbox dirs cache should be None") +def step_cache_inv_none(context: Context) -> None: + assert context.cache_inv_service._sandbox_dirs_cache is None, ( + f"Expected _sandbox_dirs_cache to be None after purge, got {context.cache_inv_service._sandbox_dirs_cache!r}" + ) + + +@then("cache invalidation second scan should not contain the previously created sandbox directories") +def step_cache_inv_second_scan(context: Context) -> None: + ss = context.cache_inv_second_scan + cp = {str(d) for d in context.cache_inv_real_dirs} + sp = {i.path for i in ss.stale_items if i.resource_type == "sandbox"} + overlap = cp & sp + assert not overlap, f"Second scan still contains previously created sandbox dirs: {overlap}" + + +@then("cache invalidation scan result should not contain the pre-cached paths") +def step_cache_inv_no_precached(context: Context) -> None: + sr = context.cache_inv_scan_after_purge + cp = {str(p) for p in context.cache_inv_pre_cached_paths} + sp = {i.path for i in sr.stale_items} + overlap = cp & sp + assert not overlap, f"Scan after purge still contains pre-cached paths: {overlap}" + + +@then("cache invalidation cache should be repopulated from filesystem") +def step_cache_inv_repopulated(context: Context) -> None: + svc = context.cache_inv_service + nd = context.cache_inv_new_dir + rep = context.cache_inv_repopulated + assert nd in rep, f"Expected repopulated to contain {nd}, got {rep!r}" + assert svc._sandbox_dirs_cache is not None, "Expected cache to be set after _get_sandbox_dirs" + assert nd in svc._sandbox_dirs_cache, f"Expected cache to contain {nd}, got {svc._sandbox_dirs_cache!r}" diff --git a/src/cleveragents/application/services/cleanup_service.py b/src/cleveragents/application/services/cleanup_service.py index 8406ed986..b9f8d95bc 100644 --- a/src/cleveragents/application/services/cleanup_service.py +++ b/src/cleveragents/application/services/cleanup_service.py @@ -174,7 +174,13 @@ class CleanupService: ) def _purge_sandboxes(self, report: CleanupReport) -> None: - """Remove stale sandbox directories.""" + """Remove stale sandbox directories. + + Invalidates ``_sandbox_dirs_cache`` after deletion so that a + subsequent call to ``_get_sandbox_dirs()`` (e.g. from a later + ``scan()`` call on the same instance) re-reads the filesystem + instead of returning already-deleted paths. + """ dirs = self._get_sandbox_dirs() for d in dirs: report.sandboxes.scanned += 1 @@ -191,6 +197,10 @@ class CleanupService: report.sandboxes.removed += 1 except OSError: report.sandboxes.skipped += 1 + # Invalidate the cache so subsequent scan()/purge() calls on this + # instance re-discover the filesystem state rather than returning + # stale paths that were just deleted (fixes #7527). + self._sandbox_dirs_cache = None # ── Checkpoint cleanup ────────────────────────────────────────