Skip to content

fix(win): close the pseudoconsole when the shell exits on its own - #990

Open
Tyler Vorpahl (Tyler-V) wants to merge 1 commit into
microsoft:mainfrom
Tyler-V:fix-close-pseudoconsole-on-exit
Open

Tyler Vorpahl (Tyler-V) wants to merge 1 commit into
microsoft:mainfrom
Tyler-V:fix-close-pseudoconsole-on-exit

Conversation

@Tyler-V

Copy link
Copy Markdown

Fixes #965.

When the shell exits on its own, the exit watcher in SetupExitCallback erases the pty baton without closing its pseudoconsole. kill() is the only other path to ClosePseudoConsole, and it does nothing once the baton is gone. So every pty whose shell exits by itself leaves a conhost.exe --headless running until the host process exits. Any child still attached to that console keeps running too, because it never gets CTRL_CLOSE_EVENT.

The fix:

  • PtyConnect resolves ClosePseudoConsole (or ConptyClosePseudoConsole with useConptyDll) the same way PtyKill does and passes it to the watcher. The watcher thread can't call LoadConptyDll because that throws N-API errors.
  • The watcher copies hpc under the lock, erases the baton, queues the exit code with BlockingCall, and then closes the pseudoconsole outside the lock. Closing after BlockingCall puts the exit code in the queue ahead of the output pipe closing, which is what fires exit. Closing before Release() means a close that blocks on Windows before 24H2 never stalls the finalizer's join() on the main thread. If BlockingCall returns napi_closing, the close is skipped because the host process is going away.
  • After PtyKill closes the pseudoconsole it sets hpc to null and skips the close when hpc is already null. Without this, kill-then-exit would close it twice. This hunk matches fix(win): don't close the pseudoconsole twice on a second kill() #989 line for line on purpose, so the two PRs merge in either order.

How I verified it (Windows 11 build 26200, Node 24.15.0, on top of d182c60):

  • Six ptys of cmd.exe /d /c exit 0. After every onExit plus 3 s, I counted the process's direct conhost.exe children. Before the fix: +6, all --headless. After: +0. With useConptyDll: true, both builds show +0 hosts.
  • cmd.exe /d /k running ping -n 300 127.0.0.1 in the foreground, then I terminated only the shell. Before the fix, ping and conhost were still alive 3 s after onExit, and onExit took about 11 s to fire. After: both were gone and onExit fired in 66 ms. With useConptyDll: true, before the fix onExit did not fire at all while ping was attached (I waited over 2 minutes). After the fix it fired in 72 ms and ping was gone.
  • I ran five rounds each of kill before exit, kill twice, and kill after exit, with and without useConptyDll. No crashes and no leftover hosts.
  • 100 natural exits with varying exit codes (30 with useConptyDll) all reported the right code.

I added a test in windowsTerminal.test.ts that runs for both useConptyDll values. It spawns cmd.exe /C exit and checks that no new conhost or OpenConsole child of the test process outlives exit. Against the unfixed build it fails for useConptyDll = false. The useConptyDll = true case passes before and after and is there as a guard. npm test passes: 59 tests plus lint.

Two caveats:

  • I could only test on build 26200, where ClosePseudoConsole returns immediately. Before 24H2 it waits until the console host exits, which needs the output pipe drained. Here that wait runs on the watcher thread after the exit code is queued, while the conout worker keeps reading, so it should not block the event loop or delay onExit. I have not run it on an older build.
  • My Build Tools install has no Spectre libraries, so I built locally with SpectreMitigation off. binding.gyp is unchanged.

🤖 Generated with Claude Code

The exit watcher erased the pty baton without closing its pseudoconsole,
and kill() does nothing once the baton is gone. Every shell that exited
on its own left a conhost.exe running until the host process exited,
along with any child still attached to that console.

The watcher now closes the pseudoconsole right after it queues the exit
code. kill() nulls hpc once it has closed it, so the watcher never closes
it a second time.

Fixes microsoft#965

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@Tyler-V

Copy link
Copy Markdown
Author

@microsoft-github-policy-service agree

@MohammedSafwan10

Copy link
Copy Markdown

Tested this locally since we're hitting #965 in an Electron app (one headless conhost per agent command, and they pile up until the app quits).

Setup: Windows 11 build 26300, Node 24.15.0, VS 2026. Built this branch and its parent (d182c60) from source. I had to turn SpectreMitigation off locally because I don't have the Spectre libs installed. Nothing else changed.

Leak. 5 shells of cmd.exe /d /c echo hi that exit on their own, then I counted direct conhost.exe children 4 s later:

  • d182c60: 5 left behind
  • this PR: 0

I also tried a background child that's still attached when the leader exits (start "" /b node child.js, then the shell exits). On d182c60 the child keeps running. With this PR it goes away, which is what you'd get on Linux when the pty hangs up.

Output. I wanted to make sure the earlier close doesn't cut off output, so I ran 10 shells that each print 3000 lines plus a marker and then exit. All 10 got everything on both builds.

One test breaks. should be 0 before ready and set after ready_datapipe (issue #763) fails every time for useConptyDll = false on this branch (3/3 runs) and passes on d182c60. I don't think the fix is wrong. The test is racing it. It only attaches its exit listener inside a setTimeout(..., 100) after ready_datapipe. Exit used to arrive about 1 s later because of the old drain path, and now it arrives about 70 ms after ready_datapipe (107-113 ms from construction, with ready at ~38 ms). So exit has already fired before the listener exists, and done() never runs.

Recording exit up front fixes it for me:

const term = new WindowsTerminal('cmd.exe', '/c echo test', { useConptyDll });
let exited = false;
term.on('exit', () => { exited = true; });
// ...existing assertions inside the ready_datapipe setTimeout...
if (exited) return done();
term.on('exit', () => done());
term.kill();

With that change, windowsTerminal.test.js passed 26/26 in 2 of 3 full runs. The other run had should kill the process tree time out once. I saw that one fail twice in about 18 runs of this branch, then couldn't get it to fail again in 10 more runs, and it didn't fail in 9 runs on d182c60. Probably machine noise, but mentioning it in case someone else sees it too.

Happy to send the test change as a separate PR if that's easier.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ConPTY: the pseudoconsole is never closed on a natural shell exit — the exit watcher erases the baton before onExit, leaking one conhost.exe per pty

2 participants