fix: restore ACP-related main CI checks - #11033
Conversation
|
Thanks for the PR! Template looks good ✓ Problem: observed, and I confirmed it independently rather than taking the description's word for it. I pulled the logs from the linked run Direction: aligned. Test infrastructure only — the production default ( Size: not applicable — no core paths, Approach: scope feels right — one shared spawn site covers every harness-launched daemon instead of patching the two suites that happened to fail. Three things I checked or would flag, none of them blockers:
Risk: no elevated risk signals — the changed path matches none of the revert-correlated patterns. Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 问题： 属于已观测到的 bug，而且我没有只采信 PR 描述——我直接拉取了关联 run 方向： 对齐。仅涉及测试基础设施——生产默认值（ACP bridge 中的 规模： 不适用——未触及核心路径，仅 方案： 范围合理——在一个共享启动点修改，即可覆盖所有由 harness 启动的 daemon，而不必逐个修补恰好失败的那两个套件。有三点我核实过或想提出，均非阻塞项：
风险： 无升级风险信号——改动路径未命中任何与 revert 相关的模式。 进入代码审查 🔍 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
Code reviewMy independent proposal first. Reading only the title and the "Why it's needed" section — macOS E2E shards dying on ACP initialize timeouts at ~10s, while the same runner completes handshakes in over 10s — I would raise the ACP initialize budget for CI-spawned daemons only, at the single shared spawn site, and leave the production default alone so real users keep tight failure detection. Concretely: pass No critical blockers. I traced the mechanism end to end rather than assuming the flag works because it looks plausible:
The nesting check that could have quietly defeated the fix. A longer ACP budget only helps if it fits inside the enclosing vitest budget; otherwise the crisp One non-blocking consequence worth knowing for the next triage: the RSS test creates 1 + 5 + 10 sessions across three daemon spawns inside that 180s budget. A handshake that is merely slow now succeeds, which is the point — but one that is genuinely stuck can burn the test budget before the ACP error surfaces, so that failure would read as "test timed out in 180000ms" instead of naming ACP. The PR's risk section already prices in the 50s delay; this is just the diagnostic shape it takes. Not worth restructuring for. Skipping the sequence diagram and the changed-files table — one file, two lines, and no new runtime flow. Neither would carry signal here. Testing evidenceThis is an unattended CI run, so per the skill's rules I executed nothing from this PR — no build, no test, no What I verified myself (not the author's claim): I pulled both failed job logs from run Current CI on the reviewed commit — nothing red, and the legs that matter most for this change had not finished at review time:
I did not poll or wait on these — the table is a single fetch, and the finalize pass rewrites it once CI settles. Note that Not verified: that 60 seconds is sufficient on a contended macOS runner. That is a statistical claim about hosted-runner load, and no single green run establishes it — it settles only by watching whether this flake recurs on subsequent Sandboxed verification would partly settle this: Real-scenario tmux testing: N/A — unattended CI run, and the change has no user-visible surface. 中文说明先说我自己的独立方案。 只看标题与「为什么需要」——macOS E2E 分片在约 10 秒时因 ACP initialize 超时失败，而同一 runner 上又有超过 10 秒才完成的握手——我的做法是：只提高 CI 启动的 daemon 的 ACP initialize 预算，改在唯一的共享启动点，并保持生产默认值不变，让真实用户仍保有紧凑的失败检测。具体就是从共享 harness 的 无阻塞性问题。 我完整追踪了这条机制链，而不是因为 flag 看起来合理就假设它有效：flag 存在且类型为 number、文档默认 10000；仅在显式传入时才向下传递，最终由 bridge 以 一个可能悄悄让修复失效的嵌套校验。 更长的 ACP 预算只有在不超过外层 vitest 预算时才有意义，否则清晰的 一个非阻塞的后续影响，供下次排查参考：RSS 测试在 180 秒预算内跨三个 daemon 创建 1 + 5 + 10 个 session。仅仅慢的握手现在会成功，这正是目的；但真正卡死的握手可能在 ACP 错误浮现前就耗尽测试预算，于是失败信息会显示为「test timed out in 180000ms」而不再点名 ACP。PR 的风险段落已经计入这 50 秒延迟，这里只是说明它呈现出的诊断形态，不值得为此重构。 跳过时序图与变更文件表——单文件、两行、无新运行时流程，两者都不会带来信号。 测试证据。 本次为无人值守 CI 运行，按规则我没有执行本 PR 的任何代码——不 build、不跑测试、不 checkout。以下证据来自通过 API 读取的 PR 自身 CI，以及我对原始失败日志的独立阅读。我自行核实的内容（非作者声称）：拉取了 run 当前 CI 无任何红色。表中 未验证： 60 秒在资源争用的 macOS runner 上是否足够。这是关于托管 runner 负载的统计性结论，单次绿色运行无法确立，只能通过观察后续 沙箱验证可部分定论： 真实场景 tmux 测试：N/A——无人值守 CI 运行，且改动无用户可见界面。 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
|
Confidence: 4/5 — the mechanism is verified end to end and the original failure is confirmed from its own logs; the half point is the handful of non-blocking nits below plus a flake-rate claim no single run can settle. Stepping back: this is the right fix, and it's the fix I proposed independently before reading the diff. Two lines, one shared spawn site, production default untouched, and the number reused from a suite already running it on The part I'd push on, and did: is a wider budget treating a symptom? Raising a timeout can paper over a genuine latency regression just as easily as it can absorb runner contention, and those two call for opposite responses. I checked rather than assumed, and the evidence points at contention:
So this isn't masking a regression. But the honest residue is that nobody has answered why a handshake approaches 10s on a macOS runner at all. This PR buys CI headroom; it doesn't make startup faster. That's a legitimate separate follow-up, not a reason to hold this one — and it's the same trade #10846 already made for the routes suite. Where I'd correct the PR's own writeup: I could not find the "10.43 and 13.12 second successful handshakes" it cites anywhere in the two macOS shard logs. Those specific numbers are unconfirmed as far as I can tell. It doesn't change my verdict — the tight 9.7–10.0s cluster plus the routes suite passing at 60s on the same runner establishes contention on their own, and more directly — but a maintainer shouldn't take those two figures as verified. Non-blocking, for whoever picks this up next:
Approving. The macOS legs that actually judge this change were still in flight when I wrote this, so approval is deferred until CI lands green on 中文说明置信度：4/5 —— 机制已端到端核实，原始失败也从其自身日志得到确认；扣掉的半分来自下面几处非阻塞小问题，以及一个单次运行无法定论的 flake 概率结论。 退一步看：这是正确的修复，也是我在读 diff 之前独立提出的方案。两行代码、一个共享启动点、生产默认值未动、数值复用自 我真正质疑并核查过的点是：放宽预算是否只是在治标？ 提高超时既可能吸收 runner 资源争用，也同样可能掩盖真实的延迟回退，而这两种情况需要相反的应对。我做了核查而非假设，证据指向资源争用：
因此它并没有掩盖回退。但需要诚实指出的是：为什么 macOS runner 上的握手会接近 10 秒，仍然无人回答。本 PR 换来的是 CI 余量，并没有让启动变快。这是一个合理的独立后续项，而不是扣住本 PR 的理由——这也正是 #10846 为 routes 套件做过的同样取舍。 需要更正 PR 自述的一点：我在两个 macOS 分片日志中都没有找到它引用的「10.43 秒与 13.12 秒的成功握手」。就我能核实的范围而言，这两个具体数字未被证实。这不改变我的结论——紧贴 9.7–10.0 秒的密集簇，加上同一 runner 上 routes 套件以 60 秒预算通过，已经更直接地确立了资源争用这一判断——但维护者不应把这两个数字当作已验证的事实。 以下几点非阻塞，供后续接手者参考：
结论为批准。真正能评判本次改动的 macOS leg 在我撰写时仍在运行，因此批准将延后至 CI 在 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
chiga0
left a comment
chiga0
left a comment
There was a problem hiding this comment.
Review — fix(test): allow slower ACP initialization in E2E
Tier: Standard — test-infra fix; cross-file context acquired for the flag counterpart.
Checked
--initialize-timeout-msis a real CLI flag declared inpackages/cli/src/commands/serve.ts:589-594with default 10000 ms. The production default inrun-qwen-serve.tsis not touched — the change is confined to the shared E2E harness.qwen-serve-baseline.test.tsimportsspawnDaemonfrom_daemon-harness.tsand benefits from this fix.qwen-serve-routes.test.tsuses its own inline spawn withACP_INITIALIZE_TIMEOUT_MS = 60 salready set via #10846 — no conflict.extraArgsis spread after the hardcoded--initialize-timeout-ms 60000; yargs last-value-wins semantics allow per-caller override if needed.
Not checked (non-blocking)
qwen-serve-streaming.test.tshas its own daemon spawn with no--initialize-timeout-ms; still defaults to 10 s. Could hit the same macOS CI timeout under load. Out of scope for this PR.
No blocking findings. Approval blockers: none.
Reviewed with AI assistance.
qqqys
left a comment
qqqys
left a comment
There was a problem hiding this comment.
Reviewed at head b785a89b. First round on this PR — no historical blocking issues exist, and my Critical-only pass finds none.
The change, verified independently: the shared E2E daemon harness (integration-tests/cli/_daemon-harness.ts) now passes --initialize-timeout-ms 60000 to spawnDaemon. I confirmed the flag is a real qwen serve option at this head (packages/cli/src/commands/serve.ts, numeric, default 10000 ms, "ACP child request timeout, including the initialize handshake"), so the harness is using a supported interface; the production default is untouched — only the E2E harness raises the bound, which is exactly the fix for the observed macOS-shard ACP initialize timeouts (~10 s) confirmed from the linked run's logs by triage. No production code, no runtime surface, no data path is touched.
CI at this head — attribution of the one red lane: Test (ubuntu-latest, Node 22.x) fails on exactly one test (57,888 passed / 1 failed): acpAgent.test.ts > QwenAgent runtime-root pinning choke point > routes every per-request runtime-root pin through runWithPinnedRuntimeBaseDir. That drift guard reds because the merged tree contains packages/cli/src/acp-integration/acpAgent.ts:9200 — return await runWithAcpRuntimeOutputDir(settings, cwd, async () => { — a call shape the guard's pattern rejects, and that line is present on main itself (verified via the main-branch copy of the file). This PR's diff is two lines in the integration-test harness and touches neither acpAgent.ts nor its test, so the failure is pre-existing on main, not introduced by this PR. web-shell E2E Smoke, the no-AK integration lane and review-pr are still pending, which does not gate this review per policy.
qwen-code-dev-bot
left a comment
qwen-code-dev-bot
left a comment
There was a problem hiding this comment.
Reviewed at head b785a89b.
- Two lines in the shared E2E daemon harness: spawn the test daemon with
--initialize-timeout-ms 60000. Verified at this head: the flag exists onservewith number parsing and an unchanged production default when unset, and the placement (before...extraArgs) lets a caller still override per test. The failure class it addresses is the one documented in the linked main E2E run — handshakes of 10.4 s and 13.1 s observed in the same run that killed two shards at ~10 s before any assertion body ran — and it mirrors the budget precedent #10846 set for the serve-routes suite. chiga0's approval at this head cross-checked the flag counterpart; no threads exist to reconcile. - CI facts, stated precisely: 13 checks pass, and the lone
Test (ubuntu-latest)failure is pre-existing onmain, not this PR — it is #10988's source-pinning guard (acpAgent.ts must not name runWithAcpRuntimeOutputDir) failing againstacp-integrationcontent that is byte-identical between this head and current main, so every PR re-running the lane inherits it until someone routes the stray direct call atacpAgent.ts:9200through the choke point (or amends the guard's allow-list). This PR's own surface (integration-tests harness) is validated by the manual E2E run linked in the body. Per the channel convention the call is on the review itself — and the red here cannot be cleared by anything this PR could change.
yiliang114
changed the title
…QwenLM#11034) (QwenLM#11041) The macOS E2E lane reddened twice in one afternoon. The earlier run lost both shards, and the review recorded on QwenLM#11033 read those logs: fourteen `AcpSessionBridge initialize timed out` failures, every one between 9710ms and 9982ms -- the 10s production default being hit under runner contention, not a broken session-creation path. QwenLM#11033 raised the budget at the shared spawn site and its review named the residue: suites that start the daemon inline keep the 10s default, and that is where this flake recurs next. Issue QwenLM#11034 is the next red macOS shard, and two of those inline suites sit in it. Give the remaining inline spawns that run in the default suite and create sessions the same 60s budget the serve-routes suite already uses. The production default is untouched; only CI-spawned daemons ask for more. Left alone deliberately: the startup benchmark stops at the listening line and never creates a session, so it never reaches the handshake; the vs-cli benchmark is gated behind QWEN_BENCHMARK_ENABLED and the first-output benchmark is excluded by the vitest config, so neither runs in CI; and the shared harness already carries the budget from QwenLM#11033. Surrogate reproduction, since the failing job's log is admin-gated and no macOS runner was available: the repo's mock ACP child with a 12s delay injected into its initialize reply, spawned by the bundled daemon the way these suites spawn it, then a session create. Without the budget it fails at 9980ms with `POST /session: AcpSessionBridge initialize timed out after 9980ms` -- the CI signature, inside the recorded 9710-9982ms range; with it the same handshake succeeds at 13.6s. That is also the mutation probe: drop the argument and the failure returns, restore it and it goes. All three touched suites pass locally against the bundled CLI (2 + 11 + 4 tests), and the focused serve-runtime unit tests still witness the flag reaching the bridge options.
|
Released in v0.23.1. |
5 participants
What this PR does
Raises the ACP initialization budget from 10 seconds to 60 seconds only for daemons launched by the shared E2E harness. It also routes the concurrently added session turn-index reader through the existing per-request runtime-root helper. The production
qwen servedefault and turn-index behavior remain unchanged.Why it's needed
Main E2E run 33884903785 failed in two macOS shards before either affected test body ran. Both failures were ACP initialization timeouts at roughly 10 seconds, while the same run recorded successful handshakes taking 10.43 and 13.12 seconds. This gives process-heavy hosted-runner tests the same CI-sized handshake budget already used by the serve-routes E2E suite in #10846.
The first PR CI run then exposed a deterministic main-branch integration gap: #10751 added the turn-index reader after #10988's branch point, while #10988 later introduced the invariant that every caller-supplied-cwd runtime-root pin must use the shared helper. Because required checks were not strict against the latest base, each PR passed independently and their merged result did not. This PR completes the behavior-equivalent delegation intended by #10988 rather than weakening its AST guard.
Reviewer Test Plan
How to verify
Confirm that the baseline session-creation paths and qwen-live prewarm path can complete when ACP initialization takes more than 10 seconds but less than 60 seconds on a loaded macOS runner. Confirm that
qwen serveoutside the E2E harness still uses its 10-second default. Confirm that the turn-index reader resolves the request cwd through the shared runtime-root helper and preserves its existing flush, read, and error behavior.Evidence (Before & After)
Before: the linked main run failed at 9.7–10.0 seconds in the ACP initialize phase before the affected assertions ran.
After: static tracing confirms both failed E2E paths use the shared harness and that its explicit value reaches the ACP bridge. Replaying the failed unit test's TypeScript AST check leaves exactly the one permitted helper delegation.
git diff --checkand Prettier both pass. Manual E2E run 33897680630 provides the hosted-runner verification.Tested on
Environment (optional)
N/A — the failure depends on hosted-runner contention.
Risk & Scope
Linked Issues
Fixes #11030
中文说明
本 PR 做了什么
仅将共享 E2E harness 启动的 daemon 的 ACP 初始化预算从 10 秒提高到 60 秒，同时让并发新增的 session turn-index reader 经过现有的 per-request runtime-root helper。生产环境中的
qwen serve默认值和 turn-index 行为都保持不变。为什么需要
main E2E run 33884903785 的两个 macOS 分片都在受影响测试的正文运行前失败。两处都是约 10 秒触发 ACP 初始化超时，而同一次运行中也记录到了耗时 10.43 秒和 13.12 秒但成功的握手。本修复让进程负载较重的托管 runner 测试使用与 #10846 中 serve-routes E2E 套件相同的 CI 级握手预算。
第一次 PR CI 随后暴露了一个确定性的 main 分支集成缺口：#10751 在 #10988 的分支基线之后新增了 turn-index reader，而 #10988 后来加入了“所有 caller-supplied-cwd runtime-root pin 都必须经过共享 helper”的约束。由于 required checks 没有强制基于最新 base 运行，两个 PR 单独都通过，但合并后的结果没有通过。本 PR 补齐 #10988 原本要求的等价委托，而不是放宽其 AST 约束。
Reviewer Test Plan
如何验证
确认在高负载 macOS runner 上，baseline 的 session 创建路径与 qwen-live 预热路径在 ACP 初始化超过 10 秒但少于 60 秒时可以完成；同时确认 E2E harness 之外的
qwen serve仍使用默认的 10 秒预算。确认 turn-index reader 通过共享 runtime-root helper 解析请求 cwd，并保留现有的 flush、读取和错误处理行为。证据（修改前后）
修改前：链接中的 main 运行在 ACP initialize 阶段约 9.7–10.0 秒时失败，尚未执行受影响的断言。
修改后：静态调用链确认两条 E2E 失败路径都经过共享 harness，显式预算会传递到 ACP bridge。复刻失败单测的 TypeScript AST 检查后，只剩唯一允许的 helper 委托。
git diff --check与 Prettier 均通过；手动 E2E 运行 33897680630 将提供托管 runner 环境的验证。测试平台
环境（可选）
N/A —— 该失败依赖托管 runner 的资源争用。
风险与范围
关联 Issue
Fixes #11030