From c2f9012fe4f68c3207f7034377f8c37742b90220 Mon Sep 17 00:00:00 2001 From: Joshua Temple Date: Fri, 10 Jul 2026 04:04:35 -0400 Subject: [PATCH] fix(orchestrate): re-derive component leaf onto fresh trunk before first state commit The component-scoped finalize committed its owned state leaf onto the checkout-time manifest and relied on a push rejection to surface staleness. That defends only when local HEAD is behind trunk. When HEAD already sits at the trunk tip but the working manifest is stale, the first push fast-forwards with no rejection, so the re-apply never runs and the stale sibling leaf lands, reverting a concurrent component's already-committed value. The Contents-API write path avoids this by re-reading fresh trunk on every attempt including the first. Make commitAndPush re-fetch trunk, hard-reset the working tree to the upstream tip, and node-patch only the owned leaf back in before the first commit, so no attempt ever rests on a stale base. Gate it on the component-scoped path; the single-component path keeps its historical commit-then-rebase behavior byte-identical. Export RefetchAndReset for the shared re-sync. Add shared-parent convergence tests covering sequential orderings, a concurrent goroutine wave, a lane behind trunk, and the first-attempt stale-working-tree case that fails before this change. Signed-off-by: Joshua Temple --- internal/git/git.go | 9 + internal/orchestrate/orchestrator.go | 20 ++ .../orchestrate/shared_parent_clobber_test.go | 284 ++++++++++++++++++ 3 files changed, 313 insertions(+) create mode 100644 internal/orchestrate/shared_parent_clobber_test.go diff --git a/internal/git/git.go b/internal/git/git.go index 1aab0897..3b993d27 100644 --- a/internal/git/git.go +++ b/internal/git/git.go @@ -336,6 +336,15 @@ func backoffForAttempt(base time.Duration, attempt int) time.Duration { return d + time.Duration(rand.Int64N(int64(base)+1)) } +// RefetchAndReset re-fetches trunk and hard-resets the working tree to the +// upstream tracking tip, so the working manifest holds the fresh trunk bytes +// (including any concurrent sibling leaf). It is exported for a state writer that +// wants to re-derive its owned leaf onto fresh trunk before its first commit, so +// that write never rests on a stale checkout base rather than relying solely on a +// push rejection to surface the staleness. It is the same operation the push +// retry loop performs on a rejected push. +func RefetchAndReset(dir string) error { return refetchAndReset(dir) } + // 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 diff --git a/internal/orchestrate/orchestrator.go b/internal/orchestrate/orchestrator.go index 25a80841..715aa2ed 100644 --- a/internal/orchestrate/orchestrator.go +++ b/internal/orchestrate/orchestrator.go @@ -685,6 +685,26 @@ func (o *Orchestrator) componentStateWrites() []config.StateWrite { // commitAndPush commits and pushes state changes. func (o *Orchestrator) commitAndPush(version string) error { + // For a component-scoped run, re-derive the owned leaf onto the CURRENT trunk + // manifest before the first commit: re-fetch trunk, hard-reset the working tree + // to the upstream tip, and node-patch only state.components.. + // back in through WriteScopedState. This makes the very first attempt rest on + // fresh trunk bytes rather than the checkout-time base, so a concurrent + // sibling's leaf is preserved even when the push then fast-forwards with no + // rejection to trigger the retry-loop re-apply. It mirrors the Contents-API + // write path, which re-reads fresh trunk on every attempt including the first. + // The single-component path (component == "") keeps its historical behavior + // byte-identical: it commits the checkout-base bytes and rebases only on a + // rejected push. + if o.component != "" { + if err := git.RefetchAndReset(o.baseDir); err != nil { + return err + } + if err := o.writeConfig(); err != nil { + return err + } + } + // Check if there are changes to commit status, _ := o.gitOutput("status", "--porcelain", o.configPath) if strings.TrimSpace(status) == "" { diff --git a/internal/orchestrate/shared_parent_clobber_test.go b/internal/orchestrate/shared_parent_clobber_test.go new file mode 100644 index 00000000..66b8170a --- /dev/null +++ b/internal/orchestrate/shared_parent_clobber_test.go @@ -0,0 +1,284 @@ +package orchestrate + +import ( + "fmt" + "path/filepath" + "strings" + "sync" + "testing" + "time" + + "github.com/stablekernel/cascade/internal/config" +) + +// seedSharedParentTwoComponents builds a bare remote seeded at ONE shared parent +// commit that already carries BOTH component leaves (api + web) at v0.1.0, then +// returns two working clones checked out at that shared parent. This models the +// shared_paths wave: one commit fires two component orchestrate lanes, each of +// which finalizes its own leaf into the single shared manifest on trunk. +func seedSharedParentTwoComponents(t *testing.T) (remote, cloneAPI, cloneWEB, statePathAPI, statePathWEB string) { + t.Helper() + + remote = t.TempDir() + runGit(t, remote, "init", "--bare", "-b", "main") + + const seed = "ci:\n" + + " state:\n" + + " components:\n" + + " api:\n" + + " dev:\n" + + " version: api-0.1.0-rc.0\n" + + " sha: aaaa000\n" + + " web:\n" + + " dev:\n" + + " version: web-0.1.0-rc.0\n" + + " sha: wwww000\n" + + seeder := t.TempDir() + runGit(t, seeder, "clone", remote, ".") + runGit(t, seeder, "checkout", "-b", "main") + writeFile(t, seeder, ".github/manifest.yaml", seed) + runGit(t, seeder, "add", ".github/manifest.yaml") + runGit(t, seeder, "commit", "-m", "chore: seed shared parent") + runGit(t, seeder, "push", "-u", "origin", "main") + + cloneAPI = t.TempDir() + runGit(t, cloneAPI, "clone", remote, ".") + statePathAPI = filepath.Join(cloneAPI, ".github", "manifest.yaml") + + cloneWEB = t.TempDir() + runGit(t, cloneWEB, "clone", remote, ".") + statePathWEB = filepath.Join(cloneWEB, ".github", "manifest.yaml") + + return remote, cloneAPI, cloneWEB, statePathAPI, statePathWEB +} + +func newComponentOrchestrator(statePath, component, baseDir, version string) *Orchestrator { + return &Orchestrator{ + configPath: statePath, + environment: "dev", + component: component, + baseDir: baseDir, + pushBackoff: time.Millisecond, + cicdFile: &config.CICDFile{ + State: map[string]*config.EnvState{ + "dev": {Version: version, SHA: component + "-new"}, + }, + }, + } +} + +// finalizeLane mirrors Finalize's write-then-commit-and-push sequence for one +// component lane. +func finalizeLane(t *testing.T, o *Orchestrator) { + t.Helper() + if err := o.writeConfig(); err != nil { + t.Fatalf("%s writeConfig: %v", o.component, err) + } + if err := o.commitAndPush(o.cicdFile.State["dev"].Version); err != nil { + t.Fatalf("%s commitAndPush: %v", o.component, err) + } +} + +func assertBothLeavesSurvive(t *testing.T, clone string, wantAPI, wantWEB string) { + t.Helper() + runGit(t, clone, "fetch", "origin", "main") + got := runGit(t, clone, "show", "origin/main:.github/manifest.yaml") + for _, want := range []string{wantAPI, wantWEB} { + if !strings.Contains(got, want) { + t.Errorf("converged trunk manifest missing %q; got:\n%s", want, got) + } + } +} + +// TestSharedParent_SequentialLanes_ApiFirst: both lanes writeConfig on the shared +// parent, then api pushes first (ff) and web pushes second (reject -> reapply). +func TestSharedParent_SequentialLanes_ApiFirst(t *testing.T) { + _, cloneAPI, cloneWEB, spAPI, spWEB := seedSharedParentTwoComponents(t) + + api := newComponentOrchestrator(spAPI, "api", cloneAPI, "api-0.2.0-rc.0") + web := newComponentOrchestrator(spWEB, "web", cloneWEB, "web-0.2.0-rc.0") + + // Both derive their leaf against the shared-parent bytes BEFORE either pushes. + if err := api.writeConfig(); err != nil { + t.Fatalf("api writeConfig: %v", err) + } + if err := web.writeConfig(); err != nil { + t.Fatalf("web writeConfig: %v", err) + } + if err := api.commitAndPush("api-0.2.0-rc.0"); err != nil { + t.Fatalf("api commitAndPush: %v", err) + } + if err := web.commitAndPush("web-0.2.0-rc.0"); err != nil { + t.Fatalf("web commitAndPush: %v", err) + } + + assertBothLeavesSurvive(t, cloneAPI, "api-0.2.0-rc.0", "web-0.2.0-rc.0") +} + +// TestSharedParent_SequentialLanes_WebFirst is the reverse ordering. +func TestSharedParent_SequentialLanes_WebFirst(t *testing.T) { + _, cloneAPI, cloneWEB, spAPI, spWEB := seedSharedParentTwoComponents(t) + + api := newComponentOrchestrator(spAPI, "api", cloneAPI, "api-0.2.0-rc.0") + web := newComponentOrchestrator(spWEB, "web", cloneWEB, "web-0.2.0-rc.0") + + if err := web.writeConfig(); err != nil { + t.Fatalf("web writeConfig: %v", err) + } + if err := api.writeConfig(); err != nil { + t.Fatalf("api writeConfig: %v", err) + } + if err := web.commitAndPush("web-0.2.0-rc.0"); err != nil { + t.Fatalf("web commitAndPush: %v", err) + } + if err := api.commitAndPush("api-0.2.0-rc.0"); err != nil { + t.Fatalf("api commitAndPush: %v", err) + } + + assertBothLeavesSurvive(t, cloneAPI, "api-0.2.0-rc.0", "web-0.2.0-rc.0") +} + +// TestSharedParent_FullLane_ApiFirst drives the full write+push per lane in +// strict sequence (api lane fully finalizes, then web lane fully finalizes), +// matching the empirical "two sequential state commits" observation. +func TestSharedParent_FullLane_ApiFirst(t *testing.T) { + _, cloneAPI, cloneWEB, spAPI, spWEB := seedSharedParentTwoComponents(t) + + api := newComponentOrchestrator(spAPI, "api", cloneAPI, "api-0.2.0-rc.0") + web := newComponentOrchestrator(spWEB, "web", cloneWEB, "web-0.2.0-rc.0") + + finalizeLane(t, api) + finalizeLane(t, web) + + assertBothLeavesSurvive(t, cloneAPI, "api-0.2.0-rc.0", "web-0.2.0-rc.0") +} + +// TestSharedParent_ConcurrentWave races N component lanes finalizing off one +// shared parent through real goroutines, mirroring a monorepo's shared_paths +// wave. Every owned leaf must survive on trunk regardless of the OS scheduler's +// interleaving. +func TestSharedParent_ConcurrentWave(t *testing.T) { + remote := t.TempDir() + runGit(t, remote, "init", "--bare", "-b", "main") + + comps := []string{"api", "web", "worker", "cron", "gateway"} + + // Seed a shared parent carrying every component's old leaf. + var seed strings.Builder + seed.WriteString("ci:\n state:\n components:\n") + for _, c := range comps { + fmt.Fprintf(&seed, " %s:\n dev:\n version: %s-0.1.0-rc.0\n sha: %s000\n", c, c, c) + } + seeder := t.TempDir() + runGit(t, seeder, "clone", remote, ".") + runGit(t, seeder, "checkout", "-b", "main") + writeFile(t, seeder, ".github/manifest.yaml", seed.String()) + runGit(t, seeder, "add", ".github/manifest.yaml") + runGit(t, seeder, "commit", "-m", "chore: seed shared parent") + runGit(t, seeder, "push", "-u", "origin", "main") + + var wg sync.WaitGroup + errs := make(chan error, len(comps)) + for _, c := range comps { + clone := t.TempDir() + runGit(t, clone, "clone", remote, ".") + sp := filepath.Join(clone, ".github", "manifest.yaml") + o := newComponentOrchestrator(sp, c, clone, c+"-0.2.0-rc.0") + wg.Add(1) + go func(o *Orchestrator) { + defer wg.Done() + if err := o.writeConfig(); err != nil { + errs <- err + return + } + if err := o.commitAndPush(o.cicdFile.State["dev"].Version); err != nil { + errs <- err + } + }(o) + } + wg.Wait() + close(errs) + for err := range errs { + if err != nil { + t.Fatalf("lane failed: %v", err) + } + } + + verify := t.TempDir() + runGit(t, verify, "clone", remote, ".") + got := runGit(t, verify, "show", "origin/main:.github/manifest.yaml") + for _, c := range comps { + want := c + "-0.2.0-rc.0" + if !strings.Contains(got, want) { + t.Errorf("converged trunk missing %q (clobbered); got:\n%s", want, got) + } + } +} + +// TestSharedParent_LaneBehindTrunk models a lane whose local checkout is behind +// trunk: the sibling already landed its NEW leaf on trunk before this lane +// pushes. The lane's first push must reject and the re-apply must preserve the +// sibling's new value rather than reverting it to the checkout-time value. +func TestSharedParent_LaneBehindTrunk(t *testing.T) { + remote, cloneAPI, cloneWEB, spAPI, spWEB := seedSharedParentTwoComponents(t) + _ = remote + + // api lands its NEW leaf on trunk first (fully finalizes). + api := newComponentOrchestrator(spAPI, "api", cloneAPI, "api-0.2.0-rc.0") + finalizeLane(t, api) + + // web's clone is still at the shared parent (behind trunk): its on-disk + // manifest shows api-0.1.0. It derives its leaf against that stale base. + web := newComponentOrchestrator(spWEB, "web", cloneWEB, "web-0.2.0-rc.0") + finalizeLane(t, web) + + // api's NEW leaf must NOT have been reverted by web's finalize. + assertBothLeavesSurvive(t, cloneWEB, "api-0.2.0-rc.0", "web-0.2.0-rc.0") + got := runGit(t, cloneWEB, "show", "origin/main:.github/manifest.yaml") + if strings.Contains(got, "api-0.1.0-rc.0") { + t.Errorf("web finalize reverted api to the stale checkout value; got:\n%s", got) + } +} + +// TestSharedParent_FirstAttemptReDerivesOntoFreshTrunk is the direct regression +// guard for the first-attempt clobber: a lane whose working manifest is stale +// (still shows the sibling's OLD value) while its local HEAD already sits at the +// trunk tip. A plain first push then fast-forwards with NO rejection, so the +// rejection-driven re-apply never runs and the stale sibling value lands, +// reverting the sibling. commitAndPush must re-derive the owned leaf onto fresh +// trunk before the first commit so the sibling survives. Fails before the fix, +// passes after. +func TestSharedParent_FirstAttemptReDerivesOntoFreshTrunk(t *testing.T) { + _, cloneAPI, cloneWEB, spAPI, spWEB := seedSharedParentTwoComponents(t) + + // api fully finalizes first: trunk is now api-0.2.0, web-0.1.0. + api := newComponentOrchestrator(spAPI, "api", cloneAPI, "api-0.2.0-rc.0") + finalizeLane(t, api) + + // Sync web's clone so its local HEAD is the trunk tip (a plain push here would + // fast-forward), then leave a stale working manifest showing api's OLD value. + // This is the state a rejection can never surface. + runGit(t, cloneWEB, "fetch", "origin", "main") + runGit(t, cloneWEB, "reset", "--hard", "origin/main") + stale := "ci:\n state:\n components:\n" + + " api:\n dev:\n version: api-0.1.0-rc.0\n sha: aaaa000\n" + + " web:\n dev:\n version: web-0.2.0-rc.0\n sha: web-new\n" + writeFile(t, cloneWEB, ".github/manifest.yaml", stale) + + web := newComponentOrchestrator(spWEB, "web", cloneWEB, "web-0.2.0-rc.0") + if err := web.commitAndPush("web-0.2.0-rc.0"); err != nil { + t.Fatalf("web commitAndPush: %v", err) + } + + runGit(t, cloneWEB, "fetch", "origin", "main") + got := runGit(t, cloneWEB, "show", "origin/main:.github/manifest.yaml") + if strings.Contains(got, "api-0.1.0-rc.0") { + t.Fatalf("first-attempt clobber: api reverted to 0.1.0 with no reject to catch it:\n%s", got) + } + for _, want := range []string{"api-0.2.0-rc.0", "web-0.2.0-rc.0"} { + if !strings.Contains(got, want) { + t.Errorf("converged trunk missing %q:\n%s", want, got) + } + } +}