Skip to content

Only save rust cache on master - #1824

Open
emmanuelist wants to merge 1 commit into
payjoin:masterfrom
emmanuelist:ci-rust-cache-save-if
Open

Only save rust cache on master#1824
emmanuelist wants to merge 1 commit into
payjoin:masterfrom
emmanuelist:ci-rust-cache-save-if

Conversation

@emmanuelist

Copy link
Copy Markdown
Contributor

Follow-up to the numbers in #1131.

Swatinem/rust-cache saves on every run, so each PR writes its own copy of the build
cache. Those entries are large, and they are what actually fills the quota. Upstream
right now:

entries size
Swatinem/rust-cache 7 9.32 GiB
cache-nix-action 2 0.59 GiB
total 9 9.91 GiB (quota is 10 GB)

So rust-cache is sitting at about 94% of the quota on its own, and 4 of those 7
entries are scoped to PR refs. A PR can only restore from its own ref or from the
default branch, never from another PR, so those 4 are doing nothing for anyone else
while pushing everything else out. That is what makes identical cache keys hit or
miss depending on which PR ran last.

This gates saving on master, so PRs restore but do not write.

I tested it on my fork. The branch without this change accumulated 7 rust-cache
entries from its runs. The branch with it, after a full green run of the same
workflows, wrote 0, and CI came back green:

entries on save-if branch: 0
Formatting -> success
Continuous integration -> success
Flake Check -> success

Two things worth knowing.

@benalleng you already said on #1131 that "for any PR that changes the dep tree it is
ok to not get the full benefit of caching", which is the main trade-off here, so I
went ahead on that basis. Shout if you meant something narrower.

There is a small transition cost. master currently holds 3 rust-cache entries, so
most PRs will still restore something straight away, but the jobs master has not run
recently will be cold until it does. rust.yml runs on push to master so that sorts
itself out on the first merge.

Happy to narrow this to rust.yml alone if you would rather keep the blast radius
small. Those 5 jobs are where the 1.4 to 1.5 GiB entries live, so they are most of
the benefit, and the FFI and release sites are much smaller.

Disclosure: co-authored by Claude Code.

Pull Request Checklist

Please confirm the following before requesting review:

Swatinem/rust-cache saves on every run, so each PR writes its own copy.
Those entries run 0.1 to 1.5 GiB and account for 9.52 GiB of the 10 GB
Actions cache quota, which is what evicts everything else and makes
identical cache keys hit or miss depending on which PR ran last (payjoin#1131).

Gate saving on master so PRs restore but do not write. This also gives
master a cache for PRs to inherit, which it has never had: every entry
in the repo is currently scoped to a pull request ref.

Trade-off is that a PR changing the dependency tree gets a colder build
until master picks the change up.
@coveralls

Copy link
Copy Markdown
Collaborator

Coverage Report for CI Build 32198642068

Coverage remained the same at 86.718%

Details

  • Coverage remained the same as the base build.
  • Patch coverage: No coverable lines changed in this PR.
  • No coverage regressions found.

Uncovered Changes

No uncovered changes found.

Coverage Regressions

No coverage regressions found.


Coverage Stats

Coverage Status
Relevant Lines: 16458
Covered Lines: 14272
Line Coverage: 86.72%
Coverage Strength: 343.73 hits per line

💛 - Coveralls

@benalleng benalleng 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.

Upon giving this some more thought I think that maybe save-if is not the ideal approach at the moment. I think we need to look a little closer at the reason our caches are bloating so much to begin with first and then if it turns out 1.5Gb is unavoidable then we can take a look at this.

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.

3 participants