fix(core): tree-kill PTY shell tree on Windows to stop pwsh leak (#5873) - #5892
Merged
Conversation
Contributor
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
Collaborator
|
Thanks for the PR @doudouOUC! Template is complete ✓ — all required sections present with bilingual translation. Direction: This is a clear, high-impact bug fix for a 100%-reproducible Windows OOM — every tool call leaked a Solution: The scope is tight and focused — 2 files, all changes win32-gated, zero risk to POSIX paths. The approach of aligning every teardown path on The second commit (0ff745d) extends coverage with promoted-shell reap tests and child_process fallback tests — closing the remaining test gap the first triage noted. The author's response to review feedback was thoughtful: adopted the Moving on to code review. 🔍 中文说明感谢 @doudouOUC 的贡献！ 模板完整 ✓ —— 所有必填章节齐全，含中英双语翻译。 方向： 这是一个明确的、影响面大的 bug 修复——Windows 下每次工具调用 100% 泄漏 方案： 范围收敛、聚焦——2 个文件，所有改动限定 win32，POSIX 路径零风险。将所有 teardown 路径对齐到 第二个提交 (0ff745d) 扩展了 promoted-shell 回收和 child_process 兜底测试——补上了首次 triage 指出的测试缺口。作者对 review 反馈的回复很到位：采纳了 进入代码审查 🔍 — Qwen Code · qwen3.7-max |
Collaborator
|
Code review: No blockers. The implementation is correct and well-reasoned across both commits.
The second commit (0ff745d) added 7 more tests covering promoted-shell reap (3 tests: onSettle, onData-only, and ESRCH skip) and the The absolute System32 path resolution for One note: the comments throughout are denser than typical for this codebase, but given that ConPTY process lifecycle is subtle and wrong teardown order causes OOM, the why documentation here earns its place. Reuse check: All new logic reuses existing infrastructure — Unit tests (worktree): All 120 tests pass (100 existing + 20 new across both commits): CI results: All platform tests passed:
Real-scenario test (Linux, non-reproducible platform): The bug requires Windows + ConPTY to reproduce. The installed The 中文说明代码审查： 无阻塞项。两个提交的实现都正确、合理。
第二个提交 (0ff745d) 新增 7 个测试：promoted-shell 回收（3 个：onSettle、onData-only、ESRCH 跳过）和
注释比项目一般风格更密集，但鉴于 ConPTY 进程生命周期微妙、teardown 顺序错误就 OOM，这里的 why 文档是有价值的。 复用检查： 所有新逻辑复用已有基础设施—— 单元测试（worktree）： 120 个测试全部通过（100 既有 + 两个提交共 20 新增）。 CI 结果： 全平台测试通过。Ubuntu ✅ (7m51s)，macOS/Windows 矩阵优化跳过，6 项必要检查全部通过 ✅。 真实场景测试（Linux，不可复现平台）： 该 bug 需要 Windows + ConPTY 才能复现。已安装的 — Qwen Code · qwen3.7-max |
Collaborator
|
This is a clean, well-scoped bug fix for a real and severe problem — every Windows tool call leaked a The tree-kill vs shell-pid-only distinction on cancel vs normal completion is the kind of detail that separates a fix that works from one that just moves the bug. The second commit (0ff745d) closed the remaining test gaps — promoted-shell reap (3 tests including the ESRCH guard) and child_process fallback (4 tests covering both failure modes and their negative guards). The total of 20 new tests is comprehensive. The author's response to review feedback was well-reasoned: adopted the All CI checks pass. The POSIX path is unaffected (confirmed by smoke test). The Approving. ✅ 中文说明这是一个干净、范围合理的 bug 修复，针对一个真实且严重的问题——Windows 下每次工具调用泄漏 取消 vs 正常完成时的树杀 vs 只杀 shell pid 区分，是把「能用的修复」和「只是把 bug 搬到别处」区分开的关键细节。第二个提交 (0ff745d) 补上了剩余的测试缺口——promoted-shell 回收（3 个测试含 ESRCH 守卫）和 child_process 兜底（4 个测试覆盖两种失败模式及其反向守卫）。共 20 个新测试，覆盖全面。 作者对 review 反馈的回复有理有据：采纳了缺失的 CI 全部通过。POSIX 路径不受影响（冒烟测试确认）。 批准。✅ — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
approved these changes
Jun 26, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
doudouOUC
enabled auto-merge
June 26, 2026 08:10
qwen-code-ci-bot
requested changes
Jun 26, 2026
doudouOUC
force-pushed
the
claude/trusting-antonelli-7cc9c6
branch
from
June 26, 2026 08:31
abb7d5a to
6a3b317
Compare
doudouOUC
force-pushed
the
claude/trusting-antonelli-7cc9c6
branch
from
June 26, 2026 09:23
6a3b317 to
176f1df
Compare
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
No review findings. Downgraded from Approve to Comment: CI still running.
Two low-confidence suggestions for the author to consider (terminal-only, not blocking):
-
Finalizer missing
ptyProcess.kill()fallback (shellExecutionService.ts:1694-1700): AfterwindowsKillPidin the onExit finalizer, addingtry { ptyProcess.kill(); } catch {}would be consistent withperformCancelKill's pattern and close a small gap if taskkill fails to launch. -
Missing test for
windowsKillPiderror listener (test file): No test fires an'error'event on themockCpSpawnEventEmitter during the normal-completion finalizer path. The cancel path has this coverage but the finalizer doesn't.
— qwen3.7-max via Qwen Code /review
doudouOUC
force-pushed
the
claude/trusting-antonelli-7cc9c6
branch
from
June 26, 2026 10:16
176f1df to
373c288
Compare
Collaborator
Author
|
Thanks — on the two low-confidence suggestions: 2. Test for the 1. 🤖 Addressed by Claude Code |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
No review findings. Downgraded from Approve to Comment: CI still running.
— qwen3.7-max via Qwen Code /review
Collaborator
|
@qwen-code /triage |
qwen-code-ci-bot
previously approved these changes
Jun 26, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
wenshao
requested changes
Jun 26, 2026
doudouOUC
force-pushed
the
claude/trusting-antonelli-7cc9c6
branch
2 times, most recently
from
June 26, 2026 12:24
7c4b6c0 to
1b9f374
Compare
| os.platform() === 'win32' && | ||
| ShellExecutionService.isPtyActive(ptyProcess.pid) | ||
| ) { | ||
| windowsKillPid(ptyProcess.pid, cancelKillDispatched); |
Collaborator
There was a problem hiding this comment.
[Suggestion] This calls windowsKillPid to reap the lingering ConPTY shell but does not also call ptyProcess.kill() to tear down the ConPTY host. If taskkill fails to launch (the 'error' event is silently swallowed by windowsKillPid's no-op listener), both the shell and the ConPTY host leak permanently — especially since line 1714 deletes from activePtys, removing the process-exit cleanup() safety net.
performCancelKill (line 2171) and windowsStrategy.killPty (line 522) both call ptyProcess.kill() unconditionally alongside taskkill as defense-in-depth. This path and the firePostSettle path (line 1973) should match:
Suggested change
| windowsKillPid(ptyProcess.pid, cancelKillDispatched); | |
| windowsKillPid(ptyProcess.pid, cancelKillDispatched); | |
| try { ptyProcess.kill(); } catch { /* already gone */ } |
ptyProcess.kill() is harmless once the process is dead (the isPtyActive guard already confirmed liveness, and the try/catch handles the already-gone case).
— qwen3.7-max via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment.
Respectfully skipping this one for the finalizer / firePostSettle paths — leaving the thread open for you to weigh in.
By the time these run, onExit/settle has already fired, and ptyProcess.kill() cannot reap the lingering pwsh process — that's the microsoft/node-pty#333 reason we tree-kill with taskkill in the first place. So adding it here does not actually close the "taskkill failed → shell leaks" gap it's meant to: if taskkill fails, the pwsh leaks regardless, and ptyProcess.kill() can't reclaim it.
performCancelKill / killPty justifiably call ptyProcess.kill() because they run while the shell/host is still live (pre-exit, or app-exit with a running shell) — a different lifecycle point. The real mitigation for a silently-failing taskkill is the diagnostic logging from your first comment, which I added.
If you've seen a case where the ConPTY host genuinely lingers after onExit and ptyProcess.kill() reclaims it, I'll add it — happy to be shown wrong.
🤖 Addressed by Claude Code
doudouOUC
force-pushed
the
claude/trusting-antonelli-7cc9c6
branch
from
June 26, 2026 15:30
1b9f374 to
de28ed3
Compare
wenshao
reviewed
Jun 26, 2026
doudouOUC
force-pushed
the
claude/trusting-antonelli-7cc9c6
branch
from
June 26, 2026 16:27
de28ed3 to
ded0acf
Compare
Under ConPTY, ptyProcess.kill() only tears down the pseudo-console host, not the pwsh/powershell/cmd process (microsoft/node-pty#333), so the interactive PTY path leaked one idle shell per tool call until the machine OOMed. The child_process fallback already tree-kills with `taskkill /f /t`; this aligns the teardown paths on Windows. taskkill is resolved once to its absolute `%SystemRoot%\System32\taskkill.exe` path and never spawned by bare name — on Windows child_process resolves a bare command through PATH and the current directory, so a planted taskkill.exe/.bat could otherwise run from these cleanup paths (RCE out of a benign teardown). All invocations use the same flags-first arg order. The runtime paths log taskkill launch/non-zero-exit failures via debugLogger (a swallowed failure would let the leak silently return); the process-exit cleanup paths don't log, since an async debug write can't flush before the process dies. - windowsStrategy.killPty (process 'exit' cleanup): spawnSync taskkill /f /t /pid for the tree, then always ptyProcess.kill() for the ConPTY host. - PTY performCancelKill (win32): tree-kill SYNCHRONOUSLY (spawnSync) so taskkill enumerates and kills the tree BEFORE ptyProcess.kill() fires ClosePseudoConsole — an async taskkill could still be launching when the CTRL_CLOSE exit orphans the descendants. ptyProcess.kill() then tears down the host so the cancel can't hang. Sets cancelKillDispatched. - child_process performCancelKill (win32): taskkill with 'error' and 'exit' listeners falling back to child.kill('SIGKILL') — a failed launch OR a non-zero exit can't leave the abort hanging. - disposeForegroundPtyResources (normal completion / cancel): guarded reap (isPtyActive). Blast radius keys off cancelKillDispatched (captured in performCancelKill before the shell exited, not a late abortSignal.aborted read). Cancel tree-kills; normal completion is shell-pid-only so a detached child (e.g. Start-Process) survives. - firePostSettle (background-promote): the same guarded shell-pid-only reap on the promoted shell's natural exit (before the onSettle early-return, so it fires for onData-only promotes too) — the background lifecycle never reaches the foreground finalizer, so without this a backgrounded command still leaked. The async taskkill in windowsKillPid attaches no-op-but-logged 'error' and 'exit' listeners: a failed launch surfaces as an async EventEmitter 'error' event, not a throw, and would otherwise crash the CLI from a cleanup path. Adds win32 regression tests across every path: absolute-System32 executable, sync cancel ordering, taskkill launch failure / non-zero exit / success, late-abort-after-normal-exit, pid already gone, ptyProcess.kill / spawnSync throwing, and promoted-shell natural-exit reaps (onSettle and onData-only). 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)
doudouOUC
force-pushed
the
claude/trusting-antonelli-7cc9c6
branch
from
June 26, 2026 16:33
ded0acf to
2c4f1bf
Compare
…ale doc (#5873) Addresses review follow-ups on the Windows pwsh-leak fix: - windowsKillPid JSDoc: its 'error'/'exit' listeners now log via debugLogger rather than being no-ops — correct the stale comment. - Add a win32 exit-cleanup test for killChildProcesses, pinning that it uses the absolute System32 taskkill path (a regression to the bare 'taskkill' name would reopen the binary-planting hole). - Add a win32 firePostSettle test asserting the post-promote reap is skipped when the promoted pty already exited (isPtyActive false / ESRCH) — the pid-reuse guard previously had no negative coverage. 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)
wenshao
reviewed
Jun 26, 2026
| // before the process dies (and a sync stderr write would be exit-time | ||
| // noise). The unconditional ptyProcess.kill() below is the mitigation; | ||
| // the runtime reap paths (windowsKillPid), which DO flush, keep logging. | ||
| spawnSync(WINDOWS_TASKKILL, ['/f', '/t', '/pid', pid.toString()]); |
Collaborator
There was a problem hiding this comment.
[Suggestion] spawnSync here (and at the PTY cancel path ~line 2202) has no timeout option. If taskkill hangs — antivirus interception, enterprise endpoint security hooking the spawn, or system under extreme I/O pressure — the Node.js event loop blocks indefinitely. The POSIX cancel path has a SIGKILL_TIMEOUT_MS escalation as a natural bound; the Windows sync path has none.
Suggested change
| spawnSync(WINDOWS_TASKKILL, ['/f', '/t', '/pid', pid.toString()]); | |
| spawnSync(WINDOWS_TASKKILL, ['/f', '/t', '/pid', pid.toString()], { timeout: 5000 }); |
When the timeout fires, spawnSync returns { error: { code: 'ETIMEDOUT' } } which the existing r.error check already logs, and the subsequent ptyProcess.kill() still fires as the fallback. Same fix applies to the PTY cancel path at ~line 2202.
— qwen3.7-max via Qwen Code /review
| ); | ||
| if (!exited) { | ||
| try { | ||
| child.kill('SIGKILL'); |
Collaborator
There was a problem hiding this comment.
[Suggestion] When taskkill fails (launch error or non-zero exit), killChildFallback calls child.kill('SIGKILL') which kills only the shell process — not its descendants. On machines where taskkill consistently fails (antivirus, enterprise lockdown, EMFILE under load), every cancelled child_process command leaks its descendant tree, reproducing the same #5873 OOM on a different code path.
The old code had no fallback at all, so this is already an improvement. But consider either retrying with spawnSync(WINDOWS_TASKKILL, ['/f', '/t', '/pid', child.pid.toString()]) as a synchronous second attempt, or at minimum logging a clear warning (e.g. "descendant tree may be leaked") so the diagnostic trail is unambiguous when this fires in production.
— qwen3.7-max via Qwen Code /review
wenshao
reviewed
Jun 26, 2026
| // still fail silently. These callers run at runtime (not the process | ||
| // 'exit' handler), so the async debug write flushes. Logging only; the | ||
| // shell-only/tree-kill behavior is unchanged. | ||
| killer.on('exit', (code, signal) => { |
Collaborator
There was a problem hiding this comment.
[Suggestion] This new windowsKillPid exit listener is the only coverage for taskkill launches that succeed but then fail, but the tests only exercise the launch-error path for this helper and the separate child_process cancel fallback. If this listener is removed or its code !== 0 || signal guard regresses, the runtime PTY reap/post-promote observability would silently disappear while the current test suite still passes.
Please add a focused Windows PTY reap test that returns a dedicated taskkill EventEmitter from mockCpSpawn, drives either normal completion or promoted settle, emits taskkillProc.emit('exit', 1), and asserts the windowsKillPid: taskkill exited non-zero... warning is produced.
— gpt-5 via Qwen Code /review
wenshao
reviewed
Jun 26, 2026
wenshao
left a comment
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
The diff itself looks correct and is thoroughly tested (120/120 in shellExecutionService.test.ts). The suggestions from the previous round — the spawnSync timeout on the Windows sync paths, and the windowsKillPid 'exit'-listener test — are still unaddressed at this HEAD and continue to apply.
One new item worth surfacing: the bare-name taskkill hardening is incomplete outside this file. This PR correctly resolves taskkill to the absolute WINDOWS_TASKKILL path in shellExecutionService.ts, closing the CWD/PATH binary-planting vector. But the same exploitable bare-name spawn remains in three other Windows runtime teardown paths:
packages/core/src/tools/shell.ts:2778— orphan-kill on the promote mkdir-error pathpackages/core/src/tools/shell.ts:2928—cancelChild(backgrounded-shell cancel)packages/core/src/tools/monitor.ts:457—killChildProcessGroup
Each is win32-gated, runs at runtime, and inherits the process CWD, so a planted taskkill.exe/.bat is still executable from these paths — the same class fixed here. Recommend exporting WINDOWS_TASKKILL (or a shared helper) and reusing it at these sites, in this PR or a fast-follow. (The two shell.ts sites also still use the pre-PR /pid … /f /t arg order this PR standardized away from.)
Optional hardening on WINDOWS_TASKKILL itself: it trusts process.env['SystemRoot'], so a relative-poisoned value (e.g. SystemRoot=Windows) would make the resolved path relative and re-open CWD resolution — a path.win32.isAbsolute() guard (falling back to C:\Windows\System32\taskkill.exe) would close that residual.
— claude-opus-4-8 via Qwen Code /qreview
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
No review findings. Downgraded from Approve to Comment: CI still running.
— qwen3.7-max via Qwen Code /review
wenshao
approved these changes
Jun 26, 2026
Collaborator
|
@qwen-code /triage |
qwen-code-ci-bot
approved these changes
Jun 26, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
This was referenced Sep 8, 2026
qqqys
pushed a commit
to qqqys/qwen-code
that referenced
this pull request
Sep 10, 2026
QwenLM#11313) * fix(core): release the ConPTY host after every PTY on Windows On Windows every completed tool call orphaned one headless `conhost.exe` (~8 MB) for the lifetime of the CLI process: QwenLM#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 QwenLM#5892, not wider: it closes only the HPCON we opened, so unlike a `taskkill /t` it can never reach a third-party application (the QwenLM#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 QwenLM#11303 * fix(core): release the ConPTY host without killing a recycled pid 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 QwenLM#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. * fix(core): make the ConPTY host release idempotent 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(core): cover the Windows release branch, which CI never reaches `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. * docs(core): stop claiming the cancel-path kill() proves the shell is 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 * test(core): pin the ConPTY release placement and the worker-dispose guard Two coverage gaps in the QwenLM#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 * fix(core): close the two remaining ConPTY release gaps on Windows 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 * docs(core): stop certifying a ConPTY host close that never happens 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 QwenLM#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`. * fix(core): release the conout worker on the remaining Windows release paths Address the closeout findings on the ConPTY worker release: - conpty-host.ts: stop gating the conout worker dispose behind the native-kill shape check — a node-pty bump that renames _pty/_ptyNative must not also lose the only teardown that frees anything today. Correct the doc block's two overbroad claims (the 'error' entry point can reach a live baton, and enableInteractiveShell only gates the shell-tool path). - web-terminal-registry.ts: release the conout worker on the live branch too. killPtyTree's pty.kill() defers its whole teardown while _isReady is false, so a terminal released before its first output byte stranded the worker. The wrapper now notes the close only when kill() really ran, so release() never double-closes a pseudo-console kill() already closed. - shellExecutionService.ts: fix the settle-path comment (it released only the worker, not the host), name the settle-time taskkill reap the unconditional attach adds, correct the 'error'-listener rationale (node-pty's listeners('error').length < 2 throw), and qualify the three "PR-2 detach-everything" statements that the unconditional attach now falsifies. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Patrol-Run: qwen-pr-closeout/jmtsxwmdgrw * fix(core): format web-terminal-registry.test.ts to satisfy prettier Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Patrol-Run: qwen-pr-conflict/jmtt2wnugs4 * fix(core): avoid double-close on web-terminal deferred live release The web-terminal live-release path called releaseConPtyHost even when node-pty's kill() was still queued in _deferreds (the shell had not yet emitted its first byte), closing the same HPCON a second time. Route that case through the new disposeConoutWorker so the queued kill() stays the single closer, and correct the two now-false invariants in the comments. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Patrol-Run: qwen-pr-closeout/jmttf1wj0so * docs(core): reattach the releaseConPtyHost contract block to its function Inserting disposeConoutWorker between the contract block and releaseConPtyHost orphaned it: the block closed with */ and a new /** started on the very next line, so TypeScript resolved the leading trivia to disposeConoutWorker and releaseConPtyHost documented as nothing. The export all three cross-file call sites invoke (shellExecutionService.ts:1871 and :2142, web-terminal-registry.ts:321) had no hover doc. Move disposeConoutWorker and its own JSDoc above the contract block, so the block sits immediately before releaseConPtyHost with a blank line on each side and neither reads as a continuation of the other. The block is relocated verbatim -- the caller enumeration in it is the text a resolved thread already ruled on. Leave a one-line pointer in the disposeConoutWorker doc for the QwenLM#6067 and win32-only rationale rather than duplicating them. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Patrol-Run: qwen-pr-closeout/jmtu0hicwtk * docs(core): make the releaseConPtyHost caller enumeration match the call graph The three-group enumeration omitted the web-terminal release of an already-exited session and filed the cancel path wholly among the note-suppressed sites. Verified against the real call graph: - release() reaches session.pty.releaseHost?.() from two arms -- :464 for !session.exited and :472 for session.exited -- and both land on the same releaseConPtyHost(spawned) at web-terminal-registry.ts:321. The exited arm runs strictly after onExit, records no note, so it is a member of the no-op group, not the note-suppressed one. It is the primary web-terminal path for QwenLM#11303. - releaseHost branches only on _isReady (:317), never on session.exited, so the disposeConoutWorker route at :318 is reached from BOTH arms, not just the live one. - performCancelKill records the note only when _isReady !== false (shellExecutionService.ts:2418), so a cancel that lands before the shell's first output byte reaches the finalizer at :1871 with no note. - windowsKillPid's interactive-shell kill at :619-623 notes unconditionally and was unlisted. That is six distinct production states reaching releaseConPtyHost plus two routing to disposeConoutWorker; the previous text named six entries total and mis-classified two of them. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Patrol-Run: qwen-pr-closeout/jmtu0hicwtk * test(core): witness the throw guard in disposeConoutWorker Its twin inside releaseConPtyHost already had a dedicated test, but this one had none: disposeConoutWorker's only call site is web-terminal-registry.ts:318 and the sole test reaching it used a bare vi.fn() for conoutDispose, so deleting the try/catch left the whole suite green. The guard is load-bearing because release() calls session.pty.releaseHost?.() bare (:464/:472), after the session is already deleted from the map (:445), and dispose()'s loop (:422-426) has no per-iteration guard -- an escaping throw would abort teardown for every session still queued behind it. Mutation-verified: with the try/catch deleted, 1 failed | 27 passed; intact, 28 passed. Runs on the Linux lane because the file mocks node:os platform, so this is not a skip-on-CI witness. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Patrol-Run: qwen-pr-closeout/jmtu0hicwtk * docs(core): state both arms of the deferred releaseHost, and test the exited one The deferred branch's rationale was written only for the live release, where killPtyTree has just queued a kill() in node-pty's _deferreds. But release() reaches releaseHost from two arms -- :464 for !session.exited and :472 for session.exited -- and releaseHost branches on _isReady alone, never on session.exited. So a shell that exits before its first output byte (COMSPEC resolving to a binary that quits immediately, or a releaseWorkspace drain racing pwsh startup) lands in the same branch, where no kill() ran, nothing is queued, and the double-close argument does not apply -- the native baton is already erased by the exit-watcher, so a close would no-op rather than double-close. The worker is still the resource to free, which is what the else arm's own comment says it is there for. Split the comment into LIVE and EXITED so each arm carries its real reason. Add the missing case: _isReady = false, then onExit({ exitCode: 0 }), asserting conoutDispose once and kill/nativeKill/spawnSync never. Per conpty-host.ts the test asserts the worker dispose and the ABSENCE of the native kill, never that the conhost half was freed. Mutation-verified: deleting the _isReady === false branch turns the new case red (3 failed | 26 passed); intact, 29 passed. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Patrol-Run: qwen-pr-closeout/jmtu0hicwtk --------- Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Co-authored-by: Shaojin Wen <shaojin.wensj@alibaba-inc.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does
On Windows, qwen-code defaults to the interactive-shell PTY path (node-pty / ConPTY), and every tool call spawned a shell whose process tree was never reaped:
ptyProcess.kill()only tears down the pseudo-console host, not thepwsh/powershell/cmdtree (microsoft/node-pty#333). This change aligns every Windows teardown path on the same tree-kill the child_process fallback already used (taskkill /f /t), and adds a guarded reap on normal command completion so a ConPTY-lingering shell is cleaned up regardless of which path it leaked through. Each teardown also gains a fallback so ataskkillthat cannot launch (or exits non-zero) can neither crash the CLI via an unhandled'error'event nor leave a cancel hanging, and the normal-completion reap kills only the shell pid so a process the command intentionally detached (e.g.Start-Process) still outlives it.Why it's needed
On Windows 11 (ConPTY) every tool call left one idle
pwshbehind, accumulating until the machine ran out of memory (100% reproducible, v0.19.2). The PTY teardown used a bareptyProcess.kill()that does not tree-kill under ConPTY, while only the child_process fallback tree-killed correctly — so PTY-spawned shells piled up until OOM. Workaround until this ships:tools.shell.enableInteractiveShell: false(routes to the child_process path, which already tree-kills).Reviewer Test Plan
How to verify
Automated:
npx vitest run packages/core/src/services/shellExecutionService.test.ts— 107 tests pass, including new win32 coverage for every taskkill path: cancel tree-kills (with aptyProcess.killfallback and never a shell-only downgrade); normal completion reaps shell-pid-only (no/t); no reap once the shell already exited; non-win32 never taskkills; process-exit cleanup tree-kills and tears down the host (including when taskkill exits non-zero); and the child_process cancel falls back tochild.kill('SIGKILL')when taskkill cannot launch.Manual (Windows — I could not reproduce this on macOS, so it needs a Windows verifier): with default settings (interactive shell on), run ~20 tool calls and watch Task Manager — the
pwshcount should return to baseline instead of climbing. Start a long command (Start-Sleep 60) and cancel with Esc; itspwshtree should disappear with no orphan. Quit qwen while a command runs; confirm no leftoverpwsh. Sanity: a backgrounded command keeps running (the promote path is intentionally not reaped).Evidence (Before & After)
The user-visible signal is the Windows
pwshprocess count across a session: Before — climbs by one per tool call without bound (until OOM); After — returns to baseline. Capturing this needs a Windows host and is left for Windows verification; the decision logic is covered by the unit tests above. Non-Windows: N/A — all changes are win32-only branches and the POSIX paths are untouched.Tested on
macOS: unit tests pass. Windows: runtime ConPTY behavior needs a Windows verifier. Linux: not run locally (win32-only branches; non-win32 paths unchanged and exercised by the unit tests).
Environment (optional)
Unit tests only (
vitest); no local app runtime.Risk & Scope
isPtyActive(pid)(aprocess.kill(pid, 0)liveness check) reports the shell still alive, so a healthy node-pty that reaps correctly never triggers an extrataskkill. There is a tiny pid-reuse window during the ≤200ms output flush before dispose — the same risk class the existing child_processtaskkill-by-pid path already accepts.tools.shell.enableInteractiveShell.Linked Issues
Fixes #5873
中文说明
这个 PR 做了什么
Windows 上 qwen-code 默认走交互式 shell 的 PTY 路径（node-pty / ConPTY），每次工具调用起的 shell 进程树从未被回收：
ptyProcess.kill()在 ConPTY 下只拆伪终端宿主，杀不掉pwsh/powershell/cmd进程树（microsoft/node-pty#333）。本 PR 把 Windows 上所有 teardown 路径都对齐到 child_process 兜底路径早已使用的树杀（taskkill /f /t），并在命令正常完成时加一个有守卫的回收，使得无论从哪条路径泄漏的滞留 shell 都能被清理。每个 teardown 还都加了兜底：当taskkill起不来（或非 0 退出）时，既不会因未处理的'error'事件崩溃 CLI，也不会让取消操作卡死；而正常完成的回收只杀 shell 自身 pid，所以命令故意 detach 的进程（如Start-Process）仍能存活。为什么需要
Windows 11（ConPTY）下每次工具调用都残留一个 idle
pwsh，不断累积直到机器内存耗尽（100% 复现，v0.19.2）。PTY 的清理用的是裸ptyProcess.kill()，在 ConPTY 下不树杀；只有 child_process 兜底路径正确树杀——于是 PTY 起的 shell 一直堆积到 OOM。发布前的临时规避：tools.shell.enableInteractiveShell: false（回退到能正确树杀的 child_process 路径）。验证方式
自动化：
npx vitest run packages/core/src/services/shellExecutionService.test.ts—— 107 个测试通过，新增覆盖了全部 win32 taskkill 路径：取消树杀（带ptyProcess.kill兜底、且绝不降级为 shell-only）；正常完成只杀 shell pid（不带/t）；shell 已退出则不回收；非 win32 永不 taskkill；进程退出清理树杀并拆 host（含 taskkill 非 0 退出）；child_process 取消在 taskkill 起不来时回退child.kill('SIGKILL')。手工（Windows —— 我在 macOS 上无法复现，需 Windows 验证）：默认设置（交互式 shell 开）下连续跑约 20 次工具调用，看任务管理器
pwsh数应回落而非攀升；Start-Sleep 60后按 Esc 取消，其pwsh树应消失、无残留；命令运行时退出 qwen，确认无残留pwsh；同时确认后台化的命令仍存活（promote 路径有意不回收）。风险与范围
isPtyActive(pid)（process.kill(pid, 0)存活检查）报告 shell 仍存活时才触发，因此健康的 node-pty 永不会多跑一次taskkill。在 dispose 前 ≤200ms 的输出 flush 期间有一个极小的 pid 复用窗口——与既有 child_processtaskkill-by-pid 路径承担的是同一类风险。tools.shell.enableInteractiveShell。关联 Issue
Fixes #5873