From a5facc8bf42697ba90b5871de2971d5e1166c323 Mon Sep 17 00:00:00 2001 From: Joshua Temple Date: Sun, 5 Jul 2026 11:22:09 -0400 Subject: [PATCH] refactor(git): unify state-push rebase-retry into one dir-aware helper The orchestrator carried its own push/pull --rebase/rebase --abort retry loop that duplicated git.CommitAndPushWithRetry. The copy existed only because the orchestrator runs git against its base directory (cmd.Dir) while the package helper ran in the process working directory, and because its test needed a fast backoff. Add functional options (WithDir, WithBackoff) to the git helpers and extract the retry loop into a single shared implementation. Expose PushWithRebaseRetry as the push half for callers that stage and commit through their own flow. The orchestrator now delegates to it with its base directory and backoff, so the rebase-abort-on-conflict behaviour lives in exactly one place. Existing CommitAndPushWithRetry callers are unchanged: the options tail is additive with no-op defaults. Signed-off-by: Joshua Temple --- internal/git/git.go | 89 +++++++++++++++++++++-- internal/git/git_test.go | 101 +++++++++++++++++++++++++++ internal/orchestrate/orchestrator.go | 55 +++------------ 3 files changed, 195 insertions(+), 50 deletions(-) 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.