From ae10b79bda9bb6a1da1eeb44ec92de82dbb12f34 Mon Sep 17 00:00:00 2001 From: Joshua Temple Date: Sat, 4 Jul 2026 15:32:38 -0400 Subject: [PATCH] fix(orchestrate): retry state-write push on a non-fast-forward trunk commitAndPush wrote orchestrator state to trunk with a plain git push and no retry, so a concurrent state writer or an automated skip-CI commit that advanced trunk between checkout and push failed the run outright with a non-fast-forward rejection. The promote and hotfix finalizers already rebase-and-retry the same state write (git.CommitAndPushWithRetry). Route the orchestrator state push through the same optimistic retry: on a rejected push, rebase onto the advanced upstream and retry, up to three attempts. The retry backoff is injectable so the regression test stays fast. Signed-off-by: Joshua Temple --- internal/orchestrate/orchestrator.go | 58 ++++++- internal/orchestrate/state_push_retry_test.go | 162 ++++++++++++++++++ 2 files changed, 215 insertions(+), 5 deletions(-) create mode 100644 internal/orchestrate/state_push_retry_test.go diff --git a/internal/orchestrate/orchestrator.go b/internal/orchestrate/orchestrator.go index 4cb5c640..9c78f120 100644 --- a/internal/orchestrate/orchestrator.go +++ b/internal/orchestrate/orchestrator.go @@ -23,8 +23,22 @@ type Orchestrator struct { environment string 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. + 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" @@ -517,13 +531,47 @@ func (o *Orchestrator) commitAndPush(version string) error { return err } - // Push - if err := o.gitRun("push"); err != nil { - return err + return o.pushStateWithRetry() +} + +// 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. +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) } - log.Info("Committed and pushed state changes") - return nil + return fmt.Errorf("failed to push state changes after %d attempts: %w", pushMaxAttempts, lastErr) } // gitOutput runs a git command and returns stdout. diff --git a/internal/orchestrate/state_push_retry_test.go b/internal/orchestrate/state_push_retry_test.go new file mode 100644 index 00000000..293ad66c --- /dev/null +++ b/internal/orchestrate/state_push_retry_test.go @@ -0,0 +1,162 @@ +package orchestrate + +import ( + "os" + "path/filepath" + "strings" + "testing" + "time" +) + +// initClonesWithSharedRemote builds a bare remote plus two working clones that +// track it. cloneA holds a committed state file; cloneB then advances the shared +// remote with an unrelated commit, so a subsequent push from cloneA is rejected +// as non-fast-forward until it rebases onto the advanced upstream. +func initClonesWithSharedRemote(t *testing.T) (cloneA, cloneB, statePath string) { + t.Helper() + + remote := t.TempDir() + runGit(t, remote, "init", "--bare", "-b", "main") + + cloneA = t.TempDir() + runGit(t, cloneA, "clone", remote, ".") + runGit(t, cloneA, "checkout", "-b", "main") + writeFile(t, cloneA, ".github/manifest.yaml", "ci:\n state:\n prerelease:\n version: v0.1.0-rc.0\n") + runGit(t, cloneA, "add", ".github/manifest.yaml") + runGit(t, cloneA, "commit", "-m", "chore: seed state") + runGit(t, cloneA, "push", "-u", "origin", "main") + + cloneB = t.TempDir() + runGit(t, cloneB, "clone", remote, ".") + // cloneB advances the shared remote with an unrelated file so the rebase in + // cloneA replays cleanly (no conflict on the state file). + writeFile(t, cloneB, "OTHER.md", "concurrent writer\n") + runGit(t, cloneB, "add", "OTHER.md") + runGit(t, cloneB, "commit", "-m", "chore: concurrent write") + runGit(t, cloneB, "push", "origin", "main") + + statePath = filepath.Join(cloneA, ".github", "manifest.yaml") + return cloneA, cloneB, statePath +} + +// TestCommitAndPush_RetriesNonFastForward reproduces F05: when the shared trunk +// advances between checkout and push, a plain "git push" is rejected +// non-fast-forward. commitAndPush must rebase onto the advanced upstream and +// retry rather than fail the orchestrate run outright. +func TestCommitAndPush_RetriesNonFastForward(t *testing.T) { + cloneA, _, statePath := initClonesWithSharedRemote(t) + + // Local, uncommitted state change in cloneA whose base is now behind trunk. + writeFile(t, cloneA, ".github/manifest.yaml", + "ci:\n state:\n prerelease:\n version: v0.1.0-rc.1\n") + + o := &Orchestrator{ + configPath: statePath, + environment: "prerelease", + baseDir: cloneA, + pushBackoff: time.Millisecond, + } + + if err := o.commitAndPush("v0.1.0-rc.1"); err != nil { + t.Fatalf("commitAndPush should rebase and retry a non-fast-forward push, got error: %v", err) + } + + // The state commit landed on the shared remote on top of the concurrent + // writer's commit (a successful rebase-and-push). + remoteLog := runGit(t, cloneA, "log", "--oneline", "origin/main") + for _, want := range []string{"seed state", "concurrent write"} { + if !strings.Contains(remoteLog, want) { + t.Fatalf("expected origin/main history to contain %q after retry, got:\n%s", want, remoteLog) + } + } +} + +// TestCommitAndPush_NoChangesIsNoOp guards the early return: with a clean tree +// there is nothing to commit, so commitAndPush succeeds without pushing. +func TestCommitAndPush_NoChangesIsNoOp(t *testing.T) { + cloneA, _, statePath := initClonesWithSharedRemote(t) + + o := &Orchestrator{ + configPath: statePath, + environment: "prerelease", + baseDir: cloneA, + pushBackoff: time.Millisecond, + } + + if err := o.commitAndPush("v0.1.0-rc.0"); err != nil { + t.Fatalf("commitAndPush with no changes should be a no-op, got error: %v", err) + } +} + +// initClonesWithConflictingRemote builds a bare remote plus two working clones +// that track it, where cloneB advances the shared remote with a change to the +// exact same line of the same state file that the caller will then also edit +// in cloneA. That sets up a genuine rebase conflict (rather than a clean +// replay) once cloneA commits its own edit and pushStateWithRetry rebases +// onto the advanced upstream. +func initClonesWithConflictingRemote(t *testing.T) (cloneA, cloneB, statePath string) { + t.Helper() + + remote := t.TempDir() + runGit(t, remote, "init", "--bare", "-b", "main") + + cloneA = t.TempDir() + runGit(t, cloneA, "clone", remote, ".") + runGit(t, cloneA, "checkout", "-b", "main") + writeFile(t, cloneA, ".github/manifest.yaml", "ci:\n state:\n prerelease:\n version: v0.1.0-rc.0\n") + runGit(t, cloneA, "add", ".github/manifest.yaml") + runGit(t, cloneA, "commit", "-m", "chore: seed state") + runGit(t, cloneA, "push", "-u", "origin", "main") + + cloneB = t.TempDir() + runGit(t, cloneB, "clone", remote, ".") + // cloneB advances the shared remote with a conflicting edit to the same + // version line that cloneA will independently change. + writeFile(t, cloneB, ".github/manifest.yaml", "ci:\n state:\n prerelease:\n version: v0.1.0-rc.9\n") + runGit(t, cloneB, "add", ".github/manifest.yaml") + runGit(t, cloneB, "commit", "-m", "chore: concurrent state write") + runGit(t, cloneB, "push", "origin", "main") + + statePath = filepath.Join(cloneA, ".github", "manifest.yaml") + return cloneA, cloneB, statePath +} + +// TestCommitAndPush_AbortsRebaseOnConflict reproduces the mid-rebase defect in +// pushStateWithRetry: when "git pull --rebase" hits a genuine conflict (a +// concurrent writer changed the same state file line cloneA is committing), +// the old code only logged a warning and fell through to the next push +// attempt, leaving the repository stuck mid-rebase after the retries were +// exhausted. The fix aborts the rebase and returns the wrapped error +// immediately. Both properties are asserted: an error is returned, and the +// clone is left in a clean, non-rebasing state. +func TestCommitAndPush_AbortsRebaseOnConflict(t *testing.T) { + cloneA, _, statePath := initClonesWithConflictingRemote(t) + + // Local, uncommitted change to the same line cloneB already advanced on + // the shared remote, so the rebase replay conflicts. + writeFile(t, cloneA, ".github/manifest.yaml", + "ci:\n state:\n prerelease:\n version: v0.1.0-rc.1\n") + + o := &Orchestrator{ + configPath: statePath, + environment: "prerelease", + baseDir: cloneA, + pushBackoff: time.Millisecond, + } + + if err := o.commitAndPush("v0.1.0-rc.1"); err == nil { + t.Fatal("commitAndPush with a conflicting remote: expected error, got nil") + } + + // The clone must not be left mid-rebase. + for _, name := range []string{"rebase-merge", "rebase-apply"} { + if _, statErr := os.Stat(filepath.Join(cloneA, ".git", name)); statErr == nil { + t.Fatalf("repository left mid-rebase: .git/%s still present", name) + } + } + + status := runGit(t, cloneA, "status", "--porcelain=v1", "--branch") + if strings.Contains(status, "rebasing") { + t.Fatalf("git status reports a rebase in progress:\n%s", status) + } +}