Skip to content

Fix false-positives with the caches and improve hit-ratio - #4971

Merged
naanselmo merged 8 commits into
mainfrom
nuno/fix-deploy-cache
Sep 28, 2026
Merged

naanselmo merged 8 commits into
mainfrom
nuno/fix-deploy-cache

Conversation

@naanselmo

@naanselmo naanselmo commented Sep 24, 2026 •

Copy link
Copy Markdown
Contributor

Description

Fixes the false-positives in the caches in the deploy job, caused by a cached crate from "the future" being incorrectly used.

Example timeline to show the bug being reproduced:

  1. Commit main-1, builds cache main-1.
  2. Commit main-2, built on top of main-1, pulls the freshest cache, main-1, and builds cache main-2.
  3. Commit hotfix-1, built on top of main-1, pulls the freshest cache... main-2. Things will now go wrong.

For files we changed in a given PR, everything is OK, because git-restore-mtime runs, the files are more recent than the caches themselves, so the caches are invalidated and things are rebuilt from scratch.

The real problem comes from changes that main-2 made: they show up in the main-2 cache. However, when git-restore-mtime runs, the files get the older timestamp. So what does cargo see? Well, a fresh cache, and the supposed source files for it are older. So the cache is considered valid.

This PR iterates on this, to build a solution that 1) keeps cache on for workspace crates and 2) keeps a high hit-rate on the cache but 3) doesn't have false-positives.

It also adds sccache, another tool that improves caching when the main cache hit misses, as it can cache certain build artifacts, but only at the rustc level (so after cargo has already determined it needs to rebuild a crate). This is the least impactful change, and it might end up being removed in the future.

PS: This PR is actually a +64/-63, but the mtime-cache that we use is used in https://github.com/denoland/deno but never made usable as an action. We could reference it to use it but it'd require pulling the entire repo on every execution.

Changes

  1. Nuke git-restore-mtime: it stamps based on the commit time, which is exactly what we don't want due to the "cache from the future" problem;
  2. Add mtime-cache: stamps based on the content hash, so the cache will contain the hashes of the files as they were when the cache was built, meaning it can invalidate partially (by not updating the timestamp) when they are different (warning: it is MIT-licensed but I believe it is OK since we include the license);
  3. Add sccache: should improve cache hit ratio on anything that the previous caching tools miss, accuracy remains to be tested (it outputs metrics so we can validate over time);
  4. Add --locked to cargo arguments, where absent.

How to test

  1. See if CI works in this PR.
  2. Make sure that the logic is sound (great testing step, I know, "see if correct", but this is basically a race condition)

@naanselmo
naanselmo requested review from a team as code owners September 24, 2026 10:10
@claude

claude Bot commented Sep 24, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @naanselmo's task in 1m 58s —— View job


Review: Fix false-positives with the caches

The change flips cache-workspace-crates from true → false on the Swatinem/rust-cache step.

This looks correct. Reasoning:

  • cache-workspace-crates: true tells rust-cache to keep the compiled artifacts of our own workspace crates in the saved cache (by default they're cleaned out before saving). Combined with the constant shared-key: deploy and save-if: main, tag/branch builds restore whatever main last compiled.
  • Unlike external dependencies (hash-locked via Cargo.lock), our own crates aren't keyed on source content, so a restored artifact can be reused when the source has actually changed → the "false positive" stale-binary reuse this PR describes.
  • Setting it to false (the action's default) forces workspace crates to be rebuilt from the current checkout every run, while external crates stay cached (cache-all-crates: true). That's the right correctness/speed tradeoff for a deploy job.

No blocking issues. One thing worth being aware of (already an inherent consequence of the fix, not a request to change): deploy builds will now always recompile all workspace crates, so expect longer build times than before — the dependency cache still helps, but the incremental workspace speedup is gone. That's the correct call for a job whose output ships to production.

The rest of the caching setup (cache-all-crates, constant shared-key, save-if on main, and the tokio-console RUSTFLAGS bucketing) is untouched and consistent with this change.

LGTM ✅ — verifying via CI (as the PR suggests) is the right final check.

  • Read the diff
  • Analyzed cache-workspace-crates semantics
  • Assessed correctness of the cache fix
  • Posted review
    · nuno/fix-deploy-cache

@naanselmo
naanselmo marked this pull request as draft September 26, 2026 10:15
@naanselmo
naanselmo marked this pull request as ready for review September 26, 2026 10:15
@claude

claude Bot commented Sep 26, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @naanselmo's task in 3m 15s —— View job


PR Review: Fix false-positives with the caches

I traced the correctness of the new caching scheme end-to-end and it holds up — this is a solid fix.

Why the mtime approach is correct (the core of the PR): mtime_cache/action.js keys each file on [mode, git-blob-hash, eol, path] and restores the recorded mtime only when the key matches. Because the mtime DB is saved together with the rust-cache artifacts under the same shared-key and only from main (save-if), the DB always reflects the exact source content that produced the cached artifacts:

  • Unchanged file → hash matches DB → old mtime restored → older than the extracted artifact → cargo reuses it. ✅
  • File differing from what main built (the main-2 / hotfix "cache from the future" case) → hash not in DB → mtime set to now → newer than the extracted artifact → cargo rebuilds. ✅ This is exactly the false-positive the PR targets, and it's closed.

Ordering is also right: mtime_cache runs after rust-cache extraction, so changed files get a timestamp newer than the restored artifacts. The DB is pruned each run (only current files are written back), so it stays bounded. Vendoring the action (rather than referencing a third-party action) keeps it auditable, and all external actions are SHA-pinned.

Other changes look good: --locked on the builds is a nice reproducibility tightening, and re-enabling cache-workspace-crates is now safe given the mtime fix.

Two non-blocking suggestions (posted inline):

  • sccache won't cache the debug/--tests builds unless CARGO_INCREMENTAL=0 is set — worth checking against the sccache metrics before keeping it.
  • The ~25-line sccache+cache block is duplicated across ~9 jobs; a local composite action taking shared-key would DRY it and keep the cache-directories ↔ cache-path pairing in sync.

No correctness or blocking issues found. Agree with the PR's own advice that CI is the right final validation.

  • Read the diff
  • Review mtime_cache/action.js logic
  • Review deploy.yaml & pull-request.yaml changes
  • Post inline comments
  • Post summary
    · nuno/fix-deploy-cache

Comment thread .github/workflows/pull-request.yaml Outdated
Comment thread .github/workflows/pull-request.yaml Outdated
@naanselmo naanselmo changed the title Fix false-positives with the caches Fix false-positives with the caches and improve hit-ratio Sep 26, 2026

@AryanGodara AryanGodara left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM 👌🏼 , just a nit

Comment thread .github/actions/mtime_cache/README.md
@naanselmo
naanselmo added this pull request to the merge queue Sep 28, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Sep 28, 2026
@naanselmo
naanselmo added this pull request to the merge queue Sep 28, 2026
Merged via the queue into main with commit 84eed3b Sep 28, 2026
24 of 25 checks passed
@naanselmo
naanselmo deleted the nuno/fix-deploy-cache branch September 28, 2026 11:41
@github-actions github-actions Bot locked and limited conversation to collaborators Sep 28, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants