ci: run Linux E2E with Vitest forks - #11290
Conversation
yiliang114
changed the title
|
Initial paired result — one sample only, so treat the timing as directional rather than a stable benchmark:
The baseline and experiment each covered the same 76 files and 464 tests per sandbox, and both passed. Baseline: https://github.com/QwenLM/qwen-code/actions/runs/34094994389. Experiment: https://github.com/QwenLM/qwen-code/actions/runs/34101867981. Actual Linux jobs:
Follow-up sample from https://github.com/QwenLM/qwen-code/actions/runs/34107488145:
Both follow-up Linux jobs passed on their first attempt and again covered 76 files and 464 tests per sandbox. The run later reran an unrelated macOS shard failure; that does not affect the Linux measurements above. The second sample is slower than the first, but still reduces the Linux critical path by 32% and Linux runner time by 71% versus the baseline. Two samples point in the same direction; shared-pool timing is still noisy, so normal follow-up runs should be observed before treating the exact percentages as stable. |
chiga0
left a comment
chiga0
left a comment
There was a problem hiding this comment.
No blocking findings.
Approval blockers: none.
Tier: Scan — CI-only change (+18/-9 across 2 files).
What I checked
- Shard matrix (
1/3,2/3,3/3→1/1): removing the three-way split from the matrix while keeping the dimension is consistent;1/1means "this runner gets all test files." --poolOptions.forks.maxForks=3on both thesandbox:dockerandsandbox:nonelegs: moves the three-way concurrency from three separate runners into three Vitest fork workers on one runner. Both occurrences are present (confirmed by the workflow test that assertsmatch(/--poolOptions\.forks\.maxForks=3/g).toHaveLength(2)).- macOS jobs and OpenTUI job: untouched by the diff, consistent with the stated scope.
- Workflow structure test (
scripts/tests/e2e-workflow.test.js): the two new assertions check the matrix shard list and the fork flag count; the existing sandbox:none command-string check is updated to match the new argument order. No vacuity risk — both assertions would fail against the base branch. - No AI-review ban found in CONTRIBUTING.md.
Unreviewed dimensions
Execution not run — no local workspace. The PR description links a completed E2E run (34101867981) showing 76 files / 464 tests per sandbox with all jobs passing.
Reviewed with AI assistance.
|
Thanks for the PR! Template looks good ✓ — every required heading is filled in with real content, including a Before/After evidence table, the risk tradeoffs, and a full Chinese translation. Problem: observed and measured, not theoretical. Rather than take the table's word for it, I pulled both cited runs through the API and confirmed them:
macOS (2 shards) and the OpenTUI job are identical in both runs, so it is a like-for-like comparison. Your figures check out exactly, including the 19m00s Linux critical path. Direction: aligned. Self-hosted pool occupancy is the recurring pain this workflow keeps documenting — the concurrency block already explains that it deliberately does not cancel in-progress runs on Size: not applicable — no core paths ( Approach: the scope feels right — collapsing the matrix dimension and pushing concurrency into Vitest is the minimal way to get this, and the diff carries no drive-by edits. Two genuine questions before the code review, neither a blocker:
Risk: no elevated risk signals — neither changed file matches the high-risk path set, and this is CI configuration with a structure test rather than runtime code. The real tradeoff is the one you already named: three forks now share CPU, memory, ports and one Docker daemon inside a single pool runner, which is the resource multiplication Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ —— 所有必填标题都有实质内容，包含 Before/After 证据表、风险取舍说明和完整中文翻译。 问题： 是已观测且已度量的，不是理论性问题。我没有直接采信表格，而是通过 API 拉取了你引用的两次 run 并独立核对：
两次 run 的 macOS（2 个 shard）和 OpenTUI job 完全一致，因此是同条件对比。你的数据完全对得上，包括 19分00秒 的 Linux 关键路径。 方向： 对齐。self-hosted runner 池的占用正是这个 workflow 反复记录的痛点 —— concurrency 配置里已经说明，之所以刻意不在 规模： 不适用 —— 未触及核心路径（ 方案： 范围合理 —— 收掉 matrix 维度、把并发下沉到 Vitest 是达成目标的最小改法，diff 里也没有夹带无关改动。进入代码审查前有两个真问题，都不是阻塞项：
风险： 无升级风险信号 —— 两个改动文件都不匹配高风险路径集合，且这是带结构测试的 CI 配置，不是运行时代码。真正的取舍是你已经点出的那一条：三个 fork 现在会在单个池 runner 内共享 CPU、内存、端口和同一个 Docker daemon，而这正是 进入代码审查 🔍 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
Maintainer verification — built a local harness and re-derived the CI numbersReviewed at Verdict: the change does what it claims, the evidence holds up, and I'm happy to merge it. Four non-blocking tidy-ups below. 1. Does
|
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Reviewed HEAD e302892. No code defect found in the two-file Linux-concurrency change: the 1/1 shard matrix and both maxForks=3 invocations are covered by the workflow test, while macOS remains unchanged. I cannot approve while E2E run 34107488145 is red: macOS shard 2/2 failed in Run E2E tests. The workflow is still running, so the failed-job log is not yet available. Please resolve or rerun that check and request re-review once it is green.
Code reviewMy independent proposal first. Reading only the title and the "Why it's needed" section, my plan was the same shape: collapse the shard matrix to one job per sandbox and cap Vitest forks at three so total test concurrency stays constant while setup is paid once. Two things I would have added that this diff doesn't, and both are the findings below — delete the now-vestigial shard dimension rather than pinning it to Nothing here is a blocker. Two findings worth fixing in this PR while you're in the file, plus three things I checked and cleared. 1. The retry budget gate is still derived from a shard, but it now retries a full suiteThe
That is exactly the edit this PR made — to the work unit rather than the timeout, which is the half the test pins. The arithmetic isn't broken today: across the two runs I read, the full suite took 818s and 886s wall on the 2.
|
Linux leg on e302892 |
Files | Tests | Wall | Cumulative | Ratio |
|---|---|---|---|---|---|
sandbox:none - shard 1/1 |
76 | 464 (442 passed, 22 skipped) | 886.58s | 2543.00s | 2.87× |
sandbox:docker - shard 1/1 |
76 | 464 (384 passed, 80 skipped) | 1072.35s | 3061.61s | 2.86× |
I also verified the benchmark independently rather than taking the PR table's word for it, pulling both cited runs, their job lists, and the Vitest summary blocks out of the job logs:
baseline 5fab39d (run 34094994389) |
after 54bb648 (run 34101867981) |
|
|---|---|---|
| Linux jobs | 6 — sandbox:none × 1/3, 2/3, 3/3 and sandbox:docker × 1/3, 2/3, 3/3 |
2 — sandbox:none - shard 1/1, sandbox:docker - shard 1/1 |
sandbox:none coverage |
26 + 26 + 24 files, 176 + 140 + 148 tests | 76 files, 464 tests |
| Shard/job wall clock | 659s / 747s / 1108s | 818s (none), 837s (docker) |
| Cumulative ÷ wall | 0.97× / 0.98× / 0.98× — one fork | 2.85× / 2.81× — three forks |
| Whole workflow | 41m40s | 24m27s |
Three things this settles. Coverage is identical — the three baseline shards sum to exactly 76 files and 464 tests, the same totals each single job now reports on both commits, so collapsing the matrix dropped nothing. The fork override really takes effect — baseline ratios of ~0.98 confirm the config's maxForks: 1 was in force, and ~2.86 confirms the CLI flag beats it. The imbalance claim is real — baseline shards ran 11m / 12.4m / 18.5m, so the critical path was set by an 18.5-minute tail that dynamic fork scheduling now absorbs. macOS (2 shards) and the OpenTUI job are unchanged across both runs, so the comparison is like-for-like.
The 54bb648..e302892 delta is one YAML comment and one test-name string — no functional change — so the benchmark run and the head run agree, as they should.
Full check state on the head commit:
| Check | Conclusion |
|---|---|
| E2E Tests / Build for E2E | success |
| E2E Test (Linux) - sandbox:none - shard 1/1 | success |
| E2E Test (Linux) - sandbox:docker - shard 1/1 | success |
| E2E Test - macOS - shard 1/2 | success |
| E2E Test - macOS - shard 2/2 | failure |
| E2E Interactive - OpenTUI renderer (bun) | success |
| web-shell Browser Regression | skipped |
| isolated-nightly | skipped |
| Qwen Code CI | not triggered |
| assign / authorize / label (bot orchestration) | success |
| triage / review-pr (bot orchestration) | in_progress |
Two honest caveats about that table. First, Qwen Triage Finalize will not refresh it: that workflow fires on Qwen Code CI, Qwen Autofix, SDK Java, SDK Python, Serve A/B and Web-shell Visuals, and deliberately excludes E2E Tests because it has no pull_request trigger. None of the listed workflows run on this PR, so what you see above is a point-in-time snapshot, not a table that will settle itself — the two bot-orchestration rows were still in flight when I read them.
Second, run 34107488145 concluded failure overall, and the macOS shard 2/2 red is not this PR's — I checked rather than assumed. The failing test is cli/acp-integration.test.ts > blocks write tools in plan mode (issue #1806), with Error: Internal error from the ACP response handler and ENOTEMPTY: directory not empty, rmdir '…/acp-plan-mode-enforcement/.qwen-home' on teardown, failing all three attempts. The diff cannot reach it: e2e-test-linux spans lines 100–426 of the workflow and e2e-test-macos starts at 427, both hunks are inside the Linux job, the macOS job has its own matrix.shard (1/2, 2/2) and its own run step, and it runs on GitHub-hosted macos-latest rather than the self-hosted pool — so it shares no changed line and no changed config with this PR. I'd stop there, except that macOS shard 2/2 was green in all six recent comparison runs I sampled (main ×4, the feat/opentui-batch10-parity-closeout dispatch, and shard 1/2 of the concurrent main run), so this is an intermittent macOS-lane flake that happened to land here rather than a standing red. Worth a re-run of that job; until then the E2E run reads red for a reason unrelated to this change, which is easy to misattribute later.
Not verified: the added scripts/tests/e2e-workflow.test.js assertions were never executed by CI on this PR. That suite runs via npm run test:scripts inside Qwen Code CI, which triggers only on pull_request to main/release/** and push to main — this PR's base is feat/e2e/fix-container-cleanup, so no lane here executes it. The author reports 30/30 locally; that is their claim, not CI evidence. I confirmed the three new/changed assertions hold by reading them against the head YAML, which is a static check, not an execution.
Sandboxed verification would settle the one gap static review can't: @qwen-code /verify — that the workflow-structure suite actually passes at e302892, since no CI lane on this PR runs it and my check of those assertions was read-only. The performance claim itself needs no lane; both runs above substantiate it on the reviewed commit. The untested worst case — both Linux legs colocated on one ECS host with six forks and the docker daemon between them — is not something /verify or /tmux can reach, since neither schedules Actions runners; the standing signal for that is the nightly E2E regression once this lands.
中文说明
代码审查
先说我自己的独立方案。 只看标题和「为什么需要」两节时，我的思路与之一致：把 shard matrix 收成每种 sandbox 一个 job，并将 Vitest fork 上限设为三，使测试级并发总量不变、而初始化只付一次。我会额外做两件事，也正是下面的两条发现：直接删掉已失效的 shard 维度而不是钉成 1/1，以及同步更新那份被本 PR 静默覆盖的 Vitest 配置。所以机制与我的方案一致，缺的是两处一致性收尾。
没有阻塞项。 两条建议趁改这个文件时一并处理，另有三项我核查后排除。
1. 重试预算门槛仍按「一个 shard」推导，但它现在重试的是整套用例
sandbox:none 那条 leg 的有界重试以 elapsed > 2100 为门槛，注释明确给出推导：3600s 减去 25 分钟预留（「实测最差的 shard 在共享宿主压力下约 21 分钟」）得到 2100s。而 run_shard 现在跑的是全部 76 个文件，预留需要覆盖的单位大了约三倍，「最差 shard 约 21 分钟」已不再描述任何真实对象。结构测试本身就知道这层耦合并写明了：改其一而不改其二，会在其他所有见证都为绿的情况下把重试预算算错。这正是本 PR 做的编辑 —— 改的是工作单位，而非被测试钉住的 timeout。今天的算术并未出错：我读到的两次 run 中，sandbox:none 整套用例墙钟分别为 818s 与 886s，因此即便 2100 + 886 ≈ 2986s 仍在 3600s 内。但这个预留现在靠巧合而非推导成立，而它要应对的恰恰是三个 fork 在单机上竞争、整套用例在宿主压力下运行的场景 —— 这个画像还没有人测过最差值，且观测到的波动已在扩大（docker leg 在两次 run 之间从 836s 变为 1072s）。建议按整套用例重新推导门槛并更新注释。
2. integration-tests/vitest.config.ts 现在记录的与 Linux 通道实际行为相反
该配置设 maxForks: isSelfHostedRunner ? 1 : 4，理由是「每台 ECS 宿主上跑着多个 Actions runner，让每个 E2E shard 只占一个子进程，避免并发 job 放大宿主负载、饿死延迟敏感的集成路径」。本 PR 在 Linux 通道上把它覆盖为三，且我从日志确认覆盖确实生效而非被忽略（见下方并行比）。因此该配置中 self-hosted 的 maxForks: 1 对这条通道已成死代码，其注释会误导任何想通过配置理解 E2E 并发的人。建议要么在配置里注明被覆盖，要么把 fork 数移进配置，只留一个真相来源。另外：该防护本就是为防止宿主负载放大而写，而每次 run 的 fork 总数仍是六 —— 但现在集中在两个 job 而非六个，因此若两条 leg 落到共享同一台 ECS 宿主的 runner 上，该宿主会同时承受六个 fork 加 docker leg 的容器。这正是注释警告的场景，而两次 run 都没有覆盖到它（基准 run 中两条 leg 落在不同宿主：actions-runner-21 与 actions-runner-hk3-14）。这不构成否决理由 —— 这就是你已点明的取舍 —— 但它是两次配对运行都留下的未测情形。
已核查并排除
- fork 间容器命名冲突。
E2E_CONTAINER_OWNER内嵌matrix.shard，因此三个 fork 现在共享同一 owner 字符串，两条 sandbox leg 也解析出同一值。安全：fix(ci): reap leaked E2E sandbox containers #11264 是以--label org.qwen-code.ci.owner=…施加的，清理步骤把它当作docker ps --filter选择器而非容器名，共享只意味着 job 级回收仍能回收该 job 启动的全部容器。 - 锁竞争反而改善。 结构测试记录了 run 33637097713：shard 1/3 持有按提交粒度的协调锁，shard 2/3 在其后超时（fix(ci): coordinate Docker sandbox workloads on shared ECS hosts #10605）。每次 run 由六个 job 变为两个，意味着该锁的并发持有者更少，因此本改动应当缩小而非放大这类失败。
- 新增测试确实钉住了改动。 它断言
matrix.shard等于['1/1']、--poolOptions.forks.maxForks=3恰好出现两次，且原有的精确命令断言在同一 diff 中被同步更新。我对照 head 的 YAML 静态核对了这三条：shard 为['1/1']，该 flag 在 docker leg 与run_shard内各出现一次。e2e-shard-retry.test.js不会被 matrix 改动打破 —— 它向脚本注入自己的字面量1/3而非读取 matrix，因此重试 bash 的执行验证保持不变。
测试证据
这是无人值守的 CI 运行，按门禁规则我没有构建或执行任何东西 —— 没有 npm、没有 vitest、没有 PR 代码。以下全部通过 GitHub API 读取。
head 提交自身的 E2E run 已经落定，且两条 Linux leg —— 本 diff 唯一触及的 job —— 在被审 SHA 上均为绿色。 run 34107488145（push，head e302892）在每条 leg 上都报告 76 文件 / 464 测试，累积与墙钟之比确认确实跑了三个 fork：sandbox:none 为 886.58s 墙钟 / 2543.00s 累积 = 2.87 倍，sandbox:docker 为 1072.35s / 3061.61s = 2.86 倍。
我也独立核对了基准数据，而非采信 PR 表格：拉取两次被引用的 run、它们的 job 列表，并从 job 日志中取出 Vitest 汇总块。基线三个 shard 合计恰为 76 文件 / 464 测试（26+26+24 文件，176+140+148 测试），与两个提交上单个 job 报告的总数一致，说明收掉 matrix 没有丢用例；基线「累积÷墙钟」约 0.98 证实配置里的 maxForks: 1 原本生效，改动后约 2.86 证实 CLI flag 覆盖了它；基线三个 shard 分别跑了 11 分 / 12.4 分 / 18.5 分，关键路径由 18.5 分钟的长尾决定，而动态 fork 调度把这条长尾吸收了。两次 run 的 macOS（2 shard）与 OpenTUI job 保持一致，因此是同条件对比。
54bb648..e302892 的差异只有一处 YAML 注释和一个测试名字符串，无功能变化，因此基准 run 与 head run 结果一致，符合预期。
关于表格有两点需要如实说明。其一，Qwen Triage Finalize 不会刷新它：该 workflow 只在 Qwen Code CI、Qwen Autofix、SDK Java、SDK Python、Serve A/B、Web-shell Visuals 完成时触发，并刻意排除了没有 pull_request 触发器的 E2E Tests；本 PR 上这些 workflow 一个都不跑，所以上表是时间点快照，不会自行落定 —— 我读取时那两行 bot 编排 job 仍在进行中。
其二，run 34107488145 整体结论为 failure，而 macOS shard 2/2 的红色不属于本 PR，这一点我是查过的而非假设。 失败用例是 cli/acp-integration.test.ts > blocks write tools in plan mode (issue #1806)，ACP 响应处理抛出 Error: Internal error，清理阶段报 ENOTEMPTY: directory not empty, rmdir '…/acp-plan-mode-enforcement/.qwen-home'，三次尝试全败。diff 触达不到它：workflow 中 e2e-test-linux 位于 100–426 行、e2e-test-macos 从 427 行开始，两处 hunk 都在 Linux job 内，macOS job 有自己的 matrix.shard（1/2、2/2）和自己的 run step，且跑在 GitHub 托管的 macos-latest 而非 self-hosted 池上 —— 与本 PR 既不共享改动行也不共享改动配置。我本可就此打住，但我采样的六次近期对照 run 中 macOS shard 2/2 全为绿色（main 四次、feat/opentui-batch10-parity-closeout 手动触发一次、以及并发 main run 的 shard 1/2），所以这是恰好落在这里的 macOS 通道偶发抖动，而非长期红色。建议重跑该 job；在此之前 E2E run 会因与本改动无关的原因显示为红色，日后很容易被误判归因。
未验证： 新增的 scripts/tests/e2e-workflow.test.js 断言在本 PR 上从未被 CI 执行过。该套件通过 npm run test:scripts 在 Qwen Code CI 中运行，而后者只在 pull_request → main/release/** 与 push → main 时触发；本 PR 的 base 是 feat/e2e/fix-container-cleanup，因此这里没有任何通道会执行它。作者报告本地 30/30 通过；那是作者的声明，不是 CI 证据。我通过对照 head 的 YAML 阅读确认了这三条新增/修改断言成立，但那是静态核对，不是执行。
沙箱验证可以补上静态审查补不了的那一处：@qwen-code /verify —— 用以确认 workflow 结构测试套件在 e302892 上确实通过，因为本 PR 没有任何 CI 通道会跑它，而我对这些断言的核对只读。性能主张本身不需要额外通道，上面两次 run 已在被审提交上支撑了它。至于未被测试的最坏情形 —— 两条 Linux leg 落在同一台 ECS 宿主上、六个 fork 加 docker daemon 同处一机 —— /verify 与 /tmux 都触达不到，因为二者都不调度 Actions runner；对此的长期信号是本改动落地后的 E2E 每晚回归。
— Qwen Code · qwen3.8-max-2026-09-02
Reviewed at e30289206c020754cd3f18bb38b9468b46e84791 · re-run with @qwen-code /triage
|
Confidence: 4/5 — the evidence held up under independent checking and the change is minimal; two consistency follow-ups are worth doing but neither blocks. Stepping back: this is what a well-argued CI change looks like. The claim was falsifiable, the author gave me the two run IDs needed to falsify it, and when I went and read the job logs instead of trusting the table, everything matched — including the 19m00s critical path to the second. Better, the head commit's own run has now landed, and both Linux legs are green on the exact SHA under review with full coverage: 76 files and 464 tests each, which is precisely what the three baseline shards summed to. Nothing was silently dropped by collapsing the matrix, and the cumulative-to-wall ratios (2.87× and 2.86×, against ~0.98× on every baseline shard) prove the fork cap actually took effect rather than being swallowed by the config. That last point was the one that could have quietly invalidated the whole PR — My independent proposal was the same shape, so I can't claim a simpler path was missed. What I did find is two places where the change leaves the surrounding code saying something untrue, and both are cheap to fix here:
The related judgement call, which I'd flag to a maintainer rather than settle myself: total forks per run is unchanged at six, but they now sit in two jobs instead of six, so two legs landing on runners that share one ECS host would put six forks plus the docker daemon's containers on that host at once — exactly what the config comment was written to prevent. Neither run exercised it (the benchmark's legs landed on On the red E2E run. Run 34107488145 concluded Two process notes, neither blocking: this is stacked on #11264, which is still open, so it cannot land first and the diff only makes sense on top of that branch's container-reaping work. And The vestigial Approving, pinned to the reviewed commit. 中文说明信心度：4/5 —— 证据在独立核查下站得住，改动也足够小；有两处一致性收尾值得做，但都不构成阻塞。 退一步看：这是一个论证得当的 CI 改动应有的样子。主张是可证伪的，作者给出了证伪它所需的两个 run ID，而当我去读 job 日志而非采信表格时，所有数据都对得上 —— 包括精确到秒的 19分00秒 关键路径。更好的是，head 提交自身的 run 现已落定，两条 Linux leg 在被审 SHA 上均为绿色且覆盖完整：各 76 文件 / 464 测试，恰好等于基线三个 shard 之和。收掉 matrix 没有静默丢用例；累积与墙钟之比（2.87 倍与 2.86 倍，而基线每个 shard 均约 0.98 倍）证明 fork 上限确实生效，而非被配置吞掉。最后这一点正是可能悄悄让整个 PR 失效的地方 —— 我的独立方案与之同形，所以我说不出有更简路径被漏掉。我确实发现的是两处让周边代码变得「名不副实」的地方，且在此处修都很便宜：
与之相关的判断题，我倾向于提请 maintainer 而非自行定论：每次 run 的 fork 总数仍为六，但现在集中在两个 job 而非六个，因此若两条 leg 落到共享同一台 ECS 宿主的 runner 上，该宿主会同时承受六个 fork 加 docker daemon 的容器 —— 正是配置注释要防范的情形。两次 run 都没有覆盖到它（基准 run 的两条 leg 落在 关于红色的 E2E run。 run 34107488145 整体结论为 两点流程提示，均不阻塞：本 PR 基于仍未合并的 #11264，因此不能先合，且该 diff 只有在那个分支的容器回收工作之上才成立。另外 我在门禁阶段问到的残留 已批准，并钉在被审提交上。 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
Verified independently rather than from the PR table: the head commit's own E2E run has both Linux legs green on this exact SHA with full coverage (76 files / 464 tests each, matching the three baseline shards' sum), and cumulative-to-wall ratios of 2.87x / 2.86x confirm the fork cap really overrides the config's self-hosted maxForks 1.
Two non-blocking follow-ups named in my Stage 3 comment: the retry-budget threshold is still derived from a single shard even though it now retries the full suite, and integration-tests/vitest.config.ts still documents one child process per shard on self-hosted runners, which this lane now overrides.
The red macOS shard 2/2 is unrelated to this diff (separate job from line 427, GitHub-hosted runner, no shared changed line) and was green in all six recent comparison runs I sampled — worth a re-run so this commit does not carry a red it did not cause.
qwen-code-dev-bot
left a comment
qwen-code-dev-bot
left a comment
There was a problem hiding this comment.
Approved at head e3028920, reviewed against this PR's base (feat/e2e/fix-container-cleanup), which is the container-reap PR rather than main.
The knob is real, and the leg that can be measured already shows the win. --poolOptions.forks.maxForks=3 is accepted by the pinned Vitest (3.2.4) — I ran vitest list --poolOptions.forks.maxForks=3 --pool=forks against the installed dependency and it exits clean rather than rejecting an unknown option, so the flag is not being swallowed. On the measurement that matters: sandbox:none - shard 1/1 just finished in 16m18s on one runner, where the sharded baseline cost three runners 14m54s / 18m41s / 20m1s. Same wall clock, two fewer pool runners per sandbox, which is what the replaced comment was buying with the path-hash sharding note.
The Docker leg — the one with real risk against the cap — came in comfortably. It finished while I was writing this at 23m55s, where the sharded baseline needed three runners for 23m/26m/35m. One runner at three forks therefore does the whole Docker suite in less wall clock than the fastest shard used to take, against a 60-minute timeout-minutes, so the headroom is roughly 2.5x rather than a coin flip. The sandbox:none leg keeps its bounded retry and the Docker leg still has none, which is unchanged from the baseline and worth knowing if a future pool gets noisy.
The guard is structural, which is the right shape for a matrix change. The new test pins both the shard: ['1/1'] matrix value and the presence of the flag in both legs (.match(...) with toHaveLength(2)), and the existing argument-list pin at :205 was updated rather than deleted, so the exclusion set and the shard expression stay anchored in the same assertion that a future edit has to satisfy.
CI facts. Build for E2E, triage (39m46s), both Linux legs (16m18s and 23m55s), E2E Test - macOS - shard 1/2 and the meta jobs are green; E2E Test - macOS - shard 2/2 is red at 9m3s. That last one is a leg this diff does not touch — nothing in the change reaches e2e-test-macos, and the same leg passed in 10m15s on the base commit — and its job log is no longer retrievable, so the only step-level fact I have is that step 8 (Run E2E tests) exited 1. This PR does not trigger the main CI workflow at all — its base is a feature branch — so Test (ubuntu-latest) and Lint & Static never run here and no lane executes the guard suite. I ran it against a detached worktree of this head instead: scripts/tests/e2e-workflow.test.js 30/30 passed, including the new fork/matrix pin.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: reverse audit — stopped before round 3 by the review time budget.
Not linted (tool limitation, not a blocker): .github/workflows/e2e.yml — actionlint embedded-shell source mapping is not yet supported.
中文说明
仅完成部分审查，审查缺口已披露。
未审查：反向审计——评审时间预算不足，未能开始第 3 轮。
未检查（工具限制，非阻断）：.github/workflows/e2e.yml——actionlint 对 workflow 内嵌 shell 的源映射尚未支持。
— qwen3.8-max via Qwen Code /review (v0.23.0)
| else | ||
| run_shard() { | ||
| QWEN_E2E_RENDERER=ink npm run test:integration:sandbox:none -- --exclude '**/interactive/cron-interactive.test.ts' --exclude '**/channel-plugin.test.ts' --exclude '**/chat-transcript-document.test.ts' --shard='${{ matrix.shard }}' | ||
| QWEN_E2E_RENDERER=ink npm run test:integration:sandbox:none -- --exclude '**/interactive/cron-interactive.test.ts' --exclude '**/channel-plugin.test.ts' --exclude '**/chat-transcript-document.test.ts' --poolOptions.forks.maxForks=3 --shard='${{ matrix.shard }}' |
There was a problem hiding this comment.
[Critical] R1-6: [fails-closed] [regression] Three Vitest forks now share one scratch root, and two files in the suite ask for that root by the same name — so one fork rm -rfs the other's live test directory mid-test.
--poolOptions.forks.maxForks=3 raises the self-hosted pool lane from one child process to three. integration-tests/globalSetup.ts computes one runDir per vitest process and assigns it to INTEGRATION_TEST_FILE_DIR, so every fork inherits the identical value. integration-tests/test-helper.ts:228 then derives testDir from the case name alone — no file, fork or pid component — and the next two lines rm -rf it and recreate it. cli/file-system.test.ts:52 and cli/write_file.test.ts:18 both call rig.setup('should be able to write a file'), which sanitizeTestName folds to the same path. When vitest co-schedules those two files across two of the three forks, the second fork's setup() wipes the first fork's live directory: its test.txt, its .qwen/settings.json and its telemetry.log. The spawned CLI runs with cwd: testDir and the assertions read join(testDir, 'telemetry.log'), so the victim throws ENOENT on rig.readFile('test.txt') or loses its cwd. On the docker leg the container mounts the test directory at the same host path, so two sandboxes mount one directory.
This is not a pre-existing hazard the diff merely exposes. At the merge base the self-hosted branch of integration-tests/vitest.config.ts held the pool lane to one child process, and vitest's own shard rule put the pair in different shards (1/3 vs 3/3) — therefore different jobs, workspaces and runDirs. The macOS lane splits them too (1/2 vs 2/2). shard: '1/1' is what puts them in one job, so no lane in this repository could co-schedule that pair before this change. What lands is a nondeterministic red on the lane that runs on every push to main, with a signature (ENOENT, cross-test telemetry) that reads as a product bug rather than a harness collision, and the bounded retry absorbs it by re-running the whole suite.
Witness:
Real TestRig driven in two vitest forks, replaying both files' real openings (reviewed commit, unmodified tree):
file-system.probe pid 2000511 after-setup testDir <runDir>/should-be-able-to-write-a-file markerExists=true
write_file.probe pid 2000510 after-setup testDir <runDir>/should-be-able-to-write-a-file
file-system.probe after-run samples: [{"dirExists":true,"testTxt":false,"settings":true,"telemetry":false}, ... x10]
file-system.probe readFileError=Error: ENOENT: no such file or directory, open '<runDir>/should-be-able-to-write-a-file/test.txt'
Two pids, one identical testDir. Over 5 repetitions per arm:
second setup() landing while the first is mid-run -> 5/5 ENOENT
both setup() calls in the same millisecond -> 0/5
Population sweep (76 executed files, 128 setup() call sites): cross-file duplicate directory names: 1
Vitest's own shard rule (read from its dist, validated against real vitest) on the real population:
count=3 (Linux, BASE) -> file-system=1/3 write_file=3/3 sameShard=false
count=2 (macOS, unchanged) -> file-system=1/2 write_file=2/2 sameShard=false
count=1 (Linux, THIS DIFF) -> file-system=1/1 write_file=1/1 sameShard=true
Candidate fix (sanitizedName + '-' + process.pid): rounds 1-5 readFileError=none (was 5/5 ENOENT)
Make the rig directory fork-unique in this same change rather than after it — in integration-tests/test-helper.ts:228, key the directory on the worker as well as the case name, or on the test file basename, or mkdtemp under the name:
// integration-tests/test-helper.ts:228
this.testDir = join(env['INTEGRATION_TEST_FILE_DIR']!, `${sanitizedName}-${process.pid}`);Renaming the rig.setup() argument in one of the two files fixes today's pair but leaves the class open for the next duplicate name; keying the shared root is what closes it.
The reset itself has to stay: test-helper.ts:229-232 records that two cases setting up under the same name share the directory, that cleanup() keeps it whenever KEEP_OUTPUT is set (which CI always sets), and that the rm exists so a case never inherits the previous one's files — so make the directory fork-unique rather than dropping the reset, and keep test-helper.test.ts's reused-directory, cleanup and keep-output cases green (measured 7/7 green both before and after the candidate fix). Please add a case to integration-tests/test-helper.test.ts asserting that two rigs created with the same case name but different worker identity resolve to different directories and that neither setup() removes the other's, then prove it by taking the pid component back out and confirming that case goes red.
中文说明
三个 Vitest fork 现在共用同一个临时根目录，而测试集中有两个文件用同一个名字去申请该目录 —— 于是一个 fork 会在测试进行中途 rm -rf 掉另一个 fork 正在使用的测试目录。
--poolOptions.forks.maxForks=3 把 self-hosted pool 通道从单个子进程提升到三个。integration-tests/globalSetup.ts 每个 vitest 进程只计算一个 runDir 并赋给 INTEGRATION_TEST_FILE_DIR，因此所有 fork 继承到完全相同的值。integration-tests/test-helper.ts:228 随后仅凭用例名推导 testDir —— 不含文件、fork 或 pid 分量 —— 紧接着两行先 rm -rf 再重建。cli/file-system.test.ts:52 与 cli/write_file.test.ts:18 都调用 rig.setup('should be able to write a file')，经 sanitizeTestName 归一后落到同一路径。当 vitest 把这两个文件调度到三个 fork 中的两个上并发执行时，后一个 fork 的 setup() 会清掉前一个 fork 正在使用的目录：它的 test.txt、.qwen/settings.json 和 telemetry.log。被拉起的 CLI 以 cwd: testDir 运行，断言又读取 join(testDir, 'telemetry.log')，因此受害方要么在 rig.readFile('test.txt') 上抛出 ENOENT，要么丢失自己的 cwd。docker 一侧更严重：容器把测试目录挂载到同一宿主路径，等于两个 sandbox 挂载同一个目录。
这并不是本 diff 仅仅「暴露」出来的既有隐患。在 merge base 上，integration-tests/vitest.config.ts 的 self-hosted 分支把 pool 通道限制为单个子进程，而 vitest 自身的分片规则会把这一对文件分到不同 shard（1/3 与 3/3），因而落在不同 job、不同 workspace、不同 runDir。macOS 通道同样会把它们分开（1/2 与 2/2）。正是 shard: '1/1' 把它们放进了同一个 job，所以在本次改动之前，仓库里没有任何通道能让这一对文件并发。落地后的表现是：在每次 push 到 main 都会跑的通道上出现非确定性红灯，其特征（ENOENT、跨用例 telemetry）看起来像产品缺陷而非测试框架冲突，而有界重试只能通过重跑整套用例来吸收它。
修复方式：在同一次改动里就让 rig 目录具备 fork 唯一性，而不是事后再补 —— 在 integration-tests/test-helper.ts:228 让目录同时以 worker 和用例名为键（或加上测试文件基名，或在名字下 mkdtemp）。只重命名两个文件之一的 rig.setup() 参数能解决当前这一对，但下一次出现重名时同类问题仍会复现；对共享根目录做键化才是真正封口。
重置逻辑本身必须保留：test-helper.ts:229-232 记录了同名用例共享该目录、cleanup() 在设置了 KEEP_OUTPUT 时（CI always 设置）会保留它、以及这个 rm 的存在是为了让用例不会继承上一个用例的文件 —— 所以应让目录具备 fork 唯一性，而不是去掉重置，并保持 test-helper.test.ts 中「重用目录」「清理目录」「KEEP_OUTPUT 时保留目录」三个用例为绿（实测在候选修复前后均为 7/7 通过）。请在 integration-tests/test-helper.test.ts 中补一个用例，断言两个用例名相同但 worker 身份不同的 rig 会解析到不同目录、且任一 setup() 都不会删除对方的目录；然后把 pid 分量去掉，确认该用例变红，以此证明它确实有效。
— qwen3.8-max via Qwen Code /review (v0.23.0)
| # The suite is ~16min of wall clock on one runner, dominated by a long | ||
| # tail of sdk-typescript files. vitest assigns files to shards by path | ||
| # hash, so those spread out instead of clustering in one shard. | ||
| # Keep three-way test concurrency inside one runner so each sandbox |
There was a problem hiding this comment.
[Suggestion] R1-3: Two comments in this same job now state the opposite of what the file does, and neither is the wording the maintainer verification already asked you to change.
Collapsing the matrix to one shard retires the shard as the unit of work, but two present-tense statements inside this job still describe the old shape:
:252-254— "Mapped forintegration-tests/vitest.config.ts, which caps each shared-pool shard at one fork and exempts pressure-flake unhandled errors" — sits in the very step this diff edits, two lines above the two flags that raise that cap to three. The second half of the sentence is still true; the first half is not.:305-306— "Per-commit coordinator: one shard per host builds the image, its siblings wait here and then find it present" — withshard: ['1/1']there is exactly onesandbox:dockerjob per run, so it has no siblings. The only remaining contenders are other runs of the sameGITHUB_SHA, because the concurrency group at:22-24is keyed on workflow / event name / ref, so aworkflow_dispatchorschedulerun of the same SHA lands in a different group and can overlap the in-flightpushrun.
The cost is a mis-diagnosis rather than a failure: a maintainer who finds the docker leg parked up to 30 minutes in flock --wait 1800 8 goes looking for the intra-run sibling the comment promises, finds none, and reads a cross-run collision as a stuck intra-run handoff. Nothing surfaces it — scripts/tests/e2e-workflow.test.js:131-135 pins the same sibling-shard narrative against run 33637097713 while asserting only lock ordering, which is still valid, so that test stays green.
I am deliberately not re-raising the mechanism attribution in the comment this diff adds at :130-131 ("shares setup work"): the maintainer verification already in this thread measured it (setup is 27s per job, 0.9% of the runner-time saving; the durable reason is dynamic load balancing) and asked for that sentence, so you already have it.
Witness:
vitest 3.2.7 resolveConfig on this workflow's own extracted argv, falsifying :252-254:
RUNNER_ENVIRONMENT=self-hosted CONFIG_ONLY {"pool":"forks","forks":{"minForks":1,"maxForks":1}}
RUNNER_ENVIRONMENT=self-hosted CONFIG+CLI {"pool":"forks","forks":{"minForks":1,"maxForks":3}}
Topology falsifying :305-306, read at the reviewed commit:
e2e.yml:132-133 shard: ['1/1'] -> one sandbox:docker job per run, no siblings
e2e.yml:22-24 concurrency group keyed on workflow / event_name / ref
-> a workflow_dispatch or schedule run of the same SHA is in a DIFFERENT group
grep -n 'docker-sandbox-build-e2e' .github/workflows/*.yml -> e2e.yml:307 only
Reword both to the post-collapse shape in this same edit: at :252-254 say what the mapping is still for (the pressure-flake unhandled-error exemption, and the fork floor) and note that the ceiling is set on the two test commands below; at :305-306 say the coordinator's remaining contention is cross-run (same GITHUB_SHA, different runs or attempts on one host) rather than between sibling shards.
One constraint on the config-comment pointer the maintainer verification asked for: the consumer inventory it would be written from does not hold, and writing it as instructed would put a second false sentence exactly where this finding says the first one must be corrected. E2E is not the only self-hosted consumer of integration-tests/vitest.config.ts — release.yml's integration_none (pool runs-on, RUNNER_ENVIRONMENT mapped at :767 and :773, running test:integration:cli:sandbox:none at :769 and :interactive at :775) and integration_docker (mapped at :819, running vitest run --root ./integration-tests at :871-872) pass no override and genuinely resolve {minForks:1, maxForks:1}. And ci.yml never maps RUNNER_ENVIRONMENT at all, so its pool legs take the non-self-hosted branch — test:integration:no-ak carries an explicit --poolOptions.forks.maxForks 2 and test:integration:cli (ci.yml:2216) carries none, resolving {minForks:2, maxForks:4} — not one fork. Keeping the override here in e2e.yml is still the right call; only the sentence describing everyone else needs to be accurate.
中文说明
同一个 job 里有两处注释现在与文件的实际行为相反，而且都不是维护者验证评论已经请你修改的那一处措辞。
把 matrix 收成单个 shard，意味着 shard 不再是工作单元，但这个 job 内仍有两处现在时的描述在讲旧形态：
:252-254—— "Mapped forintegration-tests/vitest.config.ts, which caps each shared-pool shard at one fork and exempts pressure-flake unhandled errors" —— 就在本 diff 修改的这个 step 里，位于把该上限提到三的两处 flag 上方两行。这句话的后半仍然成立，前半已不成立。:305-306—— "Per-commit coordinator: one shard per host builds the image, its siblings wait here and then find it present" —— 在shard: ['1/1']下每次 run 只有一个sandbox:dockerjob，因此不存在「兄弟 shard」。剩下的竞争者只可能是同一GITHUB_SHA的其他 run：:22-24的 concurrency group 以 workflow / event name / ref 为键，所以同一 SHA 的workflow_dispatch或schedulerun 属于不同 group，可以与进行中的pushrun 重叠。
代价是误诊而非失败：当维护者发现 docker leg 在 flock --wait 1800 8 上停留长达 30 分钟时，会按注释去找那个并不存在的同 run 兄弟 shard，找不到之后就把一次跨 run 的锁竞争误判为同 run 内的交接卡死。而且没有任何机制会把这一点暴露出来 —— scripts/tests/e2e-workflow.test.js:131-135 针对 run 33637097713 钉住了同样的兄弟 shard 叙事，但它断言的只是锁的顺序，而顺序仍然有效，所以该测试保持绿色。
我特意不重复提本 diff 在 :130-131 新增注释里的机制归因（"shares setup work"）：线程中已有的维护者验证评论已经实测过（setup 每个 job 27 秒，占 runner 时间节省的 0.9%；真正持久的原因是动态负载均衡）并请作者修改那句话，因此作者已经掌握这一点。
修复方式：在同一次编辑中把两处都改写为收拢后的形态 —— :252-254 说明该映射如今还为什么存在（pressure-flake 未处理错误豁免，以及 fork 下限），并注明上限是由下方的两条测试命令设定的；:305-306 说明该协调锁剩余的竞争是跨 run 的（同一 GITHUB_SHA、同一宿主上的不同 run 或 attempt），而不是兄弟 shard 之间的。
关于维护者建议的「在配置注释里加一行指引」，有一个前提约束：它所依据的消费者清单并不成立，若照原样书写，就会在本条发现指出需要修正的地方再放一句假话。E2E 不是 integration-tests/vitest.config.ts 唯一的 self-hosted 消费者 —— release.yml 的 integration_none（pool runs-on，:767 与 :773 映射 RUNNER_ENVIRONMENT，:769 运行 test:integration:cli:sandbox:none、:775 运行 :interactive）与 integration_docker（:819 映射，:871-872 运行 vitest run --root ./integration-tests）都没有传覆盖参数，确实解析为 {minForks:1, maxForks:1}。而 ci.yml 根本没有映射 RUNNER_ENVIRONMENT，因此它在 pool 上的 leg 走的是非 self-hosted 分支 —— test:integration:no-ak 显式带 --poolOptions.forks.maxForks 2，test:integration:cli（ci.yml:2216）不带任何覆盖，解析为 {minForks:2, maxForks:4} —— 并非单 fork。把覆盖保留在 e2e.yml 里仍然是正确选择；只是描述「其他所有人」的那句话需要写准。
— qwen3.8-max via Qwen Code /review (v0.23.0)
| ); | ||
|
|
||
| expect(linuxJob.strategy.matrix.shard).toEqual(['1/1']); | ||
| expect(runStep.run.match(/--poolOptions\.forks\.maxForks=3/g)).toHaveLength( |
There was a problem hiding this comment.
[Suggestion] R1-7: This count does not pin the docker leg — the flag can leave the docker command and the whole suite stays green.
runStep.run is the entire ~120-line step script, comments included, and /--poolOptions\.forks\.maxForks=3/g is anchored to nothing, so any two occurrences anywhere in it satisfy toHaveLength(2). The sandbox:none leg is positionally pinned at :208, which asserts the full command; the docker leg is not — its only positional pin is :159's toContain("--shard='${{ matrix.shard }}' 9>&-"), which stops before the flag.
So delete --poolOptions.forks.maxForks=3 from the docker command at .github/workflows/e2e.yml:362 and keep the string twice in the step by mentioning it in the adjacent comment: the count still reads 2, :159 still matches, :208 still matches, this file's new case still prints green — and the docker leg silently reverts to integration-tests/vitest.config.ts:38's maxForks: isSelfHostedRunner ? 1 : 4, one fork on the ECS pool that is this workflow's normal lane. Against the paired measurement this PR's own rationale rests on (aggregate test-step minutes 120.9 → 31.6, 3.8×), that is roughly a tripling of the docker job's wall clock against an unchanged timeout-minutes: 60, with no red test to say so. This file already states the rule the new assertion falls short of, for exactly this reason: "Structure, not just count" (:235).
Worth knowing because it corrects a claim already on the PR: the reported "5/5 mutants killed, including dropping the flag from only the docker leg (the toHaveLength(2) assertion is what catches that)" covers the uncompensated deletion, which drops the count to 1 and is indeed killed. The compensated deletion above survives every assertion in the repository.
Witness:
Mutation run in a scratch tree at the reviewed commit, then reverted:
BASELINE (unmodified): e2e-workflow.test.js (30 tests) 30 passed
MUTATION A (flag deleted from the docker command at e2e.yml:362, string kept twice
in the step by mentioning it in the adjacent comment):
grep -c -- "--poolOptions.forks.maxForks=3" .github/workflows/e2e.yml -> 2
e2e-workflow.test.js (30 tests) 30 passed <- the new case still green
e2e-shard-retry.test.js (6) + no-ak-integration-ci.test.js (14) -> 50 passed
(These are all three test files in the repo that read .github/workflows/e2e.yml.)
CANDIDATE FIX, then re-run:
fix + mutation A -> FAIL 'closes the lock descriptors in every child process' (mutant killed)
fix + real e2e.yml -> e2e-workflow.test.js (30 tests) 30 passed
Pin each leg's command rather than counting the string — extend the existing descriptor-close assertion at :159 to carry the flag:
expect(runStep.run).toContain(
"--poolOptions.forks.maxForks=3 --shard='${{ matrix.shard }}' 9>&-",
);and keep :208's full-command pin for the sandbox:none leg. The toHaveLength(2) count can then go, or stay as redundancy on top of two positional pins.
Keep the 9>&- suffix in that extended pin: :159's existing toContain is the witness for the descriptor close (:151-153, "closes the lock descriptors in every child process"), and dropping the suffix would unpin the fd-9 close that stops the three forks and their sandbox containers from inheriting the daemon read lock. Any edit must also keep :208 byte-identical to the run_shard() body at .github/workflows/e2e.yml:365. Please prove the new pin by removing the flag from the docker command and confirming that assertion goes red — today nothing in the repository does.
中文说明
这个计数并没有钉住 docker 一侧 —— 该 flag 可以从 docker 命令中消失，而整个测试套件仍然是绿的。
runStep.run 是整段约 120 行的 step 脚本，注释也包含在内，而 /--poolOptions\.forks\.maxForks=3/g 没有任何位置锚定，因此脚本中任意位置出现两次就能满足 toHaveLength(2)。sandbox:none 一侧在 :208 有位置性钉住（断言完整命令）；docker 一侧没有 —— 它唯一的位置性钉住是 :159 的 toContain("--shard='${{ matrix.shard }}' 9>&-")，而该断言在 flag 之前就截止了。
于是：把 --poolOptions.forks.maxForks=3 从 .github/workflows/e2e.yml:362 的 docker 命令里删掉，并在相邻注释中提一句、让这个字符串在 step 中仍出现两次 —— 计数依然是 2，:159 依然匹配，:208 依然匹配，本文件新增用例依然为绿 —— 而 docker 一侧会静默退回到 integration-tests/vitest.config.ts:38 的 maxForks: isSelfHostedRunner ? 1 : 4，也就是在这个 workflow 的常规通道（ECS pool）上只跑一个 fork。以本 PR 立论所依据的配对实测为参照（各 job 测试阶段耗时合计 120.9 分钟 → 31.6 分钟，3.8 倍），这大致等于 docker job 墙钟时间变为三倍，而 timeout-minutes: 60 未变，却没有任何红灯提示。本文件其实已经写明了这条新断言未达到的标准，且理由正是如此："Structure, not just count"（:235）。
有一点值得说明，因为它更正了 PR 上已有的一处结论：已报告的「5/5 变异体全部被杀，包括只从 docker 一侧删掉该 flag（正是 toHaveLength(2) 断言抓住了它）」针对的是未做补偿的删除 —— 那会把计数降到 1，确实会被杀掉。而上面这种带补偿的删除能躲过仓库中的每一条断言。
修复方式：钉住每一侧的命令，而不是数字符串出现次数 —— 把 :159 已有的描述符关闭断言扩展为同时包含该 flag，并保留 :208 对 sandbox:none 一侧的完整命令钉住。此后 toHaveLength(2) 计数可以删除，也可以作为两条位置性钉住之上的冗余保留。
扩展后的钉住必须保留 9>&- 后缀：:159 现有的 toContain 正是描述符关闭的见证（:151-153，"closes the lock descriptors in every child process"），去掉该后缀就会失去对 fd-9 关闭的钉住 —— 而正是它阻止三个 fork 及其 sandbox 容器继承 daemon 读锁。任何修改还必须让 :208 与 .github/workflows/e2e.yml:365 的 run_shard() 函数体保持逐字节一致。请通过从 docker 命令中删除该 flag、并确认该断言变红来证明新的钉住确实有效 —— 目前仓库中没有任何断言能做到这一点。
— qwen3.8-max via Qwen Code /review (v0.23.0)
qwen-code-dev-bot
left a comment
qwen-code-dev-bot
left a comment
There was a problem hiding this comment.
门禁 Review（复核）— head e30289206c
决定：REQUEST_CHANGES。本条推翻我在 10:23 对同一 head 提交的 APPROVE：当时这条腿没有可用的运行结果，现在 12:13 的评审 raised 了一条新的 Critical（R1-6），我在代码里逐处核对后确认它成立，而且确实是本 PR 引入的。
阻塞项：三个 fork 共用一个 scratch root，两个 spec 用同一个名字要它
我自己在 e30289206c 上核对的链路，不依赖评审的措辞：
integration-tests/globalSetup.ts:154—runDir = join(integrationTestsDir, String(Date.now()))，:197把它写进process.env['INTEGRATION_TEST_FILE_DIR']。global setup 每个 vitest 进程只跑一次，所以同进程内的所有 fork 拿到的是同一个runDir。integration-tests/test-helper.ts:227-234—testDir = join(INTEGRATION_TEST_FILE_DIR, sanitizeTestName(testName))，紧接着rm(this.testDir, { recursive: true, force: true })再mkdirSync。目录名里没有任何 file / fork / pid 成分；:514-516的 teardown 还会再删一次。integration-tests/cli/file-system.test.ts:52与integration-tests/cli/write_file.test.ts:18都调用rig.setup('should be able to write a file')→ 折叠成同一个testDir。CLI 以cwd: testDir启动，断言又去读join(testDir, 'telemetry.log')。
而并发度正是本 PR 抬上去的：它一行都没改 integration-tests/（git diff <base> e30289206c -- integration-tests 为空），基线里 integration-tests/vitest.config.ts:37-38 是 minForks/maxForks = isSelfHostedRunner ? 1 : …，也就是说 self-hosted 那条腿原本是单进程单 fork；现在命令行上的 --poolOptions.forks.maxForks=3 把它变成同一进程里三个文件并行，两个 fork 就可能同时持有同一个 testDir，第二个的 setup() 会把第一个正在用的目录（test.txt、.qwen/settings.json、telemetry.log）连底删掉。
基线上这一对撞不上：shard: '1/3' … '3/3' 时 vitest 按路径哈希分文件，这一对被分到不同 shard（不同 job、不同工作区、不同 runDir），macOS 两条腿同样把它们拆开。是 shard: '1/1' 第一次让这两个文件有机会同进程并行——所以这不是"某个老隐患被顺手暴露"，而是这个改动新建出来的。
为什么按阻塞处理：它红在每次 push main 都会跑的腿上，红形是 ENOENT 或跨用例的 telemetry，看起来像产品 bug 而不是 harness 撞车；sandbox:none 那条腿还带 bounded retry，会把一次真实回归吃成"重跑就绿"。本 PR 自己两次 Linux 运行都过，这与"竞态"完全一致，不构成反证。
任一做法都能清掉：把 scratch 目录按 fork 唯一化（test-helper.ts 里加上 process.pid 或每个文件一个槽位，顺带把 hosted 腿 maxForks=4 的潜在同类隐患一起修掉）；或者把两个用例名改成不同的字符串；或者只保留 cli/ 这一族的分片。改完请把两条 Linux 腿各连跑两三次再叫我。
随这次推送一起做掉的两条（Suggestion，本来就很便宜）
- R1-3：
:252-254还写着 "caps each shared-pool shard at one fork"，:305-306还写着 "one shard per host builds the image, its siblings wait here"。分片收成 1/1 之后，前一句被本 PR 自己那两个 flag 直接推翻，后一句描述的兄弟分片已经不存在（同一次 run 里只有一个 docker job，真正会争flock --wait 1800的是同一 SHA 的跨 run）。 - R1-7：
toHaveLength(2)数的是整段 step 脚本里出现的次数，没锚定位置。把 docker 那条命令上的 flag 删掉、在相邻注释里把这个字符串多写一遍，计数仍是 2，全仓测试仍然绿，而 docker 腿会静默回到maxForks: 1——对着 60 分钟的timeout-minutes大约是三倍墙钟。这个文件自己在:235写的规则就是"要结构不要计数"，新断言应当按整条命令行去 pin。
我 10:23 那些仍然成立的部分
--poolOptions.forks.maxForks=3 确实被 vitest 3.2.4 接受并生效；实测数字也仍然支持这个方向（sandbox:none 1/1 16m18s、sandbox:docker 1/1 23m55s，对比三条分片基线的 14m54/18m41/20m1 与 23m/26m/35m）。我要的不是回退这个改动，而是先把共用 scratch root 这件事解决掉。
CI 事实
本 head 上 Build for E2E、triage、两条 Linux 腿、macOS 1/2 与 meta 作业都绿；E2E Test - macOS - shard 2/2 红（9m3s，本 diff 不碰 macOS 作业，基线那条腿是绿的，其日志已拉不到，只有 step 8 Run E2E tests 退 1 这一条事实）。页面现在的 CHANGES_REQUESTED 来自 12:13 那份评审，也就是我这次认同的那份。
2fda8a2
into
feat/e2e/fix-container-cleanup
* ci: benchmark Vitest forks against E2E shards * ci: prepare E2E fork configuration for review
* ci: benchmark Vitest forks against E2E shards * ci: prepare E2E fork configuration for review
6 participants
What this PR does
This replaces the three Linux E2E shards per sandbox with one job running the full suite with up to three Vitest forks. It keeps the same test-level concurrency while sharing setup work and reducing the Linux self-hosted runner requirement from six to two. The test set, sandbox matrix, macOS jobs, and OpenTUI job stay unchanged.
Why it's needed
The existing static shards repeat job setup and can become imbalanced under shared-pool pressure. A paired run from the same source commit completed faster with fewer runner-minutes when concurrency moved inside Vitest.
Both runs covered the same 76 files and 464 tests per sandbox.
Reviewer Test Plan
How to verify
Compare baseline run https://github.com/QwenLM/qwen-code/actions/runs/34094994389 with the updated run https://github.com/QwenLM/qwen-code/actions/runs/34101867981. Confirm that each sandbox runs one Linux job, each job covers all 76 files and 464 tests, and all E2E jobs pass.
Evidence (Before & After)
Before: six Linux self-hosted runners, 35m 09s critical path, 139m 10s runner time. After: two Linux self-hosted runners, 19m critical path, 33m 49s runner time.
Tested on
Environment (optional)
The workflow structure test passes locally: 30/30. The full E2E workflow passes on GitHub Actions.
Risk & Scope
Linked Issues
Built on #11264.
中文说明
此 PR 做了什么
把每种 sandbox 的三个 Linux E2E shard 改为一个 job，并在完整测试集上最多运行三个 Vitest fork。测试级并发量保持不变，同时共享初始化工作，把 Linux self-hosted runner 需求从六个降为两个。测试集合、sandbox matrix、macOS job 和 OpenTUI job 均保持不变。
为什么需要
现有静态 shard 会重复执行 job 初始化，并且在共享 runner 池压力下可能出现负载不均。同一源码提交上的配对测试表明，把并发下沉到 Vitest 后，执行更快且 runner-minutes 更少。
两次运行中，每种 sandbox 均覆盖相同的 76 个文件和 464 个测试。
Reviewer Test Plan
如何验证
对比基线 run https://github.com/QwenLM/qwen-code/actions/runs/34094994389 和修改后 run https://github.com/QwenLM/qwen-code/actions/runs/34101867981。确认每种 sandbox 只运行一个 Linux job、每个 job 覆盖全部 76 个文件和 464 个测试，并且所有 E2E job 通过。
证据（Before & After）
修改前：六个 Linux self-hosted runner，关键路径 35分09秒，runner 时间 139分10秒。修改后：两个 Linux self-hosted runner，关键路径 19分钟，runner 时间 33分49秒。
测试环境
环境（可选）
workflow 结构测试在本地通过：30/30。完整 E2E workflow 在 GitHub Actions 上通过。
风险与范围
关联事项
基于 #11264。