review: batch-end quality round (#24)

* review: comment budget pass

Trim doc comments to one-line WHY across cocoonset and hibernation:
drop restated behavior and multi-line justification prose.

* review: declaration layout and modern idioms

Move TestSetupWithManagerRejectsInvalidConcurrency above the unexported
concurrencyProbe helper; use slices.Concat in place of manual append
chains in the logr sink.

* review: loc-justify cuts

Remove two dead-in-practice guards identified by the loc-justify audit:

- cocoonset/suspend.go: allOwnedPodsHibernated's nil-Registry early
  return. main.go's only construction site always supplies a non-nil
  Registry (buildRegistry fails fast on empty OCI_REGISTRY), so the
  guard never fires.
- cocoonset/delete.go: vmNamesForGC's Status fallback branch.
  stashDeleteVMNames always folds Status into the stashed annotation
  before any GC pass reads it, so the annotation is never empty when
  the fallback would matter.

* fix(cocoonset): reject numeric toolbox names colliding with agent slots

A toolbox named e.g. "1" produces the same pod name as sub-agent slot
1 ("<cs>-1"), so creating the sub-agent first silently no-ops on
AlreadyExists and the slot stays missing forever with no error.

* fix(cocoonset): keep hibernate annotation owned during reverse-desire window

applyUnsuspend only excluded pods with Desire=Hibernate, so a CR that
flips to Wake while still Hibernating (a state the hibernation
reconciler itself routes back into reconcileHibernate) lost its
exclusion and applyUnsuspend cleared the annotation mid-transition,
racing the hibernation reconciler's own re-assertion.

* fix(cocoonset): recreate a drifted main agent instead of sticking in Failed

lifecycle-state=Failed short-circuited before the spec-drift recreate
path, so a soft vk-cocoon failure that never sets the pod Ready again
parked the CocoonSet in Failed permanently with no escape short of a
manual pod delete, even after the operator fixed the spec.

* review: fit the new fix comments to the one-line budget

* fix(hibernation): let a failing registry probe still hit the hibernate deadline

HasHibernateSnapshot errors returned before the hibernateTimeout check,
so a registry outage pinned the phase at Hibernating forever; with
podsHibernatedByCR also matching that phase, a reverse-desire wake could
never reclaim the hibernate annotation. An expired deadline now converts
the probe error into the Failed phase; the success path stays ahead of
the deadline so a late-but-pushed snapshot still completes as Hibernated.

Author: CMGS

cocoonset/agents.go

@@ -21,17 +21,14 @@ import (
 	"github.com/cocoonstack/cocoon-operator/metrics"
 )
 
-// subAgentCreateConcurrency caps parallel pod creates during fan-out so a
-// large scale-up (e.g. 1→N) does not burst the apiserver. Empirically the
-// rate limiter in controller-runtime plus apiserver QPS accommodate 8 in
-// flight without priority-fairness throttling.
+// subAgentCreateConcurrency caps parallel pod creates during a batch scale-up
+// so it does not burst the apiserver.
 const subAgentCreateConcurrency = 8
 
 // ensureSubAgents creates/deletes sub-agent pods to match [1..Replicas].
 // Returns changed (true when cluster state was mutated) and requeueAfter
 // (non-zero when a sub-agent is in rebuild backoff and the caller should
-// re-reconcile when backoff elapses). Missing slots are created concurrently
-// so batch scale-ups do not serialize N apiserver round trips.
+// re-reconcile when backoff elapses).
 func (r *Reconciler) ensureSubAgents(ctx context.Context, cs *cocoonv1.CocoonSet, classified classifiedPods, mainVMName, mainNodeName string, intent restoreIntent) (bool, time.Duration, error) {
 	logger := log.WithFunc("cocoonset.Reconciler.ensureSubAgents")
 	changed := false

cocoonset/delete.go

@@ -116,15 +116,9 @@ func (r *Reconciler) stashDeleteVMNames(ctx context.Context, cs *cocoonv1.Cocoon
 	return r.Patch(ctx, cs, patch)
 }
 
-// vmNamesForGC returns the canonical GC list — read from the stashed annotation,
-// falling back to Status when the annotation is somehow missing.
+// vmNamesForGC returns the canonical GC list, read from the stashed annotation.
 func vmNamesForGC(cs *cocoonv1.CocoonSet) []string {
-	if names := parseVMNamesAnnotation(cs.Annotations[annotationDeleteVMNames]); len(names) > 0 {
-		return names
-	}
-	names := statusVMNames(cs)
-	slices.Sort(names)
-	return names
+	return parseVMNamesAnnotation(cs.Annotations[annotationDeleteVMNames])
 }
 
 // statusVMNames collects the non-empty VM names recorded in the CocoonSet

cocoonset/migrate.go

@@ -16,11 +16,10 @@ import (
 	"github.com/cocoonstack/cocoon-operator/snapshot"
 )
 
-// reconcileMigration drives cross-node migration of the main agent (slot 0):
-// quiesce -> snapshot -> recreate on the target with restore-from-hibernate ->
-// drop the snapshot; idempotent over durable state, handled=false hands back.
-// Never lose live state: the old pod dies only after the snapshot exists AND
-// this controller quiesced it; the snapshot drops only once the new VM runs.
+// reconcileMigration drives cross-node migration of the main agent (slot 0);
+// handled=false hands back to the normal flow. Never lose live state: the old
+// pod dies only after the snapshot exists AND this controller quiesced it;
+// the snapshot drops only once the new VM runs.
 func (r *Reconciler) reconcileMigration(ctx context.Context, cs *cocoonv1.CocoonSet, classified classifiedPods) (bool, ctrl.Result, error) {
 	desired := cs.Spec.NodeName
 	migrating := cs.Status.Phase == cocoonv1.CocoonSetPhaseMigrating

cocoonset/predicate.go

@@ -9,9 +9,8 @@ import (
 	"github.com/cocoonstack/cocoon-common/meta"
 )
 
-// podRelevantChange filters pod events to those that affect CocoonSet
-// reconciliation: creation, deletion, and readiness transitions.
-// Ignores pure status churn (VK notify loops, condition timestamp updates).
+// podRelevantChange ignores pure status churn (VK notify loops, condition
+// timestamp updates) that would otherwise storm reconciles.
 type podRelevantChange struct{}
 
 func (podRelevantChange) Create(_ event.CreateEvent) bool   { return true }

cocoonset/reconciler.go

@@ -43,9 +43,8 @@ type Reconciler struct {
 	Concurrency int
 }
 
-// SetupWithManager registers the reconciler. `For` uses GenerationChangedPredicate
-// to avoid status-update loops; Owns filters pod events to creation, deletion,
-// and readiness transitions to prevent reconcile storms from VK status churn.
+// SetupWithManager registers the reconciler with predicates that filter out
+// status-only churn to avoid reconcile storms.
 func (r *Reconciler) SetupWithManager(_ context.Context, mgr ctrl.Manager) error {
 	if r.Concurrency < 1 {
 		return fmt.Errorf("cocoonset concurrency must be at least 1, got %d", r.Concurrency)
@@ -97,9 +96,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
 	// Pod Phase flips); IsPodTerminal is the kubelet-driven one.
 	if classified.main != nil {
 		if reason := mainPodFailedReason(classified.main); reason != "" {
-			r.observeMainPodFailed(&cs, classified.main, reason)
-			return ctrl.Result{}, r.patchStatus(ctx, &cs,
-				buildStatus(&cs, classified, cocoonv1.CocoonSetPhaseFailed))
+			return r.handleFailedMainAgent(ctx, &cs, classified, reason)
 		}
 		if cs.Status.Phase == cocoonv1.CocoonSetPhaseFailed && meta.IsPodReady(classified.main) && r.Recorder != nil {
 			r.Recorder.Eventf(&cs, corev1.EventTypeNormal, "RecoveredFromFailure",
@@ -160,6 +157,18 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
 	return ctrl.Result{RequeueAfter: subRequeue}, nil
 }
 
+// handleFailedMainAgent recreates a terminal main agent whose spec has drifted; parking in Failed would wait for a Ready the drifted pod can never reach.
+func (r *Reconciler) handleFailedMainAgent(ctx context.Context, cs *cocoonv1.CocoonSet, classified classifiedPods, reason string) (ctrl.Result, error) {
+	if !podSpecMatchesAgent(classified.main, cs, 0) {
+		if err := r.Delete(ctx, classified.main); err != nil && !apierrors.IsNotFound(err) {
+			return ctrl.Result{}, fmt.Errorf("delete terminal drifted main agent: %w", err)
+		}
+		return ctrl.Result{Requeue: true}, nil
+	}
+	r.observeMainPodFailed(cs, classified.main, reason)
+	return ctrl.Result{}, r.patchStatus(ctx, cs, buildStatus(cs, classified, cocoonv1.CocoonSetPhaseFailed))
+}
+
 // createMainAgent builds and creates the missing main agent pod, stamping
 // restore-from-hibernate when the agent is hibernated so a cross-node recreate
 // restores from the :hibernate snapshot instead of booting fresh. It always

cocoonset/reconciler_test.go

@@ -9,6 +9,7 @@ import (
 	"time"
 
 	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/types"
 	ctrl "sigs.k8s.io/controller-runtime"
@@ -127,6 +128,28 @@ func TestEnsureToolboxesCollisionReturnsError(t *testing.T) {
 	}
 }
 
+func TestEnsureToolboxesRejectsIntegerName(t *testing.T) {
+	scheme := testScheme(t)
+	cs := newCocoonSet("demo", func(cs *cocoonv1.CocoonSet) {
+		cs.Spec.Toolboxes = []cocoonv1.ToolboxSpec{
+			{Name: "1", Image: "ghcr.io/cocoonstack/cocoon/toolbox:latest"},
+		}
+	})
+
+	cli := ctrlfake.NewClientBuilder().WithScheme(scheme).Build()
+	r := &Reconciler{Client: cli, Scheme: scheme}
+	classified := classifiedPods{
+		sub:       map[int32]*corev1.Pod{},
+		toolbox:   map[string]*corev1.Pod{},
+		allByName: map[string]*corev1.Pod{},
+	}
+
+	_, err := r.ensureToolboxes(t.Context(), cs, classified, r.newRestoreIntent(t.Context(), cs.Namespace))
+	if err == nil {
+		t.Fatal("ensureToolboxes must reject a toolbox name that collides with agent slot pod naming")
+	}
+}
+
 func TestEnsureToolboxesRejectsDuplicateNames(t *testing.T) {
 	scheme := testScheme(t)
 	cs := newCocoonSet("demo", func(cs *cocoonv1.CocoonSet) {
@@ -493,6 +516,39 @@ func TestReconcileMainLifecycleFailedTransitionsToFailed(t *testing.T) {
 	}
 }
 
+// A Failed main pod whose spec has drifted from the current CocoonSet spec
+// must be deleted for recreate, not parked in Failed forever.
+func TestReconcileMainLifecycleFailedWithDriftRecreatesPod(t *testing.T) {
+	scheme := testScheme(t)
+	cs := newCocoonSet("demo", func(cs *cocoonv1.CocoonSet) {
+		cs.Finalizers = []string{finalizerName}
+	})
+	mainPod := mustBuildAgentPod(t, cs, 0, "", "", scheme)
+	mainPod.Status.Phase = corev1.PodRunning
+	if mainPod.Annotations == nil {
+		mainPod.Annotations = map[string]string{}
+	}
+	mainPod.Annotations[meta.AnnotationLifecycleState] = string(meta.LifecycleStateFailed)
+
+	cs.Spec.Agent.Image = "ghcr.io/cocoonstack/cocoon/ubuntu:26.04"
+
+	cli := ctrlfake.NewClientBuilder().
+		WithScheme(scheme).
+		WithObjects(cs, mainPod).
+		WithStatusSubresource(&cocoonv1.CocoonSet{}).
+		Build()
+	r := &Reconciler{Client: cli, Scheme: scheme, Registry: &fakeRegistry{}}
+
+	if _, err := r.Reconcile(t.Context(), ctrl.Request{NamespacedName: types.NamespacedName{Namespace: cs.Namespace, Name: cs.Name}}); err != nil {
+		t.Fatalf("Reconcile: %v", err)
+	}
+	if err := cli.Get(t.Context(), types.NamespacedName{Namespace: mainPod.Namespace, Name: mainPod.Name}, &corev1.Pod{}); err == nil {
+		t.Error("Failed main pod with drifted spec should have been deleted for recreate")
+	} else if !apierrors.IsNotFound(err) {
+		t.Fatalf("get main pod: %v", err)
+	}
+}
+
 // A sub-agent carrying lifecycle-state=Failed but still PodPhase=Running must
 // be rebuilt so the backoff / dead-letter logic runs.
 func TestEnsureSubAgentsTreatsLifecycleFailedAsTerminal(t *testing.T) {
@@ -793,6 +849,53 @@ func TestApplyUnsuspendSkipsPodHibernatedByCR(t *testing.T) {
 	}
 }
 
+// A Wake desire mid-Hibernating transition (the hibernation reconciler's own
+// reverse-desire window) must still exclude the pod: the annotation is owned
+// by the in-flight hibernate, not by applyUnsuspend.
+func TestApplyUnsuspendSkipsPodMidHibernateOnReverseDesire(t *testing.T) {
+	scheme := testScheme(t)
+
+	hibernated := &corev1.Pod{
+		ObjectMeta: metav1.ObjectMeta{Name: "demo-0", Namespace: "ns"},
+	}
+	meta.HibernateState(true).Apply(hibernated)
+
+	hibCR := &cocoonv1.CocoonHibernation{
+		ObjectMeta: metav1.ObjectMeta{Name: "demo-hib", Namespace: "ns"},
+		Spec: cocoonv1.CocoonHibernationSpec{
+			Desire: cocoonv1.HibernationDesireWake,
+			PodRef: cocoonv1.HibernationPodRef{Name: "demo-0"},
+		},
+		Status: cocoonv1.CocoonHibernationStatus{
+			Phase: cocoonv1.CocoonHibernationPhaseHibernating,
+		},
+	}
+
+	cli := ctrlfake.NewClientBuilder().
+		WithScheme(scheme).
+		WithObjects(hibernated, hibCR).
+		Build()
+	r := &Reconciler{Client: cli, Scheme: scheme}
+	classified := classifiedPods{
+		main:      hibernated,
+		sub:       map[int32]*corev1.Pod{},
+		toolbox:   map[string]*corev1.Pod{},
+		allByName: map[string]*corev1.Pod{"demo-0": hibernated},
+	}
+
+	if err := r.applyUnsuspend(t.Context(), "ns", classified); err != nil {
+		t.Fatalf("applyUnsuspend: %v", err)
+	}
+
+	var got corev1.Pod
+	if err := cli.Get(t.Context(), types.NamespacedName{Namespace: "ns", Name: "demo-0"}, &got); err != nil {
+		t.Fatalf("get demo-0: %v", err)
+	}
+	if !bool(meta.ReadHibernateState(&got)) {
+		t.Errorf("demo-0 is mid-Hibernating under a reverse Wake desire; applyUnsuspend must leave it set")
+	}
+}
+
 // A nil manager suffices: the guard rejects before mgr is touched.
 func TestSetupWithManagerRejectsInvalidConcurrency(t *testing.T) {
 	for _, n := range []int{0, -1} {

cocoonset/suspend.go

@@ -17,10 +17,8 @@ import (
 	"github.com/cocoonstack/cocoon-operator/snapshot"
 )
 
-// reconcileSuspend ensures the main agent exists, applies the hibernate
-// annotation to every owned pod, then polls the registry to observe when all
-// managed VMs have been pushed to snapshot. Stays in Suspending with a
-// periodic requeue until every required snapshot lands.
+// reconcileSuspend polls the registry and stays in Suspending, requeueing
+// periodically, until every managed VM's snapshot lands.
 func (r *Reconciler) reconcileSuspend(ctx context.Context, cs *cocoonv1.CocoonSet, classified classifiedPods) (ctrl.Result, error) {
 	logger := log.WithFunc("cocoonset.Reconciler.reconcileSuspend")
 	if classified.main == nil {
@@ -60,11 +58,6 @@ func (r *Reconciler) reconcileSuspend(ctx context.Context, cs *cocoonv1.CocoonSe
 // Returns (false, nil) whenever the expected state is not yet observed so
 // the caller requeues rather than treats it as an error.
 func (r *Reconciler) allOwnedPodsHibernated(ctx context.Context, cs *cocoonv1.CocoonSet, classified classifiedPods) (bool, error) {
-	if r.Registry == nil {
-		// No registry configured; such deployments have no snapshot to
-		// observe, so treat the annotation write as authoritative.
-		return true, nil
-	}
 	for _, name := range slices.Sorted(maps.Keys(classified.allByName)) {
 		if ctxErr := ctx.Err(); ctxErr != nil {
 			return false, ctxErr
@@ -113,8 +106,7 @@ func (r *Reconciler) applySuspend(ctx context.Context, classified classifiedPods
 
 // applyUnsuspend clears HibernateState from owned pods, skipping pods that are
 // targets of an active CocoonHibernation CR to avoid racing the hibernation
-// reconciler. The unsorted pre-scan keeps the steady path (nothing hibernated)
-// zero-alloc: no key sort, no CR list.
+// reconciler.
 func (r *Reconciler) applyUnsuspend(ctx context.Context, namespace string, classified classifiedPods) error {
 	var hibernated []*corev1.Pod
 	for _, pod := range classified.allByName {
@@ -150,6 +142,6 @@ func (r *Reconciler) applyUnsuspend(ctx context.Context, namespace string, class
 // podsHibernatedByCR returns pod names targeted by a desire=Hibernate CR.
 func (r *Reconciler) podsHibernatedByCR(ctx context.Context, namespace string) (map[string]struct{}, error) {
 	return r.hibernationPodNames(ctx, namespace, func(h *cocoonv1.CocoonHibernation) bool {
-		return h.Spec.Desire == cocoonv1.HibernationDesireHibernate
+		return h.Spec.Desire == cocoonv1.HibernationDesireHibernate || h.Status.Phase == cocoonv1.CocoonHibernationPhaseHibernating
 	})
 }

cocoonset/toolboxes.go

@@ -5,6 +5,7 @@ import (
 	"fmt"
 	"maps"
 	"slices"
+	"strconv"
 
 	"github.com/projecteru2/core/log"
 	corev1 "k8s.io/api/core/v1"
@@ -25,6 +26,9 @@ func (r *Reconciler) ensureToolboxes(ctx context.Context, cs *cocoonv1.CocoonSet
 		if desired[tb.Name] {
 			return false, fmt.Errorf("duplicate toolbox name %q in spec", tb.Name)
 		}
+		if _, convErr := strconv.Atoi(tb.Name); convErr == nil {
+			return false, fmt.Errorf("toolbox name %q must not be an integer: collides with agent slot pod naming", tb.Name)
+		}
 		desired[tb.Name] = true
 	}
 	changed := false

hibernation/hibernate.go

@@ -28,10 +28,11 @@ func (r *Reconciler) reconcileHibernate(ctx context.Context, hib *cocoonv1.Cocoo
 	if st := meta.ReadLifecycleStatus(pod); st.State == meta.LifecycleStateHibernated &&
 		st.ObservedGeneration >= meta.ReadCocoonSetGeneration(pod) {
 		present, err := snapshot.HasHibernateSnapshot(ctx, r.Registry, vmName)
-		if err != nil {
+		// A persistently failing probe must still hit the deadline below, or the phase starves in Hibernating.
+		if err != nil && !phaseDeadlineExceeded(hib, cocoonv1.CocoonHibernationPhaseHibernating, hibernateTimeout) {
 			return ctrl.Result{}, err
 		}
-		if present {
+		if err == nil && present {
 			if r.firstTransitionAt(hib) {
 				observePhaseExit(hib, "ok")
 				r.emitEventf(hib, corev1.EventTypeNormal, "Hibernated", "snapshot %s pushed to the registry", vmName)
@@ -42,10 +43,10 @@ func (r *Reconciler) reconcileHibernate(ctx context.Context, hib *cocoonv1.Cocoo
 	if phaseDeadlineExceeded(hib, cocoonv1.CocoonHibernationPhaseHibernating, hibernateTimeout) {
 		if r.firstTransitionAt(hib) {
 			observePhaseExit(hib, "timeout")
-			r.emitEventf(hib, corev1.EventTypeWarning, "HibernateTimedOut", "vk-cocoon did not push snapshot %s within %s", vmName, hibernateTimeout)
+			r.emitEventf(hib, corev1.EventTypeWarning, "HibernateTimedOut", "snapshot %s not confirmed in the registry within %s", vmName, hibernateTimeout)
 		}
 		return ctrl.Result{}, r.markFailed(ctx, hib,
-			fmt.Sprintf("hibernate timed out after %s; vk-cocoon never pushed the snapshot", hibernateTimeout))
+			fmt.Sprintf("hibernate not confirmed within %s", hibernateTimeout))
 	}
 	if updateErr := r.setPhase(ctx, hib, cocoonv1.CocoonHibernationPhaseHibernating, vmName); updateErr != nil {
 		return ctrl.Result{}, updateErr