ci: cap validate-build at 15 minutes - #429
Conversation
Successful runs finish in 1-2 minutes. Without a job timeout, a wedged npm ci - seen on PR #395 when the node_modules cache had expired after 9 idle days and the registry connection stalled - occupies a runner for GitHub's full 6-hour default before being cancelled, and reports as an opaque failure on the PR. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
✅ Deploy Preview for docs-ui ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe Estimated code review effort: 1 (Trivial) | ~2 minutes Merge Risk: ⚪ Minimal · up to This change caps the build validation job at 15 minutes to prevent indefinitely hung runs; no actionable merge-blocking risk remains beyond normal checks and review. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
micheleRP
left a comment
micheleRP
left a comment
There was a problem hiding this comment.
Approving. The change is sound: both files parse, both checks pass, and it merges cleanly onto v4.1.0 main. I confirmed all three cited runs were cancelled at the six-hour mark on their first attempts (34130604018 now shows green only because attempt 2 was re-run on Sept 9).
Four inline comments: two are corrections to the added comment text, one is about how the validate-build cap is sized, and one is a suggestion that removes the actual hang source from that job. Please fix the two comments before merging; the rest is your call.
Optional follow-ups, not for this PR unless you want them:
- Step timeout and retry on the bloblang Chrome install. The 20-minute job cap now bounds the two steps that actually hung (
Install npm dependencies,Install Chrome for Puppeteer), but a stall still burns all 20 minutes, reports ascancelledexactly like the six-hour default did, and leaves both caches unsaved. Something liketimeout-minutes: 8on the Chrome install step (about 1.5x the observed cold download) plus a retry aroundnpx puppeteer browsers install chromefails fast and self-heals, with the job cap as backstop. - Concurrency group. Run 34213096565 (the six-hour hang on
docs-signin-nudge-and-agent-nav) was superseded by run 34213824481 on the same branch eight minutes later, which passed, and the hung run kept its runner anyway.concurrency: { group: ${{ github.workflow }}-${{ github.ref }}, cancel-in-progress: true }on both workflows releases the runner at the next push regardless of the timeout value. If cancelling a superseded push:main cache-warming run is a concern, gate it withcancel-in-progress: ${{ github.event_name == 'pull_request' }}. - Overlapping limits, nit.
Bundle UIandLint Go codeeach keep a 10-minute step cap under the new 15-minute job cap. On a cold runBundle UIstarts around minute 7, so its own cap can never bind there and a stall reports as a generic job timeout rather than a step timeout. Fine to leave; just noting the two limits no longer have a stated relationship.
| jobs: | ||
| build: | ||
| runs-on: ubuntu-22.04 | ||
| # Successful runs take 1-2 minutes; without a cap, a wedged npm ci (seen |
There was a problem hiding this comment.
Please fix the comment. "Successful runs take 1-2 minutes" is the warm-cache path only. On a node_modules cache miss the job takes about 7 minutes, nearly all of it npm ci: runs 33752155131 and 32899354267 succeeded in 7.2 and 7.0 minutes with Install dependencies at 5.4 and 5.8 minutes. So the cap has roughly 2x headroom over a legitimate cold run, not the ~7x the description derives from the warm figure. Both of this PR's check runs hit the cache and skipped npm ci, so the green checks show the YAML parses, not that the cap clears a cold install. Suggest stating both: "warm 1-2 min, cold npm ci ~7 min".
There was a problem hiding this comment.
Fixed. The comment now gives both paths and cites your two runs: warm 1-2 minutes, cold 7.2 and 7.0 with the install step at 5.4 and 5.8. I also folded in your cache-save point from the cap-value thread, since that is the reason the cap has to stay clear of the cold path rather than merely above it.
Your numbers reproduce exactly. Run 33752155131 ran 11:53:27 to 12:00:49 (7.4 min) with Install dependencies 11:54:40 to 12:00:05 (5.4), and 32899354267 ran 21:08:20 to 21:15:28 (7.1) with install 21:08:53 to 21:14:41 (5.8).
| build: | ||
| runs-on: ubuntu-22.04 | ||
| # Successful runs take 1-2 minutes; without a cap, a wedged npm ci (seen | ||
| # when the node_modules cache expires and the registry stalls) burns the |
There was a problem hiding this comment.
Suggestion, and a note on attribution. This job's npm ci runs Puppeteer's postinstall Chrome download (roughly 5 of the 7 cold minutes) although nothing in validate-build uses Chrome: gulp bundle and the four node --test suites here are Chrome-free, and Puppeteer is only imported by the bloblang, negative-cache, and property-tooltips tests, which run in the other workflow. publish-release.yml already sets PUPPETEER_SKIP_DOWNLOAD: 'true' with a comment saying that download "repeatedly stalled npm ci". Adding the same env to the install step here removes the hang vector and cuts every cold run by about 5 minutes. It can't break the bloblang workflow's shared node_modules cache, because Chrome lives in ~/.cache/puppeteer and that workflow already installs Chrome explicitly.
On attribution: all three cited six-hour hangs were Chrome downloads (33752155029 inside npm ci; 34130604018 attempt 1 and 34213096565 in Install Chrome for Puppeteer), so "the registry stalls" in this comment is probably the CDN rather than the npm registry.
There was a problem hiding this comment.
Taken. PUPPETEER_SKIP_DOWNLOAD: 'true' is now on the install step, with a comment pointing at publish-release.yml as the precedent.
I checked the Chrome-free claim by execution rather than by reading, because a wrong answer here fails the job rather than slowing it. I ran all four suites with a Module._load hook that throws on any puppeteer or puppeteer-core require: markdown-dropdown, signin-nudge, chat-panel-navigation and toc-collapsible all pass with it installed. As a control, the same hook trips test:property-tooltips (FAIL: puppeteer was required by tests/property-tooltips/truncate-description-html.test.js), which is one of the suites that runs in the other workflow, so the guard is real and not vacuously green.
| # Successful runs take 1-2 minutes; without a cap, a wedged npm ci (seen | ||
| # when the node_modules cache expires and the registry stalls) burns the | ||
| # runner for GitHub's full 6-hour default before the job is cancelled. | ||
| timeout-minutes: 15 |
There was a problem hiding this comment.
Cap value. Given the ~7-minute cold path, I'd either raise this to 20 to match the bloblang job in this PR and publish-release.yml, or take the suggestion on the line above (skip the Chrome download) so the cold path itself drops to a couple of minutes and 15 becomes genuinely generous. The failure mode with 15 as-is: a job cancelled by timeout-minutes never runs the cache save (actions/cache@v4 is post-if: success()), so on a slow-registry day a cold npm ci that would have finished in 16 minutes and warmed the cache for everyone instead times out, leaves the cache empty, and the next run (and the bloblang workflow, which restores the same node-modules-Linux-<lockhash> key) repeats the same cold timeout until the registry recovers.
There was a problem hiding this comment.
Took the suggestion on the line above rather than raising the cap, which is the resolution you offered: with the Chrome download gone the cold path drops to a couple of minutes and 15 is genuinely generous.
Your cache-save point is the part I had missed, and it is now in the comment so the next person does not raise the cap without knowing why it is where it is: actions/cache@v4 is post-if: success(), so a job cancelled by timeout-minutes leaves the cache unwarmed for everyone behind it. That is an argument about the gap between the cap and the cold path, not about the cap value, which is why closing the gap from the other side is the better fix here.
Left your three optional follow-ups out of this PR as you suggested. The step timeout plus retry on the Chrome install is the one I think is worth doing next, since the 20-minute cap still lets a stall burn the full 20 and report as cancelled.
| runs-on: ubuntu-latest | ||
| # Healthy runs take 2-8 minutes. Three runs in Sep 2026 hung for the full | ||
| # 6-hour default (twice in "Install Chrome for Puppeteer", once in npm ci); | ||
| # the step-level timeout below only covers the test step itself. |
There was a problem hiding this comment.
Please fix the comment. The only step-level timeout-minutes in this job is on Lint Go code (line 104), not on the test step. None of Build and Test Playground, Test Interactive Features, Test Negative Cache (or Test Property Tooltips on main) carries one. The PR description repeats the same claim ("Its only existing timeout was step-level, on the test step"). As written, a maintainer would believe the Puppeteer test steps are already bounded. Suggest: "the only step-level timeout below is on the Go lint step".
There was a problem hiding this comment.
Fixed. It now says the only step-level timeout below is on the Go lint step, and that neither of the steps that actually hung was bounded by anything before the job cap. Confirmed there is exactly one step-level timeout-minutes in that job, at line 104 on Lint Go code; none of Build and Test Playground, Test Interactive Features, Test Negative Cache or Test Property Tooltips carries one. I corrected the same claim in the PR description.
…e-build Both added comments were wrong about what they described. validate-build: "successful runs take 1-2 minutes" is the warm-cache path only. Runs 33752155131 and 32899354267 took 7.2 and 7.0 minutes with the install step at 5.4 and 5.8 of them, so 15 minutes was roughly 2x a cold run, not the ~7x the warm figure implies. The comment now states both paths, and notes that a job cancelled by timeout-minutes never runs the cache save, which is why the cap has to stay clear of the cold path. test-bloblang-playground: the only step-level timeout in that job is on the Go lint step, not the test step. Neither of the steps that actually hung was bounded by anything before the job cap. Rather than raise the validate-build cap, skip Puppeteer's Chrome download in that job, which is where most of the cold 7 minutes and the npm ci hang vector come from. Nothing there uses Chrome: gulp bundle and all four node --test suites are Chrome-free, verified by running them with puppeteer hard-blocked at the module loader (all pass; the same guard correctly trips property-tooltips, which runs in the other workflow). publish-release.yml already sets this env for the same reason. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2 participants
Summary
Adds
timeout-minutes: 15to thebuildjob invalidate-build.yml. Without a cap, a wedgednpm cioccupies a runner for GitHub's full 6-hour default before being cancelled.This is exactly what hit #395: the branch had been idle 9 days, so the
node_modulescache had been evicted, the resulting realnpm cistalled on the registry with no output, and the job hung for 6 hours before GitHub killed it - reported as an opaque check failure on a commit that only touched a header partial. (A re-run passed with no changes.)Sizing the cap
Warm-cache runs finish in 1-2 minutes, but that is not the figure the cap has to clear. A cold run is dominated by
npm ci: runs 33752155131 and 32899354267 took 7.2 and 7.0 minutes with the install step at 5.4 and 5.8 of them. So against a legitimate cold run, 15 minutes is about 2x headroom, not the ~7x the warm figure suggests.Rather than raise the cap, this also removes most of the cold path:
PUPPETEER_SKIP_DOWNLOAD: 'true'on the install step. Puppeteer's ~120 MB Chromium download is roughly 5 of those 7 minutes and is thenpm cihang vector, and nothing in this job uses Chrome -gulp bundleand all fournode --testsuites are Chrome-free, and puppeteer is only imported by the bloblang, negative-cache and property-tooltips tests, which run in the other workflow.publish-release.ymlalready sets the same env for the same reason.Verified by execution rather than by reading, since a wrong answer fails the job: all four suites pass with a
Module._loadhook that throws on anypuppeteerrequire, and the same hook correctly tripstest:property-tooltips, so the check is not vacuously green.This matters for the cap value because a job cancelled by
timeout-minutesnever runs the cache save (actions/cache@v4ispost-if: success()). A coldnpm cithat times out therefore leaves the cache empty for everyone behind it, so the cap has to stay clear of the cold path, not merely above it.Bloblang playground
Also caps the
test-bloblang-playgroundjob (its own workflow) at 20 minutes. That is the job that actually burned the runner this month: three runs cancelled at the 6-hour limit, twice hung in "Install Chrome for Puppeteer" and once innpm ci(runs 33752155029, 34130604018, 34213096565). Healthy runs take 2-8 minutes.Before this, the only step-level
timeout-minutesin that job was on the Go lint step (line 104). Neither of the steps that actually hung, nor any of the Puppeteer test steps, was bounded by anything.Follow-ups (not in this PR)
timeout-minutesplus a retry on the Chrome install step, so a stall fails fast instead of burning the full 20 minutes and reporting ascancelled.concurrencygroup on both workflows, so a superseded push releases the runner regardless of the cap.Test plan
🤖 Generated with Claude Code