ship/ci cache banking and janitor#6603
Conversation
The card-data job recompiled engine + draft-core on every run to produce draft-pools.json — 5m37s of cargo for a ~2s JSON transform, because draft-pool-gen cannot take --features cli and so never reuses the units gen-card-data.sh built. Cache the output instead, keyed on the MTGJSON data version (via the shared mtgjson-cache-key action, so the two roll together and /clear-caches mtgjson sweeps both) plus a hash of the sources that define it. Also convert every cache in the job from the combined actions/cache to restore + an explicit save at the point its data is complete. The combined action registers its save as a post step declared post-if: success(), so a job that dies downstream saves nothing. On 2026-07-24 that turned a routine MTGJSON publish into a self-sustaining outage: the key rolled, the run paid ~28m of cold downloads, hit the 30m job timeout mid-R2-upload, saved nothing, and every following run recomputed the same missing key and paid it again (run 30114030288). Raise that ceiling to 50m for the same reason — the budget has to clear the cold path or the cache can never be seeded at all. The pool save is skipped when any set download failed: fetch-draft-sets.sh treats a failed set as non-fatal, and freezing a short pool under a key that only rolls on the next MTGJSON publish would defeat the existing self-heal (sets/ is file-existence-gated, so the next run fetches only what is still missing).
The repo sat at 9.93GB against the hard 10GB quota across 207 entries, with 141 of those (3.4GB) written and never restored even once. The waste is not old or large entries — it is entries whose ref scope is dead. A cache is restorable only from its own ref or the default branch, so caches on vanished gh-readonly-queue branches (58 of 58 never read), on closed PRs, and on one-shot tag refs are unreachable by construction. A dry run over the live inventory reclaims 2.84GB immediately. Deliberately not generational pruning. That was measured first and does not pay: every rust family on main already carries exactly 2 generations, so keep-newest-2 reclaims 0.13GB, and keep-1 would delete the fallback Swatinem's rust-cache restores through restore-keys. Main's caches are healthy (42 of 46 re-read) and this never touches them. Mirrors merge-queue-janitor.yml: ref-absence is a scope test, not a liveness test. Uses pull_request_target for the PR-close trigger because fork-originated pull_request events get a read-only token regardless of the permissions block; safe here since nothing checks out PR code.
MTGJSON publishes roughly daily (5.3.0+20260723 -> +20260724 on consecutive days) while this ran Mondays, so for six days out of seven the committed known-tokens.toml was stale. That is not cosmetic: build.rs embeds that file, so when gen-card-data.sh regenerates it mid-run the deploy has to compile the engine a SECOND time to embed the new catalog — 5m35s on top of the first 5m39s compile, measured on run 30114030288, on a job that was timing out at 30m. Safe by construction: the vintage write-gate means a run against unchanged MTGJSON rewrites nothing and opens no PR.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe pull request adds draft-pool cache-key derivation, separates cache restoration from persistence in staging and WASM workflows, introduces cache cleanup automation, and changes scheduled card-data refreshes from weekly to daily. ChangesCache lifecycle and refresh workflows
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)Cache janitor sweepsequenceDiagram
participant CacheJanitor
participant GitHubRefs
participant GitHubPullRequests
participant GitHubCaches
CacheJanitor->>GitHubCaches: paginate cache inventory
CacheJanitor->>GitHubRefs: inspect merge-queue refs
CacheJanitor->>GitHubPullRequests: inspect pull-request states
CacheJanitor->>GitHubCaches: delete eligible caches or report dry run
GitHubCaches-->>CacheJanitor: return remaining quota usage
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
.github/workflows/cache-janitor.yml (3)
153-156: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winEvery delete failure is reported as "already gone", so a permissions or rate-limit failure looks like a clean sweep.
2>&1discards the response body along with the status, so403,409, and secondary rate-limit responses all print(already gone)and the job exits green having reclaimed nothing. Only a404genuinely means the entry is gone.🔍 Proposed error handling
if [ "$DRY_RUN" != "true" ]; then - gh api -X DELETE "/repos/$REPO/actions/caches/$id" >/dev/null 2>&1 \ - || { echo " (already gone)"; continue; } + # </dev/null so nothing can consume the loop's stdin. + if ! err="$(gh api -X DELETE "/repos/$REPO/actions/caches/$id" </dev/null 2>&1 >/dev/null)"; then + case "$err" in + *"HTTP 404"*) echo " (already gone)" ;; + *) echo "::warning::delete failed for cache $id: $err" ;; + esac + continue + fi fi🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/cache-janitor.yml around lines 153 - 156, Update the cache deletion logic in the DRY_RUN guard to distinguish a genuine 404 from other gh api failures: preserve the “already gone” message only for 404 responses, while exposing and failing on permission, conflict, rate-limit, or other errors instead of suppressing them. Keep successful deletions and dry-run behavior unchanged.
121-144: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPR-close runs sweep more than the closed PR, contradicting the comment at Lines 79-80.
Only the
refs/pull/*arm honorsCLOSED_PR; the queue and tag arms evaluate on every closed PR, and the loop at Lines 106-109 also resolves state for every cache-holding PR on those runs. Either gate the whole sweep or restrict it to the PR arm explicitly and correct the comment.♻️ Proposed scoping fix
case "$ref" in refs/heads/gh-readonly-queue/*) + if [ -n "$CLOSED_PR" ]; then + : # PR-close run: queue and tag scopes are the nightly sweep's job. # The queue branch is gone -> no run can ever check it out, so # no run can ever restore this entry. - if ! grep -qxF "${ref#refs/heads/}" live_queue.txt; then + elif ! grep -qxF "${ref#refs/heads/}" live_queue.txt; then reason="merge-queue ref no longer exists" fi ;; @@ *refs/tags/*) # Tag refs are single-use: the next release is a different ref # and cannot read these. Keep briefly for same-tag re-runs. - if [ "$(date -u -d "$created" +%s)" -lt "$cutoff" ]; then + if [ -z "$CLOSED_PR" ] && [ "$(date -u -d "$created" +%s)" -lt "$cutoff" ]; then reason="tag cache older than ${TAG_CACHE_MAX_AGE_DAYS}d" fi ;;And skip the needless state lookups on PR-close runs:
- for pr in $(cut -f2 caches.tsv | sed -n 's#^refs/pull/\([0-9][0-9]*\)/.*#\1#p' | sort -un); do + for pr in ${CLOSED_PR:-$(cut -f2 caches.tsv | sed -n 's#^refs/pull/\([0-9][0-9]*\)/.*#\1#p' | sort -un)}; do🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/cache-janitor.yml around lines 121 - 144, Scope PR-close cleanup to CLOSED_PR consistently: gate the queue and tag cases so they are skipped when a specific closed PR is provided, and avoid resolving PR state for unrelated cache-holding PRs in the loop around the state lookup. Update the comment near the sweep setup to describe the corrected behavior, while preserving normal full-sweep behavior when CLOSED_PR is unset.
38-60: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSuppress the zizmor
dangerous-triggersfinding inline rather than leaving an error-level result.The rationale for
pull_request_targetis already documented at Lines 46-53 and holds — nothing checks out or executes PR code. Since zizmor reports this at error severity, add an inline ignore so the audit stays green and the exemption is auditable at the trigger itself.Ignore comment support was added in v0.6.0, findings can be ignored inline with# zizmor: ignore[rulename]comments, and the comment must sit within the finding's span and not inside a string or block literal.🧹 Proposed inline suppression
- pull_request_target: + pull_request_target: # zizmor: ignore[dangerous-triggers] # Reclaim a PR's caches as soon as it closes, rather than waiting for the # next nightly sweep or GitHub's 7-day idle eviction.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/cache-janitor.yml around lines 38 - 60, Add an inline zizmor suppression for the dangerous-triggers finding within the pull_request_target trigger span, using the rule name specified by the audit and placing it on a YAML comment line near that trigger. Keep the existing rationale and trigger configuration unchanged, and ensure the suppression remains auditable at the trigger.Source: Linters/SAST tools
.github/workflows/release.yml (1)
100-109: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
binaryencache still uses the combined action.This step still relies on
actions/cache@v4with its implicitpost-if: success()save, the exact pattern this PR is moving away from elsewhere in the same job for the 2026-07-24 incident. The blast radius here is much smaller (a single small binaryen download vs. multi-minute compiles), so this is low priority, but converting it to restore/save for consistency would close the last combined-cache gap in this job.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/release.yml around lines 100 - 109, Convert the binaryen cache flow in the “Cache binaryen” and “Install binaryen” steps from actions/cache@v4 to explicit restore and save actions, preserving the existing path, key, and cache-hit condition. Add a separate save step after installation using the same key, and ensure saving occurs only when the cache was not already restored..github/workflows/deploy.yml (1)
80-93: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftCache restore/save/generate flow is duplicated near-verbatim across two workflows. Both files independently implement ~80 lines of MTGJSON/draft-pools/Scryfall restore, generate, and conditional save logic, and each file's comments explicitly point at the other as the reference copy — the same coupling risk the PR already solved for suffix computation by extracting
mtgjson-cache-key. A future fix applied to only one copy would silently fork behavior again, echoing the exact incident this PR is fixing.
.github/workflows/deploy.yml#L80-L93: extract the restore/save/generate sequence (MTGJSON, draft-pools, Scryfall) into a shared composite action, parameterized by path/key/gating inputs and job-specific steps (download/generate) via callbacks or a fixed contract..github/workflows/release.yml#L133-L139: consume the same composite action instead of maintaining a parallel copy.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/deploy.yml around lines 80 - 93, Extract the duplicated MTGJSON, draft-pools, and Scryfall restore/generate/conditional-save flow from .github/workflows/deploy.yml lines 80-93 into a shared composite action, parameterized for path, cache-key, gating, and job-specific download/generation behavior. Update .github/workflows/release.yml lines 133-139 to consume that action instead of maintaining its parallel implementation; ensure both workflows preserve their existing job-specific behavior and cache conditions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/actions/mtgjson-cache-key/action.yml:
- Around line 73-86: Update the source validation in the cache-key step around
the git ls-files invocation to validate each input path independently, ensuring
crates/draft-core, set_catalog.rs, and set_gating.rs each match at least one
tracked file. Fail with an error if any individual path is missing, rather than
relying on the combined srcs result.
In @.github/workflows/cache-janitor.yml:
- Around line 97-100: Update the live queue reference lookup in the
cache-janitor workflow to fail closed when the gh api pipeline fails: do not
create an empty live_queue.txt or continue to sweeping in that case. Preserve
the valid 200/empty-array result as an empty list, but abort the job before
cache deletion on API or pipeline errors, matching the existing missing-evidence
behavior near the PR lookup.
- Line 162: Update the summary echo using DRY_RUN so “would have deleted”
appears only when DRY_RUN is true; scheduled and PR-close runs with DRY_RUN set
to “false” must report “deleted” instead. Preserve the existing kept, deleted,
and freed values.
---
Nitpick comments:
In @.github/workflows/cache-janitor.yml:
- Around line 153-156: Update the cache deletion logic in the DRY_RUN guard to
distinguish a genuine 404 from other gh api failures: preserve the “already
gone” message only for 404 responses, while exposing and failing on permission,
conflict, rate-limit, or other errors instead of suppressing them. Keep
successful deletions and dry-run behavior unchanged.
- Around line 121-144: Scope PR-close cleanup to CLOSED_PR consistently: gate
the queue and tag cases so they are skipped when a specific closed PR is
provided, and avoid resolving PR state for unrelated cache-holding PRs in the
loop around the state lookup. Update the comment near the sweep setup to
describe the corrected behavior, while preserving normal full-sweep behavior
when CLOSED_PR is unset.
- Around line 38-60: Add an inline zizmor suppression for the dangerous-triggers
finding within the pull_request_target trigger span, using the rule name
specified by the audit and placing it on a YAML comment line near that trigger.
Keep the existing rationale and trigger configuration unchanged, and ensure the
suppression remains auditable at the trigger.
In @.github/workflows/deploy.yml:
- Around line 80-93: Extract the duplicated MTGJSON, draft-pools, and Scryfall
restore/generate/conditional-save flow from .github/workflows/deploy.yml lines
80-93 into a shared composite action, parameterized for path, cache-key, gating,
and job-specific download/generation behavior. Update
.github/workflows/release.yml lines 133-139 to consume that action instead of
maintaining its parallel implementation; ensure both workflows preserve their
existing job-specific behavior and cache conditions.
In @.github/workflows/release.yml:
- Around line 100-109: Convert the binaryen cache flow in the “Cache binaryen”
and “Install binaryen” steps from actions/cache@v4 to explicit restore and save
actions, preserving the existing path, key, and cache-hit condition. Add a
separate save step after installation using the same key, and ensure saving
occurs only when the cache was not already restored.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a26dc575-7664-49e4-8477-1fe0cd4079c4
📒 Files selected for processing (5)
.github/actions/mtgjson-cache-key/action.yml.github/workflows/cache-janitor.yml.github/workflows/deploy.yml.github/workflows/refresh-card-data.yml.github/workflows/release.yml
Addresses review findings on #6603. Three of these are guards that could not fire on the input they exist to catch: * The draft-pool key validated its source paths with a single combined emptiness check. `crates/draft-core` alone matches 13 tracked files, so renaming either single-file engine path left the output non-empty, passed the guard, and silently dropped that file from the hash — after which the key would never roll on its content again. Validate each path independently. * The live merge-queue lookup used `|| : > live_queue.txt`, which cannot distinguish "no queue refs exist" (200 with `[]`) from "the API call failed". A blip left the file empty, marked every queue ref orphaned, and swept the caches of in-flight entries — the exact fail-open the PR arm already refuses. Abort instead, and skip the lookup entirely on PR-close runs so they cannot fail on a scope they do not touch. * Every delete failure printed "(already gone)". Only a 404 means that; a 403 from a missing `actions: write` or a secondary rate-limit would have reported a clean sweep having reclaimed nothing. Classify the error and emit a warning annotation for anything else. Also scope PR-close runs to the PR that closed, which the env comment already claimed but only the `refs/pull/*` arm implemented, and fix the summary line's `${DRY_RUN:+...}` — DRY_RUN is the string "false" on real runs, not empty, so every sweep reported "would have deleted". Verified by dry-running the sweep against the live cache inventory: the scheduled path reclaims 115 entries / 2.84 GB unchanged, and a PR-close run for a closed cache-holding PR touches only that PR (0 queue, 0 tag, 0 other PRs, live-ref lookup skipped).
Review dispositionsFixed (fbfe65e) — three of these were guards that could not fire on the input they exist to catch.
Verified by dry-running the sweep against the live cache inventory. Scheduled path is unchanged at 115 entries / 2.84 GB; a PR-close run for a closed cache-holding PR touches only that PR — 0 queue, 0 tag, 0 other PRs, live-ref lookup skipped. Declined, with reasons: zizmor inline suppression — there is no zizmor, actionlint, or any workflow SAST configured in this repo ( binaryen restore/save conversion — the key is Shared composite action for the cache flow — composite actions have no callback mechanism, so "job-specific steps via callbacks" isn't available. The two jobs interleave genuinely different work between the same caches ( |
1 participant
Summary by CodeRabbit