fix(serve): classify channel initialization timeouts - #10404
Conversation
|
✅ Qwen Triage finished — CI landed green on ✅ Qwen Triage 已完成 —— |
|
Thanks for the PR! Template looks good ✓ Problem: real and verified in code. Direction: aligned. This completes a classification design that is already in the codebase: Size: not applicable — no core module paths touched (18 production lines in Approach: scope is right. The new branch mirrors the established sibling pattern exactly, and Risk: no elevated risk signals. One observation for reviewers: the branch lives in the shared Moving on to code review. 🔍 中文说明感谢贡献! 模板完整 ✓ 问题:真实存在,且已在代码中验证。 方向:对齐。本 PR 补全了代码库中已有的分类设计: 规模:不适用——未触及核心模块路径(生产代码 18 行,位于 方案:范围合理。新分支完全沿用既有兄弟分支的模式,且 风险:无升级风险信号。供 reviewer 参考的一点:该分支位于共享的 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewReviewed at the commit named in the footer. My independent take before reading the diff was: classify the timeout in the shared Things I checked specifically, all clean:
No blockers, no convention violations. Test evidence (this PR's own CI, read via the API — PR code was not executed here)At review time, 15 checks are green, 12 skipped, none failed. The two jobs that matter most for this change — Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行(取最新一次运行),省略 skipped,失败项排在最前。 Not verified: real-scenario (tmux) testing — this is an unattended CI run, so no PR code was built or executed; a maintainer can trigger the isolated jobs below. The author's Evidence section reports the focused test failed Sandboxed verification would settle the remaining gap: 中文说明代码审查:独立评估与 PR 做法一致——在共享的 测试证据:通过 API 读取本 PR 自身的 CI(未在此执行 PR 代码)。评审时 15 项通过、12 项跳过、无失败;最关键的两个任务—— 沙箱验证可补齐剩余缺口: — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 4/5 — clean, minimal fix that completes an existing design; the only gaps are CI still in flight and the end-to-end real-bridge path being unexercised (both handled below). Stepping back: this PR does one thing and does it the way the codebase already asks for it to be done. The problem is real and code-verified — an What I like about it: it is backward-compatible by construction — aware clients gain a safe-retry signal, conservative clients that treat every 5xx mutation as ambiguous keep failing closed — and it reuses the Two nits, neither blocking: the protocol doc documents only the The two remaining 中文说明总体评价:这是一个干净、最小的修复,补全了既有设计;仅有的缺口是 CI 尚未结束,以及端到端真实 bridge 路径未被演练(两者均在下面处理)。 退一步看:这个 PR 只做一件事,而且完全按照代码库既有的方式去做。问题真实存在且已在代码中验证—— 值得肯定之处:改动在构造上向后兼容——理解协议的客户端获得安全重试信号,把所有 5xx 写操作视为结果未知的保守客户端继续 fail closed;并且复用了诊断层已经赋值的 两个小问题,均不阻塞:协议文档只记录了 评审时两个 — Qwen Code · qwen3.8-max Reviewed at |
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. |
🩺 serve daemon A/BBuilt the PR base vs this PR head ✅ No response changes against the PR base across 12 scenario(s). — Qwen Code · serve A/B |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship — CI landed green after the review. ✅
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not explored to full depth (tool budget reached): "agent 6b": running the new regression test as a live positive control — the worktree has no node_modules / dist , and install+build was not attempted within budget; verif….
Not reviewed: reverse audit — stopped before round 3 by the review time budget.
中文说明
仅完成部分审查,审查缺口已披露。
未探索到全部深度(达到工具调用预算):"agent 6b":running the new regression test as a live positive control — the worktree has no node_modules / dist , and install+build was not attempted within budget; verif…。
未审查:反向审计——评审时间预算不足,未能开始第 3 轮。
— qwen3.8-max via Qwen Code /review (v0.22.2)
CanReader
left a comment
There was a problem hiding this comment.
The typed error shape is good and the reasoning about sideEffectPossible is sound. One scoping issue worth deciding on before this lands.
First, the thing I checked because it would have been a silent regression: this new branch sits above the SessionRestoreTimeoutError branch in sendBridgeError, and SessionRestoreTimeoutError extends BridgeTimeoutError (packages/acp-bridge/src/status.ts:60). An instanceof BridgeTimeoutError check placed first would have swallowed every restore timeout and changed its response shape. It does not, because the branch also requires err.label === 'initialize' and the subclass's label is session/load / session/resume (status.ts:65). So the ordering is safe — but only via that label check, which is load-bearing in a way the code does not say out loud. Either move the more specific SessionRestoreTimeoutError branch above it, or note in the comment that the label guard is what keeps the subclass from matching.
The scoping question: sendBridgeError has 136 call sites across the serve routes, and this branch fires for all of them, but the documentation and the test only describe POST /session. The doc paragraph reads as a contract for session creation ("channel initialization precedes the ACP newSession request"), while in practice a client could now receive phase: "channel.initialize" with sideEffectPossible: false from a skills-install or workspace-auth route. I think the claim is still true there — an initialize timeout means the handshake never completed, so nothing was dispatched on any route — but a client reading the protocol doc will not know the contract applies, which defeats the point of publishing a structured shape.
Two ways to resolve it: narrow the branch to the session-creation path, or broaden the doc to say the contract holds for any route whose bridge call fails during channel initialization. The second seems more useful and matches the actual behaviour; it just needs the doc to say so, plus a test on one non-session route so the broader contract is pinned rather than incidental.
BridgeTimeoutError is constructed generically (bridge.ts:13311) from a label parameter, so nothing stops a future call site from producing another initialize-labelled timeout in a context where the claim does not hold. Pinning the broader contract with a test is what would catch that.
qwen-code-dev-bot
left a comment
There was a problem hiding this comment.
Gate review at head ac36ea9c (CI green at this head; decision not blocked on CI). The two prior-review Criticals are unresolved at this head — zero replies on their threads and no commits since the CHANGES_REQUESTED — and I verified both against the code myself:
-
sendBridgeErrorblanket safe-retry claim across ALL routes (error-response.ts:200). The new branch fires for anyBridgeTimeoutErrorwithlabel === 'initialize', in the shared error path used by every route, and unconditionally emitsretryable: true, sideEffectPossible: false, phase: 'channel.initialize'. But onPOST /session/:id/branch/POST /session/:id/side-taskthe same error can surface fromensureChannel()after the fork mutation is durably committed (see the restore-flowensureChannel()call sites inpackages/acp-bridge/src/bridge.ts, e.g. :7639/:10332 — not wrapped in the rollback catch). A client that trustssideEffectPossible: falseand retries would then be retrying over an already-committed mutation. Expected fix: scope the claim — only reportretryable/sideEffectPossible/phaseon routes where initialize provably precedes any mutation (route-level opt-in or explicit phase evidence from the caller), and fall back tosideEffectPossible: "unknown"(or the current generic shape) elsewhere. -
Protocol doc overstates authoritativeness (qwen-serve-protocol.md:123). The added sentence declares
sideEffectPossible: falseas authoritative, yetPOST /sessionbodies carryingbranch/worktreeperform git mutations (createBranch moves the shared HEAD; worktree creates one) before channel initialize, and the failure-path rollback is best-effort only (rollbackBranchCreationswallows git failures; worktree removal is.catch(() => {})). Either narrow the sentence to the pre-dispatch initialize timeout and state the mutation-route exception, or don't claim authoritativeness.
Non-blocking, from the same review round (fix if touching the diff again): pin the negative case in server.test.ts (a BridgeTimeoutError with a non-initialize label, e.g. newSession, must NOT take this branch — none of the current tests covers it), and avoid pinning the operator-configurable --initialize-timeout-ms budget as literal 10000 values in the doc example.
--- 中文:在 head ac36ea9c 复核,前轮两条 Critical 均未处理(线程零回复、RC 后无新提交),且我已对代码实证:① 共享 sendBridgeError 的新分支对所有路由无条件给出 retryable:true / sideEffectPossible:false,但 branch/side-task 路由的 initialize 超时可发生在 fork 变更已持久提交之后,客户端据此重试会落在已提交变更上——请把该承诺限定在"initialize 确定先于任何变更"的路径,其余路径报 unknown;② 协议文档把 sideEffectPossible:false 写成权威,而 POST /session 的 branch/worktree 在初始化前就做 git 变更、回滚仅尽力而为——请收窄表述。两条建议项(负向用例测试、文档示例别钉死 10000ms)随手可带上。处理后可复审。
The initialize-timeout mapping in sendBridgeError promised retryable:true / sideEffectPossible:false for every route, but two paths mutate before or around the initialize handshake: - POST /session with branch/worktree runs createBranch (moving the shared HEAD) or creates a worktree before spawn, and the rollback on failure is best-effort — a failed checkout rollback leaves the repo on the new branch while the response claims no side effect. - POST /session/:id/branch and /side-task can time out on a replacement channel's initialize after the fork was already durably committed; a contract-trusting retry would commit a duplicate fork. The safe-retry shape is now emitted only when the caller asserts via the new initPrecedesMutations context flag that initialization strictly precedes every durable mutation — POST /session sets it only when no branch/worktree was prepared. All other paths keep the typed init_timeout code, phase, and timeoutMs but omit Retry-After, retryable, and sideEffectPossible, reporting an unknown outcome. The protocol doc narrows the authoritative claim accordingly and notes that timeoutMs reflects the configured --initialize-timeout-ms budget (values shown are the default). Tests pin the branch-body reduced shape (red without the route guard) and the non-initialize label falling through to the generic 500.
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not explored to full depth (tool budget reached): "agent 6a": did not trace whether bridge.branchSession / createSideTaskSession can actually raise a BridgeTimeoutError('initialize') at runtime (i.e., whether the doc's…; "agent 1a": executing the three new server.test.ts init-timeout tests (no node_modules/dist in the review worktree; install+build exceeded tool budget).
中文说明
未探索到全部深度(达到工具调用预算):"agent 6a":did not trace whether bridge.branchSession / createSideTaskSession can actually raise a BridgeTimeoutError('initialize') at runtime (i.e., whether the doc's…;"agent 1a":executing the three new server.test.ts init-timeout tests (no node_modules/dist in the review worktree; install+build exceeded tool budget)。
— qwen3.8-max via Qwen Code /review (v0.22.3)
The restore Errors list previously described every 504 as session_restore_timeout (retryable, fenced), but an init-budget expiry during ensureChannel returns init_timeout without Retry-After, retryable, or fence — the restore was never dispatched. Add the init_timeout 504 entry and narrow the "any other route" paragraph to mutation-bearing routes, explicitly calling out load/resume.
…out-contract # Conflicts: # docs/developers/qwen-serve-protocol.md
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
2 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- the
worktreeMetagate term is pinned by no test — already reported as R2-2 (comment 3891823731); author deferred to a follow-up test-hardening PR - the reduced init_timeout 504 shape for branch/side-task/load/resume routes is pinned by no test — already reported as R2-3 (comment 3891823734); author deferred to a follow-up test-hardening PR
Not explored to full depth (tool budget reached): "agent 5": empirical vitest run of packages/cli/src/serve/server/error-response.test.ts and the new server.test.ts cases — the monorepo prerequisite build could not co…; "agent 2": executed the two conflicting vitest suites ( packages/cli has no node_modules /built dist in this worktree and npm ci + workspace build exceeded the remai…; "agent 4": running the first new test ('returns a typed safe-retry error when channel initialization times out') individually — its siblings were executed; this one was no….
Convergence: round 3 posted 4 inline comment(s), 4 of them reported for the first time; the previous round posted 5 (5 new). docs/developers/qwen-serve-protocol.md (R1-2 → R2-1 → R3-3). Findings keep coming back to the same files: packages/cli/src/serve/server/error-response.ts (findings in round 2; 2 more now); docs/developers/qwen-serve-protocol.md (findings in round 2; 1 more now). (Evidence: the chain's newest generation carries ids stamped this round — a still-open claim re-voiced without its carried id reads the same there as a new Critical.) A mechanism whose fix grows the next Critical is diverging, not converging — raising the pattern with the mechanism's owner before the next round tends to end the loop faster than patching it again. A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. (Observation only — nothing was withheld from this review because of this observation.)
中文说明
本轮确认的 2 条建议级发现已在 PR 上报告过,不再重复发布(列表见上方英文部分)。
未探索到全部深度(达到工具调用预算):"agent 5":empirical vitest run of packages/cli/src/serve/server/error-response.test.ts and the new server.test.ts cases — the monorepo prerequisite build could not co…;"agent 2":executed the two conflicting vitest suites ( packages/cli has no node_modules /built dist in this worktree and npm ci + workspace build exceeded the remai…;"agent 4":running the first new test ('returns a typed safe-retry error when channel initialization times out') individually — its siblings were executed; this one was no…。
收敛情况:第 3 轮发布了 4 条行内评论,其中 4 条是首次提出;上一轮发布了 5 条(其中 5 条首次提出)。docs/developers/qwen-serve-protocol.md(R1-2 → R2-1 → R3-3)。发现反复回到同一批文件:packages/cli/src/serve/server/error-response.ts(第 2 轮已出过发现,本轮又有 2 条);docs/developers/qwen-serve-protocol.md(第 2 轮已出过发现,本轮又有 1 条)。(证据说明:链条最新一代携带的 id 由本轮铸造——一个未解决断言若在不携带原 id 的情况下被重新表述,在那里与新的 Critical 无法区分。)每次修复都长出下一个 Critical 的机制是在发散而非收敛——先把这一模式提给该机制的负责人,通常比继续打补丁更快结束循环。一个不断再生兄弟发现的簇,通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因,或把独立的簇拆成单独的 PR,通常比逐条修复更快结束循环。(仅为观察——本轮评审未因此扣留任何内容。)
— qwen3.8-max via Qwen Code /review (v0.22.3)
The file imported BridgeTimeoutError from both @qwen-code/acp-bridge/status and ../acp-session-bridge.js, which caused a TS2300 duplicate identifier build failure. Drop the bridge/status import; the local acp-session-bridge re-export is the one used across the route error handling.
…havior Update error-response and server tests plus the qwen-serve protocol doc to reflect that `newSession` dispatch timeouts now map to a retryable `init_timeout` 504, while `initialize` timeouts without caller context use the reduced contract.
qwen-code-dev-bot
left a comment
There was a problem hiding this comment.
Gate APPROVE at head 1203e591. Re-checking my prior REQUEST_CHANGES: both blockers are fixed in the diff itself — the safe-retry contract is now per-route opt-in (initPrecedesMutations, set only on plain POST /session creation; branch/worktree bodies and the fork-bearing routes emit the reduced 504 with no Retry-After/retryable/sideEffectPossible), and the protocol doc no longer claims authoritativeness where mutation can precede init — it spells out the exception matrix including the load/resume ensureChannel shape and the newSession-dispatch case. The SessionRestoreTimeoutError subclass is not swallowed (its labels are session/load/session/resume, not initialize). 13/13 threads resolved; six required checks were still running after today's merge-main (14 already green), decided on the review conclusion per maintainer policy. 中文:我此前两项阻塞已在 diff 内实证修复:safe-retry 契约改为逐路由 opt-in、仅纯创建路径承诺;文档不再越权声明并给出完整例外矩阵。restore 子类不被误吞(label 不同)。线程全闭,required 六项因合并 main 重跑中、已绿 14 项;按维护者指示以 Review 结论投票通过。
yiliang114
left a comment
There was a problem hiding this comment.
LGTM at 1203e59. Reviewed the full diff:
- The safe-retry contract is correctly scoped: only the plain creation path passes initPrecedesMutations (gated on the absence of branch/worktree bodies, where git mutations strictly precede the initialize handshake), earning the full typed 504 with Retry-After + retryable + sideEffectPossible:false; branch/worktree bodies and the branch/side-task/load/resume routes fall back to the reduced init_timeout shape with no retry fields, which is the right fail-closed default since their mutation outcome is genuinely unknown.
- initPrecedesMutations is excluded from bridgeErrorExtraContext so it can't leak into response bodies.
- The protocol doc matches the implementation on all four shapes (plain creation / branch|worktree creation / load|resume ensureChannel / newSession dispatch), and the tests pin each of them, including the branch-body case asserting the retry fields are absent.
- All 13 threads resolved; latest bot round approved. CI note: Test (ubuntu) and review-pr were still running at approval time on this head; the merge queue will gate on them.
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
2 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- route-level / no-fence tests pinning the documented reduced init_timeout 504 for the branch/side-task/load/resume routes — already reported as R2-3 (comment 3891823734); author deferred to a follow-up test-hardening PR
- worktreeMeta term of the initPrecedesMutations gate pinned by no test — already reported as R2-2 (comment 3891823731); author deferred to a follow-up test-hardening PR
Deferred under the convergence posture (round 4, not a blocker) — recorded, not requested in this round:
packages/cli/src/serve/server/error-response.ts:307 — [probe] D4-1 ACP-transport mapper toRpcError still maps initialize timeouts to an opaque internal error while REST now classifies them — code unchanged since round 3, deferred by the cod…
Convergence: round 4 posted 2 inline comment(s), 2 of them reported for the first time; the previous round posted 4 (4 new). Findings keep coming back to the same files: docs/developers/qwen-serve-protocol.md (findings in round 3; 2 more now). A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. (Observation only — nothing was withheld from this review because of this observation.)
中文说明
本轮确认的 2 条建议级发现已在 PR 上报告过,不再重复发布(列表见上方英文部分)。
收敛姿态下延后(第 4 轮,非阻断)——已记录,本轮不要求修改:共 1 条(原文未翻译,列表见上方英文部分)。
收敛情况:第 4 轮发布了 2 条行内评论,其中 2 条是首次提出;上一轮发布了 4 条(其中 4 条首次提出)。发现反复回到同一批文件:docs/developers/qwen-serve-protocol.md(第 3 轮已出过发现,本轮又有 2 条)。一个不断再生兄弟发现的簇,通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因,或把独立的簇拆成单独的 PR,通常比逐条修复更快结束循环。(仅为观察——本轮评审未因此扣留任何内容。)
— qwen3.8-max via Qwen Code /review (v0.22.3)
What this PR does
This PR gives ACP channel initialization timeouts a structured HTTP response instead of letting them fall through to the generic 500 handler. Only
BridgeTimeoutErrorwith theinitializelabel is classified: the daemon returns HTTP 504 withRetry-After: 5,init_timeout, the exactchannel.initializephase, and an explicit statement that no side effect was possible.The serve protocol documentation now records this response and its retry safety boundary.
Why it's needed
Channel initialization happens before the daemon dispatches
newSession. Today, if that 10-second handshake times out,POST /sessionreturns an untyped 500 containing only the error message. Clients therefore cannot distinguish a request that definitely never created a Session from an ambiguous mutation failure.The structured response lets aware clients retry safely while conservative clients that treat every 5xx mutation as ambiguous continue to fail closed. Timeouts from later phases are intentionally unchanged.
Reviewer Test Plan
How to verify
Force the bridge used by
POST /sessionto throwBridgeTimeoutError('initialize', 10000). Before this change the route returns an untyped HTTP 500. After this change it should return HTTP 504,Retry-After: 5,codeanderrorKindset toinit_timeout,phaseset tochannel.initialize, andsideEffectPossible: false.Also confirm that the special case requires the exact
initializelabel, so later bridge timeouts still use their existing conservative handling.Evidence (Before & After)
Before: the new route regression test failed with
expected 500 to be 504.After: the same focused test passes, and the CLI package build, typecheck, ESLint, and Prettier checks pass.
Tested on
Environment (optional)
Node.js 24, npm workspace checkout on macOS.
Risk & Scope
newSessionor restore timeouts, and deployment-specific reverse-proxy behavior.Linked Issues
N/A
中文说明
本 PR 做了什么
本 PR 为 ACP Channel 初始化超时增加结构化 HTTP 响应,不再让它落入通用 500 处理。只有标签为
initialize的BridgeTimeoutError会被分类:Daemon 返回 HTTP 504、Retry-After: 5、init_timeout、精确的channel.initialize阶段,以及明确的无副作用声明。同时更新 serve 协议文档,记录该响应及其安全重试边界。
为什么需要
Channel 初始化发生在 Daemon 派发
newSession之前。目前这个 10 秒握手一旦超时,POST /session只返回包含错误消息的非结构化 HTTP 500,客户端无法区分“确定没有创建 Session”和“写操作结果未知”。结构化响应让理解该协议的客户端可以安全重试;仍把所有 5xx 写操作视为结果未知的保守客户端会继续 fail closed。更晚阶段的超时行为有意保持不变。
Reviewer 测试计划
如何验证
让
POST /session使用的 bridge 抛出BridgeTimeoutError('initialize', 10000)。修改前路由返回非结构化 HTTP 500;修改后应返回 HTTP 504、Retry-After: 5、code/errorKind=init_timeout、phase=channel.initialize和sideEffectPossible=false。同时确认特殊处理要求标签精确等于
initialize,更晚阶段的 bridge 超时继续沿用现有保守处理。前后证据
修改前:新增路由回归测试失败,显示
expected 500 to be 504。修改后:同一聚焦测试通过,CLI package build、typecheck、ESLint 和 Prettier 检查均通过。
测试环境
macOS、Node.js 24、npm workspace checkout。
风险与范围
newSession或 restore 超时,以及部署特定的反向代理行为。关联 Issue
N/A