Improve index.lock retry mechanism (#5788)

In v0.63.0 we made a change to no longer use `GIT_OPTIONAL_LOCKS=0` on
git commands that are part of a "foreground" refresh, meaning the
refresh after a lazygit command or the focus-in refresh. We do this on
purpose to keep git's mod date cache from becoming stale, which could
make lazygit become slower over time. However, this caused a problem for
users who work very fast: staging a file and then immediately pressing
shift-A to amend while the staging's refresh is still running would show
the dreaded index.lock error.

We already had a retry-on-index-lock-error mechanism in place, but it
wasn't used for commands like amend or commit; fix this so that the
retry loop works for these too, and also make the retry window a little
longer, and fix a problem where it wouldn't work in linked worktrees or
submodules.

Closes #5778.

pkg/commands/git_cmd_obj_builder.go

@@ -25,8 +25,9 @@ func NewGitCmdObjBuilder(log *logrus.Entry, innerBuilder *oscommands.CmdObjBuild
 	// the price of having a convenient interface where we can say .New(...).Run() is that our builder now depends on our runner, so when we want to wrap the default builder/runner in new functionality we need to jump through some hoops. We could avoid the use of a decorator function here by just exporting the runner field on the default builder but that would be misleading because we don't want anybody using that to run commands (i.e. we want there to be a single API used across the codebase)
 	updatedBuilder := innerBuilder.CloneWithNewRunner(func(runner oscommands.ICmdObjRunner) oscommands.ICmdObjRunner {
 		return &gitCmdObjRunner{
-			log:         log,
-			innerRunner: runner,
+			log:               log,
+			innerRunner:       runner,
+			initialRetryDelay: defaultInitialRetryDelay,
 		}
 	})
 

pkg/commands/git_cmd_obj_runner.go

@@ -11,20 +11,42 @@ import (
 // here we're wrapping the default command runner in some git-specific stuff e.g. retry logic if we get an error due to the presence of .git/index.lock
 
 const (
-	WaitTime   = 50 * time.Millisecond
-	RetryCount = 5
+	// defaultInitialRetryDelay is how long we wait before the first retry of a
+	// command that failed with a transient lock error. We double it before each
+	// subsequent retry (see retryOnLockError), so across maxRetries attempts we
+	// wait for a bit over a second in total. That's long enough to outlast the
+	// brief window during which another git process holds a lock we need —
+	// typically our own foreground `git status` refresh, which takes index.lock
+	// to persist its refreshed stat-cache.
+	defaultInitialRetryDelay = 20 * time.Millisecond
+	maxRetries               = 7
 )
 
 type gitCmdObjRunner struct {
 	log         *logrus.Entry
 	innerRunner oscommands.ICmdObjRunner
+	// initialRetryDelay is the wait before the first lock-error retry. It's a
+	// field rather than the constant directly so tests can set it to zero and
+	// not actually sleep.
+	initialRetryDelay time.Duration
 }
 
-// isRetryableError returns true if the error output indicates a transient
-// lock-related error that may succeed on retry
-func isRetryableError(output string) bool {
-	return strings.Contains(output, ".git/index.lock") ||
-		strings.Contains(output, "cannot lock ref")
+// isRetryableError returns true if a failed command hit a transient
+// lock-related condition that may succeed on retry. The lock message can reach
+// us either in the command's captured output or, for streamed commands whose
+// output we don't capture, only in the returned error, so we check both.
+//
+// We match the bare "index.lock" fragment rather than a fuller path or message
+// so we catch the lock wherever git puts it: the main .git dir, a linked
+// worktree's git dir (.git/worktrees/<name>/index.lock), or a submodule's git
+// dir.
+func isRetryableError(output string, err error) bool {
+	text := output
+	if err != nil {
+		text += "\n" + err.Error()
+	}
+	return strings.Contains(text, "index.lock") ||
+		strings.Contains(text, "cannot lock ref")
 }
 
 func (self *gitCmdObjRunner) Run(cmdObj *oscommands.CmdObj) error {
@@ -33,41 +55,44 @@ func (self *gitCmdObjRunner) Run(cmdObj *oscommands.CmdObj) error {
 }
 
 func (self *gitCmdObjRunner) RunWithOutput(cmdObj *oscommands.CmdObj) (string, error) {
-	var output string
-	var err error
-	for range RetryCount {
-		newCmdObj := cmdObj.Clone()
-		output, err = self.innerRunner.RunWithOutput(newCmdObj)
-
-		if err == nil || !isRetryableError(output) {
-			return output, err
-		}
-
-		// if we have an error based on a lock, we should wait a bit and then retry
-		self.log.Warn("lock error prevented command from running. Retrying command after a small wait")
-		time.Sleep(WaitTime)
-	}
-
-	return output, err
+	return self.retryOnLockError(func() (string, error) {
+		return self.innerRunner.RunWithOutput(cmdObj.Clone())
+	})
 }
 
 func (self *gitCmdObjRunner) RunWithOutputs(cmdObj *oscommands.CmdObj) (string, string, error) {
 	var stdout, stderr string
-	var err error
-	for range RetryCount {
-		newCmdObj := cmdObj.Clone()
-		stdout, stderr, err = self.innerRunner.RunWithOutputs(newCmdObj)
+	_, err := self.retryOnLockError(func() (string, error) {
+		var runErr error
+		stdout, stderr, runErr = self.innerRunner.RunWithOutputs(cmdObj.Clone())
+		return stdout + stderr, runErr
+	})
+	return stdout, stderr, err
+}
 
-		if err == nil || !isRetryableError(stdout+stderr) {
-			return stdout, stderr, err
+// retryOnLockError runs the given function, retrying if it fails with a
+// transient lock error (see isRetryableError). The string returned by run is
+// the command output we inspect to classify the failure. We clone the command
+// for each attempt (inside run) because an *exec.Cmd can only be run once.
+func (self *gitCmdObjRunner) retryOnLockError(run func() (string, error)) (string, error) {
+	delay := self.initialRetryDelay
+	var output string
+	var err error
+	for attempt := range maxRetries {
+		output, err = run()
+
+		if err == nil || !isRetryableError(output, err) {
+			break
 		}
 
-		// if we have an error based on a lock, we should wait a bit and then retry
-		self.log.Warn("lock error prevented command from running. Retrying command after a small wait")
-		time.Sleep(WaitTime)
+		if attempt < maxRetries-1 {
+			self.log.Warnf("lock error prevented command from running; retrying in %s", delay)
+			time.Sleep(delay)
+			delay *= 2
+		}
 	}
 
-	return stdout, stderr, err
+	return output, err
 }
 
 // Retry logic not implemented here, but these commands typically don't need to obtain a lock.

pkg/commands/git_cmd_obj_runner_test.go

@@ -0,0 +1,137 @@
+package commands
+
+import (
+	"errors"
+	"testing"
+
+	"github.com/jesseduffield/lazygit/pkg/commands/oscommands"
+	"github.com/jesseduffield/lazygit/pkg/utils"
+	"github.com/stretchr/testify/assert"
+)
+
+type runnerResult struct {
+	output string
+	err    error
+}
+
+// scriptedRunner is an ICmdObjRunner stub that returns a preconfigured result
+// for each successive call, letting us drive the retry loop deterministically.
+// It counts calls so tests can assert whether a command was retried.
+type scriptedRunner struct {
+	results []runnerResult
+	calls   int
+}
+
+func (self *scriptedRunner) next() (string, error) {
+	result := self.results[self.calls]
+	self.calls++
+	return result.output, result.err
+}
+
+func (self *scriptedRunner) Run(*oscommands.CmdObj) error {
+	_, err := self.next()
+	return err
+}
+
+func (self *scriptedRunner) RunWithOutput(*oscommands.CmdObj) (string, error) {
+	return self.next()
+}
+
+func (self *scriptedRunner) RunWithOutputs(*oscommands.CmdObj) (string, string, error) {
+	output, err := self.next()
+	return output, "", err
+}
+
+func (self *scriptedRunner) RunAndProcessLines(*oscommands.CmdObj, func(string) (bool, error)) error {
+	panic("not implemented")
+}
+
+func newTestRunner(inner *scriptedRunner) *gitCmdObjRunner {
+	return &gitCmdObjRunner{
+		log:         utils.NewDummyLog(),
+		innerRunner: inner,
+		// don't actually sleep between retries
+		initialRetryDelay: 0,
+	}
+}
+
+// dummyCmdObj returns a throwaway command; only its clonability matters, since
+// the scriptedRunner ignores it and returns preconfigured results.
+func dummyCmdObj() *oscommands.CmdObj {
+	return oscommands.NewDummyCmdObjBuilder(nil).New([]string{"git", "status"})
+}
+
+func TestRunWithOutputReturnsSuccessWithoutRetrying(t *testing.T) {
+	inner := &scriptedRunner{results: []runnerResult{{output: "done", err: nil}}}
+
+	output, err := newTestRunner(inner).RunWithOutput(dummyCmdObj())
+
+	assert.NoError(t, err)
+	assert.Equal(t, "done", output)
+	assert.Equal(t, 1, inner.calls)
+}
+
+func TestRunWithOutputDoesNotRetryNonLockError(t *testing.T) {
+	inner := &scriptedRunner{results: []runnerResult{{output: "boom", err: errors.New("boom")}}}
+
+	_, err := newTestRunner(inner).RunWithOutput(dummyCmdObj())
+
+	assert.Error(t, err)
+	assert.Equal(t, 1, inner.calls)
+}
+
+func TestRunWithOutputRetriesWhenLockErrorIsInOutput(t *testing.T) {
+	inner := &scriptedRunner{results: []runnerResult{
+		{output: "fatal: Unable to create '/repo/.git/index.lock': File exists.", err: errors.New("exit status 128")},
+		{output: "done", err: nil},
+	}}
+
+	output, err := newTestRunner(inner).RunWithOutput(dummyCmdObj())
+
+	assert.NoError(t, err)
+	assert.Equal(t, "done", output)
+	assert.Equal(t, 2, inner.calls)
+}
+
+func TestRunWithOutputRetriesWhenLockErrorIsOnlyInError(t *testing.T) {
+	// A streamed command (e.g. an amend run through the gpg helper) doesn't
+	// capture its output, so a lock failure surfaces only in the returned error
+	// with an empty output string. The retry logic must still recognize it.
+	inner := &scriptedRunner{results: []runnerResult{
+		{output: "", err: errors.New("fatal: Unable to create '/repo/.git/index.lock': File exists.")},
+		{output: "", err: nil},
+	}}
+
+	_, err := newTestRunner(inner).RunWithOutput(dummyCmdObj())
+
+	assert.NoError(t, err)
+	assert.Equal(t, 2, inner.calls)
+}
+
+func TestRunWithOutputGivesUpAfterMaxRetries(t *testing.T) {
+	results := make([]runnerResult, maxRetries)
+	for i := range results {
+		results[i] = runnerResult{err: errors.New("fatal: Unable to create '/repo/.git/index.lock': File exists.")}
+	}
+	inner := &scriptedRunner{results: results}
+
+	_, err := newTestRunner(inner).RunWithOutput(dummyCmdObj())
+
+	assert.Error(t, err)
+	assert.Equal(t, maxRetries, inner.calls)
+}
+
+func TestRunWithOutputRetriesLockErrorInLinkedWorktree(t *testing.T) {
+	// In a linked worktree the lock lives at .git/worktrees/<name>/index.lock
+	// rather than .git/index.lock, so only matching the bare "index.lock"
+	// fragment lets the retry fire there too.
+	inner := &scriptedRunner{results: []runnerResult{
+		{output: "", err: errors.New("fatal: Unable to create '/repo/.git/worktrees/feature/index.lock': File exists.")},
+		{output: "", err: nil},
+	}}
+
+	_, err := newTestRunner(inner).RunWithOutput(dummyCmdObj())
+
+	assert.NoError(t, err)
+	assert.Equal(t, 2, inner.calls)
+}