diff --git a/internal/statewrite/apiwrite.go b/internal/statewrite/apiwrite.go index af6e75bb..c5131377 100644 --- a/internal/statewrite/apiwrite.go +++ b/internal/statewrite/apiwrite.go @@ -92,9 +92,12 @@ func (e *ConflictError) Error() string { func (e *ConflictError) Unwrap() error { return e.Err } // IsConflict reports whether err is (or wraps) a Contents API 409 conflict. It -// recognizes both the typed ConflictError and the raw gh-CLI 409 body, which -// carries "does not match" and a "409" status, so a client that forwards the -// gh error verbatim still triggers a retry. +// recognizes the typed ConflictError and both raw gh-CLI 409 bodies, so a client +// that forwards the gh error verbatim still triggers a retry: the blob If-Match +// mismatch carries "does not match", and the branch-ref compare-and-swap failure +// two racing finalizes produce reads "... is at X but expected Y ..." with no +// "does not match". Either lock marker alongside a "409" or "Conflict" status is +// a conflict; this mirrors classifyPutError exactly. func IsConflict(err error) bool { if err == nil { return false @@ -104,7 +107,8 @@ func IsConflict(err error) bool { return true } msg := err.Error() - return strings.Contains(msg, "does not match") && strings.Contains(msg, "409") + return (strings.Contains(msg, "does not match") || strings.Contains(msg, "is at")) && + (strings.Contains(msg, "409") || strings.Contains(msg, "Conflict")) } // asConflict is a tiny errors.As wrapper kept local so the package has no hard diff --git a/internal/statewrite/apiwrite_test.go b/internal/statewrite/apiwrite_test.go index 06216e75..0f1a3f47 100644 --- a/internal/statewrite/apiwrite_test.go +++ b/internal/statewrite/apiwrite_test.go @@ -75,6 +75,15 @@ func rawConflict() error { return &ConflictError{Err: errors.New(`{"message":".github/manifest.yaml does not match abc123","status":"409"}`)} } +// branchRefCAS409Body mirrors the second 409 shape GitHub returns: a branch-ref +// compare-and-swap failure whose body reads "... is at X but expected Y ..." +// and carries NO "does not match" substring. Two component finalizes racing to +// update one trunk branch produce this shape, so the classifier must recognize +// it as a conflict from the "is at" marker alongside the 409/Conflict status. +func branchRefCAS409Body() string { + return `PUT https://api.github.com/repos/x/y/contents/state.json: 409 Conflict [] {"message":"Update is at abc123 but expected def456","documentation_url":"https://docs.github.com"}` +} + // noSleep is an injected sleep that records that it was called but lets no real // time pass, keeping the test fast and deterministic. func noSleep(calls *int) func(time.Duration) { @@ -248,6 +257,54 @@ func TestCommitWithRetry_IdentityPersistsAcrossRetries(t *testing.T) { } } +func TestIsConflict_BranchRefCAS409(t *testing.T) { + body := branchRefCAS409Body() + // Typed path: a client that wraps the branch-ref 409 in ConflictError. + assert.True(t, IsConflict(&ConflictError{Err: errors.New(body)}), + "a typed conflict wrapping the branch-ref 409 must be recognized") + // String path: a client forwarding the raw gh body verbatim, which carries + // no "does not match" substring, must still be recognized as a conflict. + assert.True(t, IsConflict(errors.New(body)), + "the raw branch-ref 409 body must be recognized without a \"does not match\" substring") + // Negative: a genuine non-409 error must not classify as a conflict. + assert.False(t, IsConflict(errors.New("HTTP 500 Internal Server Error")), + "a 500 is not a conflict") + assert.False(t, IsConflict(errors.New("the runner is at the front of the queue")), + "an \"is at\" message without a 409/Conflict marker is not a conflict") +} + +func TestCommitWithRetry_RetriesOnBranchRefCAS409(t *testing.T) { + // The first PUT loses a branch-ref compare-and-swap race: GitHub returns a + // 409 whose body reads "... is at X but expected Y ..." with NO "does not + // match" substring. The forwarded raw error must still be recognized as a + // conflict so the writer re-fetches, re-applies, and succeeds on the retry + // rather than hard-failing after a single attempt. + fake := &fakeContents{ + content: "ci.state.test: A\n", + sha: "sha-0", + putErrs: []error{errors.New(branchRefCAS409Body())}, // raw forwarded error, not typed + } + + 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) + assert.GreaterOrEqual(t, fake.puts, 2, "the branch-ref 409 must drive at least a second PUT") + assert.Equal(t, 2, fake.gets, "writer must re-fetch the manifest after the branch-ref 409") + assert.Equal(t, 1, slept, "writer must back off once between the two attempts") + // Merge semantics: both writers' state survives the re-fetch and re-apply. + assert.Contains(t, fake.content, "ci.state.test: A", "the other writer's state must survive") + assert.Contains(t, fake.content, "ci.state.staging: B", "this caller's state must be written") +} + func TestIsConflict(t *testing.T) { tests := []struct { name string diff --git a/internal/statewrite/ghclient.go b/internal/statewrite/ghclient.go index 9ea32ff0..00a9aeb1 100644 --- a/internal/statewrite/ghclient.go +++ b/internal/statewrite/ghclient.go @@ -68,15 +68,18 @@ func (ghContents) PutContent(repo, path, ref, sha, message string, content []byt } // classifyPutError maps a gh PUT result to a typed error. A nil err is success. -// A 409 optimistic-lock failure (the body carries "does not match" alongside a -// 409, "Conflict", or "is at" marker) becomes a ConflictError so the retry loop -// re-fetches and re-applies; any other failure is wrapped verbatim. +// An optimistic-lock failure becomes a ConflictError so the retry loop re-fetches +// and re-applies; any other failure is wrapped verbatim. GitHub returns two 409 +// shapes for a stale write: a blob If-Match mismatch whose body carries "does not +// match", and a branch-ref compare-and-swap failure whose body reads "... is at X +// but expected Y ..." with no "does not match". Either lock marker alongside a +// 409 or "Conflict" status is recognized so both shapes drive a retry. func classifyPutError(out string, err error) error { if err == nil { return nil } - if strings.Contains(out, "does not match") && - (strings.Contains(out, "409") || strings.Contains(out, "Conflict") || strings.Contains(out, "is at")) { + if (strings.Contains(out, "does not match") || strings.Contains(out, "is at")) && + (strings.Contains(out, "409") || strings.Contains(out, "Conflict")) { return &ConflictError{Err: fmt.Errorf("%s: %w", strings.TrimSpace(out), err)} } return fmt.Errorf("%s: %w", strings.TrimSpace(out), err) diff --git a/internal/statewrite/ghclient_test.go b/internal/statewrite/ghclient_test.go index 5ca5e4d3..03bf561e 100644 --- a/internal/statewrite/ghclient_test.go +++ b/internal/statewrite/ghclient_test.go @@ -5,6 +5,37 @@ import ( "testing" ) +func TestClassifyPutError_BranchRefCAS409(t *testing.T) { + // A branch-ref compare-and-swap 409 (the shape two racing finalizes produce) + // carries an "is at X but expected Y" marker and NO "does not match" + // substring. It must still classify as a typed ConflictError so the retry + // loop re-fetches and re-applies instead of hard-failing. + got := classifyPutError(branchRefCAS409Body(), errors.New("exit 1")) + if got == nil { + t.Fatal("classifyPutError() = nil, want a *ConflictError for a branch-ref compare-and-swap 409") + } + var ce *ConflictError + if !errors.As(got, &ce) { + t.Fatalf("classifyPutError() = %T, want it to unwrap to *ConflictError", got) + } + if !IsConflict(got) { + t.Error("IsConflict() = false, want true for a branch-ref compare-and-swap 409") + } +} + +func TestClassifyPutError_NonConflictNotClassified(t *testing.T) { + // A genuine non-conflict failure with no 409/Conflict marker must not be + // classified as a conflict, or the retry loop would spin on unrecoverable + // errors. + got := classifyPutError("HTTP 500 Internal Server Error", errors.New("exit 1")) + if got == nil { + t.Fatal("classifyPutError() = nil, want a wrapped non-nil error") + } + if IsConflict(got) { + t.Error("IsConflict() = true, want false for a 500 with no conflict marker") + } +} + func TestClassifyPutError(t *testing.T) { tests := []struct { name string