fix: recognize an already-purged Helm release as finalizer success

controllers/conditionalttl_controller.go

@@ -217,7 +217,7 @@ func (r *ConditionalTTLReconciler) Reconcile(ctx context.Context, req ctrl.Reque
 	}
 
 	if err := r.Delete(ctx, cTTL); err != nil {
-		return ctrl.Result{}, err
+		return ctrl.Result{}, client.IgnoreNotFound(err)
 	}
 
 	return ctrl.Result{}, nil
@@ -427,11 +427,33 @@ func (r *ConditionalTTLReconciler) helmReleaseFinalizer(ctx context.Context, cTT
 			return err
 		}
 	}
+	// Check existence upfront rather than relying solely on uninstall.Run's
+	// own error to signal "already gone": when Uninstall fails partway
+	// through - resources deleted but the release record itself missing by
+	// the time it reaches the purge step (e.g. this same finalizer already
+	// ran once and got requeued, or something else purged it concurrently)
+	// - Helm wraps driver.ErrReleaseNotFound in a new, flattened
+	// errors.Errorf("uninstallation completed with N error(s): ...") that
+	// breaks errors.Is (see helm.sh/helm/v3/pkg/action/uninstall.go:163).
+	// Without this, that specific failure mode retries the finalizer
+	// forever instead of recognizing there's nothing left to clean up.
+	if _, err := action.NewGet(cfg).Run(cTTL.Spec.Helm.Release); err != nil {
+		if isReleaseNotFoundErr(err) {
+			return nil
+		}
+		r.Recorder.Eventf(cTTL, corev1.EventTypeWarning, "HelmGetFailed", "Error checking Helm release %q: %s", cTTL.Spec.Helm.Release, err.Error())
+		return err
+	}
 	uninstall := action.NewUninstall(cfg)
 	// TODO: support custom options for uninstall such as Wait and DisableHooks?
 	_, err := uninstall.Run(cTTL.Spec.Helm.Release)
 	if err != nil {
-		if errors.Is(err, driver.ErrReleaseNotFound) {
+		// Belt-and-suspenders for the same race the upfront Get above
+		// mostly closes: if the release disappears between the Get and
+		// here, Run's own error is the flattened one described above, so
+		// errors.Is can't catch it - fall back to matching the sentinel's
+		// text, which does survive the flattening.
+		if isReleaseNotFoundErr(err) {
 			return nil
 		}
 		r.Recorder.Eventf(cTTL, corev1.EventTypeWarning, "HelmUninstallFailed", "Error uninstalling Helm release %q: %s", cTTL.Spec.Helm.Release, err.Error())
@@ -441,6 +463,17 @@ func (r *ConditionalTTLReconciler) helmReleaseFinalizer(ctx context.Context, cTT
 	return nil
 }
 
+// isReleaseNotFoundErr reports whether err indicates the Helm release was
+// already gone. errors.Is(err, driver.ErrReleaseNotFound) only matches when
+// the sentinel survives unwrapped; Uninstall.Run flattens it into a new
+// errors.Errorf-formatted string when the release disappears partway
+// through (resources deleted, then the purge step finds no release record
+// left), so this also falls back to matching the sentinel's text, which
+// does survive that flattening.
+func isReleaseNotFoundErr(err error) bool {
+	return errors.Is(err, driver.ErrReleaseNotFound) || strings.Contains(err.Error(), driver.ErrReleaseNotFound.Error())
+}
+
 // cloudEventFinalizer handles cleaner.vtex.io/cloud-event-finalizer by sending
 // a CloudEvent of type conditionalTTL.deleted, from source cleaner.vtex.io/finalizer
 // to the sink configured on the cTTL spec.

controllers/helm_release_finalizer_test.go

@@ -0,0 +1,107 @@
+/*
+Copyright 2022.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package controllers
+
+import (
+	"context"
+	"errors"
+	"io"
+	"testing"
+
+	"helm.sh/helm/v3/pkg/action"
+	kubefake "helm.sh/helm/v3/pkg/kube/fake"
+	"helm.sh/helm/v3/pkg/storage"
+	"helm.sh/helm/v3/pkg/storage/driver"
+
+	cleanerv1alpha1 "github.com/vtex/cleaner-controller/api/v1alpha1"
+)
+
+// This file only ever touches Helm's in-memory storage driver
+// (driver.NewMemory()) - no real cluster, no real Helm release, nothing
+// network-facing. It's the same fixture pattern Helm's own unit tests use
+// (see helm.sh/helm/v3/pkg/action/action_test.go's actionConfigFixture).
+
+func TestIsReleaseNotFoundErr(t *testing.T) {
+	cases := []struct {
+		name string
+		err  error
+		want bool
+	}{
+		{"sentinel unwrapped", driver.ErrReleaseNotFound, true},
+		{
+			"flattened by Uninstall.Run's purge-error path",
+			errors.New("uninstallation completed with 1 error(s): uninstall: Failed to purge the release: release: not found"),
+			true,
+		},
+		{"unrelated error", errors.New("some other failure"), false},
+	}
+	for _, c := range cases {
+		t.Run(c.name, func(t *testing.T) {
+			if got := isReleaseNotFoundErr(c.err); got != c.want {
+				t.Fatalf("isReleaseNotFoundErr(%q) = %v, want %v", c.err, got, c.want)
+			}
+		})
+	}
+}
+
+// TestHelmReleaseFinalizer_AlreadyGone reproduces the tupan/trx incident: a
+// ConditionalTTL's release-finalizer targeting a Helm release that no
+// longer exists in storage. Both action.Get and action.Uninstall need a
+// working KubeClient just to check IsReachable() before touching storage,
+// so PrintingKubeClient (Helm's own no-op test double, writing to
+// io.Discard) stands in for one - it never talks to a real cluster.
+func TestHelmReleaseFinalizer_AlreadyGone(t *testing.T) {
+	cfg := &action.Configuration{
+		Releases:   storage.Init(driver.NewMemory()),
+		KubeClient: &kubefake.PrintingKubeClient{Out: io.Discard},
+		Log:        func(string, ...interface{}) {},
+	}
+
+	r := &ConditionalTTLReconciler{HelmConfig: cfg}
+	cTTL := &cleanerv1alpha1.ConditionalTTL{
+		Spec: cleanerv1alpha1.ConditionalTTLSpec{
+			Helm: &cleanerv1alpha1.HelmConfig{
+				Release: "sfj-2f40163--tupan",
+				Delete:  true,
+			},
+		},
+	}
+
+	if err := r.helmReleaseFinalizer(context.Background(), cTTL); err != nil {
+		t.Fatalf("helmReleaseFinalizer() = %v, want nil (a release that never existed must not block the finalizer)", err)
+	}
+}
+
+func TestHelmReleaseFinalizer_NoopWhenHelmSpecNil(t *testing.T) {
+	r := &ConditionalTTLReconciler{}
+	cTTL := &cleanerv1alpha1.ConditionalTTL{}
+	if err := r.helmReleaseFinalizer(context.Background(), cTTL); err != nil {
+		t.Fatalf("helmReleaseFinalizer() with nil Spec.Helm = %v, want nil", err)
+	}
+}
+
+func TestHelmReleaseFinalizer_NoopWhenDeleteFalse(t *testing.T) {
+	r := &ConditionalTTLReconciler{}
+	cTTL := &cleanerv1alpha1.ConditionalTTL{
+		Spec: cleanerv1alpha1.ConditionalTTLSpec{
+			Helm: &cleanerv1alpha1.HelmConfig{Release: "whatever", Delete: false},
+		},
+	}
+	if err := r.helmReleaseFinalizer(context.Background(), cTTL); err != nil {
+		t.Fatalf("helmReleaseFinalizer() with Delete=false = %v, want nil", err)
+	}
+}

controllers/update_conflict.go

@@ -44,21 +44,32 @@ func init() {
 }
 
 // handleUpdateErr turns the error from an r.Update/r.Status().Update call
-// into a reconcile outcome. A conflict - the object was modified since we
-// read it, e.g. by the controller's own prior status write racing a fresh
-// watch-triggered reconcile - is expected and self-heals: it's logged at
-// Info instead of Error, counted separately from real failures, and
-// answered with a requeue instead of a returned error. Returning the error
-// here would make controller-runtime log it again at Error as "Reconciler
-// error" and count it against controller_runtime_reconcile_errors_total
-// alongside genuine failures. Info rather than a synthetic Warn: logr (used
-// throughout via log.FromContext) has no Warn level, only Info and Error,
-// and Info is where every other non-fatal, expected condition in this
-// codebase already logs.
+// into a reconcile outcome.
+//
+// A conflict - the object was modified since we read it, e.g. by the
+// controller's own prior status write racing a fresh watch-triggered
+// reconcile - is expected and self-heals: it's logged at Info instead of
+// Error, counted separately from real failures, and answered with a
+// requeue instead of a returned error. Info rather than a synthetic Warn:
+// logr (used throughout via log.FromContext) has no Warn level, only Info
+// and Error, and Info is where every other non-fatal, expected condition in
+// this codebase already logs.
+//
+// A NotFound - the object was deleted by something else between our Get and
+// this Update - has nothing left to reconcile; it's swallowed with no log
+// and no requeue, same as client.IgnoreNotFound on a Get.
+//
+// Returning the error unchanged for anything else preserves
+// controller-runtime's normal handling: it logs "Reconciler error" at Error
+// and counts it against controller_runtime_reconcile_errors_total, which is
+// correct for a genuine failure.
 func handleUpdateErr(log logr.Logger, err error) (ctrl.Result, error) {
 	if err == nil {
 		return ctrl.Result{}, nil
 	}
+	if apierrors.IsNotFound(err) {
+		return ctrl.Result{}, nil
+	}
 	if !apierrors.IsConflict(err) {
 		return ctrl.Result{}, err
 	}

controllers/update_conflict_test.go

@@ -56,10 +56,19 @@ func TestHandleUpdateErr(t *testing.T) {
 		"sfj-c5bc713--umbroco",
 	)
 	res, err = handleUpdateErr(logr.Discard(), notFoundErr)
-	if err != notFoundErr {
-		t.Fatalf("handleUpdateErr(NotFound) swallowed a non-conflict error: got err = %v, want it returned unchanged so it still surfaces as a real reconcile failure", err)
+	if err != nil {
+		t.Fatalf("handleUpdateErr(NotFound) = %v, want nil (the object is already gone, nothing left to update)", err)
+	}
+	if res.Requeue {
+		t.Fatalf("handleUpdateErr(NotFound) set Requeue = true; a deleted object should not be requeued")
+	}
+
+	unrelatedErr := apierrors.NewInternalError(errors.New("boom"))
+	res, err = handleUpdateErr(logr.Discard(), unrelatedErr)
+	if err != unrelatedErr {
+		t.Fatalf("handleUpdateErr(unrelated) swallowed a real error: got err = %v, want it returned unchanged so it still surfaces as a reconcile failure", err)
 	}
 	if res.Requeue {
-		t.Fatalf("handleUpdateErr(NotFound) set Requeue = true; only conflicts should self-requeue")
+		t.Fatalf("handleUpdateErr(unrelated) set Requeue = true; only conflicts should self-requeue")
 	}
 }