Skip to content

fix(core): free an exited web terminal's PTY resources at exit time - #11572

Merged
yiliang114 merged 1 commit into
mainfrom
fix/issue-11353-exit-time-pty-release
Sep 11, 2026
Merged

fix(core): free an exited web terminal's PTY resources at exit time#11572
yiliang114 merged 1 commit into
mainfrom
fix/issue-11353-exit-time-pty-release

Conversation

@yiliang114

Copy link
Copy Markdown
Collaborator

What this PR does

Frees an exited web terminal's PTY resources at exit time instead of waiting for release(). WebTerminalRegistry.handleExit now releases the PTY through a new private releasePtyResources() helper, deferred one setImmediate. The session object and its scrollback buffer stay in the map, so replay is untouched. release() routes both of its arms through the same helper, guarded by a per-session ptyResourcesReleased flag so nothing is disposed twice when a tab close, a workspace drain, dispose() or the idle reclaim runs later on an already-freed session.

Why it's needed

releaseHost existed in exactly two places, web-terminal-registry.ts:474 and :482, both inside release(). handleExit (:264-273 on the base SHA) only set exited/exitCode and notified the exit listeners. Nothing else released the PTY on a natural exit: the browser route keeps an exited session alive for scrollback replay (finishExited leaves releaseAfterReplay at its false default), and the client will not trigger an earlier release either, because a live exit closes the socket with 4000 and 4000 is in NON_RETRYABLE_CLOSE_CODES. So an exited web terminal held node-pty's conout worker — and, once microsoft/node-pty#965 is fixed upstream, its conhost.exe — for up to IDLE_RECLAIM_MS, 15 minutes. Exited sessions are also excluded from the admission cap on purpose (filter((session) => !session.exited)), so a user opening and exiting terminals faster than they are reclaimed accumulates without bound inside that window. This is one feeder into the wider leak in #11303 (347 conhost.exe / ~2.8 GB after 12h).

Nothing needs the PTY once the shell is gone: write() and resize() already short-circuit on session.exited, and readSnapshot() replays the JS-side session.buffer, not the console.

Reviewer Test Plan

How to verify

This was previously written off as not reproducible off Windows. #11313 changed that: the web-terminal-registry.test.ts scaffolding it added exposes _agent._conoutSocketWorker.dispose plus the onData/onExit disposables through a fake PTY, which makes "was the PTY freed at exit?" directly assertable without a Windows machine. That is the reason this PR exists now rather than being parked, not a lowering of the bar — the release timing is observable on Linux even though the actual Windows resource reclamation is not.

Red on the base SHA (ac1edef97), with the three new tests added and no source change:

 FAIL  src/services/web-terminal-registry.test.ts > WebTerminalRegistry > frees an exited session at exit time, not at the idle reclaim
AssertionError: expected "spy" to be called once, but got 0 times
 ❯ src/services/web-terminal-registry.test.ts:428:27
    428|     expect(conoutDispose).toHaveBeenCalledOnce();

 FAIL  src/services/web-terminal-registry.test.ts > WebTerminalRegistry > still replays buffered scrollback after the exit-time release
AssertionError: expected "spy" to be called once, but got 0 times
 ❯ src/services/web-terminal-registry.test.ts:454:27

 Test Files  1 failed (1)
      Tests  2 failed | 30 passed (32)

Nothing in that test calls release() and no clock is advanced — real timers, a single setImmediate tick — so the 15-minute reclaim provably cannot have fired.

Green with the fix: Test Files 1 passed (1) / Tests 32 passed (32). The 29 pre-existing tests are unchanged and still pass; the test-file diff is purely additive (+77 / -0), and no existing assertion was relaxed. In particular expect(nativeKill/conoutDispose/disposeData/disposeExit).toHaveBeenCalledOnce() after onExitrelease and the deferred-arm counts still hold as written, which is what the ptyResourcesReleased flag is for.

The flag was mutation-checked rather than assumed: commenting out the if (session.ptyResourcesReleased) return; guard turns does not free an exited session twice when release follows red with expected "spy" to be called once, but got 2 times (1 failed | 31 passed), so that test has teeth. Worth noting the two pre-existing toHaveBeenCalledOnce() tests do not catch the double release — they assert synchronously after release(), before the deferred turn runs — which is exactly why the new test flushes the immediate first.

Commands:

cd packages/core
npx vitest run src/services/web-terminal-registry.test.ts   # 32 passed
npm run typecheck                                           # tsc --noEmit, 0 errors
npx prettier --check src/services/web-terminal-registry{,.test}.ts
npx eslint src/services/web-terminal-registry{,.test}.ts

No change to the live-release path's observable behaviour, and this is provable rather than merely asserted: release() now detaches the data/exit listeners after killPtyTree instead of before, but everything in between is synchronous (spawnSync, process.kill, pty.kill()), so no onData callback can interleave. killPtyTree still runs before releaseHost on the live arm, which the pre-existing does not double-close a live session whose kill already closed it test pins through its call counts.

Evidence (Before & After)

No user-visible surface changed — this is resource-release timing inside packages/core, with no pixel-level or TUI delta, so there are no screenshots. The evidence is the red/green test output above: before, conoutDispose is called 0 times after an exit; after, exactly once, with the session and its buffer still replayable. The third new test asserts scrollback replay still returns output: 'boot\r\n' plus exited: true, exitCode: 3 after the exit-time release, which is what keeps the route's releaseAfterReplay reconnect path working.

Tested on

OS Status
🍏 macOS ⚠️ not tested
🪟 Windows ⚠️ not tested — no Windows machine available; see Risk & Scope
🐧 Linux ✅ tested (mechanism level: unit tests with os.platform() mocked to win32)

Environment (optional)

Unit tests only, on a headless Linux host, node_modules reused via hardlink from an existing checkout. packages/core was built once to satisfy the vitest global-setup prerequisite guard.

Risk & Scope

Linked Issues

Fixes #11353

Related, non-closing: #11313 (merged — introduced releaseHost and the test scaffolding this builds on), #11352 (upstream microsoft/node-pty#965, status/blocked), #11303 (aggregate Windows PTY leak).

中文说明

这个 PR 做了什么

在 shell 退出时就释放已退出 web terminal 的 PTY 资源,而不是等到 release()WebTerminalRegistry.handleExit 现在通过新的私有辅助函数 releasePtyResources() 释放 PTY,并延后一个 setImmediate。session 对象及其 scrollback buffer 保留在 map 中,因此回放不受影响。release() 的两个分支都改为走同一个辅助函数,并用每会话的 ptyResourcesReleased 标志保护,使得之后的关闭标签页、workspace 排空、dispose() 或空闲回收在已释放的会话上不会二次 dispose。

为什么需要

releaseHost 此前只出现在两个地方,web-terminal-registry.ts:474:482,都在 release() 内部。handleExit(基线 SHA 上为 :264-273)只设置 exited/exitCode 并通知退出监听器。自然退出时没有其它任何路径会释放 PTY:浏览器路由为了 scrollback 回放刻意保留已退出的会话(finishExitedreleaseAfterReplay 保持默认的 false),客户端也不会更早触发释放,因为实时退出会以 4000 关闭连接,而 4000NON_RETRYABLE_CLOSE_CODES 里。于是已退出的 web terminal 会持有 node-pty 的 conout worker 最长 IDLE_RECLAIM_MS,即 15 分钟;在上游 microsoft/node-pty#965 修好之后,还会持有它的 conhost.exe。已退出会话又被有意排除在准入上限之外(filter((session) => !session.exited)),所以用户在这个窗口内开得比回收得快时,累积没有上限。这是 #11303(12 小时后 347 个 conhost.exe / 约 2.8 GB)那个更大泄漏的一个来源。

shell 退出后没有任何东西还需要 PTY:write()resize() 已经对 session.exited 短路,而 readSnapshot() 回放的是 JS 侧的 session.buffer,不是 console。

评审测试计划

如何验证

这一件此前被判定为"非 Windows 不可复现"。#11313 改变了这一点:它新增的 web-terminal-registry.test.ts 脚手架通过 fake PTY 暴露了 _agent._conoutSocketWorker.dispose 以及 onData/onExit 的 disposable,这让"退出时 PTY 是否被释放"在没有 Windows 机器的情况下也能直接断言。这才是本 PR 现在能提出来的原因,而不是放低了门槛——释放时机在 Linux 上可观测,尽管 Windows 上真实的资源回收并不可观测。

在基线 SHA(ac1edef97)上的红态:新增三个测试、不改任何源码时,

 FAIL  src/services/web-terminal-registry.test.ts > WebTerminalRegistry > frees an exited session at exit time, not at the idle reclaim
AssertionError: expected "spy" to be called once, but got 0 times
 ❯ src/services/web-terminal-registry.test.ts:428:27
    428|     expect(conoutDispose).toHaveBeenCalledOnce();

 FAIL  src/services/web-terminal-registry.test.ts > WebTerminalRegistry > still replays buffered scrollback after the exit-time release
AssertionError: expected "spy" to be called once, but got 0 times
 ❯ src/services/web-terminal-registry.test.ts:454:27

 Test Files  1 failed (1)
      Tests  2 failed | 30 passed (32)

该测试没有任何一处调用 release(),也没有推进任何时钟——使用真实定时器、只等一个 setImmediate——所以 15 分钟的空闲回收可证明不可能已经触发。

带上修复后的绿态:Test Files 1 passed (1) / Tests 32 passed (32)。29 个既有测试未作修改且仍然通过;测试文件的 diff 是纯新增(+77 / -0),没有放宽任何既有断言。特别是 onExitrelease 之后的 expect(nativeKill/conoutDispose/disposeData/disposeExit).toHaveBeenCalledOnce(),以及 deferred 分支的调用计数,都按原样成立——这正是 ptyResourcesReleased 标志的作用。

该标志做了变异验证而非想当然:把 if (session.ptyResourcesReleased) return; 这行守卫注释掉后,does not free an exited session twice when release follows 会以 expected "spy" to be called once, but got 2 times 变红(1 failed | 31 passed),说明这个测试是有牙齿的。值得一提的是,两个既有的 toHaveBeenCalledOnce() 测试并不能抓到这次重复释放——它们在 release() 之后同步断言,那时延后的那一轮还没跑——这恰恰是新测试要先 flush immediate 的原因。

命令:

cd packages/core
npx vitest run src/services/web-terminal-registry.test.ts   # 32 passed
npm run typecheck                                           # tsc --noEmit, 0 errors
npx prettier --check src/services/web-terminal-registry{,.test}.ts
npx eslint src/services/web-terminal-registry{,.test}.ts

实时释放路径的可观测行为没有变化,而且这一点是可证明的、不只是口头断言:release() 现在在 killPtyTree 之后才摘除 data/exit 监听器,而不是之前,但两者之间的所有操作都是同步的(spawnSyncprocess.killpty.kill()),因此没有 onData 回调能插入其间。实时分支上 killPtyTree 仍然先于 releaseHost,既有的 does not double-close a live session whose kill already closed it 测试通过调用计数钉住了这一点。

证据(Before & After)

没有用户可见的界面变化——这是 packages/core 内部的资源释放时机,没有像素级或 TUI 差异,因此没有截图。证据是上面的红/绿测试输出:修复前,退出之后 conoutDispose 被调用 0 次;修复后恰好 1 次,且会话与其 buffer 仍可回放。第三个新测试断言退出时释放之后,scrollback 回放仍返回 output: 'boot\r\n' 以及 exited: true, exitCode: 3,这正是保证路由 releaseAfterReplay 重连路径可用的部分。

测试环境

OS Status
🍏 macOS ⚠️ 未测试
🪟 Windows ⚠️ 未测试 —— 没有可用的 Windows 机器,见「风险与范围」
🐧 Linux ✅ 已测试(机制级:单元测试中将 os.platform() mock 成 win32

环境(可选)

仅单元测试,在一台 headless Linux 主机上,node_modules 通过硬链接复用已有 checkout。packages/core 构建过一次,用于满足 vitest global-setup 的前置检查。

风险与范围

关联 Issue

Fixes #11353

相关但不关闭:#11313(已合并——引入了本 PR 依赖的 releaseHost 与测试脚手架)、#11352(上游 microsoft/node-pty#965status/blocked)、#11303(Windows PTY 整体泄漏)。

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 #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 #11352's upstream close defect is untouched.

Fixes #11353

Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Patrol-Run: qwen-issue-patrol/jmtvhcstvvk
@github-actions github-actions Bot added the review/self-reported The linked issue was opened by the PR author (self-reported) label Sep 10, 2026
@qwen-code-ci-bot

qwen-code-ci-bot commented Sep 10, 2026

Copy link
Copy Markdown
Collaborator

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

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

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Thanks for the PR!

Template looks good ✓ — every required heading is present, and the Chinese mirror tracks the English section for section.

Problem: observed, not theoretical. I checked the base rather than taking the description's word for it: releaseHost really does appear at exactly two call sites, both inside release(), and handleExit touches no PTY resource at all. #11353 is open and is one feeder into #11303, which is a P1 field report from a different user with actual measurements (347 conhost.exe / ~2.8 GB after ~12h), so this is a leak someone hit, not a hypothesis. The unbounded-within-the-window part also holds — create() filters !session.exited out of the admission cap, so exited sessions accumulate with no ceiling until the reclaim.

Direction: aligned. Freeing a dead shell's PTY at exit instead of 15 minutes later is the obvious direction, and the reference CHANGELOG fixes this class repeatedly — orphaned --bg-pty-host processes spinning after the daemon dies, completed sessions not retiring because a backgrounded shell leaked, and a batch of long-session memory leaks. Long-running PTY resource retention is squarely something we care about.

Size: core paths (packages/core/src/services/**), so the two-tier gate applies. 79 production lines (61+/18−) in one file, 77 test lines (77+/0−), 0 generated/schema. Well under both the 500-line escalation and the 1000-line advisory, and it's a fix, not a refactor — no hard block. Tier 2's confidence bar is what this has to clear, and Stage 2 goes through it consumer by consumer.

Approach: the scope feels right, and I arrived at the same shape independently before reading the diff — tear the PTY-side resources down at exit, keep the session object and its buffer in the map for replay, share one helper with release(), and guard it with a per-session flag. It reuses releaseHost from #11313 rather than inventing a second mechanism, which is the right call. The flag isn't decoration either: without it the later release() (tab close, drain, dispose(), reclaim) would dispose the same subscriptions and the same worker a second time. The test diff is purely additive and no existing assertion was relaxed; the killPtyTree-ordering comment moved because the code it describes moved, which is not churn.

Two things I'd flag rather than block on. First, release() now detaches the data/exit listeners after killPtyTree instead of before. That is safe — everything in between is synchronous (spawnSync, process.kill, pty.kill()), so no onData can interleave, and exitListeners is still cleared before the kill in both orderings, so nothing gets a duplicate exit notification — but it is a real reordering and worth a second pair of eyes. Second, the deferral means post-exit trailing output has exactly one event-loop turn to reach buffer; you've disclosed that honestly in Risk & Scope and it's a strictly narrower loss than the zero-delay dispose shellExecutionService already accepts, so I'm treating it as a residual, not a defect.

Risk: no Stage 1e high-risk path match. The elevated part is platform coverage, not code shape: the effect is Windows-only and Test (windows-latest, Node 22.x) is skipped on this commit, so the pipeline cannot observe the actual reclamation. What is observable on Linux — the release timing and its exactly-once property — is what the new tests pin, and that's a fair division.

Moving on to code review. 🔍

中文说明

感谢贡献!

模板完整 ✓ —— 所有必需标题都在,中文部分与英文逐段对应。

问题: 是已观测到的问题,不是理论性加固。我没有只信 PR 描述,而是核对了 base:releaseHost 确实只有两个调用点,且都在 release() 内部,handleExit 完全不碰 PTY 资源。#11353 处于 open 状态,是 #11303 的一个来源;而 #11303 是另一位用户提交的 P1 现场报告,带有实测数据(约 12 小时后 347 个 conhost.exe / ~2.8 GB),所以这是有人真实踩到的泄漏,不是假设。"窗口内无上限"这一点也成立——create() 的准入上限过滤掉了 !session.exited,因此已退出的 session 在回收前会无上限累积。

方向: 对齐。把已死 shell 的 PTY 在退出时就释放、而不是等 15 分钟,是显而易见的方向;参考产品的 CHANGELOG 也反复修这一类问题——守护进程退出后空转的 --bg-pty-host 孤儿进程、因后台 shell 泄漏而无法退役的已完成会话,以及一批长会话内存泄漏。长时间持有 PTY 资源确实是我们关心的范围。

规模: 触及核心路径(packages/core/src/services/**),因此适用两级门禁。生产代码 79 行(61+/18−,单文件),测试 77 行(77+/0−),生成/schema 0 行。远低于 500 行升级线和 1000 行建议线;类型是 fix 而非 refactor,不触发硬性拦截。需要过的是 Tier 2 的信心门槛,Stage 2 会逐个下游消费者核对。

方案: 范围合理。我在看 diff 之前独立想到的也是同一个形状——退出时拆掉 PTY 侧资源、把 session 对象和 buffer 留在 map 里供回放、与 release() 共用一个辅助函数、并用每会话标志保护。它复用了 #11313releaseHost 而不是另造一套机制,这是对的。那个标志也不是摆设:没有它,之后的 release()(关闭标签页、workspace 排空、dispose()、空闲回收)会把同一批订阅和同一个 worker 再释放一次。测试 diff 是纯新增,没有放松任何既有断言;killPtyTree 顺序那段注释之所以移动,是因为它描述的代码移动了,不算无关改动。

有两点我想指出但不作为阻塞。第一,release() 现在在 killPtyTree 之后才摘除 data/exit 监听器,而之前是在之前。这是安全的——中间全是同步调用(spawnSyncprocess.killpty.kill()),没有 onData 能插进来;而且两种顺序下 exitListeners 都仍在 kill 之前清空,所以不会有重复的退出通知——但这确实是一次真实的顺序调整,值得再看一眼。第二,延后一轮意味着退出后的尾部输出只有一个事件循环轮次能进入 buffer;你在"风险与范围"里已经诚实说明,而且这个损失范围严格小于 shellExecutionService 目前已接受的零延迟摘除,所以我把它当作残留问题而不是缺陷。

风险: Stage 1e 的高风险路径没有命中。需要关注的不是代码形态而是平台覆盖:效果仅限 Windows,而该 commit 上 Test (windows-latest, Node 22.x)skipped,所以流水线无法观测真实的资源回收。Linux 上可观测的部分——释放时机及其"恰好一次"属性——正是新测试所钉住的,这个划分是合理的。

进入代码审查 🔍

Qwen Code · qwen3.8-max-2026-09-02

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

@qwen-code-ci-bot

qwen-code-ci-bot commented Sep 10, 2026

Copy link
Copy Markdown
Collaborator

Code review

I wrote my own proposal before opening the diff — free the PTY-side resources in handleExit, keep the session and its buffer in the map for replay, share one helper with release(), guard it with a per-session flag, and defer off the exit callback. The PR landed on the same shape, so I have no simpler alternative to argue for. What follows is me trying to break it.

No blockers found. The things I went looking for, and what I found:

Use-after-release of session.pty. The exit-time release leaves a live map entry holding a PTY whose listeners are detached and whose host is released, for up to 15 minutes. Every reader of that entry is safe: write() and resize() both short-circuit on session.exited before touching session.pty, readSnapshot() reads buffer only, and release()'s killPtyTree is behind if (!session.exited). There is no path that reaches a released PTY.

Exactly-once, across all six ways releasePtyResources can be reached. Tab close (control: release), workspace drain (releaseWorkspace), dispose()'s loop, the idle reclaim timer, the reconnect-after-replay arm (finishExited(…, true)), and the deferred exit-time call itself. The flag is set before any disposal happens, so every ordering collapses to one release. The interleaving worth spelling out: exit → setImmediate(A) queued → release() runs first in the same tick → flag set, worker disposed → A fires and returns immediately. One conoutDispose, not two.

The deferred callback captures the session object, not the terminalId. That is the right choice and I want to name it, because the obvious alternative is a bug: setImmediate(() => this.releasePtyResources(this.sessions.get(terminalId)!)) would let a stale callback tear down a fresh PTY after the id was reused. Capturing session makes that unreachable, and the flag makes it unreachable a second time over.

The reordering in release(). Detaching the data/exit listeners moved from before killPtyTree to after it. I checked rather than assumed: everything in between is synchronous — spawnSync, process.kill, the pty.kill() wrapper — and spawnSync blocks the loop without dispatching pending I/O callbacks to JS, so no onData can land in the gap. outputListeners and exitListeners are still cleared before killPtyTree in both orderings, so even in the (practically unreachable) case where pty.kill() fired onExit synchronously, no listener would see a second notification, and the resulting setImmediate would hit the flag. The live arm still runs killPtyTree → wrapper kill()noteConPtyHostReleasedreleaseConPtyHost's releasedHosts early return, so the no-double-close invariant from #11313 is intact.

Throw safety, now that the call sits in a setImmediate. A throw inside a setImmediate callback is an uncaught exception, which is worse than the same throw propagating out of release(). Both ConPTY helpers wrap their only external calls — _ptyNative.kill and _conoutSocketWorker.dispose() — in try/catch, and the existing still completes a deferred release when the conout worker dispose throws test pins that, so nothing reachable can escape. The two unguarded calls are node-pty's own subscription dispose(), which just splices a listener array. Fine as written; worth remembering if anything throwing is ever added to the helper.

The exited-and-never-ready case. _isReady === false plus a natural exit routes to disposeConoutWorker, which frees the worker but leaves the HPCON to a queued kill() that will never run. That is unchanged from base — releaseHost has always branched on _isReady alone, never on session.exited — and it's #11352's upstream defect, not this PR's. Calling it out only so nobody reads this diff as fixing it.

Downstream consumers, named. releaseHost has exactly two call sites repo-wide, both in this file, and both now go through the helper. ptyResourcesReleased is new, private, and read in one place. releasePtyResources is private, so no public signature changed. packages/cli/src/serve/routes/terminal.ts needs no edit and I confirmed why by reading it: the live-exit arm calls finishExited(code) with releaseAfterReplay defaulting to false, so it never released in the first place, and the reconnect arm reads the snapshot and sends the replay before calling registry.release(). Nothing outside packages/core is touched.

sequenceDiagram
    participant P1 as node-pty (shell exit)
    participant P2 as handleExit
    participant P3 as releasePtyResources
    participant P4 as session map and buffer
    participant P5 as route terminal.ts
    P1->>P2: onExit (exitCode)
    P2->>P4: mark exited, notify listeners
    P2->>P3: setImmediate (one turn later)
    P3->>P3: set flag, detach listeners, releaseHost
    P3-->>P4: entry and buffer kept for replay
    P5->>P4: reconnect, readSnapshot (replay)
    P5->>P3: release after replay
    P3-->>P5: flag already set, no second dispose
Loading

On the tests. The three new ones have teeth, and I checked the assertions rather than trusting the counts. expect(kill).not.toHaveBeenCalled() is the load-bearing one: killPtyTree calls pty.kill() unconditionally at the end, so a clean kill assertion is positive proof the exited arm was taken and no pid was signalled. Using real timers with a single setImmediate genuinely rules out the 15-minute reclaim, which is the thing the test name claims. And the second test asserting readSnapshot still returns output: 'boot\r\n' with exited: true, exitCode: 3 is what pins the replay contract the route depends on. The diff is +77/−0 — purely additive, no existing assertion relaxed — and none of the pre-existing exit tests interfere, because a stale deferred callback from an earlier test only ever touches that test's own captured mock instances.

Test evidence

This was an unattended CI run, so I did not build, run, or execute any PR-derived code — the evidence below is the PR's own CI read through the API for commit 88718ce9a6ad57ba3099a8a3f0c9721c92a5c206, plus static review. Nothing here is the author's self-reported result presented as mine; where I reference the red/green output in the description, that is the author's claim.

70 check-runs on the head SHA. No failures, so there is no log excerpt to quote. The Linux unit suite, lint/static, and integration tests were still in progress at review time — I did not poll for them, so their outcome is unknown to me and the table says so. Two rows matter for judging this PR: Test (windows-latest, Node 22.x) and Test (macos-latest, Node 22.x) are both skipped, which confirms the description's claim that Windows CI does not run on PRs. Desktop Shell (windows-2022) did pass, but that job builds the desktop shell — it does not run the core unit suite, so it is not Windows coverage for this change.

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

Check Conclusion
Classify PR ✅ success
Desktop Shell (ubuntu-22.04) ✅ success
Desktop Shell (windows-2022) ✅ success
Integration Tests (no-AK, No Sandbox) ✅ success
Lint & Static (ubuntu-latest, Node 22.x) ✅ success
Test (ubuntu-latest, Node 22.x) ✅ success
web-shell E2E Smoke (ubuntu-latest, Node 22.x) ✅ success

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

What the suite can and cannot settle: the Linux tests mock os.platform() to win32 while process.platform stays linux, so they exercise the release timing and the exactly-once property through conpty-host's Windows branch, but no real ConPTY, conout worker, or conhost.exe is involved. The actual reclamation on Windows is not verified — by me, by this pipeline, or by the author, who says so plainly in Risk & Scope.

Sandboxed verification would settle the one claim that is currently reasoning rather than measurement: @qwen-code /verify — whether a single setImmediate is a sufficient drain, i.e. whether ConPTY output that node-pty delivers after onExit still reaches session.buffer. The unit suite structurally cannot express this, and the description is candid about why: the fake's dispose is a bare vi.fn() that never detaches the listener, so handleData keeps appending after "dispose" and an assertion there would pass with or without the fix. A harness that makes dispose actually detach, A/B'd against the base build, would turn the trailing-output question into a number instead of an argument from the shellExecutionService precedent. /tmux does not apply — there is no TUI surface here, it is the serve-route web terminal on Windows.

Real-scenario tmux testing: N/A — unattended CI run, and no user-visible surface changed.

中文说明

代码审查

我在打开 diff 之前先写了自己的方案——在 handleExit 里释放 PTY 侧资源、把 session 和它的 buffer 留在 map 里供回放、与 release() 共用一个辅助函数、用每会话标志保护、并从退出回调里延后一轮。这个 PR 落在了同一个形状上,所以我没有更简的替代方案要争。下面是我试图把它打破的过程。

未发现阻塞问题。 我特意去找的几处,以及结果:

释放后仍被使用的 session.pty 退出时释放会留下一个仍在 map 里的条目,其监听器已摘除、host 已释放,最长持续 15 分钟。这个条目的每个读取方都是安全的:write()resize() 都在碰到 session.pty 之前就以 session.exited 短路;readSnapshot() 只读 bufferrelease()killPtyTreeif (!session.exited) 之后。没有任何路径能触到已释放的 PTY。

"恰好一次",覆盖 releasePtyResources 全部六种到达方式。 关闭标签页(control: release)、workspace 排空(releaseWorkspace)、dispose() 循环、空闲回收定时器、回放后重连分支(finishExited(…, true)),以及延后的退出时调用本身。标志在任何释放动作之前就置位,所以所有交错顺序都收敛为一次释放。值得写出来的交错是:退出 → 排入 setImmediate(A) → 同一轮里 release() 先跑 → 标志置位、worker 释放 → A 触发后立即返回。conoutDispose 一次,不是两次。

延后回调捕获的是 session 对象,不是 terminalId 这是正确的选择,我想专门点出来,因为最直觉的写法是个 bug:setImmediate(() => this.releasePtyResources(this.sessions.get(terminalId)!)) 会让一个陈旧回调在 id 被复用后拆掉一个全新的 PTY。捕获 session 让这条路不可达,标志又挡了第二次。

release() 里的顺序调整。 摘除 data/exit 监听从 killPtyTree 之前挪到了之后。我是核对过的,不是假定:中间全是同步调用——spawnSyncprocess.killpty.kill() 包装——而 spawnSync 会阻塞事件循环且不把待处理 I/O 回调派发给 JS,所以没有 onData 能落进这个空隙。两种顺序下 outputListenersexitListeners 都仍在 killPtyTree 之前清空,因此即便在(实际上到不了的)pty.kill() 同步触发 onExit 的情况下,也不会有监听器收到第二次通知,而由此产生的 setImmediate 会撞上标志。存活分支仍然是 killPtyTree → 包装 kill()noteConPtyHostReleasedreleaseConPtyHostreleasedHosts 提前返回,所以 #11313 的不重复关闭不变量完好。

抛错安全性,现在这个调用位于 setImmediate 里。 setImmediate 回调里抛错是未捕获异常,比同样的错误从 release() 里传播出来更糟。两个 ConPTY 辅助函数都把它们唯一的外部调用——_ptyNative.kill_conoutSocketWorker.dispose()——包在 try/catch 里,既有的 still completes a deferred release when the conout worker dispose throws 测试钉住了这一点,所以没有可达的抛错能逃出来。两个未加保护的调用是 node-pty 自己的订阅 dispose(),它只是从数组里删一个监听器。按现在的写法没问题;如果以后往这个辅助函数里加会抛错的东西,需要记住这一点。

"已退出且从未 ready"的情况。 _isReady === false 加上自然退出会走 disposeConoutWorker,它释放 worker 但把 HPCON 留给一个永远不会执行的排队 kill()。这与 base 相同——releaseHost 一直只看 _isReady,从不看 session.exited——而且那是 #11352 的上游缺陷,不属于本 PR。写出来只是为了避免有人把这个 diff 读成修好了它。

下游消费者,逐一点名。 releaseHost 全仓库恰好两个调用点,都在这个文件里,现在都经过辅助函数。ptyResourcesReleased 是新增的、私有的、只在一处被读。releasePtyResources 是私有的,所以没有公开签名变化。packages/cli/src/serve/routes/terminal.ts 无需修改,我读了代码确认了原因:存活退出分支调用 finishExited(code)releaseAfterReplay 取默认 false,所以它本来就不释放;重连分支在调用 registry.release() 之前先读快照并发送回放。packages/core 之外没有任何改动。

(时序图见上方,中文不重复。)

关于测试。 三个新测试是有牙的,我核对的是断言本身而不是数字。expect(kill).not.toHaveBeenCalled() 是承重的一个:killPtyTree 末尾无条件调用 pty.kill(),所以 kill 断言干净就是"走了已退出分支、没有对 pid 发信号"的正面证明。用真实定时器加单个 setImmediate 确实排除了 15 分钟回收,也就是测试名所声称的那件事。第二个测试断言 readSnapshot 仍返回 output: 'boot\r\n'exited: true, exitCode: 3,钉住的正是路由依赖的回放契约。diff 是 +77/−0——纯新增,没有放松既有断言——而且既有的退出相关测试不会互相干扰,因为前一个测试遗留的陈旧延后回调只会碰到它自己捕获的 mock 实例。

测试证据

这是一次无人值守的 CI 运行,所以我没有构建、运行或执行任何来自 PR 的代码——下面的证据是通过 API 读取的、commit 88718ce9a6ad57ba3099a8a3f0c9721c92a5c206 上 PR 自己的 CI,加上静态审查。这里没有把作者自报的结果当作我的证据;凡引用描述里的红/绿输出之处,都明确标注为作者的说法。

head SHA 上共 70 个 check-run。没有失败,因此没有日志片段可引。Linux 单元测试、lint/静态检查、集成测试在审查时仍在进行中——我没有轮询等待,所以它们的结果我并不知道,表格里如实标注。有两行对判断这个 PR 很重要:Test (windows-latest, Node 22.x)Test (macos-latest, Node 22.x) 都是 skipped,这印证了描述中"PR 上不跑 Windows CI"的说法。Desktop Shell (windows-2022) 确实通过了,但那个 job 构建的是桌面外壳,不跑 core 单元测试套件,所以它不构成本改动的 Windows 覆盖。

(CI 表格见上方机器可读区域,中文不重复。)

套件能与不能证明的:Linux 测试把 os.platform() mock 成 win32,而 process.platform 仍是 linux,所以它们经由 conpty-host 的 Windows 分支检验了释放时机和"恰好一次"属性,但其中没有任何真实的 ConPTY、conout worker 或 conhost.exe。Windows 上的实际资源回收没有被验证——我、这条流水线、以及作者本人都没有,作者在"风险与范围"里也直说了。

沙箱验证可以定下目前仍属推理而非实测的那一条:@qwen-code /verify —— 单个 setImmediate 是否足够完成排空,也就是 node-pty 在 onExit 之后投递的 ConPTY 输出是否仍能进入 session.buffer。单元测试在结构上无法表达这一点,描述也坦率说明了原因:fake 的 dispose 是个裸 vi.fn(),从不真正摘除监听器,所以 handleData 在"dispose"之后仍会继续追加,这样的断言无论有没有这个修复都会通过。一个让 dispose 真正摘除监听器的 harness,与 base 构建做 A/B,可以把尾部输出问题从"援引 shellExecutionService 先例的论证"变成一个数字。/tmux 不适用——这里没有 TUI 界面,它是 Windows 上的 serve 路由 web terminal。

真实场景 tmux 测试:N/A —— 无人值守 CI 运行,且没有用户可见界面变化。

Qwen Code · qwen3.8-max-2026-09-02

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

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Confidence: 4/5 — the mechanism is verified and the design matches what I'd have written; the one point I'm withholding is that nobody can measure the actual Windows reclamation from here, and that gap is real even though it isn't this PR's fault.

Stepping back. My independent proposal and this diff converged on the same shape, which is usually a sign the problem was framed correctly rather than that I anchored on the description — I wrote mine from the title and the "Why it's needed" section before opening the diff. The change is smaller than the analysis behind it, which is the right ratio: one helper, one flag, one deferred call, and a comment that moved because the code it describes moved. release() comes out the other side simpler than it went in, because two arms that each called releaseHost for different reasons now share one guarded path. In six months I'd thank whoever wrote this, not curse them.

The question I had to settle before approving was whether I'm comfortable signing off on a fix whose effect I cannot observe. I am, because the claim decomposes cleanly into two parts and only one of them is unobservable. Whether releaseHost actually frees the conout worker on real Windows was established by #11313, which is merged — this PR does not re-litigate it and does not depend on re-proving it. What this PR changes is when that call happens, and that is fully observable on Linux through the mocked-platform tests: the worker dispose now fires one event-loop turn after onExit instead of up to fifteen minutes later, exactly once, with the session and its buffer still replayable. So the unverifiable half is inherited and already landed; the new half is pinned. That's a different situation from approving a Windows-only behaviour change wholesale.

Two residuals, neither blocking, both worth having on the record:

The first is the trailing-output drain, which Stage 2 goes into. The author disclosed it, it is strictly narrower than the zero-delay dispose shellExecutionService already ships with, and the mitigation used here is literally the one that file's comment names as the thing that "could recover them". I'd rather have this than the status quo.

The second is about what Fixes #11353 will close. The issue's framing is "unbounded inside that window", and this PR bounds the native half — the conout worker, and eventually conhost.exe. The JS-side half stays: an exited session still holds up to 4 MB of scrollback buffer in the map for the full IDLE_RECLAIM_MS, and exited sessions still bypass the admission cap, so the JS memory accumulation inside the window is still unbounded. That is deliberate and it is what the issue itself prescribes ("keeping the session and its buffer intact for scrollback"), because giving it up means giving up replay — so I'm not asking for it here. But if #11353 closes on this, the buffer half loses its tracker, and it's the half that a user opening and exiting many terminals would actually feel in RSS. Worth a follow-up issue, or a line in the close comment saying which half landed.

On pattern: this is the third PR in a coherent series on one real leak (#11313 merged, #11352 open and blocked upstream on microsoft/node-pty#965, this one), each narrowly scoped and cross-referenced, and this one was deliberately split out of a review so it could stay small. That's the opposite of volume-farming, and I evaluated it on its merits without the series counting for or against it.

Verdict: approve — deferred. The PR's own CI was still running on this commit at review time (unit suite, lint/static, and integration tests all in progress), and I'm not going to attest to a result that doesn't exist yet. Approval is deferred until CI lands green on 88718ce9a6ad57ba3099a8a3f0c9721c92a5c206; the finalize job posts the commit-pinned approval once every check on that SHA completes green, and withholds it if anything lands red or the head moves. No approval is posted in this run.

中文说明

信心:4/5 —— 机制已核实,设计与我自己会写的方案一致;扣掉的一分是因为在这里没人能实测 Windows 上的真实资源回收,这个缺口是真实存在的,尽管它不是本 PR 的过错。

退一步看。我独立提出的方案与这个 diff 收敛到了同一个形状,这通常说明问题本身被正确地界定了,而不是说明我被描述带偏了——我的方案是在打开 diff 之前,仅凭标题和"为什么需要"一节写下的。这个改动比它背后的分析要小,而这是正确的比例:一个辅助函数、一个标志、一次延后调用,外加一段因为它所描述的代码移动而移动的注释。release() 出来时比进去时更简单,因为原本两个各自以不同理由调用 releaseHost 的分支,现在合并成一条受标志保护的路径。六个月后我会感谢写这段代码的人,而不是骂他。

在批准之前我必须想清楚的一个问题是:对一个我无法观测其效果的修复签字,我是否安心。我安心,因为这个主张可以干净地拆成两部分,而只有其中一部分不可观测。releaseHost 在真实 Windows 上是否确实释放 conout worker,这一点由已合并的 #11313 确立——本 PR 不重新论证它,也不依赖重新证明它。本 PR 改变的是这个调用何时发生,而这在 Linux 上通过 mock 平台的测试是完全可观测的:worker 释放现在在 onExit 之后一个事件循环轮次触发,而不是最长 15 分钟之后,恰好一次,且 session 与其 buffer 仍可回放。所以不可验证的那一半是继承来的、且已经落地;新增的那一半被测试钉住了。这与"整体批准一个仅限 Windows 的行为改动"是不同的处境。

两点残留,都不阻塞,但都值得记录在案:

第一是尾部输出排空,Stage 2 已经展开。作者主动披露了它;它的范围严格小于 shellExecutionService 目前已上线的零延迟摘除;而这里采用的缓解手段,正是那个文件的注释里点名的"可以恢复它们"的做法。相比现状,我宁愿要现在这个。

第二关系到 Fixes #11353 会关掉什么。该 issue 的表述是"窗口内无上限",而本 PR 限定的是原生那一半——conout worker,以及最终的 conhost.exe。JS 那一半留着:已退出的 session 仍会在整个 IDLE_RECLAIM_MS 期间在 map 里持有最多 4 MB 的 scrollback buffer,而且已退出的 session 仍然绕过准入上限,所以窗口内的 JS 内存累积依然无上限。这是刻意的,也正是 issue 自己规定的做法("keeping the session and its buffer intact for scrollback"),因为放弃它就意味着放弃回放——所以我不是在这里要求处理它。但如果 #11353 因本 PR 关闭,buffer 那一半就失去了跟踪条目,而反复开关终端的用户真正在 RSS 上感受到的恰恰是那一半。值得开一个后续 issue,或者在关闭说明里写清楚落地的是哪一半。

关于"是否成串":这是围绕同一个真实泄漏的第三个 PR(#11313 已合并,#11352 处于 open 且上游阻塞于 microsoft/node-pty#965,以及本 PR),每个都范围收窄并相互引用,而这一个是从一次 review 里特意拆出来的,为的是保持小。这与刷量恰恰相反;我按它本身的价值评估,这一系列既不加分也不减分。

结论:批准——延后。 审查时该 commit 上 PR 自己的 CI 仍在运行(单元测试套件、lint/静态检查、集成测试均在进行中),而我不打算为一个尚不存在的结果背书。批准延后至 CI 在 88718ce9a6ad57ba3099a8a3f0c9721c92a5c206 上全绿;当该 SHA 上每个检查都绿完成后,finalize 任务会发出绑定该 commit 的批准,若有检查变红或 head 移动则不予批准。本次运行不发出批准。

Qwen Code · qwen3.8-max-2026-09-02

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

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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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

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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Partially reviewed — gaps disclosed. Suggestions are inline.

Not reviewed: build-and-test — Test (windows-latest, Node 22.x) was skipped in CI (gated to merge_group/schedule/workflow_dispatch, never pull_request) and its suite could not run locally: this change's releaseHost path is win32-only (conpty-host.ts returns early off win32) and the review ran on Linux.

Test Plan (not a blocker): src/services/web-terminal-registry.test.tsno such file or directory; src/services/web-terminal-registry.test.ts:428:27no such file or directory; src/services/web-terminal-registry.test.ts:454:27no such file or directory; 30 passed — this review observed 24916, 2013, 30339, 300, 1842, 519, 7221 passed; 31 passed — this review observed 24916, 2013, 30339, 300, 1842, 519, 7221 passed; and 1 more.

中文说明

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

未审查(原文为英文):build-and-test — Test (windows-latest, Node 22.x) was skipped in CI (gated to merge_group/schedule/workflow_dispatch, never pull_request) and its suite could not run locally: this change's releaseHost path is win32-only (conpty-host.ts returns early off win32) and the review ran on Linux.

Test Plan(非阻断):src/services/web-terminal-registry.test.tsno such file or directory; src/services/web-terminal-registry.test.ts:428:27no such file or directory; src/services/web-terminal-registry.test.ts:454:27no such file or directory; 30 passed — this review observed 24916, 2013, 30339, 300, 1842, 519, 7221 passed; 31 passed — this review observed 24916, 2013, 30339, 300, 1842, 519, 7221 passed; and 1 more。

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

Comment on lines +295 to +296
// synchronous, so one turn is enough — there is no chain to flush.
setImmediate(() => this.releasePtyResources(session));

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Suggestion] Nothing in the suite distinguishes this deferred release from an inline one, so the one-turn deferral that the six-line comment above it exists to justify is unenforced. Replacing this line with a direct this.releasePtyResources(session); leaves all 32 tests passing, which means a later simplification pass can inline the async hop — the shorter, more obvious code — and silently drop the trailing-output protection that comment argues for, with the suite, the typecheck and lint all staying green. No other suite can catch it either: web-terminal-registry.test.ts is the only test that exercises the real registry, because packages/cli/src/serve/routes/terminal.test.ts:51-60 builds a stub (} as unknown as WebTerminalRegistry;).

Witness:

INTACT (setImmediate(() => this.releasePtyResources(session));) : Tests 32 passed (32)
MUTANT (this.releasePtyResources(session);)                     : Tests 32 passed (32)

flip-check, against a harness whose disposable really detaches:
  MUTANT: x  late data still reaches readSnapshot()
           - "output": "boot\r\nLATE-TRAILING\r\n"
           + "output": "boot\r\n"
  INTACT: 2 passed   (tree restored, diff -q-identical to 88718ce9)

The fix is a test that emits data after onExit and asserts it still reaches readSnapshot(...).output. It only pins anything if the harness's data disposable actually detaches — today disposeData is a bare vi.fn() (web-terminal-registry.test.ts:53, :72), so onData(...) still reaches handleData after "disposal" and the assertion passes on both the intact and the inlined arm:

onData: vi.fn((listener) => {
  onData = listener;
  return {
    dispose: () => {
      onData = () => {};
      disposeData();
    },
  };
}),

Then assert on the buffered output rather than on a spy call count.

That new case must go red when setImmediate(() => this.releasePtyResources(session)); is replaced by a direct call — please confirm the mutation when you add it, because with the shipped non-detaching harness the same assertion passes on both arms and pins nothing.

中文说明

套件里没有任何测试能区分「延后一轮释放」与「同步立即释放」,因此上方那段六行注释所要论证的延后本身并没有被钉住。把这一行换成直接调用 this.releasePtyResources(session);,32 个测试依然全部通过;这意味着后来一次「简化」改动完全可以把这个异步跳转内联掉(那是更短、更直观的写法),并悄无声息地移除该注释所论证的尾部输出保护,而测试套件、typecheck 与 lint 全绿。也没有别的套件能抓到它:web-terminal-registry.test.ts 是唯一真正驱动 registry 的测试,因为 packages/cli/src/serve/routes/terminal.test.ts:51-60 构造的是桩对象(} as unknown as WebTerminalRegistry;)。

证据(本次评审实测):

原始代码 (setImmediate(() => this.releasePtyResources(session));) : Tests 32 passed (32)
变异体   (this.releasePtyResources(session);)                     : Tests 32 passed (32)

翻转校验,使用真正会摘除监听器的脚手架:
  变异体: x  退出后投递的数据仍能进入 readSnapshot()
           - "output": "boot\r\nLATE-TRAILING\r\n"
           + "output": "boot\r\n"
  原始代码: 2 passed   (文件已还原,diff -q 与 88718ce9 一致)

修复方式是补一个测试:在 onExit 之后投递数据,并断言它仍然出现在 readSnapshot(...).output 里。但只有在测试脚手架的 data disposable 真正摘除监听器时它才有约束力——目前 disposeData 只是一个裸 vi.fn()web-terminal-registry.test.ts:53:72),所以「dispose」之后 onData(...) 仍会进入 handleData,该断言在原始代码和内联变异体上都会通过。请让脚手架建模真实的摘除行为,然后断言缓冲区里的输出,而不是断言 spy 的调用次数。

新测试必须在把 setImmediate(() => this.releasePtyResources(session)); 换成直接调用时变红——添加时请顺手验证这个变异,因为在现有不摘除监听器的脚手架下,同一断言两边都通过,什么也钉不住。

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

Comment on lines +535 to +537
* Called from `handleExit` (deferred one turn, so an exited web terminal
* stops holding the worker for the whole idle-reclaim window — #11353) and
* from `release()` on both of its arms, where the flag keeps a release that

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Suggestion] Collapsing release()'s two arms into one unconditional helper call deletes the else arm that the releaseHost call-site inventory in packages/core/src/services/conpty-host.ts still names as "the primary web-terminal path for #11303" — and that inventory is exactly the checklist conpty-host.ts:19-20 tells a node-pty bump to re-walk ("A bump therefore has to be re-checked against src/win/conpty.cc, not only against the JS shape"). The next maintainer bumping @lydell/node-pty walks that list, looks for release()'s else arm in this file, cannot find it, and then either treats the paragraph as describing dead code or never evaluates the site that now handles every natural exit — the one caller that reaches releaseHost's _isReady === false branch with no queued kill() behind it. So a native-semantics change on the post-onExit path, such as the std::erase_if baton change conpty-host.ts:18-21 already flags, ships unreviewed for the web terminal's main path.

Witness:

sweep: 7 sites across 3 files describe release()'s arm shape; 2 outright falsified
oracle: post-diff release() read at web-terminal-registry.ts:488-499
          if (!session.exited) { ... killPtyTree(session.pty); }
          this.releasePtyResources(session);   <- unconditional, no else
        vs the base if/else at :456-484
  conpty-host.ts:135                 "`else` arm"                              deleted construct
  web-terminal-registry.test.ts:367  "the whole point of the else branch"      deleted construct
  conpty-host.ts:136                 "the primary web-terminal path for #11303" caller mislabelled
  web-terminal-registry.test.ts:321  "release() calls ...releaseHost?.() bare"  now indirect
  web-terminal-registry.ts:332-334   "reaches it from BOTH arms"                vocabulary drift
  conpty-host.ts:144                 "from the live arm AND the exited arm"     vocabulary drift
  web-terminal-registry.ts:537       "from release() on both of its arms"       drift (this line)

Please retarget these in the same change. In conpty-host.ts:134-136 and :142-146, replace "release()'s else arm" / "the exited arm" with the exit-time path (handleExit → deferred releasePtyResourcesreleaseHost), keeping release() listed as the secondary caller. At web-terminal-registry.ts:332-334, say releaseHost is reached from releasePtyResources, which handleExit (deferred) and release() both call under the ptyResourcesReleased flag. And drop the release()-arm references at web-terminal-registry.test.ts:355 and :367. This JSDoc already names handleExit as a caller, so only its "both of its arms" phrasing needs the same touch.

One constraint on the rewrite: the new exit-time site must stay in conpty-host.ts's native no-op group rather than its close group, per conpty-host.ts:128-131 — "In src/win/conpty.cc the native exit-watcher thread erases the pty baton before it delivers the JS onExit, and PtyKill skips ClosePseudoConsole when get_pty_baton returns null" — and the exit-time release runs strictly after onExit. The rewritten enumeration must therefore keep conpty-host.ts:147-148 ("The inbox conhost half of #11303 is therefore not fixed by this function on the natural-exit path") true of it.

中文说明

release() 的两个分支收敛成一次无条件的辅助函数调用,删掉了那个 else 分支;而 packages/core/src/services/conpty-host.ts 里的 releaseHost 调用点清单至今仍把它称作「#11303 的 web-terminal 主路径」——而那份清单正是 conpty-host.ts:19-20 要求升级 node-pty 时必须重新走一遍的检查表(「A bump therefore has to be re-checked against src/win/conpty.cc, not only against the JS shape」)。下一位升级 @lydell/node-pty 的维护者照着那份清单来找本文件里 release()else 分支,找不到,于是要么把那段话当成在描述已死的代码,要么根本没有评估那个如今承接每一次自然退出的调用点——也就是唯一一个在身后没有排队 kill() 的情况下进入 releaseHost_isReady === false 分支的调用者。这样一来,onExit 之后那条路径上的原生语义变化(例如 conpty-host.ts:18-21 已经点出的 std::erase_if baton 改动)就会在 web terminal 的主路径上未经复核地随升级发布。

证据(逐行读取核对,未采信引用):

扫描:3 个文件中共 7 处描述 release() 的分支形态;其中 2 处被彻底证伪
判据:改动后的 release(),读自 web-terminal-registry.ts:488-499
          if (!session.exited) { ... killPtyTree(session.pty); }
          this.releasePtyResources(session);   <- 无条件调用,没有 else
        对比 base 上 :456-484 的 if/else
  conpty-host.ts:135                 「`else` arm」                             已删除的结构
  web-terminal-registry.test.ts:367  「the whole point of the else branch」     已删除的结构
  conpty-host.ts:136                 「the primary web-terminal path for #11303」调用者标注错误
  web-terminal-registry.test.ts:321  「release() calls ...releaseHost?.() bare」 现已是间接调用
  web-terminal-registry.ts:332-334   「reaches it from BOTH arms」               措辞过时
  conpty-host.ts:144                 「from the live arm AND the exited arm」    措辞过时
  web-terminal-registry.ts:537       「from release() on both of its arms」      措辞过时(即本行)

请在同一次改动里把这些引用改到当前的控制流上。在 conpty-host.ts:134-136:142-146,把「release()else 分支」/「exited 分支」替换为退出时路径(handleExit → 延后的 releasePtyResourcesreleaseHost),并把 release() 保留为次要调用者。在 web-terminal-registry.ts:332-334,说明 releaseHost 是由 releasePtyResources 到达的,而 handleExit(延后)与 release() 都在 ptyResourcesReleased 标志保护下调用它。同时删去 web-terminal-registry.test.ts:355:367release() 分支的引用。本段 JSDoc 已经点名 handleExit 是调用者,因此只有「both of its arms」这一措辞需要同样处理。

改写时有一条约束:这个新的退出时调用点必须留在 conpty-host.ts原生空操作那一组,而不是关闭那一组,依据是 conpty-host.ts:128-131——「In src/win/conpty.cc the native exit-watcher thread erases the pty baton before it delivers the JS onExit, and PtyKill skips ClosePseudoConsole when get_pty_baton returns null」——而退出时释放严格发生在 onExit 之后。因此改写后的清单必须让 conpty-host.ts:147-148(「The inbox conhost half of #11303 is therefore not fixed by this function on the natural-exit path」)对它依然成立。

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

@doudouOUC doudouOUC left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Review at head 88718ce9a6ad57ba3099a8a3f0c9721c92a5c206 — verdict: no Criticals, 3 Suggestions. But this looks superseded by #11585.

Superseded

#11585 is the same author, same file, functionally the same fix, opened about an hour later, and strictly better: it adds the regression test that pins the deferral, adds .unref?.() on the timer, and makes the flag optional. Your own triage comment on #11585 says as much. The only argument for landing this one instead is that it is already approved and green — and that costs the regression test.

Suggestions

  1. packages/core/src/services/web-terminal-registry.ts:286setImmediate(() => this.releasePtyResources(session)); without .unref?.(), so the timer can briefly hold the event loop open at shutdown. #11585 has the .unref?.().
  2. Same single-tick-vs-two-turn-drain gap as #11585: packages/core/src/services/shellExecutionService.ts:1927-1935 drains twice (flushChain().then(drain).then(drain)) precisely because one tick can leave queued PTY bytes unflushed, so a tail of scrollback can still be dropped.
  3. packages/core/src/services/conpty-host.ts — the releaseHost call-site inventory comment still calls release()'s else arm "the primary web-terminal path for #11303", the arm this PR deletes. Stale comment.

Re-check of the existing C=0 APPROVE

Both of its inline Suggestions still stand at this head: neither touches conpty-host.ts, and the deferral is still untested here. Neither is Critical-severity.

Basis for the no-Critical verdict

The helper is byte-identical to #11585's apart from the flag name (ptyResourcesReleased, non-optional), with the same single-writer/single-reader read-site result — not a dead switch. Same verification for double-free, recycled-pid safety, listener-clear ordering, replay/admission/idle-reclaim invariants, and the write()/resize() short-circuits.


Agent-assisted review. Findings were re-read in the file content at the exact head SHA above before filing. Posting as a comment only — no approval implied.

@wenshao

wenshao commented Sep 10, 2026

Copy link
Copy Markdown
Collaborator

Maintainer runtime verification — built a real environment for this one

Verdict: the change does what it claims, and I could not break it. One small test-coverage gap worth closing before merge (2 lines, patch below); nothing blocking.

The PR is honest that it is a Windows resource fix proved only through a mocked PTY, so I did not re-run its unit tests and stop there. I built an A/B harness that drives the real compiled registry against a real node-pty, a real serve /terminal WebSocket route, and a real browser running the actual web-shell TerminalPanel — and replaced the vi.fn() conout worker with a real OS thread, so "was the PTY freed?" becomes a measurement instead of a spy assertion.

How the harness works (why an OS-level measurement is possible on Linux)
  • Two dist artifacts from one worktree: AFTER = 88718ce9a6, BEFORE = the same tree with only web-terminal-registry.ts reverted to ac1edef97. packages/core/dist/src/services/web-terminal-registry.js is the only file that differs; everything else (harness, host, node-pty, shell) is byte-identical between the arms.
  • No product code is instrumented. Two things outside the code under test are patched before the dist is imported:
    • require('node:os').platform → () => 'win32', so the real conpty-host.ts win32 branches (releaseConPtyHost / disposeConoutWorker) actually execute. killPtyTree and resolveWebTerminalShell read process.platform, so they stay POSIX and the shell is a real bash.
    • @lydell/node-pty's spawn is wrapped to attach a Windows-shaped _agent whose _conoutSocketWorker.dispose() terminates a real worker_threads.Worker, and to tap the real onData/onExit disposables (node-pty's disposables genuinely detach the listener, unlike the test fake).
  • So conout workers still alive is counted from real worker.on('exit') and cross-checked against /proc/self/task.
  • The route is the real createTerminalWsHandler from packages/cli/src/serve/routes/terminal.ts (it has zero runtime imports, so it esbuild-bundles standalone) mounted on a real ws server; the browser page mounts the real TerminalPanel.tsx through Vite, with only useWorkspace() stubbed to point at the harness origin. Chromium 1228 via Playwright.

1. Does it actually free the PTY at exit time? Yes — measured, not asserted

Nothing calls release() in any of these runs and no clock is advanced, so the 15-minute reclaim provably cannot have fired.

Scenario (Windows-shaped agent, real worker thread) BEFORE ac1edef97 AFTER (this PR)
8 terminals exit naturally → conout workers still alive 8 0
… process threads (/proc/self/task), baseline 7 19 11
… time from onExit to the worker being freed never 0.30 – 0.37 ms
… snapshots still replayable afterwards 8 / 8 8 / 8
Shell exits before its first output byte (_isReady === false) worker alive, 0 disposes worker freed via disposeConoutWorker, native close correctly skipped
Exit, then a tab-close release() on the already-freed session 0 → 1 dispose 1 → 1 (exactly once; _ptyNative.kill also stays at 1)

Real browser, real TerminalPanel, real route — the moment after exit 3, before anything releases:

exit-time release A/B

The accumulation argument in the PR body holds up too. The admission cap is 8, exited sessions are excluded from it, so churning terminals piles up without bound inside the window:

40 terminals churned

The RSS figure shows the shape of the leak, not a Windows prediction — a Node worker is far heavier than node-pty's real conout worker. The worker count and thread count are the meaningful numbers.

2. Does it break scrollback replay? No

The real risk here is the one the PR flags as reasoned-but-unmeasured, so I measured it.

Check BEFORE AFTER
Real PTY, 320 KB payload then exit 3, snapshot bytes at exit / +1 turn / +50 ms / +500 ms / +2 s 360136 at every stage 360136 at every stage
__PAYLOAD_END_MARKER__ present in the replayed snapshot yes yes
write() / resize() after the release unavailable / false unavailable / false
Route E2E: tab A gets {type:'exit',exitCode:3} then close 4000 yes yes
Route E2E: tab B re-attaches to the exited id → replay byte-identical to what tab A saw yes yes, then releaseAfterReplay drops the session
Route E2E: tab C on the same id spawns a fresh shell with a new pid yes yes

replay after the exit-time release

On the trailing-output drain specifically. node-pty's UnixTerminal emits exit from inside the socket close handler (// XXX Sometimes a data event is emitted after exit. Wait til socket is destroyed.), so on Linux a data callback after onExit is structurally impossible — confirmed across 24 runs: lastDataAt > exitAt was never true, and the registry's buffer equalled the bytes node-pty delivered in every single run on both arms. So the setImmediate is a no-op on Linux and its value really is Windows-only, exactly as the PR says. The reasoning from the shellExecutionService precedent is the right call; it just cannot be exercised here.

Reviewer warning about a flake you will hit. ~2 runs in 24 (both arms, uncorrelated) end with a short stream — node-pty itself delivered fewer bytes, its 200 ms DESTROY_SOCKET_TIMEOUT_MS destroying the socket mid-stream. It is pre-existing and upstream of this change; the tell is registry buffer bytes == bytes node-pty delivered, which held 24/24. Don't mistake it for a regression from this PR.

3. The release() reorder is observably neutral

The PR moves the disposable disposal from before killPtyTree to after it. Live tab-close on a running sleep 300, real pty:

BEFORE AFTER
call order dataDispose → exitDispose → ptyKill ptyKill → dataDispose → exitDispose
shell pid alive afterwards no no
releaseHost → native close / conout dispose 0 / 0 (releasedHosts short-circuit) 0 / 0
session removed from the map yes yes
no onExit interleaved between kill and dispose confirmed (every step in between is synchronous)

Also audited every remaining use of the released handle: write() (:438) and resize() (:450) both short-circuit on session.exited, and killPtyTree (:497) only runs on the live arm. There is no use-after-release path.

4. Test teeth — an 8-mutant sweep, and the one gap

Red/green reproduces exactly as claimed: base source + this PR's tests → 2 failed | 30 passed; with the fix → 32 passed.

Mutant Result
drop if (session.ptyResourcesReleased) return; 1 failed ✅ (author's claim confirmed)
setImmediate(...) → inline call 32 passed ❌ — not covered
releasePtyResources before killPtyTree 3 failed ✅
drop dataDisposable.dispose() 4 failed ✅
drop exitDisposable.dispose() 4 failed ✅
drop pty.releaseHost() 7 failed ✅
ptyResourcesReleased: true at creation 8 failed ✅
setImmediatesetTimeout(…, IDLE_RECLAIM_MS) 2 failed ✅

Only one hole: the deferral itself — the PR's whole risk mitigation — is not pinned by any test. A future "simplification" to an inline call would be green. Two lines in the first new test fix it, and I verified they have teeth (unmutated 32 passed; with the inline mutant 1 failed | 31 passed, expected "spy" to not be called at all, but actually been called 1 times):

     onExit({ exitCode: 0 });
+    // The release is deferred one turn on purpose, so late PTY data still
+    // reaches `buffer` before the data listener is detached.
+    expect(conoutDispose).not.toHaveBeenCalled();
+    expect(disposeData).not.toHaveBeenCalled();
     // One turn of the event loop, on real timers: the 15-minute idle reclaim

Worth noting for the record that the third new test (does not free an exited session twice…) is vacuously green on the base SHA — it guards the new flag rather than proving the fix, which is correct, just not part of the red/green evidence.

5. Static checks

tsc --noEmit produces an identical error set on both arms (61, all pre-existing @types/node drift in unrelated test files, none in the two changed files) → this PR adds zero type errors. prettier --check clean. eslint reports 3 errors in the test file, but the same 3 at the same code on the base SHA (line numbers shifted by the +77 additions) — pre-existing, not introduced here.

6. Scope notes — not defects, just what a reader should not assume is fixed

  • The JS-side accumulation is unchanged: 40 exited sessions stay in the map with their buffers (up to 4 MB each) for the full 15 minutes, and the !session.exited cap filter means that is still unbounded. This PR closes the native-handle feeder only, which is what it says it does; the memory half of [Windows] qwen-cli (VS Code Companion) leaks headless conhost.exe ConPTY processes - 347 processes / ~2.8 GB after ~12h uptime #11303 remains.
  • On the live release arm the conout worker is deliberately not disposed (kill() records the note → releaseConPtyHost early-returns) — that is fix(core): release node-pty's conout worker after every PTY on Windows #11313's "the queued kill() stays the single closer" design, unchanged here, and my BEFORE/AFTER numbers match on it. Don't read workers alive on the live arm as a leak.
  • Everything above is mechanism-level. Actual conhost.exe reclamation on a real Windows ConPTY still needs a Windows run, and PR CI skips Windows. What I can say is that the release now happens at exit, exactly once, at the correct point in the sequence, and that the paths reaching releaseConPtyHost / disposeConoutWorker are the ones the PR describes.

LGTM for merge once the two-line deferral assertion is added (or explicitly declined).

中文版

维护者运行时验证 —— 为此专门搭了一套真实环境

结论:改动确实做到了它声称的事,我没能把它弄坏。合并前建议补一个很小的测试覆盖缺口(2 行,见下方补丁);不是阻塞项。

PR 本身很坦诚地说明这是一个 Windows 资源修复、只能通过 mock PTY 来证明,所以我没有停留在重跑它的单测上。我搭了一套 A/B 环境,用真实编译产物中的 registry,配真实 node-pty真实 serve /terminal WebSocket 路由、以及真实浏览器里跑真实的 web-shell TerminalPanel,并且把 vi.fn() 的 conout worker 换成了真实的操作系统线程——这样"PTY 到底被释放了没有"就从一个 spy 断言变成了一次实测。

环境是怎么搭的(为什么在 Linux 上也能做 OS 级别的测量)
  • 同一个 worktree 出两份 dist:AFTER = 88718ce9a6BEFORE = 同一棵树上只把 web-terminal-registry.ts 回退到 ac1edef97。两臂之间只有 packages/core/dist/src/services/web-terminal-registry.js 不同,其余(harness、主机、node-pty、shell)逐字节相同。
  • 不对产品代码做任何插桩。只在导入 dist 之前打了两处「被测代码之外」的补丁:
    • require('node:os').platform → () => 'win32',让真实的 conpty-host.ts win32 分支(releaseConPtyHost / disposeConoutWorker)真正执行。killPtyTreeresolveWebTerminalShell 读的是 process.platform,所以仍走 POSIX,shell 是真实的 bash
    • 包装 @lydell/node-ptyspawn,挂上一个 Windows 形状的 _agent,其 _conoutSocketWorker.dispose()终止一个真实的 worker_threads.Worker;同时接管真实的 onData/onExit disposable(node-pty 的 disposable 是真的会摘除监听器的,测试里的 fake 不会)。
  • 因此「仍存活的 conout worker 数」是从真实的 worker.on('exit') 统计出来的,并与 /proc/self/task 交叉核对。
  • 路由用的是 packages/cli/src/serve/routes/terminal.ts 里真实的 createTerminalWsHandler(它没有任何运行时 import,可以独立 esbuild 打包),挂在真实的 ws server 上;浏览器页面通过 Vite 挂载真实的 TerminalPanel.tsx,只把 useWorkspace() 换成指向 harness 的桩。Chromium 1228 + Playwright。

1. 真的在退出时释放了 PTY 吗?是 —— 实测,而非断言

所有这些跑法里都没有任何一处调用 release(),也没有推进任何时钟,所以 15 分钟的空闲回收可证明不可能触发。

场景(Windows 形状的 agent + 真实 worker 线程) BEFORE ac1edef97 AFTER(本 PR)
8 个终端自然退出 → 仍存活的 conout worker 8 0
…进程线程数(/proc/self/task),基线 7 19 11
…从 onExit 到 worker 被释放的耗时 从不释放 0.30 – 0.37 ms
…之后 snapshot 仍可回放 8 / 8 8 / 8
shell 在首个输出字节之前就退出(_isReady === false worker 存活,0 次 dispose disposeConoutWorker 释放,且正确跳过 native close
退出后再对已释放会话执行关闭标签页的 release() 0 → 1 次 dispose 1 → 1(恰好一次;_ptyNative.kill 同样保持 1)

真实浏览器、真实 TerminalPanel、真实路由 —— exit 3 之后、任何释放动作之前的那一刻:

exit-time release A/B

PR 里关于「累积」的论证也站得住:准入上限是 8,已退出会话被排除在外,所以反复开关终端会在窗口内无上限累积:

40 terminals churned

RSS 那一组展示的是泄漏的形状,不是对 Windows 的预测——Node worker 比 node-pty 真实的 conout worker 重得多。有意义的数字是 worker 数与线程数。

2. 会破坏 scrollback 回放吗?不会

这里真正的风险正是 PR 标注为「靠推理、未实测」的那一条,所以我把它测了。

检查项 BEFORE AFTER
真实 PTY,320 KB 输出后 exit 3,在 退出时 / +1 轮 / +50 ms / +500 ms / +2 s 的 snapshot 字节数 各阶段均 360136 各阶段均 360136
回放的 snapshot 中存在 __PAYLOAD_END_MARKER__
释放后的 write() / resize() unavailable / false unavailable / false
路由 E2E:标签页 A 收到 {type:'exit',exitCode:3} 后以 4000 关闭
路由 E2E:标签页 B 重连到已退出的 id → 回放与 A 所见逐字节一致 ,随后 releaseAfterReplay 丢弃会话
路由 E2E:标签页 C 用同一 id 拉起全新 shell(新 pid)

replay after the exit-time release

关于尾部输出 drain。 node-pty 的 UnixTerminal 是在 socket close 处理函数里才 emit exit 的(// XXX Sometimes a data event is emitted after exit. Wait til socket is destroyed.),所以在 Linux 上「onExit 之后还有 data 回调」在结构上不可能发生——24 次运行全部确认:lastDataAt > exitAt 从未成立,且两臂的每一次运行中 registry 的 buffer 都等于 node-pty 实际投递的字节数。因此 setImmediate 在 Linux 上是个 no-op,它的价值确实只在 Windows,与 PR 的说法一致。参照 shellExecutionService 先例的这个判断是对的,只是在这里无法被真正触发。

给评审者的一个 flake 提醒。 24 次里约有 2 次(两臂都出现过,且不相关)末尾会缺一截——是 node-pty 自己少投递了字节,其 200 ms 的 DESTROY_SOCKET_TIMEOUT_MS 在流未读完时 destroy 了 socket。这是既有问题、且在本改动的上游;判别方法是看 registry buffer 字节数 == node-pty 投递字节数,这一条 24/24 成立。不要把它误判成本 PR 引入的回归。

3. release() 的顺序调整在可观测层面是中性的

本 PR 把 disposable 的摘除从 killPtyTree 之前挪到了之后。对一个正在跑 sleep 300 的真实 pty 做实时关闭标签页:

BEFORE AFTER
调用顺序 dataDispose → exitDispose → ptyKill ptyKill → dataDispose → exitDispose
之后 shell pid 是否存活
releaseHost → native close / conout dispose 0 / 0(releasedHosts 短路) 0 / 0
会话从 map 中移除
kill 与 dispose 之间没有 onExit 插入 已确认(其间每一步都是同步的)

另外我审计了释放后句柄的所有剩余使用点:write():438)与 resize():450)都对 session.exited 短路,killPtyTree:497)只在实时分支上运行。不存在 use-after-release 路径。

4. 测试的牙齿 —— 8 个变异体的扫描,以及唯一的缺口

红/绿完全复现了 PR 的说法:基线源码 + 本 PR 的测试 → 2 failed | 30 passed;带上修复 → 32 passed

变异体 结果
删掉 if (session.ptyResourcesReleased) return; 1 failed ✅(作者的说法成立)
setImmediate(...) → 直接内联调用 32 passed ❌ —— 未被覆盖
releasePtyResources 移到 killPtyTree 之前 3 failed ✅
删掉 dataDisposable.dispose() 4 failed ✅
删掉 exitDisposable.dispose() 4 failed ✅
删掉 pty.releaseHost() 7 failed ✅
创建时 ptyResourcesReleased: true 8 failed ✅
setImmediatesetTimeout(…, IDLE_RECLAIM_MS) 2 failed ✅

只有一个洞:延后本身——也就是本 PR 全部的风险缓解手段——没有被任何测试钉住。 将来有人把它「简化」成内联调用,测试依然全绿。在第一个新测试里加两行即可,并且我验证过它是有牙齿的(不变异时 32 passed;配内联变异体 1 failed | 31 passedexpected "spy" to not be called at all, but actually been called 1 times):

     onExit({ exitCode: 0 });
+    // The release is deferred one turn on purpose, so late PTY data still
+    // reaches `buffer` before the data listener is detached.
+    expect(conoutDispose).not.toHaveBeenCalled();
+    expect(disposeData).not.toHaveBeenCalled();
     // One turn of the event loop, on real timers: the 15-minute idle reclaim

另外记录一点:第三个新测试(does not free an exited session twice…)在基线 SHA 上是空转全绿的——它守的是新标志,而不是证明修复本身。这没有问题,只是它不属于红/绿证据的一部分。

5. 静态检查

tsc --noEmit 在两臂上产生完全相同的错误集合(61 个,全部是无关测试文件里既有的 @types/node 漂移,两个被改文件里一个都没有)→ 本 PR 新增零个类型错误。prettier --check 通过。eslint 在测试文件里报 3 个错误,但基线 SHA 上同样的代码同样报这 3 个(行号因 +77 行新增而位移)——既有问题,不是本 PR 引入的。

6. 范围说明 —— 不是缺陷,只是提醒读者别误以为这些也被修了

  • JS 侧的累积没有变化:40 个已退出会话连同各自最多 4 MB 的 buffer 会在 map 里留满 15 分钟,而 !session.exited 的上限过滤意味着这一侧仍然无上限。本 PR 只关掉了原生句柄这一个来源,这也正是它自己说的;[Windows] qwen-cli (VS Code Companion) leaks headless conhost.exe ConPTY processes - 347 processes / ~2.8 GB after ~12h uptime #11303 里内存的那一半仍在。
  • 实时释放分支上,conout worker 是被刻意不释放的(kill() 记下 note → releaseConPtyHost 提前返回)——这是 fix(core): release node-pty's conout worker after every PTY on Windows #11313 的「让排队中的 kill() 做唯一的关闭者」设计,本 PR 未做改动,我的 BEFORE/AFTER 数据在这一点上也完全一致。不要把实时分支上的 workers alive 读成泄漏。
  • 以上全部是机制层面的验证。真实 Windows ConPTY 上 conhost.exe 的实际回收仍需要一次 Windows 运行,而 PR 上的 Windows CI 是跳过的。我能给出的结论是:释放现在确实发生在退出时、恰好一次、且处在序列中正确的位置,并且真正走到 releaseConPtyHost / disposeConoutWorker 的路径与 PR 描述一致。

同意合并,前提是补上那两行延后断言(或者明确说明不加的理由)。

@yiliang114
yiliang114 added this pull request to the merge queue Sep 11, 2026
Merged via the queue into main with commit 38688f8 Sep 11, 2026
171 checks passed
yiliang114 added a commit that referenced this pull request Sep 11, 2026
Resolve conflict in web-terminal-registry: main already landed the same
exit-time PTY release via #11572. Take main's implementation
(ptyResourcesReleased flag) and its three tests, keep this PR's
test-isolation fake (capture disposeData by value) plus the trailing
output scrollback test.

Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Patrol-Run: qwen-pr-conflict/jmtwanbuowv
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

review/self-reported The linked issue was opened by the PR author (self-reported)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Windows] WebTerminalRegistry holds an exited terminal's PTY resources until the 15-minute idle reclaim, unbounded inside that window

5 participants