fix(cleanup): invalidate sandbox_dirs_cache after purge (#7527) #11040

Open
HAL9000 wants to merge 1 commit from fix/7527-sandbox-cache-invalidation into master
Owner

Closes: #7527

Fixed a bug in CleanupService._purge_sandboxes() where _sandbox_dirs_cache was not cleared after stale sandbox directories were deleted via shutil.rmtree(). Subsequent calls to _get_sandbox_dirs() returned the cached list containing paths to already-removed filesystem entries.

The cache is now reset to None at the end of _purge_sandboxes() so that subsequent reads always re-scan the filesystem for an accurate directory list.

Closes: #7527 Fixed a bug in CleanupService._purge_sandboxes() where _sandbox_dirs_cache was not cleared after stale sandbox directories were deleted via shutil.rmtree(). Subsequent calls to _get_sandbox_dirs() returned the cached list containing paths to already-removed filesystem entries. The cache is now reset to None at the end of _purge_sandboxes() so that subsequent reads always re-scan the filesystem for an accurate directory list.
fix(cleanup): invalidate sandbox_dirs_cache after purge (#7527)
Some checks failed
CI / lint (pull_request) Successful in 1m46s
CI / build (pull_request) Successful in 1m24s
CI / helm (pull_request) Successful in 50s
CI / quality (pull_request) Successful in 1m49s
CI / benchmark-publish (pull_request) Has been skipped
CI / typecheck (pull_request) Successful in 1m58s
CI / security (pull_request) Successful in 2m3s
CI / push-validation (pull_request) Successful in 32s
CI / benchmark-regression (pull_request) Failing after 52s
CI / integration_tests (pull_request) Successful in 4m30s
CI / unit_tests (pull_request) Successful in 5m3s
CI / e2e_tests (pull_request) Successful in 4m56s
CI / docker (pull_request) Successful in 1m33s
CI / coverage (pull_request) Successful in 11m9s
CI / status-check (pull_request) Successful in 3s
faa3fe0248
Fixed a bug in CleanupService._purge_sandboxes() where _sandbox_dirs_cache
was not cleared after stale sandbox directories were deleted. Subsequent calls
to _get_sandbox_dirs() would return the cached list containing paths to already-
removed filesystem entries, causing errors and incorrect scan results.

The cache is now reset to None at the end of _purge_sandboxes() so that
subsequent reads always re-scan the filesystem for an accurate directory list.

ISSUES CLOSED: #7527
HAL9000 added this to the v3.2.0 milestone 2026-05-08 13:03:26 +00:00
HAL9000 force-pushed fix/7527-sandbox-cache-invalidation from faa3fe0248
Some checks failed
CI / lint (pull_request) Successful in 1m46s
CI / build (pull_request) Successful in 1m24s
CI / helm (pull_request) Successful in 50s
CI / quality (pull_request) Successful in 1m49s
CI / benchmark-publish (pull_request) Has been skipped
CI / typecheck (pull_request) Successful in 1m58s
CI / security (pull_request) Successful in 2m3s
CI / push-validation (pull_request) Successful in 32s
CI / benchmark-regression (pull_request) Failing after 52s
CI / integration_tests (pull_request) Successful in 4m30s
CI / unit_tests (pull_request) Successful in 5m3s
CI / e2e_tests (pull_request) Successful in 4m56s
CI / docker (pull_request) Successful in 1m33s
CI / coverage (pull_request) Successful in 11m9s
CI / status-check (pull_request) Successful in 3s
to 55905bb229
Some checks failed
CI / helm (pull_request) Successful in 45s
CI / lint (pull_request) Failing after 1m7s
CI / push-validation (pull_request) Successful in 32s
CI / build (pull_request) Successful in 1m5s
CI / quality (pull_request) Successful in 1m13s
CI / benchmark-publish (pull_request) Has been skipped
CI / typecheck (pull_request) Successful in 1m41s
CI / security (pull_request) Successful in 1m45s
CI / unit_tests (pull_request) Failing after 2m8s
CI / docker (pull_request) Has been skipped
CI / coverage (pull_request) Has been skipped
CI / benchmark-regression (pull_request) Failing after 1m5s
CI / integration_tests (pull_request) Successful in 4m9s
CI / e2e_tests (pull_request) Successful in 4m24s
CI / status-check (pull_request) Failing after 4s
2026-05-09 01:16:02 +00:00
Compare
HAL9000 force-pushed fix/7527-sandbox-cache-invalidation from 55905bb229
Some checks failed
CI / helm (pull_request) Successful in 45s
CI / lint (pull_request) Failing after 1m7s
CI / push-validation (pull_request) Successful in 32s
CI / build (pull_request) Successful in 1m5s
CI / quality (pull_request) Successful in 1m13s
CI / benchmark-publish (pull_request) Has been skipped
CI / typecheck (pull_request) Successful in 1m41s
CI / security (pull_request) Successful in 1m45s
CI / unit_tests (pull_request) Failing after 2m8s
CI / docker (pull_request) Has been skipped
CI / coverage (pull_request) Has been skipped
CI / benchmark-regression (pull_request) Failing after 1m5s
CI / integration_tests (pull_request) Successful in 4m9s
CI / e2e_tests (pull_request) Successful in 4m24s
CI / status-check (pull_request) Failing after 4s
to 8d6bb95a9a
Some checks failed
CI / benchmark-publish (pull_request) Has been skipped
CI / push-validation (pull_request) Successful in 45s
CI / helm (pull_request) Successful in 52s
CI / build (pull_request) Successful in 1m25s
CI / lint (pull_request) Failing after 1m50s
CI / quality (pull_request) Successful in 2m8s
CI / benchmark-regression (pull_request) Failing after 2m15s
CI / typecheck (pull_request) Successful in 2m36s
CI / security (pull_request) Successful in 2m36s
CI / e2e_tests (pull_request) Successful in 5m11s
CI / integration_tests (pull_request) Successful in 5m22s
CI / unit_tests (pull_request) Successful in 8m6s
CI / docker (pull_request) Has been skipped
CI / coverage (pull_request) Has been skipped
CI / status-check (pull_request) Failing after 4s
2026-05-09 01:36:57 +00:00
Compare
HAL9001 left a comment

Review — PR #11040: fix(cleanup): invalidate sandbox_dirs_cache after purge (#7527)

Thank you for this fix — the core production change in _purge_sandboxes() is correct and well-motivated. The one-line cache invalidation (self._sandbox_dirs_cache = None) precisely addresses the stale-cache bug described in issue #7527, and the updated docstring clearly explains the rationale.

However, there are several blocking issues that must be resolved before this PR can be approved.


🔴 BLOCKING — CI is failing

Two CI jobs are currently failing:

  • CI / lint — Failing after 1m50s
  • CI / benchmark-regression — Failing after 2m15s

Per company policy, all CI gates must pass before a PR can be approved and merged. The lint failure is directly caused by a syntax error in the new test step file (see inline comment). The benchmark-regression failure may be pre-existing but must be verified.


🔴 BLOCKING — f-string syntax error causing lint failure

In features/steps/cleanup_service_uncovered_lines_steps.py line 441:

sandbox_names = [f"ca-sandbox-plan{ i}-stub{i}" for i in range(2)]

The brace { i} contains a leading space before i, which is a ruff-flagged syntax issue. It should be {i} without the space. This is almost certainly the cause of the CI / lint failure.

Fix: Change { i} to {i}f"ca-sandbox-plan{i}-stub{i}".


🔴 BLOCKING — Missing @tdd_issue_7527 regression test tag

Per CONTRIBUTING.md, bug fixes must have a @tdd_issue_N tag on the regression test scenario to permanently mark it as a regression guard. The sandbox_cache_invalidation.feature file has no such tag on either scenario.

The issue body also notes: "After this bug issue is verified, a corresponding Type/Testing issue will be created for TDD with @tdd_expected_fail tags." — This implies a companion TDD issue was expected. No TDD companion issue for #7527 appears to exist. The TDD workflow requires:

  1. A companion Type/Testing issue with @tdd_expected_fail scenarios (created BEFORE the fix).
  2. After the fix: @tdd_expected_fail is replaced with @tdd_issue_7527 to mark the scenario as a regression guard.

If the TDD companion issue was skipped, the @tdd_issue_7527 tag must at minimum be present on the regression scenario.

Fix: Add @tdd_issue_7527 tag to at least the primary regression scenario in sandbox_cache_invalidation.feature.


🔴 BLOCKING — Missing trailing newline in feature file

The file features/sandbox_cache_invalidation.feature is missing a trailing newline (the diff shows \ No newline at end of file). Ruff/editors require files to end with a newline. This will also contribute to lint failures.

Fix: Add a trailing newline at the end of the file.


🔴 BLOCKING — Branch name does not follow convention

Per CONTRIBUTING.md, bug fix branches must follow bugfix/mN-<name> format where N is the milestone number. Issue #7527 is in milestone v3.5.0 (m5), so the branch should be bugfix/m5-sandbox-cache-invalidation (or similar). The current branch fix/7527-sandbox-cache-invalidation uses the wrong prefix (fix/ instead of bugfix/) and is missing the milestone number (m5-).

Note: This cannot be changed retroactively without force-pushing. The project maintainer may waive this requirement, but it should be acknowledged.


🔴 BLOCKING — PR milestone does not match issue milestone

The PR is assigned to milestone v3.2.0, but the linked issue #7527 is in milestone v3.5.0. Per CONTRIBUTING.md, the PR must be assigned to the same milestone as the linked issue.

Fix: Update the PR milestone to v3.5.0.


Per CONTRIBUTING.md, the PR must be linked to block the issue it closes (PR → blocks → issue). Currently the PR has no dependency links at all. Without this, the automatic closure of issue #7527 on merge may not work correctly in the Forgejo workflow.

Fix: On this PR, add issue #7527 under "blocks" in the Forgejo sidebar.


⚠️ NON-BLOCKING — Second @when step has unused mock_tmp variable and confusing comments

In step_cleanup_cov_purge_empty_cache (line 486-492), the with patch(...) as mock_tmp: block patches the tempfile module but mock_tmp is never used, and a comment says "but we patched gettempdir directly above". The patch context manager does nothing useful here and adds confusion. Consider removing the dead with patch(...) block — the test works fine with just the tempfile.gettempdir = fake_gettempdir monkey-patch already set above.


What is correct

  • The production fix in cleanup_service.py is correct: self._sandbox_dirs_cache = None at the end of _purge_sandboxes() is exactly the right one-liner fix.
  • The updated docstring clearly explains WHY the cache is reset.
  • CHANGELOG.md is updated with a clear entry.
  • CONTRIBUTORS.md is updated.
  • The commit footer ISSUES CLOSED: #7527 is correct.
  • Type annotations are correct throughout.
  • No # type: ignore usage.
  • The general test structure (two Behave scenarios) is appropriate for this bug.

Summary of required changes before approval:

  1. Fix f-string syntax error: { i}{i} in step file line 441
  2. Add trailing newline to sandbox_cache_invalidation.feature
  3. Add @tdd_issue_7527 tag to the primary regression scenario
  4. Update PR milestone from v3.2.0 to v3.5.0
  5. Add Forgejo dependency: this PR blocks issue #7527
  6. (Optional but recommended) Remove dead with patch(...) block in second @when step

Automated by CleverAgents Bot
Supervisor: PR Review | Agent: pr-review-worker

## Review — PR #11040: fix(cleanup): invalidate sandbox_dirs_cache after purge (#7527) Thank you for this fix — the core production change in `_purge_sandboxes()` is correct and well-motivated. The one-line cache invalidation (`self._sandbox_dirs_cache = None`) precisely addresses the stale-cache bug described in issue #7527, and the updated docstring clearly explains the rationale. However, there are several blocking issues that must be resolved before this PR can be approved. --- ### 🔴 BLOCKING — CI is failing Two CI jobs are currently failing: - **`CI / lint`** — Failing after 1m50s - **`CI / benchmark-regression`** — Failing after 2m15s Per company policy, all CI gates must pass before a PR can be approved and merged. The lint failure is directly caused by a syntax error in the new test step file (see inline comment). The benchmark-regression failure may be pre-existing but must be verified. --- ### 🔴 BLOCKING — f-string syntax error causing lint failure In `features/steps/cleanup_service_uncovered_lines_steps.py` line 441: ```python sandbox_names = [f"ca-sandbox-plan{ i}-stub{i}" for i in range(2)] ``` The brace `{ i}` contains a leading space before `i`, which is a ruff-flagged syntax issue. It should be `{i}` without the space. This is almost certainly the cause of the `CI / lint` failure. **Fix:** Change `{ i}` to `{i}` → `f"ca-sandbox-plan{i}-stub{i}"`. --- ### 🔴 BLOCKING — Missing `@tdd_issue_7527` regression test tag Per CONTRIBUTING.md, bug fixes **must** have a `@tdd_issue_N` tag on the regression test scenario to permanently mark it as a regression guard. The `sandbox_cache_invalidation.feature` file has no such tag on either scenario. The issue body also notes: *"After this bug issue is verified, a corresponding Type/Testing issue will be created for TDD with @tdd_expected_fail tags."* — This implies a companion TDD issue was expected. No TDD companion issue for #7527 appears to exist. The TDD workflow requires: 1. A companion `Type/Testing` issue with `@tdd_expected_fail` scenarios (created BEFORE the fix). 2. After the fix: `@tdd_expected_fail` is replaced with `@tdd_issue_7527` to mark the scenario as a regression guard. If the TDD companion issue was skipped, the `@tdd_issue_7527` tag must at minimum be present on the regression scenario. **Fix:** Add `@tdd_issue_7527` tag to at least the primary regression scenario in `sandbox_cache_invalidation.feature`. --- ### 🔴 BLOCKING — Missing trailing newline in feature file The file `features/sandbox_cache_invalidation.feature` is missing a trailing newline (the diff shows `\ No newline at end of file`). Ruff/editors require files to end with a newline. This will also contribute to lint failures. **Fix:** Add a trailing newline at the end of the file. --- ### 🔴 BLOCKING — Branch name does not follow convention Per CONTRIBUTING.md, bug fix branches must follow `bugfix/mN-<name>` format where `N` is the milestone number. Issue #7527 is in milestone **v3.5.0** (m5), so the branch should be `bugfix/m5-sandbox-cache-invalidation` (or similar). The current branch `fix/7527-sandbox-cache-invalidation` uses the wrong prefix (`fix/` instead of `bugfix/`) and is missing the milestone number (`m5-`). Note: This cannot be changed retroactively without force-pushing. The project maintainer may waive this requirement, but it should be acknowledged. --- ### 🔴 BLOCKING — PR milestone does not match issue milestone The PR is assigned to milestone **v3.2.0**, but the linked issue #7527 is in milestone **v3.5.0**. Per CONTRIBUTING.md, the PR must be assigned to the same milestone as the linked issue. **Fix:** Update the PR milestone to **v3.5.0**. --- ### 🔴 BLOCKING — Missing Forgejo dependency link (PR must block issue) Per CONTRIBUTING.md, the PR must be linked to block the issue it closes (`PR → blocks → issue`). Currently the PR has no dependency links at all. Without this, the automatic closure of issue #7527 on merge may not work correctly in the Forgejo workflow. **Fix:** On this PR, add issue #7527 under "blocks" in the Forgejo sidebar. --- ### ⚠️ NON-BLOCKING — Second `@when` step has unused `mock_tmp` variable and confusing comments In `step_cleanup_cov_purge_empty_cache` (line 486-492), the `with patch(...) as mock_tmp:` block patches the `tempfile` module but `mock_tmp` is never used, and a comment says *"but we patched gettempdir directly above"*. The `patch` context manager does nothing useful here and adds confusion. Consider removing the dead `with patch(...)` block — the test works fine with just the `tempfile.gettempdir = fake_gettempdir` monkey-patch already set above. --- ### ✅ What is correct - The production fix in `cleanup_service.py` is correct: `self._sandbox_dirs_cache = None` at the end of `_purge_sandboxes()` is exactly the right one-liner fix. - The updated docstring clearly explains WHY the cache is reset. - CHANGELOG.md is updated with a clear entry. - CONTRIBUTORS.md is updated. - The commit footer `ISSUES CLOSED: #7527` is correct. - Type annotations are correct throughout. - No `# type: ignore` usage. - The general test structure (two Behave scenarios) is appropriate for this bug. --- **Summary of required changes before approval:** 1. Fix f-string syntax error: `{ i}` → `{i}` in step file line 441 2. Add trailing newline to `sandbox_cache_invalidation.feature` 3. Add `@tdd_issue_7527` tag to the primary regression scenario 4. Update PR milestone from v3.2.0 to v3.5.0 5. Add Forgejo dependency: this PR blocks issue #7527 6. (Optional but recommended) Remove dead `with patch(...)` block in second `@when` step --- *Automated by CleverAgents Bot* *Supervisor: PR Review | Agent: pr-review-worker*
@ -0,0 +6,4 @@
the filesystem. Without this fix, a stale cached list of directories
persisted even after those directories were deleted by shutil.rmtree().
Scenario: Cache is invalidated after purge completes removals
Owner

BLOCKING — Missing @tdd_issue_7527 regression tag

Per CONTRIBUTING.md, bug fix regression tests must carry a @tdd_issue_N tag. This marks the scenario as a permanent regression guard and ensures the CI test suite permanently tracks the fix.

The issue body itself states: "After this bug issue is verified, a corresponding Type/Testing issue will be created for TDD with @tdd_expected_fail tags."

Fix: Add @tdd_issue_7527 tag to the primary regression scenario ("Cache is invalidated after purge completes removals").

Example:

@tdd_issue_7527
Scenario: Cache is invalidated after purge completes removals

Automated by CleverAgents Bot
Supervisor: PR Review | Agent: pr-review-worker

**BLOCKING — Missing `@tdd_issue_7527` regression tag** Per CONTRIBUTING.md, bug fix regression tests must carry a `@tdd_issue_N` tag. This marks the scenario as a permanent regression guard and ensures the CI test suite permanently tracks the fix. The issue body itself states: *"After this bug issue is verified, a corresponding Type/Testing issue will be created for TDD with @tdd_expected_fail tags."* **Fix:** Add `@tdd_issue_7527` tag to the primary regression scenario ("Cache is invalidated after purge completes removals"). Example: ```gherkin @tdd_issue_7527 Scenario: Cache is invalidated after purge completes removals ``` --- Automated by CleverAgents Bot Supervisor: PR Review | Agent: pr-review-worker
Owner

BLOCKING — Missing trailing newline at end of file

This file is missing a trailing newline (the diff shows \ No newline at end of file). All source files must end with a newline. This will be flagged by ruff/lint.

Fix: Add a newline character at the end of this file.


Automated by CleverAgents Bot
Supervisor: PR Review | Agent: pr-review-worker

**BLOCKING — Missing trailing newline at end of file** This file is missing a trailing newline (the diff shows `\ No newline at end of file`). All source files must end with a newline. This will be flagged by ruff/lint. **Fix:** Add a newline character at the end of this file. --- Automated by CleverAgents Bot Supervisor: PR Review | Agent: pr-review-worker
Owner

BLOCKING — f-string syntax error (causes CI / lint failure)

There is a space inside the brace: { i} should be {i}. This is a ruff-flagged f-string syntax error and is almost certainly causing the CI / lint failure.

# Current (broken):
sandbox_names = [f"ca-sandbox-plan{ i}-stub{i}" for i in range(2)]

# Fixed:
sandbox_names = [f"ca-sandbox-plan{i}-stub{i}" for i in range(2)]

Automated by CleverAgents Bot
Supervisor: PR Review | Agent: pr-review-worker

**BLOCKING — f-string syntax error (causes `CI / lint` failure)** There is a space inside the brace: `{ i}` should be `{i}`. This is a ruff-flagged f-string syntax error and is almost certainly causing the `CI / lint` failure. ```python # Current (broken): sandbox_names = [f"ca-sandbox-plan{ i}-stub{i}" for i in range(2)] # Fixed: sandbox_names = [f"ca-sandbox-plan{i}-stub{i}" for i in range(2)] ``` --- Automated by CleverAgents Bot Supervisor: PR Review | Agent: pr-review-worker
Owner

Non-blocking suggestion — Dead code: unused mock_tmp variable and redundant patch() context

This with patch(...) as mock_tmp: block patches the tempfile module but mock_tmp is never used inside the block. The comment even acknowledges this: "but we patched gettempdir directly above". The patch() context does nothing meaningful here — the test already works via the tempfile.gettempdir = fake_gettempdir monkey-patch set above the try block.

Suggestion: Remove the with patch(...) as mock_tmp: wrapper and call svc._purge_sandboxes(report) directly. This makes the test cleaner and removes the misleading impression that patching the module is needed.


Automated by CleverAgents Bot
Supervisor: PR Review | Agent: pr-review-worker

**Non-blocking suggestion — Dead code: unused `mock_tmp` variable and redundant `patch()` context** This `with patch(...) as mock_tmp:` block patches the `tempfile` module but `mock_tmp` is never used inside the block. The comment even acknowledges this: *"but we patched gettempdir directly above"*. The `patch()` context does nothing meaningful here — the test already works via the `tempfile.gettempdir = fake_gettempdir` monkey-patch set above the try block. Suggestion: Remove the `with patch(...) as mock_tmp:` wrapper and call `svc._purge_sandboxes(report)` directly. This makes the test cleaner and removes the misleading impression that patching the module is needed. --- Automated by CleverAgents Bot Supervisor: PR Review | Agent: pr-review-worker
Owner

Automated by CleverAgents Bot
Supervisor: PR Review | Agent: pr-review-worker

--- Automated by CleverAgents Bot Supervisor: PR Review | Agent: pr-review-worker
Some checks failed
CI / benchmark-publish (pull_request) Has been skipped
CI / push-validation (pull_request) Successful in 45s
CI / helm (pull_request) Successful in 52s
CI / build (pull_request) Successful in 1m25s
Required
Details
CI / lint (pull_request) Failing after 1m50s
Required
Details
CI / quality (pull_request) Successful in 2m8s
Required
Details
CI / benchmark-regression (pull_request) Failing after 2m15s
CI / typecheck (pull_request) Successful in 2m36s
Required
Details
CI / security (pull_request) Successful in 2m36s
Required
Details
CI / e2e_tests (pull_request) Successful in 5m11s
CI / integration_tests (pull_request) Successful in 5m22s
Required
Details
CI / unit_tests (pull_request) Successful in 8m6s
Required
Details
CI / docker (pull_request) Has been skipped
Required
Details
CI / coverage (pull_request) Has been skipped
Required
Details
CI / status-check (pull_request) Failing after 4s
This pull request has changes conflicting with the target branch.
  • CHANGELOG.md
  • CONTRIBUTORS.md
View command line instructions

Manual merge helper

Use this merge commit message when completing the merge manually.

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin fix/7527-sandbox-cache-invalidation:fix/7527-sandbox-cache-invalidation
git switch fix/7527-sandbox-cache-invalidation
Sign in to join this conversation.
No reviewers
No milestone
No project
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
cleveragents/cleveragents-core!11040
No description provided.