From 9f8039f6e77189eda53e4041b3480a879f6e7c66 Mon Sep 17 00:00:00 2001 From: Joshua Temple Date: Thu, 9 Jul 2026 14:52:53 -0400 Subject: [PATCH] fix(statewrite): retry an empty or errored manifest re-fetch instead of parsing it On a rejected optimistic write, CommitWithRetry re-fetches the current manifest and re-applies. The Contents API client swallows a transient GET failure and returns empty content, and that empty content was passed straight to the mutation, which parsed it and hard-failed the whole write with 'manifest file missing required ci key at top level' even though the committed manifest was intact. Treat an errored or empty re-fetch as a retryable condition, like a 409: back off and retry within the existing bounded loop rather than parsing it, and on exhaustion return a distinct non-parse error rather than a spurious missing-key error. The happy path, the conflict-merge behavior, and single-component behavior are unchanged. The git-based re-apply path already fails closed on a bad fetch and reads the manifest only after a successful reset, so it needs no change. Signed-off-by: Joshua Temple --- internal/statewrite/apiwrite.go | 33 +++++- internal/statewrite/apiwrite_test.go | 149 +++++++++++++++++++++++++++ 2 files changed, 180 insertions(+), 2 deletions(-) diff --git a/internal/statewrite/apiwrite.go b/internal/statewrite/apiwrite.go index ff276615..4b772b12 100644 --- a/internal/statewrite/apiwrite.go +++ b/internal/statewrite/apiwrite.go @@ -203,7 +203,31 @@ func CommitWithRetry(opts Options) error { 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) + // A re-fetch failure (a transient non-2xx or transport blip from the + // Contents-API GET) is retryable exactly like a 409: retry within the + // bounded loop rather than aborting the whole write on a blip. Crucially, + // a failed fetch must never be parsed as a manifest. + lastErr = fmt.Errorf("reading current manifest for state write: %w", err) + log.Info("cascade-state-write: refetch-error attempt=%d/%d", attempt, maxAttempts) + if attempt < maxAttempts { + sleep(backoffForAttempt(retryBaseBackoff, attempt-1)) + } + continue + } + if len(strings.TrimSpace(string(current))) == 0 { + // An empty body from a successful-looking GET is a transient read, not a + // real manifest: the state-write path only ever targets an already + // committed manifest, and Mutate derives its bytes by parsing the current + // manifest, so it cannot produce anything valid from nothing. Parsing the + // empty content would surface a spurious "missing 'ci' key" and, worse, + // risk PUTting bytes derived from garbage. Treat it as a retryable + // re-fetch, never as content. + lastErr = fmt.Errorf("re-fetched current manifest for state write was empty") + log.Info("cascade-state-write: refetch-empty attempt=%d/%d", attempt, maxAttempts) + if attempt < maxAttempts { + sleep(backoffForAttempt(retryBaseBackoff, attempt-1)) + } + continue } next, err := opts.Mutate(current) @@ -230,7 +254,12 @@ func CommitWithRetry(opts Options) error { } log.Info("cascade-state-write: exhausted attempts=%d", maxAttempts) - return fmt.Errorf("state write via API still conflicting after %d attempts: %w", maxAttempts, lastErr) + if IsConflict(lastErr) { + return fmt.Errorf("state write via API still conflicting after %d attempts: %w", maxAttempts, lastErr) + } + // Exhaustion driven by a persistently empty or errored re-fetch: surface a + // distinct, non-corrupting error rather than a spurious parse failure. + return fmt.Errorf("state write via API could not obtain a valid current manifest after %d attempts: %w", maxAttempts, lastErr) } // backoffForAttempt returns the delay before the retry following a zero-based diff --git a/internal/statewrite/apiwrite_test.go b/internal/statewrite/apiwrite_test.go index 140fc7bd..b674a941 100644 --- a/internal/statewrite/apiwrite_test.go +++ b/internal/statewrite/apiwrite_test.go @@ -26,6 +26,17 @@ type fakeContents struct { // past the slice default to applying successfully. putErrs []error + // getErrs is consumed one entry per GetContent call: a non-nil entry forces + // that GET to return an error (a transient re-fetch failure). Entries past the + // slice default to a successful read of the stored content. + getErrs []error + // getContents is consumed one entry per GetContent call: a non-nil entry + // overrides the bytes that GET returns (an empty string models a transient + // empty body). A nil entry, or an index past the slice, reads the stored + // content. It is a pointer slice so an empty override is distinguishable from + // "use the stored content". + getContents []*string + puts int // number of PutContent calls gets int // number of GetContent calls putSeen []string // content bytes presented to each PutContent @@ -34,10 +45,21 @@ type fakeContents struct { } func (f *fakeContents) GetContent(_, _, _ string) ([]byte, string, error) { + i := f.gets f.gets++ + if i < len(f.getErrs) && f.getErrs[i] != nil { + return nil, "", f.getErrs[i] + } + if i < len(f.getContents) && f.getContents[i] != nil { + return []byte(*f.getContents[i]), f.sha, nil + } return []byte(f.content), f.sha, nil } +// strptr is a tiny helper so a test can script an exact GET body (including the +// empty string that models a transient empty re-fetch). +func strptr(s string) *string { return &s } + func (f *fakeContents) PutContent(_, _, _, sha, _ string, content []byte, author Identity) error { f.puts++ f.putSeen = append(f.putSeen, string(content)) @@ -400,6 +422,133 @@ func TestCommitWithRetry_RetriesOnBranchRefCAS409(t *testing.T) { assert.Contains(t, fake.content, "ci.state.staging: B", "this caller's state must be written") } +// parseGuardMutate mimics the real state mutations (WriteScopedState / +// serializeState), which parse the current bytes and reject an empty manifest +// with the config parser's "missing required 'ci' key" error. It is the exact +// failure that a spurious empty re-fetch surfaced on real GitHub, so a test that +// injects an empty re-fetch and asserts this error never escapes proves the +// guard. +func parseGuardMutate(line string) Mutate { + return func(current []byte) ([]byte, error) { + if len(bytes.TrimSpace(current)) == 0 { + return nil, fmt.Errorf("parsing current manifest: manifest file missing required 'ci' key at top level") + } + return appendLine(line)(current) + } +} + +func TestCommitWithRetry_RetriesOnEmptyRefetchThenSucceeds(t *testing.T) { + // The first GET returns an empty body (a transient Contents-API read that the + // gh client turns into empty content with a nil error), the second returns the + // real manifest. An empty re-fetch must NOT be parsed as a manifest: doing so + // emits a spurious "missing 'ci' key". The writer must retry the fetch and + // ultimately succeed against the real bytes. + fake := &fakeContents{ + content: "ci.state.test: A\n", + sha: "sha-0", + getContents: []*string{strptr("")}, // first GET is empty, then the stored content + } + + var slept int + err := CommitWithRetry(Options{ + Client: fake, + Repo: "owner/name", + Path: ".github/manifest.yaml", + Ref: "main", + Message: "chore: record state on staging", + Mutate: parseGuardMutate("ci.state.staging: B"), + Sleep: noSleep(&slept), + }) + + require.NoError(t, err) + assert.GreaterOrEqual(t, fake.gets, 2, "an empty re-fetch must be retried, not parsed") + assert.Equal(t, 1, fake.puts, "exactly one PUT lands once a valid manifest is fetched") + assert.Contains(t, fake.content, "ci.state.test: A", "the real manifest must survive") + assert.Contains(t, fake.content, "ci.state.staging: B", "this caller's state must be written") +} + +func TestCommitWithRetry_RetriesOnErroredRefetchThenSucceeds(t *testing.T) { + // The first GET fails with a transient error; the second succeeds. A transient + // re-fetch error is retryable exactly like a 409: the writer must retry within + // the bounded loop rather than aborting the whole write on a blip. + fake := &fakeContents{ + content: "ci.state.test: A\n", + sha: "sha-0", + getErrs: []error{errors.New("HTTP 502: Bad Gateway")}, // first GET errors, then 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: parseGuardMutate("ci.state.staging: B"), + Sleep: noSleep(&slept), + }) + + require.NoError(t, err) + assert.GreaterOrEqual(t, fake.gets, 2, "a transient GET error must be retried") + assert.Equal(t, 1, fake.puts, "exactly one PUT lands once the GET recovers") + assert.Contains(t, fake.content, "ci.state.staging: B", "this caller's state must be written") +} + +func TestCommitWithRetry_EmptyRefetchExhaustsWithNonParseError(t *testing.T) { + // A re-fetch that stays empty for every attempt must exhaust the bound with a + // clear, non-corrupting error: it must NEVER surface the spurious "missing 'ci' + // key" parse error, and must NEVER PUT garbage derived from nothing. + empties := make([]*string, maxAttempts) + for i := range empties { + empties[i] = strptr("") + } + fake := &fakeContents{content: "ci.state.test: A\n", sha: "sha-0", getContents: empties} + + var slept int + err := CommitWithRetry(Options{ + Client: fake, + Repo: "owner/name", + Path: ".github/manifest.yaml", + Ref: "main", + Message: "chore: record state", + Mutate: parseGuardMutate("ci.state.staging: B"), + Sleep: noSleep(&slept), + }) + + require.Error(t, err) + assert.NotContains(t, err.Error(), "missing required 'ci' key", + "an empty re-fetch must never surface the spurious parse error") + assert.Equal(t, 0, fake.puts, "the writer must never PUT bytes derived from an empty re-fetch") +} + +func TestCommitWithRetry_InvalidNonEmptyManifestStillErrors(t *testing.T) { + // Guard against masking real invalidity: a genuinely invalid but NON-empty + // committed manifest must still error immediately (as today), not be retried + // away. The empty-guard must only catch an empty/absent re-fetch. + fake := &fakeContents{content: "this is not a manifest\n", sha: "sha-0"} + + var slept int + err := CommitWithRetry(Options{ + Client: fake, + Repo: "owner/name", + Path: ".github/manifest.yaml", + Ref: "main", + Message: "chore: record state", + Mutate: func(current []byte) ([]byte, error) { + // A non-empty but invalid manifest: the parser rejects it. This models + // WriteScopedState refusing a manifest whose ci key is absent. + return nil, fmt.Errorf("parsing current manifest: manifest file missing required 'ci' key at top level") + }, + Sleep: noSleep(&slept), + }) + + require.Error(t, err) + assert.Contains(t, err.Error(), "missing required 'ci' key", + "a real invalid manifest must still surface the parse error, not be masked") + assert.Equal(t, 1, fake.gets, "a Mutate error on valid-length bytes must not be retried") + assert.Equal(t, 0, fake.puts, "no PUT on an invalid manifest") +} + func TestIsConflict(t *testing.T) { tests := []struct { name string