fix: prevent finalizer from blocking subscription deletion when TRLP is in bad state

handleDeletion now uses best-effort cleanup: if reconcileTRLPForModel fails
(e.g. Kuadrant webhook rejects a rebuild), it falls back to force-deleting
the TRLP by labels. If that also fails, the finalizer is still removed so
the subscription deletion is never permanently blocked. An orphaned TRLP
will be garbage-collected via its HTTPRoute owner reference.

Signed-off-by: Wen Liang <liangwen12year@gmail.com>

Author: liangwen12year

maas-controller/pkg/controller/maas/maassubscription_controller.go

@@ -684,9 +684,9 @@ func (r *MaaSSubscriptionReconciler) deleteModelTRLP(ctx context.Context, log lo
 
 func (r *MaaSSubscriptionReconciler) handleDeletion(ctx context.Context, log logr.Logger, subscription *maasv1alpha1.MaaSSubscription) (ctrl.Result, error) {
 	if controllerutil.ContainsFinalizer(subscription, maasSubscriptionFinalizer) {
-		// For each model referenced by this subscription, rebuild the aggregated TokenRateLimitPolicy
-		// without the deleted subscription's limits. If no other subscriptions reference the model,
-		// the TRLP will be deleted. This ensures zero-downtime rate limiting during subscription removal.
+		// Best-effort TRLP cleanup: try to rebuild cleanly, fall back to force-delete,
+		// and proceed with finalizer removal even if cleanup fails entirely.
+		// An orphaned TRLP is preferable to a permanently stuck subscription deletion.
 		seen := make(map[string]struct{}, len(subscription.Spec.ModelRefs))
 		for _, modelRef := range subscription.Spec.ModelRefs {
 			k := modelRef.Namespace + "/" + modelRef.Name
@@ -696,14 +696,14 @@ func (r *MaaSSubscriptionReconciler) handleDeletion(ctx context.Context, log log
 			seen[k] = struct{}{}
 			log.Info("Rebuilding TokenRateLimitPolicy without deleted subscription", "model", modelRef.Namespace+"/"+modelRef.Name, "subscription", subscription.Name)
 			if err := r.reconcileTRLPForModel(ctx, log, modelRef.Namespace, modelRef.Name); err != nil {
-				log.Error(err, "failed to reconcile TokenRateLimitPolicy during deletion, will retry", "model", modelRef.Namespace+"/"+modelRef.Name)
-				return ctrl.Result{}, err
+				log.Error(err, "TRLP reconciliation failed during deletion, falling back to force-delete", "model", modelRef.Namespace+"/"+modelRef.Name)
+				if delErr := r.deleteModelTRLP(ctx, log, modelRef.Namespace, modelRef.Name); delErr != nil {
+					log.Error(delErr, "TRLP force-delete also failed, proceeding with finalizer removal anyway", "model", modelRef.Namespace+"/"+modelRef.Name)
+				}
 			}
 		}
-		// Also clean up stale TRLPs from modelRefs that were removed
-		// before the CR was deleted (edge case: edit + delete before reconcile).
 		if err := r.cleanupStaleTRLPs(ctx, log, subscription); err != nil {
-			return ctrl.Result{}, err
+			log.Error(err, "stale TRLP cleanup failed during deletion, proceeding with finalizer removal anyway")
 		}
 		controllerutil.RemoveFinalizer(subscription, maasSubscriptionFinalizer)
 		if err := r.Update(ctx, subscription); err != nil {

maas-controller/pkg/controller/maas/maassubscription_controller_test.go

@@ -18,6 +18,7 @@ package maas
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"strings"
 	"testing"
@@ -31,6 +32,7 @@ import (
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/client/fake"
+	"sigs.k8s.io/controller-runtime/pkg/client/interceptor"
 	gatewayapiv1 "sigs.k8s.io/gateway-api/apis/v1"
 
 	maasv1alpha1 "github.com/opendatahub-io/models-as-a-service/maas-controller/api/maas/v1alpha1"
@@ -1346,3 +1348,128 @@ func TestMaaSSubscriptionReconciler_WindowValuesInTRLP(t *testing.T) {
 		})
 	}
 }
+
+// TestHandleDeletion_ReconcileFails_FallsBackToDeleteTRLP verifies that when
+// reconcileTRLPForModel fails during deletion (e.g. Kuadrant webhook rejects the
+// TRLP update), the handler falls back to force-deleting the TRLP via labels and
+// still removes the finalizer so the subscription deletion is not permanently blocked.
+func TestHandleDeletion_ReconcileFails_FallsBackToDeleteTRLP(t *testing.T) {
+	const (
+		modelName = "shared-model"
+		namespace = "default"
+		trlpName  = "maas-trlp-" + modelName
+	)
+
+	// MaaSModelRef + HTTPRoute are required so reconcileTRLPForModel gets past
+	// findHTTPRouteForModel and actually attempts the TRLP Update (which we fail).
+	model := &maasv1alpha1.MaaSModelRef{
+		ObjectMeta: metav1.ObjectMeta{Name: modelName, Namespace: namespace},
+		Spec:       maasv1alpha1.MaaSModelSpec{ModelRef: maasv1alpha1.ModelReference{Kind: "ExternalModel", Name: modelName}},
+	}
+	route := &gatewayapiv1.HTTPRoute{
+		ObjectMeta: metav1.ObjectMeta{Name: modelName, Namespace: namespace},
+	}
+	existingTRLP := newPreexistingTRLP(trlpName, namespace, modelName, map[string]string{})
+	subA := newMaaSSubscription("sub-a", namespace, "team-a", modelName, 100)
+	subA.Finalizers = []string{maasSubscriptionFinalizer}
+	subB := newMaaSSubscription("sub-b", namespace, "team-b", modelName, 200)
+
+	c := fake.NewClientBuilder().
+		WithScheme(scheme).
+		WithRESTMapper(testRESTMapper()).
+		WithObjects(model, route, subA, subB, existingTRLP).
+		WithIndex(&maasv1alpha1.MaaSSubscription{}, "spec.modelRef", subscriptionModelRefIndexer).
+		WithInterceptorFuncs(interceptor.Funcs{
+			Update: func(ctx context.Context, cl client.WithWatch, obj client.Object, opts ...client.UpdateOption) error {
+				if u, ok := obj.(*unstructured.Unstructured); ok && u.GetKind() == "TokenRateLimitPolicy" {
+					return errors.New("simulated Kuadrant webhook rejection")
+				}
+				return cl.Update(ctx, obj, opts...)
+			},
+		}).
+		Build()
+
+	if err := c.Delete(context.Background(), subA); err != nil {
+		t.Fatalf("Delete sub-a: %v", err)
+	}
+
+	r := &MaaSSubscriptionReconciler{Client: c, Scheme: scheme}
+	req := ctrl.Request{NamespacedName: types.NamespacedName{Name: "sub-a", Namespace: namespace}}
+	if _, err := r.Reconcile(context.Background(), req); err != nil {
+		t.Fatalf("Reconcile: unexpected error: %v", err)
+	}
+
+	// TRLP should be deleted via the fallback deleteModelTRLP path
+	got := &unstructured.Unstructured{}
+	got.SetGroupVersionKind(schema.GroupVersionKind{Group: "kuadrant.io", Version: "v1alpha1", Kind: "TokenRateLimitPolicy"})
+	if err := c.Get(context.Background(), types.NamespacedName{Name: trlpName, Namespace: namespace}, got); !apierrors.IsNotFound(err) {
+		t.Errorf("expected TRLP to be deleted via fallback, but got err: %v", err)
+	}
+
+	// Finalizer should be removed (subscription should be gone from the store)
+	var sub maasv1alpha1.MaaSSubscription
+	if err := c.Get(context.Background(), types.NamespacedName{Name: "sub-a", Namespace: namespace}, &sub); !apierrors.IsNotFound(err) {
+		t.Errorf("expected sub-a to be fully deleted (finalizer removed), got err: %v", err)
+	}
+}
+
+// TestHandleDeletion_AllCleanupFails_StillRemovesFinalizer verifies that even when
+// both TRLP reconciliation and force-delete fail, the finalizer is still removed
+// so the subscription deletion is never permanently blocked.
+func TestHandleDeletion_AllCleanupFails_StillRemovesFinalizer(t *testing.T) {
+	const (
+		modelName = "shared-model"
+		namespace = "default"
+		trlpName  = "maas-trlp-" + modelName
+	)
+
+	model := &maasv1alpha1.MaaSModelRef{
+		ObjectMeta: metav1.ObjectMeta{Name: modelName, Namespace: namespace},
+		Spec:       maasv1alpha1.MaaSModelSpec{ModelRef: maasv1alpha1.ModelReference{Kind: "ExternalModel", Name: modelName}},
+	}
+	route := &gatewayapiv1.HTTPRoute{
+		ObjectMeta: metav1.ObjectMeta{Name: modelName, Namespace: namespace},
+	}
+	existingTRLP := newPreexistingTRLP(trlpName, namespace, modelName, map[string]string{})
+	subA := newMaaSSubscription("sub-a", namespace, "team-a", modelName, 100)
+	subA.Finalizers = []string{maasSubscriptionFinalizer}
+	subB := newMaaSSubscription("sub-b", namespace, "team-b", modelName, 200)
+
+	// Fail TRLP Update (breaks reconcileTRLPForModel) AND TRLP List (breaks deleteModelTRLP fallback).
+	c := fake.NewClientBuilder().
+		WithScheme(scheme).
+		WithRESTMapper(testRESTMapper()).
+		WithObjects(model, route, subA, subB, existingTRLP).
+		WithIndex(&maasv1alpha1.MaaSSubscription{}, "spec.modelRef", subscriptionModelRefIndexer).
+		WithInterceptorFuncs(interceptor.Funcs{
+			Update: func(ctx context.Context, cl client.WithWatch, obj client.Object, opts ...client.UpdateOption) error {
+				if u, ok := obj.(*unstructured.Unstructured); ok && u.GetKind() == "TokenRateLimitPolicy" {
+					return errors.New("simulated TRLP update failure")
+				}
+				return cl.Update(ctx, obj, opts...)
+			},
+			List: func(ctx context.Context, cl client.WithWatch, list client.ObjectList, opts ...client.ListOption) error {
+				if ul, ok := list.(*unstructured.UnstructuredList); ok && ul.GetKind() == "TokenRateLimitPolicyList" {
+					return errors.New("simulated TRLP list failure")
+				}
+				return cl.List(ctx, list, opts...)
+			},
+		}).
+		Build()
+
+	if err := c.Delete(context.Background(), subA); err != nil {
+		t.Fatalf("Delete sub-a: %v", err)
+	}
+
+	r := &MaaSSubscriptionReconciler{Client: c, Scheme: scheme}
+	req := ctrl.Request{NamespacedName: types.NamespacedName{Name: "sub-a", Namespace: namespace}}
+	if _, err := r.Reconcile(context.Background(), req); err != nil {
+		t.Fatalf("Reconcile should not return error even when all cleanup fails, got: %v", err)
+	}
+
+	// Finalizer should be removed despite all cleanup failures
+	var sub maasv1alpha1.MaaSSubscription
+	if err := c.Get(context.Background(), types.NamespacedName{Name: "sub-a", Namespace: namespace}, &sub); !apierrors.IsNotFound(err) {
+		t.Errorf("expected sub-a to be fully deleted (finalizer removed despite cleanup failures), got err: %v", err)
+	}
+}