helmrepo: Add more reconciler design improvements

- Remove ArtifactUnavailable condition and use Reconciling condition to
  convey the same.
- Make Reconciling condition affect the ready condition.
- Introduce summarizeAndPatch() to calculate the final status conditions
  and patch them.
- Introduce reconcile() to iterate through the sub-reconcilers and
  execute them.
- Simplify logging and event recording
- Introduce controller-check/status checks to assert that the status
  conditions are valid at the end of the tests.
- Create variables for various condition groups: owned conditions, ready
  dependencies and ready dependencies negative.

Signed-off-by: Sunny <darkowlzz@protonmail.com>

controllers/helmrepository_controller.go

@@ -18,6 +18,7 @@ package controllers
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"net/url"
 	"os"
@@ -26,7 +27,7 @@ import (
 	helmgetter "helm.sh/helm/v3/pkg/getter"
 	corev1 "k8s.io/api/core/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
-	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/types"
 	kerrors "k8s.io/apimachinery/pkg/util/errors"
 	kuberecorder "k8s.io/client-go/tools/record"
@@ -43,10 +44,38 @@ import (
 	"github.com/fluxcd/pkg/runtime/predicates"
 
 	sourcev1 "github.com/fluxcd/source-controller/api/v1beta2"
+	serror "github.com/fluxcd/source-controller/internal/error"
 	"github.com/fluxcd/source-controller/internal/helm/getter"
 	"github.com/fluxcd/source-controller/internal/helm/repository"
+	sreconcile "github.com/fluxcd/source-controller/internal/reconcile"
 )
 
+// Status conditions owned by HelmRepository reconciler.
+var helmRepoOwnedConditions = []string{
+	sourcev1.FetchFailedCondition,
+	sourcev1.ArtifactOutdatedCondition,
+	meta.ReadyCondition,
+	meta.ReconcilingCondition,
+	meta.StalledCondition,
+}
+
+// Conditions that Ready condition is influenced by in descending order of their
+// priority.
+var helmRepoReadyDeps = []string{
+	sourcev1.FetchFailedCondition,
+	sourcev1.ArtifactOutdatedCondition,
+	meta.StalledCondition,
+	meta.ReconcilingCondition,
+}
+
+// Negative conditions that Ready condition is influenced by.
+var helmRepoReadyDepsNegative = []string{
+	sourcev1.FetchFailedCondition,
+	sourcev1.ArtifactOutdatedCondition,
+	meta.StalledCondition,
+	meta.ReconcilingCondition,
+}
+
 // +kubebuilder:rbac:groups=source.toolkit.fluxcd.io,resources=helmrepositories,verbs=get;list;watch;create;update;patch;delete
 // +kubebuilder:rbac:groups=source.toolkit.fluxcd.io,resources=helmrepositories/status,verbs=get;update;patch
 // +kubebuilder:rbac:groups=source.toolkit.fluxcd.io,resources=helmrepositories/finalizers,verbs=get;create;update;patch;delete
@@ -66,6 +95,11 @@ type HelmRepositoryReconcilerOptions struct {
 	MaxConcurrentReconciles int
 }
 
+// helmRepoReconcilerFunc is the function type for all the helm repository
+// reconciler functions. The reconciler functions are grouped together and
+// executed serially to perform the main operation of the reconciler.
+type helmRepoReconcilerFunc func(ctx context.Context, obj *sourcev1.HelmRepository, artifact *sourcev1.Artifact, repo *repository.ChartRepository) (sreconcile.Result, error)
+
 func (r *HelmRepositoryReconciler) SetupWithManager(mgr ctrl.Manager) error {
 	return r.SetupWithManagerAndOptions(mgr, HelmRepositoryReconcilerOptions{})
 }
@@ -103,71 +137,15 @@ func (r *HelmRepositoryReconciler) Reconcile(ctx context.Context, req ctrl.Reque
 		return ctrl.Result{}, err
 	}
 
-	// Always attempt to patch the object and status after each reconciliation
+	// Result of the sub-reconciliation.
+	var recResult sreconcile.Result
+
+	// Always attempt to patch the object after each reconciliation.
+	// NOTE: This deferred block only modifies the named return error. The
+	// result from the reconciliation remains the same. Any requeue attributes
+	// set in the result will continue to be effective.
 	defer func() {
-		// Record the value of the reconciliation request, if any
+		retErr = r.summarizeAndPatch(ctx, obj, patchHelper, recResult, retErr)
-		if v, ok := meta.ReconcileAnnotationValue(obj.GetAnnotations()); ok {
-			obj.Status.SetLastHandledReconcileRequest(v)
-		}
-
-		// Summarize Ready condition
-		conditions.SetSummary(obj,
-			meta.ReadyCondition,
-			conditions.WithConditions(
-				sourcev1.FetchFailedCondition,
-				sourcev1.ArtifactOutdatedCondition,
-				sourcev1.ArtifactUnavailableCondition,
-			),
-			conditions.WithNegativePolarityConditions(
-				sourcev1.FetchFailedCondition,
-				sourcev1.ArtifactOutdatedCondition,
-				sourcev1.ArtifactUnavailableCondition,
-			),
-		)
-
-		// Patch the object, ignoring conflicts on the conditions owned by this controller
-		patchOpts := []patch.Option{
-			patch.WithOwnedConditions{
-				Conditions: []string{
-					sourcev1.FetchFailedCondition,
-					sourcev1.ArtifactOutdatedCondition,
-					sourcev1.ArtifactUnavailableCondition,
-					meta.ReadyCondition,
-					meta.ReconcilingCondition,
-					meta.StalledCondition,
-				},
-			},
-		}
-
-		// Determine if the resource is still being reconciled, or if it has stalled, and record this observation
-		if retErr == nil && (result.IsZero() || !result.Requeue) {
-			// We are no longer reconciling
-			conditions.Delete(obj, meta.ReconcilingCondition)
-
-			// We have now observed this generation
-			patchOpts = append(patchOpts, patch.WithStatusObservedGeneration{})
-
-			readyCondition := conditions.Get(obj, meta.ReadyCondition)
-			switch readyCondition.Status {
-			case metav1.ConditionFalse:
-				// As we are no longer reconciling and the end-state
-				// is not ready, the reconciliation has stalled
-				conditions.MarkStalled(obj, readyCondition.Reason, readyCondition.Message)
-			case metav1.ConditionTrue:
-				// As we are no longer reconciling and the end-state
-				// is ready, the reconciliation is no longer stalled
-				conditions.Delete(obj, meta.StalledCondition)
-			}
-		}
-
-		// Finally, patch the resource
-		if err := patchHelper.Patch(ctx, obj, patchOpts...); err != nil {
-			// Ignore patch error "not found" when the object is being deleted.
-			if !obj.ObjectMeta.DeletionTimestamp.IsZero() {
-				err = kerrors.FilterOut(err, func(e error) bool { return apierrors.IsNotFound(e) })
-			}
-			retErr = kerrors.NewAggregate([]error{retErr, err})
-		}
 
 		// Always record readiness and duration metrics
 		r.Metrics.RecordReadiness(ctx, obj)
@@ -178,53 +156,104 @@ func (r *HelmRepositoryReconciler) Reconcile(ctx context.Context, req ctrl.Reque
 	// between init and delete
 	if !controllerutil.ContainsFinalizer(obj, sourcev1.SourceFinalizer) {
 		controllerutil.AddFinalizer(obj, sourcev1.SourceFinalizer)
+		recResult = sreconcile.ResultRequeue
 		return ctrl.Result{Requeue: true}, nil
 	}
 
 	// Examine if the object is under deletion
 	if !obj.ObjectMeta.DeletionTimestamp.IsZero() {
-		return r.reconcileDelete(ctx, obj)
+		res, err := r.reconcileDelete(ctx, obj)
+		return sreconcile.BuildRuntimeResult(ctx, r.EventRecorder, obj, res, err)
 	}
 
 	// Reconcile actual object
-	return r.reconcile(ctx, obj)
+	reconcilers := []helmRepoReconcilerFunc{
+		r.reconcileStorage,
+		r.reconcileSource,
+		r.reconcileArtifact,
+	}
+	recResult, err = r.reconcile(ctx, obj, reconcilers)
+	return sreconcile.BuildRuntimeResult(ctx, r.EventRecorder, obj, recResult, err)
 }
 
-// reconcile steps through the actual reconciliation tasks for the object, it returns early on the first step that
+// summarizeAndPatch analyzes the object conditions to create a summary of the
-// produces an error.
+// status conditions and patches the object with the calculated summary. The
-func (r *HelmRepositoryReconciler) reconcile(ctx context.Context, obj *sourcev1.HelmRepository) (ctrl.Result, error) {
+// reconciler error type is also used to determine the conditions and the
-	// Mark the resource as under reconciliation
+// returned error.
-	conditions.MarkReconciling(obj, meta.ProgressingReason, "")
+func (r *HelmRepositoryReconciler) summarizeAndPatch(ctx context.Context, obj *sourcev1.HelmRepository, patchHelper *patch.Helper, res sreconcile.Result, recErr error) error {
+	// Record the value of the reconciliation request, if any.
+	if v, ok := meta.ReconcileAnnotationValue(obj.GetAnnotations()); ok {
+		obj.Status.SetLastHandledReconcileRequest(v)
+	}
 
-	// Reconcile the storage data
+	// Compute the reconcile results, obtain patch options and reconcile error.
-	if result, err := r.reconcileStorage(ctx, obj); err != nil {
+	var patchOpts []patch.Option
-		return result, err
+	patchOpts, recErr = sreconcile.ComputeReconcileResult(obj, res, recErr, helmRepoOwnedConditions)
+
+	// Summarize Ready condition.
+	conditions.SetSummary(obj,
+		meta.ReadyCondition,
+		conditions.WithConditions(
+			helmRepoReadyDeps...,
+		),
+		conditions.WithNegativePolarityConditions(
+			helmRepoReadyDepsNegative...,
+		),
+	)
+
+	// Finally, patch the resource.
+	if err := patchHelper.Patch(ctx, obj, patchOpts...); err != nil {
+		// Ignore patch error "not found" when the object is being deleted.
+		if !obj.ObjectMeta.DeletionTimestamp.IsZero() {
+			err = kerrors.FilterOut(err, func(e error) bool { return apierrors.IsNotFound(e) })
+		}
+		recErr = kerrors.NewAggregate([]error{recErr, err})
+	}
+
+	return recErr
+}
+
+// reconcile iterates through the sub-reconcilers and processes the source
+// object. The sub-reconcilers are run sequentially. The result and error  of
+// the sub-reconciliation are collected and returned. For multiple results
+// from different sub-reconcilers, the results are combined to return the
+// result with the shortest requeue period.
+func (r *HelmRepositoryReconciler) reconcile(ctx context.Context, obj *sourcev1.HelmRepository, reconcilers []helmRepoReconcilerFunc) (sreconcile.Result, error) {
+	if obj.Generation != obj.Status.ObservedGeneration {
+		conditions.MarkReconciling(obj, "NewGeneration", "reconciling new generation %d", obj.Generation)
 	}
 
 	var chartRepo repository.ChartRepository
 	var artifact sourcev1.Artifact
-	// Reconcile the source from upstream
-	if result, err := r.reconcileSource(ctx, obj, &artifact, &chartRepo); err != nil || result.IsZero() {
-		return result, err
-	}
 
-	// Reconcile the artifact.
+	// Run the sub-reconcilers and build the result of reconciliation.
-	if result, err := r.reconcileArtifact(ctx, obj, artifact, &chartRepo); err != nil || result.IsZero() {
+	var res sreconcile.Result
-		return result, err
+	var resErr error
+	for _, rec := range reconcilers {
+		recResult, err := rec(ctx, obj, &artifact, &chartRepo)
+		// Exit immediately on ResultRequeue.
+		if recResult == sreconcile.ResultRequeue {
+			return sreconcile.ResultRequeue, nil
 		}
-
+		// If an error is received, prioritize the returned results because an
-	return ctrl.Result{RequeueAfter: obj.GetRequeueAfter()}, nil
+		// error also means immediate requeue.
+		if err != nil {
+			resErr = err
+			res = recResult
+			break
+		}
+		// Prioritize requeue request in the result for successful results.
+		res = sreconcile.LowestRequeuingResult(res, recResult)
+	}
+	return res, resErr
 }
 
 // reconcileStorage ensures the current state of the storage matches the desired and previously observed state.
 //
 // All artifacts for the resource except for the current one are garbage collected from the storage.
 // If the artifact in the Status object of the resource disappeared from storage, it is removed from the object.
-// If the object does not have an artifact in its Status object, a v1beta1.ArtifactUnavailableCondition is set.
 // If the hostname of any of the URLs on the object do not match the current storage server hostname, they are updated.
-//
+func (r *HelmRepositoryReconciler) reconcileStorage(ctx context.Context, obj *sourcev1.HelmRepository, artifact *sourcev1.Artifact, chartRepo *repository.ChartRepository) (sreconcile.Result, error) {
-// The caller should assume a failure if an error is returned, or the Result is zero.
-func (r *HelmRepositoryReconciler) reconcileStorage(ctx context.Context, obj *sourcev1.HelmRepository) (ctrl.Result, error) {
 	// Garbage collect previous advertised artifact(s) from storage
 	_ = r.garbageCollect(ctx, obj)
 
@@ -236,17 +265,16 @@ func (r *HelmRepositoryReconciler) reconcileStorage(ctx context.Context, obj *so
 
 	// Record that we do not have an artifact
 	if obj.GetArtifact() == nil {
-		conditions.MarkTrue(obj, sourcev1.ArtifactUnavailableCondition, "NoArtifact", "No artifact for resource in storage")
+		conditions.MarkReconciling(obj, "NoArtifact", "no artifact for resource in storage")
-		return ctrl.Result{Requeue: true}, nil
+		return sreconcile.ResultSuccess, nil
 	}
-	conditions.Delete(obj, sourcev1.ArtifactUnavailableCondition)
 
 	// Always update URLs to ensure hostname is up-to-date
 	// TODO(hidde): we may want to send out an event only if we notice the URL has changed
 	r.Storage.SetArtifactURL(obj.GetArtifact())
 	obj.Status.URL = r.Storage.SetHostname(obj.Status.URL)
 
-	return ctrl.Result{RequeueAfter: obj.GetRequeueAfter()}, nil
+	return sreconcile.ResultSuccess, nil
 }
 
 // reconcileSource ensures the upstream Helm repository can be reached and downloaded out using the declared
@@ -254,11 +282,9 @@ func (r *HelmRepositoryReconciler) reconcileStorage(ctx context.Context, obj *so
 //
 // The Helm repository index is downloaded using the defined configuration, and in case of an error during this process
 // (including transient errors), it records v1beta1.FetchFailedCondition=True and returns early.
-// On a successful write of a new artifact, the artifact in the status of the given object is set, and the symlink in
+// If the download is successful, the given artifact pointer is set to a new artifact with the available metadata, and
-// the storage is updated to its path.
+// the index pointer is set to the newly downloaded index.
-//
+func (r *HelmRepositoryReconciler) reconcileSource(ctx context.Context, obj *sourcev1.HelmRepository, artifact *sourcev1.Artifact, chartRepo *repository.ChartRepository) (sreconcile.Result, error) {
-// The caller should assume a failure if an error is returned, or the Result is zero.
-func (r *HelmRepositoryReconciler) reconcileSource(ctx context.Context, obj *sourcev1.HelmRepository, artifact *sourcev1.Artifact, chartRepo *repository.ChartRepository) (ctrl.Result, error) {
 	// Configure Helm client to access repository
 	clientOpts := []helmgetter.Option{
 		helmgetter.WithTimeout(obj.Spec.Timeout.Duration),
@@ -275,34 +301,34 @@ func (r *HelmRepositoryReconciler) reconcileSource(ctx context.Context, obj *sou
 		}
 		var secret corev1.Secret
 		if err := r.Client.Get(ctx, name, &secret); err != nil {
-			conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason,
+			e := &serror.Event{
-				"Failed to get secret '%s': %s", name.String(), err.Error())
+				Err:    fmt.Errorf("failed to get secret '%s': %w", name.String(), err),
-			r.Eventf(obj, corev1.EventTypeWarning, sourcev1.AuthenticationFailedReason,
+				Reason: sourcev1.AuthenticationFailedReason,
-				"Failed to get secret '%s': %s", name.String(), err.Error())
+			}
-			// Return error as the world as observed may change
+			conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason, e.Err.Error())
-			return ctrl.Result{}, err
+			return sreconcile.ResultEmpty, e
 		}
 
 		// Get client options from secret
 		tmpDir, err := os.MkdirTemp("", fmt.Sprintf("%s-%s-auth-", obj.Name, obj.Namespace))
 		if err != nil {
-			conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, sourcev1.StorageOperationFailedReason,
+			return sreconcile.ResultEmpty, &serror.Event{
-				"Failed to create temporary directory for credentials: %s", err.Error())
+				Err:    fmt.Errorf("failed to create temporary directory for credentials: %w", err),
-			r.Eventf(obj, corev1.EventTypeWarning, sourcev1.StorageOperationFailedReason,
+				Reason: sourcev1.StorageOperationFailedReason,
-				"Failed to create temporary directory for credentials: %s", err.Error())
+			}
-			return ctrl.Result{}, err
 		}
 		defer os.RemoveAll(tmpDir)
 
 		// Construct actual options
 		opts, err := getter.ClientOptionsFromSecret(tmpDir, secret)
 		if err != nil {
-			conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason,
+			e := &serror.Event{
-				"Failed to configure Helm client with secret data: %s", err)
+				Err:    fmt.Errorf("failed to configure Helm client with secret data: %w", err),
-			r.Eventf(obj, corev1.EventTypeWarning, sourcev1.AuthenticationFailedReason,
+				Reason: sourcev1.AuthenticationFailedReason,
-				"Failed to configure Helm client with secret data: %s", err)
+			}
-			// Return err as the content of the secret may change
+			conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason, e.Err.Error())
-			return ctrl.Result{}, err
+			// Return err as the content of the secret may change.
+			return sreconcile.ResultEmpty, e
 		}
 		clientOpts = append(clientOpts, opts...)
 	}
@@ -312,42 +338,49 @@ func (r *HelmRepositoryReconciler) reconcileSource(ctx context.Context, obj *sou
 	if err != nil {
 		switch err.(type) {
 		case *url.Error:
-			ctrl.LoggerFrom(ctx).Error(err, "invalid Helm repository URL")
+			e := &serror.Stalling{
-			conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, sourcev1.URLInvalidReason,
+				Err:    fmt.Errorf("invalid Helm repository URL: %w", err),
-				"Invalid Helm repository URL: %s", err.Error())
+				Reason: sourcev1.URLInvalidReason,
-			return ctrl.Result{}, nil
+			}
+			conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, sourcev1.URLInvalidReason, e.Err.Error())
+			return sreconcile.ResultEmpty, e
 		default:
-			ctrl.LoggerFrom(ctx).Error(err, "failed to construct Helm client")
+			e := &serror.Stalling{
-			conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, meta.FailedReason,
+				Err:    fmt.Errorf("failed to construct Helm client: %w", err),
-				"Failed to construct Helm client: %s", err.Error())
+				Reason: meta.FailedReason,
-			return ctrl.Result{}, nil
+			}
+			conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, meta.FailedReason, e.Err.Error())
+			return sreconcile.ResultEmpty, e
 		}
 	}
 	checksum, err := newChartRepo.CacheIndex()
 	if err != nil {
-		conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, meta.FailedReason,
+		e := &serror.Event{
-			"Failed to download Helm repository index: %s", err.Error())
+			Err:    fmt.Errorf("failed to download Helm repository index: %w", err),
-		r.Eventf(obj, corev1.EventTypeWarning, sourcev1.FetchFailedCondition,
+			Reason: meta.FailedReason,
-			"Failed to download Helm repository index: %s", err.Error())
+		}
+		conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, meta.FailedReason, e.Err.Error())
 		// Coin flip on transient or persistent error, return error and hope for the best
-		return ctrl.Result{}, err
+		return sreconcile.ResultEmpty, e
 	}
 	*chartRepo = *newChartRepo
 
 	// Load the cached repository index to ensure it passes validation.
 	if err := chartRepo.LoadFromCache(); err != nil {
-		conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, sourcev1.IndexationFailedReason,
+		e := &serror.Event{
-			"Failed to load Helm repository from cache: %s", err.Error())
+			Err:    fmt.Errorf("failed to load Helm repository from cache: %w", err),
-		r.Eventf(obj, corev1.EventTypeWarning, sourcev1.FetchFailedCondition,
+			Reason: sourcev1.FetchFailedCondition,
-			"Failed to load Helm repository from cache: %s", err.Error())
+		}
-		return ctrl.Result{}, err
+		conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, sourcev1.IndexationFailedReason, e.Err.Error())
+		return sreconcile.ResultEmpty, e
 	}
 	defer chartRepo.Unload()
 
 	// Mark observations about the revision on the object.
 	if !obj.GetArtifact().HasRevision(checksum) {
-		conditions.MarkTrue(obj, sourcev1.ArtifactOutdatedCondition, "NewRevision",
+		message := fmt.Sprintf("new index revision '%s'", checksum)
-			"New index revision '%s'", checksum)
+		conditions.MarkTrue(obj, sourcev1.ArtifactOutdatedCondition, "NewRevision", message)
+		conditions.MarkReconciling(obj, "NewRevision", message)
 	}
 
 	conditions.Delete(obj, sourcev1.FetchFailedCondition)
@@ -358,81 +391,95 @@ func (r *HelmRepositoryReconciler) reconcileSource(ctx context.Context, obj *sou
 		chartRepo.Checksum,
 		fmt.Sprintf("index-%s.yaml", checksum))
 
-	return ctrl.Result{RequeueAfter: obj.GetRequeueAfter()}, nil
+	return sreconcile.ResultSuccess, nil
 }
 
-func (r *HelmRepositoryReconciler) reconcileArtifact(ctx context.Context, obj *sourcev1.HelmRepository, artifact sourcev1.Artifact, chartRepo *repository.ChartRepository) (ctrl.Result, error) {
+// reconcileArtifact stores a new artifact in the storage, if the current observation on the object does not match the
+// given data.
+//
+// The inspection of the given data to the object is differed, ensuring any stale observations as
+// v1beta1.ArtifactUnavailableCondition and v1beta1.ArtifactOutdatedCondition are always deleted.
+// If the given artifact does not differ from the object's current, it returns early.
+// On a successful write of a new artifact, the artifact in the status of the given object is set, and the symlink in
+// the storage is updated to its path.
+func (r *HelmRepositoryReconciler) reconcileArtifact(ctx context.Context, obj *sourcev1.HelmRepository, artifact *sourcev1.Artifact, chartRepo *repository.ChartRepository) (sreconcile.Result, error) {
 	// Always restore the Ready condition in case it got removed due to a transient error.
 	defer func() {
-		if obj.GetArtifact() != nil {
-			conditions.Delete(obj, sourcev1.ArtifactUnavailableCondition)
-		}
 		if obj.GetArtifact().HasRevision(artifact.Revision) {
 			conditions.Delete(obj, sourcev1.ArtifactOutdatedCondition)
 			conditions.MarkTrue(obj, meta.ReadyCondition, meta.SucceededReason,
-				"Stored artifact for revision '%s'", artifact.Revision)
+				"stored artifact for revision '%s'", artifact.Revision)
 		}
 	}()
 
 	if obj.GetArtifact().HasRevision(artifact.Revision) {
-		ctrl.LoggerFrom(ctx).Info(fmt.Sprintf("Already up to date, current revision '%s'", artifact.Revision))
+		r.eventLogf(ctx, obj, corev1.EventTypeNormal, meta.SucceededReason, "already up to date, current revision '%s'", artifact.Revision)
-		return ctrl.Result{RequeueAfter: obj.GetRequeueAfter()}, nil
+		return sreconcile.ResultSuccess, nil
 	}
 
+	// Mark reconciling because the artifact and remote source are different.
+	// and they have to be reconciled.
+	conditions.MarkReconciling(obj, "NewRevision", "new index revision '%s'", artifact.Revision)
+
 	// Clear cache at the very end.
 	defer chartRepo.RemoveCache()
 
 	// Create artifact dir.
-	if err := r.Storage.MkdirAll(artifact); err != nil {
+	if err := r.Storage.MkdirAll(*artifact); err != nil {
-		ctrl.LoggerFrom(ctx).Error(err, "failed to create artifact directory")
+		return sreconcile.ResultEmpty, &serror.Event{
-		return ctrl.Result{}, err
+			Err:    fmt.Errorf("failed to create artifact directory: %w", err),
+			Reason: sourcev1.StorageOperationFailedReason,
+		}
 	}
 
 	// Acquire lock.
-	unlock, err := r.Storage.Lock(artifact)
+	unlock, err := r.Storage.Lock(*artifact)
 	if err != nil {
-		ctrl.LoggerFrom(ctx).Error(err, "failed to acquire lock for artifact")
+		return sreconcile.ResultEmpty, &serror.Event{
-		return ctrl.Result{}, err
+			Err:    fmt.Errorf("failed to acquire lock for artifact: %w", err),
+			Reason: meta.FailedReason,
+		}
 	}
 	defer unlock()
 
 	// Save artifact to storage.
-	if err = r.Storage.CopyFromPath(&artifact, chartRepo.CachePath); err != nil {
+	if err = r.Storage.CopyFromPath(artifact, chartRepo.CachePath); err != nil {
-		r.Eventf(obj, corev1.EventTypeWarning, sourcev1.StorageOperationFailedReason,
+		return sreconcile.ResultEmpty, &serror.Event{
-			"Unable to save artifact to storage: %s", err)
+			Err:    fmt.Errorf("unable to save artifact to storage: %w", err),
-		return ctrl.Result{}, err
+			Reason: sourcev1.StorageOperationFailedReason,
+		}
 	}
 
 	// Record it on the object.
 	obj.Status.Artifact = artifact.DeepCopy()
 
 	// Update index symlink.
-	indexURL, err := r.Storage.Symlink(artifact, "index.yaml")
+	indexURL, err := r.Storage.Symlink(*artifact, "index.yaml")
 	if err != nil {
-		r.Eventf(obj, corev1.EventTypeWarning, sourcev1.StorageOperationFailedReason,
+		r.eventLogf(ctx, obj, corev1.EventTypeWarning, sourcev1.StorageOperationFailedReason,
-			"Failed to update status URL symlink: %s", err)
+			"failed to update status URL symlink: %s", err)
 	}
 
 	if indexURL != "" {
 		obj.Status.URL = indexURL
 	}
-	return ctrl.Result{RequeueAfter: obj.GetRequeueAfter()}, nil
+	return sreconcile.ResultSuccess, nil
 }
 
 // reconcileDelete handles the delete of an object. It first garbage collects all artifacts for the object from the
 // artifact storage, if successful, the finalizer is removed from the object.
-func (r *HelmRepositoryReconciler) reconcileDelete(ctx context.Context, obj *sourcev1.HelmRepository) (ctrl.Result, error) {
+func (r *HelmRepositoryReconciler) reconcileDelete(ctx context.Context, obj *sourcev1.HelmRepository) (sreconcile.Result, error) {
 	// Garbage collect the resource's artifacts
 	if err := r.garbageCollect(ctx, obj); err != nil {
 		// Return the error so we retry the failed garbage collection
-		return ctrl.Result{}, err
+		return sreconcile.ResultEmpty, err
 	}
 
 	// Remove our finalizer from the list
 	controllerutil.RemoveFinalizer(obj, sourcev1.SourceFinalizer)
 
 	// Stop reconciliation as the object is being deleted
-	return ctrl.Result{}, nil
+	return sreconcile.ResultEmpty, nil
 }
 
 // garbageCollect performs a garbage collection for the given v1beta1.HelmRepository. It removes all but the current
@@ -441,25 +488,41 @@ func (r *HelmRepositoryReconciler) reconcileDelete(ctx context.Context, obj *sou
 func (r *HelmRepositoryReconciler) garbageCollect(ctx context.Context, obj *sourcev1.HelmRepository) error {
 	if !obj.DeletionTimestamp.IsZero() {
 		if err := r.Storage.RemoveAll(r.Storage.NewArtifactFor(obj.Kind, obj.GetObjectMeta(), "", "*")); err != nil {
-			r.Eventf(obj, corev1.EventTypeWarning, "GarbageCollectionFailed",
+			return &serror.Event{
-				"Garbage collection for deleted resource failed: %s", err)
+				Err:    fmt.Errorf("garbage collection for deleted resource failed: %w", err),
-			return err
+				Reason: "GarbageCollectionFailed",
+			}
 		}
 		obj.Status.Artifact = nil
 		// TODO(hidde): we should only push this event if we actually garbage collected something
-		r.Eventf(obj, corev1.EventTypeNormal, "GarbageCollectionSucceeded",
+		r.eventLogf(ctx, obj, corev1.EventTypeNormal, "GarbageCollectionSucceeded",
-			"Garbage collected artifacts for deleted resource")
+			"garbage collected artifacts for deleted resource")
 		return nil
 	}
 	if obj.GetArtifact() != nil {
 		if err := r.Storage.RemoveAllButCurrent(*obj.GetArtifact()); err != nil {
-			r.Eventf(obj, corev1.EventTypeWarning, "GarbageCollectionFailed",
+			return &serror.Event{
-				"Garbage collection of old artifacts failed: %s", err)
+				Err:    fmt.Errorf("garbage collection of old artifacts failed: %w", err),
-			return err
+				Reason: "GarbageCollectionFailed",
+			}
 		}
 		// TODO(hidde): we should only push this event if we actually garbage collected something
-		r.Eventf(obj, corev1.EventTypeNormal, "GarbageCollectionSucceeded",
+		r.eventLogf(ctx, obj, corev1.EventTypeNormal, "GarbageCollectionSucceeded",
-			"Garbage collected old artifacts")
+			"garbage collected old artifacts")
 	}
 	return nil
 }
+
+// eventLog records event and logs at the same time. This log is different from
+// the debug log in the event recorder in the sense that this is a simple log,
+// the event recorder debug log contains complete details about the event.
+func (r *HelmRepositoryReconciler) eventLogf(ctx context.Context, obj runtime.Object, eventType string, reason string, messageFmt string, args ...interface{}) {
+	msg := fmt.Sprintf(messageFmt, args...)
+	// Log and emit event.
+	if eventType == corev1.EventTypeWarning {
+		ctrl.LoggerFrom(ctx).Error(errors.New(reason), msg)
+	} else {
+		ctrl.LoggerFrom(ctx).Info(msg)
+	}
+	r.Eventf(obj, eventType, reason, msg)
+}

controllers/helmrepository_controller_test.go

@@ -24,7 +24,9 @@ import (
 	"path/filepath"
 	"strings"
 	"testing"
+	"time"
 
+	"github.com/darkowlzz/controller-check/status"
 	"github.com/go-logr/logr"
 	. "github.com/onsi/gomega"
 	"helm.sh/helm/v3/pkg/getter"
@@ -32,7 +34,6 @@ import (
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/client-go/tools/record"
-	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	fakeclient "sigs.k8s.io/controller-runtime/pkg/client/fake"
 	"sigs.k8s.io/controller-runtime/pkg/log"
@@ -40,9 +41,12 @@ import (
 	"github.com/fluxcd/pkg/apis/meta"
 	"github.com/fluxcd/pkg/helmtestserver"
 	"github.com/fluxcd/pkg/runtime/conditions"
+	"github.com/fluxcd/pkg/runtime/patch"
 
 	sourcev1 "github.com/fluxcd/source-controller/api/v1beta2"
+	serror "github.com/fluxcd/source-controller/internal/error"
 	"github.com/fluxcd/source-controller/internal/helm/repository"
+	sreconcile "github.com/fluxcd/source-controller/internal/reconcile"
 )
 
 var (
@@ -99,9 +103,15 @@ func TestHelmRepositoryReconciler_Reconcile(t *testing.T) {
 		}
 		readyCondition := conditions.Get(obj, meta.ReadyCondition)
 		return readyCondition.Status == metav1.ConditionTrue &&
-			obj.Generation == readyCondition.ObservedGeneration
+			obj.Generation == readyCondition.ObservedGeneration &&
+			obj.Generation == obj.Status.ObservedGeneration
 	}, timeout).Should(BeTrue())
 
+	// Check if the object status is valid.
+	condns := &status.Conditions{NegativePolarity: helmRepoReadyDepsNegative}
+	checker := status.NewChecker(testEnv.Client, testEnv.GetScheme(), condns)
+	checker.CheckErr(ctx, obj)
+
 	g.Expect(testEnv.Delete(ctx, obj)).To(Succeed())
 
 	// Wait for HelmRepository to be deleted
@@ -117,7 +127,7 @@ func TestHelmRepositoryReconciler_reconcileStorage(t *testing.T) {
 	tests := []struct {
 		name             string
 		beforeFunc       func(obj *sourcev1.HelmRepository, storage *Storage) error
-		want             ctrl.Result
+		want             sreconcile.Result
 		wantErr          bool
 		assertArtifact   *sourcev1.Artifact
 		assertConditions []metav1.Condition
@@ -154,6 +164,7 @@ func TestHelmRepositoryReconciler_reconcileStorage(t *testing.T) {
 				"!/reconcile-storage/b.txt",
 				"!/reconcile-storage/a.txt",
 			},
+			want: sreconcile.ResultSuccess,
 		},
 		{
 			name: "notices missing artifact in storage",
@@ -165,12 +176,12 @@ func TestHelmRepositoryReconciler_reconcileStorage(t *testing.T) {
 				testStorage.SetArtifactURL(obj.Status.Artifact)
 				return nil
 			},
-			want: ctrl.Result{Requeue: true},
+			want: sreconcile.ResultSuccess,
 			assertPaths: []string{
 				"!/reconcile-storage/invalid.txt",
 			},
 			assertConditions: []metav1.Condition{
-				*conditions.TrueCondition(sourcev1.ArtifactUnavailableCondition, "NoArtifact", "No artifact for resource in storage"),
+				*conditions.TrueCondition(meta.ReconcilingCondition, "NoArtifact", "no artifact for resource in storage"),
 			},
 		},
 		{
@@ -190,6 +201,7 @@ func TestHelmRepositoryReconciler_reconcileStorage(t *testing.T) {
 				}
 				return nil
 			},
+			want: sreconcile.ResultSuccess,
 			assertPaths: []string{
 				"/reconcile-storage/hostname.txt",
 			},
@@ -219,7 +231,10 @@ func TestHelmRepositoryReconciler_reconcileStorage(t *testing.T) {
 				g.Expect(tt.beforeFunc(obj, testStorage)).To(Succeed())
 			}
 
-			got, err := r.reconcileStorage(context.TODO(), obj)
+			var chartRepo repository.ChartRepository
+			var artifact sourcev1.Artifact
+
+			got, err := r.reconcileStorage(context.TODO(), obj, &artifact, &chartRepo)
 			g.Expect(err != nil).To(Equal(tt.wantErr))
 			g.Expect(got).To(Equal(tt.want))
 
@@ -257,16 +272,17 @@ func TestHelmRepositoryReconciler_reconcileSource(t *testing.T) {
 		secret           *corev1.Secret
 		beforeFunc       func(t *WithT, obj *sourcev1.HelmRepository)
 		afterFunc        func(t *WithT, obj *sourcev1.HelmRepository)
-		want             ctrl.Result
+		want             sreconcile.Result
 		wantErr          bool
 		assertConditions []metav1.Condition
 	}{
 		{
 			name:     "HTTP without secretRef makes ArtifactOutdated=True",
 			protocol: "http",
-			want:     ctrl.Result{RequeueAfter: interval},
+			want:     sreconcile.ResultSuccess,
 			assertConditions: []metav1.Condition{
-				*conditions.TrueCondition(sourcev1.ArtifactOutdatedCondition, "NewRevision", "New index revision"),
+				*conditions.TrueCondition(sourcev1.ArtifactOutdatedCondition, "NewRevision", "new index revision"),
+				*conditions.TrueCondition(meta.ReconcilingCondition, "NewRevision", "new index revision"),
 			},
 		},
 		{
@@ -288,9 +304,10 @@ func TestHelmRepositoryReconciler_reconcileSource(t *testing.T) {
 			beforeFunc: func(t *WithT, obj *sourcev1.HelmRepository) {
 				obj.Spec.SecretRef = &meta.LocalObjectReference{Name: "basic-auth"}
 			},
-			want: ctrl.Result{RequeueAfter: interval},
+			want: sreconcile.ResultSuccess,
 			assertConditions: []metav1.Condition{
-				*conditions.TrueCondition(sourcev1.ArtifactOutdatedCondition, "NewRevision", "New index revision"),
+				*conditions.TrueCondition(sourcev1.ArtifactOutdatedCondition, "NewRevision", "new index revision"),
+				*conditions.TrueCondition(meta.ReconcilingCondition, "NewRevision", "new index revision"),
 			},
 		},
 		{
@@ -312,9 +329,10 @@ func TestHelmRepositoryReconciler_reconcileSource(t *testing.T) {
 			beforeFunc: func(t *WithT, obj *sourcev1.HelmRepository) {
 				obj.Spec.SecretRef = &meta.LocalObjectReference{Name: "ca-file"}
 			},
-			want: ctrl.Result{RequeueAfter: interval},
+			want: sreconcile.ResultSuccess,
 			assertConditions: []metav1.Condition{
-				*conditions.TrueCondition(sourcev1.ArtifactOutdatedCondition, "NewRevision", "New index revision"),
+				*conditions.TrueCondition(sourcev1.ArtifactOutdatedCondition, "NewRevision", "new index revision"),
+				*conditions.TrueCondition(meta.ReconcilingCondition, "NewRevision", "new index revision"),
 			},
 		},
 		{
@@ -342,25 +360,25 @@ func TestHelmRepositoryReconciler_reconcileSource(t *testing.T) {
 			},
 		},
 		{
-			name:     "Invalid URL makes FetchFailed=True and returns zero Result",
+			name:     "Invalid URL makes FetchFailed=True and returns stalling error",
 			protocol: "http",
 			beforeFunc: func(t *WithT, obj *sourcev1.HelmRepository) {
 				obj.Spec.URL = strings.ReplaceAll(obj.Spec.URL, "http://", "")
 			},
-			want:    ctrl.Result{},
+			want:    sreconcile.ResultEmpty,
-			wantErr: false,
+			wantErr: true,
 			assertConditions: []metav1.Condition{
 				*conditions.TrueCondition(sourcev1.FetchFailedCondition, sourcev1.URLInvalidReason, "first path segment in URL cannot contain colon"),
 			},
 		},
 		{
-			name:     "Unsupported scheme makes FetchFailed=True and returns zero Result",
+			name:     "Unsupported scheme makes FetchFailed=True and returns stalling error",
 			protocol: "http",
 			beforeFunc: func(t *WithT, obj *sourcev1.HelmRepository) {
 				obj.Spec.URL = strings.ReplaceAll(obj.Spec.URL, "http://", "ftp://")
 			},
-			want:    ctrl.Result{},
+			want:    sreconcile.ResultEmpty,
-			wantErr: false,
+			wantErr: true,
 			assertConditions: []metav1.Condition{
 				*conditions.TrueCondition(sourcev1.FetchFailedCondition, meta.FailedReason, "scheme \"ftp\" not supported"),
 			},
@@ -482,7 +500,7 @@ func TestHelmRepositoryReconciler_reconcileArtifact(t *testing.T) {
 		name             string
 		beforeFunc       func(t *WithT, obj *sourcev1.HelmRepository, artifact sourcev1.Artifact, index *repository.ChartRepository)
 		afterFunc        func(t *WithT, obj *sourcev1.HelmRepository)
-		want             ctrl.Result
+		want             sreconcile.Result
 		wantErr          bool
 		assertConditions []metav1.Condition
 	}{
@@ -491,9 +509,10 @@ func TestHelmRepositoryReconciler_reconcileArtifact(t *testing.T) {
 			beforeFunc: func(t *WithT, obj *sourcev1.HelmRepository, artifact sourcev1.Artifact, index *repository.ChartRepository) {
 				obj.Spec.Interval = metav1.Duration{Duration: interval}
 			},
-			want: ctrl.Result{RequeueAfter: interval},
+			want: sreconcile.ResultSuccess,
 			assertConditions: []metav1.Condition{
-				*conditions.TrueCondition(meta.ReadyCondition, meta.SucceededReason, "Stored artifact for revision 'existing'"),
+				*conditions.TrueCondition(meta.ReadyCondition, meta.SucceededReason, "stored artifact for revision 'existing'"),
+				*conditions.TrueCondition(meta.ReconcilingCondition, "NewRevision", "new index revision 'existing'"),
 			},
 		},
 		{
@@ -505,20 +524,9 @@ func TestHelmRepositoryReconciler_reconcileArtifact(t *testing.T) {
 			afterFunc: func(t *WithT, obj *sourcev1.HelmRepository) {
 				t.Expect(obj.Status.URL).To(BeEmpty())
 			},
-			want: ctrl.Result{RequeueAfter: interval},
+			want: sreconcile.ResultSuccess,
 			assertConditions: []metav1.Condition{
-				*conditions.TrueCondition(meta.ReadyCondition, meta.SucceededReason, "Stored artifact for revision 'existing'"),
+				*conditions.TrueCondition(meta.ReadyCondition, meta.SucceededReason, "stored artifact for revision 'existing'"),
-			},
-		},
-		{
-			name: "Removes ArtifactUnavailableCondition after creating artifact",
-			beforeFunc: func(t *WithT, obj *sourcev1.HelmRepository, artifact sourcev1.Artifact, index *repository.ChartRepository) {
-				obj.Spec.Interval = metav1.Duration{Duration: interval}
-				conditions.MarkTrue(obj, sourcev1.ArtifactUnavailableCondition, "Foo", "")
-			},
-			want: ctrl.Result{RequeueAfter: interval},
-			assertConditions: []metav1.Condition{
-				*conditions.TrueCondition(meta.ReadyCondition, meta.SucceededReason, "Stored artifact for revision 'existing'"),
 			},
 		},
 		{
@@ -527,9 +535,10 @@ func TestHelmRepositoryReconciler_reconcileArtifact(t *testing.T) {
 				obj.Spec.Interval = metav1.Duration{Duration: interval}
 				conditions.MarkTrue(obj, sourcev1.ArtifactOutdatedCondition, "Foo", "")
 			},
-			want: ctrl.Result{RequeueAfter: interval},
+			want: sreconcile.ResultSuccess,
 			assertConditions: []metav1.Condition{
-				*conditions.TrueCondition(meta.ReadyCondition, meta.SucceededReason, "Stored artifact for revision 'existing'"),
+				*conditions.TrueCondition(meta.ReadyCondition, meta.SucceededReason, "stored artifact for revision 'existing'"),
+				*conditions.TrueCondition(meta.ReconcilingCondition, "NewRevision", "new index revision 'existing'"),
 			},
 		},
 		{
@@ -544,9 +553,10 @@ func TestHelmRepositoryReconciler_reconcileArtifact(t *testing.T) {
 				t.Expect(err).NotTo(HaveOccurred())
 				t.Expect(localPath).To(Equal(targetFile))
 			},
-			want: ctrl.Result{RequeueAfter: interval},
+			want: sreconcile.ResultSuccess,
 			assertConditions: []metav1.Condition{
-				*conditions.TrueCondition(meta.ReadyCondition, meta.SucceededReason, "Stored artifact for revision 'existing'"),
+				*conditions.TrueCondition(meta.ReadyCondition, meta.SucceededReason, "stored artifact for revision 'existing'"),
+				*conditions.TrueCondition(meta.ReconcilingCondition, "NewRevision", "new index revision 'existing'"),
 			},
 		},
 	}
@@ -598,7 +608,7 @@ func TestHelmRepositoryReconciler_reconcileArtifact(t *testing.T) {
 			}
 			dlog := log.NewDelegatingLogSink(log.NullLogSink{})
 			nullLogger := logr.New(dlog)
-			got, err := r.reconcileArtifact(logr.NewContext(ctx, nullLogger), obj, artifact, chartRepo)
+			got, err := r.reconcileArtifact(logr.NewContext(ctx, nullLogger), obj, &artifact, chartRepo)
 			g.Expect(err != nil).To(Equal(tt.wantErr))
 			g.Expect(got).To(Equal(tt.want))
 
@@ -615,3 +625,290 @@ func TestHelmRepositoryReconciler_reconcileArtifact(t *testing.T) {
 		})
 	}
 }
+
+func TestHelmRepositoryReconciler_summarizeAndPatch(t *testing.T) {
+	tests := []struct {
+		name             string
+		generation       int64
+		beforeFunc       func(obj *sourcev1.HelmRepository)
+		result           sreconcile.Result
+		reconcileErr     error
+		wantErr          bool
+		afterFunc        func(t *WithT, obj *sourcev1.HelmRepository)
+		assertConditions []metav1.Condition
+	}{
+		// Success/Fail indicates if a reconciliation succeeded or failed. On
+		// a successful reconciliation, the object generation is expected to
+		// match the observed generation in the object status.
+		// All the cases have some Ready condition set, even if a test case is
+		// unrelated to the conditions, because it's neseccary for a valid
+		// status.
+		{
+			name:       "Success, no extra conditions",
+			generation: 4,
+			beforeFunc: func(obj *sourcev1.HelmRepository) {
+				conditions.MarkTrue(obj, meta.ReadyCondition, meta.SucceededReason, "test-msg")
+			},
+			assertConditions: []metav1.Condition{
+				*conditions.TrueCondition(meta.ReadyCondition, meta.SucceededReason, "test-msg"),
+			},
+			afterFunc: func(t *WithT, obj *sourcev1.HelmRepository) {
+				t.Expect(obj.Status.ObservedGeneration).To(Equal(int64(4)))
+			},
+		},
+		{
+			name:       "Success, Ready=True",
+			generation: 5,
+			beforeFunc: func(obj *sourcev1.HelmRepository) {
+				conditions.MarkTrue(obj, meta.ReadyCondition, meta.SucceededReason, "created")
+			},
+			assertConditions: []metav1.Condition{
+				*conditions.TrueCondition(meta.ReadyCondition, meta.SucceededReason, "created"),
+			},
+			afterFunc: func(t *WithT, obj *sourcev1.HelmRepository) {
+				t.Expect(obj.Status.ObservedGeneration).To(Equal(int64(5)))
+			},
+		},
+		{
+			name:       "Success, removes reconciling for successful result",
+			generation: 2,
+			beforeFunc: func(obj *sourcev1.HelmRepository) {
+				conditions.MarkReconciling(obj, "NewRevision", "new index version")
+				conditions.MarkTrue(obj, meta.ReadyCondition, meta.SucceededReason, "stored artifact")
+			},
+			result:  sreconcile.ResultSuccess,
+			wantErr: false,
+			assertConditions: []metav1.Condition{
+				*conditions.TrueCondition(meta.ReadyCondition, meta.SucceededReason, "stored artifact"),
+			},
+			afterFunc: func(t *WithT, obj *sourcev1.HelmRepository) {
+				t.Expect(obj.Status.ObservedGeneration).To(Equal(int64(2)))
+			},
+		},
+		{
+			name: "Success, record reconciliation request",
+			beforeFunc: func(obj *sourcev1.HelmRepository) {
+				annotations := map[string]string{
+					meta.ReconcileRequestAnnotation: "now",
+				}
+				obj.SetAnnotations(annotations)
+				conditions.MarkTrue(obj, meta.ReadyCondition, meta.SucceededReason, "test-msg")
+			},
+			generation: 3,
+			result:     sreconcile.ResultSuccess,
+			wantErr:    false,
+			assertConditions: []metav1.Condition{
+				*conditions.TrueCondition(meta.ReadyCondition, meta.SucceededReason, "test-msg"),
+			},
+			afterFunc: func(t *WithT, obj *sourcev1.HelmRepository) {
+				t.Expect(obj.Status.LastHandledReconcileAt).To(Equal("now"))
+				t.Expect(obj.Status.ObservedGeneration).To(Equal(int64(3)))
+			},
+		},
+		{
+			name:       "Fail, with multiple conditions ArtifactOutdated=True,Reconciling=True",
+			generation: 7,
+			beforeFunc: func(obj *sourcev1.HelmRepository) {
+				conditions.MarkTrue(obj, sourcev1.ArtifactOutdatedCondition, "NewRevision", "new index revision")
+				conditions.MarkReconciling(obj, "NewRevision", "new index revision")
+			},
+			reconcileErr: fmt.Errorf("failed to create dir"),
+			wantErr:      true,
+			assertConditions: []metav1.Condition{
+				*conditions.FalseCondition(meta.ReadyCondition, "NewRevision", "new index revision"),
+				*conditions.TrueCondition(sourcev1.ArtifactOutdatedCondition, "NewRevision", "new index revision"),
+				*conditions.TrueCondition(meta.ReconcilingCondition, "NewRevision", "new index revision"),
+			},
+			afterFunc: func(t *WithT, obj *sourcev1.HelmRepository) {
+				t.Expect(obj.Status.ObservedGeneration).ToNot(Equal(int64(7)))
+			},
+		},
+		{
+			name:       "Success, with subreconciler stalled error",
+			generation: 9,
+			beforeFunc: func(obj *sourcev1.HelmRepository) {
+				conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, sourcev1.FetchFailedCondition, "failed to construct helm client")
+			},
+			reconcileErr: &serror.Stalling{Err: fmt.Errorf("some error"), Reason: "some reason"},
+			wantErr:      false,
+			assertConditions: []metav1.Condition{
+				*conditions.FalseCondition(meta.ReadyCondition, sourcev1.FetchFailedCondition, "failed to construct helm client"),
+				*conditions.TrueCondition(meta.StalledCondition, "some reason", "some error"),
+				*conditions.TrueCondition(sourcev1.FetchFailedCondition, sourcev1.FetchFailedCondition, "failed to construct helm client"),
+			},
+			afterFunc: func(t *WithT, obj *sourcev1.HelmRepository) {
+				t.Expect(obj.Status.ObservedGeneration).To(Equal(int64(9)))
+			},
+		},
+		{
+			name:       "Fail, no error but requeue requested",
+			generation: 3,
+			beforeFunc: func(obj *sourcev1.HelmRepository) {
+				conditions.MarkFalse(obj, meta.ReadyCondition, meta.FailedReason, "test-msg")
+			},
+			result: sreconcile.ResultRequeue,
+			assertConditions: []metav1.Condition{
+				*conditions.FalseCondition(meta.ReadyCondition, meta.FailedReason, "test-msg"),
+			},
+			afterFunc: func(t *WithT, obj *sourcev1.HelmRepository) {
+				t.Expect(obj.Status.ObservedGeneration).ToNot(Equal(int64(3)))
+			},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			g := NewWithT(t)
+
+			builder := fakeclient.NewClientBuilder().WithScheme(testEnv.GetScheme())
+			r := &HelmRepositoryReconciler{
+				Client: builder.Build(),
+			}
+			obj := &sourcev1.HelmRepository{
+				ObjectMeta: metav1.ObjectMeta{
+					GenerateName: "test-",
+					Generation:   tt.generation,
+				},
+				Spec: sourcev1.HelmRepositorySpec{
+					Interval: metav1.Duration{Duration: 5 * time.Second},
+				},
+			}
+
+			if tt.beforeFunc != nil {
+				tt.beforeFunc(obj)
+			}
+
+			ctx := context.TODO()
+			g.Expect(r.Create(ctx, obj)).To(Succeed())
+			patchHelper, err := patch.NewHelper(obj, r.Client)
+			g.Expect(err).ToNot(HaveOccurred())
+
+			gotErr := r.summarizeAndPatch(ctx, obj, patchHelper, tt.result, tt.reconcileErr)
+			g.Expect(gotErr != nil).To(Equal(tt.wantErr))
+
+			g.Expect(obj.Status.Conditions).To(conditions.MatchConditions(tt.assertConditions))
+
+			if tt.afterFunc != nil {
+				tt.afterFunc(g, obj)
+			}
+
+			// Check if the object status is valid.
+			condns := &status.Conditions{NegativePolarity: helmRepoReadyDepsNegative}
+			checker := status.NewChecker(r.Client, testEnv.GetScheme(), condns)
+			checker.CheckErr(ctx, obj)
+		})
+	}
+}
+
+func TestHelmRepositoryReconciler_reconcileSubRecs(t *testing.T) {
+	// Helper to build simple helmRepoReconcilerFunc with result and error.
+	buildReconcileFuncs := func(r sreconcile.Result, e error) helmRepoReconcilerFunc {
+		return func(ctx context.Context, obj *sourcev1.HelmRepository, artifact *sourcev1.Artifact, repo *repository.ChartRepository) (sreconcile.Result, error) {
+			return r, e
+		}
+	}
+
+	tests := []struct {
+		name               string
+		generation         int64
+		observedGeneration int64
+		reconcileFuncs     []helmRepoReconcilerFunc
+		wantResult         sreconcile.Result
+		wantErr            bool
+		assertConditions   []metav1.Condition
+	}{
+		{
+			name: "successful reconciliations",
+			reconcileFuncs: []helmRepoReconcilerFunc{
+				buildReconcileFuncs(sreconcile.ResultSuccess, nil),
+			},
+			wantResult: sreconcile.ResultSuccess,
+			wantErr:    false,
+		},
+		{
+			name:               "successful reconciliation with generation difference",
+			generation:         3,
+			observedGeneration: 2,
+			reconcileFuncs: []helmRepoReconcilerFunc{
+				buildReconcileFuncs(sreconcile.ResultSuccess, nil),
+			},
+			wantResult: sreconcile.ResultSuccess,
+			wantErr:    false,
+			assertConditions: []metav1.Condition{
+				*conditions.TrueCondition(meta.ReconcilingCondition, "NewGeneration", "reconciling new generation 3"),
+			},
+		},
+		{
+			name: "failed reconciliation",
+			reconcileFuncs: []helmRepoReconcilerFunc{
+				buildReconcileFuncs(sreconcile.ResultEmpty, fmt.Errorf("some error")),
+			},
+			wantResult: sreconcile.ResultEmpty,
+			wantErr:    true,
+		},
+		{
+			name: "multiple object status conditions mutations",
+			reconcileFuncs: []helmRepoReconcilerFunc{
+				func(ctx context.Context, obj *sourcev1.HelmRepository, artifact *sourcev1.Artifact, repo *repository.ChartRepository) (sreconcile.Result, error) {
+					conditions.MarkTrue(obj, sourcev1.ArtifactOutdatedCondition, "NewRevision", "new index revision")
+					return sreconcile.ResultSuccess, nil
+				},
+				func(ctx context.Context, obj *sourcev1.HelmRepository, artifact *sourcev1.Artifact, repo *repository.ChartRepository) (sreconcile.Result, error) {
+					conditions.MarkTrue(obj, meta.ReconcilingCondition, "Progressing", "creating artifact")
+					return sreconcile.ResultSuccess, nil
+				},
+			},
+			wantResult: sreconcile.ResultSuccess,
+			wantErr:    false,
+			assertConditions: []metav1.Condition{
+				*conditions.TrueCondition(sourcev1.ArtifactOutdatedCondition, "NewRevision", "new index revision"),
+				*conditions.TrueCondition(meta.ReconcilingCondition, "Progressing", "creating artifact"),
+			},
+		},
+		{
+			name: "subrecs with one result=Requeue, no error",
+			reconcileFuncs: []helmRepoReconcilerFunc{
+				buildReconcileFuncs(sreconcile.ResultSuccess, nil),
+				buildReconcileFuncs(sreconcile.ResultRequeue, nil),
+				buildReconcileFuncs(sreconcile.ResultSuccess, nil),
+			},
+			wantResult: sreconcile.ResultRequeue,
+			wantErr:    false,
+		},
+		{
+			name: "subrecs with error before result=Requeue",
+			reconcileFuncs: []helmRepoReconcilerFunc{
+				buildReconcileFuncs(sreconcile.ResultSuccess, nil),
+				buildReconcileFuncs(sreconcile.ResultEmpty, fmt.Errorf("some error")),
+				buildReconcileFuncs(sreconcile.ResultRequeue, nil),
+			},
+			wantResult: sreconcile.ResultEmpty,
+			wantErr:    true,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			g := NewWithT(t)
+
+			r := &HelmRepositoryReconciler{}
+			obj := &sourcev1.HelmRepository{
+				ObjectMeta: metav1.ObjectMeta{
+					GenerateName: "test-",
+					Generation:   tt.generation,
+				},
+				Status: sourcev1.HelmRepositoryStatus{
+					ObservedGeneration: tt.observedGeneration,
+				},
+			}
+
+			ctx := context.TODO()
+
+			gotRes, gotErr := r.reconcile(ctx, obj, tt.reconcileFuncs)
+			g.Expect(gotErr != nil).To(Equal(tt.wantErr))
+			g.Expect(gotRes).To(Equal(tt.wantResult))
+
+			g.Expect(obj.Status.Conditions).To(conditions.MatchConditions(tt.assertConditions))
+		})
+	}
+}