diff --git a/.github/workflows/orchestrate.yaml b/.github/workflows/orchestrate.yaml index 1fd840e5..aae8fca7 100644 --- a/.github/workflows/orchestrate.yaml +++ b/.github/workflows/orchestrate.yaml @@ -194,7 +194,8 @@ jobs: if [[ "$GITHUB_SERVER_URL" != "https://github.com" ]]; then # act/gitea e2e: no GitHub API, and the trunk is neither protected nor # signature-checked, so push the state commit directly with retries. - for attempt in 1 2 3 4 5; do + for attempt in 1 2 3 4 5 6 7 8 9 10; do + echo "cascade-state-write: attempt=$attempt/10" git fetch origin "$BRANCH" git reset --hard "origin/$BRANCH" apply_state_edits @@ -206,19 +207,24 @@ jobs: git commit -m "chore: update state [skip ci]" if git push origin "HEAD:$BRANCH"; then echo "Pushed state on attempt $attempt" + echo "cascade-state-write: ok attempt=$attempt" exit 0 fi echo "Push attempt $attempt rejected (likely concurrent run); retrying..." >&2 - sleep $((RANDOM % 5 + 2)) + backoff=$(( 1 << (attempt - 1) )) + if [ "$backoff" -gt 8 ]; then backoff=8; fi + sleep $(( backoff + RANDOM % 2 )) done - echo "::error::Failed to push state after 5 attempts" >&2 + echo "cascade-state-write: exhausted attempts=10" >&2 + echo "::error::Failed to push state after 10 attempts" >&2 exit 1 fi # Real GitHub: write state through the Contents REST API. API commits are # signed by GitHub (Verified) and, with a bypass-capable token, update the # trunk even when a required status check protects it. - for attempt in 1 2 3 4 5; do + for attempt in 1 2 3 4 5 6 7 8 9 10; do + echo "cascade-state-write: attempt=$attempt/10" git fetch origin "$BRANCH" git reset --hard "origin/$BRANCH" apply_state_edits @@ -241,12 +247,16 @@ jobs: fi if gh api "${API_ARGS[@]}" >/dev/null; then echo "Pushed state via API on attempt $attempt" + echo "cascade-state-write: ok attempt=$attempt" exit 0 fi echo "State write attempt $attempt failed (likely concurrent run); retrying..." >&2 - sleep $((RANDOM % 5 + 2)) + backoff=$(( 1 << (attempt - 1) )) + if [ "$backoff" -gt 8 ]; then backoff=8; fi + sleep $(( backoff + RANDOM % 2 )) done - echo "::error::Failed to write state via API after 5 attempts" >&2 + echo "cascade-state-write: exhausted attempts=10" >&2 + echo "::error::Failed to write state via API after 10 attempts" >&2 exit 1 - name: Check for Failures if: contains(fromJSON('["failure", "cancelled"]'), needs.validate.result) || contains(fromJSON('["failure", "cancelled"]'), needs.build-cli.result) diff --git a/e2e/scenarios/08-state-push-retry.yaml b/e2e/scenarios/08-state-push-retry.yaml index 0bd2428d..7ffcc3e6 100644 --- a/e2e/scenarios/08-state-push-retry.yaml +++ b/e2e/scenarios/08-state-push-retry.yaml @@ -29,7 +29,10 @@ steps: - path: ".github/workflows/orchestrate.yaml" contains: - "Update Manifest" - - "for attempt in 1 2 3 4 5" + - "for attempt in 1 2 3 4 5 6 7 8 9 10" + - "cascade-state-write: attempt=$attempt/10" + - "cascade-state-write: ok attempt=$attempt" + - "cascade-state-write: exhausted attempts=10" - "git fetch origin" - "git reset --hard" - "apply_state_edits" diff --git a/e2e/scenarios/09-single-env-repo.yaml b/e2e/scenarios/09-single-env-repo.yaml index 7adec4c7..704f32e6 100644 --- a/e2e/scenarios/09-single-env-repo.yaml +++ b/e2e/scenarios/09-single-env-repo.yaml @@ -54,7 +54,8 @@ steps: contains: - "Update Latest Release State" - "apply_release_state_edits" - - "for attempt in 1 2 3 4 5" + - "for attempt in 1 2 3 4 5 6 7 8 9 10" + - "cascade-state-write: attempt=$attempt/10" - "git fetch origin" - "git reset --hard" diff --git a/internal/generate/state_write.go b/internal/generate/state_write.go index ad718c70..9a11a3b7 100644 --- a/internal/generate/state_write.go +++ b/internal/generate/state_write.go @@ -87,7 +87,8 @@ func writeStateCommitPush(sb *strings.Builder, indent string, params stateWriteP // gitea/act path: keep the existing git fetch/reset/reapply/commit/push loop. w(" # act/gitea e2e: no GitHub API, and the trunk is neither protected nor") w(" # signature-checked, so push the state commit directly with retries.") - w(" for attempt in 1 2 3 4 5; do") + w(" for attempt in 1 2 3 4 5 6 7 8 9 10; do") + w(" echo \"cascade-state-write: attempt=$attempt/10\"") w(" git fetch origin \"$BRANCH\"") w(" git reset --hard \"origin/$BRANCH\"") w(" %s", params.applyFn) @@ -99,12 +100,14 @@ func writeStateCommitPush(sb *strings.Builder, indent string, params stateWriteP writeShellCommit(sb, indent+" ", params.commitMessage) w(" if git push origin \"HEAD:$BRANCH\"; then") w(" echo \"%s on attempt $attempt\"", params.successLabel) + w(" echo \"cascade-state-write: ok attempt=$attempt\"") w(" exit 0") w(" fi") w(" echo \"Push attempt $attempt rejected (likely concurrent run); retrying...\" >&2") - w(" sleep $((RANDOM % 5 + 2))") + writeStateBackoffSleep(sb, indent+" ") w(" done") - w(" echo \"::error::Failed to push state after 5 attempts\" >&2") + w(" echo \"cascade-state-write: exhausted attempts=10\" >&2") + w(" echo \"::error::Failed to push state after 10 attempts\" >&2") w(" exit 1") w("fi") w("") @@ -113,7 +116,8 @@ func writeStateCommitPush(sb *strings.Builder, indent string, params stateWriteP w("# Real GitHub: write state through the Contents REST API. API commits are") w("# signed by GitHub (Verified) and, with a bypass-capable token, update the") w("# trunk even when a required status check protects it.") - w("for attempt in 1 2 3 4 5; do") + w("for attempt in 1 2 3 4 5 6 7 8 9 10; do") + w(" echo \"cascade-state-write: attempt=$attempt/10\"") w(" git fetch origin \"$BRANCH\"") w(" git reset --hard \"origin/$BRANCH\"") w(" %s", params.applyFn) @@ -139,15 +143,31 @@ func writeStateCommitPush(sb *strings.Builder, indent string, params stateWriteP w(" fi") w(" if gh api \"${API_ARGS[@]}\" >/dev/null; then") w(" echo \"%s via API on attempt $attempt\"", params.successLabel) + w(" echo \"cascade-state-write: ok attempt=$attempt\"") w(" exit 0") w(" fi") w(" echo \"State write attempt $attempt failed (likely concurrent run); retrying...\" >&2") - w(" sleep $((RANDOM % 5 + 2))") + writeStateBackoffSleep(sb, indent+" ") w("done") - w("echo \"::error::Failed to write state via API after 5 attempts\" >&2") + w("echo \"cascade-state-write: exhausted attempts=10\" >&2") + w("echo \"::error::Failed to write state via API after 10 attempts\" >&2") w("exit 1") } +// writeStateBackoffSleep emits an exponential-with-jitter backoff sleep for the +// state-write retry loop at the given indent. The wait doubles per attempt +// (1s, 2s, 4s, 8s) capped at eight seconds, plus a random zero-or-one-second +// jitter, so concurrent writers racing one trunk branch de-synchronize rather +// than colliding again in lockstep. It mirrors the exponential jittered backoff +// the git and Contents-API Go write paths use. The loop variable is "$attempt". +func writeStateBackoffSleep(sb *strings.Builder, indent string) { + // These lines contain literal '%' (RANDOM % 2); write them verbatim so the + // caller's fmt-based line writer never treats '%' as a format verb. + sb.WriteString(indent + "backoff=$(( 1 << (attempt - 1) ))\n") + sb.WriteString(indent + "if [ \"$backoff\" -gt 8 ]; then backoff=8; fi\n") + sb.WriteString(indent + "sleep $(( backoff + RANDOM % 2 ))\n") +} + // writeShellCommit emits a `git commit -m` line for a possibly multi-line // message at the given indent. Multi-line messages preserve the existing // behavior of embedding a trailer line under the subject. diff --git a/internal/generate/state_write_test.go b/internal/generate/state_write_test.go index 30c33b3e..a0afc52a 100644 --- a/internal/generate/state_write_test.go +++ b/internal/generate/state_write_test.go @@ -212,6 +212,45 @@ func TestStateWriteHonorsCustomGitIdentity(t *testing.T) { assertAPIAuthorStamp(t, content, "release-bot", "release-bot@example.com") } +// TestStateWriteRetryCeilingAndConvergenceMarker asserts the emitted state-write +// loop retries ten times on both the git-push and Contents-API branches, emits +// the greppable "cascade-state-write: attempt=N/10" marker per attempt, an "ok" +// marker on success and an "exhausted" marker on failure, and has dropped the old +// five-attempt bound and fixed RANDOM sleep. The marker lets a live concurrency +// proof grep every lane for convergence without any exhaustion. +func TestStateWriteRetryCeilingAndConvergenceMarker(t *testing.T) { + tmpDir := t.TempDir() + require.NoError(t, os.MkdirAll(filepath.Join(tmpDir, ".github/workflows"), 0755)) + require.NoError(t, os.WriteFile(filepath.Join(tmpDir, ".github/workflows/build.yaml"), []byte("on:\n workflow_call:\n"), 0644)) + + cfg := &config.TrunkConfig{ + TrunkBranch: "main", + Environments: []string{"dev"}, + Builds: []config.BuildConfig{ + {Name: "app", Workflow: ".github/workflows/build.yaml", Triggers: []string{"src/**"}}, + }, + } + + content, err := NewGenerator(cfg, tmpDir).Generate() + require.NoError(t, err) + + // Both the git-push (act/gitea) and the Contents-API (real GitHub) branches + // carry the raised loop bound. + assert.Equal(t, 2, strings.Count(content, "for attempt in 1 2 3 4 5 6 7 8 9 10"), + "both state-write branches must retry ten times") + // The greppable convergence markers, on both branches. + assert.Equal(t, 2, strings.Count(content, `cascade-state-write: attempt=$attempt/10`), + "each branch must emit the per-attempt convergence marker") + assert.Contains(t, content, `cascade-state-write: ok`, + "a successful write must emit the ok convergence marker") + assert.Equal(t, 2, strings.Count(content, `cascade-state-write: exhausted attempts=10`), + "each branch must emit the exhaustion marker so a live proof asserts its absence") + + // The old five-attempt bound and fixed RANDOM sleep must be gone. + assert.NotContains(t, content, "after 5 attempts", "the old five-attempt failure text must be gone") + assert.NotContains(t, content, "RANDOM % 5 + 2", "the fixed RANDOM sleep must be replaced by exponential jittered backoff") +} + // TestStateWriteNoEmDash guards the hard project rule that generated output // contains no em dashes. func TestStateWriteNoEmDash(t *testing.T) { diff --git a/internal/generate/testdata/byte_identical_baseline/.github__workflows__orchestrate.yaml.golden b/internal/generate/testdata/byte_identical_baseline/.github__workflows__orchestrate.yaml.golden index c59686af..f275092d 100644 --- a/internal/generate/testdata/byte_identical_baseline/.github__workflows__orchestrate.yaml.golden +++ b/internal/generate/testdata/byte_identical_baseline/.github__workflows__orchestrate.yaml.golden @@ -264,7 +264,8 @@ jobs: if [[ "$GITHUB_SERVER_URL" != "https://github.com" ]]; then # act/gitea e2e: no GitHub API, and the trunk is neither protected nor # signature-checked, so push the state commit directly with retries. - for attempt in 1 2 3 4 5; do + for attempt in 1 2 3 4 5 6 7 8 9 10; do + echo "cascade-state-write: attempt=$attempt/10" git fetch origin "$BRANCH" git reset --hard "origin/$BRANCH" apply_state_edits @@ -276,19 +277,24 @@ jobs: git commit -m "chore: update state for $ENVIRONMENT [skip ci]" if git push origin "HEAD:$BRANCH"; then echo "Pushed state on attempt $attempt" + echo "cascade-state-write: ok attempt=$attempt" exit 0 fi echo "Push attempt $attempt rejected (likely concurrent run); retrying..." >&2 - sleep $((RANDOM % 5 + 2)) + backoff=$(( 1 << (attempt - 1) )) + if [ "$backoff" -gt 8 ]; then backoff=8; fi + sleep $(( backoff + RANDOM % 2 )) done - echo "::error::Failed to push state after 5 attempts" >&2 + echo "cascade-state-write: exhausted attempts=10" >&2 + echo "::error::Failed to push state after 10 attempts" >&2 exit 1 fi # Real GitHub: write state through the Contents REST API. API commits are # signed by GitHub (Verified) and, with a bypass-capable token, update the # trunk even when a required status check protects it. - for attempt in 1 2 3 4 5; do + for attempt in 1 2 3 4 5 6 7 8 9 10; do + echo "cascade-state-write: attempt=$attempt/10" git fetch origin "$BRANCH" git reset --hard "origin/$BRANCH" apply_state_edits @@ -311,12 +317,16 @@ jobs: fi if gh api "${API_ARGS[@]}" >/dev/null; then echo "Pushed state via API on attempt $attempt" + echo "cascade-state-write: ok attempt=$attempt" exit 0 fi echo "State write attempt $attempt failed (likely concurrent run); retrying..." >&2 - sleep $((RANDOM % 5 + 2)) + backoff=$(( 1 << (attempt - 1) )) + if [ "$backoff" -gt 8 ]; then backoff=8; fi + sleep $(( backoff + RANDOM % 2 )) done - echo "::error::Failed to write state via API after 5 attempts" >&2 + echo "cascade-state-write: exhausted attempts=10" >&2 + echo "::error::Failed to write state via API after 10 attempts" >&2 exit 1 - name: Check for Failures if: contains(fromJSON('["failure", "cancelled"]'), needs.build-image.result) || contains(fromJSON('["failure", "cancelled"]'), needs.build-bundle.result) || contains(fromJSON('["failure", "cancelled"]'), needs.build-docs.result) || contains(fromJSON('["failure", "cancelled"]'), needs.deploy-app.result) || contains(fromJSON('["failure", "cancelled"]'), needs.deploy-sidecar.result) diff --git a/internal/statewrite/apiwrite.go b/internal/statewrite/apiwrite.go index c5131377..ff276615 100644 --- a/internal/statewrite/apiwrite.go +++ b/internal/statewrite/apiwrite.go @@ -13,18 +13,30 @@ package statewrite import ( "fmt" + "math/rand/v2" "strings" "time" + + "github.com/stablekernel/cascade/internal/log" ) -// maxAttempts bounds the read-modify-write retry loop. Five attempts comfortably -// absorbs the handful of envs that can finalize in parallel without masking a -// genuinely stuck write. -const maxAttempts = 5 +// maxAttempts bounds the read-modify-write retry loop. It is sized to survive a +// realistic concurrent wave (every component of a monorepo racing to write its +// own leaf into one shared manifest file on trunk) rather than only the handful +// of envs that finalize in parallel, and is aligned with the git push-retry +// ceiling in internal/git so a single grep of the "cascade-state-write" marker +// covers every write path. +const maxAttempts = 10 + +// retryBaseBackoff is the base delay between optimistic-lock retries. The +// effective wait grows exponentially per attempt (capped at maxRetryBackoff) and +// carries a random jitter of up to the base, so concurrent writers de-synchronize +// rather than colliding again in lockstep. See backoffForAttempt. +const retryBaseBackoff = 250 * time.Millisecond -// retryBackoff is the base delay between optimistic-lock retries. Attempt N -// waits N*retryBackoff so concurrent writers stagger rather than re-collide. -const retryBackoff = 500 * time.Millisecond +// maxRetryBackoff caps the exponential growth of the per-attempt backoff so a +// late retry never sleeps for an unbounded stretch. +const maxRetryBackoff = 8 * time.Second // defaultBotName and defaultBotEmail are the identity stamped on a state commit // when the manifest git config supplies no override. They match the identity the @@ -184,6 +196,11 @@ func CommitWithRetry(opts Options) error { var lastErr error for attempt := 1; attempt <= maxAttempts; attempt++ { + // A stable, greppable marker per attempt lets a live concurrency proof + // confirm every lane converged: the same token the git push-retry path + // emits, so one grep covers all write paths. + log.Info("cascade-state-write: attempt=%d/%d", attempt, maxAttempts) + current, sha, err := opts.Client.GetContent(opts.Repo, opts.Path, opts.Ref) if err != nil { return fmt.Errorf("reading current manifest for state write: %w", err) @@ -196,6 +213,7 @@ func CommitWithRetry(opts Options) error { err = opts.Client.PutContent(opts.Repo, opts.Path, opts.Ref, sha, opts.Message, next, author) if err == nil { + log.Info("cascade-state-write: ok attempt=%d", attempt) return nil } if !IsConflict(err) { @@ -207,9 +225,30 @@ func CommitWithRetry(opts Options) error { // merges rather than one being dropped. lastErr = err if attempt < maxAttempts { - sleep(time.Duration(attempt) * retryBackoff) + sleep(backoffForAttempt(retryBaseBackoff, attempt-1)) } } + log.Info("cascade-state-write: exhausted attempts=%d", maxAttempts) return fmt.Errorf("state write via API still conflicting after %d attempts: %w", maxAttempts, lastErr) } + +// backoffForAttempt returns the delay before the retry following a zero-based +// attempt index: an exponential growth of base (base * 2^attempt) capped at +// maxRetryBackoff, plus a random jitter of up to base so concurrent writers +// racing the same trunk de-synchronize instead of colliding again in lockstep. +// It matches the git push-retry backoff shape so both live write paths behave +// identically under a wave. +func backoffForAttempt(base time.Duration, attempt int) time.Duration { + if base <= 0 { + base = retryBaseBackoff + } + d := base + for i := 0; i < attempt && d < maxRetryBackoff; i++ { + d *= 2 + } + if d > maxRetryBackoff { + d = maxRetryBackoff + } + return d + time.Duration(rand.Int64N(int64(base)+1)) +} diff --git a/internal/statewrite/apiwrite_test.go b/internal/statewrite/apiwrite_test.go index 0f1a3f47..140fc7bd 100644 --- a/internal/statewrite/apiwrite_test.go +++ b/internal/statewrite/apiwrite_test.go @@ -1,12 +1,15 @@ package statewrite import ( + "bytes" "errors" "fmt" + "os" "strings" "testing" "time" + "github.com/stablekernel/cascade/internal/log" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -153,6 +156,98 @@ func TestCommitWithRetry_ErrorsAfterBoundedAttempts(t *testing.T) { assert.Equal(t, maxAttempts-1, slept, "writer must back off between attempts but not after the last") } +func TestCommitWithRetry_RetryCeilingIsTen(t *testing.T) { + // A realistic monorepo wave races every component to write its own leaf into + // one manifest on the trunk branch, so the read-modify-write must tolerate far + // more than the handful of parallel envs the original bound assumed. Pin the + // concrete ceiling so a silent regression back to a low bound is caught. + require.Equal(t, 10, maxAttempts, "state-write retry ceiling must be raised to ten") + + errs := make([]error, maxAttempts) + for i := range errs { + errs[i] = rawConflict() + } + fake := &fakeContents{content: "ci.state.test: A\n", sha: "sha-0", putErrs: errs} + + var slept int + err := CommitWithRetry(Options{ + Client: fake, + Repo: "owner/name", + Path: ".github/manifest.yaml", + Ref: "main", + Message: "chore: record state", + Mutate: appendLine("ci.state.staging: B"), + Sleep: noSleep(&slept), + }) + + require.Error(t, err) + assert.Equal(t, 10, fake.puts, "writer must make ten attempts before exhausting") + assert.Equal(t, 9, slept, "writer must back off between attempts but not after the last") +} + +func TestCommitWithRetry_EmitsConvergenceMarker(t *testing.T) { + // The retry loop must emit a stable, greppable marker per attempt plus an "ok" + // marker on success, so a live concurrency proof can grep every lane and assert + // convergence with zero exhaustion. Capture the log output to observe them. + var buf bytes.Buffer + log.SetOutput(&buf) + log.SetColors(false) + t.Cleanup(func() { log.SetOutput(os.Stderr); log.SetColors(true) }) + + fake := &fakeContents{ + content: "ci.state.test: A\n", + sha: "sha-0", + putErrs: []error{rawConflict()}, // first PUT 409s, second succeeds + } + + var slept int + err := CommitWithRetry(Options{ + Client: fake, + Repo: "owner/name", + Path: ".github/manifest.yaml", + Ref: "main", + Message: "chore: record state on staging", + Mutate: appendLine("ci.state.staging: B"), + Sleep: noSleep(&slept), + }) + + require.NoError(t, err) + out := buf.String() + assert.Contains(t, out, "cascade-state-write: attempt=1/10", "first attempt marker must be emitted") + assert.Contains(t, out, "cascade-state-write: attempt=2/10", "the retry attempt marker must be emitted") + assert.Contains(t, out, "cascade-state-write: ok attempt=2", "a successful write must emit the ok marker with its attempt count") +} + +func TestCommitWithRetry_EmitsExhaustionMarker(t *testing.T) { + // When every attempt conflicts the loop must emit a stable exhaustion marker so + // a live proof asserts convergence by the absence of this token across lanes. + var buf bytes.Buffer + log.SetOutput(&buf) + log.SetColors(false) + t.Cleanup(func() { log.SetOutput(os.Stderr); log.SetColors(true) }) + + errs := make([]error, maxAttempts) + for i := range errs { + errs[i] = rawConflict() + } + fake := &fakeContents{content: "ci.state.test: A\n", sha: "sha-0", putErrs: errs} + + var slept int + err := CommitWithRetry(Options{ + Client: fake, + Repo: "owner/name", + Path: ".github/manifest.yaml", + Ref: "main", + Message: "chore: record state", + Mutate: appendLine("ci.state.staging: B"), + Sleep: noSleep(&slept), + }) + + require.Error(t, err) + assert.Contains(t, buf.String(), "cascade-state-write: exhausted attempts=10", + "an exhausted retry loop must emit the exhaustion marker") +} + func TestCommitWithRetry_NonConflictErrorIsNotRetried(t *testing.T) { // A non-409 error (e.g. auth) must surface immediately without retrying. fake := &fakeContents{