From abe156bee7a866d7018467b4d8005b9ef32c3a9a Mon Sep 17 00:00:00 2001 From: Joshua Temple Date: Thu, 9 Jul 2026 02:43:20 -0400 Subject: [PATCH] fix(statewrite): raise the state-write retry ceiling with jittered backoff and convergence markers Bring the Contents-API write path and the emitted shell write loop to parity with the git push-retry hardened earlier. CommitWithRetry goes from five attempts to ten with exponential jittered backoff (base 250ms, doubling, capped at eight seconds) and emits a per-attempt convergence marker plus success and exhaustion lines. The emitted shell state-write loop raises its bound to ten, replaces the fixed random sleep with the same exponential jittered backoff, and echoes the same marker. All three write paths now log one greppable marker so a concurrency proof can assert on convergence and non-exhaustion. State semantics are unchanged; the emitted-output goldens and cascade's own regenerated workflow move only for the retry, backoff, and marker lines. The backoff helper is duplicated in the low-level statewrite package rather than importing the git package to avoid a dependency cycle. Signed-off-by: Joshua Temple --- .github/workflows/orchestrate.yaml | 22 +++-- e2e/scenarios/08-state-push-retry.yaml | 5 +- e2e/scenarios/09-single-env-repo.yaml | 3 +- internal/generate/state_write.go | 32 +++++-- internal/generate/state_write_test.go | 39 ++++++++ ...github__workflows__orchestrate.yaml.golden | 22 +++-- internal/statewrite/apiwrite.go | 55 +++++++++-- internal/statewrite/apiwrite_test.go | 95 +++++++++++++++++++ 8 files changed, 245 insertions(+), 28 deletions(-) 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{