Conversation
…eaming The serve streaming suite spawns its daemon manually and inherited the 10s initialize-timeout-ms default, which trips on loaded macOS CI runners when these tests restart or re-attach a daemon child. Two failed main runs (QwenLM#11034, QwenLM#11043) show the same signature: AcpSessionBridge initialize timed out after ~10s on exactly the restart/reload tests, while green runs interleave on the same day, so this is runner slowness rather than a regression. The shared daemon harness already passes 60000; align this spawn with it. Test-side only, the product default is unchanged. Signed-off-by: Yufeng He <40085740+he-yufeng@users.noreply.github.com>
|
Thanks for the PR — and for the diagnosis write-up on #11043, which made this easy to check rather than take on trust. Template looks good ✓ — every required heading is present, and Risk & Scope volunteers the 60s-vs-10s tradeoff before a reviewer has to go looking for it. Problem — observed, and I verified it against the real logs. #11043's Direction — aligned. Test-side only, the production 10s default stays put, and 60s isn't a number you invented: Size — not applicable. One file, +5/-0, no core paths touched. Approach — the change itself is minimal and I'd write it the same way. Two scope points to raise before the code review, both about reach rather than the diff:
Risk — no elevated risk signals. Stage 1e's high-risk path list doesn't match; the only changed file is a Flagging those two for discussion and moving on to code review, since the diff is verifiable on its own merits. 🔍 中文说明感谢贡献,也感谢你在 #11043 上写的诊断 —— 它让核对成为可能,而不必只凭描述相信。 模板完整 ✓ —— 所有必需标题齐全,Risk & Scope 也主动说明了 60s vs 10s 的取舍,不用评审者自己去翻。 问题 —— 已观测,而且我对照真实日志核实过。#11043 的 方向 —— 对齐。仅改测试侧,生产环境 10s 默认值保持不变,而且 60s 不是你自创的数字: 规模 —— 不适用。单文件,+5/-0,未触及核心路径。 方案 —— 改动本身已是最小化,我也会这么写。有两点范围问题想在代码审查前提出,都关乎覆盖面而非 diff:
风险 —— 无升级风险信号。Stage 1e 的高风险路径清单未命中;唯一改动文件是 以上两点先提出来讨论;代码审查继续进行,因为 diff 本身可以独立验证。🔍 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
Code reviewI wrote down what I'd do before opening the diff: the root cause is that inline The first half is exactly what this PR does. The second half — the sweep — is what it doesn't do, and the description asserts the sweep is unnecessary. That's the one place I'd push back; the grep and line numbers are in the Stage 1 comment, so I won't repeat the case here. On the diff itself, every load-bearing claim checked out:
One stylistic note, not a finding: No correctness bugs, no security concerns, no regressions. Test-side only, so the product surface is untouched. CI test evidenceThis run carried no local testing — the review is static and I did not build or run any PR-derived code. The evidence below is the PR's own CI, fetched once through the API for Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行(取最新一次运行),省略 skipped,失败项排在最前。 Nothing red. The The part that matters is what's skipped: not verified: that the suite stops failing on a loaded macOS runner. It can't be verified from this PR — the lane that would come closest is skipped on forks, and a statistical flake (main already interleaves green and red runs of the same lane) isn't settled by any single run in either direction. Sandboxed verification would partly settle this: 中文说明代码审查 —— 我在打开 diff 前先写下了自己的方案:根因是内联的 前半部分与本 PR 完全一致。后半部分(全量扫描)本 PR 没有做,而描述断言无需扫描 —— 这是我唯一想反驳的地方;grep 结果与行号已在 Stage 1 评论中,此处不再重复论证。 diff 本身,所有关键论断都核对通过:
一点风格说明,不构成 finding: 无正确性缺陷、无安全隐患、无回归。仅测试侧改动,产品接口未受影响。 CI 测试证据 —— 本次运行没有本地测试 —— 审查是静态的,我没有构建或运行任何 PR 派生代码。上表证据来自 PR 自身的 CI,通过 API 对 无红色项。 关键在于被跳过的部分: 未验证:在繁忙的 macOS runner 上该套件是否不再失败。这一点无法从本 PR 验证 —— 最接近的那条 lane 在 fork 上被跳过,而统计性 flake(main 上同一 lane 的绿色与红色运行本就交替出现)不会被任何单次运行判定,无论哪个方向。 沙箱验证可部分解决: — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
|
Confidence: 3/5 — the five lines are correct and I verified every load-bearing claim behind them, but two questions I can't answer from the diff decide whether this is the PR that should land. Stepping back. The change is right. I confirmed the flag exists and is wired end to end, that the default it replaces really is 10s, that the harness line the Test Plan points at says what it says, and — the part I expected to bite — that 60000 leaves the session-restore deadline untouched, because It isn't the only PR open, and that's the first question. #11041 patches this same file at this same insertion point with the same flag and value, and additionally covers The second question sits underneath it. The description's scoping claim — that this is the only serve E2E file spawning its daemon manually without the harness budget — is false in-tree, and it's load-bearing: it's the stated reason the PR stops at one file. I'd rather the author correct that sentence than have a maintainer approve believing the flake class is closed, because So I'm neither approving nor requesting changes. The diff doesn't deserve request-changes — nothing in it is wrong, and the author diagnosed this independently and honestly, volunteering the 60s tradeoff unprompted. But adding the bot's approval would mean voting to ship one of two conflicting fixes before a human has chosen between them, on a PR whose own CI skips both the platform and the suite at issue. ⏸️ Deferring to a maintainer. Two things need a human call: (1) which of #11051 and #11041 should land, given the identical hunk in 中文说明Confidence: 3/5 —— 这 5 行代码是正确的,其背后每一个关键论断我都核实过;但有两个我无法从 diff 回答的问题,决定了该合并的是不是这个 PR。 退一步看。改动本身是对的。我确认了该参数存在且端到端接通、它替换的默认值确实是 10s、Test Plan 指向的 harness 那一行确实如描述所写;也确认了我原以为最可能出问题的一点 —— 60000 不会改变 session-restore 截止时间,因为 但它不是唯一开着的 PR,这是第一个问题。#11041 在同一文件、同一插入点打了同样的参数与数值,并额外覆盖了 第二个问题在它底下。描述中的范围论断 —— 本文件是唯一手动启动 daemon 且未使用 harness 预算的 serve E2E 文件 —— 在代码树中不成立,而且它是承重的:它正是本 PR 止步于单个文件所说的理由。我更希望作者修正这句话,而不是让维护者在以为这类 flake 已收敛的情况下批准 —— 因为 所以我既不批准,也不请求修改。这段 diff 不值得 request-changes —— 其中没有错的东西,作者是独立且诚实地完成诊断的,还主动说明了 60s 的取舍。但加上机器人的批准,就等于在人类尚未在两个互相冲突的修法之间做出选择前,为其中之一投票支持;而这个 PR 自己的 CI 恰好跳过了涉事平台与涉事套件。 ⏸️ 转交维护者处理。 两件事需要人来定:(1)鉴于 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
中文说明
未审查(原文为英文):build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
— qwen3.8-max via Qwen Code /review (v0.23.0)
qqqys
left a comment
There was a problem hiding this comment.
COMMENT — the change is fully superseded by merged #11041 and now conflicts with main.
- Head
9357c991is based on the pre-#11041 blob ofintegration-tests/cli/qwen-serve-streaming.test.ts. #11041 merged at 2026-09-05T01:14:44Z and brought the exact same fix to this file, in a better-supported form: a sharedACP_INITIALIZE_TIMEOUT_MS = 60_000constant (main line 80) and the same--initialize-timeout-msspawn pair at the identical site (main lines 358-359). Verified against main's current file content. - Consequently GitHub reports
mergeable: CONFLICTING— the two edits collide in the same region — and a conflict-resolved version of this PR would contain zero delta against main (it may even reintroduce a redundant inline'60000'literal beside the constant #11041 established). - #11041 also covered the sibling suites (
qwen-serve-channel-workers,qwen-serve-client-mcp) with the same budget, so this PR's whole scope is contained.
Suggested next step: close this PR as superseded by #11041. Nothing to salvage — if #11043's macOS restart path needs anything beyond what #11041 shipped, a fresh issue against the merged state would be the cleaner carrier.
For completeness: the lone red on this head is Test (ubuntu-latest), whose unit-suite scope never executes the only file this PR touches (integration-tests run in dedicated lanes, all green/skipped here); the failure matches the main-side reds seen across other PR heads this week and is not attributed to this change.
|
@qwen-code /resolve |
…QwenLM#11041 Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
Qwen Code resolved the merge conflicts and pushed the branch update. Merge summary — PR #11051 ← origin/mainRoot causeMain merged b0a463c ( Textual or semanticSemantic — both sides modify the same logic (the same spawn argument list), not merely adjacent lines. Taking both sides would pass // top of file (auto-merged from main)
const ACP_INITIALIZE_TIMEOUT_MS = 60_000;
// beforeAll spawn args (resolved conflict)
'--workspace',
workspaceDir,
'--initialize-timeout-ms',
String(ACP_INITIALIZE_TIMEOUT_MS),
],The PR's inline rationale comment was dropped, not lost: main's comment on the constant carries the same reasoning (10s default is a desktop budget; match What is load-bearing
What I could not verifyNo build/tests run (per instructions). Net effect: the PR's entire 5-line diff is absorbed, so the merged tree is byte-identical to 中文说明根因:main 合入了 b0a463c(#11034/#11041),它对同一文件 语义冲突:双方改的是同一段逻辑(同一处启动参数),若两边都保留会重复传参。解决方式采用 main 的常量写法( 关键点:该文件只有一处 未验证项:按指令未运行构建或测试。由于 PR 的全部 5 行改动均被 main 吸收,合并后的树与 |
Local verification — real daemon, real
|
| run | spawn args | injected delay | result |
|---|---|---|---|
| baseline | as on main |
none | 11/11 pass, 12.94 s |
| arm 1 | as on main (flag present) |
12 s | 11/11 pass, 97.05 s |
| arm 2 | flag surgically removed (pre-#11041 args) | 12 s | 11/11 fail |
Every arm-2 failure is DaemonHttpError: POST /session: AcpSessionBridge initialize timed out after ~9.99s. The negative control is what makes this meaningful: the suite is green under the delay because of these two lines, not merely alongside them.
4. No collateral damage on the restore budget
The one thing worth checking about raising an initialize budget is whether it drags the session load/resume budget with it. It does not — probing the built acp-bridge dist directly:
DEFAULT_SESSION_RESTORE_TIMEOUT_MS = 60000
unset (production default) -> restore budget 60000 ms
initializeTimeoutMs: 10000 (old default) -> restore budget 60000 ms
initializeTimeoutMs: 60000 (the flag) -> restore budget 60000 ms
initializeTimeoutMs: 90000 -> restore budget 90000 ms
resolveSessionRestoreTimeoutMs takes Math.max(initializeTimeoutMs, 60000), so 60000 is a fixed point. And the production default is genuinely untouched: DEFAULT_INIT_TIMEOUT_MS = 10_000 (packages/acp-bridge/src/bridge.ts:2587) — arm A above observed it live at 9991 ms.
5. One residual gap the sweep missed (non-blocking, for a follow-up)
The triage note said the benchmark suites were left alone "since they don't create sessions". Two of the three do. The one that matters:
spawnDaemonWithTime() (integration-tests/cli/_daemon-benchmark-helpers.ts:297) builds its own argument list and, unlike spawnDaemon() in _daemon-harness.ts:121, omits --initialize-timeout-ms. qwen-daemon-vs-cli-benchmark.test.ts:312 then calls createOrAttachSession on that daemon. The same test is its own A/B: iterations 0..n-2 go through spawnDaemon() (60 s), the last one through spawnDaemonWithTime() (10 s).
Under the identical 12 s delay the harness-spawned iterations pass and the timed one fails with the same signature; adding the same two lines to that helper turns the test green. This is not urgent: the file is gated behind QWEN_BENCHMARK_ENABLED=1, which appears nowhere under .github/, so it never runs in CI today.
For completeness, the other two benchmark files are genuinely safe, but for different reasons than stated: qwen-daemon-startup-benchmark.test.ts never creates a session, so it never reaches the handshake at all; qwen-daemon-first-output-benchmark.test.ts does create sessions but is excluded from the default config (**/qwen-daemon-first-output-benchmark*) and runs under vitest.firstoutput.config.ts.
6. Housekeeping
- Main CI failed: E2E Tests on 74fe3a659dde #11043 is still open, and this PR is the only thing cross-referencing it. Its failing commit 74fe3a6 (2026-09-04 18:33 UTC) predates b0a463c (2026-09-05 01:14 UTC), so
mainalready fixes it — but closing this PR silently would leave the issue orphaned. Worth closing Main CI failed: E2E Tests on 74fe3a659dde #11043 with a pointer at b0a463c. - The earlier "
⚠️ Downgraded from Approve to Comment: CI failing: Test (ubuntu-latest, Node 22.x)" is stale — that job passes on the current head. - Nothing here is a criticism of the PR. It diagnosed the flake correctly and reached the same fix independently; it simply lost a race by about 75 minutes.
中文说明
本地验证 —— 真实 daemon、真实 qwen --acp 子进程、可复现的 flake
我在本地搭了真实验证环境,而不是只读 diff。因为这里真正值得回答的问题不是"这五行有没有写对",而是这个预算到底起不起作用,以及在 @qwen-code /resolve 把 main 合进来之后,这个 PR 还剩下什么内容。
结论:改动是对的,而且已经在 main 上了。本 PR 现在不携带任何内容,建议按"已被取代"关闭而不是合并;同时把仍然处于 open 状态、且除本 PR 外无人引用的 #11043 一并关掉。
环境:Debian 13、Linux 6.12.63 x86_64、16 核、Node v22.22.2、vitest 3.2.7。在 origin/main c39e83e 上从源码完整构建(npm run build -- --cli-only && npm run bundle);PR head 38103fc。
1. 本 PR 已无独有内容。 git diff origin/main...38103fc535 为空。更强的证明:PR head 的 tree(7c83a94a)与 merge base e3d26283 的 tree 完全相同,而 e3d26283 已是 origin/main 的祖先;PR 唯一改过的那个文件两侧 blob 逐字节相同(c730ea79);b0a463c6ec(#11041)已是 PR head 的祖先。合并只会给 main 追加两个不改变任何内容的提交。
2. 这个预算确实是承重的 —— 在协议线上验证,而非靠阅读。 我在 daemon 与 ACP 子进程之间插了一个 shim(QWEN_CLI_ENTRY → shim → 真实 qwen --acp):NDJSON JSON-RPC 双向逐帧透传,只改一件事 —— initialize 响应的到达时间。这正是繁忙共享 runner 的效果,但完全确定。同一 bundle、同一真实子进程、同样注入 20 秒延迟,只变一个变量:不带 flag → 504、AcpSessionBridge initialize timed out after 9991ms(与 #11043 中 qwen-serve-streaming.test.ts:519 的 9996ms 同类同阶段同预算);带 --initialize-timeout-ms 60000 → 200,21272ms 建会话成功。
3. 套件级 A/B(含反向对照)。 对构建产物跑了三次真实套件:基线(无延迟)11/11 通过、12.94s;arm 1(main/PR head,带 flag)+12s 延迟,11/11 通过、97.05s;arm 2(外科式删掉 flag,还原 #11041 之前的参数)+同样 12s 延迟,11/11 全挂,每条都是 DaemonHttpError: POST /session: AcpSessionBridge initialize timed out after ~9.99s。反向对照才是关键:套件在延迟下是绿的,是因为这两行,而不是恰好与它同时为绿。
4. 对 restore 预算无副作用。 抬高 initialize 预算唯一值得担心的是会不会把 session load/resume 预算一起带偏。直接探测构建后的 acp-bridge dist:未设置 → 60000;10000 → 60000;60000 → 60000;90000 → 90000。resolveSessionRestoreTimeoutMs 取 Math.max(initializeTimeoutMs, 60000),所以 60000 是不动点。产品默认值也确实没动:DEFAULT_INIT_TIMEOUT_MS = 10_000(packages/acp-bridge/src/bridge.ts:2587)—— 上面 arm A 在 9991ms 处实测到了它。
5. 遗漏的一处(非阻塞,可另开 PR)。 triage 说 benchmark 套件"不创建会话"所以不用管,三个里有两个其实会创建。要紧的一个是:spawnDaemonWithTime()(_daemon-benchmark-helpers.ts:297)自己拼参数列表,与 _daemon-harness.ts:121 的 spawnDaemon() 不同,没有传 --initialize-timeout-ms;而 qwen-daemon-vs-cli-benchmark.test.ts:312 会在这个 daemon 上 createOrAttachSession。这条用例本身就是个 A/B:第 0..n-2 轮走 spawnDaemon()(60s),最后一轮走 spawnDaemonWithTime()(10s)。同样注入 12s 延迟:前面几轮通过,最后一轮以同样签名失败;给那个 helper 补上同样两行即转绿。不紧急:该文件由 QWEN_BENCHMARK_ENABLED=1 把守,而这个变量在 .github/ 下根本不存在,所以目前不会在 CI 里跑。另外两个 benchmark 文件确实安全,但理由与 triage 所述不同:qwen-daemon-startup-benchmark.test.ts 从不创建会话,压根到不了握手;qwen-daemon-first-output-benchmark.test.ts 会创建会话,但被默认配置排除(**/qwen-daemon-first-output-benchmark*),跑在 vitest.firstoutput.config.ts 下。
6. 收尾事项。 #11043 仍然 open,且只有本 PR 交叉引用了它;其失败提交 74fe3a6(2026-09-04 18:33 UTC)早于 b0a463c(2026-09-05 01:14 UTC),main 已经修好了 —— 但如果只是悄悄关掉本 PR,这个 issue 会被落下,建议引用 b0a463c 一并关闭。另外早先那条"
Verified locally with Claude Code (model: Claude Opus 5, 1M context). Evidence images: wenshao/qwen-code@6861426 (branch assets-pr11051).
|
Confirmed superseded: current main passes |




What this PR does
Passes
--initialize-timeout-ms 60000in theqwen-serve-streaming.test.tsdaemon spawn, aligning it with the shared daemon harness (integration-tests/cli/_daemon-harness.ts), instead of letting that suite inherit the 10000ms default.Why it's needed
Two failed main E2E runs (#11034, #11043) show the same signature on different commits:
AcpSessionBridge initialize timed out after ~10sin this suite's daemon restart/reload tests. Green runs interleave with the failures on the same day, so this is shared macOS runner slowness tripping the 10s budget, not a product regression. The streaming suite is the only serve E2E file that spawns its daemon manually without the harness's larger budget. Test-side only; the product default is unchanged.Reviewer Test Plan
How to verify
Confirm the argument list change in
integration-tests/cli/qwen-serve-streaming.test.tsand that_daemon-harness.ts:121already passes the same value. Expected: the suite no longer fails with 10sAcpSessionBridge initialize timed outon loaded macOS runners; behavior of the tests themselves is unchanged.Evidence (Before & After)
N/A (test infrastructure only)
Tested on
Environment (optional)
Local vitest run of
integration-tests/cli/qwen-serve-streaming.test.tson macOS arm64 against a full build.Risk & Scope
Linked Issues
Fixes #11043
中文说明
本 PR 做了什么
在
qwen-serve-streaming.test.ts的 daemon 启动参数中传入--initialize-timeout-ms 60000,与共享 daemon harness(integration-tests/cli/_daemon-harness.ts)保持一致,不再让该测试套件继承默认的 10000ms。为什么需要
main 分支两次失败的 E2E 运行(#11034、#11043)在不同提交上呈现相同签名:本套件中重启/重连 daemon 的测试报
AcpSessionBridge initialize timed out after ~10s。同一天内绿色运行与失败交替出现,说明这是共享 macOS runner 变慢触发了 10s 预算,而非产品回归。该套件是唯一手动启动 daemon 且未使用 harness 大预算的 serve E2E 文件。仅改动测试侧,产品默认值不变。评审验证计划
如何验证
查看
integration-tests/cli/qwen-serve-streaming.test.ts的参数变更,并确认_daemon-harness.ts:121已传入相同数值。预期:在繁忙的 macOS runner 上本套件不再因 10s 初始化超时而失败;测试本身行为不变。证据(前后对比)
N/A(仅测试基础设施)
测试平台
macOS 已测,Windows/Linux 未本地测(由 CI 覆盖)。
环境(可选)
在 macOS arm64 上对完整构建本地运行了
integration-tests/cli/qwen-serve-streaming.test.ts。风险与范围
关联 Issue
Fixes #11043