ci(web-e2e): cache Playwright browsers + timeout/retry the install (fixes #1167)#1306
Conversation
…ixes #1167) The Web E2E smoke job ran `playwright install --with-deps chromium` on every run with no cache, no timeout, and no retry — so a stalled CDN download for the ~100MB chromium binary hung the step ~10min until the job-level orphan-kill fired, a recurring false-red that cost a manual `gh run rerun --failed` each hit. - Cache `~/.cache/ms-playwright` keyed on the resolved Playwright version; on a hit skip the binary download and run only the fast apt system-deps (`install-deps`). - Hard-cap each attempt with `timeout 240` and retry up to 3× so a stalled download fails fast and self-heals instead of hanging to the orphan-kill; step capped at `timeout-minutes: 8`. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
👀 Quinn is reviewing — verdict (PASS / WARN / FAIL) + findings to follow. |
WalkthroughThe ChangesPlaywright CI Install Hardening
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![protoquinn[bot]](https://avatars.githubusercontent.com/in/3282126?s=60&v=4){kind=small}
There was a problem hiding this comment.
QA Audit — PR #1306 | ci(web-e2e): cache Playwright browsers + timeout/retry the install (fixes #1167)
VERDICT: WARN (non-blocking — CI not yet terminal)
CI Status
- No check runs registered yet for head db210c1. Likely queued — this is a Gap, not a failure.
Diff Review
- Single file:
.github/workflows/checks.yml— no app code touched. - Replaces bare
playwright install --with-deps chromiumwith version-keyed cache (actions/cache@v4) + timeout/retry loop (3×, 240s cap, 8-min step limit). - Cache-hit path correctly skips binary download and runs apt system-deps only.
Observations
- GAP: CI checks not yet visible to reviewer — re-review after checks complete to confirm the workflow exercises the new code path.
- LOW: clawpatch structural review skipped (repo not in project registry) — diff is single-file and CI-only, low risk.
- The
node -p "require('@playwright/test/package.json').version"resolution from repo root relies on npm workspace hoisting — reliable on this project, no concern.
— Quinn, QA Engineer
|
Submitted COMMENT review on |
There was a problem hiding this comment.
CI terminal-green, no blockers on prior review — auto-approving on green (#748).
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/checks.yml:
- Line 169: The timeout-minutes value of 8 is insufficient to allow all retry
attempts to complete. In the checks.yml workflow file, increase the
timeout-minutes value from 8 to at least 15 to accommodate the worst-case
scenario of 3 retry attempts at 240 seconds each plus sleep intervals between
retries. This ensures the step timeout does not prematurely terminate before all
retry attempts and the explicit error handling can execute properly.
- Line 159: In the GitHub Actions workflow checks file, the `actions/cache@v4`
action reference at the specified location uses a mutable tag instead of a
pinned commit hash. Visit the actions/cache releases page to find the latest v4
release commit hash, then update the `uses: actions/cache@v4` line to reference
the specific commit hash in the format `uses: actions/cache@<commit-hash>` to
ensure supply-chain security and prevent the action from being modified by
upstream changes or potential attacks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 49304ae3-fc8c-4493-be14-9d244b095742
📒 Files selected for processing (1)
.github/workflows/checks.yml
| run: echo "version=$(node -p "require('@playwright/test/package.json').version")" >> "$GITHUB_OUTPUT" | ||
| - name: Cache Playwright browsers | ||
| id: pw-cache | ||
| uses: actions/cache@v4 |
There was a problem hiding this comment.
Pin the cache action to a commit hash for supply-chain security.
The actions/cache@v4 reference should be pinned to a specific commit hash rather than a mutable tag, per the blanket policy flagged by static analysis. Mutable tags can be modified by attackers who compromise the upstream action repository.
🔒 Proposed fix to pin the action
Visit the actions/cache releases page to find the latest v4 commit hash, then update:
- uses: actions/cache@v4
+ uses: actions/cache@0c45773b623bea8c8e75f6c82b208c3cf94ea4f9 # v4.2.0📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| uses: actions/cache@v4 | |
| uses: actions/cache@0c45773b623bea8c8e75f6c82b208c3cf94ea4f9 # v4.2.0 |
🧰 Tools
🪛 zizmor (1.26.1)
[error] 159-159: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/checks.yml at line 159, In the GitHub Actions workflow
checks file, the `actions/cache@v4` action reference at the specified location
uses a mutable tag instead of a pinned commit hash. Visit the actions/cache
releases page to find the latest v4 release commit hash, then update the `uses:
actions/cache@v4` line to reference the specific commit hash in the format
`uses: actions/cache@<commit-hash>` to ensure supply-chain security and prevent
the action from being modified by upstream changes or potential attacks.
Source: Linters/SAST tools
| # on the Playwright version (skips the download entirely on a hit — only the | ||
| # fast apt system-deps run then), and hard-cap each attempt with `timeout` so | ||
| # a stalled download fails fast and retries instead of hanging the whole job. | ||
| timeout-minutes: 8 |
There was a problem hiding this comment.
Increase the step timeout to allow all retry attempts to complete.
The step timeout of 8 minutes (480 seconds) is insufficient to accommodate 3 full retry attempts:
- Worst case: 3 attempts × 240s + 2 sleeps × 5s = 730 seconds (~12.2 minutes)
- Current step timeout: 8 minutes = 480 seconds
The step timeout will fire before all retries can complete, causing the workflow to fail mid-retry without reaching the explicit error message on line 183. This defeats the self-healing retry mechanism.
⏱️ Proposed fix to prevent premature timeout
- timeout-minutes: 8
+ timeout-minutes: 15This allows all 3 attempts to complete with buffer for system overhead.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| timeout-minutes: 8 | |
| timeout-minutes: 15 |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/checks.yml at line 169, The timeout-minutes value of 8 is
insufficient to allow all retry attempts to complete. In the checks.yml workflow
file, increase the timeout-minutes value from 8 to at least 15 to accommodate
the worst-case scenario of 3 retry attempts at 240 seconds each plus sleep
intervals between retries. This ensures the step timeout does not prematurely
terminate before all retry attempts and the explicit error handling can execute
properly.
1 participant
Closes #1167.
The Web E2E smoke job ran
playwright install --with-deps chromiumon every run with no cache, no timeout, and no retry — so a stalled CDN download for the ~100MB chromium binary hung the step ~10 min until the job-level orphan-kill fired (Terminate orphan process … playwright install). A recurring false-red that masked real E2E signal and cost a manualgh run rerun --failedeach hit (recurred on #1158/#1161/#1164 this week).Fix (stacks suggestions 1 + 2 from the issue)
~/.cache/ms-playwrightkeyed on the resolved Playwright version (@playwright/test1.60.0). On a cache hit the slow binary download is skipped entirely — only the fast apt system-deps run (playwright install-deps chromium).timeout 240and retried up to 3×, so a stalled download fails fast and self-heals instead of hanging to the ~10 min orphan-kill. Step capped attimeout-minutes: 8.Verification
YAML validated locally. The behavior is CI-only — this PR's own run is the first exercise: it's a cache miss (full
--with-depsinstall, populates the cache), and subsequent runs on the same Playwright version hit the cache. No app code touched.🤖 Generated with Claude Code
Summary by CodeRabbit