From 04c044a9a84fb4d650a9fbd8752554a2ec3efccc Mon Sep 17 00:00:00 2001 From: Joshua Temple Date: Thu, 9 Jul 2026 02:16:29 -0400 Subject: [PATCH] fix(orchestrate): re-apply the component state leaf on a rejected finalize push The Go orchestrate finalize path committed the state leaf once and then relied on a textual git pull --rebase to land it, so two component finalizes appending adjacent leaves under state.components conflicted on the rebase and hard-failed, despite a doc-comment claiming the loop re-runs the write. The promote, hotfix, and rollback finalizers already re-derive and re-apply their owned leaf per attempt. Add a WithReapply option to the shared push-retry: on a rejected push it re-fetches trunk, resets away the stale local commit, and re-derives the owned leaf onto the fresh bytes before retrying, so a concurrent sibling's leaf survives. Wire it for component-scoped orchestrate runs only, leaving single-component finalize on the textual path byte-identical. Raise the retry count to ten with exponential jittered backoff and emit a per-attempt log line. Correct the false doc-comment. Signed-off-by: Joshua Temple --- internal/git/git.go | 163 ++++++++++++++---- .../orchestrate/concurrent_finalize_test.go | 161 +++++++++++++++++ internal/orchestrate/orchestrator.go | 68 ++++++-- 3 files changed, 347 insertions(+), 45 deletions(-) create mode 100644 internal/orchestrate/concurrent_finalize_test.go diff --git a/internal/git/git.go b/internal/git/git.go index b2d9bc1d..1aab0897 100644 --- a/internal/git/git.go +++ b/internal/git/git.go @@ -4,10 +4,12 @@ import ( "bytes" "errors" "fmt" + "math/rand/v2" "os/exec" "strings" "time" + "github.com/stablekernel/cascade/internal/log" "github.com/stablekernel/cascade/internal/taggrammar" ) @@ -248,21 +250,39 @@ 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 +// pushRetryAttempts is the number of times a rejected push is retried before the +// operation is declared failed. It is sized to survive a realistic concurrent +// wave (all components of a monorepo racing to write their own leaf into one +// shared manifest file on trunk) and is aligned with the Contents-API write path +// ceiling in internal/statewrite. +const pushRetryAttempts = 10 -// 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 +// defaultPushBackoff is the base delay between push retries when no backoff is +// set via WithBackoff. The effective wait grows exponentially per attempt and +// carries a random jitter so concurrent writers de-synchronize rather than +// colliding again in lockstep. See backoffForAttempt. +const defaultPushBackoff = 250 * time.Millisecond + +// maxPushBackoff caps the exponential growth of the per-attempt backoff so a +// late retry never sleeps for an unbounded stretch. +const maxPushBackoff = 8 * 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. +// directory and retries wait defaultPushBackoff apart with no re-apply hook. type pushOptions struct { dir string backoff time.Duration + // reapply, when set, is invoked on a rejected push instead of a textual + // "git pull --rebase". The loop first re-fetches trunk and hard-resets the + // local branch to the upstream tip (dropping the local commit whose bytes + // were derived from a now-stale trunk), then calls reapply, which re-derives + // and re-writes only the owned state leaf onto the fresh trunk bytes and + // re-commits. This converges an owned leaf against a concurrent sibling's + // leaf that already landed on trunk, where a textual rebase conflicts on the + // adjacent edit. A nil reapply preserves the historical textual-rebase + // behaviour for callers (promote, hotfix, rollback) that do not need it. + reapply func() error } // Option configures the rebase-retry push helpers. Options are additive: a call @@ -277,12 +297,64 @@ 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. +// WithBackoff sets the base delay between push retries. A zero duration (the +// default) selects defaultPushBackoff. The effective wait grows exponentially +// from this base per attempt (capped at maxPushBackoff) plus a random jitter, so +// tests pass a tiny value to keep the retry loop fast. func WithBackoff(d time.Duration) Option { return func(o *pushOptions) { o.backoff = d } } +// WithReapply supplies a re-apply callback invoked on a rejected push in place of +// a textual "git pull --rebase". On a non-fast-forward reject the loop re-fetches +// trunk, hard-resets the local branch to the upstream tip, then calls fn, which +// must re-derive and re-write only the owned state leaf onto the fresh trunk +// bytes and re-commit it. This makes an owned leaf converge against a concurrent +// sibling leaf already on trunk without the textual conflict a rebase hits on the +// adjacent edit. With no WithReapply the loop keeps the historical textual-rebase +// behaviour, so callers that do not need re-apply (promote, hotfix, rollback) are +// unaffected. +func WithReapply(fn func() error) Option { + return func(o *pushOptions) { o.reapply = fn } +} + +// backoffForAttempt returns the delay before the retry following a zero-based +// attempt index: an exponential growth of base (base * 2^attempt) capped at +// maxPushBackoff, plus a random jitter of up to base so concurrent writers racing +// the same trunk de-synchronize instead of colliding again in lockstep. +func backoffForAttempt(base time.Duration, attempt int) time.Duration { + if base <= 0 { + base = defaultPushBackoff + } + d := base + for i := 0; i < attempt && d < maxPushBackoff; i++ { + d *= 2 + } + if d > maxPushBackoff { + d = maxPushBackoff + } + return d + time.Duration(rand.Int64N(int64(base)+1)) +} + +// refetchAndReset re-fetches trunk and hard-resets the working tree to the +// upstream tracking tip, dropping any local commit whose bytes were derived from +// a stale trunk. It is the pre-step of a re-apply retry: after it returns the +// working manifest holds the fresh trunk bytes (including any concurrent sibling +// leaf) onto which the owned leaf is re-derived. +func refetchAndReset(dir string) error { + fetch := exec.Command("git", "fetch") + fetch.Dir = dir + if out, err := fetch.CombinedOutput(); err != nil { + return fmt.Errorf("git fetch failed: %s: %w", string(out), err) + } + reset := exec.Command("git", "reset", "--hard", "@{u}") + reset.Dir = dir + if out, err := reset.CombinedOutput(); err != nil { + return fmt.Errorf("git reset --hard @{u} failed: %s: %w", string(out), err) + } + return nil +} + func newPushOptions(opts []Option) pushOptions { var o pushOptions for _, opt := range opts { @@ -295,8 +367,9 @@ func newPushOptions(opts []Option) pushOptions { } // 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 +// to the current branch's upstream, retrying a rejected push up to +// pushRetryAttempts 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. @@ -325,25 +398,38 @@ func CommitAndPushWithRetry(filePath, message string, opts ...Option) error { 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. +// PushWithRebaseRetry pushes the current branch to its upstream, retrying a +// rejected push up to pushRetryAttempts times (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 retry +// behaviour. By default a rejected push is retried behind a "git pull --rebase", +// aborting the rebase and returning the wrapped error on a conflict rather than +// leaving the repository mid-rebase. Passing WithReapply switches the retry to +// re-derive the owned state leaf against re-fetched trunk instead, so an owned +// leaf converges against a concurrent sibling's adjacent leaf without conflict. 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. +// means the re-apply and rebase-abort-on-conflict behaviours live in exactly one +// place. When o.reapply is set it re-derives the owned leaf against re-fetched +// trunk on each rejected push; otherwise it falls back to a textual rebase. +// +// Every attempt emits a stable "cascade-state-write:" log line carrying the +// attempt count, so a live run can be grepped to prove a concurrent wave +// converged (and, on failure, that it exhausted the ceiling rather than erroring +// for another reason). func pushWithRebaseRetry(o pushOptions) error { for i := 0; i < pushRetryAttempts; i++ { + log.Info("cascade-state-write: attempt=%d/%d", i+1, pushRetryAttempts) + cmd := exec.Command("git", "push") cmd.Dir = o.dir if _, err := cmd.CombinedOutput(); err == nil { + log.Info("cascade-state-write: ok attempt=%d", i+1) return nil } @@ -351,21 +437,34 @@ func pushWithRebaseRetry(o pushOptions) error { 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) + if o.reapply != nil { + // Re-derive the owned leaf onto re-fetched trunk bytes instead of a + // textual rebase, which conflicts when a concurrent sibling wrote an + // adjacent leaf into the same shared manifest file. + if err := refetchAndReset(o.dir); err != nil { + return err + } + if err := o.reapply(); err != nil { + return fmt.Errorf("state re-apply on push retry failed: %w", err) + } + } else { + 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(o.backoff) + time.Sleep(backoffForAttempt(o.backoff, i)) } + log.Info("cascade-state-write: exhausted attempts=%d", pushRetryAttempts) return fmt.Errorf("git push failed after %d retries", pushRetryAttempts) } diff --git a/internal/orchestrate/concurrent_finalize_test.go b/internal/orchestrate/concurrent_finalize_test.go new file mode 100644 index 00000000..2aa006fb --- /dev/null +++ b/internal/orchestrate/concurrent_finalize_test.go @@ -0,0 +1,161 @@ +package orchestrate + +import ( + "os" + "path/filepath" + "strings" + "testing" + "time" + + "github.com/stablekernel/cascade/internal/config" + "github.com/stablekernel/cascade/internal/git" +) + +// seedComponentRemote builds a bare remote seeded with a component-scoped +// manifest (one unrelated "seed" component leaf, so the manifest is +// unambiguously in the component form and there is a sibling subtree to +// preserve), plus a working clone whose upstream tracks it. The clone's local +// manifest is the seed; the caller lands a concurrent sibling leaf on trunk to +// make a subsequent push non-fast-forward. +func seedComponentRemote(t *testing.T) (clone, statePath string) { + t.Helper() + + remote := t.TempDir() + runGit(t, remote, "init", "--bare", "-b", "main") + + const seed = "ci:\n state:\n components:\n seed:\n prod:\n version: v0.0.1\n" + + clone = t.TempDir() + runGit(t, clone, "clone", remote, ".") + runGit(t, clone, "checkout", "-b", "main") + writeFile(t, clone, ".github/manifest.yaml", seed) + runGit(t, clone, "add", ".github/manifest.yaml") + runGit(t, clone, "commit", "-m", "chore: seed state") + runGit(t, clone, "push", "-u", "origin", "main") + + statePath = filepath.Join(clone, ".github", "manifest.yaml") + return clone, statePath +} + +// landSiblingLeaf clones the remote behind clone, node-patches component "b"'s +// prod leaf onto trunk via the same scoped-write the finalizers use, and pushes +// it first. That advances origin so clone's parent is stale and its own adjacent +// component-"a" leaf collides on a textual rebase. +func landSiblingLeaf(t *testing.T, clone string) { + t.Helper() + + remote := runGit(t, clone, "config", "--get", "remote.origin.url") + writer := t.TempDir() + runGit(t, writer, "clone", remote, ".") + + path := filepath.Join(writer, ".github", "manifest.yaml") + current, err := os.ReadFile(path) + if err != nil { + t.Fatalf("read writer manifest: %v", err) + } + patched, err := config.WriteScopedState(current, "ci", config.StateWrite{ + Component: "b", Env: "prod", State: &config.EnvState{Version: "v2.0.0", SHA: "bbbbbbb"}, + }) + if err != nil { + t.Fatalf("scoped write for sibling leaf: %v", err) + } + if err := os.WriteFile(path, patched, 0o644); err != nil { + t.Fatalf("write writer manifest: %v", err) + } + runGit(t, writer, "add", ".github/manifest.yaml") + runGit(t, writer, "commit", "-m", "b: land prod leaf") + runGit(t, writer, "push", "origin", "main") +} + +// remoteManifest returns the manifest at the tip of origin/main as seen from +// clone, so a test asserts what actually converged on trunk (not the local tree). +func remoteManifest(t *testing.T, clone string) string { + t.Helper() + runGit(t, clone, "fetch", "origin", "main") + return runGit(t, clone, "show", "origin/main:.github/manifest.yaml") +} + +// TestCommitAndPush_ComponentReappliesAdjacentSiblingLeaf is the convergence +// regression test for the multi-component finalize race: two component lanes +// append adjacent leaves into one shared manifest on a busy trunk. Component "b" +// lands its leaf first; component "a"'s orchestrator then finds its push rejected +// non-fast-forward. The re-apply path must re-derive only "a"'s leaf onto the +// re-fetched trunk (which already carries "b"), so BOTH leaves survive. A textual +// rebase would conflict on the adjacent insertion; the sibling regression guard +// below proves that. +func TestCommitAndPush_ComponentReappliesAdjacentSiblingLeaf(t *testing.T) { + clone, statePath := seedComponentRemote(t) + landSiblingLeaf(t, clone) + + o := &Orchestrator{ + configPath: statePath, + environment: "prod", + component: "a", + baseDir: clone, + pushBackoff: time.Millisecond, + cicdFile: &config.CICDFile{ + State: map[string]*config.EnvState{ + "prod": {Version: "v1.0.0", SHA: "aaaaaaa"}, + }, + }, + } + + // Finalize writes the owned leaf onto the (now stale) local manifest, then + // commits and pushes. Mirror that order here. + if err := o.writeConfig(); err != nil { + t.Fatalf("writeConfig: %v", err) + } + if err := o.commitAndPush("v1.0.0"); err != nil { + t.Fatalf("commitAndPush must re-apply the owned leaf and converge, got: %v", err) + } + + got := remoteManifest(t, clone) + // Both racers' leaves and the pre-existing sibling must all be present: no + // leaf was dropped and the concurrent "b" write was not clobbered. + for _, want := range []string{"a:", "b:", "seed:", "v1.0.0", "v2.0.0", "v0.0.1"} { + if !strings.Contains(got, want) { + t.Errorf("converged origin manifest missing %q; got:\n%s", want, got) + } + } +} + +// TestPushWithRebaseRetry_TextualPathConflictsOnAdjacentLeaf is the regression +// guard proving the defect the re-apply fixes: the historical textual +// "git pull --rebase" path (no WithReapply) cannot land component "a"'s leaf when +// a concurrent "b" leaf occupies the adjacent line, because the two insertions +// conflict. It must return an error and leave the clone clean (not mid-rebase). +// This is the "old path drops/conflicts" half of the red-before evidence. +func TestPushWithRebaseRetry_TextualPathConflictsOnAdjacentLeaf(t *testing.T) { + clone, statePath := seedComponentRemote(t) + landSiblingLeaf(t, clone) + + // Stage and commit "a"'s adjacent leaf on the stale local seed, then push + // through the textual-rebase path with no re-apply hook. + current, err := os.ReadFile(statePath) + if err != nil { + t.Fatalf("read local manifest: %v", err) + } + patched, err := config.WriteScopedState(current, "ci", config.StateWrite{ + Component: "a", Env: "prod", State: &config.EnvState{Version: "v1.0.0", SHA: "aaaaaaa"}, + }) + if err != nil { + t.Fatalf("scoped write for owned leaf: %v", err) + } + if err := os.WriteFile(statePath, patched, 0o644); err != nil { + t.Fatalf("write local manifest: %v", err) + } + runGit(t, clone, "add", ".github/manifest.yaml") + runGit(t, clone, "commit", "-m", "a: land prod leaf") + + err = git.PushWithRebaseRetry(git.WithDir(clone), git.WithBackoff(time.Millisecond)) + if err == nil { + t.Fatal("textual rebase path unexpectedly converged adjacent leaves; expected a conflict error") + } + + // The clone must not be left mid-rebase after the abort. + for _, name := range []string{"rebase-merge", "rebase-apply"} { + if _, statErr := os.Stat(filepath.Join(clone, ".git", name)); statErr == nil { + t.Fatalf("repository left mid-rebase: .git/%s still present", name) + } + } +} diff --git a/internal/orchestrate/orchestrator.go b/internal/orchestrate/orchestrator.go index 98211d77..8350c020 100644 --- a/internal/orchestrate/orchestrator.go +++ b/internal/orchestrate/orchestrator.go @@ -619,11 +619,13 @@ func (o *Orchestrator) serializeState(current []byte, key string) ([]byte, error // componentStateWrites builds the component-scoped write the orchestrator owns // from its in-memory state: one state directive addressing // state.components.. for the orchestrated environment. It is -// re-appliable: the rebase-retry loop reruns writeConfig against the re-fetched -// trunk bytes on a rejected push, and each call deterministically re-derives the -// same owned leaf, so a concurrent sibling component's subtree is never rebuilt -// or dropped. A missing env state yields no write (a nil State on a StateWrite -// means delete), so an unexpected miss never becomes an accidental node delete. +// re-appliable: on a rejected push the retry loop re-fetches trunk, hard-resets +// to the upstream tip, and reapplyStateLeaf re-runs writeConfig against those +// fresh trunk bytes, so each attempt deterministically re-derives the same owned +// leaf onto whatever a concurrent sibling component already landed, and that +// sibling's subtree is never rebuilt or dropped. A missing env state yields no +// write (a nil State on a StateWrite means delete), so an unexpected miss never +// becomes an accidental node delete. func (o *Orchestrator) componentStateWrites() []config.StateWrite { st := o.cicdFile.State[o.environment] if st == nil { @@ -666,21 +668,61 @@ func (o *Orchestrator) commitAndPush(version string) error { 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 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. +// pushStateWithRetry pushes the committed state change, 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 delegates +// to git.PushWithRebaseRetry against the orchestrator's base directory. +// +// For a component-scoped run it passes WithReapply(reapplyStateLeaf): on a +// rejected push the shared loop re-fetches trunk and re-derives only this +// component's owned leaf onto the fresh bytes, so a concurrent sibling component's +// adjacent leaf survives where a textual rebase would conflict. The +// single-component path passes no re-apply hook, keeping the historical +// textual-rebase behaviour byte-for-byte and semantically identical (it writes +// the whole state node via WriteManifestState, so there is no adjacent-leaf +// sibling to converge against). func (o *Orchestrator) pushStateWithRetry() error { - if err := git.PushWithRebaseRetry(git.WithDir(o.baseDir), git.WithBackoff(o.pushBackoff)); err != nil { + opts := []git.Option{git.WithDir(o.baseDir), git.WithBackoff(o.pushBackoff)} + if o.component != "" { + opts = append(opts, git.WithReapply(o.reapplyStateLeaf)) + } + if err := git.PushWithRebaseRetry(opts...); err != nil { return err } log.Info("Committed and pushed state changes") return nil } +// reapplyStateLeaf re-derives the orchestrator's owned component state leaf onto +// the re-fetched trunk bytes and re-commits it. The push-retry loop calls it +// after hard-resetting the local branch to the upstream tip, so writeConfig reads +// the fresh trunk manifest (carrying any concurrent sibling leaf), node-patches +// only state.components.. back in via WriteScopedState, and the +// re-staged commit is pushed. It mirrors, in the git-checkout world, the +// re-appliable WriteScopedState closures the promote, hotfix, and rollback +// finalizers run against the Contents API. +func (o *Orchestrator) reapplyStateLeaf() error { + if err := o.writeConfig(); err != nil { + return err + } + if err := o.gitRun("add", o.configPath); err != nil { + return err + } + message := fmt.Sprintf("chore: update state for %s [skip ci]", o.environment) + cmd := exec.Command("git", "commit", "-m", message) + cmd.Dir = o.baseDir + if out, err := cmd.CombinedOutput(); err != nil { + // The owned leaf is already present on the re-fetched trunk (for example + // a same-component racer landed it), so there is nothing to re-commit; + // the following push is a clean no-op. Any other commit failure is real. + if strings.Contains(string(out), "nothing to commit") { + return nil + } + return fmt.Errorf("git commit failed: %s: %w", string(out), err) + } + return nil +} + // gitOutput runs a git command and returns stdout. func (o *Orchestrator) gitOutput(args ...string) (string, error) { cmd := exec.Command("git", args...)