From d06720a7ee9c8f413d393c8dce99cc015d82ad4f Mon Sep 17 00:00:00 2001 From: Diego Braga Date: Mon, 28 Sep 2026 09:49:25 +0200 Subject: [PATCH] fix(repo): a status write that conflicts after a successful push no longer wedges the Repo (#26) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A Repo whose push had LANDED wedged permanently on krateo.io/external-create-pending. The status write after the push lost an optimistic-concurrency race, Create returned that 409, and provider-runtime answers a conflict from Create by requeueing without recording an outcome (reconciler.go:997-1008) — pending stays with neither -succeeded nor -failed, and every later reconcile refuses with "cannot determine creation result". Seen on krateo-057 (0.15.4) on the first publish of a blueprint, 14ms after a successful push. - Both status writes in SyncRepos (the committed path and the already-up-to-date path) go through updateStatusWithRetry: RetryOnConflict that re-reads the object and keeps the status the sync produced and the external name it set — the LocalResource fix from #16, which Repo never got. - A status-subresource write returns the server's object, whose annotations replace the copy's, so the helper sets the external name again after the write: the runtime persists it as the create's critical annotation, the one record of the push that does not depend on the status write. - Observe reads the target commit from the external name when status.targetCommitId is empty, instead of checking the history for "" — so a Repo whose status write was lost converges. Tests: a REAL conflict (a concurrent write makes the reconciler's resourceVersion stale) — the status lands, the concurrent write survives, the external name survives; with the refresh removed the test fails. A source guard pins that no bare Status().Update exists outside the helper (the LocalResource guard, #17). go test -race ./... passes. Not fixed here: Repos wedged before this release (#22) — resolving an existing pending annotation needs the runtime-side confirm-the-negative. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_016V8EVEvyty3XvXMHUWJvPg --- internal/controllers/repo/repo.go | 55 +++++++++++++- .../controllers/repo/status_write_test.go | 76 +++++++++++++++++++ 2 files changed, 127 insertions(+), 4 deletions(-) create mode 100644 internal/controllers/repo/status_write_test.go diff --git a/internal/controllers/repo/repo.go b/internal/controllers/repo/repo.go index 83c34a0..a07e458 100644 --- a/internal/controllers/repo/repo.go +++ b/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. // diff --git a/internal/controllers/repo/status_write_test.go b/internal/controllers/repo/status_write_test.go new file mode 100644 index 0000000..d10708c --- /dev/null +++ b/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") +}