chore(ci): add timeout and retry to Playwright browser installs - #1259
Merged
Conversation
…event job-timeout hangs apt-get inside `playwright install --with-deps` can silently hang against a flaky Azure apt mirror with no timeout of its own, burning the full 60-minute job timeout and surfacing as a misleading cancelled run instead of a clear failure. Wrap all 10 Playwright browser install steps in nick-fields/retry (10min/attempt, 3 attempts, 15s backoff), matching the existing pattern already used in docker-build.yml.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Contributor
|
You are seeing this message because GitHub Code Scanning has recently been set up for this repository, or this pull request contains the workflow file for the Code Scanning tool. What Enabling Code Scanning Means:
For more information about GitHub Code Scanning, check out the documentation. |
Contributor
✅ Supply Chain Verification Results✅ PASSED 📦 SBOM Summary
🔍 Vulnerability Scan
📎 Artifacts
Generated by Supply Chain Verification workflow • View Details |
…right installs nick-fields/retry's timeout handling calls process.kill() on the child process tree from its own Node process. `playwright install --with-deps` internally elevates part of that tree to root via sudo to run apt-get, so when the action's 10-minute timeout fires, the kill fails with EPERM (cross-UID kill) and throws unhandled instead of retrying. The step died on attempt 1 and never reached attempt 2 or 3. Replaced all 10 occurrences with a plain bash retry loop using coreutils `timeout`, which signals its own direct child and isn't subject to the EPERM bug class. Matches the existing bash retry-loop idiom already used in codecov-upload.yml and quality-checks.yml.
…nerated id
frontend/src/components/ui/Input.tsx rendered <label htmlFor={props.id}>
next to <input {...props} />, but 115 of 118 call sites never pass an id
prop, so the label was never programmatically associated with its
control (WCAG 1.3.1/3.3.2). Browsers fall back to the placeholder text
as the accessible name instead, which also broke Playwright's
getByLabel() matching against these fields.
Auto-generate a stable id via React.useId() when the caller doesn't
supply one, so every <Input label=.../> usage gets a properly
associated label/control pair without changing behavior for call sites
that already pass an explicit id.
…07-28 refactor gotoTolerant/reloadTolerant in tests/utils/wait-helpers.ts used waitUntil: 'commit', silently downgraded from waitUntil: 'domcontentloaded' during a 2026-07-28 refactor (7503c01) that extracted these helpers from navigateToLogin()'s previously RCA-verified-working inline implementation (d537476). 'commit' fires on Firefox's earliest possible navigation signal, which Firefox can fail to emit at all for some goto()/reload() calls -- leaving Playwright's frame-navigation tracker believing a navigation is still pending and blocking the *next* locator-based wait on the page for a second full timeout, even though the page had already rendered correctly. Five subsequent fix attempts patched around this symptom without tracing it back to the regression. Trace-level evidence (docs/plans/current_spec.md) confirms this exact mechanism caused PR #1259's "E2E Firefox (Shard 4/4)" job to fail on tests/settings/user-lifecycle.spec.ts STEP 4: reloadTolerant() burned its full 15s timeout with a swallowed TimeoutError, then the following expect(emailInput).toBeVisible() burned another full 15s blocked on the phantom pending navigation without ever querying the DOM. Restores 'domcontentloaded', the value proven correct by the original 2026-07-26 RCA and already used by 100+ other call sites in this suite. Verified with 3x repeat-each on the target spec (zero flakes, was 3/3 failures before) plus blast-radius checks on every other consumer of these helpers (user-management.spec.ts full file, long-running-operations.spec.ts grep-scoped) -- all pass.
This was referenced Aug 19, 2026
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
2 participants
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
apt-getinsidenpx playwright install --with-deps <browser>can silently hang against a flaky Azure apt mirror with no timeout of its own, burning the job's full 60-minutetimeout-minutesand surfacing as a misleadingcancelledrun instead of a clear, actionable failure (observed on run 32068948141 —E2E Chromium (Security Enforcement), triggered on release-please's PR chore(main): release 0.37.0 #1258 againstmain).e2e-tests-split.ymlwith a bashtimeout-based retry loop (10 min/attempt, 3 attempts, 15s backoff) — an earlier attempt usednick-fields/retry, but that action crashes withError: kill EPERMwhen it tries to kill a process tree thatplaywright install --with-depshas partially elevated to root via sudo, so it never actually retried. The bashtimeoutcommand signals its own direct child and avoids that cross-UID kill problem entirely.timeout-minutes: 60and theconcurrency: cancel-in-progress: trueblock are left untouched — those are correct as-is.frontend/src/components/ui/Input.tsx: its<label htmlFor>was never programmatically associated with its<input>unless the caller passed an explicitid— 115 of 118 call sites app-wide never do.Input.tsxnow auto-generates a stable id viaReact.useId()when one isn't supplied. Note: generated ids contain colons (e.g.:r0:), so a naivepage.locator('#' + id)against one of these auto-generated ids won't work as a raw CSS selector — no current call site does this, but worth knowing.tests/settings/user-lifecycle.spec.ts(this is the seventh fix attempt at this exact test's Firefox flakiness — see commit message for full trace-level RCA): a 2026-07-28 refactor (7503c01a) silently downgradedgotoTolerant/reloadTolerant(tests/utils/wait-helpers.ts) fromwaitUntil: 'domcontentloaded'towaitUntil: 'commit'. Firefox can fail to emit a trackablecommitevent at all, which leaves Playwright's frame-navigation tracker believing a navigation is still pending — so the next locator-based assertion blocks for a second full timeout without ever querying the DOM, even though the page had already rendered correctly. Restored todomcontentloaded, the value proven correct by the original RCA two days earlier and already used by 100+ other call sites in this suite.Test plan
actionlintclean,lefthook run pre-commitpasses (incl. semgrep, 0 findings)e2e-tests-split.ymlhas an unfilteredpull_request:trigger, so this PR runs the full "E2E Tests" workflow directlyInput.tsx: 2 new unit tests (label associates via auto-id; explicit id still respected), all 18 existingInput.test.tsxcases pass,npm run type-checkclean,npm run buildcleanwait-helpers.ts: existing 30-case real-browserwait-helpers.spec.tssuite passes unmodified under Firefoxtests/settings/user-lifecycle.spec.ts --project=firefox --repeat-each=3— 3/3 clean passes, zero flakes (was 3/3 failures before)tests/settings/navigation-settle-regression.spec.ts --project=firefox— passestests/settings/user-management.spec.ts(full file, 32 passed/1 pre-existing skip) andtests/tasks/long-running-operations.spec.ts(grep-scoped) — both pass, zero regressionsscripts/frontend-test-coverage.sh→ PASS (90.81% lines vs 87% minimum)scripts/local-patch-report.sh→ 100% patch coverage, all scopes passcd backend && go build ./...clean (unaffected, gated per DoD anyway)