fix(test): give the serve routes daemon a CI-sized ACP handshake budget - #10846
Conversation
The ACP child's initialize handshake gets 10s by default, a budget sized for an interactive desktop rather than for a shard sharing a 128-core ECS host. The sandbox:docker leg loses a route test to three ~10s handshake timeouts and a 504 while the same commit's sandbox:none shards pass. Nothing in the file asserts the handshake budget, so raise it to 60s for the daemon the harness spawns. Claude-Session: https://claude.ai/code/session_01MCE9CXnMVrX4fUxHpQoUr8
|
|
|
Thanks for the PR! Template looks good ✓ Problem: observed failure with solid evidence — verified against the actual job log of run 33633418567: Direction: aligned. This keeps main green under an oversubscribed shared runner and does not touch product behavior — the product default stays 10s. CHANGELOG has no direct reference to CI harness budgets, but the daemon/ACP handshake reliability area is relevant. Size: not applicable — no core paths touched; 11 added lines, all in one integration test file. Approach: the scope feels right. Raising the budget only for the daemon this file spawns, through the existing Risk: no elevated risk signals — the changed file is a test file, no high-risk-path match. Moving on to code review. 🔍 中文说明感谢贡献! 模板完整 ✓ 问题:已观测到的失败,证据扎实 —— 已从 run 33633418567 的实际 job 日志核实: 方向:对齐。这是让 main 在高争抢的共享 runner 上恢复绿色,不涉及产品行为 —— 产品默认值保持 10 秒。CHANGELOG 没有对 CI harness 预算的直接引用,但 daemon/ACP 握手可靠性这个领域是相关的。 规模:不适用 —— 未触及核心路径;11 行新增全部在一个集成测试文件内。 方案:范围合理。通过现有的 风险:无升级风险信号 —— 改动文件是测试文件,未命中高风险路径。 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewReviewed against the base branch. The flag wiring is real and the value is accepted end to end: No assertion in the file depends on the handshake budget — the only timeout literals there are the boot-readiness timer and a 10s extension-operation poll, both independent of the daemon's ACP budget. Also checked the bound the PR cites: the suite's vitest One wording nit in the PR description, non-blocking: the value does flow into The disclosed tradeoff is real: Testing evidence — the PR's own CI, read via API (no PR code executed in this run)CI on the reviewed commit so far: nothing red. The two suites that actually execute this file — Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行(取最新一次运行),省略 skipped,失败项排在最前。 One honest caveat about the central claim: the flake only reproduces under the shared ECS host's contention on the post-merge 中文说明代码审查已对照基线分支核实。flag 接线真实且取值端到端被接受: 文件中没有任何断言依赖握手预算 —— 仅有的超时字面量是启动就绪计时器和 10 秒的扩展操作轮询,都与 daemon 的 ACP 预算无关。也核对了 PR 所述的上界:该套件 vitest PR 描述中有一处措辞小问题,不阻塞:该值确实会流入 已披露的取舍是真实的: 测试证据 —— 该 PR 自己的 CI,通过 API 读取(本次运行未执行任何 PR 代码)目前该提交上的 CI:无红色。真正执行本文件的两个套件 —— 关于核心结论的一点诚实说明:该 flake 只在共享 ECS 宿主争抢下的 post-merge — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 4/5 — clean, evidence-backed deflake; the only nit is the PR description's imprecise sentence about Stepping back: the failure is real — verified from the actual job log, not just the PR's retelling — and the diagnosis matches what the code does: the bridge spawns a child that loads the CLI bundle and must answer Why not 5: the description says the value "is not part of The one thing nobody can know pre-merge is whether the flake is actually gone — that needs the shared ECS host's contention, which only the post-merge Approval is deferred until CI lands green on this commit; the finalize pass will pin it to the reviewed SHA. 中文说明置信度:4/5 —— 干净、有证据支撑的去 flake;唯一的 nit 是 PR 描述中关于 退后一步看:失败是真实的 —— 已从实际 job 日志核实,而非仅凭 PR 转述 —— 诊断与代码行为一致:bridge 派生加载 CLI bundle 的子进程,必须在 为什么不是 5:描述里说该值"不属于 pre-merge 无法知道的一件事是 flake 是否真的消失 —— 那需要共享 ECS 宿主的争抢,只有 post-merge 的 批准推迟到该提交的 CI 变绿;finalize 流程会把批准钉在所审查的 SHA 上。 — Qwen Code · qwen3.8-max Reviewed at |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not reviewed: reverse audit — stopped before round 1 by the review time budget.
Test Plan (not a blocker): cli/qwen-serve-routes.test.ts:685 — no such file or directory.
中文说明
未审查:反向审计——评审时间预算不足,未能开始第 1 轮。
Test Plan(非阻断):cli/qwen-serve-routes.test.ts:685 — no such file or directory。
— qwen3.8-max via Qwen Code /review (v0.22.3)
chiga0
left a comment
There was a problem hiding this comment.
No blocking findings.
Approval blockers: none.
Tier: Scan (test-only change, one file, 11 lines added — a constant with explanatory comment and one extra argument to the daemon spawn).
Checked:
--initialize-timeout-msflag definition:packages/cli/src/commands/serve.ts:589—type: number, description "ACP child request timeout ... Default: 10000".- Validation in
packages/cli/src/serve/run-qwen-serve.ts:477–4122:MAX_TIMEOUT_MS = 2_147_483_647;isPositiveIntegerMs+assertTimerDelayInRange. 60 000 satisfies both. - No assertions in the test file reference
ACP_INITIALIZE_TIMEOUT_MS; the value does not appear in the capabilities tag list (integration-tests/cli/qwen-serve-routes.test.ts:332+). - Product default (
DEFAULT_INIT_TIMEOUT_MS = 10_000inpackages/acp-bridge/src/bridge.ts) is untouched. - CONTRIBUTING.md: no AI-review ban.
Not covered:
- Working tree unavailable; local integration test run not possible. Scan tier does not require it — this change has no runtime logic, only a numeric flag value.
Cross-check: one existing review (qwen-code-ci-bot, COMMENTED) — zero findings, downgraded from Approve due to CI still running. No finding conflicts; CI state is outside this review's verdict per the platform adapter.
Reviewed with AI assistance.
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
|
Released in v0.23.0. |
What this PR does
This PR raises the ACP
initializehandshake budget for the daemon thatintegration-tests/cli/qwen-serve-routes.test.tsspawns, from the 10-second product default to 60 seconds, by passing--initialize-timeout-mson the harness'sqwen servecommand line. No assertion in the file depends on that budget, and the product default is untouched.Why it's needed
Run 33633418567 on
76c32fd5turned main red on one shard:E2E Test (Linux) - sandbox:docker - shard 3/3,cli/qwen-serve-routes.test.ts > qwen serve — POST /session validation + concurrent coalescing > honors and reserves a normalized caller-supplied session ID,AssertionError: expected 504 to be 200at line 685.The 504 is not a route defect. The underlying error is
DaemonHttpError: POST /session: AcpSessionBridge initialize timed out after 9993ms, and the test burned 36.5s across three attempts (retry x2) — three consecutive ~10s handshake timeouts againstDEFAULT_INIT_TIMEOUT_MS = 10_000inpackages/acp-bridge/src/bridge.ts. The same commit's threesandbox:noneshards all passed.This is the second occurrence of that exact signature, and both landed on the
sandbox:dockerleg: #10605's investigation attributed run 33351032808 to "three 10-second ACP channel initialization timeouts" (it surfaced as HTTP 500 back then; #10404 has since reclassified initialization timeouts to 504). The daemon has to spawn a Node child that loads the CLI bundle and answerinitializewithin 10s, on a shard that shares a 128-core ECS host with roughly 30 other logical runners. Ten seconds is a budget for an interactive desktop; under that contention it is a coin flip, and vitest's two retries make it a triple coin flip rather than a fix.The alternative — treating a slow subprocess handshake on an oversubscribed shared runner as a product bug — is not supported by the evidence: nothing in the failure points at the route, the session-ID normalization it exercises, or the daemon's behavior once the child answers.
Reviewer Test Plan
How to verify
--initialize-timeout-ms 60000.packages/cli/src/commands/serve.tsdefines the flag as a number andrun-qwen-serve.tsonly rejects values above the maximum JS timer delay, so 60000 is accepted and validated. The value is not part of/capabilities, so the file's exact-equality capabilities baseline is unchanged.npm run build && npm run bundle, thennpx vitest run --root ./integration-tests cli/qwen-serve-routes.test.ts— the suite passes with the daemon booting normally; the raised budget only changes how long a handshake may take before it is abandoned.sandbox:dockershard that owns this file over the next few main runs and expect noAcpSessionBridge initialize timed outfailure.Evidence (Before & After)
Before:
expected 504 to be 200atcli/qwen-serve-routes.test.ts:685, causeAcpSessionBridge initialize timed out after 9993ms, 36534ms across three attempts, shard duration 1061.93s, onecs-qwen-hk5-30.After: not reproducible locally — the failure needs the shared ECS host's contention. The change is confined to the harness's command line and is covered by the run above.
Tested on
Environment (optional)
Prettier and source inspection only; the E2E leg that exhibits the failure runs post-merge on
main.Risk & Scope
--initialize-timeout-msraises every ACP child request timeout in that daemon, not just the handshake, so a genuinely hung child in this file would now take 60s to surface instead of 10s. No test here asserts a timeout, and the file's slowest case is bounded by vitest's own timeouts.qwen serve(_daemon-harness.tsand its callers) keep the 10s default — they have not shown this signature, and widening the change without evidence would be speculative. If the same failure appears there, the harness is the place to fix it once.Linked Issues
Fixes #10833
中文说明
本 PR 做了什么
本 PR 把
integration-tests/cli/qwen-serve-routes.test.ts启动的 daemon 的 ACPinitialize握手预算,从产品默认的 10 秒提高到 60 秒,方式是在 harness 的qwen serve命令行上传入--initialize-timeout-ms。该文件中没有任何断言依赖这个预算,产品默认值也未改动。为什么需要
76c32fd5上的 run 33633418567 让 main 变红,失败的是一个 shard:E2E Test (Linux) - sandbox:docker - shard 3/3,用例cli/qwen-serve-routes.test.ts > qwen serve — POST /session validation + concurrent coalescing > honors and reserves a normalized caller-supplied session ID,报错AssertionError: expected 504 to be 200(第 685 行)。504 并不是路由缺陷。底层错误是
DaemonHttpError: POST /session: AcpSessionBridge initialize timed out after 9993ms,该用例在三次尝试(retry x2)中共耗时 36.5 秒 —— 即针对packages/acp-bridge/src/bridge.ts中DEFAULT_INIT_TIMEOUT_MS = 10_000的三次连续约 10 秒握手超时。同一 commit 的三个sandbox:noneshard 全部通过。这是该特征第二次出现,且两次都落在
sandbox:docker通道上:#10605 的排查曾把 run 33351032808 归因为"三次 10 秒的 ACP channel 初始化超时"(当时表现为 HTTP 500,#10404 之后把初始化超时重新分类为 504)。daemon 需要派生一个加载 CLI bundle 的 Node 子进程,并在 10 秒内回应initialize,而这个 shard 与大约 30 个其它逻辑 runner 共享一台 128 核 ECS 宿主。10 秒是给交互式桌面场景的预算;在这种争抢下它就是抛硬币,而 vitest 的两次重试只是把它变成抛三次硬币,并不是修复。另一种解释 —— 把超载共享 runner 上子进程握手变慢当作产品缺陷 —— 没有证据支持:失败信息里没有任何一处指向该路由、它验证的 session ID 归一化,或子进程回应之后 daemon 的行为。
Reviewer Test Plan
如何验证
--initialize-timeout-ms 60000。packages/cli/src/commands/serve.ts将该 flag 定义为 number,run-qwen-serve.ts只拒绝超过 JS 定时器最大延迟的值,因此 60000 会被接受并通过校验。该值不属于/capabilities,所以该文件的 capabilities 精确相等断言不受影响。npm run build && npm run bundle,然后npx vitest run --root ./integration-tests cli/qwen-serve-routes.test.ts—— daemon 正常启动、用例通过;提高的预算只改变握手在被放弃前允许的时长。sandbox:dockershard,预期不再出现AcpSessionBridge initialize timed out失败。Evidence(修复前后)
修复前:
cli/qwen-serve-routes.test.ts:685报expected 504 to be 200,原因为AcpSessionBridge initialize timed out after 9993ms,三次尝试共 36534ms,shard 总时长 1061.93s,运行于ecs-qwen-hk5-30。修复后:本地无法复现 —— 该失败依赖共享 ECS 宿主的争抢。改动仅限于 harness 的命令行,由上述 run 提供依据。
测试环境
Environment(可选)
仅做了 Prettier 与源码检查;出现该失败的 E2E 通道在合入
main后才运行。风险与范围
--initialize-timeout-ms会提高该 daemon 中所有 ACP 子进程请求的超时,而不只是握手,因此该文件中若真的出现子进程挂死,暴露时间会从 10 秒变为 60 秒。这里没有任何用例断言超时行为,且该文件最慢的情形受 vitest 自身超时约束。qwen serve的集成测试(_daemon-harness.ts及其调用方)保持 10 秒默认值 —— 它们没有出现过该特征,在没有证据的情况下扩大改动属于臆测。如果同样的失败出现在那里,harness 才是一次性修好的地方。关联 Issue
Fixes #10833
https://claude.ai/code/session_01MCE9CXnMVrX4fUxHpQoUr8