Let a git command decide how to recover from a lock error

The git command runner runs a command again if it failed because
another git process held a lock. This is wrong for a command that starts
an operation of several steps, such as a rebase or a cherry-pick of
several commits, if the lock stopped it partway through. Running it
again then fails because the operation is already in progress.

Add a hook that such a command can use to tell the runner what to do
instead. The following commits use it.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

pkg/commands/git_cmd_obj_runner.go

@@ -55,14 +55,14 @@ func (self *gitCmdObjRunner) Run(cmdObj *oscommands.CmdObj) error {
 }
 
 func (self *gitCmdObjRunner) RunWithOutput(cmdObj *oscommands.CmdObj) (string, error) {
-	return self.retryOnLockError(func() (string, error) {
+	return self.retryOnLockError(cmdObj, func(cmdObj *oscommands.CmdObj) (string, error) {
 		return self.innerRunner.RunWithOutput(cmdObj.Clone())
 	})
 }
 
 func (self *gitCmdObjRunner) RunWithOutputs(cmdObj *oscommands.CmdObj) (string, string, error) {
 	var stdout, stderr string
-	_, err := self.retryOnLockError(func() (string, error) {
+	_, err := self.retryOnLockError(cmdObj, func(cmdObj *oscommands.CmdObj) (string, error) {
 		var runErr error
 		stdout, stderr, runErr = self.innerRunner.RunWithOutputs(cmdObj.Clone())
 		return stdout + stderr, runErr
@@ -70,23 +70,33 @@ func (self *gitCmdObjRunner) RunWithOutputs(cmdObj *oscommands.CmdObj) (string,
 	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
+// retryOnLockError runs the given command with the given function, trying
+// again if it fails with a transient lock error (see isRetryableError). If the
+// command has a lock-error recovery (see CmdObj.RecoverFromLockErrorWith), the
+// recovery decides which command to run next. 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) {
+func (self *gitCmdObjRunner) retryOnLockError(cmdObj *oscommands.CmdObj, run func(*oscommands.CmdObj) (string, error)) (string, error) {
 	delay := self.initialRetryDelay
 	var output string
 	var err error
 	for attempt := range maxRetries {
-		output, err = run()
+		output, err = run(cmdObj)
 
 		if err == nil || !isRetryableError(output, err) {
 			break
 		}
 
+		if recovery := cmdObj.GetLockErrorRecovery(); recovery != nil {
+			cmdObj = recovery()
+			if cmdObj == nil {
+				self.log.Warn("lock error stopped command partway through; giving up")
+				break
+			}
+		}
+
 		if attempt < maxRetries-1 {
-			self.log.Warnf("lock error prevented command from running; retrying in %s", delay)
+			self.log.Warnf("lock error prevented command from completing; running %q in %s", cmdObj.ToString(), delay)
 			time.Sleep(delay)
 			delay *= 2
 		}

pkg/commands/git_cmd_obj_runner_test.go

@@ -16,29 +16,32 @@ type runnerResult struct {
 
 // 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.
+// It counts calls and records the commands it ran, so tests can assert whether
+// and how a command was retried.
 type scriptedRunner struct {
 	results []runnerResult
 	calls   int
+	ran     []string
 }
 
-func (self *scriptedRunner) next() (string, error) {
+func (self *scriptedRunner) next(cmdObj *oscommands.CmdObj) (string, error) {
 	result := self.results[self.calls]
 	self.calls++
+	self.ran = append(self.ran, cmdObj.ToString())
 	return result.output, result.err
 }
 
-func (self *scriptedRunner) Run(*oscommands.CmdObj) error {
-	_, err := self.next()
+func (self *scriptedRunner) Run(cmdObj *oscommands.CmdObj) error {
+	_, err := self.next(cmdObj)
 	return err
 }
 
-func (self *scriptedRunner) RunWithOutput(*oscommands.CmdObj) (string, error) {
-	return self.next()
+func (self *scriptedRunner) RunWithOutput(cmdObj *oscommands.CmdObj) (string, error) {
+	return self.next(cmdObj)
 }
 
-func (self *scriptedRunner) RunWithOutputs(*oscommands.CmdObj) (string, string, error) {
-	output, err := self.next()
+func (self *scriptedRunner) RunWithOutputs(cmdObj *oscommands.CmdObj) (string, string, error) {
+	output, err := self.next(cmdObj)
 	return output, "", err
 }
 
@@ -58,9 +61,15 @@ func newTestRunner(inner *scriptedRunner) *gitCmdObjRunner {
 // 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"})
+	return newCmdObj("git", "status")
 }
 
+func newCmdObj(args ...string) *oscommands.CmdObj {
+	return oscommands.NewDummyCmdObjBuilder(nil).New(args)
+}
+
+var errLock = errors.New("fatal: Unable to create '/repo/.git/index.lock': File exists.")
+
 func TestRunWithOutputReturnsSuccessWithoutRetrying(t *testing.T) {
 	inner := &scriptedRunner{results: []runnerResult{{output: "done", err: nil}}}
 
@@ -135,3 +144,73 @@ func TestRunWithOutputRetriesLockErrorInLinkedWorktree(t *testing.T) {
 	assert.NoError(t, err)
 	assert.Equal(t, 2, inner.calls)
 }
+
+func TestRunWithOutputRunsTheCommandReturnedByTheLockErrorRecovery(t *testing.T) {
+	inner := &scriptedRunner{results: []runnerResult{
+		{err: errLock},
+		{err: errLock},
+		{output: "done", err: nil},
+	}}
+	continueCmdObj := newCmdObj("git", "rebase", "--continue")
+	cmdObj := newCmdObj("git", "rebase", "--interactive", "main")
+	cmdObj.RecoverFromLockErrorWith(func() *oscommands.CmdObj { return continueCmdObj })
+
+	output, err := newTestRunner(inner).RunWithOutput(cmdObj)
+
+	assert.NoError(t, err)
+	assert.Equal(t, "done", output)
+	// After switching to the continue command, the runner uses that command's
+	// recovery; it has none, so the runner runs it again.
+	assert.Equal(t, []string{
+		"git rebase --interactive main",
+		"git rebase --continue",
+		"git rebase --continue",
+	}, inner.ran)
+}
+
+func TestRunWithOutputGivesUpWhenTheLockErrorRecoveryReturnsNil(t *testing.T) {
+	inner := &scriptedRunner{results: []runnerResult{{err: errLock}}}
+	cmdObj := dummyCmdObj()
+	cmdObj.RecoverFromLockErrorWith(func() *oscommands.CmdObj { return nil })
+
+	_, err := newTestRunner(inner).RunWithOutput(cmdObj)
+
+	assert.Equal(t, errLock, err)
+	assert.Equal(t, 1, inner.calls)
+}
+
+func TestRunWithOutputCallsTheLockErrorRecoveryAfterTheLastAttempt(t *testing.T) {
+	results := make([]runnerResult, maxRetries)
+	for i := range results {
+		results[i] = runnerResult{err: errLock}
+	}
+	inner := &scriptedRunner{results: results}
+	recoveryCalls := 0
+	cmdObj := dummyCmdObj()
+	cmdObj.RecoverFromLockErrorWith(func() *oscommands.CmdObj {
+		recoveryCalls++
+		return cmdObj
+	})
+
+	_, err := newTestRunner(inner).RunWithOutput(cmdObj)
+
+	assert.Error(t, err)
+	assert.Equal(t, maxRetries, inner.calls)
+	assert.Equal(t, maxRetries, recoveryCalls)
+}
+
+func TestRunWithOutputDoesNotCallTheLockErrorRecoveryForOtherErrors(t *testing.T) {
+	inner := &scriptedRunner{results: []runnerResult{{output: "boom", err: errors.New("boom")}}}
+	recoveryCalls := 0
+	cmdObj := dummyCmdObj()
+	cmdObj.RecoverFromLockErrorWith(func() *oscommands.CmdObj {
+		recoveryCalls++
+		return cmdObj
+	})
+
+	_, err := newTestRunner(inner).RunWithOutput(cmdObj)
+
+	assert.Error(t, err)
+	assert.Equal(t, 1, inner.calls)
+	assert.Equal(t, 0, recoveryCalls)
+}

pkg/commands/oscommands/cmd_obj.go

@@ -37,6 +37,9 @@ type CmdObj struct {
 
 	// can be set so that we don't run certain commands simultaneously
 	mutex *deadlock.Mutex
+
+	// see RecoverFromLockErrorWith()
+	lockErrorRecovery func() *CmdObj
 }
 
 type CredentialStrategy int
@@ -201,6 +204,27 @@ func (self *CmdObj) WithMutex(mutex *deadlock.Mutex) *CmdObj {
 	return self
 }
 
+// RecoverFromLockErrorWith sets what the git command runner does when this
+// command fails because another git process holds a lock (see
+// gitCmdObjRunner.retryOnLockError). Without it, the runner runs the command
+// again. That is wrong for a command that starts an operation of several
+// steps, such as a rebase, if the lock only stopped it partway through.
+//
+// The runner calls recovery after each failed attempt, including the last
+// one, so that recovery can also clean up after the command. It returns the
+// command to run next: this one to run it again, a different one (for
+// example, one that resumes the operation), or nil to give up and return the
+// error.
+func (self *CmdObj) RecoverFromLockErrorWith(recovery func() *CmdObj) *CmdObj {
+	self.lockErrorRecovery = recovery
+
+	return self
+}
+
+func (self *CmdObj) GetLockErrorRecovery() func() *CmdObj {
+	return self.lockErrorRecovery
+}
+
 // runs the command and returns an error if any
 func (self *CmdObj) Run() error {
 	return self.runner.Run(self)