fix(orchestrate): re-apply the component state leaf on a rejected finalize push

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,47 +398,73 @@ 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
 		}
 
 		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)
+		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)
 }
 

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)
+		}
+	}
+}