BUILD-11220 Skip redundant cache save when content unchanged by mikolaj-matuszny-ext-sonarsource · Pull Request #86 · SonarSource/gh-action_cache

mikolaj-matuszny-ext-sonarsource · 2026-06-18T11:30:35Z

BUILD-11220 — Skip redundant cache save when content unchanged

Problem

When a feature branch restores the default-branch fallback cache and the cached content is never modified, the action still uploads a full duplicate to S3 under the branch-scoped key. Same content hash, different branch prefix:

restored: refs/heads/master/gradle-f51e0ba06f511cb5d0f8558fb2506ea9
saved:    lj/checking-.../gradle-f51e0ba06f511cb5d0f8558fb2506ea9   <- pure duplicate

The combined runs-on/cache action only skips saving on a full exact-key match; our branch-prefixed keys never match the fallback key, so it always saves. This is a missing capability upstream, not a bug.

Fix

Split the combined runs-on/cache step into restore-only + a new internal cache-save node sub-action whose post step performs the save — but skips it when the restored content is provably identical to the default-branch cache.

Skip rule: skip when the S3 restore's cache-matched-key equals the fallback-exact-key computed by prepare-keys.sh (refs/heads/<fallback>/<key>). That equality proves the branch save would be a byte-identical duplicate.

Scenario	Outcome
Feature branch, fallback exact restored, no change	SKIP (the bug)
Feature branch, content changed (S3 miss)	SAVE
Prefix restore-key partial hit (different hash)	SAVE
Default branch run (`fallback-exact-key` empty)	SAVE (source caches always saved)
Migration import (GitHub→S3)	SAVE (imported content persisted)
`lookup-only`	no save attempted

Opt out with skip-redundant-save: false (default true).

Why a vendored bundle (design note)

The S3 upload is a runs-on/cache patch of @actions/cache — stock @actions/cache writes to GitHub, not S3 — so the save cannot be reimplemented. We also can't use uses: runs-on/cache/save (it runs inline, too early for caches populated mid-job) and a composite can't attach a post: hook to an inner uses:. So cache-save's post step forks the prebuilt dist/save-only bundle vendored from the exact runs-on/cache SHA already pinned in action.yml (88d9064, v5.0.7). Same bytes GitHub already runs — same trust boundary, now SHA256-pinned in-repo (cache-save/vendor/runs-on-save-only/PROVENANCE.md). Refresh via scripts/refresh-runs-on-save.sh <sha> whenever the pin bumps.

LIFO post-ordering

cache-save is placed before credential-guard so that, under reverse-order post execution, credential-guard's post re-exports AWS creds to GITHUB_ENV before cache-save's post performs the upload.

Tests

62 unit tests (vitest), all green. Coverage: decision (all branches), prepare-keys output (feature + default branch), fork wrapper (resolve / exit-nonzero / signal-kill / path resolution), cache-save main (record + error), cache-save post (skip / save / disabled / lookup-only / empty-key). cache-save/dist bundles match a fresh npm run build (reproducible).

Reviewer testing checklist (e2e — not yet run on a runner)

The highest-risk integration point is mocked in unit tests, so please verify on real runners:

Scenario (a): feature branch that restores default-branch fallback unchanged → log shows Cache content is identical to the default-branch cache … skipping redundant save, and no S3 object is created for the branch key.
Scenario (b): change a cached input → new branch-scoped S3 object created.
Default branch run: cache saved normally.
Migration import: GitHub→S3 imported content is persisted to S3.
AWS creds reach the forked save in the post phase (credential-guard post runs first via LIFO).
Windows runner: fork + path.join bundle resolution + cred propagation work.
Opt-out: skip-redundant-save: false → duplicate save still occurs.

Notes

Known limitation: upload-chunk-size is not threaded onto the S3 save path (no SonarSource repo sets it). Documented in README.
Pre-existing (not from this PR): cache-metrics/dist on master differs from a fresh ncc build (ncc nondeterminism). Left untouched.

Refs: BUILD-11220

sonarqubecloud · 2026-06-18T11:30:45Z

Agentic Analysis: Early Results

Agentic Analysis and Context Augmentation are available on your project. Here are some issues that could have been prevented. Follow the links to learn how to put them into action.

14 issue(s) found across 6 file(s):

Rule	File	Line	Message
`typescript:S3498`	`__tests__/content-manifest.test.ts`	29	Expected method shorthand.
`typescript:S7772`	`__tests__/prepare-keys.test.ts`	2	Prefer `node:child_process` over `child_process`.
`typescript:S7772`	`__tests__/prepare-keys.test.ts`	3	Prefer `node:fs` over `fs`.
`typescript:S7772`	`__tests__/prepare-keys.test.ts`	4	Prefer `node:os` over `os`.
`typescript:S7772`	`__tests__/prepare-keys.test.ts`	5	Prefer `node:path` over `path`.
`typescript:S4036`	`__tests__/prepare-keys.test.ts`	15	Make sure the "PATH" variable only contains fixed, unwriteable directories.
`typescript:S7785`	`src/cache-save-main.ts`	37	Prefer top-level await over an async function `run` call.
`typescript:S7785`	`src/cache-save-post.ts`	62	Prefer top-level await over an async function `run` call.
`typescript:S7772`	`src/content-manifest.ts`	1	Prefer `node:crypto` over `crypto`.
`typescript:S7772`	`src/content-manifest.ts`	2	Prefer `node:fs/promises` over `fs/promises`.
`typescript:S7772`	`src/content-manifest.ts`	3	Prefer `node:path` over `path`.
`typescript:S2871`	`src/content-manifest.ts`	64	Provide a compare function that depends on "String.localeCompare", to reliably sort elements alphabetically.
`typescript:S7772`	`src/runs-on-save.ts`	1	Prefer `node:child_process` over `child_process`.
`typescript:S7772`	`src/runs-on-save.ts`	2	Prefer `node:path` over `path`.

_{Analyzed by SonarQube Agentic Analysis in 6.3 s}

…dles from pre-commit

…laceAll, safe shell glob

mikolaj-matuszny-ext-sonarsource · 2026-06-18T12:01:05Z

Pushed fixes for the CI failures and review feedback:

CI blockers (fixed)

Missing cache-save@v1: the new sub-action doesn't exist on the v1 tag yet, so action.yml now references it from this branch (@feat/mmatuszny/BUILD-11220-...), with a TODO(BUILD-11220) to revert to @v1 once merged + tagged. This is what was cascading into all the uses: ./ integration-test failures.
pre-commit: the trailing-whitespace / end-of-file-fixer / check-added-large-files hooks were reformatting the committed ncc bundle and the 4.2 MB vendored save bundle (their exclude lists enumerate each sub-action's dist/ but cache-save/ was never added). Added cache-save/dist/ and cache-save/vendor/ to all three excludes — mirrors the existing pattern for the other sub-actions. Also fixed two markdownlint nits in PROVENANCE.md.

SonarCloud (safe findings fixed)

Dropped any types in runs-on-save.test.ts (proper handler/tuple types), removed the useless args binding, switched to replaceAll.
refresh-runs-on-save.sh: shasum … ./*.js (glob ./ prefix).
Left intentionally: the node:-prefix (S7772) and top-level-await (S7785) rules — the entire existing codebase uses the opposite convention (import * as fs from 'fs/promises', the if (!process.env.VITEST) run() guard), so changing only the new files would make them inconsistent with every sibling. Happy to revisit repo-wide separately.
S4036 (PATH hotspot in prepare-keys.test.ts): reviewed-safe, will mark as such in the UI. I initially pinned PATH to fixed dirs but reverted it — that breaks Windows (separator ;, git-bash not on a hardcoded /usr/bin list) and the unit suite is local-only (not run in CI). Kept the portable process.env inheritance with a comment.

Design observations (gitar-bot) — not code-changed, flagging for discussion

Skip rule uses restore-time key equality, not content diffing: correct, and it's the same assumption actions/cache/runs-on/cache already make (the cache key is the content identity). The skip only fires when the S3 restore matched the default-branch exact key — i.e. the branch had no own cache and the key was unchanged. If a job mutates the cached dir under a stable key, that drift wasn't being captured under the branch key today either (the branch key only gets written on a key change). I think the behavior is sound, but it's the key thing to confirm in e2e — happy to add a README note making the key-equality basis explicit.
Save failures are warnings, not job failures: intentional — matches @actions/cache semantics (a failed cache save must not fail the user's build). Worth confirming the forked-save path surfaces clearly in logs during e2e.

The branch is ready for your testing. Note the cache-save ref must flip back to @v1 before merge (tracked by the TODO).

gitar-bot · 2026-06-18T15:07:25Z

+    // Relativise each matched file against the longest search-path base that contains it, so the
+    // manifest records a stable logical sub-path (machine-independent, identical in main and post).
+    const searchPaths = globber.getSearchPaths();
+
+    const entries: string[] = [];
+    for await (const file of globber.globGenerator()) {
+      const stat = await fs.lstat(file);
+      if (stat.isDirectory()) {
+        continue;
+      }
+      const base = searchPaths.find((p) => file === p || file.startsWith(p + path.sep)) ?? path.dirname(file);
+      const rel = path.relative(base, file).split(path.sep).join('/');


💡 Quality: Comment claims 'longest base' but code uses first-match find()

In computeContentDigest, the comment at src/content-manifest.ts:43-44 says each file is relativised "against the longest search-path base that contains it," but the code at line 53 uses searchPaths.find(...), which returns the first matching base in array order, not the longest. In practice this is harmless: @actions/glob reduces overlapping patterns to their common-ancestor root, so a file is contained in at most one search path, and the relativisation is deterministic across the main and post invocations on the same runner (which is all the digest comparison requires). The comment is simply inaccurate and could mislead future maintainers into assuming longest-prefix semantics that aren't implemented. Recommend rewording the comment (e.g. "against the search-path base that contains it") or, if longest-prefix is actually intended, replacing find with a reduce that selects the longest matching p.

Reword the comment to match the actual first-match behavior.:

// Relativise each matched file against the search-path base that contains it (@actions/glob // reduces overlapping patterns to a single common-ancestor root, so at most one base matches), // so the manifest records a stable logical sub-path identical in main and post.

Apply fix

_{Check the box to apply the fix or reply for a change | Was this helpful? React with 👍 / 👎}

…run)

sonarqubecloud · 2026-06-18T15:18:27Z

Quality Gate failed

Failed conditions
12 New issues
1 Security Hotspot

🛠️ Remediation Agent ready

Fix automatically
Creates a separate PR with fixes for eligible issues

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

gitar-bot · 2026-06-18T15:18:32Z

Code Review 👍 Approved with suggestions 2 resolved / 3 findings

Implements a redundant cache save bypass by splitting the action into restore and post-execution save steps. Update the comment in computeContentDigest to accurately reflect the use of find() logic instead of the claimed longest-base match.

💡 Quality: Comment claims 'longest base' but code uses first-match find()

📄 src/content-manifest.ts:43-54

In computeContentDigest, the comment at src/content-manifest.ts:43-44 says each file is relativised "against the longest search-path base that contains it," but the code at line 53 uses searchPaths.find(...), which returns the first matching base in array order, not the longest. In practice this is harmless: @actions/glob reduces overlapping patterns to their common-ancestor root, so a file is contained in at most one search path, and the relativisation is deterministic across the main and post invocations on the same runner (which is all the digest comparison requires). The comment is simply inaccurate and could mislead future maintainers into assuming longest-prefix semantics that aren't implemented. Recommend rewording the comment (e.g. "against the search-path base that contains it") or, if longest-prefix is actually intended, replacing find with a reduce that selects the longest matching p.

Reword the comment to match the actual first-match behavior.

// Relativise each matched file against the search-path base that contains it (@actions/glob
// reduces overlapping patterns to a single common-ancestor root, so at most one base matches),
// so the manifest records a stable logical sub-path identical in main and post.

✅ 2 resolved

✅ Edge Case: Skip rule infers "content unchanged" from restore-time key equality

📄 src/cache-save-decision.ts:24-26 📄 README.md:298-303
The skip decision treats a restore-time key match as proof that the branch save would be a "byte-identical duplicate":
if (input.fallbackExactKey && input.matchedKey === input.fallbackExactKey) {
  return { skip: true, reason: 'restored-from-default-branch-fallback' };
}
This only proves that the cache key (typically a hash of lockfiles / build files, not of the cached directory contents) matched the default-branch fallback at restore time. It does NOT prove the cached path is unchanged at save time. Common caches mutate under a stable key — e.g. ~/.gradle, ~/.m2, or node_modules can gain transitive artifacts during a job even though the lockfile-derived key is unchanged.

Consequence: when a feature branch restores the default-branch fallback and then enriches the cached path without changing the key inputs, the branch-scoped save is silently skipped (default skip-redundant-save: true). The branch never builds its own enriched cache, so every subsequent run on that branch restores the stale master fallback and re-does the work. Before this PR, runs-on/cache's automatic post-save would have captured those changes under the branch key.

The PR's design table equates "content changed" with "S3 cache-key miss," but content can change without a key miss — that is the gap. This is a behavioral change shipped on-by-default. Recommend: (a) clearly document this limitation in the README (the skip is based on key equality, not actual content diffing, and may drop legitimate content enrichments), and/or (b) reconsider defaulting to true for cache types whose contents drift under a stable key, and (c) verify the e2e scenarios on real runners (already flagged as not yet run).

✅ Quality: Cache save failures are swallowed as warnings only

📄 src/cache-save-post.ts:35-37
In cache-save-post.ts, any error from the forked save (including the bundle exiting non-zero, e.g. missing AWS creds or bucket env in the post phase) is caught and reported only via core.warning:
} catch (error) {
  core.warning(`Cache save failed: ${error instanceof Error ? error.message : error}`);
}
This matches @actions/cache's non-fatal save behavior, so it is reasonable, but it means a genuinely broken S3 save path (the main integration risk called out in the PR — AWS creds / RUNS_ON_S3_BUCKET_CACHE not reaching the post step) will silently produce no cache object and only a log warning, with no signal on the job status. Since the highest-risk integration point is mocked in unit tests, consider surfacing save failures more prominently (e.g. a distinct annotation) so a misconfigured fork is noticeable in CI rather than silently degrading cache hit rates.

🤖 Prompt for agents

Code Review: Implements a redundant cache save bypass by splitting the action into restore and post-execution save steps. Update the comment in `computeContentDigest` to accurately reflect the use of `find()` logic instead of the claimed longest-base match.

1. 💡 Quality: Comment claims 'longest base' but code uses first-match find()
   Files: src/content-manifest.ts:43-54

   In `computeContentDigest`, the comment at src/content-manifest.ts:43-44 says each file is relativised "against the longest search-path base that contains it," but the code at line 53 uses `searchPaths.find(...)`, which returns the *first* matching base in array order, not the longest. In practice this is harmless: `@actions/glob` reduces overlapping patterns to their common-ancestor root, so a file is contained in at most one search path, and the relativisation is deterministic across the main and post invocations on the same runner (which is all the digest comparison requires). The comment is simply inaccurate and could mislead future maintainers into assuming longest-prefix semantics that aren't implemented. Recommend rewording the comment (e.g. "against the search-path base that contains it") or, if longest-prefix is actually intended, replacing `find` with a reduce that selects the longest matching `p`.

   Fix (Reword the comment to match the actual first-match behavior.):
   // Relativise each matched file against the search-path base that contains it (@actions/glob
   // reduces overlapping patterns to a single common-ancestor root, so at most one base matches),
   // so the manifest records a stable logical sub-path identical in main and post.

Options

Auto-apply is off → Gitar will not commit updates to this branch.
Display: compact → Showing less information.

Comment with these commands to change:

`Auto-apply`	`Compact`
`gitar auto-apply:on`	`gitar display:verbose`

_{Was this helpful? React with 👍 / 👎 | Gitar}

sonarqube-cloud-us · 2026-06-18T15:18:34Z

Quality Gate failed

Failed conditions
11 New issues
1 Security Hotspot

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

mikolaj-matuszny-ext-sonarsource added 11 commits June 18, 2026 11:59

BUILD-11220 emit fallback-exact-key output from prepare-keys.sh

7b1d19a

BUILD-11220 add pure shouldSkipSave decision function

93ec826

BUILD-11220 vendor runs-on save-only bundle and add fork wrapper

54e8624

BUILD-11220 harden runs-on-save fork: settle once, report signal

84ee04d

BUILD-11220 add cache-save main step recording save state

d452b13

BUILD-11220 add cache-save post step with skip-redundant-save gate

8b37910

BUILD-11220 cover cache-save error, lookup-only, and empty-key paths

6202e69

BUILD-11220 add cache-save sub-action manifest and dist bundles

9cb254b

BUILD-11220 rewire action to restore + conditional save

178c3bc

BUILD-11220 expose skip-redundant-save as top-level input

728594c

BUILD-11220 cover runs-on-save signal-kill rejection path

e23e9f6

mikolaj-matuszny-ext-sonarsource requested a review from a team as a code owner June 18, 2026 11:30

gitar-bot Bot reviewed Jun 18, 2026

View reviewed changes

Comment thread src/cache-save-decision.ts Outdated

gitar-bot Bot reviewed Jun 18, 2026

View reviewed changes

Comment thread src/cache-save-post.ts

mikolaj-matuszny-ext-sonarsource added 2 commits June 18, 2026 13:55

BUILD-11220 fix CI: reference cache-save from branch, exclude its bun…

3229e20

…dles from pre-commit

BUILD-11220 address SonarCloud findings: drop test any-types, use rep…

d37bd70

…laceAll, safe shell glob

mikolaj-matuszny-ext-sonarsource marked this pull request as draft June 18, 2026 14:39

mikolaj-matuszny-ext-sonarsource added 3 commits June 18, 2026 16:46

BUILD-11220 add content-manifest digest (size+path) via @actions/glob

681a8a9

BUILD-11220 make skip decision content-aware (size+path digest)

46606a9

BUILD-11220 fix README markdownlint (merge limitation blockquotes)

b5366a2

gitar-bot Bot reviewed Jun 18, 2026

View reviewed changes

BUILD-11220 restore exact-key-hit skip (fix redundant save on warm re…

b88b129

…run)

Navigation Menu

Uh oh!

BUILD-11220 Skip redundant cache save when content unchanged - #86

BUILD-11220 Skip redundant cache save when content unchanged#86
mikolaj-matuszny-ext-sonarsource wants to merge 17 commits into
masterfrom
feat/mmatuszny/BUILD-11220-skip-redundant-cache-save

mikolaj-matuszny-ext-sonarsource commented Jun 18, 2026

Uh oh!

sonarqubecloud Bot commented Jun 18, 2026 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

mikolaj-matuszny-ext-sonarsource commented Jun 18, 2026

Uh oh!

gitar-bot Bot Jun 18, 2026

Uh oh!

sonarqubecloud Bot commented Jun 18, 2026 •

edited by SonarQube Agent AI

Loading

Uh oh!

gitar-bot Bot commented Jun 18, 2026

Uh oh!

sonarqube-cloud-us Bot commented Jun 18, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

mikolaj-matuszny-ext-sonarsource commented Jun 18, 2026