diff --git a/pkg/commands/oscommands/pty_unix.go b/pkg/commands/oscommands/pty_unix.go index 6cf63cdff..cd3962a8e 100644 --- a/pkg/commands/oscommands/pty_unix.go +++ b/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() {} diff --git a/pkg/commands/oscommands/pty_windows.go b/pkg/commands/oscommands/pty_windows.go index 853fe1fae..e645ae627 100644 --- a/pkg/commands/oscommands/pty_windows.go +++ b/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) } diff --git a/pkg/commands/oscommands/pty_windows_test.go b/pkg/commands/oscommands/pty_windows_test.go index 46a92babc..e6581a85d 100644 --- a/pkg/commands/oscommands/pty_windows_test.go +++ b/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 diff --git a/pkg/gui/gui.go b/pkg/gui/gui.go index 7713cbd57..4dcd3c209 100644 --- a/pkg/gui/gui.go +++ b/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) {