fix: retry manifest state writes on contents API 409 conflicts

e2e/scenarios/hotfix/hotfix-generation-threshold.yaml

@@ -40,7 +40,8 @@ steps:
             - "pull_request:"
             - "types: [closed]"
             - "'env/*'"
-            - "group: hotfix-"
+            - "format('hotfix-finalize-{0}', github.repository)"
+            - "format('hotfix-{0}', github.event.inputs.target_env)"
             - "  plan:"
             - "  apply:"
             - "  check:"

internal/generate/hotfix.go

@@ -183,11 +183,21 @@ func (g *HotfixGenerator) writePermissions(sb *strings.Builder) {
 	writeTopLevelPermissions(sb, base)
 }
 
-// writeConcurrency keys the group per target environment. On dispatch the env is
-// the operator input; on pull_request close it is derived from the base ref.
+// writeConcurrency keys the apply (dispatch) path per target environment, but
+// keys the finalize (pull_request close) path on a per-repository constant so
+// concurrent per-environment finalize runs QUEUE rather than race.
+//
+// Each env's finalize commits the manifest to trunk through the Contents API.
+// Keyed per base ref, two envs whose resolution PRs close together fall into
+// different concurrency groups and PUT in parallel; the second writer's blob SHA
+// is stale and GitHub returns 409, dropping that env's state. A per-repo finalize
+// group with cancel-in-progress: false serializes those writes instead. This is
+// defense-in-depth: the durable fix is the Contents API 409 read-modify-write
+// retry in internal/statewrite, which still protects against any other writer
+// (orchestrate, promote, rollback) that a manifest-global group cannot serialize.
 func (g *HotfixGenerator) writeConcurrency(sb *strings.Builder) {
 	sb.WriteString("concurrency:\n")
-	sb.WriteString("  group: hotfix-${{ github.event.inputs.target_env || github.event.pull_request.base.ref }}\n")
+	sb.WriteString("  group: ${{ github.event_name == 'pull_request' && format('hotfix-finalize-{0}', github.repository) || format('hotfix-{0}', github.event.inputs.target_env) }}\n")
 	sb.WriteString("  cancel-in-progress: false\n")
 	sb.WriteString("\n")
 }

internal/generate/hotfix_test.go

@@ -172,7 +172,16 @@ func TestHotfixGenerator_Concurrency(t *testing.T) {
 	gen := NewHotfixGenerator(threeEnvHotfixConfig(), "")
 	content, err := gen.Generate()
 	require.NoError(t, err)
-	assert.Contains(t, content, "group: hotfix-")
+
+	// The finalize (pull_request close) path keys on a per-repository constant so
+	// concurrent per-environment finalize runs queue instead of racing on the
+	// shared manifest blob SHA. The apply (dispatch) path stays keyed per target
+	// environment so unrelated cherry-picks still run in parallel.
+	assert.Contains(t, content,
+		"github.event_name == 'pull_request' && format('hotfix-finalize-{0}', github.repository)",
+		"finalize must use a per-repository concurrency group so writes queue")
+	assert.Contains(t, content, "format('hotfix-{0}', github.event.inputs.target_env)",
+		"the dispatch path must stay keyed per target environment")
 	assert.Contains(t, content, "cancel-in-progress: false")
 }
 

internal/hotfix/finalize.go

@@ -1,7 +1,6 @@
 package hotfix
 
 import (
-	"encoding/base64"
 	"fmt"
 	"os"
 	"os/exec"
@@ -13,6 +12,7 @@ import (
 	"github.com/stablekernel/cascade/internal/config"
 	"github.com/stablekernel/cascade/internal/git"
 	"github.com/stablekernel/cascade/internal/release"
+	"github.com/stablekernel/cascade/internal/statewrite"
 	"github.com/stablekernel/cascade/internal/version"
 )
 
@@ -135,51 +135,39 @@ func (execTagLister) ListTags() ([]string, error) {
 type gitStatePusher struct{}
 
 func (gitStatePusher) CommitAndPush(path, branch, message string) error {
-	if isRealGitHub() {
-		return writeStateViaAPI(path, branch, message)
-	}
 	return commitAndPushGit(path, branch, message)
 }
 
-// isRealGitHub reports whether the workflow runs on github.com rather than an
-// act/gitea e2e environment, detected by GITHUB_SERVER_URL as the generated
-// dispatch steps do.
-func isRealGitHub() bool {
-	server := os.Getenv("GITHUB_SERVER_URL")
-	return server == "" || server == "https://github.com"
+// apiStatePusher commits the manifest to trunk through the GitHub Contents REST
+// API using the shared optimistic-lock retry loop, so concurrent env finalizers
+// that each touch only their own env state merge rather than clobbering each
+// other on the file blob SHA. mutate re-applies this hotfix's state change onto
+// whatever trunk bytes the loop fetches.
+type apiStatePusher struct {
+	mutate statewrite.Mutate
 }
 
-// writeStateViaAPI writes the manifest to the trunk branch through the GitHub
-// Contents REST API using the gh CLI, producing a signed (Verified) commit.
-func writeStateViaAPI(path, branch, message string) error {
+func (p apiStatePusher) CommitAndPush(path, branch, message string) error {
 	repo := os.Getenv("GITHUB_REPOSITORY")
 	if repo == "" {
 		return fmt.Errorf("GITHUB_REPOSITORY is not set; cannot write state via API")
 	}
+	return statewrite.CommitWithRetry(statewrite.Options{
+		Client:  statewrite.NewGHClient(),
+		Repo:    repo,
+		Path:    path,
+		Ref:     branch,
+		Message: message,
+		Mutate:  p.mutate,
+	})
+}
 
-	data, err := os.ReadFile(path)
-	if err != nil {
-		return fmt.Errorf("read manifest failed: %w", err)
-	}
-	contentB64 := base64.StdEncoding.EncodeToString(data)
-	apiPath := fmt.Sprintf("repos/%s/contents/%s", repo, path)
-
-	shaOut, _ := exec.Command("gh", "api", fmt.Sprintf("%s?ref=%s", apiPath, branch), "--jq", ".sha").Output()
-	currentSHA := strings.TrimSpace(string(shaOut))
-
-	args := []string{
-		"api", apiPath, "-X", "PUT",
-		"-f", "message=" + message,
-		"-f", "content=" + contentB64,
-		"-f", "branch=" + branch,
-	}
-	if currentSHA != "" {
-		args = append(args, "-f", "sha="+currentSHA)
-	}
-	if out, err := exec.Command("gh", args...).CombinedOutput(); err != nil {
-		return fmt.Errorf("state write via API failed: %s: %w", strings.TrimSpace(string(out)), err)
-	}
-	return nil
+// isRealGitHub reports whether the workflow runs on github.com rather than an
+// act/gitea e2e environment, detected by GITHUB_SERVER_URL as the generated
+// dispatch steps do.
+func isRealGitHub() bool {
+	server := os.Getenv("GITHUB_SERVER_URL")
+	return server == "" || server == "https://github.com"
 }
 
 // commitAndPushGit commits the manifest and pushes it to the trunk branch with
@@ -227,6 +215,11 @@ type Finalizer struct {
 	pusher      statePusher
 	tipReader   gitTipReader
 	trunkReader trunkStateReader
+
+	// pusherInjected records whether a caller supplied an explicit statePusher.
+	// When true, Finalize uses that pusher verbatim (tests inject a recorder);
+	// when false on real GitHub, Finalize swaps in the API retry pusher.
+	pusherInjected bool
 }
 
 // FinalizerOptions carries the required inputs for NewFinalizer.
@@ -270,6 +263,7 @@ func WithStatePusher(p statePusher) FinalizeOption {
 	return func(f *Finalizer) {
 		if p != nil {
 			f.pusher = p
+			f.pusherInjected = true
 		}
 	}
 }
@@ -433,32 +427,45 @@ func (f *Finalizer) Finalize(targetEnv, mergeSHA string, fixSHAs []string, baseS
 		return nil
 	}
 
-	// Snapshot the prior state into the deploy-history ring (newest first,
-	// bounded). The idempotency gate above already returned when the state
-	// records mergeSHA, so this records a genuine transition; the gate inside
-	// PushPreviousSnapshot is belt-and-suspenders.
-	prior.PushPreviousSnapshot(mergeSHA)
-
-	// Carry BaseSHA forward when already diverged; otherwise anchor it now.
-	if prior.BaseSHA == "" {
-		prior.BaseSHA = baseSHA
+	// Apply the state mutation in place onto the trunk manifest, snapshotting the
+	// prior state into the Previous ring and writing the divergence fields and
+	// substates. Extracted so the same change can be re-applied against freshly
+	// fetched trunk bytes inside the optimistic-lock retry loop below.
+	if err := f.applyHotfixState(f.cicd, targetEnv, mergeSHA, hotfixVersion, baseSHA, timestamp, fixSHAs); err != nil {
+		return err
 	}
-	prior.Patches = append(prior.Patches, fixSHAs...)
-	prior.Ref = branch
-	prior.SHA = mergeSHA
-	prior.Version = hotfixVersion
-	prior.CommittedAt = timestamp
-	prior.CommittedBy = f.actor
-
-	// Record per-deploy and per-build substates for successful jobs.
-	f.recordSubstates(prior, mergeSHA, hotfixVersion, timestamp)
 
 	if err := f.writeConfig(); err != nil {
 		return err
 	}
 
 	message := fmt.Sprintf("chore: record hotfix %s on %s [skip ci]", hotfixVersion, targetEnv)
-	if err := f.pusher.CommitAndPush(f.configPath, trunk, message); err != nil {
+
+	pusher := f.pusher
+	if !f.pusherInjected && isRealGitHub() {
+		capturedVersion := hotfixVersion
+		capturedTimestamp := timestamp
+		capturedBaseSHA := baseSHA
+		capturedFixSHAs := append([]string(nil), fixSHAs...)
+		capturedTarget := targetEnv
+		capturedMerge := mergeSHA
+		key := f.manifestKey
+		pusher = apiStatePusher{mutate: func(current []byte) ([]byte, error) {
+			fresh, err := config.ParseManifestBytes(current, key)
+			if err != nil {
+				return nil, fmt.Errorf("parsing current manifest: %w", err)
+			}
+			if err := f.applyHotfixState(fresh, capturedTarget, capturedMerge, capturedVersion, capturedBaseSHA, capturedTimestamp, capturedFixSHAs); err != nil {
+				return nil, err
+			}
+			data, err := yaml.Marshal(map[string]any{key: fresh})
+			if err != nil {
+				return nil, fmt.Errorf("marshaling merged manifest: %w", err)
+			}
+			return data, nil
+		}}
+	}
+	if err := pusher.CommitAndPush(f.configPath, trunk, message); err != nil {
 		return fmt.Errorf("committing hotfix state: %w", err)
 	}
 
@@ -471,6 +478,37 @@ func (f *Finalizer) Finalize(targetEnv, mergeSHA string, fixSHAs []string, baseS
 	return nil
 }
 
+// applyHotfixState applies the hotfix state mutation for targetEnv onto cicd
+// using pre-computed values, so it can be re-applied against freshly fetched
+// trunk bytes inside the optimistic-lock retry loop. It is idempotent: when the
+// manifest already records mergeSHA for targetEnv the call is a no-op, so a retry
+// (or rerun) neither double-appends patches nor re-snapshots the Previous ring.
+func (f *Finalizer) applyHotfixState(cicd *config.CICDFile, targetEnv, mergeSHA, hotfixVersion, baseSHA, timestamp string, fixSHAs []string) error {
+	if cicd.State == nil {
+		cicd.State = make(map[string]*config.EnvState)
+	}
+	prior := cicd.State[targetEnv]
+	if prior == nil {
+		prior = &config.EnvState{}
+		cicd.State[targetEnv] = prior
+	}
+	if prior.SHA == mergeSHA {
+		return nil
+	}
+	prior.PushPreviousSnapshot(mergeSHA)
+	if prior.BaseSHA == "" {
+		prior.BaseSHA = baseSHA
+	}
+	prior.Patches = append(prior.Patches, fixSHAs...)
+	prior.Ref = envBranch(targetEnv)
+	prior.SHA = mergeSHA
+	prior.Version = hotfixVersion
+	prior.CommittedAt = timestamp
+	prior.CommittedBy = f.actor
+	f.recordSubstates(prior, mergeSHA, hotfixVersion, timestamp)
+	return nil
+}
+
 // readTrunkManifest fetches the manifest as it exists on the trunk branch and
 // returns the parsed manifest. It is read from trunk because promote finalize
 // writes env state only to trunk; the env branch the hotfix merged into lags

internal/hotfix/finalize_test.go

@@ -138,6 +138,33 @@ func loadState(t *testing.T, manifest, env string) *config.EnvState {
 	return cicd.State[env]
 }
 
+// TestFinalizeHotfixMutatePreservesUntouchedEnv verifies that applying the
+// hotfix state mutation for one env on a manifest leaves every other env's
+// recorded state untouched, so re-applying it against re-fetched trunk bytes in
+// the optimistic-lock loop merges rather than clobbers concurrent writers.
+func TestFinalizeHotfixMutatePreservesUntouchedEnv(t *testing.T) {
+	cicd := &config.CICDFile{State: map[string]*config.EnvState{
+		"dev":     {SHA: "dev-sha", Version: "v1.0.0"},
+		"staging": {SHA: "stg-sha", Version: "v1.1.0"},
+	}}
+	f := &Finalizer{actor: "tester", manifestKey: "ci"}
+
+	err := f.applyHotfixState(cicd, "staging", "merge-sha", "v1.1.1", "base-sha", "2026-01-01T00:00:00Z", []string{"fix-sha"})
+	if err != nil {
+		t.Fatalf("applyHotfixState: %v", err)
+	}
+
+	if cicd.State["dev"].SHA != "dev-sha" {
+		t.Errorf("dev.sha = %q, want unchanged dev-sha", cicd.State["dev"].SHA)
+	}
+	if cicd.State["staging"].SHA != "merge-sha" {
+		t.Errorf("staging.sha = %q, want merge-sha", cicd.State["staging"].SHA)
+	}
+	if cicd.State["staging"].Version != "v1.1.1" {
+		t.Errorf("staging.version = %q, want v1.1.1", cicd.State["staging"].Version)
+	}
+}
+
 func TestFinalize_WritesDivergedState(t *testing.T) {
 	newScratchRepo(t)
 	base := commitFile(t, "a.txt", "one", "first")