fix(ci): improve caching - #11128
Conversation
848c80e to
5373f8f
Compare
There was a problem hiding this comment.
Pull request overview
Improves CI cache efficiency by limiting writes, consolidating matrix caches, and disabling unnecessary caches.
Changes:
- Restricts cache writes primarily to
main. - Shares caches across crate and lint matrices.
- Documents the caching policy.
The selected cache seed crates are not the widest dependency closures. Test evidence and completed AI disclosure are also missing.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
.github/workflows/tests-unit.yml |
Restricts unit-test cache writes. |
.github/workflows/test-crates.yml |
Consolidates crate caches. |
.github/workflows/lint.yml |
Optimizes lint-job caching. |
.github/workflows/README.md |
Documents cache policy. |
Suppressed comments (2)
.github/workflows/test-crates.yml:197
- This seed is also not the widest eligible MSRV closure.
zebra-utilsdirectly depends onzebra-rpcand, under this job's--all-features --all-targetscommand, adds dependencies such asreqwest, full Tokio, and its utility targets. Selectingzebra-utilspreserves thezebra-rpcdependency artifacts plus those additional artifacts instead of rebuilding them on each MSRV run.
cache-save-if: ${{ github.ref == 'refs/heads/main' && matrix.crate == 'zebra-rpc' }}
.github/workflows/README.md:160
- The documented seed is not the widest dependency closure:
zebraddepends onzebra-rpcandzebra-utils, whilezebra-utilsitself depends onzebra-rpc. Document the actual widest eligible seed for each matrix so future cache changes do not preserve the less effectivezebra-rpcchoice.
2. **Wide matrices share one key.** The `test-crates.yml` matrices use `cache-shared-key` and seed
the shared cache from a single leg (`zebra-rpc`, which has the widest dependency closure), rather
than one ~1 GB near-duplicate cache per crate.
alchemydc
left a comment
alchemydc
left a comment
There was a problem hiding this comment.
Reviewed the mechanism against the pinned action and the vendored rust-cache source rather than just the diff. I found no functional bug — the design is sound and I'm not holding the PR for prose. Four small asks below, then the parts I checked so they don't get re-litigated.
Verified
- All four inputs used here exist at the pinned
46268bd(cache,cache-key,cache-shared-key,cache-save-if); it wrapsSwatinem/rust-cache@c193711(v2.9.1). - rust-cache v2.9.1
src/config.ts:shared-keyreplaceskey+GITHUB_JOB, and matrix values are never part of the key. Socache-key: crate-build-${{ matrix.crate }}really did mint 12 distinct keys, andcache-shared-keyreally does collapse them to one. The diagnosis in #11127 holds. lint.yml,test-crates.ymlandtests-unit.ymlall havepush: branches: [main], and eachchangesgate defaults to'true'on push events — sorefs/heads/mainruns genuinely happen and no job is left permanently cacheless under the newcache-save-if. This was my main worry going in.- Seed legs can actually fire: the matrix is 12 crates reverse-sorted so
zebradis first, andzebra-rpcis present inbuild-msrv's matrix. - The exception list is accurate —
check-no-git-dependenciesandrelease-readinessare both A-release-PR-only,benchmarks.ymlisworkflow_dispatch+C-benchmark.denyreally is 12 legs (4 checks × 3 feature sets).merge_groupexists but native merge queue isn't enabled, so the non-matchinggh-readonly-queue/*ref is harmless. - README TOC lists
##headings only, so the 5 → 6 renumber needs no TOC edit. CI-only change, so noCHANGELOG.mdentry needed.
One under-sold win worth noting in the comment: cache-shared-key: clippy is close to a no-op for the key itself (rust-cache already derived v0-rust-clippy from GITHUB_JOB). The real fix is the save gate — previously both clippy legs raced to write that same key, so the narrower release leg could win and PRs would restore a cache missing the proptest-impl / lightwalletd-grpc-tests dependencies.
Requests
1. The README names the wrong seed crate. Rule 2 says the test-crates.yml matrices seed from "zebra-rpc, which has the widest dependency closure", but build seeds from zebrad and only build-msrv uses zebra-rpc. zebrad is the correct stable seed — so the code is right and the doc is wrong, which means a future maintainer following the doc would change the code to the worse option. (Copilot's review already tripped on this and suggested changing the seeds; please don't — see below.)
Suggested framing: the stable build matrix seeds from zebrad because it has the widest closure; build-msrv can't use zebrad because its build step is if: matrix.crate != 'zebrad', so that leg compiles nothing and would save an empty cache — hence zebra-rpc.
Related nit: the MSRV comment's "zebra-rpc pulls in the rest of the workspace" overstates it — zebra-rpc doesn't pull in zebra-utils. For the record on Copilot's zebra-utils suggestion: neither crate is a strict superset, because -p zebra-utils --all-features activates only zebra-rpc's default features, not all of them. The current choices are fine; only the prose needs a trim.
2. Narrow the policy wording in rule 1. "Every job that builds the workspace sets cache-save-if" isn't true — coverage.yml, book.yml, advisory.yml (5 jobs) and zcashd-compat-regtest.yml all build without it. And I don't think expanding this PR to cover them is the right fix: advisory.yml and zcashd-compat-regtest.yml are schedule + workflow_dispatch, and a manual dispatch can target any branch or tag, so they aren't effectively main-only either. Scoping the stated policy to high-volume workflows that run on pull requests is simpler and stays true.
3. Please drop the Copilot co-author trailer. fe96728's body carries Co-authored-by: Copilot Autofix powered by AI <...>, which AGENTS.md explicitly prohibits for AI tools. This isn't cosmetic: .github/mergify.yml sets merge_method: merge and the repo has allow_squash_merge: false, so the trailer lands in main's history verbatim rather than being collapsed away. Needs an amend + force-push.
4. PR metadata. The Tests section is empty and both AI-disclosure boxes are unchecked (the disclosure text ended up on the "No AI tools were used" line). Given the note that this wasn't thoroughly checked, some before/after cache evidence would carry most of the review weight here — see below.
Post-merge checks
The acceptance criteria in #11127 work as verification for this, with two corrections now that I've read the diff:
- "A representative PR creates no new
v0-rust-*entry under its merge ref" needs to exclude A-release andC-benchmarkPRs, per this PR's own documented exceptions. - "No shared key has multiple writers" now holds for the four keys this PR touches —
clippy,crate-build,crate-build-msrv,unit-tests-1.97.0-default-release-binaries. (lint.yml'smsrvmatrix is a single leg, so there's no residual race there.)
Also: gh api --paginate --jq applies the jq filter per page, so the README snapshot command groups page-by-page. --paginate --slurp | jq ... is the exhaustive form. Not blocking — the live inventory is well under one page.
Explicitly not asking for
Flagging these so they don't resurface: the repeat-push cache-warmth loss on Cargo.lock-changing PRs, fail-fast: false on the crate matrices, documenting the non-seed legs' artifact coverage, hard-coding the newly raised quota number (mutable admin state — the "10 GB default, admins can configure more" wording is more durable), and an extra sentence on PR-cache economics (the README already covers it). All optional at best.
Unrelated follow-up
AGENTS.md says "(PRs are squash-merged — the PR title becomes the commit message)", but squash merge is disabled at the repo level and Mergify uses merge_method: merge. I'll file that separately — it's the reason per-commit trailers matter.
Review assisted by Claude Code, then independently cross-reviewed with OpenAI Codex, which caught the co-author trailer and the workflow_dispatch hole in my original rule-1 analysis.
The raised 25 GB quota is already exhaustedSnapshot taken just now (2026-08-05), after the quota was raised from 10 GB to 25 GB earlier today: $ gh api repos/ZcashFoundation/zebra/actions/cache/usage
{"active_caches_size_in_bytes":26643191326,"active_caches_count":47}26.64 GB against a 25 GB limit — already over, so LRU eviction is running right now. By ref:
Two pull requests own 26.02 of 26.64 GB. The 2.5× headroom increase bought nothing because a single PR can exceed the whole pool on its own. The fix has to be write discipline, not more space. Why we're confident this PR fixes itGrouping the PR-ref entries by key family maps them directly onto this PR's three rules:
Three independent reasons the arithmetic works out:
Together, a typical PR's cache footprint goes from ~20 GB to roughly zero, and I also confirmed the write path actually functions after the change: Reproduce the snapshot with: gh api 'repos/ZcashFoundation/zebra/actions/caches?per_page=100' --paginate --slurp \
| jq -r '[.[].actions_caches[]] | group_by(.ref)
| map({ref: .[0].ref, entries: length, gb: ((map(.size_in_bytes)|add)/1e9*100|round/100)})
| sort_by(-.gb)[] | "\(.ref) n=\(.entries) \(.gb)GB"'(Note Data collected with Claude Code. |
alchemydc
left a comment
alchemydc
left a comment
There was a problem hiding this comment.
Approving. Mechanism verified against the pinned action and rust-cache source — no functional bug found. Supporting cache data in the comment above; the four items in my earlier review are non-blocking, except that I'd still like the Copilot co-author trailer on fe96728 dropped, since merge_method: merge preserves it into main (see #11206).
Merge Queue Status
This pull request spent 27 minutes 17 seconds in the queue, with no time running CI. Waiting for
All conditions
ReasonThe pull request #11128 has been manually updated Requeued — the merge queue status continues in this comment ↓. |
fe96728 to
b1bac5f
Compare
There was a problem hiding this comment.
Re-approving after the force-push — my previous approval was auto-dismissed when history was rewritten.
All three code/doc asks landed:
- Seed crate wording ✅ Rule 2 names
zebradfor the stable matrix andzebra-rpcfor MSRV, and the in-code comment explains why (zebrad's build step is skipped there, so that leg compiles nothing). Doc and code now agree. - Policy wording ✅ No longer claims every workspace-building job sets
cache-save-if, so the workflows that legitimately don't no longer contradict it. - Copilot co-author trailer ✅ Gone — verified all three commit messages via the API.
Nothing regressed: lint.yml and tests-unit.yml are byte-identical to what I reviewed, and test-crates.yml changed only in the MSRV comment.
Minor, not worth a round-trip: naming the deliberate exceptions (check-no-git-dependencies, release-readiness, benchmarks.yml) was useful, and "(There are some minor exceptions to this rule.)" leaves the next auditor to rediscover them.
Merge Queue Status
This pull request spent 1 minute 31 seconds in the queue, including 49 seconds running CI. Waiting for
All conditions
ReasonThe merge conditions cannot be satisfied due to failing checks
Failing checks: HintYou may have to fix your CI before adding the pull request to the queue again. Requeued — the merge queue status continues in this comment ↓. |
b1bac5f to
9fe499b
Compare
Merge Queue Status
This pull request spent 1 minute 5 seconds in the queue, including 11 seconds running CI. Required conditions to merge
|
Motivation
Closes #11127
Solution
See doc update in the diff
Tests
Specifications & References
Follow-up Work
AI Disclosure
PR Checklist
type(scope): description