Repository navigation
Conversation
All Rust jobs shared one rust-cache key, so the fastest job saved it and every slower job restored a cache built for a different target, logged "Cache up-to-date", and rebuilt its dependencies on every run. Use a per-job shared key, restrict saves to main so PR-scoped caches stop evicting main's, and uninstall the runner image's preinstalled stable toolchain so image rollouts no longer rotate the cache key. Integration tests restore the axum job's cache, which builds the same artifacts. Closes #1258 Signed-off-by: dhruv8sh <dhruv8sh@proton.me>
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Review summary
Reviewed b9235ca43d6e7dd1cd484eb5a69daeb3eae671db against 7a0ecb4cbfa39af3d83f7e1ed2733d7eabdc4cb1. No actionable issues introduced by this PR were found.
Inspected all three changed files, every affected job, all four integration-action consumers, and the resolved setup/cache action implementations.
Safety proof
- CI logs show all 12 affected job executions removing
stable, installing pinned Rust 1.95.0, and completing successfully. Coverage includes native Linux/macOS, both WASM targets, Clippy, parity, and integration tests. - Logs from runs
37701852034,37701851968, and37701851955confirm separate OS/job cache namespaces, matching Axum/integration keys, andsave-if: falseforwarded in every affected PR execution. No Rust cache saves occurred.
Validation and remaining uncertainty
- All 22 PR checks pass. The CI merge tree is identical to the reviewed head tree.
- Diff whitespace checks and actionlint without ShellCheck pass. Actionlint with ShellCheck reports 12 unchanged SC2086 diagnostics, confirmed against the base revision. All new removal scripts pass ShellCheck.
- In-memory assertions pass for all setup definitions, key uniqueness, intentional integration sharing, Rust pins, shell syntax, the absent-rustup branch, and CI log observations.
- Existing reviews, inline comments, issue comments, and review threads were empty.
- Main-branch cache saving, subsequent warm restores, and the claimed storage/performance improvements remain unverified on GitHub after merge.
aram356
left a comment
There was a problem hiding this comment.
Summary
Gives each Rust CI job its own rust-cache key, saves Rust caches only from main, and removes the runner image's stable so image updates stop changing the keys. This PR's CI logs confirm the mechanics: cache-save-if reaches rust-cache v2.9.1 as save-if, every job's key now lists only Rust 1.95.0, the four integration jobs compute test-axum's exact key, and nothing saved from the PR ref. One gap remains: the integration action can still save on a workflow_dispatch run from main.
1 of the inline comments below carries a one-click GitHub
suggestion: use Commit suggestion to apply it. The other three describe the change in prose because it spans several files or adds a new one.
Blocking
🔧 wrench
- The save gate is open on
workflow_dispatch: see inline at.github/actions/setup-integration-test-env/action.yml:69
Non-blocking
🌱 seedling / 🤔 thinking / ♻️ refactor
- 🌱 Most of the integration jobs' Rust compile isn't in
cargo-axum: see inline at.github/actions/setup-integration-test-env/action.yml:68 - 🤔 Removing the default toolchain lets rustup reinstall
stablesilently: see inline at.github/workflows/test.yml:40 - ♻️ One shared Rust setup action instead of nine copies: see inline at
.github/workflows/test.yml:36
CI Status
- cargo fmt: PASS (required)
- format-typescript: PASS (required)
- cargo test: PASS (required)
- format-docs: PASS (required)
- CLAUDE.md symlink guard: PASS
- cargo test (axum native): PASS
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- cargo check/build/test (spin native + wasm32-wasip1): PASS
- cargo test (cross-adapter parity): PASS
- cargo test (ts CLI, native) (ubuntu-latest): PASS
- cargo test (ts CLI, native) (macos-latest): PASS
- vitest: PASS
- prepare integration artifacts: PASS
- integration tests: PASS
- integration tests (Fastly EC lifecycle): PASS
- browser integration tests: PASS
- CodeQL: PASS
- Analyze (rust): PASS
- Analyze (actions): PASS
- Analyze (javascript-typescript) (CodeQL Advanced): PASS
- Analyze (javascript-typescript) (CodeQL - Code Quality): PASS
- Analyze (python): PASS
Make the integration cache restore-only so a workflow_dispatch run on main cannot claim the axum key, and unset the rustup default after removing stable so rustup cannot silently reinstall it. Signed-off-by: dhruv8sh <dhruv8sh@proton.me>
…o chore/ci-per-job-rust-cache Signed-off-by: dhruv8sh <dhruv8sh@proton.me>
prk-Jr
left a comment
There was a problem hiding this comment.
Summary
Gives each Rust CI job its own rust-cache key, saves Rust caches only from main, and moves the pinned toolchain setup (including removing the runner image's stable and unsetting the default) into a shared .github/actions/setup-rust composite action. The earlier review's wrench, thinking, and refactor findings are all resolved in bcec6ad, and the seedling is reasonably deferred as a follow-up.
Checked beyond the earlier review:
- Save gate still fires where it should:
test.ymlandformat.ymlboth run onpush: [main];integration-tests.ymlis PR +workflow_dispatchonly and is now restore-only viasave-cache: "false". rustup default noneis safe for every current Rust call:cargo install viceroy(both copies) andrustup target add wasm32-unknown-unknownrun from the checkout, where the directory override applies; the onlyworking-directorychanges are JS/docs/browser steps.- The CLI matrix keeps distinct keys (
cargo-cli-Linux/cargo-cli-macOS) becauserunner.osis appended in the shared action.
Non-blocking
📝 note
- Old caches linger until idle eviction: after merge, the old shared
v0-rust-cargo-Linux-…entry and the ~8.5 GB of existing PR-ref caches stay until GitHub's 7-day idle eviction. An optional one-offgh cache deleteafter merge would free space sooner. No code change needed.
👍 praise
- The shared setup action keeps the toolchain pin, the stable removal, and the main-only save gate in one place, so a new Rust job can't silently skip any of them. The before/after
actbenchmark in the description is a nice touch.
CI Status
All 22 checks pass (cargo fmt, cargo test, axum, cloudflare, spin, parity, ts CLI on ubuntu and macOS, vitest, format-typescript, format-docs, integration, browser integration, Fastly EC lifecycle, CodeQL/Analyze, CLAUDE.md symlink guard).
Brings #1225 at c3a33e6. Integration decisions: - .gitignore, AGENTS.md and scripts/README.md keep both sides: #1254's lint-domains gate stays CI gate 10 and #1225's documentation proposal scripts become gate 11; the scripts table keeps #1257's check-ci-node-pin.py row beside #1225's docs-proposal rows. - The lint-domains job from #1254 now uses #1259's setup-rust action and pins Node before cargo, so #1257's check-ci-node-pin gate passes. The two PRs merged cleanly but the combination failed that gate.
aram356
left a comment
There was a problem hiding this comment.
Summary
The round-1 findings are resolved in bcec6ad77. The shared .github/actions/setup-rust action holds the pin, the stable removal with rustup default none, and the main-only save gate, and the integration action now restores cargo-axum read-only. This head's CI logs confirm it: all 12 Rust jobs keep their round-1 keys, list only Rust 1.95.0, log default toolchain unset, pass save-if: false to rust-cache, and save nothing. One gap remains: nothing stops a workflow from setting up Rust without the shared action, and three open PR lines already do.
1 of the inline comments below carries a one-click GitHub
suggestion: use Commit suggestion to apply it.
Blocking
🔧 wrench
- Nothing stops a job from bypassing
setup-rust: see inline at.github/workflows/format.yml:48
Cross-cutting / body-level findings
📝 note: The PR description no longer matches the diff
The Changes table has no row for the new .github/actions/setup-rust/action.yml, still lists the uninstall step under test.yml, and says the integration action gets the "same save and toolchain changes", though it's now restore-only (save-cache: "false"). The summary doesn't mention rustup default none either. Squash merges here use the commit messages, so main's history is fine, but reviewers and the #1258 trail read the description. Suggested rows:
| File | Change |
|---|---|
.github/actions/setup-rust/action.yml |
New shared setup: reads the pin from .tool-versions, uninstalls the image's stable and unsets the rustup default, keys the cache as <cache-key>-<os>, and saves only when save-cache is "true" on main |
.github/workflows/test.yml |
All six Rust jobs use the shared action with per-job keys (cargo-fastly, cargo-axum, cargo-cloudflare, cargo-spin, cargo-parity, cargo-cli) |
.github/workflows/format.yml |
The clippy job uses it with cargo-clippy |
.github/actions/setup-integration-test-env/action.yml |
Restores cargo-axum read-only (save-cache: "false") |
📌 out of scope: File the deferred integration follow-up
The reply on the 🌱 thread defers dropping --target from the two integration cargo test steps and switching them to cargo-parity, but no issue exists yet. Please file it and link it here so the deferral is tracked.
CI Status
All checks ran on this head on 2026-10-12. The branch is 10 commits behind main (including #1226's new lint-wasm-clock.sh step in the clippy job) and merges cleanly; that step runs cargo from the repository root, so the rustup default none change doesn't affect it.
- cargo fmt: PASS (required)
- format-typescript: PASS (required)
- cargo test: PASS (required)
- format-docs: PASS (required)
- CLAUDE.md symlink guard: PASS
- cargo test (axum native): PASS
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- cargo check/build/test (spin native + wasm32-wasip1): PASS
- cargo test (cross-adapter parity): PASS
- cargo test (ts CLI, native) (ubuntu-latest): PASS
- cargo test (ts CLI, native) (macos-latest): PASS
- vitest: PASS
- prepare integration artifacts: PASS
- integration tests: PASS
- integration tests (Fastly EC lifecycle): PASS
- browser integration tests: PASS
- CodeQL: PASS
- Analyze (rust): PASS
- Analyze (actions): PASS
- Analyze (javascript-typescript) (CodeQL Advanced): PASS
- Analyze (javascript-typescript) (CodeQL - Code Quality): PASS
- Analyze (python): PASS
| - uses: actions/checkout@v4 | ||
|
|
||
| - name: Retrieve Rust version | ||
| id: rust-version | ||
| run: echo "rust-version=$(grep '^rust ' .tool-versions | awk '{print $2}')" >> $GITHUB_OUTPUT | ||
| shell: bash | ||
|
|
||
| - name: Set up rust toolchain |
There was a problem hiding this comment.
🔧 wrench: Nothing stops a job from bypassing setup-rust
The shared action fixes this PR's jobs, but a workflow can still call actions-rust-lang/setup-rust-toolchain or Swatinem/rust-cache directly, and three open PR lines already do:
- Add opt-in ordered HTML processor EOF completion #1290 adds a job with
cache-shared-key: cargo-${{ runner.os }}(the old shared key) and the default save gate. - The inspector stack (Add the permission model with the Privacy Taxonomy vocabulary #1045, Open the integration seam so a vendor module can live outside core #1094, Add one module context for every kind of module #1293 through Move the loading cache module to the released caching crate #1319) adds
inspector.ymlwith the default cache settings: a job-based key, saved on every PR. - Gate PRs on non-allowlisted URL hosts in changed lines #1254 restores
cargo-cliread-only but keeps the image'sstable, so its key never matches test-cli's.
The first two save PR-scoped caches and keep the image's stable in their keys, so whichever merges after this PR brings back the eviction pressure #1258 describes, and nothing fails. A grep in this required job turns the bypass into a CI failure:
| - uses: actions/checkout@v4 | |
| - name: Retrieve Rust version | |
| id: rust-version | |
| run: echo "rust-version=$(grep '^rust ' .tool-versions | awk '{print $2}')" >> $GITHUB_OUTPUT | |
| shell: bash | |
| - name: Set up rust toolchain | |
| - uses: actions/checkout@v4 | |
| # Rust jobs must set up the toolchain through ./.github/actions/setup-rust, | |
| # which removes the image's stable and only saves caches from main. A | |
| # direct setup-rust-toolchain or rust-cache step skips both silently. | |
| - name: Require the shared Rust setup action | |
| shell: bash | |
| run: | | |
| if grep -rnE 'uses:[[:space:]]*(actions-rust-lang/setup-rust-toolchain|Swatinem/rust-cache)@' .github \ | |
| | grep -v '^\.github/actions/setup-rust/action\.yml:'; then | |
| echo "::error::Set up Rust with ./.github/actions/setup-rust, not setup-rust-toolchain or rust-cache directly." | |
| exit 1 | |
| fi | |
| - name: Set up rust toolchain |
Checked in a scratch copy: actionlint (with shellcheck) passes. Under the runner's bash -eo pipefail with GNU grep, the step exits 0 on this PR's tree and on its merge with main, and ignores a comment that only mentions the action. It exits 1 and names the line for #1319's inspector.yml, a #1290-style job, and a bare Swatinem/rust-cache step. Those open PRs would each need a short switch to ./.github/actions/setup-rust after this lands.
Summary
cargo-${{ runner.os }}, and a job with an exact key match never saves. The first job to save (cloudflare or parity) owns the cache, and the axum, fastly, spin and clippy jobs rebuild 150–1,000 dependency crates on every run even though they report a full hit.main. PR-scoped caches can't be reused by other PRs and were pushing the repo past its 10 GB limit (12.6 GB, ~10.8 GB of it from PRs), evicting main's caches.stabletoolchain before setup. rust-cache hashes every installed toolchain into its key, so each runner-image update was changing the key.Changes
.github/workflows/test.ymlcargo-fastly,cargo-axum,cargo-cloudflare,cargo-spin,cargo-parity;cargo-cliunchanged),cache-save-ifonmainonly, and a step that uninstalls the image'sstabletoolchain.github/workflows/format.ymlcargo-clippy).github/actions/setup-integration-test-env/action.ymlcargo-axum, which builds the same axum debug and Fastly release WASM artifacts. Same save and toolchain changes.Closes
Closes #1258
Test plan
cargo test-fastly && cargo test-axumcargo clippy-fastly && cargo clippy-axumcargo fmt --all -- --checkcd crates/trusted-server-js/lib && npx vitest run(ran withNODE_OPTIONS=--no-experimental-webstoragebecause local Node is 26, not the pinned 24.12.0)cd crates/trusted-server-js/lib && npm run formatcd docs && npm run formatcargo build --package trusted-server-adapter-fastly --release --target wasm32-wasip1fastly compute serveactbenchmark ofmain'stest.ymlvs this branch, running the cloudflare, fastly and axum jobs sequentially (one cold main-push run, then two warm PR runs per variant)cargo testWhat the benchmark confirmed:
mainruns save one cache per job.stabletoolchain.Not covered locally:
After merge, check these:
gh cache listshows Rust caches only forrefs/heads/main, under 10 GB.Compilinglines.The first PR runs after merge build cold until
mainhas saved the new keys.Checklist
unwrap()in production code — useexpect("should ...")tracingmacros (notprintln!)actbenchmark above.)