Visitar URL original
Reap pty process trees synchronously when quitting on Windows (#5879) · jesseduffield/lazygit@d251fad · GitHub
Skip to content

Commit d251fad

Browse files
stefanhallerclaude
andcommitted
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>
1 parent b4dee2a commit d251fad

4 files changed

Lines changed: 93 additions & 1 deletion

File tree

‎pkg/commands/oscommands/pty_unix.go‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,3 +32,9 @@ func StartPty(cmd *exec.Cmd, cols, rows uint16) (StartedPty, error) {
3232
Wait: cmd.Wait,
3333
}, nil
3434
}
35+
36+
// TerminateLivePtys is a no-op on Unix: stopping a pty task signals the
37+
// child (SIGTERM, plus SIGHUP to the foreground process group when the
38+
// master closes), and the processes clean themselves up without lazygit
39+
// having to wait for them.
40+
func TerminateLivePtys() {}

‎pkg/commands/oscommands/pty_windows.go‎

Lines changed: 49 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -77,6 +77,42 @@ func (p *winPty) closeHpc() {
7777
// are gone, before concluding that it never will (see Close) and reaping it.
7878
const conhostExitTimeout = time.Second
7979

80+
var (
81+
// ptyTeardowns counts the in-flight teardown goroutines spawned by
82+
// Close; TerminateLivePtys waits for them when lazygit exits.
83+
ptyTeardowns sync.WaitGroup
84+
// ptyQuit is closed by TerminateLivePtys. In-flight teardowns skip the
85+
// conhost rundown wait once it is closed: the conhost serves nothing
86+
// once its clients are gone, and the exit must not stall for its sake.
87+
ptyQuit = make(chan struct{})
88+
ptyQuitOnce sync.Once
89+
)
90+
91+
// TerminateLivePtys synchronously terminates the process trees and console
92+
// hosts of all ptys whose teardown hasn't finished yet. Call it when lazygit
93+
// is about to exit: the asynchronous teardowns in Close won't get to finish
94+
// (the conhost rundown wait outlives the process), and while
95+
// KILL_ON_JOB_CLOSE reaps the clients when the job handles are closed at
96+
// process death, nothing would reap the conhosts on the Windows builds that
97+
// need it (see Close). A long diff on screen keeps its git process running
98+
// the whole time it is shown, so quitting with such a teardown in flight is
99+
// the rule, not the exception.
100+
func TerminateLivePtys() {
101+
ptyQuitOnce.Do(func() { close(ptyQuit) })
102+
103+
done := make(chan struct{})
104+
go utils.Safe(func() {
105+
ptyTeardowns.Wait()
106+
close(done)
107+
})
108+
select {
109+
case <-done:
110+
case <-time.After(2 * time.Second):
111+
// Don't hold up the exit any longer; the job handles' rundown
112+
// still covers the clients.
113+
}
114+
}
115+
80116
// Close tears the pty down without waiting for it: the teardown runs on a
81117
// background goroutine and Close returns immediately.
82118
//
@@ -127,8 +163,14 @@ const conhostExitTimeout = time.Second
127163
// exactly the clients the job kill is for — and such a conhost sits
128164
// around forever, serving nothing (#5879). The reap is inert on healthy
129165
// builds: the wait succeeds and only the handle is closed.
166+
//
167+
// When lazygit is quitting, the conhost rundown wait is skipped; see
168+
// TerminateLivePtys.
130169
func (p *winPty) Close() error {
170+
ptyTeardowns.Add(1)
131171
go utils.Safe(func() {
172+
defer ptyTeardowns.Done()
173+
132174
p.inWrite.Close()
133175
p.outRead.Close()
134176
go utils.Safe(p.closeHpc)
@@ -137,7 +179,13 @@ func (p *winPty) Close() error {
137179
_ = windows.CloseHandle(p.job)
138180

139181
if p.conhost != 0 {
140-
event, err := windows.WaitForSingleObject(p.conhost, uint32(conhostExitTimeout/time.Millisecond))
182+
timeout := conhostExitTimeout
183+
select {
184+
case <-ptyQuit:
185+
timeout = 0
186+
default:
187+
}
188+
event, err := windows.WaitForSingleObject(p.conhost, uint32(timeout/time.Millisecond))
141189
if err != nil || event != windows.WAIT_OBJECT_0 {
142190
_ = windows.TerminateProcess(p.conhost, 1)
143191
}

‎pkg/commands/oscommands/pty_windows_test.go‎

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,38 @@ func TestStartPtyIdentifiesConhost(t *testing.T) {
4343
_ = sp.Pty.Close()
4444
}
4545

46+
// TerminateLivePtys must reap a still-running pty synchronously: it runs
47+
// when lazygit is about to exit, where the asynchronous teardown would not
48+
// get to finish. Note that it switches the package's pty teardowns into
49+
// quit mode for the remainder of the test binary's lifetime; that's fine
50+
// for the other tests here, which must hold in either mode (quit mode only
51+
// shortens the teardown's conhost rundown wait).
52+
func TestTerminateLivePtysReapsRunningPty(t *testing.T) {
53+
// The output redirect is there for the reason described in
54+
// TestStartPtyWithZeroSize.
55+
sp, err := StartPty(exec.Command("cmd", "/c", "ping -n 30 127.0.0.1 >nul"), 80, 24)
56+
assert.NoError(t, err)
57+
if err != nil {
58+
return
59+
}
60+
61+
_ = sp.Pty.Close()
62+
TerminateLivePtys()
63+
64+
// The teardown has completed as part of TerminateLivePtys, so the child
65+
// must be gone already; the timeout is generosity, not a grace period.
66+
exited := make(chan struct{})
67+
go func() {
68+
_ = sp.Wait()
69+
close(exited)
70+
}()
71+
select {
72+
case <-exited:
73+
case <-time.After(time.Second):
74+
t.Fatal("child process was not terminated by TerminateLivePtys")
75+
}
76+
}
77+
4678
// Closing the pty must terminate the process tree it was running, even when
4779
// it is closed so soon after starting that the child hasn't attached to the
4880
// pseudoconsole yet: such a child misses the CTRL_CLOSE_EVENT that the close

‎pkg/gui/gui.go‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1005,6 +1005,12 @@ func (gui *Gui) RunAndHandleError(startArgs appTypes.StartArgs) error {
10051005
manager.Close()
10061006
}
10071007

1008+
// The pty teardowns spawned by the manager closes above run on
1009+
// background goroutines that won't get to finish before the
1010+
// process exits; reap their process trees synchronously instead
1011+
// so that they don't outlive lazygit.
1012+
oscommands.TerminateLivePtys()
1013+
10081014
close(gui.stopChan)
10091015

10101016
if errors.Is(err, gocui.ErrQuit) {

0 commit comments

Comments
 (0)