feat: reconcile on plumbing v1.10.0 + no-strip ApplyNamespace + pending grace

go.mod

@@ -6,7 +6,7 @@ require (
 	github.com/davecgh/go-spew v1.1.2-0.20180830191138-d8f796af33cc
 	github.com/go-logr/logr v1.4.3
 	github.com/gobuffalo/flect v1.0.3
-	github.com/krateoplatformops/plumbing v1.7.15
+	github.com/krateoplatformops/plumbing v1.10.0
 	github.com/krateoplatformops/unstructured-runtime v1.1.0
 	github.com/stretchr/testify v1.11.1
 	go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp v0.61.0
@@ -160,12 +160,13 @@ require (
 // Use the braghettos fork of unstructured-runtime: create-pending Observe-before-refuse
 // recovery (#2), jq-based statusprojection (#3), the shared OTel JSON log handler
 // NewOTelJSONHandler (#4), AND the OTel metrics-resource attrs + trace pipeline / reconcile
-// span (#5) -- unified in tag v1.3.0.
+// span (#5, tag v1.3.0). v1.3.1 drops the plumbing/slogs/pretty test dependency so the module
+// builds against plumbing's current main line.
 // The fork keeps the upstream module path, so pin it via replace.
-replace github.com/krateoplatformops/unstructured-runtime => github.com/braghettos/unstructured-runtime v1.3.0
+replace github.com/krateoplatformops/unstructured-runtime => github.com/braghettos/unstructured-runtime v1.3.1
 
-// Source plumbing from the braghettos fork (v1.7.7): carries the jqutil int64/int32
-// gojq-panic fix, crdgen array-default markers, AND the krateo.io/traceparent child-manifest
-// post-render stamping for cross-composition trace propagation. (v1.7.x maintenance line —
-// the divergent v1.8.x fork line dropped slogs/pretty, which unstructured-runtime tests need.)
-replace github.com/krateoplatformops/plumbing => github.com/braghettos/plumbing v1.7.15
+// Source plumbing from the braghettos fork (v1.10.0, main line): carries the jqutil int64/int32
+// gojq-panic fix, crdgen array-default markers, the krateo.io/traceparent child-manifest
+// post-render stamping for cross-composition trace propagation, AND the fork-free
+// apply-if-changed reconcile with semantic change-detection that this controller drives.
+replace github.com/krateoplatformops/plumbing => github.com/braghettos/plumbing v1.10.0

go.sum

@@ -28,10 +28,10 @@ github.com/beorn7/perks v1.0.1 h1:VlbKKnNfV8bJzeqoa4cOKqO6bYr3WgKZxO8Z16+hsOM=
 github.com/beorn7/perks v1.0.1/go.mod h1:G2ZrVWU2WbWT9wwq4/hrbKbnv/1ERSJQ0ibhJ6rlkpw=
 github.com/blang/semver/v4 v4.0.0 h1:1PFHFE6yCCTv8C1TeyNNarDzntLi7wMI5i/pzqYIsAM=
 github.com/blang/semver/v4 v4.0.0/go.mod h1:IbckMUScFkM3pff0VJDNKRiT6TG/YpiHIM2yvyW5YoQ=
-github.com/braghettos/plumbing v1.7.15 h1:ZR3KN9a+u/hjz3PS8gTocs7027hjDl34dsBF8rxzSUA=
-github.com/braghettos/plumbing v1.7.15/go.mod h1:7iHgxAVJWAZeYTnnTV+JgX1U/Sm4ADsu+hlna0L6vqM=
-github.com/braghettos/unstructured-runtime v1.3.0 h1:t8QnEN3TIdfJVfO9iNP5OSFIbQlwpR/xIzWMfcDvwHc=
-github.com/braghettos/unstructured-runtime v1.3.0/go.mod h1:hkDWf74tgbfYONkwKEjaX24KgQLPe7fUvArz8wqglnA=
+github.com/braghettos/plumbing v1.10.0 h1:TZmyD9OtpI83sxFV/B7jBR3cRxi/nA9YQ53IvLKME50=
+github.com/braghettos/plumbing v1.10.0/go.mod h1:v5uFPU7mbHGg2Nyt1UFG2c+5tFmqxLJeUVRIX4V+MjE=
+github.com/braghettos/unstructured-runtime v1.3.1 h1:ZwuLl1VP7Ysqk/JSrYHIC0GOYVSN7tV0XdiFkZAIa3Q=
+github.com/braghettos/unstructured-runtime v1.3.1/go.mod h1:FhrvKaArNeocZaW0TDQuvR2nmPo+fgbFHFNP2l7H/i4=
 github.com/bshuster-repo/logrus-logstash-hook v1.0.0 h1:e+C0SB5R1pu//O4MQ3f9cFuPGoOVeF2fE4Og9otCc70=
 github.com/bshuster-repo/logrus-logstash-hook v1.0.0/go.mod h1:zsTqEiSzDgAa/8GZR7E1qaXrhYNDKBYy5/dWPTIflbk=
 github.com/cenkalti/backoff/v4 v4.3.0 h1:MyRJ/UdXutAwSAT+s3wNd7MfTIcy71VQueUuFK343L8=
@@ -447,4 +447,4 @@ sigs.k8s.io/randfill v1.0.0/go.mod h1:XeLlZ/jmk4i1HRopwe7/aU3H5n1zNUcX6TM94b3QxO
 sigs.k8s.io/structured-merge-diff/v6 v6.3.0 h1:jTijUJbW353oVOd9oTlifJqOGEkUw2jB/fXCbTiQEco=
 sigs.k8s.io/structured-merge-diff/v6 v6.3.0/go.mod h1:M3W8sfWvn2HhQDIbGWj3S099YozAsymCo/wrT5ohRUE=
 sigs.k8s.io/yaml v1.6.0 h1:G8fkbMSAFqgEFgh4b1wmtzDnioxFCUgTZhlbj5P9QYs=
-sigs.k8s.io/yaml v1.6.0/go.mod h1:796bPqUfzR/0jLAl6XjHl3Ck7MiyVv8dbTdyT3/pMf4=
+sigs.k8s.io/yaml v1.6.0/go.mod h1:796bPqUfzR/0jLAl6XjHl3Ck7MiyVv8dbTdyT3/pMf4=

internal/composition/composition.go

@@ -51,6 +51,13 @@ import (
 var (
 	krateoNamespace = env.String(krateoNamespaceEnvVar, krateoNamespaceDefault)
 	helmMaxHistory  = env.Int(helmMaxHistoryEnvvar, 3)
+	// pendingOperationGrace: a helm release found in a pending-* status is rolled back ONLY if it
+	// has been pending LONGER than this — i.e. genuinely stuck (the controller died mid-operation),
+	// not legitimately in-flight. A large composition's upgrade (hundreds of children + hooks) can
+	// stay pending for tens of seconds; rolling that back mid-flight caused an Upgrade<->Rollback
+	// thrash on the 319-resource portal composition. Default 5m (>= helm's default op timeout), so
+	// only operations that exceeded a real upgrade are treated as stuck. Tunable per deployment.
+	pendingOperationGrace = env.Duration(pendingGraceEnvvar, 5*time.Minute)
 )
 
 const (
@@ -64,6 +71,7 @@ const (
 	// Environment variables
 	helmMaxHistoryEnvvar  = "HELM_MAX_HISTORY"
 	krateoNamespaceEnvVar = "KRATEO_NAMESPACE"
+	pendingGraceEnvvar    = "COMPOSITION_CONTROLLER_PENDING_GRACE"
 
 	// Default namespace for Krateo Installation
 	krateoNamespaceDefault = "krateo-system"
@@ -226,8 +234,26 @@ func (h *handler) Observe(ctx context.Context, mg *unstructured.Unstructured) (c
 	}
 
 	if rel.Status == helmconfig.StatusPendingInstall || rel.Status == helmconfig.StatusPendingUpgrade || rel.Status == helmconfig.StatusPendingRollback {
-		log.Debug("Composition stuck install or upgrade in progress. Rolling back to previous release before re-attempting.")
-		// Rollback to previous release
+		// A pending status means a helm operation is in flight OR its process died mid-flight — helm
+		// labels both the same and never re-labels a crash. Status alone can't tell them apart, so we
+		// use how long it has been pending (rel.Updated = helm Info.LastDeployed). A LARGE composition's
+		// upgrade (hundreds of children + hooks) legitimately stays pending for tens of seconds; the old
+		// unconditional rollback reverted such in-flight operations mid-flight (even rolling back a
+		// pending-rollback), which — with the reconcile re-enqueue cadence — produced an Upgrade<->Rollback
+		// thrash on the 319-resource portal composition.
+		pendingFor := time.Since(rel.Updated)
+		if pendingFor < pendingOperationGrace {
+			// Recent => legitimately in flight. Do NOT roll it back and do NOT start a concurrent
+			// operation; report up-to-date so the in-flight op settles and the next reconcile proceeds.
+			log.Debug("Release operation in progress; waiting for it to settle.",
+				"status", string(rel.Status), "pendingFor", pendingFor.String())
+			return controller.ExternalObservation{ResourceExists: true, ResourceUpToDate: true}, nil
+		}
+		// Pending longer than any real operation => genuinely stuck (e.g. controller died mid-op). Roll
+		// back once to clear the stale pending lock so a fresh upgrade can proceed (helm refuses to
+		// upgrade a release that is stuck pending).
+		log.Debug("Composition stuck in a pending helm operation past the grace period; rolling back to clear it.",
+			"status", string(rel.Status), "pendingFor", pendingFor.String(), "grace", pendingOperationGrace.String())
 		rel, err = hc.Rollback(ctx, releaseName, &helmconfig.RollbackConfig{
 			MaxHistory:     helmMaxHistory,
 			ReleaseVersion: rel.Revision,
@@ -339,7 +365,7 @@ func (h *handler) Observe(ctx context.Context, mg *unstructured.Unstructured) (c
 			// Without this, one un-adoptable child 500s the entire reconcile ("cannot be imported
 			// into the current release: invalid ownership metadata") and wedges the platform (D1,
 			// 2026-07-08); with it the release takes ownership, self-healing the conflict.
-			TakeOwnership:         true,
+			TakeOwnership: true,
 		},
 		MaxHistory: helmMaxHistory,
 	})
@@ -516,7 +542,7 @@ func (h *handler) Create(ctx context.Context, mg *unstructured.Unstructured) err
 		// non-Helm ownership metadata (out-of-band-created/edited composition instance). Otherwise one
 		// un-adoptable child 500s the entire reconcile and wedges the platform (D1); with it the
 		// release takes ownership and self-heals the conflict.
-		TakeOwnership:         true,
+		TakeOwnership: true,
 	}
 
 	// Check if the release already exists before attempting to install, this can happen if the create event is triggered after a failed install
@@ -938,4 +964,4 @@ func (h *handler) getHelmLogger(verbose bool) func(format string, v ...interface
 		}
 	}
 	return func(format string, v ...interface{}) {}
-}
+}

internal/tools/rbac/installer.go

@@ -72,11 +72,6 @@ func (i *RBACInstaller) ApplyRBAC(rbac *RBAC) error {
 }
 
 func (r *RBACInstaller) ApplyNamespace(ctx context.Context, namespace *corev1.Namespace) (*corev1.Namespace, error) {
-	m, err := runtime.DefaultUnstructuredConverter.ToUnstructured(namespace)
-	if err != nil {
-		return nil, fmt.Errorf("failed to convert Namespace to unstructured: %w", err)
-	}
-	u := &unstructured.Unstructured{Object: m}
 	cli := r.DynamicClient.Resource(
 		schema.GroupVersionResource{
 			Group:    "",
@@ -85,28 +80,72 @@ func (r *RBACInstaller) ApplyNamespace(ctx context.Context, namespace *corev1.Na
 		},
 	)
 
-	if _, err := cli.Get(ctx, namespace.Name, metav1.GetOptions{}); errors.IsNotFound(err) {
-		res, err := cli.Create(ctx, u, metav1.CreateOptions{})
+	existingU, err := cli.Get(ctx, namespace.Name, metav1.GetOptions{})
+	if errors.IsNotFound(err) {
+		m, err := runtime.DefaultUnstructuredConverter.ToUnstructured(namespace)
+		if err != nil {
+			return nil, fmt.Errorf("failed to convert Namespace to unstructured: %w", err)
+		}
+		res, err := cli.Create(ctx, &unstructured.Unstructured{Object: m}, metav1.CreateOptions{})
 		if err != nil {
 			return nil, fmt.Errorf("failed to Create Namespace: %w", err)
 		}
 		namespace = &corev1.Namespace{}
-		err = runtime.DefaultUnstructuredConverter.FromUnstructured(res.Object, namespace)
-		if err != nil {
+		if err := runtime.DefaultUnstructuredConverter.FromUnstructured(res.Object, namespace); err != nil {
 			return nil, fmt.Errorf("failed to convert unstructured to Namespace: %w", err)
 		}
 		return namespace, nil
 	} else if err != nil {
 		return nil, fmt.Errorf("failed to Get Namespace: %w", err)
 	}
 
-	res, err := cli.Update(ctx, u, metav1.UpdateOptions{})
+	// The Namespace already exists. It may be owned by the composition's Helm
+	// release (e.g. the portal's demo-system, declared as a chart child), in
+	// which case its metadata carries foreign labels such as the postrenderer's
+	// krateo.io/composition-* set. A blind full-PUT of our freshly-built object
+	// would replace metadata.labels and strip those, so helm re-adds them on the
+	// next reconcile — an endless label tug-of-war that bumps a helm revision
+	// every cycle. Instead merge our labels/annotations INTO the existing object
+	// (like ApplyRole does for rules) and only Update when something is missing.
+	existing := &corev1.Namespace{}
+	if err := runtime.DefaultUnstructuredConverter.FromUnstructured(existingU.Object, existing); err != nil {
+		return nil, fmt.Errorf("failed to convert unstructured to Namespace: %w", err)
+	}
+
+	modified := false
+	if len(namespace.Labels) > 0 && existing.Labels == nil {
+		existing.Labels = map[string]string{}
+	}
+	for k, v := range namespace.Labels {
+		if existing.Labels[k] != v {
+			existing.Labels[k] = v
+			modified = true
+		}
+	}
+	if len(namespace.Annotations) > 0 && existing.Annotations == nil {
+		existing.Annotations = map[string]string{}
+	}
+	for k, v := range namespace.Annotations {
+		if existing.Annotations[k] != v {
+			existing.Annotations[k] = v
+			modified = true
+		}
+	}
+
+	if !modified {
+		return existing, nil
+	}
+
+	m, err := runtime.DefaultUnstructuredConverter.ToUnstructured(existing)
+	if err != nil {
+		return nil, fmt.Errorf("failed to convert Namespace to unstructured: %w", err)
+	}
+	res, err := cli.Update(ctx, &unstructured.Unstructured{Object: m}, metav1.UpdateOptions{})
 	if err != nil {
 		return nil, fmt.Errorf("failed to Update Namespace: %w", err)
 	}
 	namespace = &corev1.Namespace{}
-	err = runtime.DefaultUnstructuredConverter.FromUnstructured(res.Object, namespace)
-	if err != nil {
+	if err := runtime.DefaultUnstructuredConverter.FromUnstructured(res.Object, namespace); err != nil {
 		return nil, fmt.Errorf("failed to convert unstructured to Namespace: %w", err)
 	}
 	return namespace, nil

internal/tools/rbac/installer_test.go

@@ -1083,6 +1083,60 @@ func TestApplyNamespace_UpdateExisting(t *testing.T) {
 	testenv.Test(t, f)
 }
 
+// TestApplyNamespace_PreservesForeignLabels is the regression guard for the
+// demo-system label-strip churn: when the Namespace already exists and carries
+// labels this installer does not manage (e.g. the postrenderer's
+// krateo.io/composition-* set stamped by Helm), ApplyNamespace must merge its
+// own labels in WITHOUT clobbering the foreign ones. A blind full-PUT of the
+// bare CreateNamespace object used to strip them, and Helm re-added them every
+// reconcile — one helm revision per cycle, forever.
+func TestApplyNamespace_PreservesForeignLabels(t *testing.T) {
+	f := features.New("ApplyNamespace_PreservesForeignLabels").
+		Setup(e2e.Logger("test")).
+		Setup(func(ctx context.Context, t *testing.T, cfg *envconf.Config) context.Context {
+			return ctx
+		}).Assess("Foreign composition labels survive a bare apply", func(ctx context.Context, t *testing.T, cfg *envconf.Config) context.Context {
+		dyn := dynamic.NewForConfigOrDie(cfg.Client().RESTConfig())
+
+		installer := &RBACInstaller{DynamicClient: dyn}
+
+		// Simulate the Helm-owned namespace: created by the chart + postrenderer
+		// with a krateo.io/composition-* label the RBAC installer knows nothing about.
+		helmOwned := &corev1.Namespace{
+			ObjectMeta: metav1.ObjectMeta{
+				Name: "test-namespace-foreign",
+				Labels: map[string]string{
+					"krateo.io/composition-id": "abc123",
+				},
+			},
+		}
+		if _, err := installer.ApplyNamespace(context.Background(), helmOwned); err != nil {
+			t.Fatalf("expected no error seeding helm-owned namespace, got %v", err)
+		}
+
+		// The RBAC installer's own view: only the managed-by label (see CreateNamespace).
+		bare := CreateNamespace("test-namespace-foreign", "some-release", "krateo-system")
+
+		result, err := installer.ApplyNamespace(context.Background(), bare)
+		if err != nil {
+			t.Fatalf("expected no error applying bare namespace, got %v", err)
+		}
+
+		// Our label must be present...
+		if result.Labels["app.kubernetes.io/managed-by"] != "Helm" {
+			t.Errorf("expected managed-by=Helm to be merged in, got %q", result.Labels["app.kubernetes.io/managed-by"])
+		}
+		// ...AND the foreign composition label must NOT have been stripped.
+		if result.Labels["krateo.io/composition-id"] != "abc123" {
+			t.Errorf("foreign label krateo.io/composition-id was stripped: got %q, want abc123", result.Labels["krateo.io/composition-id"])
+		}
+
+		return ctx
+	}).Feature()
+
+	testenv.Test(t, f)
+}
+
 func TestApplyNamespace_InvalidName(t *testing.T) {
 	f := features.New("ApplyNamespace_InvalidName").
 		Setup(e2e.Logger("test")).