Skip to content

fix(core): widen the workflow stall watchdog past the transport retry ladder - #9397

Open
qqqys wants to merge 3 commits into
QwenLM:mainfrom
qqqys:fix/workflow-stall-window
Open

fix(core): widen the workflow stall watchdog past the transport retry ladder#9397
qqqys wants to merge 3 commits into
QwenLM:mainfrom
qqqys:fix/workflow-stall-window

Conversation

@qqqys

@qqqys qqqys commented Aug 18, 2026

Copy link
Copy Markdown
Collaborator

What this PR does

Raises the workflow stall watchdog's default window from 60s to 180s so it outlasts the transport's own silent retry ladder, and corrects the claim about how the watchdog is armed — including the version of it that ships to the model.

Why it's needed

The comment justifying 60s is wrong

attachStallWatchdog carries this rationale:

The watchdog arms on the FIRST progress event, not at attach time. The time-to-first-response window — connection setup, server-side queueing, and a reasoning model's pre-first-token thinking — emits no events (ROUND_START fires only AFTER await sendMessageStream resolves), so counting it would false-trip on a healthy-but-slow first response

The parenthetical is true. The conclusion drawn from it is backwards.

sendMessageStream returns a lazily iterated async generator — its last statement is return (async function* () {, and the API call (makeApiCallAndProcessStream) sits inside that body. Nothing primes the generator, so the body is suspended until the caller's first iteration. await sendMessageStream(...) therefore resolves before the request reaches the wire, and agent-core emits ROUND_START on the very next line, before for await (const streamEvent of responseStream) triggers anything.

So the deferred arm skips only round 1's pre-request work — the send-lock drain, route resolution, auto-compaction. Connection setup, server-side queueing and pre-first-token thinking all elapse with the timer already running.

Verified against the real code rather than by reading: a temporary test drove the real attachStallWatchdog exactly as agent-core drives it — await sendMessageStream(...), emit ROUND_START, then a 400ms simulated first response against a 200ms window:

t+   0ms  await chat.sendMessageStream(...)
t+   0ms  emit ROUND_START -> watchdog ARMED (stallMs=200)
t+ 400ms  first stream event: chunk
t+ 400ms  stalled()=true aborted=true reason=stalled

It aborts a perfectly healthy request. A companion case asserted the API call count at the moment the await resolves is 0.

The number is wrong too, for a different reason

Time-to-first-token is real but it is not the binding case. The transport's own retry ladder is: DEFAULT_RETRY_OPTIONS (utils/retry.ts) is { maxAttempts: 7, initialDelayMs: 1500, maxDelayMs: 30000 }, doubling with a cap, so the sleeps are 1.5s + 3s + 6s + 12s + 24s + 30s = 76.5s. And agent-core handles streamEvent.type === 'retry' by resetting round state and continue-ing, with no emit — so nothing in that ladder reaches the watchdog.

A plain 429/5xx ladder is therefore 76.5s of watchdog-invisible silence on a request that is retrying exactly as designed. 60s elapses during the ladder's sixth sleep. 180s clears it, and independently matches upstream's own default for the same watchdog.

What 180s does not fix

The stream-side rate-limit ladder (RATE_LIMIT_RETRY_OPTIONS, initialDelayMs: 60000, maxDelayMs: 300000) sleeps 60s, 120s, 240s, then 300s × 7 — about 42 minutes worst case. 180s equals its first two sleeps exactly, so a throttled dispatch still trips the watchdog, just later. Widening further is not the answer; making retries visible to the watchdog is, and that belongs in its own change.

What is deliberately not in this PR

MAX_STALL_ATTEMPTS stays at 3. The parity argument for 5 does not hold — upstream's 5 bounds retries on top of an initial attempt (6 total), while qwen's constant is the total by its own docstring, so 5 is neither number. Widening the window also removes the reason the extra attempts were wanted: the 76.5s ladder now completes inside a single attempt. And raising it would break two assertions that hardcode "stalled on all 3 attempts", for retries least likely to succeed — a provider that stalled three times at 180s will not come good on the fourth.

No re-arm throttle. Benchmarked at 114ns per re-arm — 0.41ms of CPU over a three-minute stream at 20 events/s. It buys timer churn, not capability, and it adds a correctness surface the existing tests are blind to: because onToolResult calls arm() immediately after onToolCall called clear(), a throttle placed in arm() leaves the watchdog permanently disarmed, and the whole suite still passes.

Reviewer Test Plan

How to verify

cd packages/core && npx vitest run src/agents/runtime/ src/tools/workflow/ — 656 passed, 6 skipped.

The constant change needs no edits to existing tests — they reference DEFAULT_STALL_MS symbolically, and the per-call stallMs option, the QWEN_CODE_WORKFLOW_STALL_SECONDS override, stallMs: 0 returning an inert handle, tool-call timer suspension and the abandoned-error wording all still pass untouched.

Two tests change:

  • A new test pins the relationship the number depends on — DEFAULT_STALL_MS must exceed the 76.5s retry ladder — rather than the literal, so it stays meaningful if either value is retuned.
  • The old does NOT fire during the time-to-first-response window test is replaced. It advanced 10s without ever emitting ROUND_START, so it only proved that a never-armed watchdog does not fire; in production ROUND_START has already fired by then. It is split into what is actually true: one test that the watchdog does not arm before the first progress event, and one that it does count the time-to-first-token window, emitting ROUND_START first exactly as agent-core does.

To see the mechanism yourself: packages/core/src/core/geminiChat.ts — note the return (async function* () { near the end of sendMessageStream and that makeApiCallAndProcessStream is inside it; then packages/core/src/agents/runtime/agent-core.ts, where ROUND_START is emitted immediately after that await and the stream is not iterated until the for await below.

Evidence (Before & After)

N/A — no user-visible or TUI change. The model-facing tool description changes text (see below), not behaviour.

Tested on

OS Status
🍏 macOS N/A
🪟 Windows N/A
🐧 Linux

Environment (optional)

Unit tests only, Node 22.23.0 on Linux.

Risk & Scope

  • Main risk or tradeoff: a genuinely wedged dispatch now holds its concurrency slot for 3 minutes per attempt instead of 1. With attempts left at 3 the worst case is ~9 minutes against the 30-minute run wall clock, which is the real backstop — max_time_minutes is not, because it is per attempt and resets on every stall retry. The trade is deliberate: the previous value was aborting healthy requests, and an abort costs a full re-dispatch.
  • Not validated / out of scope: the stream-side rate-limit ladder above; making StreamEventType.RETRY visible to the watchdog (the real fix, and a better PR on its own); MAX_STALL_ATTEMPTS; the re-arm throttle.
  • Breaking changes / migration notes: none. QWEN_CODE_WORKFLOW_STALL_SECONDS still overrides, and 0 still disables.

Linked Issues

None.

中文说明

这个 PR 做了什么

把 workflow stall watchdog 的默认窗口从 60s 提到 180s,使其能覆盖传输层自身的静默重试阶梯;并纠正关于 watchdog 何时装载的错误说明——包括那份会发给模型的版本。

为什么需要

为 60s 辩护的那段注释是错的

attachStallWatchdog 里写着:

watchdog 在第一个进度事件时装载,而不是在 attach 时。首响应窗口——连接建立、服务端排队、推理模型吐出第一个 token 前的思考——不产生任何事件(ROUND_START 只在 await sendMessageStream resolve 之后才触发),所以把它计入会对健康但缓慢的首响应误判

括号里那句是对的,从它推出的结论却是反的。

sendMessageStream 返回的是一个惰性迭代的 async generator——函数最后一句是 return (async function* () {,而 API 调用(makeApiCallAndProcessStream)就在这个生成器体内部。没有任何代码预先推进该生成器,因此其函数体在调用方第一次迭代前一直挂起。于是 await sendMessageStream(...)请求上线之前就 resolve 了,而 agent-core 紧接着下一行就发出 ROUND_START,此时下面的 for await (const streamEvent of responseStream) 还没开始触发任何东西。

所以"延迟装载"实际只跳过了第 1 轮的请求前工作——发送锁排空、路由解析、自动压缩。连接建立、服务端排队、首 token 前的思考,全都是在计时器已经运行的情况下流逝的。

这一点是跑出来验证的,不是读出来的:一个临时测试按 agent-core 的真实驱动方式驱动真实的 attachStallWatchdog——await sendMessageStream(...)、发出 ROUND_START,然后在 200ms 的窗口下模拟 400ms 首响应:

t+   0ms  await chat.sendMessageStream(...)
t+   0ms  emit ROUND_START -> watchdog ARMED (stallMs=200)
t+ 400ms  first stream event: chunk
t+ 400ms  stalled()=true aborted=true reason=stalled

它中止了一个完全健康的请求。配套用例断言了在 await resolve 的那一刻,API 调用次数为 0

数值也不对,但原因是另一个

首 token 时延是真实存在的,但它不是决定性的那一个。真正决定性的是传输层自己的重试阶梯:DEFAULT_RETRY_OPTIONSutils/retry.ts)为 { maxAttempts: 7, initialDelayMs: 1500, maxDelayMs: 30000 },按倍数增长并封顶,因此各次睡眠为 1.5s + 3s + 6s + 12s + 24s + 30s = 76.5s。而 agent-corestreamEvent.type === 'retry' 的处理是重置轮次状态并 continue不发出任何事件——所以整条阶梯对 watchdog 完全不可见。

于是一次普通的 429/5xx 阶梯,就是 76.5s 的"watchdog 看不见的静默",而请求本身正在完全按设计重试。60s 恰好在阶梯的第 6 次睡眠期间耗尽。180s 能盖住它,并且与上游同一 watchdog 的默认值独立吻合。

180s 解决不了什么

流式侧的限流阶梯(RATE_LIMIT_RETRY_OPTIONSinitialDelayMs: 60000maxDelayMs: 300000)依次睡 60s、120s、240s,然后 300s × 7——最坏约 42 分钟。180s 恰好等于它前两次睡眠之和,因此被限流的 dispatch 仍会触发 watchdog,只是更晚。继续加宽不是答案;让重试对 watchdog 可见才是,而那应当是独立的一次改动。

本 PR 刻意不做什么

MAX_STALL_ATTEMPTS 保持 3。 "对齐上游"的理由站不住:上游的 5 约束的是初次尝试之上的重试次数(合计 6 次),而 qwen 这个常量按其自身文档是总次数,所以 5 既不是 3 也不是 6。加宽窗口本身也消解了想要更多次数的理由:76.5s 的阶梯现在能在单次尝试内跑完。而且改它会打破两处硬编码的 "stalled on all 3 attempts" 断言,代价是把 token 花在最不可能成功的那几次重试上——一个在 180s 窗口下连停三次的 provider,第四次也不会好转。

不做重装载节流。 实测每次重装载 114ns——三分钟流、20 事件/秒,总共 0.41ms CPU。它买到的是定时器开销的减少,不是能力;而且引入了现有测试完全看不见的正确性风险面:由于 onToolResult 会在 onToolCall 调用 clear() 之后立刻调用 arm(),把节流放进 arm() 会让 watchdog 在该次 dispatch 剩余时间里永久失效,而整个测试套件照样全绿。

审阅者验证方案

如何验证

cd packages/core && npx vitest run src/agents/runtime/ src/tools/workflow/——656 通过、6 跳过。

常量改动不需要修改任何既有测试——它们都是符号引用 DEFAULT_STALL_MS;每次调用的 stallMs 选项、QWEN_CODE_WORKFLOW_STALL_SECONDS 覆盖、stallMs: 0 返回惰性句柄、工具在飞时挂起计时器,以及最终放弃时的错误文案,全部原样通过。

有两处测试变化:

  • 新增一个测试固定该数值所依赖的关系——DEFAULT_STALL_MS 必须大于 76.5s 的重试阶梯——而不是固定字面量,这样将来任一数值被重新调参时它依然有意义。
  • 旧的 does NOT fire during the time-to-first-response window 被替换。它推进 10s 却从未发出 ROUND_START,因此只证明了"从未装载的 watchdog 不会触发";而在生产中此时 ROUND_START 早已触发。它被拆成两个真实成立的断言:watchdog 在第一个进度事件之前不装载;以及它确实把首 token 窗口计入——测试会像 agent-core 那样先发出 ROUND_START

想自己看机制:packages/core/src/core/geminiChat.tssendMessageStream 末尾的 return (async function* () {,注意 makeApiCallAndProcessStream 在其内部;再看 packages/core/src/agents/runtime/agent-core.tsROUND_START 紧跟在那个 await 之后发出,而流直到下面的 for await 才开始迭代。

证据(前后对比)

N/A——没有用户可见或 TUI 变化。面向模型的工具描述只是文案变化,不是行为变化。

测试环境

OS Status
🍏 macOS N/A
🪟 Windows N/A
🐧 Linux

环境(可选)

仅单元测试,Linux 上的 Node 22.23.0。

风险与范围

  • 主要风险或取舍:真正卡死的 dispatch 现在每次尝试会占住并发槽位 3 分钟而非 1 分钟。次数保持 3 次时最坏约 9 分钟,对应 30 分钟的 run 墙钟上限——那才是真正的兜底,max_time_minutes 不是,因为它是每次尝试的且在每次 stall 重试后重置。这个取舍是有意的:此前的数值在中止健康请求,而一次中止的代价是完整重新派发。
  • 未验证 / 不在范围内:上文的流式限流阶梯;让 StreamEventType.RETRY 对 watchdog 可见(真正的修复,更适合独立成一个 PR);MAX_STALL_ATTEMPTS;重装载节流。
  • 破坏性变更 / 迁移说明:无。QWEN_CODE_WORKFLOW_STALL_SECONDS 仍可覆盖,0 仍可关闭。

关联 Issue

无。

… ladder

`DEFAULT_STALL_MS` was 60s, and the comment justifying it is wrong about how
the watchdog is armed:

    ROUND_START fires only AFTER `await sendMessageStream` resolves ... so
    counting [time-to-first-response] would false-trip

That parenthetical is true and its conclusion is backwards. `sendMessageStream`
returns a lazily iterated async generator — the API call lives in the generator
body and runs on first iteration — so the `await` resolves BEFORE the request
reaches the wire, and agent-core emits ROUND_START on the very next line. The
deferred arm skips only round 1's pre-request work; connection setup, queueing
and pre-first-token thinking all elapse with the timer running. Verified against
the real `attachStallWatchdog`: driven exactly as agent-core drives it, with a
400ms first response and a 200ms window, it aborts a perfectly healthy request.

The number was wrong too, but for a reason nothing in the tree states. The
binding case is the transport's own silent retry ladder: `DEFAULT_RETRY_OPTIONS`
sleeps 1.5s, 3s, 6s, 12s, 24s, 30s = 76.5s, and agent-core consumes each `retry`
stream event without emitting anything the watchdog counts as progress. So a
plain 429/5xx ladder is 76.5s of watchdog-invisible silence on a request that is
retrying exactly as designed — and 60s elapses during the ladder's sixth sleep.
180s clears it, and matches upstream's own default.

This does NOT cover the stream-side rate-limit ladder, whose first two sleeps
are 60s and 120s — 180s equals them exactly, and its worst case is ~42 minutes.
A throttled dispatch still trips the watchdog, just later. The real fix there is
to make retries visible to the watchdog rather than to keep widening it.

Also corrects the same stale claim everywhere it is repeated, including the
model-facing tool description, which shipped "a dispatch that produces no first
response is bounded by the subagent time cap, not this watchdog" — false twice
over, since `max_time_minutes` is per attempt and resets on every stall retry.

MAX_STALL_ATTEMPTS stays at 3. Widening the window removes the reason extra
attempts were wanted, and upstream's 5 is a retry bound on top of an initial
attempt where qwen's constant is the total — so it is not the parity it looks
like.

Replaces a test that asserted the watchdog does not fire during the
time-to-first-response window: it never emitted ROUND_START, so it only proved
that a never-armed watchdog does not fire, and it encoded the refuted model.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@qwen-code-ci-bot

qwen-code-ci-bot commented Aug 18, 2026

Copy link
Copy Markdown
Collaborator

Qwen Triage finished — CI landed green on cfc68b6 and the deferred approval was posted. finalize run

Qwen Triage 已完成 —— cfc68b6 的 CI 全绿,延迟审批已提交。查看 finalize 运行

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Thanks for the PR — the description does its homework, so I checked its claims against the code rather than taking them on faith.

Template: complete, bilingual ✓

Problem: exists — and verified from the code, not just argued in the description. Three facts make it true: sendMessageStream returns a lazily iterated async generator with the HTTP call (makeApiCallAndProcessStream) inside the generator body, so await sendMessageStream(...) resolves before the request reaches the wire; agent-core emits ROUND_START on the very next line, arming the watchdog before any bytes are sent; and retry stream events are consumed with a bare continue, so nothing the transport's backoff ladder does reaches the watchdog. The ladder itself (DEFAULT_RETRY_OPTIONS: 7 attempts, 1.5s initial, doubling, 30s cap) sleeps 1.5+3+6+12+24+30 = 76.5s nominal (~89s worst case with +30% jitter) — comfortably past the old 60s default, which elapsed during the ladder's sixth sleep. That is a healthy request, retrying exactly as designed, aborted by its own runtime. No linked issue, but with the mechanism demonstrated this is a defect by construction, not theoretical hardening.

Direction: aligned — reliability tuning of qwen-code's own workflow runtime, no scope pull. The PR is honest about what 180s does NOT cover (the stream-side rate-limit ladder sleeps up to ~42 minutes worst case) and correctly resists widening further, deferring the real fix (making retries visible to the watchdog) to its own change.

Size: core paths (packages/core/src/agents/runtime/**, packages/core/src/tools/workflow/**) — 63 production lines (51 in workflow-stall.ts, most of it rationale comments; the rest is doc-comment number updates) vs 43 test lines. fix type, far below any escalation threshold.

Approach: minimal — the constant, the comments that encoded the wrong model (including the model-facing tool description, which shipped the same wrong claim to the model; right to fix atomically), and two test changes: a relationship test pinning DEFAULT_STALL_MS above the ladder total, and a replacement for the old time-to-first-response test that only ever proved a never-armed watchdog doesn't fire. The deliberate exclusions check out: MAX_STALL_ATTEMPTS stays at 3 (upstream's 5 counts retries on top of an initial attempt, qwen's constant counts the total — 5 is neither number), and the re-arm throttle is 114ns of churn that adds a disarming hazard the suite can't see.

Risk: no Stage 1e high-risk path matches. The tradeoff is named in the PR and worth repeating: a genuinely wedged dispatch now holds its concurrency slot up to ~9 minutes (3 attempts × 3 min) before abandonment; the 30-minute run wall clock remains the real backstop.

Moving on to code review. 🔍

中文说明

感谢贡献——PR 描述做了充分论证,因此我对照代码逐一核实了这些论断,而不是照单全收。

模板:完整、双语 ✓

问题:真实存在——不是采信 PR 的叙述,而是直接从代码验证。三个事实使其成立:sendMessageStream 返回惰性迭代的 async generator,HTTP 调用(makeApiCallAndProcessStream)位于生成器体内部,因此 await sendMessageStream(...) 在请求上线之前就已 resolve;agent-core 紧接着下一行就发出 ROUND_START,在任何字节发出之前 watchdog 就已装载;而 retry 流事件被一个裸 continue 消费,传输层退避阶梯的任何动作都不会到达 watchdog。阶梯本身(DEFAULT_RETRY_OPTIONS:7 次尝试、初始 1.5s、倍增、30s 封顶)名义总睡眠 1.5+3+6+12+24+30 = 76.5s(+30% 抖动最坏约 89s)——远超旧的 60s 默认值,后者在阶梯第 6 次睡眠期间就已耗尽。这是一个完全按设计重试的健康请求,被自己的运行时中止。没有关联 issue,但机制已被证明,这是构造性缺陷,不是理论性加固。

方向:对齐——是 qwen-code 自身 workflow 运行时的可靠性调参,不偏离核心。PR 也诚实地说明了 180s 覆盖不了什么(流式限流阶梯最坏可睡约 42 分钟),并正确地拒绝继续加宽,把真正的修复(让重试对 watchdog 可见)留给独立改动。

规模:核心路径(packages/core/src/agents/runtime/**packages/core/src/tools/workflow/**)——生产代码 63 行(workflow-stall.ts 占 51 行,其中大部分是说明性注释;其余为文档注释中的数字更新),测试 43 行。fix 类型,远低于任何升级阈值。

方案:最小化——常量本身、编码了错误模型的注释(包括面向模型的工具描述,它把同样的错误论断发给了模型;原子性地一并修正是对的),以及两处测试变化:一个关系测试固定 DEFAULT_STALL_MS 必须大于阶梯总和;另一个替换了旧的"首响应窗口"测试——那个测试只证明了从未装载的 watchdog 不会触发。刻意排除项也站得住:MAX_STALL_ATTEMPTS 保持 3(上游的 5 约束的是初次尝试之上的重试次数,而 qwen 的常量计的是总次数——5 两者都不是);重装载节流每次仅 114ns,却带来测试套件看不见的失效风险。

风险:未命中 Stage 1e 高风险路径。PR 已点明、这里重申的取舍:真正卡死的 dispatch 现在会占住并发槽位最长约 9 分钟(3 次尝试 × 3 分钟)才被放弃;30 分钟的 run 墙钟上限仍是真正的兜底。

进入代码审查。🔍

Qwen Code · qwen3.8-max

Reviewed at cfc68b64d86aa281646d17d83c91c097594c2548 · re-run with @qwen-code /triage

@qwen-code-ci-bot

qwen-code-ci-bot commented Aug 18, 2026

Copy link
Copy Markdown
Collaborator

Code review

I wrote my independent proposal before opening the diff — raise the default past the ladder, fix the comments that encode the wrong arming model, update the model-facing text, keep the tests symbolic — and the PR matches it almost exactly, plus two things I'd also have wanted: the relationship test, and splitting the old arming test into what is actually true.

I verified every load-bearing claim in the description against the code:

  • Lazy generator ✓ — sendMessageStream returns (async function* () { ... })() with makeApiCallAndProcessStream inside the body, so the await resolves before the request reaches the wire.
  • ROUND_START ordering ✓ — emitted immediately after that await, before the first iteration arms the request.
  • Retry blindness ✓ — retry stream events are consumed with a bare continue and no emitter event, so the whole ladder is watchdog-invisible.
  • Ladder arithmetic ✓ — 7 attempts, 1.5s initial, doubling capped at 30s → 76.5s nominal, ~89s worst case under +30% jitter; 60s elapsed during the sixth sleep.

No critical findings. All consumers of DEFAULT_STALL_MS are symbolic or templated (resolveStallMs fallback, the tool description interpolates the constant), the QWEN_CODE_WORKFLOW_STALL_SECONDS override and the 0-disables semantics are untouched, and the watchdog only attaches via runStallResilient, which only the workflow orchestrator calls — the main session is unaffected. The relationship test hardcodes the ladder sleeps rather than deriving them; that's the right call, since DEFAULT_RETRY_OPTIONS is module-private in utils/retry.ts — the literal total acts as a tripwire forcing a conscious update if either side is retuned. The two comment-only hunks in workflow-orchestrator.ts / workflow-sandbox.ts and the model-facing description in workflow.ts all land on the corrected semantics; I found no stale 60s reference anywhere else in the repo (code, docs, or integration tests).

One non-blocking observation, already flagged by the PR itself: 180s equals exactly the first two sleeps of the stream-side rate-limit ladder (60s + 120s), so a throttled dispatch still trips the watchdog, just later. The PR names the real fix — making StreamEventType.RETRY visible to the watchdog — and correctly keeps it out of scope.

Test evidence (the PR's own CI, read via API — per policy this review never runs PR code)

At review time the Linux unit suite is still in progress; the macOS/Windows unit jobs and the CLI integration suite are skipped by design on pull_request events (they run in the merge queue — ci.yml gates them on merge_group). Security checks and both Desktop Shell jobs are green. The Qwen Triage Finalize job updates the table below once CI settles. Note: the author's self-reported "656 passed, 6 skipped" is their claim, not evidence — the CI results below are what count.

Final CI results for cfc68b6 (auto-updated by the triage finalize job after CI completed):

Check Conclusion
Classify PR ✅ success
Dependency CVE audit ✅ success
Desktop Shell (ubuntu-22.04) ✅ success
Desktop Shell (windows-2022) ✅ success
Secret scan (TruffleHog) ✅ success
Test (ubuntu-latest, Node 22.x) ✅ success
web-shell E2E Smoke (ubuntu-latest, Node 22.x) ✅ success

One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行(取最新一次运行),省略 skipped,失败项排在最前。

Sandboxed verification could settle the one thing neither static review nor this CI proves end-to-end: @qwen-code /verify — the suite pins the constant-vs-ladder relationship, but no test drives a dispatch through an actual 429/5xx retry ladder; an A/B against the base build could show the old default aborting a healthy dispatch mid-ladder and the new one surviving it.

中文说明

代码审查

打开 diff 之前我先写下了自己的独立方案——把默认值提到超过阶梯、修正编码了错误装载模型的注释、更新面向模型的文案、测试保持符号引用——PR 与之几乎完全一致,并且还包含我也会想要的两处:关系测试,以及把旧的装载测试拆分成真实成立的断言。

描述中每个承重论断我都对照代码核实过:惰性生成器(await 在请求上线前 resolve)✓;ROUND_START 紧随该 await 发出 ✓;retry 事件被裸 continue 消费、不产生任何 emitter 事件(整条阶梯对 watchdog 不可见)✓;阶梯算术(7 次尝试、1.5s 起步倍增、30s 封顶 → 名义 76.5s,+30% 抖动最坏约 89s;60s 在第 6 次睡眠期间耗尽)✓。

没有关键问题。DEFAULT_STALL_MS 的所有消费方都是符号或模板引用(resolveStallMs 兜底、工具描述直接插值常量);QWEN_CODE_WORKFLOW_STALL_SECONDS 覆盖与 0 关闭语义不变;watchdog 只经由 runStallResilient 挂载,只有 workflow orchestrator 调用——主会话不受影响。关系测试硬编码阶梯睡眠值而不是推导——这是对的,因为 DEFAULT_RETRY_OPTIONSutils/retry.ts 的模块私有常量;字面总和充当绊线,任一侧调参都会强制有意识地更新。workflow-orchestrator.ts / workflow-sandbox.ts 的两处纯注释修改与 workflow.ts 面向模型的描述都落在修正后的语义上;全仓(代码、文档、集成测试)未发现其他遗留的 60s 引用。

一个非阻塞观察(PR 自己也已点明):180s 恰好等于流式限流阶梯的前两次睡眠(60s + 120s),因此被限流的 dispatch 仍会触发 watchdog,只是更晚。真正的修复——让 StreamEventType.RETRY 对 watchdog 可见——被 PR 正确地留在范围之外。

测试证据(PR 自己的 CI,经 API 读取——按规则本审查从不运行 PR 代码)

审查时 Linux 单元测试仍在进行;macOS/Windows 单测与 CLI 集成套件在 pull_request 事件下按设计跳过(只在 merge queue 中运行——ci.ymlmerge_group 为条件)。安全检查与两个 Desktop Shell 任务全绿。CI 落定后 Qwen Triage Finalize 任务会更新上方英文版的 CI 表格。注意:作者自报的 "656 passed, 6 skipped" 是其声明,不是证据——以上 CI 结果才算数。

沙箱验证可以解决静态审查和这次 CI 都无法端到端证明的一件事:@qwen-code /verify——测试套件固定的是常量与阶梯的关系,但没有任何测试真正驱动 dispatch 走完一条 429/5xx 重试阶梯;与 base 构建做 A/B,可以展示旧默认值在阶梯中途中止健康请求、而新默认值能撑过去。

Qwen Code · qwen3.8-max

Reviewed at cfc68b64d86aa281646d17d83c91c097594c2548 · re-run with @qwen-code /triage

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Confidence: 4/5 — clean, minimal, independently verified; the one thing withheld is procedural, CI had not settled when this review ran.

Stepping back: the PR's approach matches my independent proposal and exceeds it in the ways that matter — the relationship test outlives the literal values, and the replacement tests pin what is actually true instead of a comfortable fiction. The motivation survives scrutiny: this is not a number that felt wrong, it is a number provably smaller than a silence window the runtime itself designs — the transport's own retry ladder. Every claim in the description checked out against the code, including the two that are easy to get wrong (the lazy generator, and the retry-event blindness). The scope discipline is the strongest part of the diff: it names what 180s does not fix and refuses to widen further, keeps MAX_STALL_ATTEMPTS at 3 with a real argument instead of a false parity, and skips the re-arm throttle with measurements to back it. Six months from now the rationale comments will still explain why the number is 180,000 and not 60,000 — that is the part I'd thank the author for.

Reservations, for the record: no user-reported abort in the wild is attached to this PR — the case is structural (default < designed silence window), which I verified in code, and the author's temporary-test reproduction lives in the description, not the suite; the suite pins the forward-looking invariants instead. The stream-side rate-limit ladder can still trip the watchdog later — acknowledged and deferred by design, not a blocker.

Verdict: approve — with one procedural condition. One pull_request workflow run (Qwen Code CI) is still in flight on this commit, so approval is deferred until CI lands green on cfc68b64d86aa281646d17d83c91c097594c2548. The finalize job posts the commit-pinned approval if everything settles green, and withholds it if anything lands red or the head moves.

中文说明

置信度:4/5 —— 干净、最小化、已独立核实;唯一保留的是程序性的——审查时 CI 尚未落定。

退一步看:PR 的方案与我独立写下的一致,并在关键处更好——关系测试比字面量更长寿;替换后的测试固定的是真实行为,而不是一个自洽的假象。动机经得起推敲:这不是一个"感觉不对"的数字,而是一个被证明小于运行时自己设计出的静默窗口(传输层重试阶梯)的数字。描述中每个论断都通过了代码核对,包括两个最容易弄错的(惰性生成器、retry 事件的不可见性)。范围纪律是这份 diff 最强的部分:点明 180s 修不了什么并拒绝继续加宽;用真实论证(而不是虚假的"对齐上游")把 MAX_STALL_ATTEMPTS 保持在 3;带着测量数据跳过重装载节流。六个月后,这些注释仍然会解释为什么是 180,000 而不是 60,000——这是我最感谢作者的部分。

保留意见,记录在案:没有附带真实用户报告的中止案例——论据是结构性的(默认值小于设计出的静默窗口),我已在代码中核实;作者的临时测试复现存在于描述里而非测试套件中,套件固定的是面向未来的不变量。流式限流阶梯仍可能更晚触发 watchdog——PR 已承认并按设计推迟处理,不构成阻塞。

结论:批准——带一个程序性条件。该提交上仍有一个 pull_request 工作流(Qwen Code CI)在运行,因此批准推迟到 CI 在该提交上全绿之后;若全部通过,finalize 任务会代投绑定该提交的批准;若有失败或头部移动,则不会。

Qwen Code · qwen3.8-max

Reviewed at cfc68b64d86aa281646d17d83c91c097594c2548 · re-run with @qwen-code /triage

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, looks ready to ship — CI landed green after the review. ✅

@doudouOUC doudouOUC left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary — PR #9397

fix(core): widen the workflow stall watchdog past the transport retry ladder

审查维度

11 个审查代理均未发现问题。该 PR 代码干净、文档完善、测试充分。

代理 维度 结果
1a 逐行正确性 ✅ 无问题
1b 删除行为审计 ✅ 无问题
2 安全性 ✅ 无安全问题
3a 复用与重复 ✅ 无问题
3b 抽象层次适配 ✅ 无问题
3c 一致性与清晰度 ✅ 无问题
4 性能与效率 ✅ 无问题
5 测试覆盖率 ✅ 覆盖充分
6a 攻击者视角 ✅ 无问题
6b Oncall 视角 ✅ 无问题
6c 维护者视角 ✅ 无问题

验证结果

  • 656 个测试全部通过,6 个跳过(与 PR 描述一致)
  • 常量变更使用符号引用(DEFAULT_STALL_MS),现有测试无需修改
  • 新增测试验证 DEFAULT_STALL_MS > 76.5s 重试阶梯
  • 修正后的首响应窗口行为已通过测试验证

审查结论

PR 逻辑正确,改动范围精确,测试覆盖充分。将默认值从 60s 提升到 180s 以覆盖传输层静默重试阶梯(76.5s),并修正了关于 watchdog 装载时机的注释和模型可见的工具描述。由于 CI 仍在运行,本次以 COMMENT 形式提交(而非 APPROVE)。

@danialzivehdadr

This comment has been minimized.

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed — no blockers. Suggestions are inline.

Test Plan (not a blocker): 656 passed — this review observed 20504, 1578, 21373, 1597, 494 passed.

中文说明

已审查——无阻断问题。 建议见行内评论。

Test Plan(非阻断):656 passed — this review observed 20504, 1578, 21373, 1597, 494 passed

— qwen3.8-max via Qwen Code /review (v0.21.13)

Comment thread packages/core/src/agents/runtime/workflow-stall.ts Outdated
Comment thread packages/core/src/agents/runtime/workflow-stall.test.ts Outdated
Comment thread packages/core/src/agents/runtime/workflow-stall.test.ts
…zed against

Three wording defects from round 1, all in the same direction: the comments
read as coverage-complete while longer watchdog-invisible waits exist.

R1-1: "Sized against the transport's own silent retry ladder" (and "The
binding case is the transport's silent retry ladder" at `attachStallWatchdog`)
reads as if all silent-retry silence is covered. Three longer layers are not:
stream-side rate-limit sleeps (`RATE_LIMIT_RETRY_OPTIONS` — 60s/120s/240s/300s,
so two consecutive sleeps already reach 180s), a provider `Retry-After`
honored unclamped on the normal HTTP path, and unattended-mode persistent
backoff. Both sites now name `retryWithBackoff` as the binding case and list
what the window does not cover. Behaviour unchanged — the abort predates this
PR and is strictly mitigated by it.

R1-2: the relationship test hand-copies the ladder and asserts
`toBe(76_500)` against its own literal, so it guards only the
`DEFAULT_STALL_MS` side; a retune of `DEFAULT_RETRY_OPTIONS` would leave it
green while the real ladder overtook the window. Records that, the ±30%
jitter the nominal figures omit, and a TODO to derive it once
`DEFAULT_RETRY_OPTIONS` is exported.

R1-3: the file asserted and refuted the same arming model within ~20 lines —
the new test pins that the time-to-first-token window IS watched, while three
sibling comments still said it is exempt. Reworded to say what actually
happens: ROUND_START fires before the request reaches the wire, so the
pre-arm silence is only round 1's pre-generator work.

Verified: workflow-stall 26/26. Comment-only; no behaviour change.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@qqqys

qqqys commented Aug 18, 2026

Copy link
Copy Markdown
Collaborator Author

@qwen-code /review

@github-actions

Copy link
Copy Markdown
Contributor

Qwen Code review request accepted. Review is queued in workflow run.

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed. Suggestions are inline.

Not explored to full depth (tool budget reached): "agent 5": executed workflow-stall.test.ts under vitest — node_modules is absent in the worktree and the parent checkout (ERR_MODULE_NOT_FOUND on vitest), so test/implem….

Test Plan (not a blocker): 656 passed — this review observed 20504, 1578, 21375, 1597, 494, 3747, 529 passed.

中文说明

已审查。 建议见行内评论。

未探索到全部深度(达到工具调用预算):"agent 5"executed workflow-stall.test.ts under vitest — node_modules is absent in the worktree and the parent checkout (ERR_MODULE_NOT_FOUND on vitest), so test/implem…

Test Plan(非阻断):656 passed — this review observed 20504, 1578, 21375, 1597, 494, 3747, 529 passed

— qwen3.8-max via Qwen Code /review (v0.21.13)

Comment thread packages/core/src/agents/runtime/workflow-stall.test.ts Outdated
Comment thread packages/core/src/agents/runtime/workflow-stall.test.ts

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Partially reviewed — gaps disclosed. Suggestions are inline.

2 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:

  • R1-2 relationship-test hand-copy and unasserted jitter floor — already re-reported at this commit (comment 3804486763)
  • R1-3 old-model residue in the SUCCEEDS-test comment — already re-reported at this commit (comment 3804486771)

Not reviewed: reverse audit — stopped before round 6 by the review time budget.

Test Plan (not a blocker): 656 passed — this review observed 1578, 21373, 1597, 494, 3747, 529 passed.

Deferred under the convergence posture (round 2, not a blocker) — recorded, not requested in this round:

  • packages/core/src/agents/runtime/workflow-stall.test.ts:111 — [probe] 'These two tests pin what actually happens' overstates: the ROUND_START-before-wire ordering is pinned by no test (deferred under the code-age rule — anchored on code unc…
  • packages/core/src/agents/runtime/workflow-stall.ts:11 — [test] hunk-survived: fileoverview 'default 3 min' comment ungated by any test
  • packages/core/src/agents/runtime/workflow-stall.ts:112 — [test] hunk-survived: attachStallWatchdog doc-comment rewrite ungated by any test
  • packages/core/src/agents/runtime/workflow-stall.ts:193 — [test] hunk-survived: 'Intentionally NOT armed here' inline comment ungated by any test
  • packages/core/src/agents/runtime/workflow-orchestrator.ts:173 — [test] hunk-survived: 'stallMs (3 min default)' comment ungated by any test
  • packages/core/src/agents/runtime/workflow-sandbox.ts:418 — [test] hunk-survived: 'Defaults to 180_000' JSDoc ungated by any test
中文说明

仅完成部分审查,审查缺口已披露。 建议见行内评论。

本轮确认的 2 条建议级发现已在 PR 上报告过,不再重复发布(列表见上方英文部分)。

未审查:反向审计——评审时间预算不足,未能开始第 6 轮。

Test Plan(非阻断):656 passed — this review observed 1578, 21373, 1597, 494, 3747, 529 passed

收敛姿态下延后(第 2 轮,非阻断)——已记录,本轮不要求修改:共 6 条(原文未翻译,列表见上方英文部分)。

— qwen3.8-max via Qwen Code /review (v0.21.13)

Comment thread packages/core/src/agents/runtime/workflow-stall.test.ts
Comment thread packages/core/src/agents/runtime/workflow-stall.ts Outdated
Round 2 of the review on QwenLM#9397 kept two suggestions from round 1 open and
added two more. All four, plus the export the first one needed.

R1-2 — the relationship test hand-copied the ladder as a local literal, so
`toBe(76_500)` pinned only that copy: the reviewer's probe retuned
`DEFAULT_RETRY_OPTIONS` to a 226.5s ladder and the shipped test stayed green
while the real ladder overtook the 180s window. The test now derives the
ladder from `DEFAULT_RETRY_OPTIONS` through the real `getRetryDelayMs`,
mirroring retryWithBackoff's error path (`maxAttempts - 1` sleeps,
`currentDelay` doubling under the `maxDelayMs` cap). It also pins the jittered
worst case (89_250ms) rather than the nominal sum, because the normal path
applies ±30% jitter — a `DEFAULT_STALL_MS` retuned into the (76.5s, 89.25s]
band false-trips on an unlucky run and a nominal-only assertion would not
notice. `DEFAULT_RETRY_OPTIONS` is exported for this; it was already the
symbol the old TODO named.

R2-2 — the sizing rationale said unattended-mode persistent backoff sleeps
"up to 5 min per sleep". That bound is `PERSISTENT_MAX_BACKOFF_MS`, and it
only covers the exponential branch: a provider `Retry-After` in persistent
mode is capped at `PERSISTENT_CAP_MS`/6h instead (retry.ts:428-430), so a 429
carrying `Retry-After: 7200` sleeps two hours, 24x the documented worst case
a maintainer would design the follow-up against.

R2-1 / R1-3 — the last two residues of the discarded arming model. The test
title `fires after stallMs of no activity once the first response has arrived`
stated the pre-PR model in vitest and CI output, directly contradicting the
sibling test this PR adds; and the `retries on stall then SUCCEEDS` comment
still called ROUND_START "a first response event" under the `#8` tag. Both now
say ROUND_START, which fires before the request reaches the wire.

Verification: `workflow-stall.test.ts` 26/26, `retry.test.ts` +
`retryPolicy.test.ts` 110/110, eslint and prettier clean. `tsc --noEmit` on
packages/core reports the same single pre-existing error before and after the
diff (the worktree's `sharp` typing skew). Mutation-checked against the
reviewer's own two probes, both of which shipped green before: retuning
`DEFAULT_RETRY_OPTIONS` to `maxAttempts: 12` now fails with `expected 226500
to be 76500`, and `DEFAULT_STALL_MS = 80_000` now fails with `expected 80000
to be greater than 89250`.

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No blocking issues. LGTM! ✅

Deferred under the convergence posture (round 3, not a blocker) — recorded, not requested in this round:

  • packages/core/src/agents/runtime/workflow-stall.test.ts:148 — [review] ROUND_START-before-wire ordering is pinned by no test
  • packages/core/src/agents/runtime/workflow-stall.ts:67 — [probe] sizing rationale claims coverage of healthy first responses the 180s window does not cover
中文说明

无阻断问题。LGTM!✅

收敛姿态下延后(第 3 轮,非阻断)——已记录,本轮不要求修改:共 2 条(原文未翻译,列表见上方英文部分)。

— qwen3.8-max via Qwen Code /review (v0.21.13)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants