[eno][tombstone-recovery 6/7] Clean up inventory Secrets on Composition deletion

docs/reconciliation/resource-synthesis-self-heal-and-recovery.md

@@ -6,7 +6,8 @@
 - **Recover:** Read saved inventory and restore missing tombstones into the current synthesis. A tombstone is a manifest with `Deleted: true`; normal resource reconciliation performs the downstream deletion.
 - **Coordinate:** Prevent reconstitution and readiness from advancing before the current synthesis has a terminal recovery decision.
 - **Record:** Save the current Ready synthesis's inventory as one or more Secrets for future recovery.
-- **Not included:** General inventory garbage collection or cleanup on Composition deletion. Recording only rotates older lineage snapshots after writing their replacement.
+- **Clean up:** Delete a Composition's lineage inventory once it starts deleting, unless its Symphony is being deleted.
+- **Not included:** General inventory garbage collection. Recording only rotates older lineage snapshots after writing their replacement.
 - **Not guaranteed:** Recovery of every historical resource when inventory is missing or invalid, or atomic behavior across Composition status, ResourceSlices, and downstream resources.
 - **Related guide:** [Resource Reconciliation](./reconciliation.md) explains ordinary apply, deletion, readiness, and sharding behavior.
 
@@ -65,7 +66,7 @@ Synthesis detects incomplete history or inherits an outstanding requirement
 - **Inventory matches current identities:** Write no additional tombstones and record `FinishedTombstoneRecovery`.
 - **Historical identities are missing:** Persist tombstones for those identities and record `FinishedTombstoneRecovery`.
 - **Operator disabled:** Reconstitution records `TombstoneRecoveryOperatorNotEnabled` for an annotated, synthesized, non-deleting Composition without a terminal decision, then proceeds.
-- **Unannotated or deleting Composition:** Bypass recovery gates and retain normal reconciliation/deletion behavior.
+- **Unannotated or deleting Composition:** Bypass recovery gates and retain normal reconciliation/deletion behavior. A deleting opted-in Composition also has its lineage inventory deleted unless its Symphony is being deleted.
 
 ## Component Responsibilities
 
@@ -82,7 +83,7 @@ Synthesis detects incomplete history or inherits an outstanding requirement
 - **Does:** Treat current manifests, existing tombstones, and Eno Patch targets as present, using group/kind/namespace/name identity.
 - **Does:** Record retriable errors or terminal decisions, publishing overflow references together with completion.
 - **Does:** On a subsequent event, record inventory when the persisted current synthesis is Ready and its recovery decision is finished.
-- **Does not:** Change an existing desired manifest into a tombstone, directly delete downstream resources, or garbage-collect inventory Secrets.
+- **Does not:** Change an existing desired manifest into a tombstone, directly delete downstream resources, or garbage-collect inventory Secrets for Compositions that are already gone.
 - **Entry points:** [`Reconcile` and `recoverMissingTombstones`](../../internal/controllers/tombstonerecovery/controller.go), [`missingTombstones`](../../internal/controllers/tombstonerecovery/inventory.go).
 
 ### Packing Planner and Writer
@@ -101,6 +102,14 @@ Synthesis detects incomplete history or inherits an outstanding requirement
 - **Does not:** Reopen a finished recovery decision, overwrite different contents under the same synthesis UUID, or perform general orphan garbage collection.
 - **Entry points:** [`recordInventory` and `writeInventoryChunk`](../../internal/controllers/tombstonerecovery/recording.go).
 
+### Inventory Cleanup
+
+- **Does:** When an opted-in Composition has a `DeletionTimestamp`, list downstream `kube-system` Secrets by its `eno.azure.io/inventory-lineage` label and delete them all. `NotFound` counts as deleted.
+- **Does:** Start as soon as deletion starts, without waiting for dependents, downstream resource deletion, or `CurrentSynthesis.Reconciled`. Retry list and delete errors while the Composition still exists.
+- **Does not:** Clean up when the Composition has `eno.azure.io/symphony-deleting: "true"`; Symphony deletion orphans downstream resources, so their inventory is kept.
+- **Does not:** Add a finalizer, block Composition deletion, change Composition status, or delete downstream resources.
+- **Entry point:** [`cleanupInventory`](../../internal/controllers/tombstonerecovery/cleanup.go).
+
 ### Reconstitution
 
 - **Does:** Wait for the current synthesis's terminal recovery decision when recovery is enabled and the Composition is opted in and not deleting.
@@ -184,7 +193,7 @@ Synthesis detects incomplete history or inherits an outstanding requirement
 ### Composition Deletion, Opt-Out, or Reconciler Restart
 
 - **Situation:** The Composition begins deleting, loses its opt-in annotation, or the reconciler restarts.
-- **Handling:** Deleting/unannotated Compositions bypass recovery gates. Pre-write checks reject observed deletion or opt-out. A restarted controller reconstructs progress from persisted slices and status.
+- **Handling:** Deleting/unannotated Compositions bypass recovery gates. Pre-write checks reject observed deletion or opt-out. A deleting opted-in Composition has its lineage inventory deleted unless its Symphony is being deleted. A restarted controller reconstructs progress from persisted slices and status.
 - **Boundary:** Removing the annotation does not roll back tombstones already written; recovery-specific cleanup protection ends for unreferenced overflow.
 
 ### Inventory Recording Fails or Is Superseded
@@ -217,6 +226,7 @@ Synthesis detects incomplete history or inherits an outstanding requirement
 - **Concurrent downstream deletion:** An already-running deletion can overlap a newer synthesis that wants the resource. Recovery status preconditions do not make downstream operations transactional.
 - **Progress depends on the environment:** Permanent errors, inhibited resynthesis, or continuous synthesis churn can prevent completion.
 - **Namespace and ownership configuration:** Matching reconciliation/recovery scopes and assigning a single recovery owner per Composition are deployment responsibilities, not guarantees enforced by the annotation.
-- **Follow-up implementation:** General orphan inventory garbage collection and Composition-deletion handling.
+- **Best-effort inventory cleanup:** Cleanup does not block Composition deletion. If the Composition disappears before cleanup succeeds, its inventory Secrets remain.
+- **Follow-up implementation:** General orphan inventory garbage collection.
 - **Follow-up hardening:** Admission policies restricting unauthorized inventory or ResourceSlice changes, while permitting legitimate cleanup and garbage collection.
 - **Follow-up validation:** End-to-end coverage is deferred; focused unit regressions exercise recovery decisions, inheritance, partial-write retries, status races, and lifecycle guards.

internal/controllers/tombstonerecovery/cleanup.go

@@ -0,0 +1,27 @@
+package tombstonerecovery
+
+import (
+	"context"
+	"fmt"
+
+	apiv1 "github.com/Azure/eno/api/v1"
+	"github.com/go-logr/logr"
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
+)
+
+// cleanupInventory deletes every inventory Secret for a deleting Composition's lineage.
+// Best effort: it does not block Composition deletion.
+func (c *tombstoneRecoveryController) cleanupInventory(ctx context.Context, comp *apiv1.Composition) error {
+	items, err := c.readInventories(ctx, comp)
+	if err != nil {
+		return err
+	}
+	for i := range items {
+		item := &items[i]
+		if err := c.downstream.Delete(ctx, item); err != nil && !apierrors.IsNotFound(err) {
+			return fmt.Errorf("deleting inventory Secret %q: %w", item.Name, err)
+		}
+		logr.FromContextOrDiscard(ctx).Info("deleted inventory chunk", "secretName", item.Name)
+	}
+	return nil
+}

internal/controllers/tombstonerecovery/controller.go

@@ -40,6 +40,8 @@ const (
 	compositionOperationRecover   compositionOperation = "recoverMissingTombstones"
 	compositionOperationNotNeeded compositionOperation = "recordRecoveryNotNeeded"
 	compositionOperationRecord    compositionOperation = "recordInventory"
+	compositionOperationCleanup   compositionOperation = "cleanupInventory"
+	symphonyDeletingLabel                              = "eno.azure.io/symphony-deleting"
 )
 
 var errSuperseded = errors.New("tombstone recovery operation superseded")
@@ -141,24 +143,19 @@ func (c *tombstoneRecoveryController) Reconcile(ctx context.Context, req ctrl.Re
 		logger.Error(err, "failed to get composition")
 		return ctrl.Result{}, err
 	}
-	// Deletion reuses existing slices and must not wait for tombstone recovery.
-	if comp.DeletionTimestamp != nil {
-		logger.Info("composition is deleting; skipping recovery and inventory recording", "compositionUID", comp.UID,
-			"synthesisUUID", comp.Status.GetCurrentSynthesisUUID())
-		return ctrl.Result{}, nil
-	}
 	syn := comp.Status.CurrentSynthesis
-	if syn == nil || syn.Synthesized == nil {
+	// Deletion cleanup finds inventory by lineage, so it does not need a synthesis.
+	if comp.DeletionTimestamp == nil && (syn == nil || syn.Synthesized == nil) {
 		return ctrl.Result{}, nil
 	}
-	logger = logger.WithValues("compositionUID", comp.UID, "compositionGeneration", comp.Generation, "synthesisUUID", syn.UUID,
+	logger = logger.WithValues("compositionUID", comp.UID, "compositionGeneration", comp.Generation, "synthesisUUID", comp.Status.GetCurrentSynthesisUUID(),
 		"synthesizerName", comp.Spec.Synthesizer.Name, "operationID", comp.GetAzureOperationID(), "operationOrigin", comp.GetAzureOperationOrigin())
 	ctx = logr.NewContext(ctx, logger)
 	if !comp.RecoveryEnabled() {
 		logger.Info("composition has not opted into recovery")
 		return ctrl.Result{}, nil
 	}
-	if syn.UUID == "" {
+	if comp.DeletionTimestamp == nil && syn.UUID == "" {
 		logger.Error(err, "invalid synthesis")
 		return ctrl.Result{}, fmt.Errorf("published synthesis has no UUID")
 	}
@@ -173,6 +170,8 @@ func (c *tombstoneRecoveryController) Reconcile(ctx context.Context, req ctrl.Re
 		err = c.recordRecoveryDecision(ctx, comp, reasonNotNeeded, "", nil)
 	case compositionOperationRecord:
 		err = c.recordInventory(ctx, comp)
+	case compositionOperationCleanup:
+		err = c.cleanupInventory(ctx, comp)
 	}
 
 	if errors.Is(err, errSuperseded) {
@@ -187,10 +186,21 @@ func (c *tombstoneRecoveryController) Reconcile(ctx context.Context, req ctrl.Re
 
 // selectTombstoneRecoveryOperation selects one idempotent operation without performing side effects.
 func selectTombstoneRecoveryOperation(comp *apiv1.Composition) compositionOperation {
-	if comp == nil || comp.Status.CurrentSynthesis == nil || comp.Status.CurrentSynthesis.Synthesized == nil {
+	if comp == nil {
 		return compositionOperationNone
 	}
+	// Deletion reuses existing slices and must not wait for tombstone recovery or record inventory.
+	if comp.DeletionTimestamp != nil {
+		// Symphony deletion orphans downstream resources, so their inventory is kept.
+		if comp.Labels[symphonyDeletingLabel] == "true" {
+			return compositionOperationNone
+		}
+		return compositionOperationCleanup
+	}
 	syn := comp.Status.CurrentSynthesis
+	if syn == nil || syn.Synthesized == nil {
+		return compositionOperationNone
+	}
 	if !syn.IsTombstoneRecoveryFinished() {
 		if syn.TombstoneRecoveryRequired {
 			return compositionOperationRecover

internal/controllers/tombstonerecovery/controller_test.go

@@ -242,6 +242,20 @@ func TestSelectTombstoneRecoveryOperation(t *testing.T) {
 				},
 			}},
 		}, want: compositionOperationRecover},
+		{name: "deleting cleans up inventory", comp: &apiv1.Composition{
+			ObjectMeta: metav1.ObjectMeta{DeletionTimestamp: ptr.To(metav1.Now())},
+			Status: apiv1.CompositionStatus{CurrentSynthesis: &apiv1.Synthesis{
+				Synthesized: ptr.To(metav1.Now()), TombstoneRecoveryRequired: true,
+			}},
+		}, want: compositionOperationCleanup},
+		{name: "symphony deleting keeps inventory", comp: &apiv1.Composition{
+			ObjectMeta: metav1.ObjectMeta{
+				DeletionTimestamp: ptr.To(metav1.Now()), Labels: map[string]string{symphonyDeletingLabel: "true"},
+			},
+			Status: apiv1.CompositionStatus{CurrentSynthesis: &apiv1.Synthesis{
+				Synthesized: ptr.To(metav1.Now()), TombstoneRecoveryRequired: true,
+			}},
+		}},
 	} {
 		t.Run(tc.name, func(t *testing.T) {
 			assert.Equal(t, tc.want, selectTombstoneRecoveryOperation(tc.comp))
@@ -567,19 +581,37 @@ func TestTombstoneRecoveryOperatorEmptyCurrentRecoveryAnnotation(t *testing.T) {
 	}
 }
 
-func TestTombstoneRecoveryOperatorDeletingSkipsInventory(t *testing.T) {
-	for _, recording := range []bool{false, true} {
-		t.Run(fmt.Sprintf("recording-%t", recording), func(t *testing.T) {
-			f := newControllerTestFixture(t, !recording)
+func TestTombstoneRecoveryOperatorDeletingCleansUpInventory(t *testing.T) {
+	for _, tc := range []struct {
+		name      string
+		recording bool
+		change    func(*apiv1.Composition)
+		keep      bool
+	}{
+		{name: "recovery-pending"},
+		{name: "recording", recording: true},
+		{name: "symphony-deleting", keep: true, change: func(comp *apiv1.Composition) {
+			comp.Labels = map[string]string{symphonyDeletingLabel: "true"}
+		}},
+		{name: "not-opted-in", keep: true, change: func(comp *apiv1.Composition) {
+			delete(comp.Annotations, "eno.azure.io/recovery-enabled")
+		}},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			f := newControllerTestFixture(t, !tc.recording)
 			history := f.history("removed")
-			if recording {
+			if tc.recording {
 				f.finish(reasonNotNeeded)
 				f.ready()
 			}
 			f.updateStatus(func(syn *apiv1.Synthesis) { syn.ResourceSlices = nil })
+			if tc.change != nil {
+				comp := f.composition()
+				tc.change(comp)
+				require.NoError(t, f.upstream.Update(t.Context(), comp))
+			}
 			require.NoError(t, f.upstream.Delete(t.Context(), f.composition()))
 			before := f.composition()
-			f.controller.downstream = nil
 			f.controller.client = interceptor.NewClient(f.upstream, interceptor.Funcs{
 				SubResourcePatch: func(context.Context, client.Client, string, client.Object, client.Patch, ...client.SubResourcePatchOption) error {
 					t.Error("deleting Composition must not receive tombstone recovery status writes")
@@ -588,11 +620,44 @@ func TestTombstoneRecoveryOperatorDeletingSkipsInventory(t *testing.T) {
 			})
 			require.NoError(t, f.reconcile())
 			assert.Equal(t, before, f.composition())
-			assert.Equal(t, []corev1.Secret{*history}, f.inventories())
+			if tc.keep {
+				assert.Equal(t, []corev1.Secret{*history}, f.inventories())
+			} else {
+				assert.Empty(t, f.inventories())
+			}
+			require.NoError(t, f.reconcile(), "repeated cleanup must be idempotent")
 		})
 	}
 }
 
+func TestTombstoneRecoveryOperatorDeletingRetriesInventoryCleanup(t *testing.T) {
+	f := newControllerTestFixture(t, true)
+	other := f.history("removed")
+	other.Name, other.UID, other.ResourceVersion = "unrelated", "unrelated-uid", ""
+	other.Labels = map[string]string{inventoryLineageLabel: "other-lineage"}
+	require.NoError(t, f.downstream.Create(t.Context(), other))
+	require.NoError(t, f.upstream.Delete(t.Context(), f.composition()))
+
+	injected := fmt.Errorf("injected delete failure")
+	fail := true
+	f.controller.downstream = interceptor.NewClient(f.downstream, interceptor.Funcs{
+		Delete: func(ctx context.Context, c client.WithWatch, obj client.Object, opts ...client.DeleteOption) error {
+			if fail {
+				return injected
+			}
+			return c.Delete(ctx, obj, opts...)
+		},
+	})
+	require.ErrorIs(t, f.reconcile(), injected)
+	assert.Len(t, f.inventories(), 2)
+
+	fail = false
+	require.NoError(t, f.reconcile())
+	items := f.inventories()
+	require.Len(t, items, 1, "only the deleting Composition's lineage is removed")
+	assert.Equal(t, "unrelated", items[0].Name)
+}
+
 func TestTombstoneRecoveryOperatorObsoleteWork(t *testing.T) {
 	for _, mode := range []string{"already-deleting", "deletion-during-read", "new-synthesis"} {
 		t.Run(mode, func(t *testing.T) {
@@ -603,7 +668,6 @@ func TestTombstoneRecoveryOperatorObsoleteWork(t *testing.T) {
 			if mode == "already-deleting" {
 				require.NoError(t, f.upstream.Delete(t.Context(), f.composition()))
 				expected = f.composition()
-				f.controller.downstream = nil
 			} else {
 				f.controller.downstream = interceptor.NewClient(f.downstream, interceptor.Funcs{
 					List: func(ctx context.Context, cli client.WithWatch, obj client.ObjectList, opts ...client.ListOption) error {
@@ -637,12 +701,16 @@ func TestTombstoneRecoveryOperatorObsoleteWork(t *testing.T) {
 			require.NotNil(t, expected, "the external deletion or supersession must occur")
 			assert.Equal(t, expected, f.composition(), "obsolete recovery must not acknowledge or change the new state")
 			assert.Equal(t, original.Spec, f.slice("desired").Spec)
-			assert.Equal(t, []corev1.Secret{*old}, f.inventories())
+			if mode == "already-deleting" {
+				assert.Empty(t, f.inventories(), "deleting Composition removes its lineage inventory")
+			} else {
+				assert.Equal(t, []corev1.Secret{*old}, f.inventories())
+			}
 			if mode != "new-synthesis" {
 				f.restart()
-				f.controller.downstream = nil
 				require.NoError(t, f.reconcile())
 				assert.Equal(t, expected, f.composition())
+				assert.Empty(t, f.inventories(), "deleting Composition removes its lineage inventory")
 			}
 		})
 	}

internal/controllers/tombstonerecovery/recording.go

@@ -33,7 +33,7 @@ func (c *tombstoneRecoveryController) recordInventory(ctx context.Context, comp
 		return fmt.Errorf("building inventory: %w", err)
 	}
 	for i := range secrets {
-		if err := c.writeInventoryChunk(ctx, comp, &secrets[i]); err != nil {
+		if err := c.writeInventoryChunk(ctx, &secrets[i]); err != nil {
 			return err
 		}
 	}
@@ -44,7 +44,7 @@ func (c *tombstoneRecoveryController) recordInventory(ctx context.Context, comp
 	return nil
 }
 
-func (c *tombstoneRecoveryController) writeInventoryChunk(ctx context.Context, comp *apiv1.Composition, intended *corev1.Secret) error {
+func (c *tombstoneRecoveryController) writeInventoryChunk(ctx context.Context, intended *corev1.Secret) error {
 	existing := &corev1.Secret{}
 	err := c.downstream.Get(ctx, client.ObjectKeyFromObject(intended), existing)
 	if apierrors.IsNotFound(err) {

internal/controllers/tombstonerecovery/recording_test.go

@@ -69,7 +69,9 @@ func TestInventoryRecordingEligibility(t *testing.T) {
 			case "deleting":
 				require.NoError(t, f.upstream.Delete(t.Context(), comp))
 			}
-			f.controller.downstream = nil // Neither inventory reads nor writes are allowed in these cases.
+			if mode != "deleting" {
+				f.controller.downstream = nil // Neither inventory reads nor writes are allowed in these cases.
+			}
 			f.reconcileEvent()
 			assert.Empty(t, f.inventories())
 		})