fix(statewrite): classify the branch-ref-CAS 409 as a conflict so concurrent writes retry

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

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

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)

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