Repository navigation
fix(win): close the pseudoconsole when the shell exits on its own - #990
Open
Tyler Vorpahl (Tyler-V) wants to merge 1 commit into
Open
Tyler Vorpahl (Tyler-V) wants to merge 1 commit into
Tyler Vorpahl (Tyler-V) wants to merge 1 commit into
Conversation
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>
Author
|
@microsoft-github-policy-service agree |
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.
Fixes #965.
When the shell exits on its own, the exit watcher in
SetupExitCallbackerases the pty baton without closing its pseudoconsole.kill()is the only other path toClosePseudoConsole, and it does nothing once the baton is gone. So every pty whose shell exits by itself leaves aconhost.exe --headlessrunning until the host process exits. Any child still attached to that console keeps running too, because it never getsCTRL_CLOSE_EVENT.The fix:
PtyConnectresolvesClosePseudoConsole(orConptyClosePseudoConsolewithuseConptyDll) the same wayPtyKilldoes and passes it to the watcher. The watcher thread can't callLoadConptyDllbecause that throws N-API errors.hpcunder the lock, erases the baton, queues the exit code withBlockingCall, and then closes the pseudoconsole outside the lock. Closing afterBlockingCallputs the exit code in the queue ahead of the output pipe closing, which is what firesexit. Closing beforeRelease()means a close that blocks on Windows before 24H2 never stalls the finalizer'sjoin()on the main thread. IfBlockingCallreturnsnapi_closing, the close is skipped because the host process is going away.PtyKillcloses the pseudoconsole it setshpcto null and skips the close whenhpcis 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):
cmd.exe /d /c exit 0. After everyonExitplus 3 s, I counted the process's directconhost.exechildren. Before the fix: +6, all--headless. After: +0. WithuseConptyDll: true, both builds show +0 hosts.cmd.exe /d /krunningping -n 300 127.0.0.1in the foreground, then I terminated only the shell. Before the fix, ping and conhost were still alive 3 s afteronExit, andonExittook about 11 s to fire. After: both were gone andonExitfired in 66 ms. WithuseConptyDll: true, before the fixonExitdid 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.useConptyDll. No crashes and no leftover hosts.useConptyDll) all reported the right code.I added a test in
windowsTerminal.test.tsthat runs for bothuseConptyDllvalues. It spawnscmd.exe /C exitand checks that no new conhost or OpenConsole child of the test process outlivesexit. Against the unfixed build it fails foruseConptyDll = false. TheuseConptyDll = truecase passes before and after and is there as a guard.npm testpasses: 59 tests plus lint.Two caveats:
ClosePseudoConsolereturns 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 delayonExit. I have not run it on an older build.SpectreMitigationoff.binding.gypis unchanged.🤖 Generated with Claude Code