fix(ci): retry Playwright install-deps with backoff in the merge-queue lane (BLO-27699) - #1398
Merged
allyblockcast[bot] merged 1 commit intoAug 17, 2026
Conversation
…e lane `Install Playwright system dependencies` ran `pnpm exec playwright install-deps chromium` bare. That is an apt fetch against public mirrors, which fails transiently often enough to take out a whole merge_group run on its own — run 31107120860 died on exactly this step and forced PR #1038 to be admin-merged by kkroo. Wrap it in a 3-attempt loop with exponential backoff (15s, then 30s), so a transient mirror failure no longer fails the enclosing run. Adds scripts/__tests__/playwright-install-deps-retry.test.mjs, which extracts the real `run:` block out of pr.yml and executes it against a stub `pnpm` rather than re-implementing it — so it asserts the wrapper genuinely re-invokes on a non-zero exit, caps at 3 attempts, and still fails the job when every attempt fails. This is the regression coverage Ally asked for on #1119. All 4 assertions fail against the pre-change bare step and pass after it. The retry delay is overridable via PLAYWRIGHT_INSTALL_DEPS_RETRY_DELAY_SECONDS so the test runs fast; CI leaves it unset and gets the 15s default. Carries forward the Playwright half of #1119, whose lockfile half already landed independently as 07ab6f2. #1119 is superseded. Refs BLO-27699 Refs BLO-22675 Co-Authored-By: Claude <noreply@anthropic.com>
Author
1 similar comment
Author
13 tasks
Author
|
@ally please review at head Review focus:
Not in scope: the |
Author
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 332f3d1
Looks good. The retry wrapper is correct, the test exercises the real workflow script rather than a copy of it, and the failure path still fails the job.
Verified during this review, not inferred:
- Extracted the
run:block from.github/workflows/pr.ymlat this head and rannode --test scripts/__tests__/playwright-install-deps-retry.test.mjsagainst it — 4/4 pass. - Re-ran the extracted script under
bash -e(GitHub Actions' default shell isbash -e {0}, while the harness usesbash -c) with an always-failingpnpmstub: 3 invocations, exit 1. The-eflag does not short-circuit the loop, because the failing command sits in anifcondition. - Attempt/backoff trace: 3 attempts, sleeps of 15s then 30s, no sleep after the final attempt → worst case 45s added against the
e2ejob'stimeout-minutes: 75. - The new test step (
pr.yml:185) lands in thepolicyjob, which is reached by both thepull_requestandmerge_grouptriggers, so the guard runs in the lane it protects.
Critical Issues (0)
Important Issues (0)
Suggestions (3)
- [native-codex]
.github/workflows/pr.yml:940— the very next step,pnpm exec playwright install chromium, is the same class of transient network dependency (CDN browser download) and is still unretried. It is guarded byif: steps.playwright_cache.outputs.cache-hit != 'true', i.e. it only runs on the cold-cache path — precisely the run that is already paying full setup cost and is most expensive to lose. Worth wrapping with the same loop in a follow-up. - [gstack/review]
.github/workflows/e2e.yml:56,.github/workflows/release-smoke.yml:85— both still callpnpm exec playwright install-deps chromiumbare. Out of scope for a merge-queue-lane fix, so not a blocker, but three copies of this invocation with one hardened is a divergence that will drift. A composite action or a smallscripts/wrapper shared by all three call sites would keep them honest. - [pr-review-toolkit/tests]
scripts/__tests__/playwright-install-deps-retry.test.mjs:63— the harness runs the extracted script viaspawnSync("bash", ["-c", ...]), whereas Actions runs it asbash -e {0}. I confirmed the behaviour is identical today, but pinning-ein the harness (["-e", "-c", ...]) would keep the test faithful if the script later grows a command outside anif, where-ewould abort the loop early.
Strengths
getInstallDepsScript()extracts and executes the actualrun:block instead of re-implementing it, mirroringpr-verify-lane-outcome.test.mjs. This is the right pattern: the test cannot drift from the workflow, and it fails loudly if the step is restructured.- Test coverage hits the cases that matter rather than padding: success-on-first-attempt asserts no retry (
invocations === 1) and pins the argv; transient-then-success asserts genuine re-invocation (the #1119 regression, where a wrapper could swallow the exit code instead of retrying); persistent failure asserts the job still fails; and a fourth test pins the backoff shape (sleep, doubling, 15s CI default) which the stubbed-delay tests deliberately cannot observe. - Failing closed after 3 attempts is the right call — a persistent apt breakage still surfaces instead of being masked.
- The
PLAYWRIGHT_INSTALL_DEPS_RETRY_DELAY_SECONDS:-15seam keeps the suite fast without weakening CI, and the fourth test asserts the production default so the override cannot silently become the real value. - The comment states the concrete evidence — the failing run id and the resulting admin-merge of #1038 — so the next reader can judge whether the retry is still earned.
Recommended Action
- No Critical issues — nothing blocking merge.
- No Important issues.
- Consider the Suggestions opportunistically; the
playwright install chromiumcache-miss step (Suggestion 1) is the highest-value follow-up, since it sits in the same job and the same lane as the failure this PR fixes.
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
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.
Thinking Path
Linked Issues or Issue Description
install-depsstep in the merge-queue lane (BLO-22675 residue)"07ab6f2dd, runner setup via6b1ea3b4a.07ab6f2dd. Only its Playwright half was still wanted, and this PR carries it forward on a fresh branch off master. fix(ci): quiet pr-lockfile artifact noise, retry Playwright apt install #1119 is being closed.What Changed
.github/workflows/pr.yml— theInstall Playwright system dependenciesstep (jobe2e) now retries up to 3 attempts with exponential backoff (15s, then 30s) instead of failing the run on first error. Base delay is overridable viaPLAYWRIGHT_INSTALL_DEPS_RETRY_DELAY_SECONDS; CI leaves it unset and gets 15s.scripts/__tests__/playwright-install-deps-retry.test.mjs— new. Extracts the actualrun:block out ofpr.ymland executes it against a stubpnpm, rather than re-implementing the logic. Asserts the wrapper re-invokes on non-zero exit, caps at 3 attempts, still fails the job when all attempts fail, and backs off between attempts..github/workflows/pr.yml— wires that test into thepolicyjob, next to the other workflow-assertion tests. A test that runs nowhere is not coverage.Verification
Control — the test has teeth. Re-run against the unpatched (bare) step, all four assertions fail:
So the suite genuinely discriminates the fixed step from the broken one; it is not a tautology that would pass either way.
Run alongside the neighbouring workflow test to confirm nothing regressed:
pr.ymlre-parsed withjs-yamlafter the edit: valid YAML;attempts=3, the realpnpm exec playwright install-deps chromiuminvocation, and the doubling backoff all present in jobe2e.Risks
Low risk. The change is confined to one CI step plus a new test.
Model Used
claude-opus-4-5), 1M context, extended thinking, with tool use and code execution. Authored and verified via Claude Code.Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue template