Repository navigation
Conversation
ChristianPavilonis
left a comment
There was a problem hiding this comment.
😃 Approving the cache-seed hardening while keeping deployment smoke jobs independent of cache probes.
🤔 One non-blocking concern remains: compiler write counters can report success despite failed remote finalization. A bounded remote read-back hit and finalization-failure coverage would close that publication gap; details are inline.
dhruv8sh
left a comment
There was a problem hiding this comment.
PR Review
Summary
This PR checks that the cache-probe seed actually publishes its compiler, source and target caches, with bounded retries, while the deploy and release jobs stay independent of it. The change is well-scoped and thoroughly covered by mutation tests. Approving.
😃 Praise
- 😃 The final check doesn't trust
fail-on-cache-missalone: the "Require exact … seed publication" steps demand a literaltruefrom one of the lookup outputs, and all 16 input combinations are exercised (fixture.sh:85-97). This coversactions/cachev5 returning early without applyingfail-on-cache-miss. - Inline: workflow mutation guards (
publication.sh) and confined directory cleanup (seed-compiler-cache.sh).
Findings
Blocking
None.
Non-blocking
- 🤔 A cache-outage run can add up to about 8 minutes: retry waits are 60s then 120s per target (360s for native plus WASM), and the source and target saves can each add another 60s. That's acceptable because only the cache-probe jobs are affected, not the deploy jobs. Just confirming the bound is intended (
seed-compiler-cache.sh:40). - 📝 Release jobs were already independent on
main:fixture-releaseandstore-free-releasealready had noneeds. The new assertion infixture.sh:44-47locks that in as a regression guard. - 📝 A duplicate source-cache save is harmless: when there is no exact source-cache hit (including a partial match through
restore-keys), the action's post-job hook tries to save the same key again and the cache rejects it with a warning only. The workflow comment already explains this. - Inline: ♻️ the source cache paths are copied four times; ⛏ the deploy workflow runs a script from a
tests/directory.
CI Status
- fmt: PASS
- clippy: PASS
- tests: PASS
tests/seed.sh: PASS (fixture.shandpublication.shwere not run locally becauseyqisn't installed; the hosted checks pass)
aram356
left a comment
There was a problem hiding this comment.
PR Review
Summary
This verifies that the cache-probe seed actually publishes its compiler, source and target caches, with bounded retries, while the release and deploy jobs stay independent of it. The compiler read-back in 265076783 is sound. In sccache v0.16.0, src/server.rs:1520-1537 awaits the cache write before it sends the compile response, so stats read right after cargo build are complete. The hosted seed log shows one write and then one Rust hit on the clean rebuild for each target. One question needs an answer before merge: this change leaves no hosted job running the action's save-target: true path.
Findings
Blocking
- ❓ No hosted job runs
save-target: trueany more: the seed was the only job using the action's save path, and the adoption guide still recommends it (.github/workflows/deploy-action.yml:141)
Non-blocking
- 🤔
compiler_hitaccepts a partial hit: it checks hits > 0 but not misses == 0. A dependency added to the fixture would silently skip the read-back, andassert-compiler-reuse.shwould pass without proving anything (seed-compiler-cache.sh:34) - 🤔 Unreadable stats are retried as if the cache service were down: a jq parse failure spends 180s on retries, then reports a publication failure (
seed-compiler-cache.sh:29) - ♻️
fixture.shis now mostly a workflow test: split out the workflow checks so the mutation suite targets only them (publication.sh:14) - ⛏ The read-back build is logged under the same label as the publication build (
seed-compiler-cache.sh:20) - ⛏ The PR description is out of date: it was written before
265076783. It still says "All 28 hosted checks pass on3373552e". The Changes table describesseed-compiler-cache.shwithout the read-back. "Review decisions" presents the write-counter approach as the final compiler strategy. The description becomes the squash commit message, so please update it to describe the read-back and the current check run. - 🌱
cache-restore-seedhas notimeout-minutes: the retries are bounded, but a hungcargo build, sccache request or cache call still runs to GitHub's 6-hour default. Something liketimeout-minutes: 20on the seed job would make "bounded" hold for the whole job. No other job in this workflow sets one, so this is optional.
📌 Out of Scope
- None of the
deploy-action.ymljobs are required status checks onmain. The ruleset lists only the cargo, wasm and docs checks, sostatic-checks(which runs this cache suite) and the cache probe jobs block merges only by convention. That's a repository-settings decision, separate from this PR.
CI Status
- fmt: PASS
- clippy: PASS
- tests: PASS (1550 passed, 0 failed)
setup-rust-build-cache/tests/run.sh(includes 38 workflow mutations and the seed regressions): PASS- ShellCheck, actionlint, action-pin gate,
git diff --check: PASS - Hosted checks on
265076783: all passing
Restore save-target: true on the cache seed so a hosted job still runs the action's post-job save path and target-cache-hit wiring; the explicit save and lookup verification still publish first. Require zero Rust misses for a compiler hit in both the seed and the reuse probe via a shared predicate, fail immediately on unreadable sccache stats, label read-back builds, bound the seed job at 20 minutes, and move workflow contracts from fixture.sh into workflow.sh so mutations target only them. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@aram356 thanks for the review. The two summary-level items are handled in 8e5e852:
Each inline item has a reply in its own thread. |
Summary
fail-on-cache-miss.save-target: truepath under hosted test, and keep the deployment release/smoke jobs independent of cache-probe availability. This extracts the unrelated hardening from Add pre-dispatch hooks and explicit request ingress fidelity #403 without its deploy-job coupling.Changes
setup-rust-build-cache/tests/seed-compiler-cache.shpublish/read-back. Canonicalize the target root, validate the host path component, and reject symlinked target namespaces/debug directories before cleanup.setup-rust-build-cache/tests/compiler-stats.shcompiler_hit(Rust hits > 0, misses == 0, no cache errors) andcompiler_wrotepredicates, used by both the seed andassert-compiler-reuse.sh.setup-rust-build-cache/tests/seed.shsetup-rust-build-cache/tests/workflow.shfixture.sh: seed wiring,save-target: true, lookup output wiring, all 32 hit-output combinations, action paths/keys and retry conditions, independent release jobs.setup-rust-build-cache/tests/{fixture,publication,review,run}.shfixture.shtests only the fixture.publication.shapplies 39 workflow mutations toworkflow.shalone.review.shchecks the writer step'ssave-targetcondition andtarget-cache-hitwiring..github/workflows/deploy-action.ymlsave-target: true, explicitly saves and lookup-verifies source and target seeds first, retries each once and enforces an exact lookup-hit assertion. The seed job hastimeout-minutes: 20. Exhaustion fails only the cache-probe chain.Closes
Closes #433
Test plan
Verified locally on
8e5e8522with Rust 1.98.1:cargo test --workspace --all-targets(1550 passed, 0 failed)cargo clippy --workspace --all-targets --all-features -- -D warningscargo fmt --all -- --checkcargo check --workspace --all-targets --features "fastly cloudflare spin"cargo check -p edgezero-adapter-spin --target wasm32-wasip2 --features spinbash .github/actions/setup-rust-build-cache/tests/run.sh: action, revision, review, fixture, workflow, 39 workflow mutations and seed regressions pass.git diff --check.target-cache-hitwiring or the writer'ssave-targetcondition makes the matching regression fail.8e5e8522, including the cache seed and probe jobs. The seed log shows apublishwrite followed by aread-backRust hit for each target. It also shows the action's post-job save running and logging the expected duplicate-key warning.Worst-case publication backoff is 60s + 120s per compiler target and 60s each for source and target archives (about 8 minutes). It affects only the cache-probe chain, which
timeout-minutes: 20now bounds as a whole.Review decisions
FinalizeCacheEntryUploaderrors. A write can be counted while nothing readable is published. The seed therefore rebuilds from a clean target and requires a Rust hit. sccache awaits cache writes before replying to the compile (src/server.rs), so stats read right aftercargo buildare complete. An existing full hit proves publication without a second build.save-target: true. It is the only hosted job that runs the action'scargo-target-savestep (actions/cache@v5with post-job save) and itstarget-cache-hitwiring. Explicit save and lookup steps still publish and verify first, so consumers never depend on the warning-only post-job hook. That hook then logs a harmless duplicate-key warning, as the source cache already does. The hook's own publication is not separately verified.actions/cachev5 can return early. Restore can succeed withcache-hit=falsebefore parsingfail-on-cache-misswhen feature availability fails. The final shell gate therefore requirestruefrom one of the actual lookup outputs.rm -rf. Cleanup now rejects namespace/debug symlinks and unsafe host components.Local tests simulate publication failures without relying on live outages. Hosted happy-path checks do not prove recovery during an actual cache-service outage.
Checklist
CLAUDE.md; changes limited to cache CI/tooling.