jesseduffield / jesseduffield/lazygit

Lazygit's PTY pager stop path only kills the direct child, not the process group, which can leave zombie/orphan pager processes

Open
#5,675 5 comments 0 reactions 0 assignees View on GitHub
Dominant language
Go
Stars
82.4k
Forks
3k
Avg merge
2d 18h
Merged PRs (30d)
19

Description

### Describe the bug

Lazygit delegates some git output to a pager (commonly `less`, but also any user-configured pager) via a PTY. When the user navigates away, lazygit closes the PTY master and waits for the direct child process to exit ([jesseduffield/lazygit#4782](https://github.com/jesseduffield/lazygit/pull/4782)). However, this assumes the pager correctly observes a closed controlling terminal and exits promptly. Not all pagers do.

An upstream bug in `less` ([gwsw/less#793](https://github.com/gwsw/less/issues/793), PR [gwsw/less#794](https://github.com/gwsw/less/pull/794)) demonstrates that `less` can spin at 100% CPU instead of exiting when its tty fd returns EOF, because `getchr()` loops forever without handling `read() == 0`. Even if a fix is merged upstream, most Linux distributions will not ship an updated `less` package for months. More importantly, lazygit supports arbitrary user-configured pagers (`core.pager`, `PAGER`, or `GIT_PAGER`), and lazygit cannot rely on every pager being bug-free.

Today lazygit's stop path:

1. Calls `TerminateProcessGracefully(cmd)`, which on non-Windows sends `SIGTERM` only to the direct `cmd.Process`.
2. Closes the PTY master (`ptmx.Close()`).
3. Waits for `cmd.Wait()` in a goroutine (or synchronously in some paths).

If the pager ignores `SIGTERM` and does not notice the closed PTY, it can survive as an orphan, consuming CPU and holding any `LESSOPEN` preprocessor children alive. For `less` specifically this is a known failure mode; for other pagers it is an unvalidated risk.

### To Reproduce

1. Set `core.pager=less` (default on many systems) or any pager that may not exit cleanly on closed PTY.
2. Open a file/diff large enough to trigger the pager inside lazygit's panel.
3. Navigate away so lazygit closes the PTY and signals the direct child.
4. Observe (e.g. via `ps`) whether the pager process (and any `LESSOPEN` child processes) remain.
With current `less` versions affected by `gwsw/less#793`, the process may spin at 100% CPU.

### Expected behavior

When lazygit closes a PTY, **all processes in that PTY's session should be reliably terminated**, regardless of whether the pager correctly handles a closed terminal. Lazygit should not depend on the pager being well-behaved.

### Screenshots

N/A

### Version info

Reproducible on current `master` and any recent release that uses PTY-based pagers.

### Terminal info

Any terminal / OS where lazygit spawns pagers via PTY (not Windows, where PTY pager path is different).

### Additional context

Lazygit already improved graceful termination in [jesseduffield/lazygit#4782](https://github.com/jesseduffield/lazygit/pull/4782) to reduce stale `index.lock`. The remaining gap is that `TerminateProcessGracefully` only signals the direct child process, not the process group. When `creack/pty`'s `StartWithSize` starts the process in a new session (`Setsid` and `Setctty`), the child's PID is also its process-group ID. Lazygit could send `SIGHUP` to the process group (`kill(-cmd.Process.Pid, syscall.SIGHUP)`), close the PTY, then wait with a short timeout (e.g. 500 ms), and finally `SIGKILL` the group if still alive. This should be limited to non-Windows platforms where process-group signals are meaningful.

This is a **lazygit-side reliability bug**: the stop path trusts the pager to exit, but that trust is not safe for arbitrary pagers or lagging distro packages.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.