Skip to content

fix(core): settle a cancelled workflow at once, and size the window by usable CPUs - #10468

Merged
qqqys merged 16 commits into
QwenLM:mainfrom
qqqys:fix/workflow-cancel-settles
Aug 29, 2026
Merged

fix(core): settle a cancelled workflow at once, and size the window by usable CPUs#10468
qqqys merged 16 commits into
QwenLM:mainfrom
qqqys:fix/workflow-cancel-settles

Conversation

@qqqys

@qqqys qqqys commented Aug 29, 2026

Copy link
Copy Markdown
Collaborator

What this PR does

Fixes two defects in a workflow run's lifecycle. This is the reopened, reduced form of #9974 — same two fixes, same six files, on a fresh branch so the review starts from the content actually under review rather than from threads about code that was cut.

Cancelling a workflow now ends it. The sandbox raced the script only against the wall clock, and its own in-source comment admitted there was no abort arm. A script that was not currently blocked on a dispatch — sitting in ungated awaits, or simply hung — kept the foreground tool call open until the banked remainder of the 30-minute clock expired, and the user watched a cancelled run refuse to go away. The race now has an abort arm, so a cancel settles the run at once, and a script whose signal is already aborted before run() is entered is not started at all. The wall clock stays: it is the only bound on a script awaiting a promise that never resolves. One subtlety: the wall-clock timeout itself aborts the controller before rejecting (deliberately, per T40, so in-flight subagents see the cancellation first), and a naive abort arm would win that race and report every timeout as a cancellation. The arm ignores that one abort, so a timeout is still reported as the timeout it is.

Settling early exposed a gap in the runner. Its success branch already guarded against a registry entry settled terminal from outside — the dialog's cancel, the approval contingency's fail — by reporting the entry's state rather than an ok that contradicts it. The catch branch had no such guard, and once a cancel rejects the sandbox promptly, the rejection reached it first and was reported as the run's message. The catch branch gains the mirror of that guard.

The concurrency default reads the CPUs the process can actually use. os.cpus().length - 2, floored at 1, ignores the CPU affinity mask and container limits, and os.cpus() can return an empty array in some sandboxes — which made every run strictly serial. It now reads os.availableParallelism(), floored at 2: a window of 1 turns every parallel() into a sequence and silently defeats the point of a fan-out on a small machine. The two model-facing description strings that stated the formula are updated to match.

Why it's needed

The first is the one users hit. Pressing ESC on a workflow whose script is between dispatches did nothing visible for minutes; the run was cancelled in the registry but the tool call stayed open, so the conversation could not continue. That is the engine's edge misbehaving, and it is cheap to fix.

The second is silent: a serial window on a container degrades a fan-out with no signal that it did. It is wrong by construction, not by load, so it is fixed at the source.

Why a reopen. #9974 started as these two fixes plus a worktree-provisioning mutex. Review found, correctly, that serialising provisioning made a pre-existing .qwen/.gitignore defect deterministic; fixing that opened a symlink surface in a service AgentTool shares; and a later addition on the branch — holding the sandbox's unhandledRejection observer open across teardown — went three rounds with two fix-induced Critical findings at one site. Each of those is real work and each got cut from the branch, but the PR carried ten rounds of threads about code no longer in it. Those two pieces return as their own PRs, designed from their failure modes: the mutex together with the gitignore root cause, and the teardown-observation question by attaching the observer at the dispatch boundary rather than guessing a window at teardown.

Reviewer Test Plan

How to verify

cd packages/core
npx vitest run src/agents/runtime/ src/tools/workflow/ src/agents/workflow-run-registry.test.ts

Observed: Test Files 18 passed | 1 skipped (19), Tests 929 passed | 6 skipped (935). The same command with the six files at upstream/main passes three fewer — this PR adds a net three tests and regresses none.

Three existing tests were re-targeted, not deleted. They pinned the old settlement behaviour — "a cancelled run settles on the banked wall-clock remainder" — which is precisely what this PR changes. Their scenarios (cancel while paused, cancel mid-pausing with a post-abort drain, signal already aborted at run()) remain the right scenarios; their assertions now say the run settles immediately with a cancellation, and the already-aborted case additionally asserts that no dispatch was ever issued. One more, does not mirror plain-Error teardown rejections of an aborted run, changes from awaiting a resolved run to expecting a cancelled one: the abort's rejection is queued ahead of a same-tick completion, so cancel wins that race — matching the runner, which already reports a cancelled registry entry over an ok outcome.

New tests, by claim:

  • cancellation settles the run at once while the script still runs its own finally — the host-side run settles immediately, and the script's finally still executes because its dispatch rejected on the aborted signal.
  • does not run a script whose signal was already aborted before run() — a dispatch spy asserts zero calls.
  • resolveConcurrencyLimit derives the default from availableParallelism, floored at 2 — via an injected parallelism source: 0/1/3 → 2, 6 → 4, 18/64 → 16.
  • The two existing peak === cap fan-out tests and the [1,16] clamp test now compute the cap from availableParallelism() with the new floor.

Mutation-checked, each in isolation, restoring from the commit between runs: removing the abort arm from the race fails exactly the three abort-settlement tests; flooring back to 1 fails exactly the floor test; dropping the wall-clock guard on the abort arm fails exactly the T40 timeout test; dropping the runner's catch-branch guard fails exactly the three external-settlement runner tests. Nothing else moves in any of the four.

Also run: npx tsc --noEmit -p tsconfig.json (exit 0), npx prettier --check, npx eslint on all six files.

Evidence (Before & After)

N/A for the TUI — the change is in packages/core. The behavioural before/after is the test rewrite above: the cancel-while-paused scenario previously asserted settled === false 20 ms after the abort and then rejects.toThrow(/exceeded 200 ms of active time/); it now asserts settled === true on the next tick and rejects.toThrow(/aborted \(cancelled\)/).

Tested on

OS Status
🍏 macOS ⚠️ not tested
🪟 Windows ⚠️ not tested
🐧 Linux ✅ tested

Environment

Unit tests via npx vitest run from packages/core, plus tsc --noEmit, prettier and eslint.

Risk & Scope

  • Main risk or tradeoff: cancel now wins a same-tick race against completion. If a user cancels at the exact instant a script returns, the run reports cancelled rather than ok. This is consistent with the runner, which already reports a cancelled registry entry over an ok outcome, and with what the user asked for — but it is a change in which side wins a tie. Second: the floor moves from 1 to 2, so a machine reporting 1–3 usable CPUs now runs two agents at a time where it ran one; still bounded by the same QWEN_CODE_MAX_WORKFLOW_CONCURRENCY override.
  • Not validated / out of scope: the abort arm settles the host-side run; the script's own promise is left to finish by itself. A detached dispatch rejection landing after run()'s finally has detached the adoption-escape hook is the teardown-observation question that was cut from fix(core): three run-lifecycle defects that silently cost a run #9974 and returns as its own PR — this PR neither widens nor narrows that window. No change to pause/resume semantics, the pause gate, or the env-var clamps. resolveConcurrencyLimit gains an injectable parallelism source for testing; its default is os.availableParallelism and no caller passes anything else.
  • Breaking changes / migration notes: none to APIs or schemas. Two user-visible behaviour changes: a cancelled workflow now settles immediately (previously up to the remaining wall-clock budget), and the default concurrency window is max(2, min(16, availableParallelism() - 2)) rather than max(1, min(16, os.cpus().length - 2)).

Linked Issues

Supersedes #9974.

中文说明

这个 PR 做了什么

修复 workflow 运行生命周期中的两个缺陷。这是 #9974 关闭后重开的精简版——同样的两个修复、同样的六个文件,放在一个新分支上,让 review 从实际受审的内容开始,而不是从一堆针对已被砍掉代码的讨论线程开始。

取消 workflow 现在真的会结束它。 sandbox 此前只让脚本与 wall clock 竞争,它自己的源码注释就承认没有 abort 分支。一个当下没有阻塞在 dispatch 上的脚本——停在未加门的 await 上,或者干脆挂住——会让前台工具调用一直开着,直到 30 分钟时钟的剩余额度耗尽,用户眼睁睁看着一个已取消的运行拒绝结束。现在这个 race 有了 abort 分支,取消会立刻让运行结束;而在进入 run() 之前信号就已中止的脚本根本不会被启动。wall clock 保留:它是唯一能约束"等待一个永不 resolve 的 promise"的脚本的东西。一个细节:wall clock 超时本身会中止 controller 再 reject(这是 T40 有意为之,让进行中的子 agent 先看到取消),朴素的 abort 分支会赢下这场 race,把每一次超时都报成取消。该分支会忽略这一次特定的 abort,因此超时仍然按超时来报告。

提前结束暴露了 runner 里的一个缺口。它的成功分支本来就有一道守卫——当注册表条目已被外部置为终态(对话框的取消、审批应急处理的失败)时,报告条目的状态,而不是一个与之矛盾的 ok。catch 分支没有这道守卫,而一旦取消让 sandbox 立即 reject,这个 reject 就先到达了 catch 分支,被当作运行的消息报告出来。catch 分支补上了对称的守卫。

并发默认值读取的是进程实际可用的 CPU。 os.cpus().length - 2、下限 1,会忽略 CPU 亲和性掩码和容器限制,而 os.cpus() 在某些沙箱里会返回空数组——这让每次运行都变成严格串行。现在读取 os.availableParallelism(),下限为 2:窗口为 1 会把每个 parallel() 都变成顺序执行,在小机器上悄悄抵消扇出的意义。两处面向模型、写明该公式的描述文字同步更新。

为什么需要

第一个是用户会真实撞上的。在脚本处于两次 dispatch 之间时按 ESC,几分钟内看不到任何反应;注册表里运行已被取消,但工具调用仍然开着,对话无法继续。这是引擎边缘的行为失当,而且修起来很便宜。

第二个是静默的:容器上的串行窗口会在没有任何信号的情况下劣化扇出。它是构造上的错误而非负载导致,所以在源头修掉。

为什么重开。 #9974 最初是这两个修复加一个 worktree 创建互斥锁。review 正确地发现:串行化让一个原本就存在的 .qwen/.gitignore 缺陷变成了确定性失败;修它又在一个 AgentTool 共用的服务里打开了符号链接面;分支上后来加入的一项改动——让 sandbox 的 unhandledRejection 观察者跨越 teardown 保持打开——在同一处经历了三轮 review、两条由修复引入的 Critical。这些每一项都是真实的工作,也都已从分支上砍掉,但这个 PR 背着十轮针对已不存在代码的讨论线程。那两块会作为独立 PR、从各自的失败模式出发重新设计后回来:互斥锁与 gitignore 根因一起;teardown 观察的问题则通过在 dispatch 边界挂观察者来解决,而不是在 teardown 时猜一个窗口。

审阅者测试计划

如何验证

cd packages/core
npx vitest run src/agents/runtime/ src/tools/workflow/ src/agents/workflow-run-registry.test.ts

实测结果:Test Files 18 passed | 1 skipped (19)Tests 929 passed | 6 skipped (935)。同一条命令在六个文件均处于 upstream/main 状态下少三个通过——本 PR 净增三个测试,没有回归。

三个现有测试被重新定向,而不是删除。它们钉住的是旧的结算行为——"被取消的运行要到 wall clock 剩余额度用完才结束"——而这恰恰是本 PR 要改的。它们的场景(暂停时取消、pausing 中取消并有 abort 之后的排空、进入 run() 时信号已中止)仍然是正确的场景;断言改为运行立即以取消结束,已中止的用例还额外断言没有任何 dispatch 被发出。另有一个测试 does not mirror plain-Error teardown rejections of an aborted run 从等待运行 resolve 改为期望它被取消:abort 的 reject 会排在同一 tick 完成的前面,所以取消赢下这场 race——这与 runner 一致,后者本来就会在 ok 结果之上优先报告已取消的注册表条目。

新增测试,按其主张:

  • cancellation settles the run at once while the script still runs its own finally——宿主侧的运行立即结束,而脚本的 finally 仍然执行,因为它的 dispatch 在中止信号上 reject 了。
  • does not run a script whose signal was already aborted before run()——用 dispatch spy 断言零次调用。
  • resolveConcurrencyLimit derives the default from availableParallelism, floored at 2——通过注入的并行度来源:0/1/3 → 2,6 → 4,18/64 → 16。
  • 两个已有的 peak === cap 扇出测试和 [1,16] 钳制测试现在用 availableParallelism() 和新下限来计算上限。

做了逐项隔离的变异检查,每次从 commit 恢复:从 race 中移除 abort 分支,恰好只让三个 abort 结算测试失败;下限改回 1,恰好只让下限测试失败;去掉 abort 分支上的 wall-clock 守卫,恰好只让 T40 超时测试失败;去掉 runner 的 catch 分支守卫,恰好只让三个外部终态的 runner 测试失败。四次变异中没有其它测试发生变化。

另外执行了:npx tsc --noEmit -p tsconfig.json(退出码 0)、npx prettier --checknpx eslint,六个文件均无问题。

证据(前后对比)

TUI 方面 N/A——改动位于 packages/core。行为上的前后对比就是上面的测试改写:"暂停时取消"场景此前断言 abort 后 20 ms 时 settled === false,然后 rejects.toThrow(/exceeded 200 ms of active time/);现在断言下一个 tick 时 settled === true,并 rejects.toThrow(/aborted \(cancelled\)/)

测试环境

OS 状态
🍏 macOS ⚠️ 未测试
🪟 Windows ⚠️ 未测试
🐧 Linux ✅ 已测试

packages/core 下通过 npx vitest run 运行单元测试,另加 tsc --noEmit、prettier 与 eslint。

风险与范围

  • 主要风险或取舍:取消现在会赢下与完成之间的同 tick 竞争。 如果用户恰好在脚本返回的那一瞬间取消,运行会报告为已取消而不是 ok。这与 runner 一致(它本就在 ok 结果之上优先报告已取消的注册表条目),也与用户的意图一致——但它改变了平局时哪一方获胜。其次:下限从 1 变为 2,因此报告 1–3 个可用 CPU 的机器现在会同时跑两个 agent,而不是一个;仍受 QWEN_CODE_MAX_WORKFLOW_CONCURRENCY 覆盖项约束。
  • 未验证 / 不在范围内:abort 分支结束的是宿主侧的运行;脚本自己的 promise 被留给它自行完成。一个在 run()finally 摘掉 adoption-escape hook 之后才落地的游离 dispatch rejection,正是从 fix(core): three run-lifecycle defects that silently cost a run #9974 砍掉、将作为独立 PR 回来的 teardown 观察问题——本 PR 既不扩大也不缩小那个窗口。暂停/恢复语义、暂停门、以及环境变量钳制均未改动。resolveConcurrencyLimit 新增了一个可注入的并行度来源用于测试;其默认值是 os.availableParallelism,没有任何调用方传入其它值。
  • 破坏性变更 / 迁移说明:对 API 或 schema 无。两处用户可见的行为变化:被取消的 workflow 现在会立即结束(此前最长要等到剩余的 wall-clock 额度用完),默认并发窗口现在是 max(2, min(16, availableParallelism() - 2)) 而非 max(1, min(16, os.cpus().length - 2))

关联 Issue

取代 #9974

qqqys and others added 16 commits August 25, 2026 10:44
Cancelling a workflow did not always end it. The sandbox raced the script
only against the wall clock, and its own in-source note admitted there was
no abort arm: a script not currently blocked on a dispatch — sitting in
ungated awaits, or simply hung — kept the foreground tool call open until
the banked remainder of the 30-minute clock expired, and the user watched a
cancelled run refuse to go away. Add the abort arm to the race so a cancel
settles the run at once, and refuse to start a script whose signal is
already aborted. The wall clock stays: it is the only bound on a script
awaiting a promise that never resolves. Its own timeout aborts the
controller before rejecting (T40), so the arm ignores that one abort — a
timeout is reported as the timeout it is, not as a cancellation.

The runner's catch branch gains the mirror of the guard its success branch
already had: when the registry entry was settled terminal from outside —
the dialog's cancel, the approval contingency's fail — the rejection that
follows is a consequence of that settlement, so report the entry's state
and message rather than the rejection's.

Sixteen concurrent `git worktree add` calls contended on `.git/worktrees`
and the index lock; the losers became null slots indistinguishable from an
agent that returned nothing. Provisioning is serialised behind a module-
level mutex — `async-mutex` is already a core dependency.

The concurrency default read `os.cpus()`, which ignores the CPU affinity
mask and container limits and can return an empty array, making every run
strictly serial. Read `os.availableParallelism()` instead, floored at 2:
a window of 1 turns every `parallel()` into a sequence.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MDuRWMhKbqEYZXbbLhn3en
Provisioning writes `<projectRoot>/.qwen/.gitignore` with a `worktrees/`
entry. That entry covers the directory but not the file, so writing it turns
a previously clean parent into one reporting `?? .qwen/`. Every caller that
fail-closes on a dirty parent — the workflow tool's isolation:'worktree'
provisioning, AgentTool's — then refuses every provision after the first,
naming uncommitted changes the user never made.

Serialising provisioning made that ordering deterministic rather than raced,
which is how the review caught it: the first dispatch of a fan-out succeeds
and every later one is refused.

The generated body now ignores itself, anchored so it hides only this file
and not a `.gitignore` under `.qwen/agents/`; genuine user content under
`.qwen/` still reports as a change. A file left by the previous version is
upgraded in place only when byte-identical to what that version wrote, so a
user-edited file is never rewritten.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MDuRWMhKbqEYZXbbLhn3en
…ock wait (QwenLM#9974)

Round-3 review fixes for the worktree provisioning lifecycle:

- Run the `.qwen/.gitignore` repair BEFORE the dirty-parent gate in both
  gated provision paths (workflow dispatch + AgentTool): an untracked
  pre-fix body is exactly what makes the gate report dirty, so the repair
  queued behind it never ran.
- Never rewrite a tracked legacy `.qwen/.gitignore` (ignore rules do not
  apply to tracked files; the rewrite would surface as a user-blamed
  modification) and skip symlinked/FIFO paths via `lstat` so a planted
  symlink cannot redirect the rewrite outside `.qwen/`.
- Race the provisioning-lock wait against the dispatch's signal so an
  abort while parked rejects at once as a cancellation instead of
  draining the acquire deadline and misreporting it as a stall.
- Fix the stale "no-op if exists" doc contract and witness the converse
  self-ignore invariant plus the lock's post-stall recovery.
…e witnesses (QwenLM#9974)

Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Back out the worktree-provisioning serialisation and everything the review
grew out of it, leaving this PR the two lifecycle fixes that were never in
question: the sandbox's abort arm (with the runner's matching catch-branch
guard) and the concurrency default reading availableParallelism().

Why they go together rather than one staying: the mutex is what makes the
pre-existing `.qwen/.gitignore` defect deterministic — provisioning writes a
file that does not ignore itself, the parent turns dirty, and every fail-
closed dispatch after the first is refused. Serialising guarantees that
ordering. Fixing the gitignore in turn opened a symlink surface in a service
AgentTool shares, and three rounds of review kept finding another path
component to harden. That is a coherent piece of work, but it is not a run-
lifecycle fix, and carrying it here took the PR from 6 files / +223 to 10
files / +963 with three open Critical findings.

Restored to main: gitWorktreeService.ts and its integ tests, agent.ts and its
tests, and the orchestrator's provisioning lock. Kept: the abort arm, the
runner guard, and the availableParallelism floor of 2.

The worktree race and the gitignore defect return as their own PR, where the
symlink hardening and the "the upgrade cannot reach the checkouts it exists
to repair" finding can be designed for rather than patched at.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MDuRWMhKbqEYZXbbLhn3en
…flow-run-lifecycle

# Conflicts:
#	packages/core/src/tools/workflow/workflow.ts
…ns' cleanup logs

Addresses both Critical findings from review round 8.

R8-2 (workflow-sandbox.ts): cancellation settles a run immediately by
design, so a detached async wrapper can still be awaiting an aborted
dispatch when `run()`'s finally detaches the adoption-escape hook. The
wrapper's rejection then lands with no 'unhandledRejection' listener on
the process and Node's default mode terminates the host — a workflow
cancel taking down a headless daemon.

Hold this run's listener across teardown instead, bounded three ways:
it waits only for THIS sandbox's in-flight dispatch count to reach zero
(counted host-side at the vmAsync boundary, the single chokepoint every
host call crosses), then a fixed two macrotask turns for the queued
rejection events, and an unref'd 5s cap retires it even if a dispatch
never settles. A dispatch-free run still retires synchronously, so the
wall-clock backstop's fake-timer tests keep their timer-free settlement.
A new run() on the same sandbox cancels a pending drain rather than
double-installing the same listener.

Extending the window makes abort-marked rejections reachable by the
hook for the first time, so it now applies observeDispatch's R11-30
teardown clause: a correctly cancelled run's log stays empty.

R8-1 (workflow-run-registry.ts): the approval contingency fails the
entry and aborts the handle while the sandbox is still collecting the
script's `finally` output. That final account reached setRecentLogs
after the entry was terminal, and the guard rejected every terminal
state except 'cancelled' — the returned failure kept the external
message while the persisted entry and snapshot silently lost the
cleanup diagnostics. Allow the mirror for a 'failed' entry when the
caller presents the handle that is settling right now; a replacement
run attaches its own handle and the settled run's finally releases
this one, so stale callbacks are still rejected. 'completed' stays
final.

Regressions, each mutation-checked against its own repair:
- listener retained at an immediately-cancelled run's settlement,
  released once the dispatch drains, log left empty
- a detached wrapper's late non-abort rejection reaches the run log
  instead of escaping
- registry: failed + settling handle writes; replaced or released
  handle does not
- runner end-to-end: an externally failed run keeps 'cleanup ran' in
  both persisted log projections

Claude-Session: https://claude.ai/code/session_01M7z4PccYfDPyyfg3oGr8V1
Bring the branch to main and keep only the two run-lifecycle fixes that
were never in question: the sandbox's abort arm (with the runner's matching
catch-branch guard and the wall-clock-fired guard that keeps a timeout
reported as a timeout) and the concurrency default reading
availableParallelism() with a floor of 2.

Removed: the rejection-observation drain, its hold-across-teardown window,
the registry's handle-keyed log allowance, and every test written for them.
That work answers a real question -- can a detached dispatch rejection after
a cancel take down a headless host -- but three review rounds each found the
next hole at the same site, two of them fix-induced. When every fix invites
the next finding the mechanism is the thing to question, not the bound, and
that deserves its own PR designed from the failure mode rather than patched
in the margins of this one.

Tree is exactly main plus the six-file patch this PR carried at bfe5d8d;
merge commit so nothing is rewritten.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MDuRWMhKbqEYZXbbLhn3en
@qwen-code-ci-bot

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

Copy link
Copy Markdown
Collaborator

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

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

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Thanks for the PR — a clean reduction of the earlier attempt, and the description is unusually honest about what was cut and why.

Template looks good ✓ — all sections present, including the bilingual translation.

Problem: both defects are real and concrete, not theoretical hardening. The cancellation one is an observed user-facing failure (ESC on a workflow between dispatches leaves the foreground tool call open until the banked wall-clock budget runs out) with a verifiable mechanism — the script-vs-clock race has no abort arm. The concurrency one is a correctness defect by construction: os.cpus() ignores affinity/container limits and can return an empty array, silently serializing every fan-out. The PR is the reopened, reduced form of #9974, and the cut pieces (provisioning mutex, teardown observation) are explicitly deferred to their own PRs.

Direction: aligned. Cancellation settling promptly is a core responsiveness fix for the workflow engine, and deriving the window from os.availableParallelism() is the right source. No auth/security/public-contract surface involved.

Size: core paths touched (packages/core/src/agents/runtime/, packages/core/src/tools/workflow/). Production logic: 108 lines (4 files); tests: 113 lines (2 files); generated/schema: 0. Well under every escalation threshold.

Approach: scope feels right — this is the minimal residue of #9974. One behavioral choice worth a human eye during review: the floor moves 1 → 2, so machines with 1–3 usable CPUs go from serial to two concurrent agents. The author defends it (a window of 1 turns every parallel() into a sequence) and documents it in Risk & Scope, with the QWEN_CODE_MAX_WORKFLOW_CONCURRENCY override untouched — reasonable, but it is a semantic change, not just a CPU-reading fix.

Risk: no elevated risk signals — none of the changed files match the high-risk paths from the revert-history analysis.

Moving on to code review. 🔍

中文说明

感谢贡献——这是对先前尝试的一次干净裁剪,描述非常坦诚地说明了砍掉了什么、为什么。

模板完整 ✓ —— 所有章节齐全,包含中文翻译。

问题:两个缺陷都是真实具体的,不是理论性加固。取消问题是用户可观测的故障(在 dispatch 间隙按 ESC,前台 tool call 会一直挂到剩余的 wall-clock 额度耗尽),机制可验证——script 与 wall-clock 的 race 没有 abort 分支。并发问题是构造性正确性缺陷:os.cpus() 忽略 affinity/容器限制,且可能返回空数组,导致所有 fan-out 被静默串行化。本 PR 是 #9974 重新开启、缩小范围后的版本,被砍掉的部分(provisioning 互斥、teardown 观察)明确留给各自的独立 PR。

方向:对齐。取消立即结算是 workflow 引擎的核心响应性修复,用 os.availableParallelism() 推导窗口是正确的来源。不涉及 auth/安全/公共契约面。

规模:触及核心路径(packages/core/src/agents/runtime/packages/core/src/tools/workflow/)。生产逻辑:108 行(4 个文件);测试:113 行(2 个文件);生成/schema:0。远低于所有升级阈值。

方案:范围合理——这是 #9974 的最小残留。审查时值得人眼关注一处行为选择:下限从 1 提到 2,报告 1–3 个可用 CPU 的机器会从串行变为同时跑两个 agent。作者的理由成立(窗口为 1 会让每个 parallel() 退化成顺序执行),并已在风险与范围中说明,QWEN_CODE_MAX_WORKFLOW_CONCURRENCY 覆盖项不变——合理,但这是语义变化,不只是 CPU 读数修复。

风险:无升级风险信号——改动文件均未命中基于 revert 历史的高风险路径。

进入代码审查 🔍

Qwen Code · qwen3.8-max

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

@qwen-code-ci-bot

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

Copy link
Copy Markdown
Collaborator

Code review

No blockers found. The implementation matches what I'd have proposed independently — abort arm on the race, guarded so the watchdog's own abort doesn't read as a cancellation, and the mirror guard in the runner's catch branch. Details:

The abort arm is race-safe against the wall clock. wallClockFired is set synchronously inside the timeout callback, before abort() is called, and abort listeners run synchronously during .abort() — so settleOnAbort always sees the flag when the timeout fires and correctly stays silent; a real cancel (no flag) rejects the race at once. Either way Promise.race keeps handlers on all three promises, so the script's later rejection can't go unhandled, and the new listener is removed in the finally alongside the existing one. The re-arm listener stays as a backstop — belt and braces, as the comment says.

The pre-start check is in the right place. Throwing before runInContext means a signal already aborted at run() entry never executes model-authored code — the new dispatch-spy test pins exactly that. (A cancel landing in the tiny window between the check and addEventListener would fall back to the wall-clock bound — same characteristic the existing re-arm listener has always had, not a regression.)

The runner's catch-branch guard is a correct mirror. It returns the entry's terminal state instead of the rejection's message, and dropping entry?.status === 'cancelled' from the condition below it is safe: terminal entries now return earlier, and registry.cancel()/fail() are no-ops on terminal entries anyway (workflow-run-registry.ts:586). The failed-status branch uses entry.error ?? message, which is strictly more informative than the success path's static fallback.

Concurrency change is sound. os.availableParallelism() exists since Node 18.14 and the repo requires ≥22; os is already imported, and both production callers (workflow-runner.ts:264, workflow-orchestrator.ts:1625) use the default — the new parameter is a test-injection seam only. Both model-facing description strings are updated; no stale max(1, …) formula remains anywhere else. The floor 1→2 is a real semantic change (documented honestly in Risk & Scope, override untouched) — worth a maintainer's eye, not a blocker.

Tests were re-targeted, not deleted. The three settlement tests keep their scenarios (cancel while paused, cancel mid-pausing with a post-abort drain, already-aborted signal) and assert the new contract; the same-tick race test now expects cancel to win, matching the runner's existing preference for a cancelled registry entry over an ok outcome.

One non-blocking observation: workflowCancelledError() is a plain Error with name = 'AbortError', while this file's existing convention is new DOMException(…, 'AbortError') (see the note near line 2074 of workflow-orchestrator.ts). isAbortError matches on the name, so both classify identically — functionally fine, just a consistency nit.

Testing — CI evidence (this is an unattended run; PR code was not executed here). The unit suite is still running on the reviewed commit as of this write-up; the table below is a snapshot and the finalize job will refresh it once CI settles. The three cancelled checks are bot-orchestration jobs (review-pr / fallback-comment / delay-automatic-review) cancelled by design, not failures.

Final CI results for 0b82f5d (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
Integration Tests (no-AK, No Sandbox) ✅ 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,失败项排在最前。

Not verified here: that a live ESC/dialog cancel actually settles the foreground tool call immediately in the real product, and that a wall-clock timeout still surfaces as a timeout — CI green would prove the tests pass, not that the behaviour holds end-to-end. Sandboxed verification would settle this: @qwen-code /verify — an A/B proof that the abort arm is load-bearing (cancel settles at once, timeout is not misreported as cancel, floor-2 default sticks). This is a fork PR, so that's a sponsored run: a maintainer posts the trigger, which approves exactly the head it names; the run carries a pre-execution risk screen and a full workspace wipe, but its report should still be read with the same skepticism as the fork's own CI logs.

中文说明

代码审查未发现阻塞问题。实现与独立推导的方案一致:给 race 增加 abort 分支,并加守卫避免把 watchdog 自身的 abort 误判为取消;runner 的 catch 分支补上与成功分支对称的守卫。

要点:超时回调在调用 abort() 之前同步置位 wallClockFired,而 abort 监听器在 .abort() 期间同步执行,因此超时时 abort 分支必然看到标志并保持沉默,真实取消则立即结束 race;Promise.race 对三个 promise 都保持挂接,脚本后续的 rejection 不会变成未处理拒绝,新监听器在 finally 中与既有监听器一并移除。预启动检查位于 runInContext 之前,已 abort 的信号不会执行任何模型代码(新的 dispatch spy 测试钉住了这一点);检查与 addEventListener 之间的极小窗口会退回 wall-clock 兜底,与既有 re-arm 监听器一直以来的特性相同,不是回归。

runner catch 分支守卫正确地镜像了成功分支:返回条目的终态而非 rejection 的消息;随后条件中移除 entry?.status === 'cancelled' 是安全的——终态条目已提前返回,且 registry.cancel()/fail() 对终态条目本就是 no-op。并发改动可靠:os.availableParallelism() 自 Node 18.14 可用,仓库要求 ≥22;两处生产调用均用默认参数,新参数只是测试注入口;两处面向模型的描述字符串均已更新,无遗漏的旧公式。下限 1→2 是真实语义变化(风险与范围中已如实说明,覆盖项不变),值得维护者过目,不构成阻塞。测试是改写而非删除,场景保留、断言改为新契约。一个非阻塞观察:workflowCancelledError() 用普通 Error 改名,而本文件既有惯例是 DOMException(…, 'AbortError')——isAbortError 按 name 匹配,功能等价,仅为一致性小问题。

测试证据:本次为无人值守 CI 运行,未在此执行 PR 代码;审查时单元测试仍在该提交上运行,表格为快照,CI 结束后由 finalize 任务刷新。三个 cancelled 检查是机器人编排任务按设计取消,不是失败。未验证:真实产品中 ESC/对话框取消是否立即结束前台 tool call、超时是否仍按超时呈现——可沙箱验证:@qwen-code /verify(A/B 证明 abort 分支不可或缺)。这是 fork PR,需维护者发起赞助运行,并对报告保持与 fork CI 日志同等的审慎。

Qwen Code · qwen3.8-max

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

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Confidence: 4/5 — clean, minimal review with only non-blocking nits; approval deferred until CI lands green on the reviewed commit.

Stepping back: this is exactly what a reopened-and-reduced PR should look like. My independent proposal for both defects — an abort arm on the race with a guard distinguishing the watchdog's own timeout-abort from a user cancel, plus the catch-branch mirror in the runner — is what the diff does, and I didn't find a simpler path it missed. The motivation holds up: the cancellation defect is user-observable (ESC between dispatches left the tool call open for minutes) with a verifiable mechanism, not a hypothetical; the concurrency default was wrong by construction in containers. Every edit in the diff is necessary — the pieces that weren't (provisioning mutex, teardown observation) were explicitly cut to their own PRs, which is the right call after the earlier rounds.

The tests are the strongest part: scenarios re-targeted rather than deleted, the same-tick tie-break made explicit and matched to the runner's existing preference, and an injectable parallelism source instead of mocking os. The remaining nits are the DOMException-vs-plain-Error consistency point and the floor 1→2 semantic change, both named in the review comment — neither blocks.

If I were maintaining this in six months, the comments in the race would thank the author — they explain the one genuinely subtle thing here (why the timeout's own abort must not read as a cancellation) at exactly the site where it matters.

The unit suite and one integration run were still in flight on the reviewed commit at write-up, so approval is deferred until CI lands green on 0b82f5d09cf66a49355bfd4755d62c6bb0a9bfe2; the finalize step will pin the approval to that commit or withhold it if anything lands red.

中文说明

总体评价:这正是"重新开启并缩小范围"的 PR 应有的样子。我对两个缺陷的独立方案——给 race 加 abort 分支、用守卫区分 watchdog 自身的超时 abort 与用户取消、runner catch 分支补镜像守卫——与 diff 完全一致,没有更简单的路径被遗漏。动机成立:取消缺陷是用户可观测、机制可验证的,并发默认值在容器中是构造性错误。diff 中每一处改动都是必要的——不必要的部分(provisioning 互斥、teardown 观察)已被明确砍掉留给独立 PR。

测试是最强的部分:场景改写而非删除,同 tick 平局的胜负被显式化并与 runner 既有偏好一致,用可注入的并行度来源代替 mock os。剩余小问题只有 DOMException 与 plain Error 的一致性、以及下限 1→2 的语义变化,均已在审查评论中说明,不构成阻塞。

审查时该提交上的单元测试与一个集成测试仍在运行,因此批准推迟到 CI 在该提交上全绿;finalize 步骤会把批准钉在该提交上,若有检查变红则不予批准。

Qwen Code · qwen3.8-max

Reviewed at 0b82f5d09cf66a49355bfd4755d62c6bb0a9bfe2 · 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. ✅

@qwen-code-dev-bot qwen-code-dev-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.

Pre-merge gate review — APPROVE

Approved on head 0b82f5d0.

  • History: no open review threads or unresolved findings; the CI bot's prior review already approved with green CI.
  • CI: all verification suites pass on this exact head (unit + integration no-AK + web-shell E2E + Java matrix + CVE/secret scans). Only the repo's automated review-pr workflow is still queued; it is not a validation gate.
  • Independent pass over the diff (4 production files):
    • The sandbox abort arm is sound: listener registration is synchronous within the same turn as the already-aborted check (an AbortSignal also fires late-registered listeners immediately), the wallClockFired latch keeps a timeout reported as a timeout rather than a cancellation, both listeners are removed in finally, and the losing race arm stays handled.
    • The runner's new catch-branch guard correctly subsumes the removed entry?.status === 'cancelled' OR-arm (cancelled is terminal) and mirrors the existing success-path guard; the foreground-abort and callerWasAbortedBeforeStart arms are preserved for non-terminal entries.
    • resolveConcurrencyLimit via availableParallelism() (respects affinity/container limits, Node ≥ 19.4 — this project requires 22) with the injectable default param is backward-compatible; the [1, HARD_MAX] env-override semantics are unchanged.
    • No new Critical issues found; the one deliberate behavior change (cancel now wins the same-tick race with completion) is documented in the PR itself and consistent with the runner's existing terminal-entry preference.
  • Tests: re-targeted scenarios verified as re-targeted, not deleted; net +3 tests, matching the plan.

No blocking issues found.

中文:head 0b82f5d0 独立复查无新 Critical;abort 分支、runner 终态守卫、并发默认值三处逻辑均验证成立,历史线程无未决项,CI 验证套件全绿。同意合入。

— qwen-code-dev-bot · pre-merge final gate

@qqqys
qqqys enabled auto-merge August 29, 2026 08:17
@qqqys
qqqys added this pull request to the merge queue Aug 29, 2026
Merged via the queue into QwenLM:main with commit 348301c Aug 29, 2026
148 of 152 checks passed
@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Released in v0.23.0.

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