From d52c0a604f830ffac5340aaac662bf575cd64690 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Sun, 2 Aug 2026 15:38:57 +0200 Subject: [PATCH 1/4] Give task commands a Terminate method instead of exposing their process The task stop path terminates the still-running command by pulling its *os.Process out of the Cmd interface and applying one global strategy (TerminateProcessGracefully) to it. That shape can't accommodate the upcoming fix for orphaned process trees on Windows: there, stopping a pty task requires terminating the entire process tree via a job object whose handle lives with the pty, not with the process. And the two Cmd implementations genuinely need different strategies anyway: a process-group kill (the likely future fix for #5675 on Unix) is only safe for pty children, which run as session leaders, while plain commands share lazygit's own process group. So let each Cmd implementation decide how to terminate itself, and drop GetProcess, which had no other callers. No change in behavior. Co-Authored-By: Claude Fable 5 --- pkg/gui/pty.go | 6 +++--- pkg/tasks/tasks.go | 17 +++++++++-------- 2 files changed, 12 insertions(+), 11 deletions(-) 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()) } From 6048fee49d152da28c07bfc867d797a205945266 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Mon, 3 Aug 2026 18:58:23 +0200 Subject: [PATCH 2/4] 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 --- pkg/commands/oscommands/pty_windows.go | 79 ++++++++++++++++++++- pkg/commands/oscommands/pty_windows_test.go | 30 ++++++++ 2 files changed, 106 insertions(+), 3 deletions(-) diff --git a/pkg/commands/oscommands/pty_windows.go b/pkg/commands/oscommands/pty_windows.go index 1bfb29ab5..dcc6ab1a3 100644 --- a/pkg/commands/oscommands/pty_windows.go +++ b/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"), } diff --git a/pkg/commands/oscommands/pty_windows_test.go b/pkg/commands/oscommands/pty_windows_test.go index 0b4173561..688393843 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,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") + } +} From b4dee2af0eb247060200b64f244c95a7711a0b03 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Mon, 3 Aug 2026 14:23:08 +0200 Subject: [PATCH 3/4] 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 --- pkg/commands/oscommands/pty_windows.go | 98 ++++++++++++++++++++- pkg/commands/oscommands/pty_windows_test.go | 18 ++++ 2 files changed, 113 insertions(+), 3 deletions(-) diff --git a/pkg/commands/oscommands/pty_windows.go b/pkg/commands/oscommands/pty_windows.go index dcc6ab1a3..853fe1fae 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" @@ -15,7 +17,11 @@ type winPty struct { 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 + 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 @@ -67,6 +73,10 @@ 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 + // Close tears the pty down without waiting for it: the teardown runs on a // background goroutine and Close returns immediately. // @@ -108,6 +118,15 @@ func (p *winPty) closeHpc() { // 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. func (p *winPty) Close() error { go utils.Safe(func() { p.inWrite.Close() @@ -116,6 +135,14 @@ func (p *winPty) Close() error { _ = windows.TerminateJobObject(p.job, 1) _ = windows.CloseHandle(p.job) + + if p.conhost != 0 { + event, err := windows.WaitForSingleObject(p.conhost, uint32(conhostExitTimeout/time.Millisecond)) + if err != nil || event != windows.WAIT_OBJECT_0 { + _ = windows.TerminateProcess(p.conhost, 1) + } + _ = windows.CloseHandle(p.conhost) + } }) return nil } @@ -151,6 +178,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 @@ -176,9 +249,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) @@ -188,6 +276,9 @@ 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) + } } }() @@ -318,6 +409,7 @@ 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 688393843..46a92babc 100644 --- a/pkg/commands/oscommands/pty_windows_test.go +++ b/pkg/commands/oscommands/pty_windows_test.go @@ -25,6 +25,24 @@ func TestStartPtyWithZeroSize(t *testing.T) { } } +// 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() +} + // 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 From d251faddec3311051eafaf48c767389cd02a00c2 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Mon, 3 Aug 2026 19:04:03 +0200 Subject: [PATCH 4/4] 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 --- pkg/commands/oscommands/pty_unix.go | 6 +++ pkg/commands/oscommands/pty_windows.go | 50 ++++++++++++++++++++- pkg/commands/oscommands/pty_windows_test.go | 32 +++++++++++++ pkg/gui/gui.go | 6 +++ 4 files changed, 93 insertions(+), 1 deletion(-) 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) {