Repository navigation
Conversation
Separate stable dependency source keys from source-revision target keys, keep build paths stable, and add local isolation guards and real cache/compiler probes. Closes #416
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Approved. The revision-isolated target cache contract, opt-in saves, and migration requirements are clearly documented.
dhruv8sh
left a comment
There was a problem hiding this comment.
PR Review
Summary
This keeps dependency and sccache reuse across commits, but keys the saved target/ archive to the application's exact revision, with an explicit save-target opt-in and a stable CARGO_TARGET_DIR. The design is solid and thoroughly tested, and all 28 checks pass. The findings below are non-blocking.
😃 Praise
- 😃 Retry-safe compiler-reuse checks: the seed job exports its fixture ID, so a "rerun failed jobs" rebuilds the same SHA. The fixture library includes the run ID, so leftover cache from an older run can't produce a false pass.
assert-compiler-reuse.shrequires hits on the very first native and WASM builds, with no build/clean/rebuild shortcut, andfixture.shchecks all of these properties itself. - 😃 Thorough validation in
prepare.sh, with a negative test for each check (see inline).
Findings
Non-blocking
- 🤔 Fixture target saves on main will rarely be restored (
.github/workflows/deploy-action.yml:264,:325, inline) - 🤔 Several apps set up in one job overwrite each other's
CARGO_TARGET_DIR(.github/actions/setup-rust-build-cache/scripts/prepare.sh:160, inline) - ⛏ Missing
yqgives a misleading error (.github/actions/deploy-core/tests/run.sh:1996, inline) - ⛏ The plan docs have every checkbox unticked:
docs/superpowers/plans/2026-10-12-…and2026-10-13-…have 48 open- [ ]items, though the work is done. Tick them or mark the plans complete so they aren't read as pending. - 📝 Both hit outputs read
falseon a repeated setup call: on an identical second call,source-cache-hitandtarget-cache-hitarefalseeven if the first call hit. The guide already says this; callers should read hit status from the first call. - 🌱 The cross-job seed saves a new small target entry on every run (
deploy-action.yml:140): the fixture ID changes per run, so every run (PRs included) saves a new entry. They're tiny now and GitHub's 7-day eviction cleans them up, but worth watching if the fixture grows.
CI Status
- fmt: PASS
- clippy: PASS on GitHub. A local-only
clippy::missing_trait_methodsunfulfilled-expectation error in unchangededgezero-corecode is a toolchain quirk on my machine, not this PR. - tests: PASS (1,520 passed).
setup-rust-build-cache/tests/run.sh: PASS.deploy-core/tests/run.sh: 658 passed, 1 failed. The failure (healthcheck-fastly public surface) is a locale sort-order difference in an action this PR doesn't touch. - GitHub checks: all 28 PASS
| with: | ||
| app-name: store-aware-fixture | ||
| cache-target: true | ||
| save-target: ${{ github.ref == 'refs/heads/main' }} |
There was a problem hiding this comment.
🤔 Fixture target saves on main will rarely be restored
save-target: ${{ github.ref == 'refs/heads/main' }} is keyed to the fixture's commit SHA. That SHA covers .edgezero-source-revision, which holds EdgeZero's own HEAD (make-smoke-fixture.sh:159), so every qualifying push to main gets a new key. PR runs use a merge-commit HEAD, so they never match a saved key either.
The result is two full release target/ archives (fixture-release and store-free-release; the fixture CLI depends on edgezero-cli) saved per main push. Only a rerun of that same main commit would restore them. They count against the repo's 10 GB cache limit and can push out the dependency and sccache entries that actually give cross-commit reuse. It also goes against the PR's own guide (docs/guide/deploy-action-adoption.md:97: "Do not save a full target on every disposable fixture/PR run").
Fix: rely on sccache and dependency caching for these fixtures:
with:
app-name: store-aware-fixture
cache-target: false
working-directory: fixture-appOr keep restore-only and drop save-target, then update the fixture.sh:148-154 assertion that requires the main-only save expression. The same applies at line 325.
| with: | ||
| app-name: store-free-fixture | ||
| cache-target: true | ||
| save-target: ${{ github.ref == 'refs/heads/main' }} |
There was a problem hiding this comment.
🤔 Same as the comment at line 264: this full-release target save will be keyed to a new fixture SHA on each main push and is unlikely ever to be restored.
| printf 'RUSTC_WRAPPER=sccache\n' | ||
| printf 'SCCACHE_GHA_ENABLED=true\n' | ||
| printf 'CARGO_INCREMENTAL=0\n' | ||
| printf 'CARGO_TARGET_DIR=%s\n' "$target_dir" |
There was a problem hiding this comment.
🤔 Several apps set up in one job overwrite each other's CARGO_TARGET_DIR
Every setup writes CARGO_TARGET_DIR and EDGEZERO__RUST_CACHE__TARGET_DIR to GITHUB_ENV, and the last call wins. With apps A and B set up in one job, later builds of A (including build-app-cli) go into B's folder. If B has save-target: true, B's revision-keyed archive is saved with A's artifacts inside it, which defeats the per-revision isolation. On main this only happened with cache-target: true; now it happens in every mode.
The guide's wording "Prefer one setup per application per job" (:109, :142) suggests several apps per job is fine.
Fix: either document "one application per job" as a hard rule, or make prepare.sh refuse a second setup when a different app's selection marker already exists under $RUNNER_TEMP/edgezero-rust-cache/.
| if [[ -z "$script" ]]; then | ||
| # actions/cache declares cache-hit itself, not through a local script. | ||
| # Ref pinning is enforced separately; accept only this documented output. | ||
| if [[ "$out_name" == cache-hit ]] && command -v yq >/dev/null 2>&1; then |
There was a problem hiding this comment.
⛏ Missing yq gives a misleading error
Without yq, a legitimate cache-hit output falls through to "names step '…', which runs no script", which hides the real cause. Suggest failing clearly instead:
command -v yq >/dev/null || fail "yq v4 is required to check $name_of output '$out_name'"| local source_prefix source_key target_key='' | ||
| source_prefix="edgezero-app-$app_name-rust-sources-$runner_os-$runner_arch-" | ||
| source_key="$source_prefix$lock_sha" | ||
| [[ -z "$revision" ]] || target_key="edgezero-app-$app_name-rust-target-v2-$runner_os-$runner_arch-$lock_sha-$revision-exact" |
There was a problem hiding this comment.
😃 Thorough validation
- The
-exactkey suffix stops a 40-character SHA from prefix-matching a 64-character one, and there's a test for it. realpathoutput is checked for control characters while keeping trailing newlines.- It rejects symlinks, refuses to adopt non-empty targets it doesn't own, and keeps the ownership marker outside the cached folder.
- Everything is validated before anything is written to
GITHUB_OUTPUT/GITHUB_ENV.
Each of these has a negative test in revision.sh/review.sh.
Summary
Improve dependency-source and compiler-cache reuse across application commits
while keeping writable Cargo target archives strictly revision-isolated.
Breaking build/cache contract: callers must supply the application's full
SHA when enabling target restores, explicitly opt into target saves, and stop
assuming builds write to
<application>/target. Production/staging deploymentsemantics, credential boundaries, CLI packaging and release verification are
unchanged.
Closes #416.
Review feedback addressed
save-target(default false). Readers useactions/cache/restore; writers useactions/cache's success-only post-jobsave, after compilation. Neither restores targets from another revision.
Fixed Git dates make unchanged fixture revisions stable across reruns; a
tracked upstream SHA accounts for EdgeZero path-dependency changes.
library identity excludes accidental older compiler hits. The next revision
changes only README and requires error-free hits on its first builds with a
fresh target directory, not a same-job build/clean/rebuild.
retrying only failed jobs does not change their expected source SHA. All
three jobs install/select the same
.tool-versionsRust compiler.Cache-disabled Git workspaces can use local HEAD for ownership; unidentified
non-Git repeats still fail. Mixed source/workspace/lock/cache settings, unsafe
paths and unowned targets remain rejected.
pinning; the separate pin gate still applies. Prepare uses
append_output.control-character rejection and unused-output removal address the cleanup nits.
explicit migration, GitHub branch scope and named sccache limitations. CLI
comments distinguish a shared job-local target from remote target caching.
The former lockfile-only target policy is intentionally replaced, as required
by #416. Cargo fingerprinting is not a reason to relax this isolation contract.
This does not guarantee a warm complete application build: linker-invoking
crate types, build-script execution and linking still cost work on cold targets.
Migration
Use a stable app-name and capture the checked-out application's full Git SHA,
not the deployment repository SHA. Configure:
save-target: truerequires target caching enabled. Its default is restore-only.Main-only saves select the workflow ref, not the application branch being built.
Use
CARGO_TARGET_DIRor thecargo-target-diroutput for build paths, includingwhen remote target caching is disabled. Prefer one setup per app per job; an
identical repeat is safe but performs no cache restore. First use of v2 target
keys, and each new source revision, has a cold target cache.
Verification
checks skipped locally on macOS and covered by PR CI.
target directory; first native and WASM builds each hit with zero cache errors.
docs format/lint/build and git diff --check.
first-build compiler-reuse probes on commit ff18102.
observed 204 Rust hits and 17 misses (92.3%), zero cache read/write errors,
and no full target archive save on this PR run. This is a warmed CI fixture,
not a production deployment benchmark or a promised hit ratio.
No cache deletion, live deployment, merge, release, Trusted Server source change
or Rust dependency upgrade is included. Deployer adoption is in
trusted-server-deployer #36.