From 46c1fa7db28d1b9399dfb6df2dcce76ab1865755 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Thu, 9 Jul 2026 19:11:33 +0200 Subject: [PATCH 1/6] Don't let a leaked subprocess wedge the headless test runner When lazygit exits but leaves behind a subprocess that inherited its stderr pipe and has detached from the pty, cmd.Wait() blocks in awaitGoroutines waiting for that pipe to reach EOF -- which never happens while the straggler is alive. With no WaitDelay set, that wait is unbounded, so a single leaked process hangs the whole test binary until the 10-minute global timeout fires and panics. Worse, the timeout discards whatever lazygit wrote to stderr before exiting (a panic, a -race report), which is exactly the output needed to diagnose the failure. This surfaces under -race, where lazygit runs slow enough to widen the window for a spawned command to still be alive when lazygit quits, and it's a blocker for enabling the race detector on CI. Bound the wait with cmd.WaitDelay so Wait force-closes the pipe and returns ErrWaitDelay instead of hanging, surface the captured stderr as the error (falling back to the wait error when nothing was printed), and kill the child's process group on failure so a straggler can't linger into a later test or pile up across a run. Co-Authored-By: Claude Opus 4.8 (1M context) --- pkg/integration/clients/go_test.go | 39 +++++++++++++++++++++++++++--- 1 file changed, 36 insertions(+), 3 deletions(-) diff --git a/pkg/integration/clients/go_test.go b/pkg/integration/clients/go_test.go index 11f6e754e..d3f15b88a 100644 --- a/pkg/integration/clients/go_test.go +++ b/pkg/integration/clients/go_test.go @@ -11,7 +11,9 @@ import ( "io" "os" "os/exec" + "syscall" "testing" + "time" "github.com/creack/pty" "github.com/jesseduffield/lazycore/pkg/utils" @@ -75,6 +77,17 @@ func runCmdHeadless(cmd *exec.Cmd) (int, error) { stderr := new(bytes.Buffer) cmd.Stderr = stderr + // If lazygit exits but leaves behind a subprocess that inherited its stderr + // pipe, cmd.Wait blocks waiting for that pipe to reach EOF for as long as the + // subprocess stays alive. Unbounded, that hangs the whole test binary until + // its global timeout fires, and the timeout throws away whatever lazygit + // wrote to stderr before exiting (a panic, a -race report) -- the very output + // needed to diagnose the failure. WaitDelay caps the wait: once the process + // has exited, Wait gives the stderr goroutine at most this long to drain, + // then closes the pipe and returns ErrWaitDelay, so the captured stderr + // surfaces as the test error instead of being lost. + cmd.WaitDelay = 5 * time.Second + // these rows and columns are ignored because internally we use tcell's // simulation screen. However we still need the pty for the sake of // running other commands in a pty. @@ -83,12 +96,32 @@ func runCmdHeadless(cmd *exec.Cmd) (int, error) { return -1, err } + // pty.StartWithSize starts lazygit in its own process group, so we can signal + // the whole group at once. Capture the id now, while the process is alive: + // once Wait has reaped it we can no longer look it up. + pgid, pgidErr := syscall.Getpgid(cmd.Process.Pid) + _, _ = io.Copy(io.Discard, f) - if cmd.Wait() != nil { + waitErr := cmd.Wait() + + // On any failure -- including a WaitDelay expiry caused by a leaked + // subprocess -- kill the whole process group so a straggler can't linger and + // wedge a later test or pile up across a CI run. Best effort: usually the + // group is already gone (ESRCH), and a subprocess that called setsid to + // detach into its own group is out of reach, but WaitDelay still unblocks us. + if waitErr != nil && pgidErr == nil { + _ = syscall.Kill(-pgid, syscall.SIGKILL) + } + + if waitErr != nil { _ = f.Close() - // return an error with the stderr output - return cmd.Process.Pid, errors.New(stderr.String()) + // Prefer lazygit's own stderr as the error; fall back to the wait error + // itself (e.g. ErrWaitDelay) when it exited without printing anything. + if stderr.Len() > 0 { + return cmd.Process.Pid, errors.New(stderr.String()) + } + return cmd.Process.Pid, waitErr } return cmd.Process.Pid, f.Close() From 7fce58b09bc7b9bb61cbd8c3950579121a0bb832 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Fri, 10 Jul 2026 12:55:36 +0200 Subject: [PATCH 2/6] Scale the integration test watchdog up under the race detector The integration test watchdog fails a test if its recording takes longer than 40 seconds. Under the race detector everything runs several times slower, so legitimately slow tests (e.g. a conflicting interactive rebase) blow that budget and fail even though nothing is actually stuck. Key the timeout off a build-tag constant: the `race` tag is set automatically when the binary is built with -race, so a race build gets a 5x-longer budget while a normal build is unchanged, and the two can't drift apart the way a runtime flag would. The base 40s stays in one place; only the multiplier varies by build. Co-Authored-By: Claude Opus 4.8 (1M context) --- pkg/gui/test_mode.go | 5 +++-- pkg/gui/test_timeout_norace.go | 5 +++++ pkg/gui/test_timeout_race.go | 10 ++++++++++ 3 files changed, 18 insertions(+), 2 deletions(-) create mode 100644 pkg/gui/test_timeout_norace.go create mode 100644 pkg/gui/test_timeout_race.go diff --git a/pkg/gui/test_mode.go b/pkg/gui/test_mode.go index d6893c92d..0b24bb8cd 100644 --- a/pkg/gui/test_mode.go +++ b/pkg/gui/test_mode.go @@ -45,9 +45,10 @@ func (gui *Gui) handleTestMode() { }() if os.Getenv(components.WAIT_FOR_DEBUGGER_ENV_VAR) == "" { + timeout := 40 * time.Second * testTimeoutMultiplier go utils.Safe(func() { - time.Sleep(time.Second * 40) - log.Fatal("40 seconds is up, lazygit recording took too long to complete") + time.Sleep(timeout) + log.Fatalf("%v is up, lazygit integration test took too long to complete", timeout) }) } } diff --git a/pkg/gui/test_timeout_norace.go b/pkg/gui/test_timeout_norace.go new file mode 100644 index 000000000..7f924ea71 --- /dev/null +++ b/pkg/gui/test_timeout_norace.go @@ -0,0 +1,5 @@ +//go:build !race + +package gui + +const testTimeoutMultiplier = 1 diff --git a/pkg/gui/test_timeout_race.go b/pkg/gui/test_timeout_race.go new file mode 100644 index 000000000..7d633def3 --- /dev/null +++ b/pkg/gui/test_timeout_race.go @@ -0,0 +1,10 @@ +//go:build race + +package gui + +// The race detector makes everything run several times slower, so the +// recording watchdog needs a correspondingly longer timeout; otherwise it +// fires on tests that are merely slow under -race rather than actually stuck. +// The `race` build tag is set automatically when the binary is built with +// -race, so this can't drift out of sync with the actual build. +const testTimeoutMultiplier = 4 From 334daccfab6e080df95b94678d33e607b106a850 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Fri, 10 Jul 2026 15:29:28 +0200 Subject: [PATCH 3/6] Increase integration test timeout to 30 minutes Go's default 10-minute timeout was enough for running integration tests normally (both locally and on CI), but with race detection turned on they can take much longer to run. Increase the timeout unconditionally to 30 minutes; we don't bother making a distinction between race vs. normal, because a longer timeout doesn't hurt (I can't recall having hit the global timeout ever; and we still have the per-test watchdog that kills an individual test after 40s). --- justfile | 2 +- scripts/run_integration_tests.sh | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/justfile b/justfile index 64d9d1ee2..0851179b9 100644 --- a/justfile +++ b/justfile @@ -40,7 +40,7 @@ lint: ./scripts/gofumpt-check.sh ./scripts/golangci-lint-shim.sh run -e2e-test-command := "go test pkg/integration/clients/*.go" +e2e-test-command := "go test -timeout 30m pkg/integration/clients/*.go" # Run integration tests headlessly: no args runs all tests, a test name (or path) runs just that one. Use e2e-cli for a visible UI. e2e *args: diff --git a/scripts/run_integration_tests.sh b/scripts/run_integration_tests.sh index 579e6d77c..1dadeec97 100755 --- a/scripts/run_integration_tests.sh +++ b/scripts/run_integration_tests.sh @@ -19,7 +19,7 @@ if [ -n "$LAZYGIT_GOCOVERDIR" ]; then # hacky. To capture the coverage data for the test runner we pass the test.gocoverdir positional # arg, but if we do that then the GOCOVERDIR env var (which you typically pass to the test binary) will be overwritten by the test runner. So we're passing LAZYGIT_COCOVERDIR instead # and then internally passing that to the test binary as GOCOVERDIR. - go test -cover -coverpkg=github.com/jesseduffield/lazygit/pkg/... pkg/integration/clients/*.go -args -test.gocoverdir="/tmp/code_coverage" + go test -timeout 30m -cover -coverpkg=github.com/jesseduffield/lazygit/pkg/... pkg/integration/clients/*.go -args -test.gocoverdir="/tmp/code_coverage" EXITCODE=$? # We're merging the coverage data for the sake of having fewer artefacts to upload. @@ -29,7 +29,7 @@ if [ -n "$LAZYGIT_GOCOVERDIR" ]; then rm -rf /tmp/code_coverage mv /tmp/code_coverage_merged /tmp/code_coverage else - go test pkg/integration/clients/*.go + go test -timeout 30m pkg/integration/clients/*.go EXITCODE=$? fi From 840a66e7333ae412003b33cd9ff3799a251e92f9 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Fri, 10 Jul 2026 13:09:40 +0200 Subject: [PATCH 4/6] Run the integration tests under the race detector on CI Add one extra integration-tests job that runs the whole suite under the race detector. A `race` matrix dimension (default false) plus an include entry adds a single git-latest job with LAZYGIT_RACE_DETECTOR set; races live in lazygit's own Go code rather than in git, so one git version is enough, and using latest skips the git-build steps. The race job skips coverage collection: it's redundant with the non-race latest job and would only slow the -race build down further. --- .github/workflows/ci.yml | 22 +++++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 4e7538b0d..d14778483 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -53,8 +53,16 @@ jobs: - 2.38.2 # first version that supports the rebase.updateRefs config - 2.44.0 - latest # We rely on github to have the latest version installed on their VMs + race: + - false + # Additionally run the whole suite once under the race detector. Data + # races live in lazygit's own Go code rather than in git, so a single + # git version is enough; use the latest to skip the git-build steps. + include: + - git-version: latest + race: true runs-on: ubuntu-latest - name: "Integration Tests - git ${{matrix.git-version}}" + name: "Integration Tests - git ${{matrix.git-version}}${{ matrix.race && ' (race)' || '' }}" env: GOFLAGS: -mod=vendor steps: @@ -92,12 +100,20 @@ jobs: run: git --version - name: Test code env: - # See https://go.dev/blog/integration-test-coverage - LAZYGIT_GOCOVERDIR: /tmp/code_coverage + # See https://go.dev/blog/integration-test-coverage. The race variant + # skips coverage: it's redundant with the non-race latest job and + # would only slow the -race build down further. Leaving the dir unset + # makes run_integration_tests.sh take its non-coverage path. + LAZYGIT_GOCOVERDIR: ${{ !matrix.race && '/tmp/code_coverage' || '' }} + # Only set for the race variant. The race detector needs cgo; it's on + # by default on the Linux runner, but we set it explicitly to be safe. + LAZYGIT_RACE_DETECTOR: ${{ matrix.race && '1' || '' }} + CGO_ENABLED: ${{ matrix.race && '1' || '' }} run: | mkdir -p /tmp/code_coverage ./scripts/run_integration_tests.sh - name: Upload code coverage artifacts + if: ${{ !matrix.race }} uses: actions/upload-artifact@v7 with: name: coverage-integration-${{ matrix.git-version }}-${{ github.run_id }} From 03a914c04c713df8b7dbb504f2154185047675cb Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Fri, 10 Jul 2026 17:28:19 +0200 Subject: [PATCH 5/6] Dump goroutine stacks when the test watchdog fires The watchdog only log.Fatal'd with a message, so a hung test told us that it timed out but not where it was stuck -- useless for diagnosing an intermittent deadlock under the race detector. Dump all goroutine stacks to stderr first (the harness surfaces this process's stderr on failure), turning a bare timeout into an actionable stack trace. Co-Authored-By: Claude Opus 4.8 (1M context) --- pkg/gui/test_mode.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/pkg/gui/test_mode.go b/pkg/gui/test_mode.go index 0b24bb8cd..2644e989b 100644 --- a/pkg/gui/test_mode.go +++ b/pkg/gui/test_mode.go @@ -3,6 +3,7 @@ package gui import ( "log" "os" + "runtime/pprof" "time" "github.com/jesseduffield/lazygit/pkg/gocui" @@ -48,6 +49,10 @@ func (gui *Gui) handleTestMode() { timeout := 40 * time.Second * testTimeoutMultiplier go utils.Safe(func() { time.Sleep(timeout) + // Dump all goroutine stacks before dying, so a hung test shows + // where it got stuck rather than just that it timed out. The + // test harness surfaces this process's stderr on failure. + _ = pprof.Lookup("goroutine").WriteTo(os.Stderr, 2) log.Fatalf("%v is up, lazygit integration test took too long to complete", timeout) }) } From b6b5436d571d377af1e487dee871fad101a46a25 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Fri, 10 Jul 2026 17:28:19 +0200 Subject: [PATCH 6/6] Log per-test durations during integration tests To spot slow or anomalous tests across CI runs, record each test's run duration when LAZYGIT_TEST_TIMING is set (to a file path); run_integration_tests.sh prints them at the end, sorted by slowest first. CI sets it for all integration jobs. The harness appends to a file rather than writing to stdout/stderr because `go test` captures those and only surfaces them with -v, which would drown the signal in every test's verbose logs. Co-Authored-By: Claude Opus 4.8 (1M context) --- .github/workflows/ci.yml | 3 +++ pkg/integration/clients/go_test.go | 4 +++- pkg/integration/components/runner.go | 34 ++++++++++++++++++++++++++++ scripts/run_integration_tests.sh | 8 +++++++ 4 files changed, 48 insertions(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index d14778483..c6d2fdf6d 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -109,6 +109,9 @@ jobs: # by default on the Linux runner, but we set it explicitly to be safe. LAZYGIT_RACE_DETECTOR: ${{ matrix.race && '1' || '' }} CGO_ENABLED: ${{ matrix.race && '1' || '' }} + # Append each test's duration to this file; run_integration_tests.sh + # prints the slowest at the end, to spot slow/anomalous tests. + LAZYGIT_TEST_TIMING: /tmp/test_timings.txt run: | mkdir -p /tmp/code_coverage ./scripts/run_integration_tests.sh diff --git a/pkg/integration/clients/go_test.go b/pkg/integration/clients/go_test.go index d3f15b88a..4c1faa557 100644 --- a/pkg/integration/clients/go_test.go +++ b/pkg/integration/clients/go_test.go @@ -30,6 +30,7 @@ func TestIntegration(t *testing.T) { parallelTotal := tryConvert(os.Getenv("PARALLEL_TOTAL"), 1) parallelIndex := tryConvert(os.Getenv("PARALLEL_INDEX"), 0) raceDetector := os.Getenv("LAZYGIT_RACE_DETECTOR") != "" + logTimingsPath := os.Getenv("LAZYGIT_TEST_TIMING") // LAZYGIT_GOCOVERDIR is the directory where we write coverage files to. If this directory // is defined, go binaries built with the -cover flag will write coverage files to // to it. @@ -58,7 +59,8 @@ func TestIntegration(t *testing.T) { CodeCoverageDir: codeCoverageDir, InputDelay: 0, // Allow two attempts at each test to get around flakiness - MaxAttempts: 1, + MaxAttempts: 1, + LogTimingsPath: logTimingsPath, }) assert.NoError(t, err) diff --git a/pkg/integration/components/runner.go b/pkg/integration/components/runner.go index 5640c3e70..78cb5439f 100644 --- a/pkg/integration/components/runner.go +++ b/pkg/integration/components/runner.go @@ -5,6 +5,8 @@ import ( "os" "os/exec" "path/filepath" + "sync" + "time" lazycoreUtils "github.com/jesseduffield/lazycore/pkg/utils" "github.com/jesseduffield/lazygit/pkg/commands/git_commands" @@ -24,6 +26,12 @@ type RunTestArgs struct { CodeCoverageDir string InputDelay int MaxAttempts int + // If set, each test's run duration is appended to this file (as + // " "). run_integration_tests.sh prints the slowest at + // the end, so slow or anomalous tests can be spotted across CI runs. We + // write to a file rather than stdout/stderr because `go test` captures + // those and only shows them with -v. Empty disables it. + LogTimingsPath string } // This function lets you run tests either from within `go test` or from a regular binary. @@ -47,6 +55,11 @@ func RunTests(args RunTestArgs) error { return err } + // Start each run with a fresh timings file (see RunTestArgs.LogTimingsPath). + if args.LogTimingsPath != "" { + _ = os.Remove(args.LogTimingsPath) + } + for _, test := range args.Tests { args.TestWrapper(test, func() error { paths := NewPaths( @@ -99,7 +112,11 @@ func runTest( return err } + start := time.Now() pid, err := args.RunCmd(cmd) + if args.LogTimingsPath != "" { + logTestTiming(args.LogTimingsPath, test.Name(), time.Since(start)) + } // Print race detector log regardless of the command's exit status if args.RaceDetector { @@ -112,6 +129,23 @@ func runTest( return err } +// timingsMutex serializes appends to the timings file, since tests run in +// parallel. +var timingsMutex sync.Mutex + +func logTestTiming(path, name string, duration time.Duration) { + timingsMutex.Lock() + defer timingsMutex.Unlock() + + f, err := os.OpenFile(path, os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0o644) + if err != nil { + return + } + defer f.Close() + + fmt.Fprintf(f, "%.2f %s\n", duration.Seconds(), name) +} + func prepareTestDir( test *IntegrationTest, paths Paths, diff --git a/scripts/run_integration_tests.sh b/scripts/run_integration_tests.sh index 1dadeec97..2bf010f19 100755 --- a/scripts/run_integration_tests.sh +++ b/scripts/run_integration_tests.sh @@ -37,4 +37,12 @@ if test -f ~/.gitconfig.lazygit.bak; then mv ~/.gitconfig.lazygit.bak ~/.gitconfig fi +# If per-test timings were collected (LAZYGIT_TEST_TIMING points at the file the +# harness appends to), print them sorted by slowest first so they show up in the +# CI log. +if [ -n "$LAZYGIT_TEST_TIMING" ] && [ -f "$LAZYGIT_TEST_TIMING" ]; then + echo "Test timings (seconds):" + sort -rn "$LAZYGIT_TEST_TIMING" +fi + exit $EXITCODE