fix(repo): a status write that conflicts after a successful push no longer wedges the Repo (#26)

internal/controllers/repo/repo.go

@@ -29,7 +29,9 @@ import (
 	plumbingevent "github.com/krateo-platformops/plumbing/kubeutil/event"
 	"github.com/krateo-platformops/plumbing/kubeutil/eventrecorder"
 	"github.com/krateo-platformops/plumbing/ptr"
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	record "k8s.io/client-go/tools/events"
+	"k8s.io/client-go/util/retry"
 )
 
 var (
@@ -146,6 +148,11 @@ func (e *external) Observe(ctx context.Context, mg resource.Managed) (reconciler
 
 	if cr.Status.TargetCommitId != "" {
 		meta.SetExternalName(cr, cr.Status.TargetCommitId)
+	} else if name := meta.GetExternalName(cr); name != "" {
+		// The external name is the pushed commit (Create sets both). A status write lost after the
+		// push leaves only the name — the runtime persists it as a critical annotation — so read the
+		// target commit from it rather than checking the history for "" and calling the push lost.
+		cr.Status.TargetCommitId = name
 	}
 
 	if meta.GetExternalName(cr) == "" {
@@ -453,8 +460,7 @@ func (e *external) SyncRepos(ctx context.Context, cr *repov1alpha1.Repo, commitM
 		cr.Status.TargetBranch = toRepo.CurrentBranch()
 		cr.Status.OriginBranch = fromRepo.CurrentBranch()
 
-		err = e.kube.Status().Update(ctx, cr)
-		if err != nil {
+		if err := e.updateStatusWithRetry(ctx, cr); err != nil {
 			return e.failSync(ctx, cr, fmt.Errorf("unable to update status: %w", err))
 		}
 		return nil
@@ -478,13 +484,54 @@ func (e *external) SyncRepos(ctx context.Context, cr *repov1alpha1.Repo, commitM
 	cr.Status.TargetCommitId = toRepoCommitId
 	cr.Status.TargetBranch = toRepo.CurrentBranch()
 	cr.Status.OriginBranch = fromRepo.CurrentBranch()
-	err = e.kube.Status().Update(ctx, cr)
-	if err != nil {
+	if err := e.updateStatusWithRetry(ctx, cr); err != nil {
 		return e.failSync(ctx, cr, fmt.Errorf("unable to update status: %w", err))
 	}
 	return nil
 }
 
+// updateStatusWithRetry persists the sync outcome, re-reading the object when the write loses an
+// optimistic-concurrency race — the LocalResource controller's fix (#16), which this controller
+// never got (#26).
+//
+// This write happens AFTER the push. Returning its conflict made Create fail with a 409, and
+// provider-runtime answers a conflict from Create by requeueing WITHOUT recording an outcome
+// (reconciler.go:997-1008): `krateo.io/external-create-pending` stays, neither -succeeded nor
+// -failed is written, and every later reconcile refuses with "cannot determine creation result".
+// The push had landed; only its record was lost — seen on publish-e2e-bp-0928-source, the first
+// publish of a blueprint, 14ms after a successful push.
+//
+// A status-subresource write returns the SERVER's object, whose annotations replace the copy's — so
+// the external name set just before is gone from `cr` after any successful write. It is set again
+// afterwards: the runtime persists it as the create's critical annotation, the one record of the
+// push that does not depend on this status write having landed.
+func (e *external) updateStatusWithRetry(ctx context.Context, cr *repov1alpha1.Repo) error {
+	pushed := meta.GetExternalName(cr)
+	defer func() {
+		if pushed != "" {
+			meta.SetExternalName(cr, pushed)
+		}
+	}()
+	return retry.RetryOnConflict(retry.DefaultRetry, func() error {
+		err := e.kube.Status().Update(ctx, cr)
+		if err == nil || !apierrors.IsConflict(err) {
+			return err
+		}
+		// Refresh metadata (resourceVersion) from the server but KEEP the status this sync produced
+		// and the external name just set, then let RetryOnConflict try the write again.
+		latest := &repov1alpha1.Repo{}
+		if getErr := e.kube.Get(ctx, client.ObjectKeyFromObject(cr), latest); getErr != nil {
+			return getErr
+		}
+		status := cr.Status
+		externalName := meta.GetExternalName(cr)
+		latest.DeepCopyInto(cr)
+		cr.Status = status
+		meta.SetExternalName(cr, externalName)
+		return err
+	})
+}
+
 // templatingOptions decides which templating pass — if any — a Repo gets, in one place and with
 // the same vocabulary the localresource controller uses.
 //

internal/controllers/repo/status_write_test.go

@@ -0,0 +1,76 @@
+package repo
+
+import (
+	"context"
+	"os"
+	"regexp"
+	"strings"
+	"testing"
+
+	"github.com/krateo-platformops/provider-runtime/pkg/logging"
+	"github.com/krateo-platformops/provider-runtime/pkg/meta"
+	"github.com/stretchr/testify/require"
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/runtime"
+	"sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/controller-runtime/pkg/client/fake"
+
+	repov1alpha1 "github.com/krateoplatformops/git-provider/apis/repo/v1alpha1"
+)
+
+// A status write that loses one optimistic-concurrency race after a successful push must still
+// land, with the status the sync produced and the external name it set (#26). Before the fix the
+// 409 escaped Create, provider-runtime requeued without recording an outcome, and the Repo wedged
+// on external-create-pending with the push already on the remote.
+func TestStatusWriteSurvivesAConflictAfterThePush(t *testing.T) {
+	scheme := runtime.NewScheme()
+	require.NoError(t, repov1alpha1.SchemeBuilder.AddToScheme(scheme))
+
+	stored := &repov1alpha1.Repo{ObjectMeta: metav1.ObjectMeta{Name: "publish-x-source", Namespace: "krateo-system"}}
+	kube := fake.NewClientBuilder().WithScheme(scheme).WithObjects(stored).WithStatusSubresource(stored).Build()
+	e := &external{kube: kube, log: logging.NewNopLogger()}
+	ctx := context.Background()
+
+	// The reconciler's copy, as Create holds it after the push: status and external name set.
+	cr := &repov1alpha1.Repo{}
+	require.NoError(t, kube.Get(ctx, client.ObjectKeyFromObject(stored), cr))
+	meta.SetExternalName(cr, "29dd9b15c0ab")
+	cr.Status.TargetCommitId = "29dd9b15c0ab"
+	cr.Status.TargetBranch = "builder/x"
+
+	// Someone else writes the object in the meantime, so the reconciler's resourceVersion is stale:
+	// a write of `cr` as it stands is a REAL 409, on every attempt, until the helper re-reads.
+	concurrent := &repov1alpha1.Repo{}
+	require.NoError(t, kube.Get(ctx, client.ObjectKeyFromObject(stored), concurrent))
+	concurrent.SetLabels(map[string]string{"concurrent": "writer"})
+	require.NoError(t, kube.Update(ctx, concurrent))
+	stale := cr.DeepCopy()
+	err := kube.Status().Update(ctx, stale)
+	require.True(t, apierrors.IsConflict(err), "the setup must produce a real conflict, or this test proves nothing: %v", err)
+
+	require.NoError(t, e.updateStatusWithRetry(ctx, cr))
+
+	got := &repov1alpha1.Repo{}
+	require.NoError(t, kube.Get(ctx, client.ObjectKeyFromObject(stored), got))
+	require.Equal(t, "29dd9b15c0ab", got.Status.TargetCommitId)
+	require.Equal(t, "builder/x", got.Status.TargetBranch)
+	require.Equal(t, "writer", got.GetLabels()["concurrent"], "the concurrent write must survive — the helper re-reads, it does not overwrite")
+	require.Equal(t, "29dd9b15c0ab", meta.GetExternalName(cr), "the external name must survive — the runtime persists it as the create's record")
+}
+
+// Every status write in the sync path goes through updateStatusWithRetry — the LocalResource guard
+// (#17 fixed one write and missed its twin in the already-up-to-date branch), applied here.
+func TestNoBareStatusUpdateOutsideTheRetryHelper(t *testing.T) {
+	src, err := os.ReadFile("repo.go")
+	require.NoError(t, err)
+	helper := regexp.MustCompile(`(?s)func \(e \*external\) updateStatusWithRetry\(.*?\n}`)
+	outside := helper.ReplaceAllString(string(src), "")
+	var offenders []string
+	for _, line := range strings.Split(outside, "\n") {
+		if strings.Contains(line, "Status().Update(") {
+			offenders = append(offenders, strings.TrimSpace(line))
+		}
+	}
+	require.Empty(t, offenders, "bare Status().Update outside updateStatusWithRetry — a lost conflict here strands the Repo on external-create-pending")
+}