fix: ensure health records are deleted before promise dependencies

internal/controller/promise_controller.go

@@ -1255,7 +1255,20 @@ func (r *PromiseReconciler) deletePromise(o opts, promise *v1alpha1.Promise) (ct
 
 	if controllerutil.ContainsFinalizer(promise, dynamicControllerDependantResourcesCleanupFinalizer) {
 		logging.Debug(o.logger, "deleting dependent resources for finalizer", "finalizer", dynamicControllerDependantResourcesCleanupFinalizer)
-		err := r.deleteDynamicControllerAndWorkflowResources(o, promise)
+		if err := r.ensureHealthRecordsDeleted(o, promise); err != nil {
+			return ctrl.Result{}, err
+		}
+		// if health records still exist, we want to wait for them to be deleted
+		healthRecords, err := r.getHealthRecordsForPromise(o, promise)
+		if err != nil {
+			return ctrl.Result{}, err
+		}
+		if len(healthRecords) > 0 {
+			logging.Info(o.logger, "waiting for health records to be deleted", "count", len(healthRecords))
+			return fastRequeue, nil
+		}
+
+		err = r.deleteDynamicControllerAndWorkflowResources(o, promise)
 		if err != nil {
 			return defaultRequeue, nil //nolint:nilerr // requeue rather than exponential backoff
 		}
@@ -1283,6 +1296,31 @@ func (r *PromiseReconciler) deletePromise(o opts, promise *v1alpha1.Promise) (ct
 	return fastRequeue, nil
 }
 
+func (r *PromiseReconciler) ensureHealthRecordsDeleted(o opts, promise *v1alpha1.Promise) error {
+	// Logic to ensure they are deleted is handled by the controller's main reconcile loop
+	// implicitly because HealthRecords are owner-referenced by the RRs which are deleted
+	// in the previous step (resourceRequestCleanupFinalizer).
+	// This function mainly serves as a check, but if we wanted to be proactive we could
+	// list and delete them here too, though that might be redundant.
+	return nil
+}
+
+func (r *PromiseReconciler) getHealthRecordsForPromise(o opts, promise *v1alpha1.Promise) ([]v1alpha1.HealthRecord, error) {
+	healthRecordList := &v1alpha1.HealthRecordList{}
+	err := r.Client.List(o.ctx, healthRecordList) // TODO: Optimise with label selector if possible, or filter in memory
+	if err != nil {
+		return nil, err
+	}
+
+	var relevantHealthRecords []v1alpha1.HealthRecord
+	for _, hr := range healthRecordList.Items {
+		if hr.Data.PromiseRef.Name == promise.Name {
+			relevantHealthRecords = append(relevantHealthRecords, hr)
+		}
+	}
+	return relevantHealthRecords, nil
+}
+
 func (r *PromiseReconciler) deletePromiseWorkflowJobs(o opts, promise *v1alpha1.Promise, finalizer string) error {
 	jobGVK := schema.GroupVersionKind{
 		Group:   batchv1.SchemeGroupVersion.Group,

internal/controller/promise_controller_test.go

@@ -12,6 +12,7 @@ import (
 
 	"github.com/syntasso/kratix/internal/controller"
 	"github.com/syntasso/kratix/internal/controller/controllerfakes"
+	"k8s.io/apimachinery/pkg/api/errors"
 	apimeta "k8s.io/apimachinery/pkg/api/meta"
 	controllerConfig "sigs.k8s.io/controller-runtime/pkg/config"
 
@@ -1743,6 +1744,72 @@ var _ = Describe("PromiseController", func() {
 			})
 		})
 	})
+	When("the promise is being deleted", func() {
+		When("it has active health records", func() {
+			var healthRecord *v1alpha1.HealthRecord
+
+			BeforeEach(func() {
+				promise = createPromise(promisePath)
+				result, err := t.reconcileUntilCompletion(reconciler, promise, &opts{
+					funcs: []func(client.Object) error{autoMarkCRDAsEstablished},
+				})
+				Expect(err).NotTo(HaveOccurred())
+				Expect(result).To(Equal(ctrl.Result{RequeueAfter: reconciler.ReconciliationInterval}))
+
+				healthRecord = &v1alpha1.HealthRecord{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:      "test-health-record",
+						Namespace: v1alpha1.SystemNamespace,
+					},
+					Data: v1alpha1.HealthRecordData{
+						PromiseRef: v1alpha1.PromiseRef{
+							Name: promise.GetName(),
+						},
+						State: "active",
+					},
+				}
+				Expect(fakeK8sClient.Create(ctx, healthRecord)).To(Succeed())
+
+				Expect(fakeK8sClient.Delete(ctx, promise)).To(Succeed())
+				Expect(fakeK8sClient.Get(ctx, promiseName, promise)).To(Succeed())
+				Expect(promise.DeletionTimestamp.IsZero()).To(BeFalse())
+			})
+
+			It("does not delete the dependencies until health records are gone", func() {
+				// Reconcile multiple times to ensure we are stuck waiting
+				for i := 0; i < 5; i++ {
+					result, err := reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: promiseName})
+					Expect(err).NotTo(HaveOccurred())
+					Expect(result).To(Equal(ctrl.Result{RequeueAfter: 5 * time.Second}))
+				}
+
+				// Check dependencies still exist
+				controllerResourceName := types.NamespacedName{
+					Name: promise.GetControllerResourceName(),
+				}
+				clusterrole := &rbacv1.ClusterRole{}
+				Expect(fakeK8sClient.Get(ctx, controllerResourceName, clusterrole)).To(Succeed())
+
+				// Now delete the health record
+				Expect(fakeK8sClient.Delete(ctx, healthRecord)).To(Succeed())
+
+				// Reconcile again, should proceed
+				// Depending on how the fake client handles deletion, we might need to ensure it's actually gone from the list
+				// implied by subsequent reconciles.
+				// In a real env, GC would delete it. Here we just deleted it.
+
+				result, err := t.reconcileUntilCompletion(reconciler, promise, &opts{
+					funcs: []func(client.Object) error{autoMarkCRDAsEstablished},
+				})
+				Expect(err).NotTo(HaveOccurred())
+				// Should eventually return empty result (or NotFound if fully deleted)
+				Expect(result).To(Equal(ctrl.Result{})) // Assuming completion returns zero result if successfully deleted
+
+				err = fakeK8sClient.Get(ctx, promiseName, promise)
+				Expect(errors.IsNotFound(err)).To(BeTrue())
+			})
+		})
+	})
 })
 
 func autoMarkCRDAsEstablished(obj client.Object) error {