Kill the pty child's process tree on Windows to avoid orphans (#5879)

Stopping a pty task on Windows relies on ClosePseudoConsole, which
delivers CTRL_CLOSE_EVENT to the console's attached clients. But only
to those attached at that moment: when the user flicks quickly through
commits, a task is often stopped within the first few milliseconds of
its life, before the child has attached to the pseudoconsole. Such a
child misses the event and survives, running the entire diff to
completion in the background (spawning one external differ per changed
file) and keeping its conhost.exe alive; rapid navigation accumulates
these git/difft/conhost trees, and they outlive lazygit. Grandchildren
are affected too: git for Windows runs commands through a two-level
git.exe wrapper, so a single task has several attach windows, and a
grandchild spawned while the console is going down is orphaned even
when its parent got the event and exited.

Fix this by putting the child into a job object before it runs its
first instruction (created suspended, assigned, then resumed), so that
every descendant is in the job from the start; the teardown in Close
terminates the job right after initiating the pseudoconsole close.
There is no point in a grace period between the two: the close event
is not a graceful signal -- git and the common diff tools leave it to
the default handler, which calls ExitProcess at an arbitrary point --
so clients that received it are already dying, and the kill exists for
those that missed it. Killing at an arbitrary point cannot leak a
stale index.lock, because pty-rendered commands no longer take that
lock (see withPtyGitConfig in pkg/gui/pty.go).

The pseudoconsole close runs on its own goroutine because the kill
must not wait for it: on builds where ClosePseudoConsole blocks until
the console host exits (pre-24H2), the host keeps running as long as a
surviving client does, and that client only goes away through the job
kill; sequencing the kill after a blocking close would deadlock in
exactly the case the kill exists for.

KILL_ON_JOB_CLOSE doubles as a safety net: if lazygit exits without
running the teardown, the OS closes the job handle and reaps the tree.

In a harness that mimicked the stop path with randomized 0-120ms stop
delays, 3 of 30 process trees survived as orphans before this change;
none survive with it.

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

pkg/commands/oscommands/pty_windows.go

@@ -12,7 +12,10 @@ import (
 )
 
 type winPty struct {
-	hpc     windows.Handle
+	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
 	inWrite *os.File
 	outRead *os.File
 
@@ -83,11 +86,36 @@ func (p *winPty) closeHpc() {
 // nobody is reading anymore, so that flush can only complete once the pipe
 // is broken. The background waiter's closeHpc may already be wedged in such
 // a flush while holding p.mu; closing the pipes is what unblocks it.
+//
+// Closing the pseudoconsole delivers CTRL_CLOSE_EVENT only to the clients
+// attached to it at that moment. A child that is stopped right after being
+// spawned is still starting up and not attached yet, so the event misses it
+// and it survives, running its command to completion as an orphan — and
+// keeping its console host alive with it (#5879); the same holds for
+// grandchildren spawned while the console is going down, and for clients
+// that ignore the event (the Windows flavor of #5675). The job kill reaps
+// all of those. There is no point in delaying it: the close event is not a
+// graceful signal worth waiting on — git and the common diff tools leave it
+// to the default handler, which calls ExitProcess at whatever instruction
+// the process happens to execute — so clients that got the event are
+// already dying. Killing at an arbitrary point cannot leak a stale
+// index.lock, because pty-rendered commands don't take that lock (see
+// withPtyGitConfig in pkg/gui/pty.go).
+//
+// The pseudoconsole close gets its own goroutine because the kill must not
+// wait for it: on builds where ClosePseudoConsole blocks until the console
+// host exits (pre-24H2), the host keeps running as long as a surviving
+// 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.
 func (p *winPty) Close() error {
 	go utils.Safe(func() {
 		p.inWrite.Close()
 		p.outRead.Close()
-		p.closeHpc()
+		go utils.Safe(p.closeHpc)
+
+		_ = windows.TerminateJobObject(p.job, 1)
+		_ = windows.CloseHandle(p.job)
 	})
 	return nil
 }
@@ -163,6 +191,34 @@ func StartPty(cmd *exec.Cmd, cols, rows uint16) (sp StartedPty, err error) {
 		}
 	}()
 
+	// The child goes into a job object so that the teardown in Close can
+	// terminate the whole process tree. KILL_ON_JOB_CLOSE makes the OS do
+	// that when the last handle to the job is closed, which doubles as a
+	// safety net: if lazygit exits without running the teardown, the handle
+	// is closed for it and the tree is reaped.
+	job, err := windows.CreateJobObject(nil, nil)
+	if err != nil {
+		return StartedPty{}, fmt.Errorf("CreateJobObject: %w", err)
+	}
+	defer func() {
+		if err != nil {
+			// Kills the child on error paths where it was already assigned
+			// to the job; plain handle cleanup before that.
+			_ = windows.CloseHandle(job)
+		}
+	}()
+	limits := windows.JOBOBJECT_EXTENDED_LIMIT_INFORMATION{
+		BasicLimitInformation: windows.JOBOBJECT_BASIC_LIMIT_INFORMATION{
+			LimitFlags: windows.JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE,
+		},
+	}
+	if _, err = windows.SetInformationJobObject(
+		job, windows.JobObjectExtendedLimitInformation,
+		uintptr(unsafe.Pointer(&limits)), uint32(unsafe.Sizeof(limits)),
+	); err != nil {
+		return StartedPty{}, fmt.Errorf("SetInformationJobObject: %w", err)
+	}
+
 	// Attach the pseudoconsole to the child via a process attribute list.
 	attrList, err := windows.NewProcThreadAttributeList(1)
 	if err != nil {
@@ -221,7 +277,7 @@ func StartPty(cmd *exec.Cmd, cols, rows uint16) (sp StartedPty, err error) {
 		nil, // process security
 		nil, // thread security
 		false,
-		windows.EXTENDED_STARTUPINFO_PRESENT|windows.CREATE_UNICODE_ENVIRONMENT,
+		windows.EXTENDED_STARTUPINFO_PRESENT|windows.CREATE_UNICODE_ENVIRONMENT|windows.CREATE_SUSPENDED,
 		envPtr,
 		dirPtr,
 		&si.StartupInfo,
@@ -230,6 +286,22 @@ func StartPty(cmd *exec.Cmd, cols, rows uint16) (sp StartedPty, err error) {
 	if err != nil {
 		return StartedPty{}, fmt.Errorf("CreateProcess: %w", err)
 	}
+
+	// The child was created suspended so that it can be assigned to the job
+	// before it runs its first instruction; that way every descendant it
+	// ever spawns is in the job from the start.
+	if err = windows.AssignProcessToJobObject(job, pi.Process); err != nil {
+		// Not in the job yet, so the deferred job-handle close can't reap it.
+		_ = windows.TerminateProcess(pi.Process, 1)
+		_ = windows.CloseHandle(pi.Thread)
+		_ = windows.CloseHandle(pi.Process)
+		return StartedPty{}, fmt.Errorf("AssignProcessToJobObject: %w", err)
+	}
+	if _, err = windows.ResumeThread(pi.Thread); err != nil {
+		_ = windows.CloseHandle(pi.Thread)
+		_ = windows.CloseHandle(pi.Process)
+		return StartedPty{}, fmt.Errorf("ResumeThread: %w", err)
+	}
 	_ = windows.CloseHandle(pi.Thread)
 
 	// Re-open the process by PID to get an *os.Process to wait on. Do this
@@ -245,6 +317,7 @@ func StartPty(cmd *exec.Cmd, cols, rows uint16) (sp StartedPty, err error) {
 
 	wp := &winPty{
 		hpc:     hpc,
+		job:     job,
 		inWrite: os.NewFile(uintptr(inWrite), "conpty-in"),
 		outRead: os.NewFile(uintptr(outRead), "conpty-out"),
 	}

pkg/commands/oscommands/pty_windows_test.go

@@ -3,6 +3,7 @@ package oscommands
 import (
 	"os/exec"
 	"testing"
+	"time"
 
 	"github.com/stretchr/testify/assert"
 )
@@ -23,3 +24,32 @@ func TestStartPtyWithZeroSize(t *testing.T) {
 		_ = 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
+// delivers to attached clients, and only the job-object kill reaps it.
+// Without the kill, cmd and its ping child keep running for ~30 seconds and
+// the Wait here times out.
+func TestClosePtyTerminatesChildProcessTree(t *testing.T) {
+	// The output redirect is there for the reason described in
+	// TestStartPtyWithZeroSize.
+	sp, err := StartPty(exec.Command("cmd", "/c", "ping -n 30 127.0.0.1 >nul"), 80, 24)
+	assert.NoError(t, err)
+	if err != nil {
+		return
+	}
+
+	_ = sp.Pty.Close()
+
+	exited := make(chan struct{})
+	go func() {
+		_ = sp.Wait()
+		close(exited)
+	}()
+	select {
+	case <-exited:
+	case <-time.After(5 * time.Second):
+		t.Fatal("child process was not terminated by closing the pty")
+	}
+}