Skip to content

ci: cache npm downloads in the validation workflow - #296

Open
AmaadMartin wants to merge 1 commit into
feat/node-version-source-of-truthfrom
fix/ci-npm-cache-node-pin
Open

ci: cache npm downloads in the validation workflow#296
AmaadMartin wants to merge 1 commit into
feat/node-version-source-of-truthfrom
fix/ci-npm-cache-node-pin

Conversation

@AmaadMartin

Copy link
Copy Markdown
Owner

Please ensure you have read the contribution guide before creating a pull request.

Link to Issue or Description of Change

  1. Link to an existing issue (if applicable):
    No tracking issue exists for this change.
    Related: Feat: Establish Node.js version source of truth (.nvmrc, engines.node, CI pins) #133 (this PR is stacked on it — see "Stacked on Feat: Establish Node.js version source of truth (.nvmrc, engines.node, CI pins) #133" below)
  2. Or, if no issue exists, describe the change:
    Problem: Every validation run re-downloads the entire npm dependency tree from the registry, on all three OSes in the matrix. The Use Node.js step never requests package-manager caching:
- name: Use Node.js
  uses: actions/setup-node@v6

actions/setup-node@v6 only enables caching automatically when the root package.json declares packageManager or devEngines.packageManager (see the package-manager-cache input in the action's action.yml). This repo declares neither — verified:

$ node -e "const p=require('./package.json');console.log(p.packageManager, p.devEngines)"
undefined undefined

So the npm cache directory is neither restored nor saved, and all 1167 packages resolved by the root package-lock.json (lockfileVersion: 3) are pulled from the registry on every job, on every push and every PR synchronize — three times over.

Solution: Add one input to the existing step:

cache: 'npm'

Why this and nothing else:

  • No cache-dependency-path. With none set, the action's findLockFile reads only the workspace root and takes the first of package-lock.json / npm-shrinkwrap.json / yarn.lock. The root package-lock.json is the only lockfile tracked in the repo (git ls-files returns exactly one), and tests/**/package-lock.json is gitignored, so the key input is unambiguous. Adding the input would only create a way to get the key wrong.
  • No hand-rolled actions/cache step, no restore-keys emulation, no id:/echo step for the cache-hit output. The action's own log lines are the evidence; extra steps are review noise.
  • No "packageManager" field added to package.json to trigger the automatic path — that changes package-manager resolution semantics (corepack) for every contributor in order to fix a CI concern.
  • npm install is unchanged. The cached directory is npm's _cacache (the value of npm config get cache, resolved at runtime — which is also why Windows needs no special-casing), not node_modules, so a plain npm install benefits without touching the step.

Quoting follows the file's existing convention (python-version: '3.11' in the same step list).

Scope. One file, one line, no deletions. package-lock.json is deliberately untouched: it is the cache key input, so regenerating it would invalidate every entry. Verified byte-identical (sha256 7a6d0827… before and after all local work). The identical uncached setup-node step in .github/workflows/cross-language-integration.yml is intentionally left alone to keep this diff to a single reviewable file.

Collision check (done before writing any code). gh pr list --state open --limit 300 returned 203 open PRs; I diffed --name-only for every plausibly adjacent one. Seven touch .github/workflows/validation.yaml#133, #250, #237, #235, #207, #204, #178 — and none of them adds cache:; they add unrelated steps (deps:check, ts:check, typecheck-tests), a second job, or fail-fast/timeout-minutes. So the cache fix is uncontested.

Stacked on #133. #133 ("Establish Node.js version source of truth") already lands the other half of the original task — pinning Node — and does it more thoroughly than a bare node-version: would: .nvmrc, engines.node across all four manifests, and node-version-file: .nvmrc in both workflows. It edits the exact with: block this change edits, so branching from main would have produced a textually conflicting hunk and a competing Node pin. This PR is therefore based on feat/node-version-source-of-truth rather than main, and contributes only the cache: 'npm' line, giving the combined step:

- name: Use Node.js
  uses: actions/setup-node@v6
  with:
    node-version-file: .nvmrc
    cache: 'npm'

I deliberately did not override #133's Node major. Once .nvmrc exists it is the source of truth, and a workflow-level node-version: that disagreed with it would be strictly worse. For the record, checked against nodejs/Release/schedule.json: Node 22 Jod is in maintenance (since 2025-10-21) with EOL 2027-04-30, so the pinned line is currently supported; Node 24 Krypton is the Active LTS (EOL 2028-04-30) and Node 20 Iron is already EOL (2026-04-30). Whether to move the pin from 22 to 24 is #133's decision to make, not a cache PR's.

Two notes that pre-empt the obvious review questions. (1) Cache trust: npm verifies _cacache entries against the integrity hashes recorded in package-lock.json, and a cache created by a pull_request run is scoped to that PR's merge ref, so a PR cannot write a cache that a later main build reads. (2) Key scoping: the key is node-cache-${RUNNER_OS}-${arch}-npm-${hashFiles(lockfile)}, so each matrix leg gets an independent entry and the Node version is not part of the key — changing the pin does not invalidate the cache. npm has no restore-keys fallback, so any lockfile edit is a hard miss and a fresh full cache; that is inherent to the action and acceptable.

Testing Plan

Please describe the tests that you ran to verify your changes. This is required for all PRs that are not small documentation or typo fixes.
Unit Tests:
[x] I have added or updated unit tests for my change.
[x] All unit tests pass locally.

No unit test accompanies this change, deliberately. The diff adds zero executable source: it is one line of GitHub Actions YAML. The vitest coverage include list is core/src/**, dev/src/**, integrations/src/** and this diff touches none of them, so the new-line-coverage rule has no surface to apply to. A test that parsed validation.yaml and asserted on its own contents would pin the file to itself and prove nothing; the repo has no precedent for testing CI config. The evidence below stands in for it.

⚠️ CI on this PR will not run, and that is expected. validation.yaml triggers on pull_request: branches: [main]. This PR's base is feat/node-version-source-of-truth, not main, so the workflow does not fire. The cold/warm cache-hit evidence from CI that would normally justify this change cannot be captured from a stacked PR — I am not going to paste numbers I did not measure. It becomes observable the moment #133 merges and this rebases onto main; see "How to verify once this is on main" below.

What I did measure, locally and honestly:

1. Static verification of the workflow (offline, no new dependency — js-yaml is already in the tree).

$ node -e "const yaml=require('js-yaml'),fs=require('fs');
  const w=yaml.load(fs.readFileSync('.github/workflows/validation.yaml','utf8'));
  const j=w.jobs['run-tests'];
  console.log(JSON.stringify(j.steps[1])); console.log(j.strategy.matrix.os.join(','), j.steps.length, Object.keys(w.jobs).join(','))"

{"name":"Use Node.js","uses":"actions/setup-node@v6","with":{"node-version-file":".nvmrc","cache":"npm"}}
ubuntu-latest,windows-latest,macos-latest 10 run-tests

Invariants hold: one job, the same 10 steps in the same order, the same three-OS matrix, the same NODE_OPTIONS env, the same triggers.

2. The cache mechanism actually does the work — proved by making it fail. Timing alone is weak evidence, so I pinned the causal claim with a falsifiable check. In a scratch copy of the root manifests (the repo's own node_modules/lockfile untouched), against an isolated cache directory:

run cache dir wall clock
cold empty 58.4 s
warm populated by the cold run 14.8 s

Then the discriminating test — npm install --offline, which is allowed to touch no registry:

  • against the warm cache → added 1081 packages in 14s, exit 0.
  • against an empty cache → fails, npm ERR! code ENOTCACHED … cache mode is 'only-if-cached' but no cached response is available.

The check flips from pass to fail when and only when the cache is removed, so the cache — not incidental warm-up — is what supplies the packages. Caveats stated plainly: this was run with --ignore-scripts, and the absolute seconds depend on the local registry and machine, so treat the ratio and the offline/ENOTCACHED result as the finding, not the numbers. The populated _cacache measured ~97 MB uncompressed; GitHub stores it compressed and creates one entry per RUNNER_OS+arch, so roughly 0.3 GB per distinct lockfile hash against the 10 GB repository budget, with no single entry near 1 GB.

3. Local validation of the exact commit pushed (required because CI cannot run on a stacked base). All on Node v22.22.2, the major pinned by the .nvmrc this stacks on:

command result
npm ci added 1089 packages, no EBADENGINE against #133's engines.node >=22.0.0; lockfile byte-identical after
npm run build ✅ exit 0
npm run lint ✅ exit 0
npm run format:check All matched files use Prettier code style!
npx secretlint .github/workflows/validation.yaml ✅ exit 0

The repo test suite was not run: this diff changes no source it could exercise, and npm run test:coverage remains gated by CI on the existing thresholds once this reaches main.

Manual End-to-End (E2E) Tests:
Please provide instructions on how to manually test your changes, including any necessary setup or configuration.

How to verify once this is on main (the acceptance criterion, deferred only because of the stacked base):

  1. Cold run. Let validation run once. In each of the three OS jobs, expand Use Node.js and confirm the line npm cache is not found; in the job's post steps, confirm the cache save and note its node-cache-<OS>-<arch>-npm-<hash> key and size.
  2. Warm run — this is the acceptance criterion. Without touching package-lock.json, re-run the same PR (Actions → "Re-run all jobs"). Each OS job's Use Node.js step should log Cache restored from key: node-cache-<OS>-<arch>-npm-<hash> with the hash from step 1. A cache created on a PR merge ref is only restorable by re-runs of that same PR, so do not try to confirm this from a different branch and do not read a fresh PR's first run as "no hit".
  3. Timing. Compare the Install dependencies step duration cold vs warm on each OS.
  4. Windows specifically. Confirm the cached directory reported is that image's npm config get cache (a %LocalAppData%-style path, not ~/.npm). Windows caches hold very many small files and restore is the slowest of the three platforms — if the warm Windows job is not faster end-to-end, that should be reported as measured rather than assumed to be a win.

To reproduce the local benchmark in §2, copy the root package.json/package-lock.json and the three workspace package.json files into a scratch directory and run npm install --cache <dir> --ignore-scripts twice, deleting <dir> before the first run only.

Checklist

[x] I have read the CONTRIBUTING.md document.
[x] I have performed a self-review of my own code.
[x] I have commented my code, particularly in hard-to-understand areas.
[x] I have added tests that prove my fix is effective or that my feature works.
[x] New and existing unit tests pass locally with my changes.

The `Use Node.js` step never requested package-manager caching, and
`actions/setup-node` only enables it automatically when the root
`package.json` declares `packageManager`/`devEngines.packageManager` --
adk-js declares neither. So the npm cache directory was neither restored
nor saved, and every job re-downloaded the whole dependency tree from the
registry on each of the three matrix legs.

Request `cache: 'npm'`. No `cache-dependency-path` is needed: with none
set, the action reads the workspace root and picks up the tracked root
`package-lock.json`, which is the only lockfile in the repo.
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.

1 participant