Repository navigation
fix(ci): restore the pre-commit npm cache from a prefix key - #45909
Open
sadpandajoe wants to merge 2 commits into
Open
sadpandajoe wants to merge 2 commits into
sadpandajoe wants to merge 2 commits into
Conversation
setup-node's built-in `cache: npm` keys on the exact superset-frontend/package-lock.json hash and has no restore-keys fallback, so any lockfile change (every dependency bump) runs `npm ci` from an empty cache and re-downloads every tarball, including xlsx from cdn.sheetjs.com rather than the npm registry. On 2026-10-28 that cold path failed `pre-commit (current)` on two Dependabot PR runs (40649064300: ECONNRESET after a 10-minute stall; 40679638770: E503 from cdn.sheetjs.com), both green on re-run with no code change, and stalled a master `pre-commit (next)` job into its timeout (40678215774). Cache ~/.npm with actions/cache instead, keyed on the lockfile hash with a prefix restore-key, so a lockfile change starts from the most recent cache and only the changed packages are fetched. The step is gated on needs_frontend, like the npm ci step it feeds. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Restore ~/.npm whenever Node is set up (needs_node), as setup-node's `cache: npm` did, so a websocket-only change still gets a warm cache for oxfmt-websocket's npx. needs_frontend implies needs_node, so the frontend install is unaffected. Reword the comment: an exact-key cache miss does not cause the failures, it makes npm ci re-download every tarball and so exposes the install to whatever registry/CDN outage is in progress; the prefix restore cuts the downloads to the changed packages. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
sadpandajoe
requested review from
betodealmeida,
dpgaspar,
eschutho,
geido,
hainenber,
kgabryje,
mistercrunch,
msyavuz,
nytai,
rusackas and
sha174n
as code owners
October 30, 2026 22:15
github-actions
Bot
added
the
github_actions
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| # failed. Restoring from the prefix instead cuts the downloads to the | ||
| # changed packages. | ||
| - name: Cache npm | ||
| if: steps.changed_files.outputs.needs_node == 'true' |
Contributor
There was a problem hiding this comment.
Suggestion: Websocket-only changes set needs_node but skip npm ci; this can save a cache with only npx data at the lockfile key, which later frontend runs cannot update.
Assessment: 🟠 Major · 🔁 Occurrence: Rarely · 🏷️ Cache
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** .github/workflows/pre-commit.yml
**Line:** 186:186
**Comment:**
*Cache: Websocket-only changes set `needs_node` but skip `npm ci`; this can save a cache with only `npx` data at the lockfile key, which later frontend runs cannot update.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Contributor
|
Contributor
|
I'm hesitant to introduce |
Member
Author
thanks @hainenber for the insight, I'm not super attached to this PR so if we think it will cause more issues than it will help, more than happy to just close it out. |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
SUMMARY
The pre-commit workflow's Setup Node.js step used setup-node's built-in
cache: npm, which keys on the exactsuperset-frontend/package-lock.jsonhash with no fallback. Every push or PR that changes the lockfile (every Dependabot npm bump, for example) therefore startsnpm ciwith an empty cache and downloads ~2,850 tarballs, includingxlsxfromcdn.sheetjs.comrather than the npm registry. That makes the job fail whenever the registry or that CDN has a blip during the install. Between 2026-10-22 and 10-29 twopre-commit (current)runs failed this way and passed on re-run with no code change (40649064300:ECONNRESETafter an 11-minute stall; 40679638770:503oncdn.sheetjs.com/xlsx-0.20.3/xlsx-0.20.3.tgz), and a masterpre-commit (next)run timed out stalled in the same step (40678215774). No specific introducing change: this is how setup-node's cache has always keyed.This replaces it with an
actions/cachestep on~/.npmkeyednpm-v1-<os>-<lockfile hash>with anpm-v1-<os>-restore prefix, gated on the sameneeds_nodecondition as Setup Node.js. After a lockfile change,npm cistarts from the most recent cache and fetches only the changed packages. This reduces the exposure; it does not remove it (the first run after merge, or after the cache is evicted, still installs cold). Not a retry, timeout or skip.Measured locally (npm 11,
npm ci --ignore-scripts, one-package lockfile change): 2 network GETs with a cache from the previous lockfile (the changed tarball plus a revalidation of thedom-to-imagegit dependency) vs 1,270 with an empty cache;xlsxserved from cache.CI on this PR is green on 1c3a7ad, but the npm steps are skipped here:
needs_nodeis false for a workflow-only change. The cache gets measured after merge. The first master run seedsnpm-v1-Linux-<hash>, and the next PR that changes the lockfile should restore from it viarestore-keys.TESTING INSTRUCTIONS
pre-commit (current)job: it passes, and Setup Node.js, Cache npm and Install Frontend Dependencies show as skipped (needs_nodeis false for a workflow-only change). After merge, the first master run's Cache npm step logsCache not foundfornpm-v1-Linux-<hash>and saves the cache at the end.superset-frontend/package-lock.json, the Cache npm step logsCache restored from key: npm-v1-Linux-<a different hash>(prefix fallback), and Install Frontend Dependencies finishes without re-downloading the unchanged packages.npm-v1tonpm-v2resets it.ADDITIONAL INFORMATION
🤖 Generated with Claude Code