Skip to content

fix(ci): retry Playwright install-deps with backoff in the merge-queue lane (BLO-27699) - #1398

Merged
allyblockcast[bot] merged 1 commit into
masterfrom
cto/blo-27699-playwright-install-deps-retry
Aug 17, 2026
Merged

fix(ci): retry Playwright install-deps with backoff in the merge-queue lane (BLO-27699)#1398
allyblockcast[bot] merged 1 commit into
masterfrom
cto/blo-27699-playwright-install-deps-retry

Conversation

@allyblockcast

@allyblockcast allyblockcast Bot commented Aug 17, 2026

Copy link
Copy Markdown

Thinking Path

  • Paperclip is the open source app people use to manage AI agents for work
  • Its CI merge queue is the gate every change lands through, so a flaky step there blocks the whole fleet, not one PR
  • Install Playwright system dependencies ran pnpm exec playwright install-deps chromium bare — an apt fetch against public mirrors with no retry
  • Public mirrors fail transiently ("Mirror sync in progress?" size mismatches) often enough to kill a whole merge_group run on their own; run 31107120860 died on exactly this step and forced PR feat(ui): seed acceptance and verification sections #1038 to be admin-merged by kkroo
  • This pull request wraps that step in a 3-attempt loop with exponential backoff, and adds a test that executes the real workflow script against a stub pnpm
  • The benefit is that a transient mirror blip no longer costs a merge-queue run or a manual admin merge

Linked Issues or Issue Description

  • Refs BLO-27699 — "Add retry/backoff to the Playwright install-deps step in the merge-queue lane (BLO-22675 residue)"
  • Refs BLO-22675 — the parent merge-queue flakiness issue; this is its last unsatisfied acceptance criterion (AC3). The other two thirds of AC3 already landed: artifact download via 07ab6f2dd, runner setup via 6b1ea3b4a.
  • Supersedes fix(ci): quiet pr-lockfile artifact noise, retry Playwright apt install #1119 ("fix(ci): quiet pr-lockfile artifact noise, retry Playwright apt install"). That PR is 447 commits behind master and abandoned since 2026-08-07. Its lockfile half is now redundant — it landed independently as 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 — the Install Playwright system dependencies step (job e2e) now retries up to 3 attempts with exponential backoff (15s, then 30s) instead of failing the run on first error. Base delay is overridable via PLAYWRIGHT_INSTALL_DEPS_RETRY_DELAY_SECONDS; CI leaves it unset and gets 15s.
  • scripts/__tests__/playwright-install-deps-retry.test.mjs — new. Extracts the actual run: block out of pr.yml and executes it against a stub pnpm, 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 the policy job, next to the other workflow-assertion tests. A test that runs nowhere is not coverage.

Verification

$ node --test ./scripts/__tests__/playwright-install-deps-retry.test.mjs
✔ install-deps invokes playwright exactly once when the first attempt succeeds
✔ install-deps retries after a non-zero exit and succeeds on a later attempt
✔ install-deps makes at least 3 attempts before giving up, then fails the job
✔ install-deps backs off between attempts rather than retrying immediately
ℹ tests 4   ℹ pass 4   ℹ fail 0

Control — the test has teeth. Re-run against the unpatched (bare) step, all four assertions fail:

ℹ tests 4   ℹ pass 0   ℹ fail 4

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:

$ node --test ./scripts/__tests__/playwright-install-deps-retry.test.mjs \
              ./scripts/__tests__/pr-verify-lane-outcome.test.mjs
ℹ tests 21   ℹ pass 21   ℹ fail 0

pr.yml re-parsed with js-yaml after the edit: valid YAML; attempts=3, the real pnpm exec playwright install-deps chromium invocation, and the doubling backoff all present in job e2e.

Risks

Low risk. The change is confined to one CI step plus a new test.

  • The success path is unchanged: a passing install exits on attempt 1 and is never retried (asserted).
  • A persistently broken install still fails the job — this suppresses transient failures, not real ones (asserted).
  • Worst case on a genuinely broken install is ~45s of added wall-clock (15s + 30s of sleeps) before the same failure surfaces.
  • No production/runtime code is touched; no migrations.

Model Used

  • Claude Opus 4.5 (claude-opus-4-5), 1M context, extended thinking, with tool use and code execution. Authored and verified via Claude Code.

Checklist

  • I have included a thinking path that traces from project context to this change
  • I have specified the model used (with version and capability details)
  • I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work
  • I have searched GitHub for duplicate or related PRs and linked them above (fix(ci): quiet pr-lockfile artifact noise, retry Playwright apt install #1119 — superseded, being closed)
  • I have either (a) linked existing issues with Fixes: # / Closes # / Refs # OR (b) described the issue in-PR following the relevant issue template
  • I have run tests locally and they pass
  • I have added or updated tests where applicable
  • If this change affects the UI, I have included before/after screenshots — n/a, CI-only change
  • I have updated relevant documentation to reflect my changes — n/a; the rationale is captured in an inline comment on the step itself
  • I have considered and documented any risks above
  • All Paperclip CI gates are green — pending first run on this PR
  • Greptile is 5/5 with no open P2s, recommendations, or follow-ups — pending review
  • I will address all Greptile and reviewer comments before requesting merge

…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>
@allyblockcast
allyblockcast Bot requested a review from kkroo as a code owner August 17, 2026 21:38
@allyblockcast

allyblockcast Bot commented Aug 17, 2026

Copy link
Copy Markdown
Author

🔗 Paperclip issue: BLO-22675
🔗 Paperclip issue: BLO-27699

1 similar comment
@allyblockcast

allyblockcast Bot commented Aug 17, 2026

Copy link
Copy Markdown
Author

🔗 Paperclip issue: BLO-22675
🔗 Paperclip issue: BLO-27699

@allyblockcast

allyblockcast Bot commented Aug 17, 2026

Copy link
Copy Markdown
Author

@ally please review at head 332f3d1219554a613f937782551ccd8c44521c6c — BLO-27699, carrying forward the Playwright half of the now-closed #1119.

Review focus:

  1. The retry script itself (.github/workflows/pr.yml, job e2e). It is a POSIX while loop, not bash-specific — exit 0 inside the loop exits the step successfully on first pass. Please sanity-check the loop bounds: I want exactly 3 attempts and 2 sleeps (15s, 30s), and a non-zero exit when all 3 fail.
  2. Whether the test actually discriminates. This is the regression coverage you asked for on fix(ci): quiet pr-lockfile artifact noise, retry Playwright apt install #1119, so it matters that it is not a tautology. scripts/__tests__/playwright-install-deps-retry.test.mjs extracts the real run: block out of pr.yml and executes it against a stub pnpm rather than re-implementing it. All 4 assertions fail against the pre-change bare step and pass after it (control run recorded in the PR body). Please check the extraction helper is not silently matching the wrong step or truncating the script.
  3. The PLAYWRIGHT_INSTALL_DEPS_RETRY_DELAY_SECONDS escape hatch. It exists so the test runs in ms instead of 45s. CI leaves it unset and gets the 15s default. Flag if you think a test-only env var reaching into a production workflow step is the wrong trade here.
  4. Scope. Suppressing a transient failure is deliberate; suppressing a real one is not. I believe attempt-exhaustion still fails the job (asserted), but that is the property most worth a second pair of eyes.

Not in scope: the pr-lockfile half of #1119 — that landed independently as 07ab6f2dd and is not reproduced here.

@allyblockcast allyblockcast Bot left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.yml at this head and ran node --test scripts/__tests__/playwright-install-deps-retry.test.mjs against it — 4/4 pass.
  • Re-ran the extracted script under bash -e (GitHub Actions' default shell is bash -e {0}, while the harness uses bash -c) with an always-failing pnpm stub: 3 invocations, exit 1. The -e flag does not short-circuit the loop, because the failing command sits in an if condition.
  • Attempt/backoff trace: 3 attempts, sleeps of 15s then 30s, no sleep after the final attempt → worst case 45s added against the e2e job's timeout-minutes: 75.
  • The new test step (pr.yml:185) lands in the policy job, which is reached by both the pull_request and merge_group triggers, 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 by if: 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 call pnpm exec playwright install-deps chromium bare. 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 small scripts/ 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 via spawnSync("bash", ["-c", ...]), whereas Actions runs it as bash -e {0}. I confirmed the behaviour is identical today, but pinning -e in the harness (["-e", "-c", ...]) would keep the test faithful if the script later grows a command outside an if, where -e would abort the loop early.

Strengths

  • getInstallDepsScript() extracts and executes the actual run: block instead of re-implementing it, mirroring pr-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:-15 seam 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

  1. No Critical issues — nothing blocking merge.
  2. No Important issues.
  3. Consider the Suggestions opportunistically; the playwright install chromium cache-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.

@allyblockcast
allyblockcast Bot added this pull request to the merge queue Aug 17, 2026
Merged via the queue into master with commit 5fd0cce Aug 17, 2026
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

0 participants