Skip to content

fix: skip cache save when path is missing#266

Open
etienne-martin wants to merge 1 commit into
pnpm:masterfrom
sudden-network:fix/cache-save-missing-path
Open

fix: skip cache save when path is missing#266
etienne-martin wants to merge 1 commit into
pnpm:masterfrom
sudden-network:fix/cache-save-missing-path

Conversation

@etienne-martin

@etienne-martin etienne-martin commented Jun 11, 2026

Copy link
Copy Markdown

Summary

  • Skip post-job cache save when the resolved PNPM cache path does not exist
  • Avoid failing cold-cache workflows that restore cache but never create a PNPM store

Fixes #265.

Validation

  • pnpm install --frozen-lockfile
  • pnpm run build
  • pnpm exec tsc --noEmit

Summary by CodeRabbit

Bug Fixes

  • Cache operations now gracefully handle missing cache paths by logging informational messages instead of attempting to save to non-existent directories.

@etienne-martin etienne-martin requested a review from zkochan as a code owner June 11, 2026 18:09
@coderabbitai

coderabbitai Bot commented Jun 11, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 9ccab350-bd41-448a-913c-0b6ab7221ae0

📥 Commits

Reviewing files that changed from the base of the PR and between 0e279bb and 49997ca.

⛔ Files ignored due to path filters (1)
  • dist/index.js is excluded by !**/dist/**
📒 Files selected for processing (1)
  • src/cache-save/run.ts
📜 Recent review details
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-05-11T12:04:07.383Z
Learnt from: zkochan
Repo: pnpm/action-setup PR: 256
File: src/install-pnpm/run.ts:150-166
Timestamp: 2026-05-11T12:04:07.383Z
Learning: In pnpm/action-setup, when validating or forwarding the version value used for `pnpm self-update` and/or `devEngines.packageManager.version`, do not restrict it to exact versions or tags only—pnpm supports semver ranges for these inputs. Ensure any code that parses/validates `devEngines.packageManager.version` (and the value passed to `pnpm self-update`) allows range syntax such as `^`, `~`, and comparators (e.g. `>=8 <10`) instead of rejecting anything that isn’t a single exact version. Note: the plain `packageManager` field is different, so apply this “allow semver ranges” rule specifically to `devEngines.packageManager.version` / `self-update` handling.

Applied to files:

  • src/cache-save/run.ts
🔇 Additional comments (2)
src/cache-save/run.ts (2)

3-3: LGTM!


15-18: Prevent cache save “Path Validation Error” by skipping saveCache when cache_path is missing/empty

  • runSaveCache now returns early when existsSync(cachePath) is false, so saveCache([cachePath], primaryKey) is never called with a non-existent path.
  • @actions/core getState('cache_path') returns '' when the state key isn’t set, so existsSync('') evaluates false and safely triggers the early return.
  • Logging at info level matches the “cold-cache is not an error” intent; this should stop the post-job failure path noted in #265.

Recommended: run a cold-cache workflow with cache: true and confirm post-job logs no longer contain “Path Validation Error”.


📝 Walkthrough

Walkthrough

The PR adds a guard to runSaveCache that verifies the resolved cachePath exists on disk before attempting to save the cache. If the path does not exist, it logs an informational message and returns early, preventing downstream errors.

Changes

Cache path existence check

Layer / File(s) Summary
Path existence check guard
src/cache-save/run.ts
fs.existsSync import added and guard logic introduced to validate cachePath exists before calling saveCache; missing paths are logged and skipped.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~5 minutes

Poem

A rabbit hops through paths with care,
Checking if the cache is there—
If missing found, a graceful bow,
No errors crash, we gently go. 🐰✨

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately reflects the main change: adding a check to skip cache save when the resolved path is missing.
Linked Issues check ✅ Passed The PR directly addresses issue #265 by implementing a check to skip cache save when the path doesn't exist, preventing post-job failures on cold-cache runs.
Out of Scope Changes check ✅ Passed All changes are directly related to the stated objective of skipping cache save when paths are missing; no unrelated modifications detected.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@qodo-free-for-open-source-projects

Copy link
Copy Markdown

PR Summary by Qodo

Skip cache save when PNPM store path is missing
🐞 Bug fix 🕐 10-20 Minutes

Grey Divider

Walkthroughs

Description
• Skip post-job cache save when the resolved cache path does not exist.
• Prevent cold-cache workflows from failing when pnpm store is never created.
• Regenerate the bundled action output to include the new guard.
Diagram
graph TD
  A["GitHub Actions job"] --> B["Restore cache"] --> C["runSaveCache()"] --> D{"Primary key hit?"}
  D -- "Yes" --> E["Skip save"]
  D -- "No" --> F{"Cache path exists?"}
  F -- "No" --> E
  F -- "Yes" --> G["saveCache()"] --> H["GitHub Cache"]
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Try/catch around saveCache() and treat ENOENT as non-fatal
  • ➕ Avoids an extra filesystem check
  • ➕ Centralizes error-handling around the call that can fail
  • ➖ Requires inspecting error shapes/codes across runners/node versions
  • ➖ Harder to ensure only missing-path errors are suppressed
2. Ensure pnpm store directory exists before saving (mkdir -p)
  • ➕ Allows saving an empty store directory (if desired)
  • ➕ May simplify downstream logic if empty cache artifacts are acceptable
  • ➖ Could create empty cache entries and waste cache quota/time
  • ➖ Changes behavior beyond 'do not fail' (creates filesystem side effects)

Recommendation: The current existsSync(path) guard is the simplest and most predictable fix: it prevents a known failure mode (missing store directory) without altering cache contents or error semantics for other failures. The alternatives add complexity or introduce new side effects (saving empty caches).

Grey Divider

File Changes

Bug fix (1)
run.ts Guard cache save when the resolved cache path is missing +6/-0

Guard cache save when the resolved cache path is missing

• Imports fs.existsSync and skips calling @actions/cache.saveCache when the resolved cache path does not exist. Logs an informational message explaining why the save was skipped.

src/cache-save/run.ts


Other (1)
index.js Regenerate bundled action output with missing-path cache-save guard +111/-111

Regenerate bundled action output with missing-path cache-save guard

• Updates the compiled distribution bundle to include the new existsSync(cachePath) check and log message in the post-job cache save codepath.

dist/index.js


Grey Divider

Qodo Logo

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant