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 1bfb29ab5..e645ae627 100644 --- a/pkg/commands/oscommands/pty_windows.go +++ b/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" @@ -12,7 +14,14 @@ 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 + // 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 @@ -64,6 +73,46 @@ 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 + +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. // @@ -83,11 +132,65 @@ 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. +// +// 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. +// +// 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() - p.closeHpc() + go utils.Safe(p.closeHpc) + + _ = windows.TerminateJobObject(p.job, 1) + _ = windows.CloseHandle(p.job) + + if p.conhost != 0 { + 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) + } + _ = windows.CloseHandle(p.conhost) + } }) return nil } @@ -123,6 +226,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 @@ -148,9 +297,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) @@ -160,9 +324,40 @@ 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) + } } }() + // 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 +416,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 +425,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 +456,8 @@ 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"), } diff --git a/pkg/commands/oscommands/pty_windows_test.go b/pkg/commands/oscommands/pty_windows_test.go index 0b4173561..e6581a85d 100644 --- a/pkg/commands/oscommands/pty_windows_test.go +++ b/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,82 @@ func TestStartPtyWithZeroSize(t *testing.T) { _ = sp.Pty.Close() } } + +// 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() +} + +// 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 +// 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") + } +} 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) { diff --git a/pkg/gui/pty.go b/pkg/gui/pty.go index 2184621ae..719f5c348 100644 --- a/pkg/gui/pty.go +++ b/pkg/gui/pty.go @@ -49,9 +49,9 @@ type ptyCmd struct { wait func() error } -func (p ptyCmd) Wait() error { return p.wait() } -func (p ptyCmd) String() string { return p.cmd.String() } -func (p ptyCmd) GetProcess() *os.Process { return p.process } +func (p ptyCmd) Wait() error { return p.wait() } +func (p ptyCmd) String() string { return p.cmd.String() } +func (p ptyCmd) Terminate() error { return oscommands.TerminateProcessGracefully(p.process) } // Some commands need to output for a terminal to active certain behaviour. // For example, git won't invoke the GIT_PAGER env var unless it thinks it's diff --git a/pkg/tasks/tasks.go b/pkg/tasks/tasks.go index 3a964c838..3768e0c19 100644 --- a/pkg/tasks/tasks.go +++ b/pkg/tasks/tasks.go @@ -4,7 +4,6 @@ import ( "bufio" "fmt" "io" - "os" "os/exec" "sync" "sync/atomic" @@ -24,7 +23,9 @@ import ( type Cmd interface { Wait() error String() string - GetProcess() *os.Process + // Terminate makes the process stop early, as gracefully as the platform + // allows. It doesn't wait for the process to exit. + Terminate() error } // ExecCmd adapts *exec.Cmd to Cmd. @@ -32,8 +33,11 @@ type ExecCmd struct { *exec.Cmd } -func (c ExecCmd) GetProcess() *os.Process { - return c.Process +// Terminate sends SIGTERM on Unix. On Windows it does nothing, so a stopped +// command keeps running until it next writes to its (by then closed) output +// pipe. +func (c ExecCmd) Terminate() error { + return oscommands.TerminateProcessGracefully(c.Process) } // This file revolves around running commands that will be output to the main panel @@ -213,10 +217,7 @@ func (self *ViewBufferManager) NewCmdTask(start func() (Cmd, io.Reader), prefix // when flicking through several very long diffs when diff.algorithm = histogram is // being used, in which case multiple git processes continue to calculate expensive // diffs in the background even though they have been stopped already. - // - // Unfortunately this will do nothing on Windows, so Windows users will have to live - // with the higher CPU usage. - if err := oscommands.TerminateProcessGracefully(cmd.GetProcess()); err != nil { + if err := cmd.Terminate(); err != nil { self.Log.Errorf("error when trying to terminate cmd task: %v; Command: %v", err, cmd.String()) }