fix(core): release node-pty's conout worker after every PTY on Windows - #11313
Conversation
On Windows every completed tool call orphaned one headless `conhost.exe` (~8 MB) for the lifetime of the CLI process: #11303 measured 347 of them (~2.8 GB) under a VS Code Companion session after 12 h, growing 1:1 with tool shell commands. `taskkill` (`windowsKillPid`, `performCancelKill`) owns the *shell* process. It cannot reach the pseudo-console host, which is a separate process bound to the HPCON handle this process holds, not a descendant of the shell. Only `ptyProcess.kill()` releases it — it is the sole caller of node-pty's `WindowsPtyAgent.kill()` -> `conptyNative.kill()` -> `ClosePseudoConsole()`. On a natural shell exit node-pty runs `_$onProcessExit`, which only flushes buffered data and destroys its sockets, so the pseudo-console stays open. Three teardown paths dropped the PTY without that call: - `disposeForegroundPtyResources` — the healthy path. Its `isPtyActive` guard correctly skips taskkill when the shell exited cleanly, and that is exactly the per-tool-call leak. - the background-promote settle path — which has already removed the pid from `activePtys`, so the process-exit `cleanup()` cannot reclaim it either. - `WebTerminalRegistry.release()` — skips `killPtyTree` entirely for a session whose shell already exited. Release the host in all three. This is narrower than #5892, not wider: it closes only the HPCON we opened, so unlike a `taskkill /t` it can never reach a third-party application (the #6067 collateral-kill regression). win32-only, deliberately — there is no ConPTY host elsewhere, and `UnixTerminal.kill()` would signal an already-exited, possibly recycled pid. Fixes #11303
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
@yiliang114 the investigation behind this is thorough, but the PR body doesn't use the repository's pull request template, so admission stops here — before code review. None of the required sections are present: ## What this PR does, ## Why it's needed, ## Reviewer Test Plan (with ### How to verify, ### Evidence (Before & After), ### Tested on), ## Risk & Scope, ## Linked Issues, and the 中文说明 <details> block. The six headings that are there ("The leak", "Acceptance test", "Not in this PR", …) are your own.
Template: https://github.com/QwenLM/qwen-code/blob/main/.github/pull_request_template.md
The good news is this is a re-shape rather than a rewrite — most of the content already exists and just needs to move: "The leak" → Why it's needed, the three teardown paths → What this PR does, "Acceptance test" → How to verify, "Not in this PR" → Risk & Scope (it already reads as a scope/tradeoff list), and the opening Fixes #11303 → Linked Issues.
Two sections need genuinely new content, and for this particular change they are the ones a maintainer needs most:
- Tested on — the change is win32-only and you state plainly that it was not run locally. The per-OS rows (Windows
⚠️ , macOS/Linux N/A) are what makes that visible at a glance instead of buried in the last paragraph. - Evidence (Before & After) — the
conhost.exe --headlesscount before and after N tool shell commands from #11303 is exactly the right evidence. If you cannot produce it from this environment, writeN/Awith the reason, so the gap is explicit rather than something the reviewer has to infer.
Worth saying why this is not just box-ticking here: the diff lands in the shell execution service and the web terminal registry, which are on this repo's revert-correlated path list, and the central claim is Windows runtime behaviour that the unit tests cannot fully settle — the acceptance criterion is a real-machine resource count. The template's evidence sections are the only place that gap gets recorded.
Once the body matches the template, re-run with @qwen-code /triage and the code review will pick up from there.
中文说明
@yiliang114 这个 PR 背后的排查很扎实，但正文没有使用本仓库的 pull request 模板，所以准入检查就停在这里——在代码审查之前。所有必需章节都缺失：## What this PR does、## Why it's needed、## Reviewer Test Plan（含 ### How to verify、### Evidence (Before & After)、### Tested on）、## Risk & Scope、## Linked Issues，以及 中文说明 的 <details> 区块。现有的六个小标题（"The leak"、"Acceptance test"、"Not in this PR" 等）都是你自定义的。
模板地址：https://github.com/QwenLM/qwen-code/blob/main/.github/pull_request_template.md
好消息是这只是重新排版，不是重写——大部分内容已经存在，只需要挪位置："The leak" → Why it's needed，三条 teardown 路径 → What this PR does，"Acceptance test" → How to verify，"Not in this PR" → Risk & Scope（它本身就已经是一份范围与取舍清单），开头的 Fixes #11303 → Linked Issues。
有两个章节需要补充新内容，而对这个改动来说，它们恰恰是 maintainer 最需要的：
- Tested on —— 改动仅限 win32，而你也明确说明本地没有运行过。逐系统的状态（Windows
⚠️ ，macOS/Linux N/A）能让这一点一眼可见，而不是埋在最后一段里。 - Evidence (Before & After) —— #11303 中"执行 N 个 tool shell 命令前后
conhost.exe --headless的数量"正是最合适的证据。如果在当前环境无法产出，请写N/A并说明原因，让这个缺口显式可见，而不是靠 reviewer 自己推断。
也说明一下为什么这在这里不只是走形式：diff 落在 shell execution service 与 web terminal registry，属于本仓库 revert 相关性较高的路径；而核心结论是 Windows 运行时行为，单测无法完全验证——验收标准是真机上的资源计数。模板的证据章节正是唯一会把这个缺口记录下来的地方。
正文补齐模板后，用 @qwen-code /triage 重新触发，代码审查会继续。
— Qwen Code · qwen3.8-max-2026-09-02
The ACP tool dispatch called `invocation.execute(signal, onToolProgress)`
with no third argument, so `ShellToolInvocation` fell back to
`shellExecutionConfig ?? {}`. The PTY was then sized 80x30 from
`shellExecutionService`'s own fallbacks — not even `Config`'s 80x24
default — and `pager`, `showColor` and `maxBufferedOutputBytes` were
dropped for every ACP tool call.
The TUI scheduler already passes `config.getShellExecutionConfig()`
(coreToolScheduler); do the same here.
This is how the process tree in QwenLM#11303 was identified: the reporter's
orphans were all `conhost.exe --headless --width 80 --height 30`, which
is this fallback's signature and nothing else's. The leak itself is a
separate defect, fixed in QwenLM#11313.
Follow-up to the previous commit, correcting how the host is released. `ptyProcess.kill()` does three things on Windows, and only two of them are wanted here. Besides `ClosePseudoConsole` and disposing the conout worker, node-pty's `WindowsPtyAgent.kill()` forks a helper to run `GetConsoleProcessList` on the shell pid and then `process.kill()`s every pid it returns. On the healthy path the shell has already exited, so `AttachConsole` throws, the helper dies with an uncaught error, node-pty's 5s timeout falls back to `resolve([shellPid])` — and we `TerminateProcess` a pid `ClosePseudoConsole` just freed for reuse. That is exactly the #6067 collateral-kill failure mode, and the previous commit would have fired it on every tool call. Drive the two teardowns directly instead, extracted to `conpty-host.ts`. The shape is checked before use and the release degrades to a no-op if node-pty's internals ever change, so a dependency bump can only bring the leak back, never a kill we did not intend. Verified unchanged in 1.2.0-beta.15. This also fixes a second leak found while tracing the first: node-pty runs a `worker_threads` Worker per PTY to read the conout pipe, and `ConoutConnection.dispose()` is likewise reachable only from `kill()`. That accounts for the 353 threads the reporter measured against 347 orphaned conhosts — one leaked worker each. `WebTerminalRegistry` now goes through the same helper rather than `pty.kill()`, via a new optional `releaseHost()` on `WebTerminalPty`. `performCancelKill` deliberately keeps using `ptyProcess.kill()`: it early-returns unless the shell is still running, so the console process list is real there and killing it is the intended tree-kill fallback for when taskkill fails to launch.
yiliang114
changed the title
node-pty's native `PtyKill` looks its baton up by pty id and calls `ClosePseudoConsole` without removing the entry from its handle list, so closing the same pseudo-console twice is a double-free on an already-closed HPCON — undefined behavior in-process, not a catchable throw. The cancel path reaches teardown twice: `performCancelKill` runs `ptyProcess.kill()` (which closes the host itself), and the finalizer then runs `disposeForegroundPtyResources`. Without a guard the release added here would have turned that into a crash. Track released PTYs in a WeakSet, and have the two sites that call node-pty's own `kill()` — `performCancelKill` and the process-exit `windowsStrategy.killPty` — record it, so a later release is a no-op. `performCancelKill` keeps using `kill()` on purpose: it early-returns unless the shell is still running, so node-pty's console-process-list lookup resolves for real there and killing it is the intended fallback for a taskkill that never launched.
`Test (windows-latest)` is skipped on PRs, so the web-terminal release test only ever ran its POSIX half — the win32 assertions were dead code in CI, on the platform the whole fix is about. `web-terminal-registry.test.ts` now steers `os.platform()` (the only thing conpty-host reads; `killPtyTree` branches on `process.platform`, so it is untouched) and asserts both halves: the host and conout worker are released on win32, and nothing is touched elsewhere. The `os` mock passes everything else through, since Storage — reached via debugLogger — needs the real `homedir()`/`tmpdir()`. The shellExecutionService tests already mocked `os.platform`, so those were exercising the win32 paths on Linux runners.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — Test (windows-latest, Node 22.x) was skipped in CI (ci.yml:1642-1652 gates it to merge_group/schedule/workflow_dispatch, so it never runs on a pull request) and its suite could not run locally: this review's build and tests ran on Linux, where every new code path sits behind the win32 platform guard.
Not explored to full depth (tool budget reached): "agent reverse-audit (round 1)": confirming the WindowsPtyAgent internals releaseConPtyHost drives ( _pty , _useConptyDll , _ptyNative.kill , _conoutSocketWorker.dispose ) against real …; "agent reverse-audit (round 3)": verifying @lydell/node-pty 's WindowsPtyAgent internals (the _pty / _useConptyDll / _ptyNative.kill / _conoutSocketWorker field names and kill() 's ….
— qwen3.8-max via Qwen Code /review (v0.23.0)
…alive performCancelKill runs `spawnSync(taskkill /f /t)` immediately above this call, by design, so on the normal cancel the shell is already dead when kill() runs and node-pty's console-process-list lookup takes the 5 s `[innerPid]` fallback (windowsPtyAgent._getConsoleProcessList has only a message listener plus that timeout) instead of resolving for real. The comment asserted the opposite invariant, which would mislead the next maintainer on a file already flagged as revert-correlated. Comment only, no behaviour change. kill() stays as the fallback for a taskkill that never launched, and noteConPtyHostReleased stays paired with it so the finalizer does not close the same pseudo-console twice. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Patrol-Run: qwen-pr-closeout/jmtrow8ztpw
…uard Two coverage gaps in the #11303 release path, both measured green before: - the release in firePostSettle sits above the `!postPromote?.onSettle` early return on purpose, but no test told the two placements apart: sliding the call one statement down kept the whole suite green while every promoted shell whose caller passes postPromote without onSettle leaked a conhost and a conout worker. The existing onData-only test now asserts the release fired. - the try/catch around `_conoutSocketWorker.dispose()` had no test at all. It is load-bearing because of where the release runs: in finalize()'s finally, immediately before activePtys.delete(pid) and after the result already settled. An escaping throw skips that delete and leaves a finished pid registered for the process-exit `taskkill /f /t`, against a pid Windows may have recycled. The new case drives dispose to throw and asserts a following cleanup() does not tree-kill that pid. Both run on Linux (the platform is mocked), so neither is a win32 skip. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Patrol-Run: qwen-pr-closeout/jmtrow8ztpw
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
4 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- R1-2 the release note records a host close that may never have happened — already reported (comment 3952434066)
- R1-3 the dependency premise is neither observable nor pinned — already reported (comment 3952434073)
- R1-6 a promote without postPromote attaches no settle listener, so that path releases nothing — already reported (comment 3952434096)
- R1-8 the registry releases the host only at release(), never at exit — already reported (comment 3952434110)
Not reviewed: build-and-test on win32 — Test (windows-latest, Node 22.x) was skipped in CI (gated to merge_group/schedule/workflow_dispatch, so it never runs on a pull request); this review's build and tests ran on Linux, and every new code path sits behind a win32 platform guard whose native half the unit tests stub, so no run in this review exercised real Windows ConPTY behaviour.
Not explored to full depth (tool budget reached): "agent 2": none — every check I set out to run completed within the tool budget; the item above is unverifiable on this platform, not cut short.; "agent reverse-audit (round 2)": verifying node-pty's Windows internals against the pinned 1.2.0-beta.10 — only @lydell/node-pty-linux-x64 is installed in this worktree ( node_modules/@lydell…; "agent reverse-audit (round 2)": mutation-running the two 784bd15c9d test additions — I read them and traced what each asserts against disposeForegroundPtyResources ( :1793 , called from th…; "agent 1d": none — no check was cut short..
— qwen3.8-max via Qwen Code /review (v0.23.0)
chiga0
left a comment
chiga0
left a comment
There was a problem hiding this comment.
1 blocker · 1 minor · conout-worker leak is fixed; conhost.exe leak is not
Cross-check against prior reviews
R1-1 from the previous round (qwen-code-ci-bot) was confirmed by the author and is still unresolved at this head — it is re-stated as A1-1 below. Findings R1-2, R1-3, R1-6, R1-8 are independently confirmed here and are in the ledger as A1-2. R1-4, R1-5, R1-7 were fixed at this head; they are not re-raised.
A1-1 — Blocker · packages/core/src/services/conpty-host.ts:108
releaseConPtyHost's nativeKill call is a no-op after onExit fires; the conhost.exe leak is not fixed
The fix's claim is that calling _ptyNative.kill(ptyId, ...) after the shell exits closes the conhost.exe --headless process. It does not, at @lydell/node-pty 1.2.0-beta.10 (and 1.2.0-beta.15):
conpty.cc in that tag's native source runs the following sequence on the exit-watcher thread:
WaitForSingleObject(baton->hShell, INFINITE)
→ GetExitCodeProcess(...)
→ CloseHandle(baton->hShell)
→ assert(remove_pty_baton(baton->id)) // ← baton erased here
→ tsfn.BlockingCall(exit_event, callback) // ← JS onExit delivered here
PtyKill (called as nativeKill) at lines 546–566 of the same file is:
const pty_baton* handle = get_pty_baton(id);
if (handle != nullptr) {
pfnClosePseudoConsole(handle->hpc);
}Since the baton is erased before onExit, get_pty_baton(id) returns nullptr, the if branch is skipped, and ClosePseudoConsole is never called — silently, with no throw, so debugLogger.warn in releaseConPtyHost never fires either. Every call site in this PR fires after onExit: disposeForegroundPtyResources (from finalize()'s finally), the promote-settle exit arm, and WebTerminalRegistry.release()'s session.exited branch. So the conhost.exe --headless accumulation from #11303 is not reduced by this change.
The _conoutSocketWorker.dispose() half of releaseConPtyHost does not depend on the baton and likely works correctly — the thread count fix stands.
The author independently confirmed this finding and escalated to a maintainer decision rather than fixing it in this PR. A fix would need to call ClosePseudoConsole while the baton still exists — either by storing the HPCON handle before onExit fires, by extending @lydell/node-pty to expose a dedicated pre-exit hook, or by calling teardown from an earlier lifecycle point than onExit.
A1-2 — Minor · packages/core/src/services/shellExecutionService.ts:2131
A background abort without postPromote attaches no settle listener, so that PTY releases nothing
firePostSettle (which calls releaseConPtyHost) is attached inside if (postPromote) { at line 2161. When performBackgroundPromote runs with postPromote undefined, neither postPromoteExitDisposable nor postPromoteErrorDisposable is created, so firePostSettle never fires. The pid was already deleted from activePtys at line 2034 during the promote itself, so cleanup()'s killPty loop cannot reach it either, and the foreground disposeForegroundPtyResources is excluded by exitDisposable.dispose() at line 2011. Both leaked resources — the conout worker (which releaseConPtyHost does close) and the conhost.exe (which it currently cannot, per A1-1) — accumulate for the life of the CLI.
Confirmed by mutation probe (bot): running a win32 promote with postPromote: undefined and checking that releaseConPtyHost is never called.
Mitigation options: (a) add a postPromote-less exit listener that calls releaseConPtyHost; (b) make the promote path unconditionally attach a settle listener regardless of postPromote.
Coverage note: CI's Test (windows-latest, Node 22.x) is gated to merge_group/schedule/workflow_dispatch and never runs on a pull request, so every new code path in this diff sat behind a win32 platform guard that no CI run exercised on real Windows ConPTY behaviour. The conout-worker half of the fix is testable on Linux (no native dependency); the conhost.exe half is not, and A1-1 is the consequence.
Three review findings, all on the release path this PR added: - Attach the post-promote settle listener unconditionally. It was gated on `postPromote`, so a background promote that passed no handlers dropped the pid from activePtys, disposed the foreground exit listener, and left nothing that could ever release the conout worker or the ConPTY host. Only the caller forwarding stays gated: firePostSettle early-returns on `!postPromote?.onSettle` after the reap and the release, so the PR-2 detach-everything contract still holds for callers that did not opt in. Attaching the 'error' listener unconditionally also keeps a post-promote pty error from being emitted on an EventEmitter with no listener. The PR-2.5 compat test now pins 2 onExit registrations and still asserts no data listener and no caller callback. - Stop recording a ConPTY release that never happened on the cancel path. WindowsTerminal.kill() runs its whole teardown through _deferNoArgs, which queues it until `_isReady` — set only on the conout socket's first data byte — so a cancel before the shell's first output byte queued a teardown that may never run while noteConPtyHostReleased permanently suppressed the finalizer's release. The note now sits inside the try, after kill(), and is skipped when the teardown was deferred; reading the optional `_isReady` degrades to the previous behavior if the field is ever renamed. - Pin the activePtys invariant directly in the dispose-throws test instead of only through the process-exit taskkill argv, so the conout dispose guard keeps a witness if windowsStrategy.killPty is ever refactored. Tests: 145 passed in src/services/shellExecutionService.test.ts. Each new or changed assertion was mutation-checked red against the unfixed source (restore of the `if (postPromote)` wrapper, unconditional note, and removal of the dispose guard in conpty-host.ts). Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Patrol-Run: qwen-pr-closeout/jmts86p0gqr
qwen-code-dev-bot
left a comment
qwen-code-dev-bot
left a comment
There was a problem hiding this comment.
门禁 Review — head b755bba247
决定：REQUEST_CHANGES。 不是因为这轮改动写得不好——conpty-host.ts 的降级方向和防重复释放的推理我都读着是对的——而是这个 PR 的核心断言还压在一个未被反驳、作者自己也承认属实的 [Blocker] 上，而且最新那个提交标题（"close the two remaining ConPTY release gaps"）之后仍然有 2 条 Critical 挂着没解。这种状态下我不签。
我在 head 上自己核到的
- 释放只有三个入口：
shellExecutionService.ts:1864（taskkill 之后，注释说明 shell 由 taskkill 拥有、这里只管伪控制台和 conout worker）、:2131（取消/收尾路径），以及web-terminal-registry.ts:289的releaseHost。noteConPtyHostReleased在:620和:2386登记外部已关闭。 - registry 那个入口确实只在显式回收时被调用：
:283的exitDisposable = spawned.onExit(handleExit)里handleExit只置exited/error标记，不碰releaseHost。所以一个已经退出的网页终端会继续占着conhost.exe（约 8 MB）和一个 worker 线程，直到 15 分钟 idle 回收——这正是 #11303 要收的那类残留，只是换了一层。这条（R1，web-terminal-registry.ts:430）作者回复是"确认属实、这轮不做"。 conpty-host.ts:99-106的形状检查降级方向我认同：内部结构对不上时宁可继续泄漏也不 kill，注释写的理由（泄漏可靠重启解决，误杀回收 pid 不可恢复）与#6067那类护栏一致，releasedHosts用 WeakSet 避免留引用也对。这些不需要改。
为什么卡住不签
挂着的两条 Critical 和 chiga0 的 [Blocker] 是同一个缺陷的三次记录：releaseConPtyHost 依赖 agent._ptyNative.kill(ptyId, useConptyDll) 去走 ClosePseudoConsole，而 node-pty 原生退出监视线程在把 onExit 回调交给 JS 之前就已经把该 pty 的 baton 摘掉；于是从 onExit 之后进来的那一路调用是对一个已经不存在的 baton 操作，kill() 静默返回，conhost.exe 活下来。也就是说：这个 PR 标题承诺的"每个 PTY 退出后都释放 ConPTY host"，在最主要的触发路径上不成立，而 :108 外面那圈 try/catch 只会记录"没抛异常"，不会告诉你它没做事。node-pty 内部时序我没法在这次复核里独立跑到（要 C++ 侧断点），所以我按证据强度看：提出者是本轮 reviewer、给了具体行与调用序，作者在两条 thread 上的回复都没有否认机制，只说"已升级给 maintainer 决定、本轮故意不改"。
要什么：三选一，都能让这条签掉。
- 把释放点挪到 baton 还在的时候（在原生摘除之前，或从仍持有 baton 的那一侧发起），并补一条能证明"这次调用真的关掉了伪控制台"的用例——现在最缺的就是这个，否则任何"我们释放了"的说法都只是断言。
- 或者接受
onExit之后关不掉这个事实，把 PR 的标题、conpty-host.ts:15-17的"只会带回泄漏、不会误杀" clearance 措辞、以及:29-32关于PtyKill不移除 handle 的前提，一律改成代码实际保证的范围——并在 thread 上留 maintainer 的书面决定。 - 顺手把 registry 那条补上（
handleExit里释放，或退出即释放），它和 1/2 是同一类"claim 与生命周期不匹配"。
其他
conpty-host.ts:15 的依赖前提（"每次 bump 都重新核对过、字段全可选所以最坏是退回 no-op"）也被单列成一条未解 Suggestion：目前它没有测试或 CI 断言把 node-pty 的内部形状钉住，所以那句"bump 只会带回泄漏"是靠读代码得出的结论，需要一条形状断言（哪怕是 conpty-host.test 里对 _ptyNative.kill 存在性的检查）来支撑。测试面还有那条：整套用例都在 mock 的 internals 上跑，nativeKill 成为 no-op 这种情况在 CI 里不可见。
CI 事实。 这个 head 上 Test (ubuntu-latest) 22m28s、Lint & Static 24m20s 绿，review-pr 还在跑，没有红。CI 不是我要卡的东西——卡的是那条 [Blocker] 需要一个决定，而不是一条"稍后处理"。
chiga0
left a comment
chiga0
left a comment
There was a problem hiding this comment.
Round 2 — head b755bba
Prior findings
| Id | Severity | Round-1 status | Round-2 status |
|---|---|---|---|
| A1-1 | Blocker | Confirmed | Still present — code unchanged; baton removed before onExit fires |
| A1-2 | Minor | Confirmed | Fixed — settle listener now attached unconditionally |
R2-1 (= A1-1) — Blocker · packages/core/src/services/conpty-host.ts:108
releaseConPtyHost's nativeKill.call is a no-op — ClosePseudoConsole is never called, and the conhost.exe --headless accumulation from #11303 remains.
Confirmed by reading the actual microsoft/node-pty@v1.2.0-beta.10 C++ source (src/win/conpty.cc):
// Exit-watcher thread (SetupExitCallback):
assert(remove_pty_baton(baton->id)); // line 106 — baton erased from ptyHandles
auto status = tsfn.BlockingCall(exit_event, callback); // line 108 — JS onExit delivered// PtyKill, lines 546-558 — called via nativeKill.call:
const pty_baton* handle = get_pty_baton(id); // nullptr — baton already gone
if (handle != nullptr) { // always false after onExit
pfnClosePseudoConsole(handle->hpc); // never reached
}Every call site in this PR fires after onExit: disposeForegroundPtyResources (in finalize()'s finally), the promote-settle exit arm, and WebTerminalRegistry.release() under session.exited. All of them invoke releaseConPtyHost after the baton is already gone. PtyKill returns undefined without throwing, so debugLogger.warn never fires and the no-op is silent.
The _conoutSocketWorker.dispose() half of releaseConPtyHost has no baton dependency and correctly fixes the thread count. The worker-thread half of the fix is sound.
A fix needs to call ClosePseudoConsole while the baton still exists — before the exit-watcher thread's remove_pty_baton. That requires either storing the HPCON handle before the baton is erased, or an upstream hook in @lydell/node-pty that fires before baton removal. The author confirmed this independently and noted it has been escalated to the maintainer. Neither path is absorbable into this PR without touching node-pty.
Cross-check
qwen-code-ci-bot's Critical at conpty-host.ts:108 and the carried-forward Critical at shellExecutionService.ts:1864 name the same mechanism — confirmed. qwen-code-dev-bot's gate review at the current head remains open for the same reason.
Fixes confirmed at current head (b755bba):
- Unconditional settle listener — A1-2 fixed
_isReadyguard onnoteConPtyHostReleasedin cancel path — correct, prevents false suppression of the finalizer on pre-ready cancelssession.pty.releaseHost?.()in the exited web-terminal branch — correct
qwen-code-ci-bot's suggestions at shellExecutionService.ts:2130, 2364, and web-terminal-registry.ts:430 are addressed at the current head. The dispose throw-guard test (conpty-host.ts:115 concern) is covered by the new "does not fail the result when ClosePseudoConsole throws" and "still drops the pid from activePtys when the conout worker dispose throws" test cases.
Scope: Source only. Not exercised: platform-specific native behaviour (no Windows host); CI Windows test jobs gated to merge_group/schedule/workflow_dispatch and never run on PRs, so every win32-gated code path in this diff is untested by CI on real ConPTY behaviour.
Reviewed with AI assistance.
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
两阶段审查 第一轮（deepseek-v4-flash）结果
本轮因本地环境 git fetch 解析失败无法创建工作树，模型基于 gh api 获取的 diff 与已有 PR 上下文做了静态分析。
结论：发现 / 确认问题（无新增独立发现，但既有问题仍未解决）
- Blocker / 关键缺陷（A1-1 / R1-1）：
releaseConPtyHost的nativeKill.call在onExit后是 no-op，node-pty 的 native exit-watcher 在触发 JSexit回调前已移除 pty baton，ClosePseudoConsole实际未被调用。作者已独立确认，等待维护者决策。 - Minor（A1-2）：已修复 — settle listener 现在无条件注册。
- Suggestion（R1-8）：未在本 PR 处理 — Web-terminal
handleExit仍未调用releaseHost()。
其它观察
_conoutSocketWorker.dispose()这部分修复是正确的，能解决线程数持续增长的问题。releasedHosts/noteConPtyHostReleased/_isReady等防护基础设施为后续真正关闭 HPCON 的修复做了正确准备。
由于第一轮已确认问题，未启动第二轮 qwen3.8-max。
Review found that `releaseConPtyHost` cannot close the pseudo-console on any of its three call sites, and the finding was confirmed independently against the exact pin. At `@lydell/node-pty` 1.2.0-beta.10, the native exit-watcher thread in `src/win/conpty.cc` erases the pty baton *before* it delivers the JS `onExit`, and `PtyKill` skips `ClosePseudoConsole` when `get_pty_baton` returns null — silently, so not even the warn fires. `struct pty_baton` has no destructor, so the erase leaks the HPCON rather than closing it. Every call site here runs strictly after `onExit`. So the split is: - `_conoutSocketWorker.dispose()` is pure JS and genuinely frees the worker thread. That is the 353-threads half of #11303, and it is real. - `_ptyNative.kill()` is a no-op. The `conhost.exe` half — the dominant term the reporter measured — is NOT fixed. The unit tests cannot see the difference, because `_ptyNative.kill` is a `vi.fn()`: the assertion passes on a call that does nothing on Windows. Comments and names only, no behaviour change. Corrected here: the doc block, both call-site comments, the `releasedHosts` rationale (which after this finding applies only to the cancel path, where the shell is still alive and the baton still exists), the version clearance (the JS field names were re-checked on beta.15, the native semantics were not and did change at beta.14), the warn text, the web-terminal comments, and the test names and describe block that read as certifying the host. The call itself stays: the call site is right, and it starts working unchanged the moment upstream closes the HPCON when the baton is erased. Until then the only mitigation for the host half is `tools.shell.enableInteractiveShell: false`.
yiliang114
changed the title
|
⏸️ Deferring to @wenshao — not a defect, a missing measurement at this head. Two production commits ( Narrow ask, not a re-review: either re-point the harness at head Everything else is settled from my side — CI is green with zero failures at this head, the base-drift lint blocker is genuinely resolved, and the design and the honest half-a-fix scoping are right. The one code-review finding (three doc sites claiming a queued Needs a human call on this one. 中文说明⏸️ 转交 @wenshao —— 不是缺陷，而是当前 head 上缺一次测量。 两个生产提交（ 请求很窄，不是重新审查：要么把 harness 指向 head 我这边其他一切均已定论 —— 当前 head 上 CI 全绿、零失败，base-drift 的 lint 阻塞确已解决，设计以及「只修一半」的诚实范围划分都是对的。唯一的代码审查发现（三处文档宣称排队的 这一项需要人来定夺。 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
|
Triage re-run completed without a new review. The bot already has a review of its own on 机器人在 The stage comments above were updated with the latest result. View workflow run. 上方各阶段评论已更新为最新结果。查看工作流运行。 |
|
@qwen-code /verify |
|
Sandboxed verification: ❌ not passed — findings reported (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 116 passed · 0 failed · 116 total Flakiness gate: ✅ 2 changed test file(s) x 5 identical rounds, no divergence 中文 — 判定：❌ 不通过 · 报告了发现（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：116 通过 · 0 失败 · 116 总计 抖动门：✅ 2 changed test file(s) x 5 identical rounds, no divergence Verification reportVerdict: Verified head: The head and base OIDs are byte-identical to round 2's — same Targeted gate: 175/175 tests pass in the two changed test files (round 2: 175 — unchanged, as expected for an unchanged head). 中文 — 判定：
|
| # | Finding (round 2) | Severity | Status at 3718031f |
Evidence this round |
|---|---|---|---|---|
| 1 | disposeConoutWorker forecloses the deferred teardown its own justification defers to; the comment states the opposite |
Suggestion | stands — reproduces cell for cell | Re-measured with a rebuilt harness lifting node-pty's handler blocks verbatim out of the sha512-verified tarball: control drains the queued kill (1/1), head inside the 1 s window drains (1/1), head after the window never drains (0/1, 1 still queued), head with no byte ever never drains (0/1). 41/41 assertions. See 02-deferred-window-control-drains-head-after-1s-never.png |
| 2 | noteConPtyHostReleased in windowsStrategy.killPty (shellExecutionService.ts:623) is inert |
Nice to have | stands | Line 623 still present; M2 re-run: SURVIVED (175/175 green), with the same-file positive control M4 KILLED (1 failed | 174 passed) proving the runner does collect tests that exercise that file. Its unreachability is now census-proven, not read: ShellExecutionService.cleanup() has exactly one production caller, the process.on('exit') handler at line 692 |
| 3 | Both arms of WebTerminalRegistry.release() end in the identical session.pty.releaseHost?.() |
Nice to have | stands — declined-with-rationale, and I agree | Lines 474/482 still duplicate the call, each carrying its own per-arm rationale comment. Hoisting would lose the per-arm reason even though the call is one. Defensible under Simplicity-First |
| 4 | Web-terminal path attaches no socket error listener, leaving node-pty's uncaught-throw path live |
Note (bounded) | stands, still did not fire | All four cells: socket 'error' events = 0, uncaughtException = null, and Terminal._close does not clear _deferreds (newly asserted this round) — so the queued teardown survives the socket close and stays stranded rather than being dropped |
| — | Round 2's upstream table only 10 of 14 rows re-run | gap | closed | All rows re-run as 49 scripted assertions against the correct artifacts; see Corrections and the upstream table below |
| — | Round 2 did not drive teardown paths 1–2 end-to-end through ShellExecutionService |
gap | closed | Path 1 driven end-to-end through the real compiled service in all three A/B arms; see the A/B table |
| — | Round 2 did not census the new unconditional-settle-listener comment's reachability claim | gap | closed — the claim holds | See Corrections row 2 |
Scope selection
Central claim: releaseConPtyHost frees node-pty's conout worker thread at the teardown paths that previously released nothing.
Secondary claim 1 (negative): the _ptyNative.kill() half is a silent no-op on the natural-exit paths, so conhost.exe is not fixed here.
Secondary claim 2 (new this round, from the delta's longest comment): attaching the settle listener unconditionally is safe because no production execute() call site can reach a background promote without also passing postPromote.
Budget went to (1) rebuilding the A/B as an end-to-end drive through the real service with two independent controls, (2) re-measuring Finding 1 with verbatim-lifted node-pty handlers, (3) completing the upstream verification at the binary, JS and C++ level, (4) the reachability census, (5) a 4-mutant subset with a same-file positive control, (6) the gate, (7) three captures. Out of scope: real Windows, the full 9-mutant matrix, per-commit attribution, repo-wide gates.
A/B cell table — central claim, driven end-to-end through the real service
Round 2 called releaseConPtyHost directly. This round drives ShellExecutionService.execute() from the compiled dist/, spawns a real PTY per round, lets it exit naturally, and attaches a real node-pty ConoutConnection (real worker_threads Worker, real Unix-socket conout pipe) to the live ptyProcess the service is holding. The release then happens where the PR put it — inside the service's own finalizer — not where a harness chose to call it. Oracle: OS thread count from /proc/self/task. Witness: 01-ab-e2e-three-arms-head-plus0-both-controls-plus8.png; raw log logs/ab-e2e.log.
| Cell | Environment | Release executed by | Oracle (threads, 8 rounds) | Result |
|---|---|---|---|---|
| head | compiled packages/core/dist at 3718031f |
the service's own finalizer, via releaseConPtyHost |
7 → 7 | +0 — 8/8 freed, 8/8 _isDisposed, 8 native-kill calls (42xx,false) |
| control A | hardlink copy of the same dist with the two releaseConPtyHost(ptyProcess); statements reverted — verified diff against head shows exactly those two lines and nothing else |
nothing | 7 → 15 | +8 — 8/8 stranded, 0 disposed, 0 native kills |
| control B | head dist unchanged; each ptyProcess pre-noted through the PR's own exported noteConPtyHostReleased, so releaseConPtyHost takes its releasedHosts early return |
nothing (early return) | 7 → 15 | +8 — identical to control A |
8/8 flip from stranded to freed, confirmed by two independent controls that agree exactly. Control B is the stronger hygiene of the two: it needs no file surgery, and it additionally proves the releasedHosts early return really does suppress the release — the guard is load-bearing, not decorative.
Head-arm assertions also passing: every round attached _agent to a live ptyProcess (8/8, so the release saw a valid agent rather than warning); every command exited 0 through a real PTY round-trip; conpty-host consulted the win32 gate 8 times (the gate was not short-circuited); _ptyNative.kill fired exactly once per PTY with (pty, false); ptyProcess.kill() — the #6067 recycled-pid route — was never invoked.
Base census (in-harness, 2 assertions): conpty-host.ts does not exist at HEAD^1, and the two changed files contain zero occurrences of releaseConPtyHost / noteConPtyHostReleased / disposeConoutWorker / releaseHost there (13/5/3/5 at head).
Control hygiene: package.json and package-lock.json are untouched by the PR, so reusing the root node_modules is a clean code-only control. No harness crosses a @qwen-code/* workspace boundary — conpty-host.js imports only node:os and the sibling debugLogger — so the symlink-into-head-tree hazard does not apply here. Platform is forced to win32 for conpty-host only, by a stack-scoped os.platform() override, so the service still spawns a real Linux PTY and the windowsKillPid taskkill reap stays dormant. That is deliberate: it isolates the release, and it matches the scenario the PR itself names as the leak — isPtyActive false, clean exit, no taskkill.
Delta harness — Finding 1 re-measured
harness/deferred-window.mjs. Nothing about node-pty's semantics is retyped. Three blocks are lifted verbatim out of the shipped tarball whose sha512 equals the package-lock.json integrity hash, and eval'd against a shim: windowsPtyAgent.js:47-57 (the outSocket + ConoutConnection wiring and the 'connect' → emit('ready_datapipe') hop), windowsTerminal.js:59-101 (the once('data') that flips _isReady and drains _deferreds, plus the error and close handlers), and windowsTerminal.js:147-168 (kill() + _deferNoArgs). The conout worker is node-pty's real worker/conoutSocketWorker.js; the byte path is real end to end (harness → pipe → worker → worker-server → outSocket). The function under test is the compiled head disposeConoutWorker. One cell per process. 41/41 assertions; raw log logs/deferred-window.log.
| Cell | disposeConoutWorker? |
shell's 1st byte | queued kill drained? | native close? | still queued | worker |
|---|---|---|---|---|---|---|
| 1 control — base behaviour | no | t = 2.0 s | yes (1) | yes (1) | 0 | freed |
2 head, inside the 1 s FLUSH_DATA_INTERVAL |
yes | t = 0.4 s | yes (1) | yes (1) | 0 | freed |
| 3 head, after the window | yes | t = 2.0 s | no (0) | no (0) | 1 | freed |
| 4 head, byte never arrives (slow pwsh startup) | yes | never | no (0) | no (0) | 1 | freed |
Cell 1 ran first, as a validity control: with nothing disposed, a late byte still flips _isReady and drains the queued teardown, so the harness provably can make the deferred close fire. Cells 3/4 are then a real absence, not a dead probe. New this round: Terminal._close is asserted not to clear _deferreds, which is what makes cell 3's still queued = 1 permanent rather than transient — the socket close that follows worker termination runs _close() (observed: 1 invocation) and the queued teardown survives it, never to run.
Also re-measured as not holding, exactly as round 2 reported: in all four cells the worker termination produced a socket close (1 event) and never an error (0 events), and uncaughtException stayed null — node-pty's throw err path did not fire.
Corrections to the PR description
Two claims in the description are inaccurate about provenance and reachability, not about behaviour. Both are labelled as corrections to the text; neither asks for a code change.
1. The conpty.cc citation names an artifact that does not contain it — and the cited lines are nonetheless all correct.
The body's code block is headed "src/win/conpty.cc @ node-pty 1.2.0-beta.10 (the npm tarball @lydell/node-pty builds from)". Measured:
@lydell/node-pty-win32-x64@1.2.0-beta.10ships 21 files and no.ccat all — onlylib/*.js,prebuilds/win32-x64/*, LICENSE, README, package.json.- The installed
@lydell/node-ptytarball ships 5 entries (LICENSE, README.md, index.js, node-pty.d.ts, package.json) and nosrc/. - The
lydell/node-ptygit repo at tagv1.2.0-beta.10has nosrc/directory — its root is.gitignore, LICENSE, README.md, build.js, index.js, package-lock.json, package.json, publish.js. It is a packaging repo. - The real source is the dependency that repo declares:
"dependencies": { "node-pty": "1.2.0-beta.10" }— microsoft's package, whose tarball does shipsrc/win/conpty.cc(582 lines, sha256d502cce570552c7a…).
Against that correct artifact, every one of the body's 12 line citations is exact (49 scripted assertions, logs/upstream-census.log): :41 struct pty_baton {, :49 ctor only, :50 }; with zero ~pty_baton anywhere in the file, :101 WaitForSingleObject(baton->hShell, INFINITE), :105 CloseHandle, :106 assert(remove_pty_baton(baton->id));, :108 tsfn.BlockingCall(exit_event, callback) — erase at 106 strictly before the JS delivery at 108 — :546 get_pty_baton(id), :548 if (handle != nullptr) {, :558 pfnClosePseudoConsole(handle->hpc), :566 return env.Undefined(); with no throw and no log anywhere in 546–566, and :573-578 exporting exactly five functions with kill → PtyKill as the only JS route to ClosePseudoConsole (1 call site in the whole file). conpty_console_list.cc:21-22 does throw Napi::Error::New(env, "AttachConsole failed") rather than returning an empty list, and _getConsoleProcessList does fall back to resolve([_this._innerPid]) after 5000 ms — the #6067 premise is accurate. So the substance stands entirely; only the pointer to it is wrong. Worth fixing because a maintainer who follows the citation as written finds nothing.
Corroborated at the binary level, independently of any source: strings -el on the shipped conpty.node yields D:\a\_work\1\s\src\win\conpty.cc, remove_pty_baton(baton->id) and the MSVC _wassert format Assertion failed: %Ts, file %Ts, line %d. That triple is precisely what assert(remove_pty_baton(baton->id)) compiles into, so the erase-before-onExit is in the artifact Windows actually loads — not merely in a source tree someone read.
2. The conpty-host.ts doc block's version claims are all true — now verified against artifacts, not quoted.
The block says the JS field names "were re-checked on 1.2.0-beta.15 and are unchanged", and that "from 1.2.0-beta.14 upstream erases the pty baton with an unconditional std::erase_if rather than under assert". Both confirmed: beta.15's windowsPtyAgent.js still assigns _useConptyDll, _ptyNative, _pty = term.pty and _conoutSocketWorker, and FLUSH_DATA_INTERVAL is still 1000 with the _isDisposed guard intact; while both beta.14's and beta.15's conpty.node contain zero remove_pty_baton strings and zero _wassert format strings, against beta.10's one of each. The assert genuinely disappears at exactly the version the doc names, so its operational warning — "a bump has to be re-checked against src/win/conpty.cc, not only against the JS shape" — is validated by the artifacts themselves.
3. The reachability claim behind the unconditional settle listener holds — census-proven, not read.
The new comment argues that routing a no-postPromote promote through firePostSettle (and therefore through an unrequested windowsKillPid(pid, false) taskkill) is safe because no shipped call site can reach it. Proved by census:
- Exactly one production producer of a reason-carrying
.abort({:AppContainer.tsx:4848,executingShell.promoteAbortController.abort({ kind: 'background' }). - That producer is gated on
tc.request.name === ToolNames.SHELL && tc.promoteAbortController !== undefined. promoteAbortControlleris constructed at exactly one place:tools/shell.ts:2266, the foreground path.- That path's
execute()call passes{ postPromote }(tools/shell.ts:2623).
So a no-postPromote background promote is unreachable in production, and the surprise taskkill the comment concedes cannot fire. The other execute() call sites that omit postPromote (tools/shell.ts:3862 and :4532) both pass shouldUseNodePty = false, so they never enter the PTY promote path at all. This is the claim the comment asks the reader to take on trust; it survives being checked.
Findings
1. Suggestion (carried from round 2, stands) — disposeConoutWorker forecloses the deferred teardown its own justification defers to, and the comment states the opposite
web-terminal-registry.ts:312-318 and conpty-host.ts:84-86:
"That queued teardown runs the native ClosePseudoConsole when it fires … Dispose only the conout worker now … and leave the native close to the queued kill()." / "The worker dispose is idempotent … so doing it here and again in the queued teardown is safe."
Both assume the queued teardown eventually runs. Re-measured against the shipped node-pty, it cannot, past one second: _isReady flips only inside the conout socket's first data callback (windowsTerminal.js:61-66), that socket is fed exclusively by the conout worker, ConoutConnection.dispose() terminates that worker after FLUSH_DATA_INTERVAL = 1000 ms (windowsConoutConnection.js:52, :110), and Terminal._close does not clear _deferreds — so the queued entry outlives the only channel that could ever run it.
Reproduce:
node tmp/pr11313-verify-20260909-194739/harness/deferred-window.mjs --cell=control-no-dispose-late-byte
node tmp/pr11313-verify-20260909-194739/harness/deferred-window.mjs --cell=head-dispose-byte-after-window
# cell 1: deferredKillRan=1 nativeClose=1 stillQueued=0 (base: the queued close happens)
# cell 3: deferredKillRan=0 nativeClose=0 stillQueued=1 (head: never)So beyond the window there is no queued close left to defer to — and correspondingly no double-close for the branch to avoid. The branch is still the right call for the worker: cells 3 and 4 both free a thread that base strands, which is the whole point of the PR. Only its stated rationale is wrong past 1 s.
What does NOT hold (re-measured, and why this stays a Suggestion rather than a blocker): this is not a host-leak regression on the common path. On the live arm killPtyTree runs taskkill /f /t /pid first (web-terminal-registry.ts:86-92); once the shell dies, node-pty's native exit watcher erases the baton (conpty.cc:106), so any later PtyKill no-ops at :548 — the deferred close was already a no-op there, and base and head end identically. The regression window is narrow and conjunctive: taskkill fails to kill the tree (its exit status is never checked — spawnSync's result is discarded) and the shell's first output byte arrives more than 1 s after release(). Only then would base eventually have closed the HPCON and head never does. Cell 1 vs cell 3 is the A/B of that mechanism; the taskkill-failure precondition itself needs a Windows host and is not reproduced here.
Suggested fix (doc-accurate variant; not measured on Windows)
Keep the branch — it is correct for the worker — and correct the rationale to the measured truth: the queued kill can only still fire inside ConoutConnection's 1 s drain window, after which terminating the worker forecloses it, so on this arm the pseudo-console close is deliberately abandoned, matching the natural-exit arms where the baton is already gone. Round 2 noted, and M6 re-confirms, that the suite pins the branch's behaviour either way (3 tests go red when it is removed), so no test change is needed for a doc-only fix. If a maintainer would rather preserve the narrow taskkill-failed case, disposing the worker only after the drain window cannot matter reintroduces the strand this commit fixed, so the doc correction is the smaller change.
2. Nice to have (carried, stands) — noteConPtyHostReleased at shellExecutionService.ts:623 remains inert
Re-measured: M2 SURVIVED at 175/175, with the same-file positive control M4 KILLED (1 failed | 174 passed, AssertionError: expected "spy" to be called with arguments: [ 777, false ]), so the survival is a property of the suite and not of my runner failing to collect the file. Classification unchanged: dead code, not a coverage gap. The write targets a WeakSet whose only reader is releaseConPtyHost, and its caller ShellExecutionService.cleanup() has exactly one production caller — the process.on('exit') handler at line 692 (census-proven this round) — after which no release can run. The note is also unguarded, unlike its two siblings. No leak and no regression follow. Fix remains deletion, not a test.
3. Nice to have (carried, stands) — duplicated releaseHost?.() in both arms of release()
Lines 474/482 still end in the identical call, each with its own per-arm rationale comment. Round 1 suggested hoisting; the author declined by documenting instead, which is coherent. I agree it is defensible as-is; recorded for completeness.
4. Note (bounded, did not fire) — the web-terminal path attaches no socket error listener
windowsTerminal.js:92-93 throws when listeners('error').length < 2. This PR newly terminates the worker on a path that previously never did, widening the surface for a socket error. Re-measured: all four cells produced close (1) and never error (0), with uncaughtException staying null. Pre-existing exposure, not introduced here, and not observed — reported as bounded, not as a defect.
Mutation matrix (subset re-run: 4 mutants + control)
Control green first (175/175), so every kill means something. Witnesses 03-mutation-m2-survives-m4-same-file-control-kills.png; raw logs/m2-rerun.log, logs/mutation-subset.log. Every mutant restored; git status --porcelain empty after each and at the end.
| # | Mutation | Expected | Result | Evidence |
|---|---|---|---|---|
| — | none (validity control) | green | 175 passed | suite is live |
| M1 | remove agent._conoutSocketWorker?.dispose?.() from releaseConPtyHost (conpty-host.ts:210) — the central fix |
killed | KILLED | 6 named red tests, incl. releases the conout worker on a clean win32 completion (the leaking path), still disposes the worker when only the native-kill shape drifts, releases an exited session's conout worker, never by signalling the pid |
| M2 | drop noteConPtyHostReleased from windowsStrategy.killPty (shellExecutionService.ts:623) |
survives | SURVIVED | 175/175 green, ANSI-stripped detector |
| M4 | drop the _isReady !== false gate on the cancel-path note (shellExecutionService.ts:2418) — same-file positive control for M2 |
killed | KILLED | 1 failed | 174 passed, still releases after a cancel that landed before the terminal was ready |
| M6 | drop releaseHost's _isReady === false branch (web-terminal-registry.ts:327) |
killed | KILLED | 3 named red tests, AssertionError: expected "spy" to not be called at all, but actually been called 1 times |
M1's kill is the non-vacuity proof for the PR's central test: removing the one statement the fix exists to add turns six tests red with expected-versus-actual assertions, not broken imports.
Harness fault of mine, disclosed because it produced a wrong intermediate verdict. The first matrix run parsed vitest's summary line without stripping ANSI colour, so the summary was empty and the detector defaulted every mutant to KILLED — including M2, which showed no red test at all. I fixed the detector (cat -v + strip) and re-ran M2 with its control. M1's and M6's verdicts are unaffected: they rest on named red tests and assertion messages captured independently of the broken summary parser, not on the count.
Not covered
- No Windows execution. Every win32 behaviour was reached by forcing
os.platform()forconpty-hostonly and driving node-pty's own unmodified JS and worker as the peer. This reproduces the mechanism — real threads, real sockets, real_isReady/_deferredssemantics, realFLUSH_DATA_INTERVALtiming — not the cause on a live ConPTY: noconhost.exe, no HPCON, notaskkillwas exercised. The taskkill-failure precondition of Finding 1 is therefore unmeasured, as it was in rounds 1 and 2. - The
conhost.exe/HPCON half is physically unmeasurable here. Everything about it is source- and binary-level verification of the shipped artifacts — which this round completed (49 assertions across the beta.10 binary, the beta.14/15 binaries, the beta.10/15 JS, and the realconpty.cc), rather than carrying rows forward as round 2 had to. - Teardown path 2 (
firePostSettle) and path 3 (WebTerminalRegistry.release) were not driven end-to-end. Path 1 was, in all three A/B arms. Paths 2 and 3 rest on the PR's unit tests (non-vacuity proven by M1/M6) plus the mechanism harness; the release function they call is the same one path 1 exercises physically. - Only 4 of round 2's 9 mutants re-run (M1, M2, M4, M6). M3, M5, M7, M8, M9 were not re-executed; since the head is byte-identical to round 2 their results cannot have changed, but they are carried as round-2 evidence, not re-measured — notably M9's
redundant defenceclassification. - Repo-wide gates not run. Only the two changed test files (175). No full
packages/coresuite, nonpm run typecheck, nonpm run lintat head; the workflow-builtdist/is the compile evidence. - Per-commit attribution out of reach. The snapshot lists 17 commits; the depth-2 merge-ref checkout exposes 1 (
git rev-list HEAD^1..HEAD^2→3718031f;--is-shallow-repository= true). AggregateHEAD^1..HEADdiff verified only. - Snapshot
baseRefOid(1919ff97…) absent locally and differs fromHEAD^1(655baed6…); per the CI contract I usedHEAD^1. No trial merge into currentmain(nomainat depth 2). - The
strings -el/ conpty.cc checks needed network (npm pack, GitHub contents API), which the CI contract describes as unreliable forgh. It worked here; the tarball's sha512 was matched againstpackage-lock.jsonbefore any of it was trusted, so a tampered fetch would have failed assertion A1 rather than poisoning the results.
Methodology
Environment: the workflow's node:22-bookworm container (Node v22.23.2) at refs/pull/11313/merge, depth 2, with npm ci + npm run build pre-completed at HEAD. Five harnesses under harness/: ab-e2e.mjs (drives the real compiled ShellExecutionService over 8 real PTY round-trips per arm, with real ConoutConnection worker threads attached to the live ptyProcess, and a stack-scoped win32 override so only conpty-host sees Windows), deferred-window.mjs (verbatim-lifted node-pty blocks A/B/C over a real worker and real Unix sockets, one cell per process), upstream-census.mjs (49 assertions over the fetched tarballs, the shipped PE binaries, the real conpty.cc, and the repo-wide reachability census), mutation-subset.sh and m2-rerun.sh (single-point sed edits + vitest + git checkout -- restore, tree verified clean after each). The control dist is a hardlink copy of the head dist with the two release statements reverted; diff against head shows exactly those two lines. Raw logs in logs/; images via node scripts/verify-capture.mjs. Fetched node-pty artifacts were kept outside the repo in /tmp/ptyfetch and verified against the lockfile integrity hash before use.
Flakiness gate log
rounds=5 files=2 skipped=0
file packages/core/src/services/shellExecutionService.test.ts: (cd packages/core) npx --no-install vitest run ./src/services/shellExecutionService.test.ts
file packages/core/src/services/web-terminal-registry.test.ts: (cd packages/core) npx --no-install vitest run ./src/services/web-terminal-registry.test.ts
per-file results (P=pass F=fail I=infra-exit, one letter per run):
packages/core/src/services/shellExecutionService.test.ts: PPPPP
packages/core/src/services/web-terminal-registry.test.ts: PPPPP
verdict: pass
summary: 2 changed test file(s) x 5 identical rounds, no divergence
--- per-invocation detail (full copy in the artifact) ---
round 1 · packages/core/src/services/shellExecutionService.test.ts: P (exit 0)
round 1 · packages/core/src/services/web-terminal-registry.test.ts: P (exit 0)
round 2 · packages/core/src/services/shellExecutionService.test.ts: P (exit 0)
round 2 · packages/core/src/services/web-terminal-registry.test.ts: P (exit 0)
round 3 · packages/core/src/services/shellExecutionService.test.ts: P (exit 0)
round 3 · packages/core/src/services/web-terminal-registry.test.ts: P (exit 0)
round 4 · packages/core/src/services/shellExecutionService.test.ts: P (exit 0)
round 4 · packages/core/src/services/web-terminal-registry.test.ts: P (exit 0)
round 5 · packages/core/src/services/shellExecutionService.test.ts: P (exit 0)
round 5 · packages/core/src/services/web-terminal-registry.test.ts: P (exit 0)
Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention).
— Qwen Code · sandboxed verification
Merge origin/main into the existing PR branch and retain both Windows ConPTY regression suites. Keep worker cleanup active after bundled ConPTY has already closed its host. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
Merged current The merge exposed one real interaction with #11497: with bundled ConPTY, Verification at
The PR is now mergeable with zero active unresolved review threads. CI for the new head is still running, and the PR body has been refreshed to distinguish #11497's shell-host fix from this PR's worker cleanup and the remaining inbox web-terminal scope. @wenshao, when the current CI snapshot settles, could you please re-review the merged head? |
qqqys
left a comment
qqqys
left a comment
There was a problem hiding this comment.
COMMENT — both carried-forward Criticals are fixed at this head; two gates remain unconfirmed, so I am escalating rather than approving.
Reviewed at head f7fedc20 (a merge of current main plus the newest production commit). Full production delta read: conpty-host.ts (+211, new), shellExecutionService.ts (+124/-42), web-terminal-registry.ts (+68/-1).
Historical blocking issues — both confirmed fixed
R5-5 (certifies-falsely) — fixed. The blocking half of that finding was the false certification, not the native limitation, which the reviewer itself recorded as upstream (microsoft/node-pty#965, tracked as #11352) under a maintainer decision to "land the worker half and stop certifying the host half". At this head the certification is retired: conpty-host.ts opens the contract block with "What this function itself reliably frees is the conout worker thread", states that _ptyNative.kill() reaches a live pseudo-console from exactly one call site and is "a silent no-op" everywhere else, spells out the remove_pty_baton-before-onExit mechanism and the missing ~pty_baton, concludes "The inbox conhost half of #11303 is therefore not fixed by this function on the natural-exit path", and forbids the misleading test explicitly. The title and body now say "conout worker" and "Partially addresses", with Refs #11303 rather than Fixes.
The specific contradiction the finding named is also resolved at the authoritative site. web-terminal-registry.ts:311-328 now carries a correct two-arm narrative — LIVE: killPtyTree queued a kill() in _deferreds, so releaseHost disposes only the worker and leaves the native close to it; EXITED: "no kill() ran and nothing is queued, because release() only calls killPtyTree on the live arm", the baton is already erased so a native close would no-op. conpty-host.ts:144-147 still ends with "so its queued kill() stays the single closer" while covering both arms in one sentence, which reads imprecisely against that split — but it is a comment, the load-bearing statement at the call site is now correct, and comments are outside this gate's reporting bar.
R4-1 (template) — fixed. The live PR body at this head carries all nine required headings from .github/pull_request_template.md: ## What this PR does, ## Why it's needed, ## Reviewer Test Plan, ### How to verify, ### Evidence (Before & After), ### Tested on, ## Risk & Scope, ## Linked Issues, and the 中文说明 block, with per-OS rows and an explicit N/A-plus-reason in the evidence row.
chiga0's two [Blocker] threads (3954648761, 3955716235) and the qwen-code-dev-bot gate review are the same root cause that R5-5 consolidates, and are addressed by the same correction.
What I verified positively in the current code
conpty-host.tscannot make things worse on an upstream shape change: every field read is optional-chained, both mutations sit intry/catchbehind adebugLogger.warn, everything iswin32-guarded, and the shape-changed arm degrades to the pre-existing leak rather than tokill()— the correct choice given #6067.- The newest commit's substance is sound.
performCancelKillnow notes the release only when_isReady !== false, so a cancel landing before the shell's first output byte no longer queues a teardown that never runs while permanently suppressing the finalizer'sreleaseConPtyHost; and thecatcharm correctly notes nothing, leaving the finalizer's release intact. releaseHost's two arms cannot double-close. Live-and-ready: the wrapperkill()recorded the note, soreleaseConPtyHosttakes thereleasedHostsearly return and disposes the worker only when_useConptyDll. Live-and-deferred: no note, anddisposeConoutWorkerleaves the native close to the queuedkill(). Exited: nokillPtyTreeran, so nothing is queued to conflict with.- Attaching the post-promote
'error'listener unconditionally is load-bearing independent of the leak fix: node-pty routeson('error')to the conout socket, whose handler throws below two listeners, and the foreground handler is gone at promote — without it a post-promote socket error escapes as anuncaughtException.
Gate 1 — the new unconditional settle listener adds a taskkill /f whose reachability I could not confirm
The post-promote onExit/'error' attachment was previously gated on if (postPromote) and is now unconditional. That routes a promote which passes no postPromote through firePostSettle, whose reap runs windowsKillPid(ptyProcess.pid, false) — taskkill /f /pid — whenever isPtyActive(pid) is still true. The diff's own comment concedes this is "a taskkill the caller did not explicitly ask for" and defends it purely on reachability: that performBackgroundPromote is entered only from a { kind: 'background' } abort whose sole producer is the shell tool's Ctrl+B handler in tools/shell.ts, which also passes postPromote.
I did not confirm that producer enumeration within this review's budget, and it is the whole of the safety argument: if any execute() caller can reach a background promote without postPromote, the settle reap force-kills a pid that a natural exit may already have recycled — the #6067 collateral-kill mode this PR is otherwise careful to avoid, and one it avoids everywhere else by never signalling the pid. The round-6 review recorded the same paragraph as concluding "the opposite of its own premise" (if the sole producer always passes postPromote, a no-postPromote promote is unreachable rather than reachable-only-from-Ctrl+B), so the prose does not currently settle the question either way.
What would close this: name every producer of the promote abort and every execute() call site that can reach performBackgroundPromote, and state which of them pass postPromote. If the enumeration holds, this gate closes with no code change; if it does not, the reap needs to stay gated while the 'error' listener and the worker release do not.
Gate 2 — nothing has verified this head, and the project's own gate is deferred
Every new code path is behind a win32 platform guard whose native half the unit tests stub (_ptyNative.kill is a vi.fn()), so no test run anywhere exercises real ConPTY behaviour. Test (windows-latest, Node 22.x) is gated to merge_group/schedule/workflow_dispatch and never runs on a pull request. The author records no Windows host, and the acceptance criterion is the reporter's four-step conhost.exe/thread-count recipe, which has not been run at any head.
The newest production commit landed at 2026-09-10T03:54Z, after the last bot review (2026-09-09T18:53Z, at 3718031f) and after the last sandboxed verification, which reported ❌ at that prior head. The project gate is currently ⏸️ deferred to @wenshao as "not a defect, a missing measurement at this head" — and that deferral names a head two commits older than this one. So the current head has had neither a review nor a verification.
Under AGENTS.md's core-infrastructure rule this diff lands in packages/core/src/services/**, where a small-scope change may be cleared only at 100% confidence with every downstream consumer named, and any doubt escalates. Gates 1 and 2 are that doubt. The code reading above is genuinely clean and the worker half is worth landing on its own; what is missing is the caller enumeration and one Windows measurement at this head, not a rework.
CI
Green at this head — Test (ubuntu-latest, Node 22.x), Lint & Static, Integration Tests (no-AK, No Sandbox), Desktop Shell (ubuntu-22.04), Desktop Shell (windows-2022) and web-shell E2E Smoke all pass, with only review-pr still in progress. No failure attributable to this diff, and CI is not the reason for this COMMENT: the Windows lane that would matter never runs on a PR.
chiga0
left a comment
chiga0
left a comment
There was a problem hiding this comment.
Round 3 — head f7fedc2
Prior findings
| Id | Severity | Round-2 status | Round-3 status |
|---|---|---|---|
| R2-1 (= A1-1) | Blocker | Confirmed — nativeKill is a no-op, ClosePseudoConsole never called |
Resolved — commit 133c52659d removes the false claim; code and docs now correctly document that nativeKill is a silent no-op post-onExit and the conhost.exe leak is explicitly out of scope |
| A1-2 | Minor | Fixed | Still fixed |
New commits since round 2 (8 commits, b755bba → f7fedc2)
133c52659d — docs(core): stop certifying a ConPTY host close that never happens
releaseConPtyHost's docstring now correctly describes nativeKill as reaching a live pseudo-console from only one call site (firePostSettle's 'error' entry) and being a silent no-op everywhere else. The PR description now says "Partially addresses #11303" and the conhost.exe leak is listed as explicitly out of scope. R2-1 is resolved.
9a527d9c1d — fix(core): avoid double-close on web-terminal deferred live release
web-terminal-registry.ts now branches releaseHost on _isReady === false: the deferred-kill path disposes only the worker via disposeConoutWorker, leaving the HPCON close to the queued kill(); the ready path calls releaseConPtyHost as before. The kill() wrapper also conditions noteConPtyHostReleased on _isReady !== false, matching the cancel-path guard in performCancelKill. Both the live-release and already-exited branches of release() call releaseHost?.().
Checked: _isReady === false and _isReady !== false branches are mutually exclusive and exhaustive. No double-close path exists: when _isReady === false, disposeConoutWorker is called (worker only); when _isReady !== false, releaseConPtyHost checks releasedHosts — if kill() ran and noted the release, worker dispose via disposeConoutWorker only; if kill() did not run (already-exited arm), native no-op then worker dispose.
f7fedc20d0 — fix(core): preserve bundled ConPTY worker cleanup after cancel
releaseConPtyHost now adds if (agent?._useConptyDll) disposeConoutWorker() inside the releasedHosts.has(key) early return. This preserves the worker cleanup for bundled ConPTY after a cancel path that recorded noteConPtyHostReleased: node-pty defers worker disposal through _deferreds rather than executing it in kill(), so without this branch the cancel path would leave the worker stranded. Inbox ConPTY's kill() disposes the worker directly, so the _useConptyDll gate is correct.
Verified: disposeConoutWorker.dispose() is documented as idempotent on the bundled backend ("resets the one-second drain timer"), so the deferred kill() and this explicit disposal do not conflict.
Coverage gap (unchanged from round 2)
Real Windows process counts remain unverified — no Windows host available. The author confirmed 181/181 unit tests pass and that a mutation removing the bundled-worker cleanup makes the cancellation assertion fail with zero worker disposals, confirming that assertion is load-bearing. Not an approval blocker.
No blocking findings.
Approval blockers: none.
Reviewed with AI assistance.
qwen-code-dev-bot
left a comment
qwen-code-dev-bot
left a comment
There was a problem hiding this comment.
Approved at head f7fedc20.
Required CI is green at this commit — Test (ubuntu-latest, Node 22.x), Lint & Static, Integration Tests (no-AK, No Sandbox), web-shell E2E Smoke and both Desktop Shell lanes completed successfully; only review-pr (the reviewer's own job) is still running.
This gate filed one of the six records of the same root cause on this PR, so I re-checked it against this head rather than assuming the decision was applied:
- The over-claim is retired, in the direction the maintainer decision chose. The title and both issue links now say worker, and
releaseConPtyHost's own contract block states plainly that "What this function itself reliably frees is the conout worker thread", that_ptyNative.kill()reaches a live pseudo-console from exactly one call site and is a silent no-op elsewhere, and why — the native exit-watcher erases the baton before JS seesonExit,PtyKillskipsClosePseudoConsoleon a null baton without throwing, andpty_batonhas no destructor. It also now says the opposite of what the earlier rounds certified: "The inbox conhost half of #11303 is therefore not fixed by this function on the natural-exit path", plus an explicit instruction not to write a test that treats a stubbed_ptyNative.killas evidence of a host release. The host half is tracked upstream and in #11352, which is where it belongs. - The self-contradiction between the two files is gone.
releaseHostnow documents both arms separately and correctly — LIVE has akill()queued in_deferreds, EXITED states "no kill() ran and nothing is queued, becauserelease()only callskillPtyTreeon the live arm" — andrelease()'s two arms carry the matching narratives, so the exited-and-never-ready terminal is no longer described as having a pending closer. - The template finding is resolved: the body now carries every required section, including an honest
Evidence (Before & After): N/Aand a per-OSTested onrow.
I also ran the two changed suites at this head instead of trusting the reported count: 181/181 pass. To confirm the newest commit is load-bearing rather than decorative, I disabled just the _useConptyDll worker fallback in the already-noted early return — exactly one test went red, does not close the pseudo-console twice and still disposes the bundled worker after cancel, and the other 180 stayed green. The rest of the lifecycle holds up on reading too: the un-noted arm always disposes the worker after the (possibly no-op) native call, shape drift degrades to the previous leak instead of signalling a pid, the note site is gated on _isReady !== false so a deferred kill() is never recorded as a completed close, firePostSettle single-fires and runs the reap plus the release before its onSettle early-return, and the unconditionally attached 'error' listener is covered by a case that emits a post-promote error and asserts it does not escape (the comment attributes the necessity to node-pty routing on('error') to the conout socket, whose own handler throws below two listeners — that half is the author's premise, not something the suite can reach).
No new Critical found. Three things remain open, none of them gates:
- R5-2 still stands as filed: when
_conoutSocketWorkeritself is the field that drifts away, the release is silent — that is the one drift that costs the only resource this module frees, and it is the one without adebugLogger.warn. - The reachability paragraph at
shellExecutionService.ts:2222-2232argues against its own premise (if the sole{ kind: 'background' }producer always passespostPromote, a no-postPromotepromote is not reachable today), and it describesisPtyActiveas if it consultedactivePtyswhen the static is aprocess.kill(pid, 0)existence probe — so the "surprise taskkill" it warns about turns on pid liveness, not on the map. - The Windows process-level acceptance check (parent thread and
conhost.exe --headlesscounts across N commands) has still not been run anywhere: CI gatesTest (windows-latest)to merge queue/schedule/dispatch, so it never runs on a pull request. That is the outstanding evidence for #11303's thread half, and it needs a maintainer machine or the next release run.
|
Closeout snapshot at I am not adding code for the remaining verification notes in this pass. The latest The producer census in that report also closes Gate 1 from the latest review: there is one production background-abort producer, its foreground shell call passes No local code change was needed in this pass. |
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
Self-review at head f7fedc20 (GitHub does not let me approve my own PR). Posting mainly because @qqqys's Gate 1 asked for a caller enumeration that only the author side was going to produce, and because re-reading the full delta turned up two things I would fix.
Gate 1 — the enumeration holds; a no-postPromote PTY promote is unreachable
Every link below was read at f7fedc20, not recalled:
- The changed settle code lives in the PTY
performBackgroundPromote(shellExecutionService.ts:2009). It has exactly one call site:case 'background'of theabortHandlerswitch at:2505. (The otherperformBackgroundPromoteat:1029, called at:1402, is thechild_processpath and does not contain this code.) - That case requires
getShellAbortReasonKind(abortSignal.reason) === 'background', i.e. a{ kind: 'background' }abort reason. - The only production code that aborts with that reason is
packages/cli/src/ui/AppContainer.tsx:4848-4850(Ctrl+B). It is double-gated at:4830-4842ontc.request.name === ToolNames.SHELL && tc.promoteAbortController !== undefined, with the tool-name gate added specifically so a future property collision cannot fire it on a non-shell tool. promoteAbortControlleris populated in exactly one place:coreToolScheduler.ts:5020-5026(setPromoteAbortControllerCallback), whose only caller istools/shell.ts:2646.shell.ts:2646runs on the same foregroundexecute()call that passes{ postPromote }atshell.ts:2623, andpostPromoteis built unconditionally atshell.ts:2557— there is no branch that reaches:2623without it.
So the two sets ("promotes that reach the unconditional settle listener" and "promotes that pass postPromote") are currently identical, and the taskkill /f in firePostSettle gains no new caller. Two further points on the reap itself, since it is the part that sounded scary: the reap block is context in this diff, not an addition — it is the pre-existing #5873 settle reap — and isPtyActive (:2591) is a real process.kill(pid, 0) liveness probe rather than activePtys membership, so it fires only against a pid that is genuinely alive at settle time.
What I am not claiming: nothing enforces that enumeration. It is a runtime coupling across packages/cli → coreToolScheduler → tools/shell.ts → this service, held together by prose in a comment. A future execute() caller that aborts { kind: 'background' } without postPromote would reach the reap with no type error and no failing test, and the consequence is the #6067 collateral-kill mode this PR is otherwise careful to avoid. Inline suggestion on that comment.
Both carried-forward Criticals are retired at this head
- R5-5 (
certifies-falsely) — the certification is gone, which was the blocking half.conpty-host.tsnow opens with "What this function itself reliably frees is the conout worker thread", names the single call site where_ptyNative.kill()reaches a live pseudo-console, spells out theremove_pty_baton-before-onExitmechanism and the missing~pty_baton, concludes "The inbox conhost half of #11303 is therefore not fixed by this function on the natural-exit path", and forbids the stubbed-kill test outright. The three-arm narrative atweb-terminal-registry.ts:311-328is now correct on the EXITED arm, which is the contradiction the finding named. Title and body say "conout worker" and "Partially addresses",RefsnotFixes. - R4-1 (template) — I re-read the live body against
.github/pull_request_template.md: all nine required headings are present, with per-OS rows and an N/A-plus-reason in the evidence row.
Two findings of my own, both non-blocking
Inline: the _useConptyDll gate on the membership branch of releaseConPtyHost is asymmetric with the unconditional dispose on the first-call branch, and the noteConPtyHostReleased in windowsStrategy.killPty is the one kill site of three that is neither inside the try nor _isReady-guarded.
One thing I checked and am not filing: a double-close window on the cancel-before-first-output-byte arm when the preceding taskkill fails (shell alive, so the baton is still present, so the finalizer's nativeKill genuinely closes it, and a later _deferreds flush would close it again). It self-closes — ClosePseudoConsole terminates the attached client, so no further conout data arrives to flip _isReady and flush the queue. Narrow enough that I would not hold the PR on it, but it is the reason the _isReady !== false guard reads as a trade rather than a pure win.
Gate 2 stands, and it is a measurement gap, not a code defect
Nothing has exercised real ConPTY at this head: every new path is win32-guarded with the native half stubbed (_ptyNative.kill is a vi.fn()), and Test (windows-latest, Node 22.x) is gated to merge_group/schedule/workflow_dispatch so it never runs on a pull_request. The acceptance criterion remains the reporter's four-step conhost.exe / thread-count recipe on a Windows host. That is a maintainer decision about whether the worker half lands on Linux-verified evidence, not something a re-read can close.
CI at f7fedc20: Test (ubuntu-latest, Node 22.x), Lint & Static, Integration Tests (no-AK, No Sandbox), Desktop Shell (both), web-shell E2E Smoke all pass; only review-pr is pending. I did not run the suite locally — this checkout has no node_modules, and the Windows lane that would matter is not runnable here either way.
中文说明
在 head f7fedc20 上做的一次自审（GitHub 不允许我批准自己的 PR）。发出来的主要原因：@qqqys 的 Gate 1 要的调用方枚举只有作者侧能给出，而且重读完整 delta 时发现两处我会改的地方。
Gate 1 —— 枚举成立；不带 postPromote 的 PTY promote 不可达
下面每一环都是在 f7fedc20 上读出来的，不是回忆：
- 本次改动的 settle 代码在 PTY 版
performBackgroundPromote（shellExecutionService.ts:2009），只有一个调用点：:2505的abortHandlerswitch 里的case 'background'。（:1029的另一个performBackgroundPromote、在:1402被调用，是child_process路径，不含这段代码。） - 该分支要求
getShellAbortReasonKind(abortSignal.reason) === 'background'，即 abort reason 必须是{ kind: 'background' }。 - 生产代码里唯一用该 reason abort 的地方是
packages/cli/src/ui/AppContainer.tsx:4848-4850（Ctrl+B），且在:4830-4842被双重门禁：tc.request.name === ToolNames.SHELL && tc.promoteAbortController !== undefined——工具名这道门禁正是为了防止将来属性撞名时误触发到非 shell 工具。 promoteAbortController只在一处被写入：coreToolScheduler.ts:5020-5026（setPromoteAbortControllerCallback），其唯一调用方是tools/shell.ts:2646。shell.ts:2646与shell.ts:2623传入{ postPromote }的是同一次前台execute()调用，而postPromote在shell.ts:2557无条件构造——不存在能到:2623却不带它的分支。
所以「会走到无条件 settle 监听器的 promote」与「传了 postPromote 的 promote」目前是同一个集合，firePostSettle 里的 taskkill /f 没有获得新调用方。关于这个 reap 还有两点，因为它听起来最吓人：该 reap 块在 diff 里是上下文而非新增——它是既有的 #5873 settle reap；而 isPtyActive（:2591）是真正的 process.kill(pid, 0) 存活探针，不是 activePtys 成员判断，因此它只对 settle 时确实存活的 pid 生效。
我不主张的部分： 这套枚举没有任何强制机制。它是 packages/cli → coreToolScheduler → tools/shell.ts → 本服务之间靠注释散文维系的运行时耦合。将来某个 execute() 调用方若不带 postPromote 就 abort { kind: 'background' }，会走到这个 reap，且没有类型错误、没有测试失败——后果正是本 PR 在别处小心回避的 #6067 误杀已回收 pid 模式。已就该注释留了内联建议。
两条历史 Critical 在此 head 均已退役
- R5-5（
certifies-falsely）——阻塞的那一半（虚假认证）已消失。conpty-host.ts现在以「本函数自身可靠释放的是 conout worker 线程」开头，点明_ptyNative.kill()唯一能触达活伪控制台的调用点，写清remove_pty_baton先于onExit的机制与缺失的~pty_baton，结论是「因此 inbox conhost 那一半的 #11303 不被本函数在自然退出路径上修复」，并明确禁止那种用 stub kill 冒充证据的测试。web-terminal-registry.ts:311-328的三臂叙述在 EXITED 臂上已经正确，而那正是该 finding 点出的自相矛盾处。标题与正文改为「conout worker」「Partially addresses」，用Refs而非Fixes。 - R4-1（模板）——我按
.github/pull_request_template.md重读了线上正文：九个必需小节全部在位，含各 OS 行与证据行的「N/A + 原因」。
我自己发现的两处，均非阻塞
见内联：releaseConPtyHost 成员分支上的 _useConptyDll 门禁与首次调用分支的无条件 dispose 不对称；以及 windowsStrategy.killPty 里的 noteConPtyHostReleased 是三个 kill 点中唯一既不在 try 内、也没有 _isReady 守卫的那一个。
有一处我查过但不作为 finding 提出：cancel 落在首个输出字节之前、且前面的 taskkill 失败时的重复关闭窗口（shell 仍活 → baton 仍在 → finalizer 的 nativeKill 确实会关闭它，之后 _deferreds 冲刷会再关一次）。它会自行闭合——ClosePseudoConsole 会终止挂接的客户端进程，因此不会再有 conout 数据到来去翻转 _isReady 并冲刷队列。窗口足够窄，我不会因此压住这个 PR，但这也正是 _isReady !== false 那道守卫读起来像取舍而非纯收益的原因。
Gate 2 依然成立，而它是测量缺口，不是代码缺陷
此 head 上没有任何东西真正跑过 ConPTY：所有新路径都在 win32 守卫之后、原生一半被 stub（_ptyNative.kill 是 vi.fn()），而 Test (windows-latest, Node 22.x) 被门禁限定为 merge_group/schedule/workflow_dispatch，在 pull_request 上从不运行。验收标准仍然是报告者的四步 conhost.exe / 线程计数配方，需要一台 Windows 主机。这是维护者关于「worker 那一半是否可以凭 Linux 证据落地」的决定，不是再读一遍代码能关闭的。
f7fedc20 的 CI：Test (ubuntu-latest, Node 22.x)、Lint & Static、Integration Tests (no-AK, No Sandbox)、Desktop Shell（两个）、web-shell E2E Smoke 全部通过，只有 review-pr 待完成。我没有在本地跑测试——这个 checkout 没有 node_modules，而真正关键的 Windows lane 在这里本来也跑不了。
| const agent = (ptyProcess as { _agent?: WindowsPtyAgentInternals } | null) | ||
| ?._agent; | ||
| if (releasedHosts.has(key)) { | ||
| if (agent?._useConptyDll) { |
There was a problem hiding this comment.
[Suggestion] One thing I noticed re-reading this: the membership branch gates the worker dispose on _useConptyDll, while the first-call branch at line 205 disposes unconditionally. That asymmetry is load-bearing on a node-pty internal this file itself says was not verified.
The path that hits it is the web-terminal live release of a ready terminal — the interactive tab close: release() → killPtyTree → the wrapper kill() records the note (web-terminal-registry.ts:296-305, _isReady !== false) → releaseHost() → releaseConPtyHost (web-terminal-registry.ts:331) takes this branch. Web-terminal PTYs are the inbox backend, so _useConptyDll is false, line 176 never runs, and freeing the worker on that arm is delegated entirely to node-pty's own kill() having disposed _conoutSocketWorker synchronously. The releasedHosts doc block states that premise ("Bundled ConPTY still needs the worker fallback because node-pty defers its own worker dispose until more output"), so I read the gate as deliberate rather than an oversight — but lines 16-24 also say the native/internal semantics were only re-checked for the JS field shape, not for behaviour.
Might be worth dropping the condition and calling disposeConoutWorker unconditionally here too, matching line 205. It is free either way by this file's own contract at lines 82-84 — idempotent on inbox, and on bundled it only resets the one-second drain timer — so if the premise holds it is a harmless no-op, and if it does not hold this stops being the arm that still leaks one worker per closed terminal.
| } catch { | ||
| // already gone | ||
| } | ||
| noteConPtyHostReleased(pty.ptyProcess); |
There was a problem hiding this comment.
[Nice-to-have] This is the one kill site of the three that records the note neither inside the try nor behind an _isReady !== false guard — performCancelKill at 2470-2473 does both, and the web-terminal wrapper at web-terminal-registry.ts:296-305 does both. So it records a pseudo-console close on two paths where no close happened: kill() threw (the catch right above), and _isReady === false so node-pty only queued the teardown into _deferreds.
Not observable today, and I checked rather than assumed: killPty has exactly one caller (:675, inside cleanup()), and ShellExecutionService.cleanup() has exactly one non-test caller (:692, the process.on('exit') handler — which the comment at 596-597 also states). The module-level WeakSet dies with the process, so no later releaseConPtyHost can read a poisoned entry. Non-win32 exposure is nil because getCleanupStrategy() selects posixStrategy off Windows.
Worth aligning anyway, because it becomes a real leak-suppressor the first time cleanup() gains a live caller (graceful shutdown, serve/daemon teardown) — a false note makes the later release skip both the native close and, on the inbox backend, the worker dispose. Moving it inside the try with the same _isReady !== false guard as 2470-2472 makes the three sites read alike; alternatively a win32 guard inside noteConPtyHostReleased itself would stop its correctness depending on caller discipline.
| // `{ kind: 'background' }` abort (see the abortHandler switch below), | ||
| // and that abort's sole producer is the shell tool's Ctrl+B handler | ||
| // firing the `promoteAbortController` it created on the foreground | ||
| // `execute()` path (tools/shell.ts) — the one call site that also |
There was a problem hiding this comment.
[Suggestion] I verified this claim end to end at f7fedc20 and it holds — the enumeration is in the review body. But it is a runtime coupling across three packages with nothing pinning it: AppContainer.tsx:4848 (double-gated on ToolNames.SHELL + promoteAbortController !== undefined) → coreToolScheduler.ts:5020 (the only writer of that field) → tools/shell.ts:2646, which is the same foreground execute() that passes { postPromote } at :2623. A future caller that aborts { kind: 'background' } without postPromote reaches the windowsKillPid reap below with no type error and no failing test, and the failure mode is the #6067 recycled-pid kill this module avoids everywhere else by never signalling the pid.
Two ways to make the argument local instead of a cross-package enumeration, either fine:
- Keep the unconditional attach (the worker release and the
'error'-listener uncaughtException guard both need it) but gate only thewindowsKillPidreap onpostPromotebeing present. The reap is a caller-ownership transfer and reads naturally as opt-in; the release and the listener are internal housekeeping and are not. - Or pin the enumeration with a test: promote without
postPromote, assert the settle path releases the worker and does not dispatchwindowsKillPid. That converts the comment's claim into something a futureexecute()caller can trip.
The first is a one-line change and makes the safety argument survive a refactor of tools/shell.ts or the scheduler.
Stale CHANGES_REQUESTED, raised at 3718031; head is now f7fedc2 (latest main merged in). R4-1 (PR body off-template) is fixed — the body now carries all nine required template headings plus the Chinese section. R5-5's remaining native-close half is not this client's to fix: upstream microsoft/node-pty#965, tracked locally as #11352 under the recorded maintainer decision to land the worker half and stop certifying the host half. 30/30 review threads resolved, CI green at f7fedc2, chiga0 and qwen-code-dev-bot both APPROVED at that head. Dismissing to unblock; the remaining doc-comment over-claim in conpty-host.ts (releaseConPtyHost, 'from the live arm AND from the exited arm alike ... its queued kill() stays the single closer') is noted as a follow-up.
* fix(acp): pass the shell execution config through ACP tool dispatch
The ACP tool dispatch called `invocation.execute(signal, onToolProgress)`
with no third argument, so `ShellToolInvocation` fell back to
`shellExecutionConfig ?? {}`. The PTY was then sized 80x30 from
`shellExecutionService`'s own fallbacks — not even `Config`'s 80x24
default — and `pager`, `showColor` and `maxBufferedOutputBytes` were
dropped for every ACP tool call.
The TUI scheduler already passes `config.getShellExecutionConfig()`
(coreToolScheduler); do the same here.
This is how the process tree in QwenLM#11303 was identified: the reporter's
orphans were all `conhost.exe --headless --width 80 --height 30`, which
is this fallback's signature and nothing else's. The leak itself is a
separate defect, fixed in QwenLM#11313.
* fix(core): reap surviving hook process trees on Windows
`terminateSurvivingHookProcessGroup` was an explicit no-op on win32, so
when a parent-exit-surviving hook (MessageDisplay / StopFailure /
SessionDelete) was cancelled, nothing reaped the shell the supervisor had
started. Those hooks spawn a detached supervisor plus a shell child and
`unref()` it, so the parent's own tree kill on the supervisor is not a
reliable substitute: the supervisor may already have exited, which
reparents the shell out of its tree, or its taskkill may fail. The result
is the leftover `cmd.exe` processes reported in QwenLM#11303.
Implement the branch with `taskkill /f /t`, extracted from
`terminateWindowsHookProcessTree` as a shared pid-only helper.
Guarded by a `process.kill(pid, 0)` liveness probe first. Windows has no
process group to signal, so an unguarded taskkill against a pid that has
already exited could land on a recycled pid belonging to an unrelated
application — the QwenLM#6067 collateral-kill failure mode. The POSIX branch
gets that safety for free by signalling the negated pid.
* fix(vscode): shut the ACP CLI down gracefully instead of killing it
`AcpConnection.disconnect()` did a bare `this.child.kill()`. On Windows
that is `TerminateProcess`: the CLI's `process.on('exit')` handlers never
run, so every PTY, ConPTY host, sleep inhibitor and tracked child process
it was holding is orphaned. That is why the leaked processes in QwenLM#11303
survived until the VS Code window itself was closed.
Close the child's stdin instead. Ending the ndjson stream is the CLI's
own shutdown path: `await connection.closed` returns, it fires SessionEnd
hooks, drains the MCP pool, disposes its sessions and exits normally, so
its exit cleanup actually runs.
Force-kill only if that does not land within 5s — `taskkill /f /t` on
Windows, since at that point the CLI is unresponsive and nothing else
will reap the shells underneath it, and SIGKILL elsewhere. The timer is
cleared as soon as the child exits.
A late write error on a pipe whose reader is gone arrives as an 'error'
event, and an unhandled one on an EventEmitter throws — in the extension
host, not here — so stdin gets a one-shot error listener before it is
ended.
* fix(vscode): bind the ACP exit handler to the child that owns it
`setupChildProcessHandlers` keyed its exit handler on `this.child`, not on
the child it was installed for. A superseded child exiting after
`connect()` installed its replacement therefore nulled out the *live*
connection and reported it as disconnected.
Latent before, because `disconnect()` force-killed the old child and it
was usually gone before the replacement was assigned. The graceful
shutdown in the previous commit widens that window to seconds, so bind
the handlers to their own child.
* fix(vscode): handle the ACP exit rejection before anything races it
`processExitPromise` is created in `setupChildProcessHandlers` but its
only consumer is the `Promise.race` in `initialize()`, which attaches
much later. A child that exits in between — a failed startup, or a
superseded child winding down after `disconnect()` — rejected it with no
handler attached: an unhandled rejection in the extension host.
Mark it handled at creation. The race still receives the original promise
and still sees the rejection, so nothing else changes.
Caught by CI on this branch: all 521 companion tests passed but vitest
reported the suite as failed on the unhandled error, which is the same
shape the extension host would have hit at runtime.
* test(vscode): cover the Windows escalation branch, which CI never reaches
`Test (windows-latest)` is skipped on PRs, so a test that branches on
`process.platform` only ever exercises its POSIX half in CI — and the
Windows half is the whole point here.
Pin the platform on both escalation tests: one asserts SIGKILL on POSIX,
the other asserts the `taskkill /f /t` tree kill on win32. `execFile` is
now mocked alongside `spawn` so the Windows branch can be asserted
without a real taskkill on the runner.
* fix(vscode): let the CLI finish its own shutdown before force-killing it
The 5s grace was shorter than the shutdown it was waiting for. On the
ide_close path the CLI budgets 8s for the MCP pool drain
(shutdownMcpPool(8_000), acpAgent.ts:2902) and 30s for the session drain
(SESSION_DRAIN_TIMEOUT_MS, acpAgent.ts:496), so a single slow MCP server
put the escalation in the middle of a wind-down that was progressing
correctly, skipping the process.on('exit') cleanup this teardown exists
to protect. Raise the grace to 40s to cover both stages, and pin the
boundary in the escalation tests: nothing may be signalled one tick
before the deadline, so a grace shorter than the CLI's own budget now
reds them.
No SIGTERM stage on POSIX: shutdownHandler is still attached during the
wind-down (process.off('SIGTERM', ...) sits in the finally at
acpAgent.ts:3119, after the awaits at 3112-3116), and `shuttingDown`
only blocks a second signal, so a SIGTERM would run a second concurrent
shutdown over the same pool and sessions.
Also split the escalation log by branch: only win32 performs a tree
kill, the POSIX branch signals the non-detached child alone, and the
shared message claimed a tree kill on both.
And move the two new constants above the class JSDoc, which they had
been inserted between, detaching the AcpConnection doc comment from the
class.
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Patrol-Run: qwen-pr-closeout/jmts86p0gqr
* fix(core): fall back to a direct kill when the surviving hook's taskkill fails
terminateSurvivingHookProcessGroup discarded the boolean
taskkillProcessTree was extracted to return, so a taskkill that errored
or exceeded WINDOWS_TASKKILL_TIMEOUT_MS left the hook's cmd.exe tree
running with nothing else able to reap it and only a debugLogger.warn as
a trace. The sibling caller in terminateWindowsHookProcessTree already
honours it.
The fallback stays behind the isProcessAlive probe, so the QwenLM#6067
recycled-pid guard is unchanged.
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Patrol-Run: qwen-pr-closeout/jmts86p0gqr
* fix: close Windows process cleanup review gaps
* fix: reap CLI process group and gate superseded ACP callbacks
Address qwen-code-ci-bot review findings on QwenLM#11102:
- Spawn the ACP child detached on POSIX so disconnect() can signal its
process group (reaping the PTYs, ConPTY hosts and MCP children the CLI
tracks) with a root-only fallback. Previously SIGKILL reached the CLI
root process alone and orphaned the tree.
- Gate the five SDK inbound callbacks on `this.child !== ownChild` so a
superseded connection stops dispatching into callbacks that read
`this.*` at call time.
- Re-probe liveness before the Windows pid-based SIGKILL fallback in
terminateSurvivingHookProcessGroup, so a pid taskkill already reported
dead is never signalled directly (collateral-kill risk).
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Patrol-Run: qwen-pr-closeout/jmtspbz90ri
* fix: guard retired ACP dispatch and pin teardown escalation tests
Two gaps remained after d67635b:
- sendPrompt's onEndTurn tail ran unguarded after a stale prompt resolved
post-disconnect, clearing the replacement session's streaming state. Bail
out when the connection was retired (this.sdkConnection !== conn).
- The five SDK inbound callback guards compared this.child !== ownChild,
which is wrong across the shutdown grace window (this.child is nulled
before the grace timer and initialize() re-runs on the still-current
child). Compare against the wired connection instead.
Pin the positive halves the first fix left unpinned: the surviving hook pid
is still reaped after the supervisor exits, a live child exit clears the
connection and fires onDisconnected, and the force-kill escalation still
fires when stdin could not be ended.
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Patrol-Run: qwen-pr-closeout/jmtspbz90ri
* fix: cover runExitCleanup in the ACP shutdown grace
The ide_close path budgets 8s for the MCP pool drain and 30s for the
session drain, then runs runExitCleanup() (up to OVERALL_CLEANUP_TIMEOUT_MS
= 5s) in the finally wrapping runAcpAgent. The 40s grace fired 3s before
that 43s bounded wind-down finished, cutting the CLI off mid-cleanup and
dropping the recording flush and MCP disconnects. Bump to 45s to cover all
three bounded stages.
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Patrol-Run: qwen-pr-closeout/jmtspbz90ri
* fix(vscode-ide-companion): guard stale session writes after supersede
newSession and loadSession wrote this.sessionId after awaiting the
connection they captured, with no supersede guard. disconnect() now
leaves the retired CLI alive for up to SHUTDOWN_GRACE_MS, so a late
session/new or session/load response from that retired CLI can land
after a replacement connection is installed and stamp the dead
session's id back onto the field disconnect() just nulled.
Gate both post-await writes on `this.sdkConnection === conn` (the same
guard sendPrompt already carries), and correct the escalation comment
and its test comment to name exactly what the POSIX group kill does and
does not reach. Add tests pinning the detached-spawn premise and the
stale-session guard.
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Patrol-Run: qwen-pr-closeout/jmtsxwmdgrw
* fix(vscode-ide-companion): pin re-connect supersede gate and align spawn comment
The spawn-option comment claimed the POSIX group signal reaps PTYs,
ConPTY hosts and MCP children. The escalation comment (corrected by
faab984) says the group kill only reaches the CLI root and its
non-detached MCP stdio children — it does NOT reach descendants that
call setsid() (detached hook supervisors/monitors, node-pty sessions).
Align the spawn comment with that.
Every supersede test drove the gate through disconnect(), which nulls
this.child and this.sdkConnection together, so the captured-identity
predicate and a weaker !this.child always agree. Add a re-connect test
that installs a replacement child while the old connection is still
winding down, pinning the stronger predicate.
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Patrol-Run: qwen-pr-closeout/jmttcwqrask
* fix(vscode-ide-companion): pin re-connect supersede gates for outbound session writes
The outbound guards (newSession/loadSession/sendPrompt) key on
`this.sdkConnection !== conn` (or `=== conn`) to detect a superseded
connection. Every test drove the gate through disconnect(), which nulls
this.child and this.sdkConnection together, so a weakened `!this.child`
still bails and the predicate is never discriminated. Add re-connect cases
that install a replacement child while the retired connection's promises
are still in flight: a guard weakened to `!this.child` would then stamp
the retired session id (newSession/loadSession) or fire onEndTurn
(sendPrompt), turning the test red.
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Patrol-Run: qwen-pr-closeout/jmttf1wj0so
* fix(vscode-ide-companion): fail superseded newSession/loadSession instead of returning the retired CLI's payload
The supersede guards added at newSession and loadSession correctly skipped the
stale write to this.sessionId, but still returned the retired CLI's response.
QwenAgentManager applies that payload unconditionally: applySessionStateFromResult
(qwenAgentManager.ts:1283) and restoreBaselineSessionStateAfterLoad (:1066) write
the dead CLI's model and mode state into the live webview's baselines, and
createNewSession hands the same promise to every concurrent caller through
sessionCreateInFlight (:1226-1234), so they all adopt it.
Both now throw RequestError.internalError({ details: 'connection superseded' }),
matching the convention the inbound callback guards in this same file already use.
Callers already have error paths: createNewSession rethrows non-auth errors
(isAuthenticationRequiredError matches AUTH_REQUIRED -32000, not INTERNAL_ERROR
-32603) and its finally clears sessionCreateInFlight, so concurrent callers get
the rejection instead of stale state; loadSessionViaAcp rethrows as well.
loadSession's gate moves outside its catch, so a supersede is no longer logged as
a request failure, and ahead of the success log, so a discarded load prints no
unqualified success line.
sendPrompt's guard is deliberately left returning: its only caller
(qwenAgentManager.sendMessage:397) is Promise<void> and discards the response, so
no payload is certified to anyone, and the guard's real job -- skipping onEndTurn
so the replacement session's streaming state survives -- is already correct.
Tests: both supersede cases now assert the rejection (code + data.details) rather
than awaiting a resolution. Mutation-verified: reverting either guard to
`return response` reds exactly those two cases ("promise resolved
{ sessionId: 'stale-from-retired-cli' } instead of rejecting"), 31 passed.
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Patrol-Run: qwen-pr-closeout/jmtu0hicwtk
* fix(vscode-ide-companion): close the superseded session on session replace
The agent keeps every session alive until told otherwise, and a retained
session still fires autonomous model turns when its background tasks
complete — each turn can spawn shells, which is what grew conhost.exe
without bound in QwenLM#11303 while the window stayed open. session/new and
session/load now close the superseded session once the replacement is
confirmed. Fire-and-forget: a refused or unsupported close (older CLI)
must not block the user's new session, and a later session/load of the
closed id simply re-reads the flushed transcript.
* fix(acp): narrow shell execution follow-up
Remove the risky hook-process and superseded-session cleanup changes, leaving only the ACP shell execution configuration passthrough.
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
---------
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
Released in v0.23.3. |
…wenLM#11572) WebTerminalRegistry released PTY resources only in release(). handleExit set `exited`/`exitCode` and notified the exit listeners, touching nothing else, and the browser route deliberately keeps an exited session alive for scrollback replay (finishExited leaves releaseAfterReplay at its false default). The client will not release earlier either: a live exit closes the socket with 4000, which is non-retryable, so only a tab close, a workspace drain, dispose() or the 15-minute idle reclaim ever freed the PTY. So every exited web terminal held node-pty's conout worker - and, upstream, its conhost.exe - for up to IDLE_RECLAIM_MS. Exited sessions are also excluded from the admission cap on purpose, so accumulation inside that window was unbounded. Extract the PTY-resource half of release() into releasePtyResources() and call it from handleExit, deferred one setImmediate so the trailing onData callbacks node-pty may still have queued reach the buffer first - the same race shellExecutionService drains before finalizing. The helper keeps the session's map entry and its buffer, so readSnapshot() replay and the route's releaseAfterReplay path are unaffected, and it never signals the pid: the shell is gone and its pid may be recycled, which is why QwenLM#11313 added releaseHost instead of reusing kill(). A per-session flag keeps a later release() from disposing anything twice. release() now routes both arms through the helper. killPtyTree stays on the live arm only and still runs first, so releaseHost keeps seeing the close the wrapper noted. Everything between the old detach site and the new one is synchronous (spawnSync, process.kill, pty.kill), so no onData callback can interleave and the live path's observable behaviour is unchanged. Adds three tests: resources freed at exit time rather than at the reclaim, scrollback still replayable afterwards, and no double free when release() follows. Verified at the mechanism level on Linux with os.platform() mocked to win32; actual conout-worker and conhost.exe reclamation on a real Windows ConPTY is not verified here, and QwenLM#11352's upstream close defect is untouched. Fixes QwenLM#11353 Patrol-Run: qwen-issue-patrol/jmtvhcstvvk Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
8 participants
What this PR does
Partially addresses #11303 by releasing node-pty's Windows conout worker when a foreground shell exits, a backgrounded shell settles, or a web terminal is released. These paths previously dropped their PTY ownership without reaching node-pty's worker teardown, so one worker thread remained for the lifetime of the CLI.
This branch now includes current
mainand #11497. Shell-tool PTYs therefore use node-pty's bundled ConPTY backend: that backend releases its host reference after spawn, but its natural-exit callback deliberately skips worker cleanup. This PR supplies that missing worker teardown. It also keeps host-close tracking separate from worker cleanup so a cancelled bundled PTY does not close the native host twice and still starts the worker drain when no more output arrives.Web-terminal PTYs still use the Windows inbox backend. This PR releases their worker at terminal release time; the inbox backend's natural-exit host leak remains outside this PR.
Why it's needed
#11303 reported 347 orphaned Windows console hosts and 353 parent-process threads after a long VS Code Companion session. node-pty creates one worker thread per PTY to drain the conout pipe, but does not terminate that worker after a natural exit. Even with #11497 handling the shell-tool host lifecycle through bundled ConPTY, the worker leak remains unless the owner explicitly releases it.
The implementation avoids using the public PTY kill path after natural exit. The inbox backend can fall back to terminating a recycled shell pid (#6067), while the bundled backend waits for more output before disposing its worker. Direct, best-effort worker teardown avoids both failure modes and degrades to the previous leak if node-pty's private shape changes.
Reviewer Test Plan
How to verify
The focused unit suite covers foreground completion, background settle, release of exited and live web terminals, cancellation before terminal readiness, bundled cancellation after the host was already closed, non-Windows no-op behavior, and failure containment. The combined suite passes 181/181 tests. A mutation that removes the bundled-worker cleanup makes the new cancellation assertion fail with zero worker disposals, confirming that the assertion is load-bearing.
For a process-level Windows check, start a VS Code Companion session containing current
mainplus this PR, record the parent thread count and all direct ConPTY host children (conhost.exeandOpenConsole.exe), run repeated foreground, background, and cancelled shell commands, then recount. The parent thread count should not grow by one per completed PTY. For shell-tool PTYs, the combined #11497 + this PR result should also avoid accumulating host children. Web-terminal inbox-host lifecycle is not an acceptance criterion for this PR.Evidence (Before & After)
N/A — this is resource-lifecycle hardening with no UI change. No real Windows host was available locally; the Windows process-level check above remains for CI or a maintainer machine.
Tested on
✅ tested ·⚠️ pending CI or maintainer verification
Environment (optional)
macOS, Node 22.22.0: core typecheck passed; targeted ESLint passed; 181/181 focused tests passed. The core package built successfully as part of the repository build. The repository-wide build later stopped at the unchanged Web Templates export-size gate (4,217,267 bytes versus the 4,200,000-byte budget), outside this PR's core-only diff.
Risk & Scope
Linked Issues
Refs #11303 — deliberately not
Fixes, because the remaining inbox web-terminal and agent-view host lifecycle is outside this PR.Refs #11497, #6067, and #11102.
中文说明
这个 PR 做了什么
本 PR 部分解决 #11303：当前台 shell 退出、后台 shell settle，或 web terminal 被释放时，显式释放 node-pty 的 Windows conout worker。此前这些路径会放弃 PTY 所有权，却没有走到 node-pty 的 worker teardown，因此每次都会留下一个线程直到 CLI 退出。
该分支现已合入当前
main和 #11497。shell tool PTY 因而使用 node-pty 自带的 ConPTY 后端：该后端会在 spawn 后释放 host 引用，但它的自然退出回调会刻意跳过 worker 清理。本 PR 补上这一步。同时，代码把“host 已关闭”和“worker 已清理”分开记录，因此取消 bundled PTY 时既不会重复关闭 native host，也会在没有后续输出时启动 worker drain。Web terminal PTY 仍使用 Windows inbox 后端。本 PR 在 terminal release 时释放它的 worker；inbox 后端在自然退出时的 host 泄漏不属于本 PR 范围。
为什么需要它
#11303 报告了一个长时间运行的 VS Code Companion 会话中出现 347 个孤儿 Windows console host 和 353 个父进程线程。node-pty 会为每个 PTY 创建一个 worker 线程来读取 conout pipe，但自然退出后不会终止该 worker。即使 #11497 已通过 bundled ConPTY 处理 shell tool 的 host 生命周期，只要 owner 没有显式释放，worker 泄漏仍然存在。
实现没有在自然退出后调用公开的 PTY kill 路径。inbox 后端可能回退到终止已经复用的 shell pid（#6067），而 bundled 后端会等待更多输出后才 dispose worker。直接、尽力而为的 worker teardown 避开了这两种失败模式；如果 node-pty 的私有结构变化，则只会退化回原来的泄漏。
审阅者测试计划
如何验证
定向单元测试覆盖前台完成、后台 settle、已退出和仍存活的 web terminal release、terminal ready 前取消、host 已关闭后的 bundled cancel、非 Windows no-op，以及失败隔离。组合测试 181/181 通过。内存变异删除 bundled worker 清理后，新的 cancel 断言会因 worker dispose 次数为 0 而失败，证明该断言确实承重。
如要在 Windows 上做进程级验证，请启动包含当前
main与本 PR 的 VS Code Companion 会话，记录父进程线程数及所有直接 ConPTY host 子进程（conhost.exe和OpenConsole.exe），重复运行前台、后台和被取消的 shell 命令，再次统计。每个已完成 PTY 不应再让父进程线程数增加 1。对于 shell tool PTY，#11497 与本 PR 的组合结果也不应累积 host 子进程。Web terminal 的 inbox host 生命周期不是本 PR 的验收项。证据（改动前后）
N/A——这是资源生命周期加固，没有 UI 变化。本地没有可用的真实 Windows 主机；上述 Windows 进程级验证仍需由 CI 或 maintainer 机器完成。
测试环境
✅ 已测试 ·⚠️ 等待 CI 或 maintainer 验证
环境（可选）
macOS，Node 22.22.0：core typecheck 通过；定向 ESLint 通过；181/181 个定向测试通过。core package 已在仓库构建中成功完成。全仓构建随后停在未被本 PR 修改的 Web Templates 导出体积门禁（4,217,267 字节，预算为 4,200,000 字节），不属于本 PR 的 core-only diff。
风险与范围
关联 Issue
Refs #11303——刻意不用
Fixes，因为 web terminal 和 agent-view 的 inbox host 生命周期仍然不在本 PR 范围内。Refs #11497、#6067 和 #11102。