Skip to content

Fix #1313: CI jobs die in npm i at the 5-minute timeout, reporting a red required gate on PRs that pass - #1314

Merged
philcunliffe merged 1 commit into
masterfrom
fix/issue-1313
Sep 4, 2026
Merged

Fix #1313: CI jobs die in npm i at the 5-minute timeout, reporting a red required gate on PRs that pass#1314
philcunliffe merged 1 commit into
masterfrom
fix/issue-1313

Conversation

@philcunliffe

@philcunliffe philcunliffe commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Feature or issue

CI jobs are killed by the 5-minute timeout-minutes while npm i is still running, so a PR whose code is fine reports a red CI required gate and npm test never runs. On run 33815447042 (PR #1312, two markdown files under llp/) all four matrix jobs died in Run npm i after 5:06, and a re-run 23 minutes later killed #1310 typecheck (22) and #1312 typecheck (22), test (22), test (24) the same way. The same suite on master finishes in 99-112s across the last twelve runs, so the suite is nowhere near the ceiling: only the install is slow, and only when several jobs install concurrently. actions/setup-node@v4 had no cache: key at either call site in .github/workflows/ci.yml, so each of the four matrix jobs per PR pulled the whole dependency tree from the registry cold.

Solution

  • Give actions/setup-node@v4 cache: npm at both call sites in .github/workflows/ci.yml, so the npm cache directory is restored across runs instead of every job refetching the tree from the registry, and raise timeout-minutes from 5 to 10 on both jobs so a cold install finishes rather than being killed mid-install.
  • package-lock.json is gitignored in this repo, contrary to the issue's reading, so the cache key is hashed from package.json via cache-dependency-path: without it setup-node fails looking for a lock file, and npm ci is unavailable for the same reason.
  • Verified locally with npm test (5992 pass, 0 fail) and npm run typecheck (clean). Verified on this PR's own CI: the first run reported npm cache is not found and saved node-cache-Linux-x64-npm-35e79a4d...; a re-run got Cache restored from key on all four jobs and every check went green. Install wall time is 92-117s either way on an uncontended runner, so the cache does not shorten an install that was never contended; what it removes is the registry download, which is the part that stalls when a dozen jobs install at once, and the 10-minute budget leaves 5x headroom over a warm install instead of 2.5x. A cache saved on a PR branch is scoped to that PR, so the fleet-wide effect starts once the first master push run seeds one.
  • .github/workflows/llp-check.yml is the only other workflow and installs no dependencies (it runs scripts/llp-numbers.js, which imports only in-repo and node: modules), so it needs no cache.

Code: +0 / -0 lines

Fixes #1313

…d gate on PRs that pass (#1313)

Give actions/setup-node a cache: npm key at both call sites so the npm cache
directory is restored across runs instead of every job pulling the whole
dependency tree from the registry, and raise the job budget to 10 minutes so a
cold install finishes rather than being killed mid-install.

package-lock.json is gitignored in this repo, so cache-dependency-path names
package.json: without it setup-node fails looking for a lock file, and npm ci is
unavailable for the same reason.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@philcunliffe

Copy link
Copy Markdown
Contributor Author

Review round: 233b582b - clean

Verdict: approve. 0 actionable findings, nothing fixed, nothing pushed. The head is unchanged at 233b582b677c7329bf953b1af79bb1f95cafd638.

The diff is 14 lines in one file, .github/workflows/ci.yml: cache: npm + cache-dependency-path: package.json at both actions/setup-node@v4 call sites (ci.yml:33-34, ci.yml:49-50), timeout-minutes 5 -> 10 on both jobs (ci.yml:24, ci.yml:40), and an 8-line header comment. No runtime code, no new dependency, no schema or config key.

What I verified

The premise holds. package-lock.json is untracked and gitignored at .gitignore:9, so npm ci is genuinely unavailable and a bare cache: npm would fail with "Dependencies lock file is not found" before installing anything. cache-dependency-path: package.json is the correct and only available key source. Root package.json declares no workspaces and is the only tracked package.json in the repo, so hashing it covers the entire declared dependency set - there is no second manifest whose changes the key would miss.

The fix works, empirically, on this SHA. Run 33818848413 (attempt 2, head 233b582b) logged Cache restored from key: node-cache-Linux-x64-npm-35e79a4d... on all four matrix jobs. Run npm i took 96s, 103s, 117s and 96s against a 10-minute budget, and every check including CI required went green. This is the acceptance condition in #1313, met and observed rather than asserted.

Scope is right. .github/workflows/llp-check.yml is the only other workflow and installs no dependencies (checkout plus node scripts/llp-numbers.js, which imports only in-repo and node: modules), so leaving it uncached is correct, not an omission.

Conventions. No em dash (U+2014), no NUL byte, no tab, no trailing whitespace anywhere in the changed file. No LLP is owed: CLAUDE.md puts bug fixes outside the LLP threshold, and no doc or LLP states a CI timeout or cache policy this contradicts (LLP 0125 fixes only the 22/24 setup-node matrix, which is untouched).

Local checks in a clean worktree at 233b582b: npm test 5992 pass / 0 fail (exit 0), npm run typecheck clean.

Non-blocking observations

Neither is a finding; both are recorded so the next reader does not have to rediscover them.

  1. All four matrix jobs share one cache key. setup-node's key omits the Node version, so typecheck (22), typecheck (24), test (22) and test (24) all compute node-cache-Linux-x64-npm-35e79a4d.... This is benign: the npm cache holds registry tarballs, which are Node-version agnostic, and the run above shows all four restoring from that single key. The only visible effect is that three of the four skip the save as already-reserved, which is a warning, not a failure.

  2. A version bump busts the cache. The key is hashFiles('package.json'), so a release version bump invalidates it even though no dependency changed, and setup-node exposes no restore-keys fallback to soften that. The cost is one cold install per bump, comfortably inside the new 10-minute budget. Buying it back would mean replacing setup-node's built-in caching with an explicit actions/cache step keyed on a narrower input - a larger change than CI jobs die in npm i at the 5-minute timeout, reporting a red required gate on PRs that pass: no dependency cache on setup-node #1313 asks for, so leaving it is the right call under "make the smallest change that fixes the problem".

Residual risk

Raising the budget to 10 minutes means a genuinely wedged job now burns 10 runner-minutes instead of 5. That is the trade the issue explicitly asks for, and the cache is what makes the wedge less likely in the first place. A PR-scoped cache only helps that PR; the fleet-wide effect starts once the first master push run seeds a default-branch cache, which the PR body already calls out.

@philcunliffe philcunliffe added the neutral:approved neutral reviewed this and holds it for a maintainer merge (own or adopted PR; LLP 0025/0030) label Sep 4, 2026
@philcunliffe

Copy link
Copy Markdown
Contributor Author

Ship risk: low

Who could be affected: Nobody using HypAware. This change only affects the project's own automated checks, so the only people who could notice a difference are the people working on the code.

What could happen: Nothing a user of the product can see. The published version of HypAware is unchanged, and nothing the app runs reads the material that was edited. For contributors, the automated checks now reuse packages downloaded on earlier runs instead of fetching everything again, and are allowed ten minutes instead of five before being cut off. The trade-off is that a check that genuinely hangs now takes twice as long to give up.

Why this level: The change cannot reach anyone's data, access, privacy, or installed software. Its worst case is slower feedback for the project's own contributors, and it can be undone by reverting one settings change.

What was checked: The package that ships to users was rebuilt and confirmed to contain none of the edited material, using a check proven to catch it if it had. The project's own checks were also observed running to completion successfully on this exact version, inside the new time limit.

@philcunliffe
philcunliffe added this pull request to the merge queue Sep 4, 2026
Merged via the queue into master with commit 6257c77 Sep 4, 2026
13 checks passed
@philcunliffe
philcunliffe deleted the fix/issue-1313 branch September 4, 2026 00:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

neutral:approved neutral reviewed this and holds it for a maintainer merge (own or adopted PR; LLP 0025/0030)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

CI jobs die in npm i at the 5-minute timeout, reporting a red required gate on PRs that pass: no dependency cache on setup-node

1 participant