diff --git a/internal/git/git.go b/internal/git/git.go index 680157ec..3d69d5ac 100644 --- a/internal/git/git.go +++ b/internal/git/git.go @@ -199,19 +199,73 @@ func ListTags() ([]string, error) { return parseLines(output), nil } +// pushRetryAttempts is the number of times a rejected push is retried behind a +// rebase before the operation is declared failed. +const pushRetryAttempts = 3 + +// defaultPushBackoff is the delay between push retries when no backoff is set via +// WithBackoff. It gives a concurrent state writer time to settle before the next +// attempt. +const defaultPushBackoff = 2 * time.Second + +// pushOptions holds the tunable behaviour of the rebase-retry push helpers. Its +// zero value reproduces the historical behaviour: git runs in the process working +// directory and retries wait defaultPushBackoff apart. +type pushOptions struct { + dir string + backoff time.Duration +} + +// Option configures the rebase-retry push helpers. Options are additive: a call +// with no options behaves exactly as the original positional API did. +type Option func(*pushOptions) + +// WithDir runs the git commands with cmd.Dir set to dir instead of the process +// working directory. An empty dir (the default) leaves the process working +// directory in effect. This lets a caller drive a repository other than the one +// the process was launched in. +func WithDir(dir string) Option { + return func(o *pushOptions) { o.dir = dir } +} + +// WithBackoff sets the delay between push retries. A zero duration (the default) +// selects defaultPushBackoff. Tests pass a tiny value to keep the retry loop fast. +func WithBackoff(d time.Duration) Option { + return func(o *pushOptions) { o.backoff = d } +} + +func newPushOptions(opts []Option) pushOptions { + var o pushOptions + for _, opt := range opts { + opt(&o) + } + if o.backoff == 0 { + o.backoff = defaultPushBackoff + } + return o +} + // CommitAndPushWithRetry stages filePath, commits it with message, and pushes // to the current branch's upstream, retrying the push up to three times behind a // pull --rebase. A "nothing to commit" state is treated as success (no-op). This // is the manifest state-write path shared by promote and hotfix finalize: an // API-created commit on real GitHub goes through a different path, so this is the // plain-git fallback used when committing locally. -func CommitAndPushWithRetry(filePath, message string) error { +// +// Optional behaviour is supplied through Options: WithDir runs the commands in a +// specific repository, and WithBackoff tunes the retry delay. With no options the +// call behaves identically to the original positional signature. +func CommitAndPushWithRetry(filePath, message string, opts ...Option) error { + o := newPushOptions(opts) + cmd := exec.Command("git", "add", filePath) + cmd.Dir = o.dir if out, err := cmd.CombinedOutput(); err != nil { return fmt.Errorf("git add failed: %s: %w", string(out), err) } cmd = exec.Command("git", "commit", "-m", message) + cmd.Dir = o.dir if out, err := cmd.CombinedOutput(); err != nil { if strings.Contains(string(out), "nothing to commit") { return nil @@ -219,26 +273,51 @@ func CommitAndPushWithRetry(filePath, message string) error { return fmt.Errorf("git commit failed: %s: %w", string(out), err) } - for i := 0; i < 3; i++ { - cmd = exec.Command("git", "push") + return pushWithRebaseRetry(o) +} + +// PushWithRebaseRetry pushes the current branch to its upstream, retrying up to +// three times behind a "git pull --rebase" when the push is rejected (for example +// a non-fast-forward caused by a concurrent state writer landing on trunk between +// checkout and push). It is the push half of CommitAndPushWithRetry, exposed for +// callers that stage and commit through their own flow and only need the shared +// rebase-retry behaviour. On a rebase conflict it aborts the rebase and returns +// the wrapped error rather than leaving the repository mid-rebase. +func PushWithRebaseRetry(opts ...Option) error { + return pushWithRebaseRetry(newPushOptions(opts)) +} + +// pushWithRebaseRetry is the single rebase-retry loop shared by +// CommitAndPushWithRetry and PushWithRebaseRetry. Keeping one implementation +// means the rebase-abort-on-conflict fix lives in exactly one place. +func pushWithRebaseRetry(o pushOptions) error { + for i := 0; i < pushRetryAttempts; i++ { + cmd := exec.Command("git", "push") + cmd.Dir = o.dir if _, err := cmd.CombinedOutput(); err == nil { return nil } + if i == pushRetryAttempts-1 { + break + } + cmd = exec.Command("git", "pull", "--rebase") + cmd.Dir = o.dir if out, err := cmd.CombinedOutput(); err != nil { // A failed rebase (typically a conflict) leaves the repository // mid-rebase. Abort it so we neither leave a conflicted state // behind nor loop into a guaranteed-failing push, and surface the // real error instead of the generic "push failed" summary. abort := exec.Command("git", "rebase", "--abort") + abort.Dir = o.dir _, _ = abort.CombinedOutput() // best effort; nothing to abort is fine return fmt.Errorf("git pull --rebase failed: %s: %w", string(out), err) } - time.Sleep(2 * time.Second) + time.Sleep(o.backoff) } - return fmt.Errorf("git push failed after 3 retries") + return fmt.Errorf("git push failed after %d retries", pushRetryAttempts) } // CurrentBranch returns the name of the currently checked-out branch. diff --git a/internal/git/git_test.go b/internal/git/git_test.go index f03805d2..cfe48204 100644 --- a/internal/git/git_test.go +++ b/internal/git/git_test.go @@ -7,6 +7,7 @@ import ( "reflect" "strings" "testing" + "time" ) // newScratchRepo initializes a git repository in a temp directory, changes the @@ -438,6 +439,106 @@ func TestCommitAndPushWithRetry_AbortsRebaseOnConflict(t *testing.T) { } } +// sharedRemoteClones builds a bare remote plus two working clones tracking it. +// The seed clone commits and pushes seedFile; the other clone then advances the +// remote with otherFile so a subsequent push from seed is rejected as +// non-fast-forward. When otherFile equals seedFile the advance conflicts on the +// same path, setting up a genuine rebase conflict. Both clones have signing +// disabled and an identity configured. +func sharedRemoteClones(t *testing.T, seedFile, seedBody, otherFile, otherBody string) (seed, other string) { + t.Helper() + + origin := t.TempDir() + if out, err := exec.Command("git", "init", "--bare", "--initial-branch=main", origin).CombinedOutput(); err != nil { + t.Fatalf("git init --bare: %v\n%s", err, out) + } + + seed = t.TempDir() + gitAt(t, "", "clone", origin, seed) + configRepo(t, seed) + if err := os.WriteFile(filepath.Join(seed, seedFile), []byte(seedBody), 0o600); err != nil { + t.Fatalf("write seed file: %v", err) + } + gitAt(t, seed, "add", seedFile) + gitAt(t, seed, "commit", "-m", "seed state") + gitAt(t, seed, "push", "origin", "main") + + other = t.TempDir() + gitAt(t, "", "clone", origin, other) + configRepo(t, other) + if err := os.WriteFile(filepath.Join(other, otherFile), []byte(otherBody), 0o600); err != nil { + t.Fatalf("write other file: %v", err) + } + gitAt(t, other, "add", otherFile) + gitAt(t, other, "commit", "-m", "concurrent write") + gitAt(t, other, "push", "origin", "main") + + return seed, other +} + +// TestPushWithRebaseRetry_RetriesNonFastForward proves the exported push half of +// the shared helper rebases onto an advanced upstream and retries when the first +// push is rejected non-fast-forward. WithDir drives the seed clone without +// changing the process working directory, and WithBackoff keeps the retry fast. +func TestPushWithRebaseRetry_RetriesNonFastForward(t *testing.T) { + // The concurrent writer touches an unrelated file so the rebase replays + // cleanly rather than conflicting. + seed, _ := sharedRemoteClones(t, "state.txt", "base\n", "OTHER.md", "concurrent\n") + + // A local, committed change on the seed clone whose base is now behind trunk. + if err := os.WriteFile(filepath.Join(seed, "state.txt"), []byte("local change\n"), 0o600); err != nil { + t.Fatalf("write local file: %v", err) + } + gitAt(t, seed, "add", "state.txt") + gitAt(t, seed, "commit", "-m", "local change") + + if err := PushWithRebaseRetry(WithDir(seed), WithBackoff(time.Millisecond)); err != nil { + t.Fatalf("PushWithRebaseRetry should rebase and retry a non-fast-forward push, got: %v", err) + } + + log := runGitOut(t, seed, "log", "--oneline", "origin/main") + for _, want := range []string{"seed state", "concurrent write", "local change"} { + if !strings.Contains(log, want) { + t.Fatalf("expected origin/main history to contain %q after retry, got:\n%s", want, log) + } + } +} + +// TestPushWithRebaseRetry_AbortsRebaseOnConflict proves the exported push helper +// aborts the rebase and returns the wrapped error, leaving no mid-rebase state, +// when the pull --rebase conflicts. WithDir targets the seed clone directly. +func TestPushWithRebaseRetry_AbortsRebaseOnConflict(t *testing.T) { + // The concurrent writer changes the same file, so the rebase conflicts. + seed, _ := sharedRemoteClones(t, "state.txt", "base\n", "state.txt", "remote change\n") + + if err := os.WriteFile(filepath.Join(seed, "state.txt"), []byte("local change\n"), 0o600); err != nil { + t.Fatalf("write local file: %v", err) + } + gitAt(t, seed, "add", "state.txt") + gitAt(t, seed, "commit", "-m", "local change") + + if err := PushWithRebaseRetry(WithDir(seed), WithBackoff(time.Millisecond)); err == nil { + t.Fatal("PushWithRebaseRetry with a conflicting remote: expected error, got nil") + } + + for _, name := range []string{"rebase-merge", "rebase-apply"} { + if _, statErr := os.Stat(filepath.Join(seed, ".git", name)); statErr == nil { + t.Fatalf("repository left mid-rebase: .git/%s still present", name) + } + } +} + +// runGitOut runs a git command in dir and returns its trimmed stdout, failing the +// test on error. +func runGitOut(t *testing.T, dir string, args ...string) string { + t.Helper() + out, err := exec.Command("git", append([]string{"-C", dir}, args...)...).Output() + if err != nil { + t.Fatalf("git -C %s %s: %v", dir, strings.Join(args, " "), err) + } + return string(out) +} + // tagHead creates a lightweight tag pointing at the current HEAD. func tagHead(t *testing.T, name string) { t.Helper() diff --git a/internal/orchestrate/orchestrator.go b/internal/orchestrate/orchestrator.go index 9c78f120..66d290d4 100644 --- a/internal/orchestrate/orchestrator.go +++ b/internal/orchestrate/orchestrator.go @@ -24,21 +24,11 @@ type Orchestrator struct { cicdFile *config.CICDFile baseDir string // pushBackoff is the delay between state-write push retries. A zero value - // selects the default (defaultPushBackoff); tests override it to keep the - // retry loop fast. + // selects the shared git package default; tests override it to keep the retry + // loop fast. It is threaded into git.PushWithRebaseRetry via git.WithBackoff. pushBackoff time.Duration } -// State-write push retry policy. commitAndPush retries a rejected (for example -// non-fast-forward) push behind a rebase so a concurrent state writer or a -// "[skip ci]" commit that advances trunk between checkout and push does not fail -// the run outright. This mirrors git.CommitAndPushWithRetry, the plain-git retry -// path the promote and hotfix finalizers use for the manifest state write. -const ( - pushMaxAttempts = 3 - defaultPushBackoff = 2 * time.Second -) - // DefaultStateKey is used for state tracking when no environments are configured. const DefaultStateKey = "prerelease" @@ -537,41 +527,16 @@ func (o *Orchestrator) commitAndPush(version string) error { // pushStateWithRetry pushes the committed state change, rebasing onto the // upstream and retrying when the push is rejected (for example a non-fast-forward // caused by a concurrent state writer or a "[skip ci]" commit landing on trunk -// between checkout and push). It mirrors git.CommitAndPushWithRetry so the -// orchestrator state write and the promote/hotfix finalize state write share the -// same optimistic push behaviour. +// between checkout and push). It delegates to git.PushWithRebaseRetry, the single +// rebase-retry implementation the promote and hotfix finalizers also use, run +// against the orchestrator's base directory so both state-write paths share one +// rebase-abort-on-conflict behaviour. func (o *Orchestrator) pushStateWithRetry() error { - backoff := o.pushBackoff - if backoff == 0 { - backoff = defaultPushBackoff - } - - var lastErr error - for attempt := 0; attempt < pushMaxAttempts; attempt++ { - lastErr = o.gitRun("push") - if lastErr == nil { - log.Info("Committed and pushed state changes") - return nil - } - - if attempt == pushMaxAttempts-1 { - break - } - - // Integrate the advanced upstream and replay the state commit on top, - // then retry the push. - if err := o.gitRun("pull", "--rebase"); err != nil { - // A failed rebase (typically a conflict) leaves the repository - // mid-rebase. Abort it so we neither leave a conflicted state - // behind nor loop into a guaranteed-failing push, and surface the - // real error instead of a generic push-failed summary. - _ = o.gitRun("rebase", "--abort") // best effort; nothing to abort is fine - return fmt.Errorf("git pull --rebase before push retry failed: %w", err) - } - time.Sleep(backoff) + if err := git.PushWithRebaseRetry(git.WithDir(o.baseDir), git.WithBackoff(o.pushBackoff)); err != nil { + return err } - - return fmt.Errorf("failed to push state changes after %d attempts: %w", pushMaxAttempts, lastErr) + log.Info("Committed and pushed state changes") + return nil } // gitOutput runs a git command and returns stdout.