ci: serialize E2E tests on shared ECS runners - #10567
Conversation
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
chiga0
left a comment
chiga0
left a comment
There was a problem hiding this comment.
Review — ci: serialize E2E tests on shared ECS runners
Verdict: APPROVE — no blockers found.
Scope
- Read all 3 changed files in full at head SHA
7c529d179509b019a3dd2724179fa9d22cb59b9b - Cross-file check:
RUNNER_ENVIRONMENTwriter (e2e.yml${{ runner.environment }}) vs. reader (vitest.config.ts=== 'self-hosted') — contract intact, no new gap introduced - Checked
isSelfHostedRunnerconstant evaluation timing and test isolation pattern - Checked all 4 new/rewritten tests for non-vacuity
- CI check enumeration: see note below
Cross-file checks
RUNNER_ENVIRONMENT contract (class 1): The env var was already set in e2e.yml via ${{ runner.environment }} before this PR. The new vitest.config.ts reads it at module import time with === 'self-hosted'. GitHub Actions sets runner.environment to 'self-hosted' on self-hosted runners, which matches. No asymmetry.
Module-level constant timing: isSelfHostedRunner is evaluated once at import time. The tests correctly handle this by calling vi.resetModules() before each import() to force re-evaluation. The afterEach restores the ambient RUNNER_ENVIRONMENT and resets the module cache. This is the established pattern already used for the dangerouslyIgnoreUnhandledErrors tests.
dangerouslyIgnoreUnhandledErrors refactoring: process.platform !== 'linux' || process.env['RUNNER_ENVIRONMENT'] === 'self-hosted' → process.platform !== 'linux' || isSelfHostedRunner. Semantically identical; isSelfHostedRunner is a read-only alias for the same expression at the same evaluation point.
Test validity (class 5)
Checked all 4 tests for non-vacuity:
| Test | Mutation that would kill it |
|---|---|
serializes test files on shared self-hosted runners |
Revert minForks/maxForks back to 2/4 unconditionally |
keeps the existing fork limits outside the shared pool |
Change isSelfHostedRunner ? 1 : 2 to always 1 |
exempts self-hosted pool runners on every platform |
Remove ` |
keeps unhandled errors fatal on github-hosted Linux and local runs |
Remove the dangerouslyIgnoreUnhandledErrors field entirely (test uses toBe, so undefined !== false fails on Linux) |
All 4 tests are non-vacuous.
The loop in 'keeps the existing fork limits outside the shared pool' iterates ['github-hosted', undefined] — covers both explicit non-self-hosted and absent env var, and each call does its own vi.resetModules() + fresh import(). Valid.
CI status
Checks at time of review:
Dependency CVE audit— passSecret scan (TruffleHog)— passassign— pass (automation)precheck-pr— skipping (×2)review-config— skippingClassify PR/authorize/label— pending (automation)
No build, typecheck, lint, or test run is present in this check set. The CI covers security scanning and PR-automation only. The code changes are syntactically and semantically straightforward (a new boolean constant and two ternary expressions), so the type-error risk is low, but this dimension is unconfirmed by CI.
Not covered
- Execution rungs 1–3 not run (no local toolchain in this environment)
- macOS/Windows runner behaviour not tested (not relevant — the change is a process-count cap, not a platform branch)
Reviewed with AI assistance.
|
|
|
Thanks for the PR! Template looks good ✓ Problem: observed, not theoretical. Linked issue #10566 tracks the E2E failure on Direction: aligned. Pure CI-stability work for the shared ECS pool; no product surface, and the 100 ms recall contract is deliberately left untouched. Size: not applicable — no core paths. 11 production-config lines, a comment-only workflow touch-up, and 76 lines of test restructuring. Approach: the scope feels right. Capping the forks pool at one worker on the shared pool removes exactly the per-job process multiplier the pool controls, reuses the Risk: no elevated risk signals (none of the changed files match the revert-correlated path set). Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 问题：已观测到，不是理论问题。关联 issue #10566 跟踪的是 方向：对齐。纯粹的共享 ECS 池 CI 稳定性修复，不涉及产品面，且刻意不触碰 100 ms 召回预算。 规模：不适用——未触及核心路径。11 行生产配置、一处仅改注释的 workflow 更新，以及 76 行测试重构。 方案：范围合理。在共享池上把 fork 池限制为单 worker，恰好消除了池内可控的每 job 进程倍增；复用了 #10552 已接好的 风险：无升级风险信号（改动文件均未命中与 revert 相关的高风险路径集）。 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewNo blocking issues. The change reads as the minimal fix for the stated problem:
Nothing to flag on conventions: no core paths, no new dependencies, and the test stays in the existing config-pin file rather than adding a new one. Testing — the PR's own CICI on this commit is still settling: at the time of this pass the Linux unit suite ( Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 The substantive verification here is the new config regression test itself, which runs inside the queued unit suite: it pins the fork counts for both environments, so the mechanical claim ("one fork on self-hosted, unchanged elsewhere") is settled by CI once green. The environmental claim (fewer pressure flakes on the shared ECS pool) cannot be settled by this PR's CI, and no sandboxed lane applies either — neither Not verified: actual wall-clock of serialized shards — only observable in the E2E workflow post-merge; the remedy is the same post-merge signal, plus the 60-minute watch item from Stage 1. 中文说明代码审查无阻塞问题。改动就是该问题的最小修复：
约定方面无可指摘：未触及核心路径、无新依赖，测试也留在既有的配置固定文件里，没有新增文件。 测试——PR 自身的 CI本提交的 CI 尚未跑完：截至本次审查，Linux 单测（ 这里实质性的验证是新增的配置回归测试本身（在排队的单测套件中运行）：它固定了两种环境的 fork 数量，因此机械性结论（"自托管下 1 个 fork，其他环境不变"）在 CI 变绿后即被证实。环境性结论（共享 ECS 池上压力 flake 减少）无法被本 PR 的 CI 证实，也没有适用的沙箱通道—— 未验证：串行 shard 的实际墙钟耗时——只能在合入后的 E2E workflow 中观察；对应的验证途径同样是合入后的信号，外加 Stage 1 提到的 60 分钟上限观察项。 — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 4/5 — clean, minimal, well-pinned fix for a real observed flake; the only reservation is post-merge wall-clock, which nothing pre-merge can settle. The approach matches what I would have proposed for this problem: cap the per-shard process multiplier where the pool actually controls it, keep the production 100 ms recall contract intact, and pin both environments with a regression test so a future config edit cannot silently restore the oversubscription. The diff carries nothing extra — the constant hoisting is the one piece of tidying, and it is load-bearing for the fork cap anyway. The failure it targets is confirmed in the logs of run 33317457036 (missing The honest limit of this PR: its own CI can prove the config change (the new pins run in the unit suite), but only post-merge E2E runs on Approving — CI is still settling on this commit, so approval is deferred until it lands green on the reviewed commit. 中文说明置信度：4/5 —— 针对真实观测到的 flake 的干净、最小、固定充分的修复；唯一的保留意见是合入后的墙钟耗时，这在合入前无法证实。 方案与我对该问题的独立提议一致：在共享池真正可控的范围内限制每 shard 的进程倍增，保持生产 100 ms 召回预算不变，并用回归测试固定两种环境，使后续的配置修改无法悄悄恢复过度并发。diff 没有多余内容——常量提取是唯一一处顺手整理，且本身就是 fork 上限所依赖的。其针对的失败已在 run 33317457036 的日志中确认（第一次模型请求中缺少 这个 PR 的诚实边界：它自己的 CI 能证实配置变更（新增的固定测试在单测套件中运行），但只有合入后 予以批准——本提交的 CI 尚未完成，批准将延迟到该提交的 CI 全绿后生效。 — Qwen Code · qwen3.8-max Reviewed at |
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. Suggestions are inline.
Not reviewed: build-and-test — Test (macos-latest, Node 22.x) and Test (windows-latest, Node 22.x) were skipped in CI, and the workspace-scoped local test run collected no suite for the changed files (they ran only in Linux verification probes).
Not linted (tool limitation, not a blocker): the executable-script lint — .github/workflows/e2e.yml: actionlint embedded-shell source mapping is not yet supported — not linted.
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。
未审查：build-and-test — Test (macos-latest, Node 22.x) and Test (windows-latest, Node 22.x) were skipped in CI, and the workspace-scoped local test run collected no suite for the changed files (they ran only in Linux verification probes)。
未检查（工具限制，非阻断）：the executable-script lint — .github/workflows/e2e.yml: actionlint embedded-shell source mapping is not yet supported — not linted。
— qwen3.8-max via Qwen Code /review (v0.22.3)
| # Mapped for integration-tests/vitest.config.ts, which caps each | ||
| # shared-pool shard at one fork and exempts pressure-flake unhandled | ||
| # errors. |
There was a problem hiding this comment.
[Suggestion] R1-1: The RUNNER_ENVIRONMENT mapping this new comment points at (line 222) is pinned by no test, yet as of this PR it is load-bearing for both the new one-fork cap and the unhandled-error exemption — both read it through isSelfHostedRunner. The new config test injects the env var itself via configFor() and never reads e2e.yml, and e2e-workflow.test.js asserts nothing about this step's env:. If a future edit to the Run E2E tests env block — it is regularly touched for the OPENAI_* secrets around it — drops or misspells the RUNNER_ENVIRONMENT: '${{ runner.environment }}' line, every test stays green while on the ecs-qwen pool shards silently revert to 2–4 forks and unhandled errors become fatal again: the #10325/#10566 pressure-flakes return in post-merge E2E with the whole suite green and no test signal pointing at the cause. Add an assertion in scripts/tests/e2e-workflow.test.js that the Run E2E tests step of e2e-test-linux carries the mapping (the file already resolves that step by name for the TMPDIR test, and scripts/tests/qwen-autofix-workflow.test.js:9953 already pins the identical literal for qwen-autofix.yml):
expect(runStep.env.RUNNER_ENVIRONMENT).toBe('${{ runner.environment }}');Witness:
BASELINE: Test Files 2 passed (2) / Tests 13 passed (13)
MUTATED (mapping line deleted): Test Files 2 passed (2) / Tests 13 passed (13) <- nothing turns red
MUTATED+FIX (assertion added): x routes Linux E2E scratch files away from /tmp
expect(runStep.env.RUNNER_ENVIRONMENT).toBe('${{ runner.environment }}')
RESTORED+FIX: Tests 13 passed (13)
The new assertion is the fix's acceptance criterion: deleting the RUNNER_ENVIRONMENT line from the Run E2E tests env block must turn it red while scripts/tests/integration-vitest-config.test.ts stays green — please confirm that mutation goes red when adding it.
中文说明
这行新注释所指向的 RUNNER_ENVIRONMENT 映射（第 222 行）目前没有任何测试固定，但自本 PR 起它同时支撑新的一步一 fork 上限和未处理错误豁免——两者都通过 isSelfHostedRunner 读取它。新增的配置测试用 configFor() 自行注入该环境变量，从不读取 e2e.yml；e2e-workflow.test.js 也没有对该步骤 env: 的任何断言。如果未来某次对 Run E2E tests env 块的修改（该块经常因周围的 OPENAI_* secrets 而被改动）删除或拼错了 RUNNER_ENVIRONMENT: '${{ runner.environment }}' 这一行，所有测试仍会是绿的，而在 ecs-qwen 池上 shard 会悄悄回到 2–4 个 fork、未处理错误重新变为致命：#10325/#10566 这类压力性 flake 会在合入后的 E2E 中回归，且没有任何测试信号指向原因。建议在 scripts/tests/e2e-workflow.test.js 中增加断言，确认 e2e-test-linux 的 Run E2E tests 步骤带有该映射（该文件已经按名称解析了这个步骤用于 TMPDIR 测试，且 scripts/tests/qwen-autofix-workflow.test.js:9953 已为 qwen-autofix.yml 固定了完全相同的字面量）。上面的变异验证表明：删除该映射行时现有 13 个测试全部仍通过，加上建议的断言后删除该行则会精确变红。该断言即修复的验收标准：请确认加上后删除映射行的变异会失败。
— qwen3.8-max via Qwen Code /review (v0.22.3)
github-actions
Bot
added
the
skip-changelog-auto
…ller prose (QwenLM#10587) * fix(review): readable bilingual disclosures for lint deferrals and caller prose The review body's disclosure sentences carried two readability defects, both visible on PR QwenLM#10567's posted round-1 body: - The deferred-checker line stuttered: script-lint's deferral/skip reasons ended with an "— not linted" tail written for a standalone context, the gate spliced them under a "the executable-script lint —" prefix, and the body wrapped the result in a sentence that already opens "Not linted:". Posted: "Not linted (tool limitation, not a blocker): the executable-script lint — ... — not linted." The reasons drop the tail, and the disclosure drops the circular prefix — the wrapper names the fact once, the path and reason carry the rest. - The body's Chinese half presented untranslated English as its translation. Two legs: - The deferral disclosure is machine-built from the report, so it can carry a real translation: `scriptLintGate` now returns bilingual disclosure pairs, the report schema gains an optional `reasonZh` (the actionlint deferral supplies it), and an older CLI's report without one falls back to the English reason in both halves. - Caller-prose "Not reviewed" entries stay untranslatable by construction, and the Chinese label now says so — `未审查（原文为英文）：` — instead of presenting an all-English sentence as a translation; the payload keeps its own English full stop rather than closing an English sentence with `。`. * test(review): pin the bilingual deferral disclosures the review asked for Five review-suggested pins, each verified by re-running its witness mutation: - The comment-grammar fixture now carries a malicious reasonZh, so the stripCommentGrammar(d.reasonZh) leg is exercised against a live marker (deleting the call previously survived the suite). - The pipeline reasonZh is pinned to its Chinese literal — a toContain('actionlint') fragment was satisfied by the English reason too. - Both skipped reasons get not.toContain('not linted') pins; re-appending either tail previously shipped green. - A two-entry deferred fixture pins the en '; ' and zh '；' joins and the reasonZh-carrying branch end-to-end — every prior fixture held one entry, so no join separator was ever observable, and the only full-sentence zh pin exercised the English-fallback branch.
|
Released in v0.23.0. |
4 participants
What this PR does
This change limits each integration-test shard to one Vitest fork when it runs on the shared self-hosted ECS pool. GitHub-hosted and local runs keep the existing two-to-four-fork range. A focused regression test pins both environments so later configuration changes cannot silently restore the shared-host oversubscription.
Why it's needed
PR #9992 added an ACP auto-memory E2E assertion that the deterministic local-memory match reaches the first model request within the production 100 ms initial-recall budget. On the ECS pool, several Actions runners share one physical host and every E2E shard could additionally launch up to four Vitest child processes. Under that multiplied CPU and I/O pressure, the deterministic scan can lose its bounded wait even though the feature and the same test normally pass.
The failure in run 33317457036 is the same missing
ACP-MEMORY-ZEPHYR-4207marker observed in run 33279084999. The earlier run predates PR #10552, so #10552 did not introduce this ACP failure. This is a shared-runner scheduling problem exposed by #9992's latency-sensitive coverage.Serializing files inside each self-hosted shard removes the extra per-job process multiplier without changing application code, weakening the 100 ms production contract, or moving the job back to GitHub-hosted runners.
Reviewer Test Plan
How to verify
Load the integration-test configuration with
RUNNER_ENVIRONMENT=self-hostedand confirm the forks pool is limited to exactly one worker. Load it withRUNNER_ENVIRONMENT=github-hostedor unset and confirm the existing two-to-four-worker range remains. In the E2E workflow, confirm the runner environment is passed to both sandbox command paths.Focused verification completed locally: the configuration regression suite passed all 4 tests; ESLint, Prettier, and
git diff --checkalso passed.Evidence (Before & After)
N/A — CI test-runner configuration only.
Tested on
Environment (optional)
Local focused configuration test with the repository's pinned Vitest toolchain.
Risk & Scope
Linked Issues
Fixes #10566
Related failure: #10533
中文说明
本 PR 做了什么
在共享的自托管 ECS 池上运行时，将每个集成测试 shard 限制为一个 Vitest fork。GitHub-hosted 和本地运行继续保持现有的 2–4 个 fork。新增聚焦回归测试固定这两种环境的行为，避免后续配置修改无意中恢复共享主机上的过度并发。
为什么需要
PR #9992 新增了 ACP 自动记忆 E2E 断言，要求确定性的本地记忆匹配在生产的 100 ms 初始召回预算内进入第一次模型请求。ECS 池中多个 Actions runner 共享同一台物理主机，而每个 E2E shard 还可以再启动最多 4 个 Vitest 子进程。CPU 和 I/O 压力被成倍放大后，即使功能和同一测试通常可以通过，确定性扫描也可能错过这个有上限的等待时间。
run 33317457036 的失败与 run 33279084999 中缺少
ACP-MEMORY-ZEPHYR-4207标记的失败完全相同。较早的 run 发生在 PR #10552 之前，因此 #10552 并未引入本次 ACP 失败。这是 #9992 的延迟敏感覆盖暴露出的共享 runner 调度问题。在每个自托管 shard 内串行运行测试文件，可以移除每个 job 额外放大的进程并发，同时不修改应用代码、不放宽生产的 100 ms 契约，也不把任务切回 GitHub-hosted runner。
Reviewer 测试计划
如何验证
使用
RUNNER_ENVIRONMENT=self-hosted加载集成测试配置，确认 forks pool 被严格限制为 1 个 worker。使用RUNNER_ENVIRONMENT=github-hosted或不设置该变量时，确认继续保持现有的 2–4 个 worker。在 E2E workflow 中确认 runner 环境变量会传递给两条 sandbox 命令路径。本地聚焦验证已完成：配置回归测试 4 个全部通过；ESLint、Prettier 和
git diff --check也全部通过。证据（修改前后）
N/A — 仅修改 CI 测试 runner 配置。
测试平台
环境（可选）
使用仓库锁定的 Vitest 工具链在本地运行聚焦配置测试。
风险与范围
关联 Issue
Fixes #10566
相关失败：#10533