Repository navigation
ci(frontend-e2e): fail the job when Chromium install wedges, and cache it - #1872
Merged
Merged
Conversation
…e it `npx playwright install --with-deps chromium` wedged five times on 2026-08-19: three attempts on #1864 and two on #1868. Each ran until the job's 10-minute cap killed it, so the run ended `cancelled` with every spec skipped. Cancelled is not failed, so on the two PRs whose entire subject was layout that jest structurally cannot see, the e2e signal was absent while nothing showed as red. Measured over the last 40 runs, walking every attempt rather than only the latest (`gh run list` reports the latest attempt, which is what hides a re-run to green): 39 successful installs, 36 at or under 39s, outliers at 101s, 140s and 381s; 5 cancelled, none under 585s. Two changes: - `timeout-minutes: 8` on the install step, converting the silent skip into a red X naming the step. The distinction it relies on is in the runner itself: `StepsRunner.cs` sets `TaskResult.Failed` when the step's own token fired and the job's did not, and `TaskResult.Canceled` otherwise. 8 minutes sits in the gap the data leaves: above the 381s slowest healthy install, below the 585s floor of every hang. - The job cap goes 10 -> 12 minutes so that step cap is always what fires. Every other step at its worst observed duration sums to 78s, plus about 30s for the cache restore and save, so a full 480s install would land at ~600s, exactly the old job cap. At 10 the job would time out first and cancel the run, reproducing the bug. A cap is not a cost: the median successful job takes 69s (n=39) and only pathological runs approach either limit. `cancel-in-progress` is now scoped to pull requests. Left unconditional it would apply to the new push trigger too, superseding exactly the main run that populates the cache and carries the post-merge signal. - Restore and save `~/.cache/ms-playwright`, keyed on the Playwright version resolved from `package-lock.json`, so the common path skips the download and the 22s it costs. The workflow now also runs on push to main, because a cache written by a `pull_request` run lands in that PR's own scope: with no run on the default branch there is no base-branch scope to inherit, and the first run of every PR would miss. That also gives main a post-merge Playwright signal it did not have. No retry. A retry has to be capped above the healthy maximum or it kills good runs, and two 381s-capped attempts do not fit in a 10-minute job alongside npm ci, the build and the specs. An earlier draft of this change used three 90s attempts, which the distribution above shows would have turned all three healthy outliers red. Restore and save are separate steps rather than the combined `actions/cache`, whose save is a post step gated on `post-if: success()` (verified in the action's own action.yml at the pinned SHA). That never stores the browser on a run whose specs failed, which is exactly the run being iterated on. Saving straight after a successful install decouples the cache from the verdict. Verified locally: actionlint 1.7.12 and zizmor 1.29.0 at `--persona=pedantic --min-severity=medium` both exit 0 with the finding count unchanged from origin/main at every severity; structural assertions on the parsed workflow cover the timeout ordering, that `Run Playwright tests` is unconditional and reachable on both the hit and miss paths, and that the save is the only step gated on the cache; and the version-resolution block was run against the real lockfile to confirm a Playwright bump changes the key, an unrelated lockfile edit does not, and a missing entry fails the step instead of keying on an empty string. The wedge is not reproducible on demand, so none of this shows the download no longer hangs. It shows that a hang now costs a named red step instead of a silently skipped suite. Refs #1869
cristim
added
priority/p1
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
Included review availability: 0 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 1 review per hour. 📝 WalkthroughWalkthroughThe Frontend E2E workflow now runs on relevant pushes to ChangesFrontend E2E reliability
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to The workflow now bounds Chromium installation and caches Playwright artifacts; no actionable merge-blocking risk remains, so it is merge-ready after normal checks and review. Sequence Diagram(s)sequenceDiagram
participant GitHubActions as GitHub Actions runner
participant PackageLock as package-lock.json
participant PlaywrightCache as Chromium cache
participant PlaywrightInstaller as Playwright installer
GitHubActions->>PackageLock: Resolve locked Playwright version
GitHubActions->>PlaywrightCache: Restore versioned Chromium cache
GitHubActions->>PlaywrightInstaller: Install Chromium with 8-minute timeout
PlaywrightInstaller->>PlaywrightCache: Save cache after cache miss and successful install
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
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
1 participant
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.
Closes #1869
What makes this work rather than reduce the odds
Two measured facts carry the whole change.
The separation is clean. Walking the last 40 runs and every attempt of each (not just the latest, which is what hid this):
Nothing sits between 381s and 585s, so "slow" and "wedged" are separable by a cap, but only by one well above 381s.
A step timeout fails; a job timeout cancels. This is not inferred from the docs.
actions/runner,src/Runner.Worker/StepsRunner.cs(~L322-337): when the step's own cancellation token fired and the job's did not, the runner setsTaskResult.Failed; otherwiseTaskResult.Canceled. So moving the bound from the job to the step converts the exact failure #1869 is about, acancelledrun that skipped every spec and read as not-a-failure, into a red X reading "The action 'Install Chromium' has timed out after 8 minutes".This is the repo's first step-level
timeout-minutes(every other one in.github/workflowsis job-level), so there was no local precedent to copy and the runner source was the only available proof.The change
timeout-minutes: 8onInstall Chromiumtimeout-minutes10 → 12~/.cache/ms-playwrightpackage-lock.json. Removes the download from the common path.push: branches: [main]pull_requestrun is readable only within that PR. With no default-branch run there is no base scope to inherit, so the first run of every PR would miss. Also gives main a post-merge Playwright signal it lacked (frontend-build-sentinelruns jest, which cannot resolve stylesheets into layout, which is what #1777 and #1776 were).cancel-in-progressscoped topull_requestFrontend E2E (PR)→Frontend E2EPlaywright Chromium, which is deliberately untouched.No retry, and that is a finding rather than an omission
The issue suggested one, and my first draft used 3 × 90s. The distribution above kills it: a per-attempt cap under 381s turns the three healthy outliers red, and two attempts capped above it do not fit the job budget alongside
npm ci, the build and the specs. The 22s figure in the issue is the best case, not the distribution. The cache is what removes the download from the common path; a retry is what would have to replace it, and cannot.The wedge durations are lower bounds, not measurements
All five wedged attempts were truncated by the job cap, not self-terminating. Job wall clock is a constant 615-618s across all five against the then-600s cap plus cancellation overhead, and the install duration varies inversely with the preamble, which is what confirms truncation:
Install ChromiumSo 585-592s is a floor. Nothing observed says whether the install would have cleared at 601s or never, and a slow download is indistinguishable from a wedged lock in this data. A
__dirlockcontention hypothesis and the concrete diagnostic for a recurrence are recorded in LeanerCloud/cloud-commitments-platform#212.What this does not fix
Playwright Chromiumis not in theprotect-default-branchruleset's required contexts (it requires onlyCI SuccessandRun pre-commit hooks), so a wedge is now visible but still gates nothing. That is the other half of #1869 and is filed as #1871, separately because adding it interacts with thepaths:filter: a required check that never reports on PRs missing the filter blocks them with no failing job to point at.Verification
actionlint 1.7.12(pinned digest) andzizmor 1.29.0 --offline --persona=pedantic --min-severity=medium, the versions gated by chore(ci): gate workflows on actionlint and zizmor #1870: both exit 0, with the finding count identical to a cleanorigin/maincheckout at every severity (106 ignored, 14 suppressed), including thecache-poisoningaudit that a cache plus a default-branch trigger could trip.runblocks were extracted from the parsed YAML and executed: the cache key resolves toLinux-X64-playwright-1.62.0-chromium, changes on a Playwright bump, does not change on an unrelated lockfile edit, and the step fails rather than emitting an empty key if the lockfile entry is missing.Run Playwright testsis unconditional and reachable on both the cache-hit and cache-miss paths.Not verified: the wedge is upstream and not reproducible on demand. Nothing here shows the download stops hanging; it shows a hang costs a named red step instead of a silently skipped suite.
Summary by CodeRabbit