Reap pty process trees synchronously when quitting on Windows (#5879)

The pty teardown in Close runs on a background goroutine that doesn't
get to finish when lazygit is quitting: the process exits milliseconds
after the view buffer managers are closed. The job objects still cover
the clients -- KILL_ON_JOB_CLOSE reaps them when the process's handles
are rundown at exit -- but nothing reaps the conhost, so on Windows
builds whose conhost fails to run down on its own, quitting leaks one
conhost per live pty.

This is not a rare timing window: a diff longer than what has been
read keeps its git process (and thus its pty and conhost) running for
the entire time it is displayed, so that scrolling can read more.
Quitting while looking at a long diff is therefore the common case,
and with an external differ configured it leaks a conhost on affected
builds on almost every quit.

Fix this by having the gui's shutdown path wait synchronously for the
in-flight teardowns after closing the view buffer managers. A quit
signal makes the teardowns skip the conhost rundown wait -- the
conhost serves nothing once its clients are dead, and the exit must
not stall for its sake -- so the wait normally completes in
milliseconds, keeping quit as fast as before; a 2-second cap protects
the exit path even if a teardown wedges.

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

pkg/commands/oscommands/pty_unix.go

@@ -32,3 +32,9 @@ func StartPty(cmd *exec.Cmd, cols, rows uint16) (StartedPty, error) {
 		Wait:    cmd.Wait,
 	}, nil
 }
+
+// TerminateLivePtys is a no-op on Unix: stopping a pty task signals the
+// child (SIGTERM, plus SIGHUP to the foreground process group when the
+// master closes), and the processes clean themselves up without lazygit
+// having to wait for them.
+func TerminateLivePtys() {}

pkg/commands/oscommands/pty_windows.go

@@ -77,6 +77,42 @@ func (p *winPty) closeHpc() {
 // are gone, before concluding that it never will (see Close) and reaping it.
 const conhostExitTimeout = time.Second
 
+var (
+	// ptyTeardowns counts the in-flight teardown goroutines spawned by
+	// Close; TerminateLivePtys waits for them when lazygit exits.
+	ptyTeardowns sync.WaitGroup
+	// ptyQuit is closed by TerminateLivePtys. In-flight teardowns skip the
+	// conhost rundown wait once it is closed: the conhost serves nothing
+	// once its clients are gone, and the exit must not stall for its sake.
+	ptyQuit     = make(chan struct{})
+	ptyQuitOnce sync.Once
+)
+
+// TerminateLivePtys synchronously terminates the process trees and console
+// hosts of all ptys whose teardown hasn't finished yet. Call it when lazygit
+// is about to exit: the asynchronous teardowns in Close won't get to finish
+// (the conhost rundown wait outlives the process), and while
+// KILL_ON_JOB_CLOSE reaps the clients when the job handles are closed at
+// process death, nothing would reap the conhosts on the Windows builds that
+// need it (see Close). A long diff on screen keeps its git process running
+// the whole time it is shown, so quitting with such a teardown in flight is
+// the rule, not the exception.
+func TerminateLivePtys() {
+	ptyQuitOnce.Do(func() { close(ptyQuit) })
+
+	done := make(chan struct{})
+	go utils.Safe(func() {
+		ptyTeardowns.Wait()
+		close(done)
+	})
+	select {
+	case <-done:
+	case <-time.After(2 * time.Second):
+		// Don't hold up the exit any longer; the job handles' rundown
+		// still covers the clients.
+	}
+}
+
 // Close tears the pty down without waiting for it: the teardown runs on a
 // background goroutine and Close returns immediately.
 //
@@ -127,8 +163,14 @@ const conhostExitTimeout = time.Second
 // 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.
+//
+// When lazygit is quitting, the conhost rundown wait is skipped; see
+// TerminateLivePtys.
 func (p *winPty) Close() error {
+	ptyTeardowns.Add(1)
 	go utils.Safe(func() {
+		defer ptyTeardowns.Done()
+
 		p.inWrite.Close()
 		p.outRead.Close()
 		go utils.Safe(p.closeHpc)
@@ -137,7 +179,13 @@ func (p *winPty) Close() error {
 		_ = windows.CloseHandle(p.job)
 
 		if p.conhost != 0 {
-			event, err := windows.WaitForSingleObject(p.conhost, uint32(conhostExitTimeout/time.Millisecond))
+			timeout := conhostExitTimeout
+			select {
+			case <-ptyQuit:
+				timeout = 0
+			default:
+			}
+			event, err := windows.WaitForSingleObject(p.conhost, uint32(timeout/time.Millisecond))
 			if err != nil || event != windows.WAIT_OBJECT_0 {
 				_ = windows.TerminateProcess(p.conhost, 1)
 			}

pkg/commands/oscommands/pty_windows_test.go

@@ -43,6 +43,38 @@ func TestStartPtyIdentifiesConhost(t *testing.T) {
 	_ = sp.Pty.Close()
 }
 
+// TerminateLivePtys must reap a still-running pty synchronously: it runs
+// when lazygit is about to exit, where the asynchronous teardown would not
+// get to finish. Note that it switches the package's pty teardowns into
+// quit mode for the remainder of the test binary's lifetime; that's fine
+// for the other tests here, which must hold in either mode (quit mode only
+// shortens the teardown's conhost rundown wait).
+func TestTerminateLivePtysReapsRunningPty(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()
+	TerminateLivePtys()
+
+	// The teardown has completed as part of TerminateLivePtys, so the child
+	// must be gone already; the timeout is generosity, not a grace period.
+	exited := make(chan struct{})
+	go func() {
+		_ = sp.Wait()
+		close(exited)
+	}()
+	select {
+	case <-exited:
+	case <-time.After(time.Second):
+		t.Fatal("child process was not terminated by TerminateLivePtys")
+	}
+}
+
 // 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

pkg/gui/gui.go

@@ -1005,6 +1005,12 @@ func (gui *Gui) RunAndHandleError(startArgs appTypes.StartArgs) error {
 				manager.Close()
 			}
 
+			// The pty teardowns spawned by the manager closes above run on
+			// background goroutines that won't get to finish before the
+			// process exits; reap their process trees synchronously instead
+			// so that they don't outlive lazygit.
+			oscommands.TerminateLivePtys()
+
 			close(gui.stopChan)
 
 			if errors.Is(err, gocui.ErrQuit) {