Skip to content

ops(vera): schedule a checkout-cache prune until the plugin prunes itself - #48

Merged
mabry1985 merged 1 commit into
mainfrom
ops/checkout-cache-prune
Aug 24, 2026
Merged

ops(vera): schedule a checkout-cache prune until the plugin prunes itself#48
mabry1985 merged 1 commit into
mainfrom
ops/checkout-cache-prune

Conversation

@mabry1985

Copy link
Copy Markdown
Member

Stopgap for pr-reviewer-plugin#87, which is still open.

Why

CheckoutCache.prune() is defined, documented in its module docstring, and unit-tested — and nothing in the plugin ever calls it. On this deployment the cache reached 43 GiB across 1248 entries against its own 5 GiB / 50-entry / 1-hour-TTL caps, with 1247 entries already past the TTL. It was the largest single consumer on ava and took the host to 92% disk.

A manual sweep reclaimed 36 GiB. The cache then regrew ~0.5 GiB in 90 minutes — about 8 GiB/day — so this needs a schedule, not a one-off cleanup. Daily at 05:41, through the same wrapper as the other checks so a failure raises a Discord alert instead of a silent log line.

The policy, and why it isn't just the plugin's TTL

  • Keep the 3 newest entries per repo. A pure TTL sweep evicts checkouts the next review immediately re-clones. The cache exists so an unchanged head reaffirms in under a second; buying disk with latency on every repo is a bad trade.
  • Keep anything touched in the last hour, regardless of count. Reviews run 3–10 minutes with several in flight, and deleting a checkout out from under a running panel is the one way this script could cause the failure it exists to prevent. The window is deliberately far longer than the longest observed review.

Both rules are additive — an entry survives if either holds. The script dry-runs by default (--apply to delete) and is scoped to the cache root.

Verified

  • Dry run then apply against the live container: 24 entries / 0.5 GiB removed, 92 kept
  • vera-watchdog.sh prune exercised end-to-end; second run correctly reports 0 deletions
  • Cron installed and confirmed — all five modes now scheduled (health, drift, fallback, oauth, prune)
  • Host: 43 GiB → 2.5 GiB of checkouts, disk 92% → 87% (115 G free)

Delete this when #87 ships

The script, the wrapper mode, the cron entry, and the README row. That instruction is in the commit message, the script's docstring, the wrapper comment, and the cron comment — deliberately, because a stopgap that outlives its bug becomes the next mystery. A cache with a documented, tested, uncalled pruner is indistinguishable from one with no pruner at all, which is why this went unnoticed for two weeks.

🤖 Generated with Claude Code

https://claude.ai/code/session_01FnbQVnHNvTsvbnJ1pdUDdD

…self

pr-reviewer's `CheckoutCache.prune()` is defined, documented in its module docstring,
and unit-tested — and nothing in the plugin calls it (pr-reviewer-plugin#87). The
cache reached 43 GiB across 1248 entries against its own 5 GiB / 50-entry / 1-hour
caps, with 1247 already past the TTL, and took ava to 92% disk.

A manual sweep reclaimed 36 GiB, and the cache regrew ~0.5 GiB in the next 90 minutes
— roughly 8 GiB/day. So this is a schedule, not a cleanup: daily at 05:41, through the
same wrapper as the other checks so a failure is a Discord alert rather than a silent
log line.

The policy is deliberately not just the plugin's TTL:

  * keep the 3 newest entries per repo — a pure TTL sweep evicts checkouts the next
    review immediately re-clones, and the cache exists so an unchanged head reaffirms
    in under a second; buying disk with latency on every repo is a bad trade
  * keep anything touched in the last hour regardless of count — reviews run 3-10
    minutes with several in flight, and deleting a checkout out from under a running
    panel is the one way this could cause the failure it exists to prevent

Both rules are additive, the script dry-runs by default, and it is scoped to delete
only inside the cache root.

DELETE ALL OF THIS when #87 ships: the script, the wrapper mode, the cron entry, and
the README row. A cache with a documented, tested, uncalled pruner is indistinguishable
from a cache with no pruner — which is exactly why it went unnoticed for two weeks and
why this stopgap should not outlive the fix.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FnbQVnHNvTsvbnJ1pdUDdD

@protoreview protoreview Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

QA panel review — WARN

code-review-structural · head 90cd1f79e6ed · formal

Low-to-moderate risk: the PR adds a self-contained prune mode to the watchdog script, but the prune-success path can silently report success when deletions fail, so the cache can keep growing without an alert. Fix scripts/prune_checkout_cache.py:83 first — count freed/deleted after rmtree succeeds (or surface rmtree errors) so a failed prune exits nonzero. The panel did not disagree; verification confirmed all three findings and refuted none. Also carry the testability gap at line 75 and the stale "Four checks" README count. Caveat: the head-SHA file 404'd as a fork ref, so confirmation leaned on the PR diff.

Findings

Severity Location Finding Verified
🟡 minor scripts/prune_checkout_cache.py:83 Deletion failures are silently counted as success: freed and deleted are incremented before shutil.rmtree(p, ignore_errors=True) swallows the errors, and main … confirmed
🟡 minor scripts/prune_checkout_cache.py:75 The prune script's load-bearing eviction rule (keep N newest OR protect-window) is embedded in the inline _SWEEP string run via docker exec python3 -c, so it c… confirmed
nit README.md:238 The watchdog section still opens with 'Four checks' while this PR adds a fifth (prune) mode to the same table, leaving the prose count inconsistent with the ta… confirmed
findings JSON (machine-readable)
[
  {
    "file": "scripts/prune_checkout_cache.py",
    "line": 83,
    "severity": "minor",
    "category": "correctness",
    "claim": "Deletion failures are silently counted as success: freed and deleted are incremented before shutil.rmtree(p, ignore_errors=True) swallows the errors, and main then returns 0, so vera-watchdog.sh prune reports a successful prune (no alert) even when checkouts were not actually removed and the cache stays unbounded.",
    "evidence": "freed += dsize(p)\n            deleted += 1\n            if APPLY:\n                shutil.rmtree(p, ignore_errors=True)",
    "verdict": "confirmed",
    "note": "PR diff (full new-file content) contains these exact lines; counters increment before rmtree, ignore_errors=True swallows failures with no `errors` append, the inner python exits 0, main() returns 0 when docker exec succeeds, and vera-watchdog.sh alerts only on rc 1/*. (Head-SHA file reads 404'd as a fork ref, but the diff is the PR head content.)"
  },
  {
    "file": "scripts/prune_checkout_cache.py",
    "line": 75,
    "severity": "minor",
    "category": "tests",
    "claim": "The prune script's load-bearing eviction rule (keep N newest OR protect-window) is embedded in the inline _SWEEP string run via docker exec python3 -c, so it cannot be unit-tested, and no test is added \u2014 violating the repo's watchdog convention that load-bearing rules be extracted as importable pure functions and covered by tests/test_watchdog_checks.py.",
    "evidence": "keep = {p for _, p in ents[:KEEP]} | {p for m, p in ents if now - m < PROTECT}",
    "verdict": "confirmed",
    "note": "Diff shows this exact rule inside the `_SWEEP` raw string passed to `docker exec ... python3 -c`; no importable function and no test added in the PR. tests/test_watchdog_checks.py documents the convention verbatim: both prior watchdog scripts were flagged on qaEngineer#45 and fixed by extracting pure functions (`fallback_requests`, `evaluate`) covered in that file."
  },
  {
    "file": "README.md",
    "line": 238,
    "severity": "nit",
    "category": "conventions",
    "claim": "The watchdog section still opens with 'Four checks' while this PR adds a fifth (prune) mode to the same table, leaving the prose count inconsistent with the table the PR updated.",
    "evidence": "Four checks, all run from the ava fleet cron through `scripts/vera-watchdog.sh`, which is",
    "verdict": "confirmed",
    "note": "The PR diff leaves the section-opening 'Four checks' sentence untouched while adding a fifth `prune` row to the same table; the current README shows the exact 'Four checks...' text at the section head, so the count is inconsistent after the change."
  }
]

@protoreview protoreview Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Promoting the WARN verdict for head 90cd1f79e6ed: all checks terminal-green, zero unresolved review threads. (approve-on-green)

Open findings carried by this approval — non-blocking, but they did not go away:

  • minor scripts/prune_checkout_cache.py:83 — Deletion failures are silently counted as success: freed and deleted are incremented before shutil.rmtree(p, ignore_errors=True) swallows the errors, and main then returns 0, so vera-watchdog.sh prune
  • minor scripts/prune_checkout_cache.py:75 — The prune script's load-bearing eviction rule (keep N newest OR protect-window) is embedded in the inline _SWEEP string run via docker exec python3 -c, so it cannot be unit-tested, and no test is adde
  • nit README.md:238 — The watchdog section still opens with 'Four checks' while this PR adds a fifth (prune) mode to the same table, leaving the prose count inconsistent with the table the PR updated.

Approving a WARN does not resolve its findings (issue #22).

@mabry1985
mabry1985 merged commit 0f311dd into main Aug 24, 2026
2 checks passed
@mabry1985
mabry1985 deleted the ops/checkout-cache-prune branch August 24, 2026 18:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant