From 3d9bfe1b98369e409db4b4d3f2fd9ac0ef40073c Mon Sep 17 00:00:00 2001 From: Joshua Temple Date: Thu, 25 Jun 2026 15:12:42 -0400 Subject: [PATCH] fix(reset): retry reset push on non-fast-forward, re-applying the baseline cascade reset --state --push did a plain git push with no retry, so a concurrent write to main rejected it non-fast-forward (observed as the keystone failure in the 4env validation-fleet lifecycle, which seeds state via reset --push). Every other state writer already self-heals via fetch/reset/reapply; reset was the gap. On rejection the push now resolves the branch, fetches and hard-resets onto origin, re-parses the freshly fetched manifest so trunk-owned config survives, re-applies the state baseline, recommits, and retries (bounded 3 attempts, 2s backoff). Reset still wins on the state and latest-release sections. DryRun, no-push, and no-change paths are unchanged. Signed-off-by: Joshua Temple --- internal/reset/reset.go | 111 ++++++++++++++++++++-- internal/reset/reset_push_test.go | 150 ++++++++++++++++++++++++++++++ 2 files changed, 255 insertions(+), 6 deletions(-) create mode 100644 internal/reset/reset_push_test.go diff --git a/internal/reset/reset.go b/internal/reset/reset.go index 617a219d..eeda4ee9 100644 --- a/internal/reset/reset.go +++ b/internal/reset/reset.go @@ -9,6 +9,7 @@ import ( "os/exec" "path/filepath" "strings" + "time" "gopkg.in/yaml.v3" @@ -317,19 +318,117 @@ func (r *Resetter) commitAndPush() error { return err } - // Add, commit, push + // Add and commit the baseline reset. if err := r.gitRun("add", configPath); err != nil { return err } - if err := r.gitRun("commit", "-m", "chore: reset state for testing [skip ci]"); err != nil { + if err := r.gitRun("commit", "-m", resetCommitMessage); err != nil { return err } - if err := r.gitRun("push"); err != nil { - return err + + return r.pushWithRetry(configPath) +} + +// resetCommitMessage is the commit subject used for every state-reset commit, +// including each re-applied attempt after a non-fast-forward push. +const resetCommitMessage = "chore: reset state for testing [skip ci]" + +// maxPushAttempts bounds the non-fast-forward recovery loop, matching the bound +// used by the other state-write push paths. +const maxPushAttempts = 3 + +// pushWithRetry pushes the committed reset, recovering from a non-fast-forward +// rejection. Concurrent writers can advance the trunk between the reset's read +// and its push; on rejection it fetches the updated trunk, re-applies the +// baseline reset on top of it, recommits, and retries. The reset always wins: +// the baseline overwrites the state section rather than merging a concurrent +// writer's state back in. +func (r *Resetter) pushWithRetry(configPath string) error { + for attempt := 1; ; attempt++ { + pushErr := r.gitRun("push") + if pushErr == nil { + log.Info("Committed and pushed state reset") + return nil + } + + if attempt >= maxPushAttempts { + return fmt.Errorf("push rejected after %d attempts: %w", maxPushAttempts, pushErr) + } + + log.Warn("Push rejected (attempt %d/%d); re-applying reset onto updated trunk", attempt, maxPushAttempts) + recommitted, err := r.reapplyResetOntoTrunk(configPath) + if err != nil { + return err + } + if !recommitted { + // The trunk already carries the baseline state, so there is nothing + // left for this reset to push. + log.Info("Trunk already at baseline state after fetch; nothing to push") + return nil + } + + time.Sleep(pushRetryBackoff) } +} - log.Info("Committed and pushed state reset") - return nil +// pushRetryBackoff is the short pause between non-fast-forward recovery attempts. +const pushRetryBackoff = 2 * time.Second + +// reapplyResetOntoTrunk fetches the current trunk, hard-resets the working tree +// onto it, re-applies the baseline reset to the freshly fetched manifest, and +// commits the result. It reports whether a new commit was created; a false value +// means the trunk is already at the baseline and there is nothing to push. +func (r *Resetter) reapplyResetOntoTrunk(configPath string) (bool, error) { + branch, err := r.currentBranch() + if err != nil { + return false, err + } + + if err := r.gitRun("fetch", "origin", branch); err != nil { + return false, fmt.Errorf("fetch origin during non-fast-forward recovery: %w", err) + } + if err := r.gitRun("reset", "--hard", "origin/"+branch); err != nil { + return false, fmt.Errorf("reset to trunk tip during non-fast-forward recovery: %w", err) + } + + // Re-read the freshly fetched manifest so the baseline is applied on top of + // the current trunk, not the stale copy the reset first read. + cicdFile, err := config.ParseManifestFile(r.configPath, r.manifestKey) + if err != nil { + return false, fmt.Errorf("reload manifest during non-fast-forward recovery: %w", err) + } + r.cicdFile = cicdFile + r.cicdFile.State = nil + r.cicdFile.LatestRelease = nil + if err := r.writeConfig(); err != nil { + return false, fmt.Errorf("rewrite baseline during non-fast-forward recovery: %w", err) + } + + status, _ := r.gitOutput("status", "--porcelain", configPath) + if strings.TrimSpace(status) == "" { + return false, nil + } + + if err := r.gitRun("add", configPath); err != nil { + return false, err + } + if err := r.gitRun("commit", "-m", resetCommitMessage); err != nil { + return false, err + } + return true, nil +} + +// currentBranch returns the checked-out branch name, scoped to the repo path. +func (r *Resetter) currentBranch() (string, error) { + branch, err := r.gitOutput("rev-parse", "--abbrev-ref", "HEAD") + if err != nil { + return "", fmt.Errorf("resolve current branch: %w", err) + } + branch = strings.TrimSpace(branch) + if branch == "" || branch == "HEAD" { + return "", fmt.Errorf("resolve current branch: detached or empty HEAD") + } + return branch, nil } // Helper functions diff --git a/internal/reset/reset_push_test.go b/internal/reset/reset_push_test.go new file mode 100644 index 00000000..7588be78 --- /dev/null +++ b/internal/reset/reset_push_test.go @@ -0,0 +1,150 @@ +package reset + +import ( + "os" + "os/exec" + "path/filepath" + "strings" + "testing" + + "github.com/stretchr/testify/require" + + "github.com/stablekernel/cascade/internal/config" +) + +// gitInDir runs a git command scoped to dir (matching how the Resetter scopes +// every git invocation to r.repoPath) and fails the test on error. +func gitInDir(t *testing.T, dir string, args ...string) { + t.Helper() + full := append([]string{"-C", dir}, args...) + if out, err := exec.Command("git", full...).CombinedOutput(); err != nil { + t.Fatalf("git %s: %v\n%s", strings.Join(args, " "), err, out) + } +} + +// configureGitIdentity sets a committer identity and disables signing so commits +// succeed in a bare CI sandbox without a configured user or GPG key. +func configureGitIdentity(t *testing.T, dir string) { + t.Helper() + gitInDir(t, dir, "config", "user.email", "tester@example.com") + gitInDir(t, dir, "config", "user.name", "Reset Tester") + gitInDir(t, dir, "config", "commit.gpgsign", "false") +} + +// originHeadManifest returns the manifest contents at the origin's main tip. +func originHeadManifest(t *testing.T, origin, relPath string) string { + t.Helper() + out, err := exec.Command("git", "-C", origin, "show", "main:"+relPath).Output() + if err != nil { + t.Fatalf("git show main:%s: %v", relPath, err) + } + return string(out) +} + +// TestCommitAndPush_RecoversFromNonFastForward proves the seeding race that broke +// the multi-environment lifecycle job: a concurrent writer advances origin/main +// between the reset's read and its push, so a plain push is rejected +// non-fast-forward. commitAndPush must fetch, re-apply the baseline reset on top +// of the updated trunk, and push. The baseline reset must WIN, leaving the +// concurrent writer's state overwritten rather than merged back in. +func TestCommitAndPush_RecoversFromNonFastForward(t *testing.T) { + const relPath = "manifest.yaml" + + // Bare origin shared by the reset checkout and the concurrent writer. + origin := t.TempDir() + if out, err := exec.Command("git", "init", "--bare", "-b", "main", origin).CombinedOutput(); err != nil { + t.Fatalf("git init --bare: %v\n%s", err, out) + } + + // Seed origin with a manifest that already carries state. This is the value + // the reset checkout reads before the race. + const seed = `ci: + config: + trunk_branch: main + environments: + - dev + state: + dev: + sha: seedsha + version: v1.0.0 +` + bootstrap := t.TempDir() + gitInDir(t, "", "clone", origin, bootstrap) + configureGitIdentity(t, bootstrap) + require.NoError(t, os.WriteFile(filepath.Join(bootstrap, relPath), []byte(seed), 0o600)) + gitInDir(t, bootstrap, "add", relPath) + gitInDir(t, bootstrap, "commit", "-m", "seed manifest") + gitInDir(t, bootstrap, "push", "origin", "HEAD:main") + + // The reset checkout: this is the repo under test, scoped via repoPath. + resetRepo := t.TempDir() + gitInDir(t, "", "clone", origin, resetRepo) + configureGitIdentity(t, resetRepo) + + configPath := filepath.Join(resetRepo, relPath) + cicdFile, err := config.ParseManifestFile(configPath, config.DefaultManifestKey) + require.NoError(t, err) + + r := &Resetter{ + opts: Options{ + RepoPath: resetRepo, + ResetState: true, + Push: true, + }, + repoPath: resetRepo, + repoOwner: "test", + repoName: "repo", + configPath: configPath, + manifestKey: config.DefaultManifestKey, + cicdFile: cicdFile, + } + + // The reset clears state on disk, mirroring Reset() calling resetState() + // before commitAndPush(). + require.NoError(t, r.resetState()) + + // A concurrent writer advances origin/main AFTER the reset read its copy, + // writing a fresh promotion into the same state section. The reset checkout's + // parent is now stale, so its next push is rejected non-fast-forward. + writer := t.TempDir() + gitInDir(t, "", "clone", origin, writer) + configureGitIdentity(t, writer) + const writerManifest = `ci: + config: + trunk_branch: main + environments: + - dev + state: + dev: + sha: writersha + version: v2.0.0 +` + require.NoError(t, os.WriteFile(filepath.Join(writer, relPath), []byte(writerManifest), 0o600)) + gitInDir(t, writer, "add", relPath) + gitInDir(t, writer, "commit", "-m", "concurrent promote") + gitInDir(t, writer, "push", "origin", "HEAD:main") + + // commitAndPush must self-heal across the rejected push. + if err := r.commitAndPush(); err != nil { + t.Fatalf("commitAndPush() error = %v, want nil (must recover from non-fast-forward)", err) + } + + // The baseline reset must win: origin's tip carries neither the seed state nor + // the concurrent writer's state. + got := originHeadManifest(t, origin, relPath) + if strings.Contains(got, "writersha") { + t.Errorf("reset did not win: origin HEAD still carries concurrent writer state; got:\n%s", got) + } + if strings.Contains(got, "seedsha") { + t.Errorf("origin HEAD still carries the pre-reset seed state; got:\n%s", got) + } + if strings.Contains(got, "state:") { + t.Errorf("state section was not reset to baseline; got:\n%s", got) + } + // The non-state config the writer left in place must survive the rebase. + if !strings.Contains(got, "trunk_branch: main") { + t.Errorf("origin HEAD lost config section after recovery; got:\n%s", got) + } + + _ = bootstrap +}