Visitar URL original
Reap the pty's conhost on Windows builds where it fails to run down (… · jesseduffield/lazygit@323b0e5 · GitHub
Skip to content

Commit 323b0e5

Browse files
stefanhallerclaude
stefanhaller
and
claude
committed
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>
1 parent 23ccd36 commit 323b0e5

2 files changed

Lines changed: 112 additions & 3 deletions

File tree

‎pkg/commands/oscommands/pty_windows.go‎

Lines changed: 94 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import (
44
"fmt"
55
"os"
66
"os/exec"
7+
"strings"
78
"sync"
89
"time"
910
"unsafe"
@@ -16,7 +17,11 @@ type winPty struct {
1617
hpc windows.Handle
1718
// job holds the child and every descendant it spawns; terminating it
1819
// kills whatever is left of the process tree (see Close).
19-
job windows.Handle
20+
job windows.Handle
21+
// conhost is a handle to the conhost.exe serving this pty, or 0 if it
22+
// couldn't be identified. Held so that the teardown in Close can reap
23+
// it on Windows builds whose conhost fails to run down on its own.
24+
conhost windows.Handle
2025
inWrite *os.File
2126
outRead *os.File
2227

@@ -77,6 +82,10 @@ func (p *winPty) closeHpc() {
7782
// killed.
7883
const childExitGracePeriod = 500 * time.Millisecond
7984

85+
// How long Close waits for the conhost to run itself down after its clients
86+
// are gone, before concluding that it never will (see Close) and reaping it.
87+
const conhostExitTimeout = time.Second
88+
8089
// Close tears the pty down without waiting for it: the teardown runs on a
8190
// background goroutine and Close returns immediately.
8291
//
@@ -116,6 +125,15 @@ const childExitGracePeriod = 500 * time.Millisecond
116125
// attached client does, and a client that missed the close event only
117126
// goes away through the job kill — sequencing the kill after a blocking
118127
// close would thus deadlock in exactly the case the kill exists for.
128+
//
129+
// After the kill, the conhost serving the pty is reaped as well if it
130+
// doesn't exit by itself: a healthy conhost runs down once the reference
131+
// handle is closed and its clients are gone, but conhost builds before
132+
// Windows 11 24H2 fail to complete the rundown when a client attached
133+
// after the close event was delivered and was then killed — the fate of
134+
// exactly the clients the job kill is for — and such a conhost sits
135+
// around forever, serving nothing (#5879). The reap is inert on healthy
136+
// builds: the wait succeeds and only the handle is closed.
119137
func (p *winPty) Close() error {
120138
go utils.Safe(func() {
121139
p.inWrite.Close()
@@ -128,6 +146,14 @@ func (p *winPty) Close() error {
128146
}
129147
_ = windows.TerminateJobObject(p.job, 1)
130148
_ = windows.CloseHandle(p.job)
149+
150+
if p.conhost != 0 {
151+
event, err := windows.WaitForSingleObject(p.conhost, uint32(conhostExitTimeout/time.Millisecond))
152+
if err != nil || event != windows.WAIT_OBJECT_0 {
153+
_ = windows.TerminateProcess(p.conhost, 1)
154+
}
155+
_ = windows.CloseHandle(p.conhost)
156+
}
131157
})
132158
return nil
133159
}
@@ -163,6 +189,52 @@ func startWaiter(proc *os.Process, p *winPty) func() error {
163189
}
164190
}
165191

192+
// conhostScanMu serializes CreatePseudoConsole and the child-process scans
193+
// around it, so that two concurrently starting ptys can't make each other's
194+
// "which conhost is new" diff ambiguous.
195+
var conhostScanMu sync.Mutex
196+
197+
// conhostChildren returns the pids of all conhost.exe processes that are
198+
// direct children of this process. Errors just yield a smaller (possibly
199+
// empty) set; the caller treats identification as best-effort.
200+
func conhostChildren() map[uint32]bool {
201+
pids := map[uint32]bool{}
202+
snap, err := windows.CreateToolhelp32Snapshot(windows.TH32CS_SNAPPROCESS, 0)
203+
if err != nil {
204+
return pids
205+
}
206+
defer func() { _ = windows.CloseHandle(snap) }()
207+
me := uint32(os.Getpid())
208+
var pe windows.ProcessEntry32
209+
pe.Size = uint32(unsafe.Sizeof(pe))
210+
for err := windows.Process32First(snap, &pe); err == nil; err = windows.Process32Next(snap, &pe) {
211+
if pe.ParentProcessID == me && strings.EqualFold(windows.UTF16ToString(pe.ExeFile[:]), "conhost.exe") {
212+
pids[pe.ProcessID] = true
213+
}
214+
}
215+
return pids
216+
}
217+
218+
// openNewConhostChild returns a handle to the single conhost child that
219+
// appeared since the before scan, or 0 if there isn't exactly one candidate
220+
// or it can't be opened.
221+
func openNewConhostChild(before map[uint32]bool) windows.Handle {
222+
var found []uint32
223+
for pid := range conhostChildren() {
224+
if !before[pid] {
225+
found = append(found, pid)
226+
}
227+
}
228+
if len(found) != 1 {
229+
return 0
230+
}
231+
h, err := windows.OpenProcess(windows.SYNCHRONIZE|windows.PROCESS_TERMINATE, false, found[0])
232+
if err != nil {
233+
return 0
234+
}
235+
return h
236+
}
237+
166238
func StartPty(cmd *exec.Cmd, cols, rows uint16) (sp StartedPty, err error) {
167239
// Two pipes: one for the child's stdin (we never write to it, but ConPTY
168240
// needs a handle), one for the child's stdout/stderr multiplexed through
@@ -188,9 +260,24 @@ func StartPty(cmd *exec.Cmd, cols, rows uint16) (sp StartedPty, err error) {
188260

189261
// CreatePseudoConsole dupes the handles it needs internally; we release
190262
// our references to the child-side ends immediately after.
191-
var hpc windows.Handle
263+
//
264+
// It also spawns the conhost.exe serving the console session, as a
265+
// direct child of this process. The teardown in Close needs a handle to
266+
// that conhost (see there), but Windows offers no way to obtain one
267+
// from the HPCON, so identify it by diffing our conhost children around
268+
// the call. Open a real handle right away so that pid reuse can't later
269+
// misdirect the teardown's reap. If identification fails, the handle
270+
// stays 0 and the teardown skips the reap.
271+
var hpc, conhost windows.Handle
192272
size := clampPtySize(cols, rows)
193-
if err = windows.CreatePseudoConsole(size, inRead, outWrite, 0, &hpc); err != nil {
273+
conhostScanMu.Lock()
274+
conhostsBefore := conhostChildren()
275+
err = windows.CreatePseudoConsole(size, inRead, outWrite, 0, &hpc)
276+
if err == nil {
277+
conhost = openNewConhostChild(conhostsBefore)
278+
}
279+
conhostScanMu.Unlock()
280+
if err != nil {
194281
_ = windows.CloseHandle(inRead)
195282
_ = windows.CloseHandle(outWrite)
196283
return StartedPty{}, fmt.Errorf("CreatePseudoConsole: %w", err)
@@ -200,6 +287,9 @@ func StartPty(cmd *exec.Cmd, cols, rows uint16) (sp StartedPty, err error) {
200287
defer func() {
201288
if err != nil {
202289
windows.ClosePseudoConsole(hpc)
290+
if conhost != 0 {
291+
_ = windows.CloseHandle(conhost)
292+
}
203293
}
204294
}()
205295

@@ -330,6 +420,7 @@ func StartPty(cmd *exec.Cmd, cols, rows uint16) (sp StartedPty, err error) {
330420
wp := &winPty{
331421
hpc: hpc,
332422
job: job,
423+
conhost: conhost,
333424
inWrite: os.NewFile(uintptr(inWrite), "conpty-in"),
334425
outRead: os.NewFile(uintptr(outRead), "conpty-out"),
335426
exited: make(chan struct{}),

‎pkg/commands/oscommands/pty_windows_test.go‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,24 @@ func TestStartPtyWithZeroSize(t *testing.T) {
2525
}
2626
}
2727

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

0 commit comments

Comments
 (0)