Reap the pty's conhost on Windows builds where it fails to run down (#5879)

A pseudoconsole's conhost.exe is spawned by CreatePseudoConsole as a
child of lazygit, so it is not part of the job object that the pty
teardown kills. That is normally fine: a healthy conhost runs itself
down once the reference handle is closed and its clients are gone. But
conhost builds before the ConPTY overhaul that shipped with Windows 11
24H2 (confirmed on 23H2, build 22631) fail to complete the rundown
when a client attached after the close event was delivered and was
then killed -- the fate of exactly the clients the job kill exists for
-- and such a conhost lingers forever with no clients, at a rate of
about one per five fast commit navigations. These builds remain
widespread: all of Windows 10 (whose ESU tail runs into 2028, and
whose hardware often cannot run Windows 11 at all) plus pre-24H2
Windows 11 fleets.

Since Windows offers no way to obtain the conhost's pid or handle from
the HPCON, identify it by diffing lazygit's direct conhost children
around the CreatePseudoConsole call, serialized by a mutex so that two
concurrently starting ptys can't confuse each other's diff, and open a
handle immediately so that pid reuse is harmless. The teardown then
gives conhost a second to exit on its own before terminating it; on
healthy builds the wait succeeds and the reap never fires. If the
conhost can't be identified unambiguously, we simply don't reap, which
is no worse than before.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

pkg/commands/oscommands/pty_windows.go

@@ -4,7 +4,9 @@ import (
 	"fmt"
 	"os"
 	"os/exec"
+	"strings"
 	"sync"
+	"time"
 	"unsafe"
 
 	"github.com/jesseduffield/lazygit/pkg/utils"
@@ -15,7 +17,11 @@ type winPty struct {
 	hpc windows.Handle
 	// job holds the child and every descendant it spawns; terminating it
 	// kills whatever is left of the process tree (see Close).
-	job     windows.Handle
+	job windows.Handle
+	// conhost is a handle to the conhost.exe serving this pty, or 0 if it
+	// couldn't be identified. Held so that the teardown in Close can reap
+	// it on Windows builds whose conhost fails to run down on its own.
+	conhost windows.Handle
 	inWrite *os.File
 	outRead *os.File
 
@@ -67,6 +73,10 @@ func (p *winPty) closeHpc() {
 	windows.ClosePseudoConsole(p.hpc)
 }
 
+// How long Close waits for the conhost to run itself down after its clients
+// are gone, before concluding that it never will (see Close) and reaping it.
+const conhostExitTimeout = time.Second
+
 // Close tears the pty down without waiting for it: the teardown runs on a
 // background goroutine and Close returns immediately.
 //
@@ -108,6 +118,15 @@ func (p *winPty) closeHpc() {
 // client does, and that client only goes away through the job kill —
 // sequencing the kill after a blocking close would thus deadlock in
 // exactly the case the kill exists for.
+//
+// After the kill, the conhost serving the pty is reaped as well if it
+// doesn't exit by itself: a healthy conhost runs down once the reference
+// handle is closed and its clients are gone, but conhost builds before
+// Windows 11 24H2 fail to complete the rundown when a client attached
+// after the close event was delivered and was then killed — the fate of
+// exactly the clients the job kill is for — and such a conhost sits
+// around forever, serving nothing (#5879). The reap is inert on healthy
+// builds: the wait succeeds and only the handle is closed.
 func (p *winPty) Close() error {
 	go utils.Safe(func() {
 		p.inWrite.Close()
@@ -116,6 +135,14 @@ func (p *winPty) Close() error {
 
 		_ = windows.TerminateJobObject(p.job, 1)
 		_ = windows.CloseHandle(p.job)
+
+		if p.conhost != 0 {
+			event, err := windows.WaitForSingleObject(p.conhost, uint32(conhostExitTimeout/time.Millisecond))
+			if err != nil || event != windows.WAIT_OBJECT_0 {
+				_ = windows.TerminateProcess(p.conhost, 1)
+			}
+			_ = windows.CloseHandle(p.conhost)
+		}
 	})
 	return nil
 }
@@ -151,6 +178,52 @@ func startWaiter(proc *os.Process, p *winPty) func() error {
 	}
 }
 
+// conhostScanMu serializes CreatePseudoConsole and the child-process scans
+// around it, so that two concurrently starting ptys can't make each other's
+// "which conhost is new" diff ambiguous.
+var conhostScanMu sync.Mutex
+
+// conhostChildren returns the pids of all conhost.exe processes that are
+// direct children of this process. Errors just yield a smaller (possibly
+// empty) set; the caller treats identification as best-effort.
+func conhostChildren() map[uint32]bool {
+	pids := map[uint32]bool{}
+	snap, err := windows.CreateToolhelp32Snapshot(windows.TH32CS_SNAPPROCESS, 0)
+	if err != nil {
+		return pids
+	}
+	defer func() { _ = windows.CloseHandle(snap) }()
+	me := uint32(os.Getpid())
+	var pe windows.ProcessEntry32
+	pe.Size = uint32(unsafe.Sizeof(pe))
+	for err := windows.Process32First(snap, &pe); err == nil; err = windows.Process32Next(snap, &pe) {
+		if pe.ParentProcessID == me && strings.EqualFold(windows.UTF16ToString(pe.ExeFile[:]), "conhost.exe") {
+			pids[pe.ProcessID] = true
+		}
+	}
+	return pids
+}
+
+// openNewConhostChild returns a handle to the single conhost child that
+// appeared since the before scan, or 0 if there isn't exactly one candidate
+// or it can't be opened.
+func openNewConhostChild(before map[uint32]bool) windows.Handle {
+	var found []uint32
+	for pid := range conhostChildren() {
+		if !before[pid] {
+			found = append(found, pid)
+		}
+	}
+	if len(found) != 1 {
+		return 0
+	}
+	h, err := windows.OpenProcess(windows.SYNCHRONIZE|windows.PROCESS_TERMINATE, false, found[0])
+	if err != nil {
+		return 0
+	}
+	return h
+}
+
 func StartPty(cmd *exec.Cmd, cols, rows uint16) (sp StartedPty, err error) {
 	// Two pipes: one for the child's stdin (we never write to it, but ConPTY
 	// needs a handle), one for the child's stdout/stderr multiplexed through
@@ -176,9 +249,24 @@ func StartPty(cmd *exec.Cmd, cols, rows uint16) (sp StartedPty, err error) {
 
 	// CreatePseudoConsole dupes the handles it needs internally; we release
 	// our references to the child-side ends immediately after.
-	var hpc windows.Handle
+	//
+	// It also spawns the conhost.exe serving the console session, as a
+	// direct child of this process. The teardown in Close needs a handle to
+	// that conhost (see there), but Windows offers no way to obtain one
+	// from the HPCON, so identify it by diffing our conhost children around
+	// the call. Open a real handle right away so that pid reuse can't later
+	// misdirect the teardown's reap. If identification fails, the handle
+	// stays 0 and the teardown skips the reap.
+	var hpc, conhost windows.Handle
 	size := clampPtySize(cols, rows)
-	if err = windows.CreatePseudoConsole(size, inRead, outWrite, 0, &hpc); err != nil {
+	conhostScanMu.Lock()
+	conhostsBefore := conhostChildren()
+	err = windows.CreatePseudoConsole(size, inRead, outWrite, 0, &hpc)
+	if err == nil {
+		conhost = openNewConhostChild(conhostsBefore)
+	}
+	conhostScanMu.Unlock()
+	if err != nil {
 		_ = windows.CloseHandle(inRead)
 		_ = windows.CloseHandle(outWrite)
 		return StartedPty{}, fmt.Errorf("CreatePseudoConsole: %w", err)
@@ -188,6 +276,9 @@ func StartPty(cmd *exec.Cmd, cols, rows uint16) (sp StartedPty, err error) {
 	defer func() {
 		if err != nil {
 			windows.ClosePseudoConsole(hpc)
+			if conhost != 0 {
+				_ = windows.CloseHandle(conhost)
+			}
 		}
 	}()
 
@@ -318,6 +409,7 @@ func StartPty(cmd *exec.Cmd, cols, rows uint16) (sp StartedPty, err error) {
 	wp := &winPty{
 		hpc:     hpc,
 		job:     job,
+		conhost: conhost,
 		inWrite: os.NewFile(uintptr(inWrite), "conpty-in"),
 		outRead: os.NewFile(uintptr(outRead), "conpty-out"),
 	}

pkg/commands/oscommands/pty_windows_test.go

@@ -25,6 +25,24 @@ func TestStartPtyWithZeroSize(t *testing.T) {
 	}
 }
 
+// StartPty must identify the conhost.exe that CreatePseudoConsole spawned to
+// serve the pty: the teardown in Close reaps it on Windows builds whose
+// conhost fails to run down on its own, and a failed identification silently
+// degrades to not reaping. If this fails, the child-scan in
+// openNewConhostChild no longer matches how Windows hosts pseudoconsoles.
+func TestStartPtyIdentifiesConhost(t *testing.T) {
+	sp, err := StartPty(exec.Command("cmd", "/c", "exit 0"), 80, 24)
+	assert.NoError(t, err)
+	if err != nil {
+		return
+	}
+
+	assert.NotZero(t, sp.Pty.(*winPty).conhost)
+
+	_ = sp.Wait()
+	_ = sp.Pty.Close()
+}
+
 // Closing the pty must terminate the process tree it was running, even when
 // it is closed so soon after starting that the child hasn't attached to the
 // pseudoconsole yet: such a child misses the CTRL_CLOSE_EVENT that the close