From 3a538fc2a8e2e5cba5dc2e88dcaa3ba1d08e04cc Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Sat, 7 Nov 2026 09:33:12 +0100 Subject: [PATCH] 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 --- pkg/commands/git_cmd_obj_runner.go | 24 ++++-- pkg/commands/git_cmd_obj_runner_test.go | 97 ++++++++++++++++++++++--- pkg/commands/oscommands/cmd_obj.go | 24 ++++++ 3 files changed, 129 insertions(+), 16 deletions(-) diff --git a/pkg/commands/git_cmd_obj_runner.go b/pkg/commands/git_cmd_obj_runner.go index 8112b0f30..4f6bd37ec 100644 --- a/pkg/commands/git_cmd_obj_runner.go +++ b/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 } diff --git a/pkg/commands/git_cmd_obj_runner_test.go b/pkg/commands/git_cmd_obj_runner_test.go index bf938da54..cc6496bde 100644 --- a/pkg/commands/git_cmd_obj_runner_test.go +++ b/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) +} diff --git a/pkg/commands/oscommands/cmd_obj.go b/pkg/commands/oscommands/cmd_obj.go index 2f7e66f7f..dd38fd384 100644 --- a/pkg/commands/oscommands/cmd_obj.go +++ b/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)