fix(ci): avoid apt on self-hosted Playwright smoke - #6865
Conversation
|
Thanks for the PR! (Re-run — the previous review was against Template looks good ✓ Problem: Observed bug with evidence. CI run 29302774843 failed with apt lock contention ( Direction: Aligned. This is a CI stability fix for a real, observed failure. Linux packages belong in the runner image, not in a per-job Size: Not applicable — CI-only change ( Approach: The approach has been reworked since the first pass. Previously the fix pinned One addition beyond the minimal fix: a Moving on to code review. 🔍 中文说明感谢贡献!(Re-run — 上次审查基于 模板完整 ✓ 问题:已观测到的 bug,有证据。 CI run 29302774843 在共享 self-hosted ECS runner 上因 方向:对齐。这是针对已观测到的 CI 失败的稳定性修复。Linux 系统依赖应固化在 runner 镜像中,不应每次 Job 都跑 规模:不适用——仅 CI 改动( 方案:方案相比上次审查已大幅调整。之前是将 在最小修复之外增加了一个 进入代码审查 🔍 — Qwen Code · qwen3.7-max Reviewed at |
Code ReviewIndependent proposal before reading the diff: to fix the apt lock race on self-hosted runners without abandoning the dynamic runner routing, split the Playwright install into two conditional steps — The PR matches this closely. The key changes:
No correctness bugs, no security concerns, no regressions. The conditional expressions are clean and mutually exclusive ( One minor observation: the test name "does not install Linux packages on self-hosted Playwright runners" is slightly misleading — the test asserts the existence of both install variants, not that self-hosted runners don't get TestingThis is a CI workflow change — the meaningful verification is the CI run itself, not a tmux CLI session. All checks on the PR pass: Key results:
Note: This fork PR routes to GitHub-hosted runners, so the 中文说明代码审查读 diff 前的独立方案:在不放弃动态 runner 路由的前提下修复 apt 锁竞态,将 Playwright 安装拆为两个条件步骤——hosted 用 PR 与此高度一致。关键改动:
无正确性 bug,无安全隐患,无回归。条件表达式干净且互斥。已有的 self-hosted 设置步骤(Node 回退、npm 缓存)保持不变。 一个小观察:测试名 "does not install Linux packages on self-hosted Playwright runners" 略有误导——测试断言的是两种安装变体的存在,而不是 self-hosted runner 不使用 测试这是 CI workflow 改动——有效验证来自 CI 运行本身。PR 上的所有检查通过:Test Job(30m6s)包含更新的测试套件全部通过;Web-shell E2E Smoke(4m53s)在 hosted runner 上成功运行;precheck-pr(actionlint + yamllint)YAML 语法有效。 此 fork PR 路由到 GitHub-hosted runner,因此验证了 — Qwen Code · qwen3.7-max Reviewed at |
|
Confidence: 5/5 — Clean, targeted fix for an observed CI failure; every conditional is correct and well-tested. The approach has been reworked since the first review (which pinned to The All CI checks pass. The test correctly asserts the existence of both install variants. The only caveat (acknowledged by the author) is that the self-hosted path needs the ECS runner image to already include Playwright's system deps — that's an out-of-repo dependency, not a code concern. Approving. ✅ 中文说明信心:5/5 — 针对已观测到的 CI 失败的干净、精准修复;每个条件表达式正确且经过测试。 方案自上次审查后已调整(之前固定到
所有 CI 检查通过。测试正确断言两种安装变体的存在。唯一的注意事项(作者已说明)是 self-hosted 路径需要 ECS runner 镜像已包含 Playwright 系统依赖——这是仓库外的依赖,不是代码问题。 已批准。✅ — Qwen Code · qwen3.7-max Reviewed at |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. Not reviewed: chunk 1 — no agent reported covering these; nobody read them.
— qwen3.7-max via Qwen Code /review
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. Not reviewed: chunk 1 — no agent reported covering these; nobody read them.
— qwen3.7-max via Qwen Code /review
|
Added one more pre-merge validation pass for the updated workflow on a real self-hosted ECS runner. Manual dispatch run: https://github.com/QwenLM/qwen-code/actions/runs/29322239672 What this validated:
This run used the temporary branch |
yiliang114
left a comment
There was a problem hiding this comment.
Independent review — clean and correct. Verified the routing chain: workflow_dispatch makes test run (its if: github.event_name != 'push' holds) and ci_profile defaults to full (the classifier only downgrades on same-repo PRs), so web_shell_e2e_smoke's needs.test.outputs.ci_profile == 'full' is satisfied and the smoke job actually runs under dispatch. The Playwright split is the right fix for the apt-lock flake — --with-deps on a shared self-hosted host racing apt-get is exactly the failure in run 29302774843.
One minor nit (non-blocking): the self-hosted install step keys on runner.environment == 'self-hosted'. If an enterprise runner ever appears, it matches neither branch and Chromium isn't installed (smoke fails with no browser). runner.environment != 'github-hosted' for the no-deps step would be more future-proof, but QwenLM on github.com won't hit enterprise runners, so it's purely defensive.
LGTM.
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not reviewed: chunk 1 — no agent reported covering these; nobody read them.
— qwen3.7-max via Qwen Code /review
|
Automated follow-up summary:
Both threads are being resolved. |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. Not reviewed: chunk 1 — no agent reported covering these; nobody read them.
— qwen3.7-max via Qwen Code /review
|
Automated follow-up summary for the latest review:\n\n- No code change for stronger YAML-structure assertions around the hosted/self-hosted Playwright steps. The current workflow behavior is not reported broken; this is future regression-hardening for the test.\n- No code change for adding workflow-dispatch runner-selection coverage. That branch is not reported broken, and covering the full dispatch input/fallback matrix would expand the test surface beyond this bounded apt-lock fix.\n\nBoth items are Suggestions rather than current behavior or CI defects. The two threads are being resolved under the minimal-scope policy. |
|
@qwen-code /triage |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
What this PR does
Keeps the web-shell browser smoke on the selected Linux runner. GitHub-hosted runners continue installing Chromium with its Linux packages, while self-hosted ECS runners install only the browser and do not invoke
apt-getduring the job.Why it's needed
Playwright's
--with-depsoption invokesapt-get. CI run 29302774843 failed before the browser test started because another process held the shared ECS runner's apt lock, then passed unchanged on another runner. Linux packages belong in the ECS runner image; the CI job should not mutate the host package database on every run.Reviewer Test Plan
How to verify
Confirm the smoke job still follows the existing selected-runner routing. On GitHub-hosted runners it should run
npx playwright install --with-deps chromium; on self-hosted runners it should runnpx playwright install chromiumand never invoke apt. The ECS image must already provide Playwright's Linux dependencies.Evidence (Before & After)
Before: CI attempt 1 failed in the Playwright install step with
/var/lib/apt/lists/lockheld by another process. After: the focused workflow test passes 5/5, and actionlint and yamllint pass.Tested on
Environment (optional)
Local workflow validation on macOS. This fork PR routes to GitHub-hosted CI, so the ECS branch still requires validation after the runner image rollout.
Risk & Scope
Linked Issues
Related to #6856
Does not close #6856 until the self-hosted runner image rollout is complete and verified.
中文说明
这个 PR 做了什么
web-shell 浏览器冒烟测试继续使用现有的 selected Linux runner 路由。GitHub-hosted runner 仍安装 Chromium 和 Linux 系统依赖;self-hosted ECS runner 只安装浏览器,不再在任务执行期间调用
apt-get。为什么需要
Playwright 的
--with-deps会调用apt-get。CI run 29302774843 在浏览器测试开始前失败,因为共享 ECS runner 上另一个进程持有 apt 锁;代码不变换 runner 重跑后通过。Linux 系统依赖应固化在 ECS runner 镜像中,CI Job 不应每次运行都修改宿主机包数据库。Reviewer 测试计划
如何验证
确认 smoke Job 仍跟随现有 selected-runner 路由。GitHub-hosted runner 应执行
npx playwright install --with-deps chromium;self-hosted runner 应执行npx playwright install chromium,且不调用 apt。ECS 镜像必须已经提供 Playwright 所需的 Linux 系统依赖。证据(Before & After)
Before:CI attempt 1 在 Playwright 安装阶段因另一个进程持有
/var/lib/apt/lists/lock而失败。After:聚焦 workflow 测试 5/5 通过,actionlint 和 yamllint 通过。测试平台
环境(可选)
在 macOS 上完成本地 workflow 验证。fork PR 会路由到 GitHub-hosted CI,因此 ECS 分支仍需在 runner 镜像发布后验证。
风险与范围
关联 Issue
关联 #6856
在 self-hosted runner 镜像完成发布并验证前,这个 PR 不应关闭 #6856。