Skip to content

Verify cache seeds without gating deploy smoke jobs - #435

Open
prk-Jr wants to merge 5 commits into
mainfrom
fix/verify-cache-seeds
Open

prk-Jr wants to merge 5 commits into
mainfrom
fix/verify-cache-seeds

Conversation

@prk-Jr

@prk-Jr prk-Jr commented Oct 22, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

  • Detect unpublished compiler, source and target cache seeds at the producer, retry transient cache-service failures within bounded limits, and stop immediately on compilation errors.
  • Require exact source/target-cache lookup hits even when the cache action returns early without applying fail-on-cache-miss.
  • Keep 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

Crate / File Change
setup-rust-build-cache/tests/seed-compiler-cache.sh Verify native/WASM compiler-cache publication, accept confirmed existing hits, and retry cache write failures. Canonicalize the target root, validate the host path component, and reject symlinked target namespaces/debug directories before cleanup.
setup-rust-build-cache/tests/seed.sh Cover transient/permanent native and WASM failures, independent retry limits, existing hits, empty/malformed/error stats, compile failures, and symlink/traversal cleanup boundaries with outside sentinels.
setup-rust-build-cache/tests/{fixture,publication,run}.sh Verify lookup output wiring, all 32 hit-output combinations, action paths/keys and retry conditions; reject 38 workflow mutations; keep deployment jobs independent.
.github/workflows/deploy-action.yml Explicitly save/lookup-verify source and target seeds and retry each once. Retain fail-on-cache-miss diagnostics and enforce an exact lookup-hit assertion. Exhaustion fails only the cache-probe chain.

Closes

Closes #433

Test plan

Verified locally with Rust 1.98.1 after the independent review corrections:

  • cargo test --workspace --all-targets
  • cargo clippy --workspace --all-targets --all-features -- -D warnings
  • cargo fmt --all -- --check
  • cargo check --workspace --all-targets --features "fastly cloudflare spin"
  • cargo check -p edgezero-adapter-spin --target wasm32-wasip2 --features spin
  • bash .github/actions/setup-rust-build-cache/tests/run.sh — action, revision, review, fixture, workflow mutation and seed regressions pass.
  • ShellCheck all cache scripts/tests; actionlint; repository action-pin gate; git diff --check.
  • Independent compiler-cache and workflow investigations, separate fix-strategy proposals, and independent post-fix verification.
  • All 28 hosted checks pass on 3373552e, including cache publication/reuse and deployment smoke jobs, runtime tests and CodeQL.

Compiler seed retry backoff can add up to 180 seconds per target; source and target publication can each add 60 seconds. These waits affect only the cache-probe chain.

Review decisions

The review reproduced three publication/cleanup gaps and fixed all three:

  • actions/cache v5 restore can return success with cache-hit=false before parsing fail-on-cache-miss when feature availability fails. The final shell gate therefore requires true from one of the actual lookup outputs. Tests check both the expressions feeding that gate and its behavior; the assertion is not redundant.
  • Source archives previously saved only in a warning-only post-job hook. On a cold/evicted key, failed publication could leave the seed green while the source-restore consumer failed. The smoke producer now explicitly saves and verifies source archives with bounded retries before consumers start. The public action is unchanged; its later duplicate immutable save on an initial miss is benign.
  • Validating only the cleanup root allowed an intermediate target symlink to redirect rm -rf outside it. Cleanup now rejects namespace/debug symlinks and unsafe host components; tests preserve sentinels outside the confined directory.

The existing compiler retry strategy remains: pinned sccache v0.16.0 waits for cache writes before reporting compilation completion. Immediate stats inspection is valid, and an existing hit proves publication. Polling/restarting the server is unnecessary. Moving each attempt to another target directory would complicate compiler-cache identity and final archive ownership; guarded cleanup preserves the existing layout.

Local mutation tests simulate publication failures without relying on live outages. Hosted happy-path checks do not prove recovery during an actual cache-service outage.

Checklist

  • Follows CLAUDE.md; changes limited to cache CI/tooling.
  • Behavior and failure handling have committed regression coverage.
  • No runtime dependencies, routing, store wiring or application behavior changes.
  • Bash/YAML only; no Python added.
  • No credentials, secrets or bylines added.

@prk-Jr prk-Jr self-assigned this Oct 22, 2026
@aram356 aram356 added this to the 202610 milestone Oct 22, 2026

@ChristianPavilonis ChristianPavilonis left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

😃 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.

Comment on lines +32 to +34
if jq -e '((.stats.cache_hits.counts.Rust // 0) + (.stats.cache_writes // 0)) > 0 and (.stats.cache_errors.counts.Rust // 0) == 0 and (.stats.cache_read_errors // 0) == 0 and (.stats.cache_write_errors // 0) == 0' <<<"$stats" >/dev/null; then
break
fi

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[P2] Verify compiler publication with a remote read-back hit

A successful write counter does not establish publication with the pinned backend. sccache v0.16.0 uses OpenDAL 0.55.0, whose GHAC-v2 writer discards errors from FinalizeCacheEntryUpload. When uploads succeed but finalization returns 503, cache_writes becomes 1 while cache_write_errors remains 0. Reproduced with the real pinned binary and Rust 1.98.1 against a local cache-v2 service: both targets were accepted on attempt 1 and the seed exited 0, but the fresh-target compiler-reuse check exited 1. This bypasses the new retries for unpublished entries and leaves the downstream smoke job failing. Require a bounded remote read-back hit before accepting a new write, and add finalization-failure coverage.

@dhruv8sh dhruv8sh left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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-miss alone: the "Require exact … seed publication" steps demand a literal true from one of the lookup outputs, and all 16 input combinations are exercised (fixture.sh:85-97). This covers actions/cache v5 returning early without applying fail-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-release and store-free-release already had no needs. The new assertion in fixture.sh:44-47 locks 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.sh and publication.sh were not run locally because yq isn't installed; the hosted checks pass)

source "$ACTION_DIR/tests/common.sh"
workflow="$ACTION_DIR/../../workflows/deploy-action.yml"

reject_mutation() {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

😃 Workflow guards are themselves tested by breaking the workflow: making 38 deliberate edits to the workflow and requiring each one to fail fixture.sh with the matching error message proves the guards actually catch the problems, not just that they run.


# Publish each target before downstream jobs test first-build compiler reuse.
fixture="${GITHUB_WORKSPACE:?}/cache-restore-fixture"
target_dir=$(realpath "${CARGO_TARGET_DIR:?}")

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

😃 Directory cleanup is safely confined: the target path is resolved and must be exactly the fixture directory, unsafe host target names are rejected, and symlinked target or debug directories are refused before rm -rf. seed.sh checks that marker files outside the directory survive every symlink and .. attempt.

id: source-save-1
uses: actions/cache/save@v5
with:
path: |

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

♻️ Source cache paths are copied four times: the three-line ~/.cargo/... path list is repeated in all four source save and lookup steps. fixture.sh already compares each copy against action.yml, so drift would be caught; this is about readability only.

Fix (optional): use a YAML anchor (path: &cargo_source_paths | …, then path: *cargo_source_paths), which GitHub Actions now supports.


- name: Seed native and WASM compiler outputs
working-directory: cache-restore-fixture
run: bash .github/actions/setup-rust-build-cache/tests/seed-compiler-cache.sh

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

⛏ The deploy workflow runs a script from a tests/ directory: seed-compiler-cache.sh is real CI logic invoked by the cache-restore-seed job. It sits next to the test scripts, as make-fixture.sh and setup-toolchain.sh already do, so this follows the existing pattern. Optionally add a header comment noting the workflow calls this script and seed.sh covers it.

This branch has not been deployed

No deployments
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.

Verify cache seed publication without coupling deploy smoke jobs

4 participants