fix: re-read object in completePlan/failPlan to avoid stale resourceVersion (#82)

The plan executor patches status mid-reconcile (task/plan completion),
bumping resourceVersion. completePlan/failPlan then tried to patch
status using the statusBase from the start of Reconcile, which carried
the old resourceVersion. This caused guaranteed optimistic lock
conflicts, leaving deployments permanently stuck in Upgrading phase.

Fix: completePlan and failPlan now re-read the object via r.Get before
building their patch base, ensuring a fresh resourceVersion. The stale
statusBase parameter is removed from both functions and drivePlan.

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Author: bdchatham

internal/controller/nodedeployment/nodes.go

@@ -81,6 +81,10 @@ func (r *SeiNodeDeploymentReconciler) detectGenesisCeremonyNeeded(group *seiv1al
 // by comparing the current template hash against the stored hash. Only
 // fields that require new nodes (image, entrypoint, chainId) are hashed;
 // sidecar, overrides, and replica changes propagate in-place.
+// TODO: guard against empty incumbentNodes — if populateIncumbentNodes found
+// zero nodes (e.g. missing owner references), a rollout with empty node lists
+// creates a plan where all tasks complete as no-ops. Should return early here
+// when len(group.Status.IncumbentNodes) == 0.
 func (r *SeiNodeDeploymentReconciler) detectDeploymentNeeded(group *seiv1alpha1.SeiNodeDeployment) {
 	if group.Status.TemplateHash == "" {
 		return // first reconcile, no baseline to compare against

internal/controller/nodedeployment/plan.go

@@ -21,7 +21,7 @@ import (
 func (r *SeiNodeDeploymentReconciler) reconcilePlan(ctx context.Context, group *seiv1alpha1.SeiNodeDeployment, statusBase client.Patch) (ctrl.Result, error) {
 	// Drive active plan.
 	if group.Status.Plan != nil && group.Status.Plan.Phase == seiv1alpha1.TaskPlanActive {
-		return r.drivePlan(ctx, group, statusBase)
+		return r.drivePlan(ctx, group)
 	}
 
 	// No active plan — ask the planner if one is needed.
@@ -38,17 +38,17 @@ func (r *SeiNodeDeploymentReconciler) reconcilePlan(ctx context.Context, group *
 	return r.startPlan(ctx, group, statusBase, plan)
 }
 
-func (r *SeiNodeDeploymentReconciler) drivePlan(ctx context.Context, group *seiv1alpha1.SeiNodeDeployment, statusBase client.Patch) (ctrl.Result, error) {
+func (r *SeiNodeDeploymentReconciler) drivePlan(ctx context.Context, group *seiv1alpha1.SeiNodeDeployment) (ctrl.Result, error) {
 	result, err := r.PlanExecutor.ExecutePlan(ctx, group, group.Status.Plan)
 	if err != nil {
 		return result, err
 	}
 
 	switch group.Status.Plan.Phase {
 	case seiv1alpha1.TaskPlanComplete:
-		return r.completePlan(ctx, group, statusBase)
+		return r.completePlan(ctx, group)
 	case seiv1alpha1.TaskPlanFailed:
-		return r.failPlan(ctx, group, statusBase)
+		return r.failPlan(ctx, group)
 	}
 
 	return result, nil
@@ -70,9 +70,17 @@ func (r *SeiNodeDeploymentReconciler) startPlan(ctx context.Context, group *seiv
 	return planner.ResultRequeueImmediate, nil
 }
 
-func (r *SeiNodeDeploymentReconciler) completePlan(ctx context.Context, group *seiv1alpha1.SeiNodeDeployment, statusBase client.Patch) (ctrl.Result, error) {
+func (r *SeiNodeDeploymentReconciler) completePlan(ctx context.Context, group *seiv1alpha1.SeiNodeDeployment) (ctrl.Result, error) {
 	logger := log.FromContext(ctx)
 
+	// Re-read to get a fresh resourceVersion. The plan executor patches
+	// status mid-reconcile (task/plan completion), which invalidates the
+	// statusBase computed at the start of Reconcile.
+	if err := r.Get(ctx, client.ObjectKeyFromObject(group), group); err != nil {
+		return ctrl.Result{}, fmt.Errorf("re-reading group for completePlan: %w", err)
+	}
+	status := client.MergeFromWithOptions(group.DeepCopy(), client.MergeFromWithOptimisticLock{})
+
 	isDeploymentPlan := group.Status.Rollout != nil
 
 	if isDeploymentPlan {
@@ -97,15 +105,20 @@ func (r *SeiNodeDeploymentReconciler) completePlan(ctx context.Context, group *s
 	r.Recorder.Event(group, corev1.EventTypeNormal, "PlanComplete", "Plan completed successfully")
 	logger.Info("plan completed")
 
-	if err := r.updateStatus(ctx, group, statusBase); err != nil {
+	if err := r.updateStatus(ctx, group, status); err != nil {
 		return ctrl.Result{}, fmt.Errorf("updating status after plan completion: %w", err)
 	}
 	return planner.ResultRequeueImmediate, nil
 }
 
-func (r *SeiNodeDeploymentReconciler) failPlan(ctx context.Context, group *seiv1alpha1.SeiNodeDeployment, statusBase client.Patch) (ctrl.Result, error) {
+func (r *SeiNodeDeploymentReconciler) failPlan(ctx context.Context, group *seiv1alpha1.SeiNodeDeployment) (ctrl.Result, error) {
 	logger := log.FromContext(ctx)
 
+	if err := r.Get(ctx, client.ObjectKeyFromObject(group), group); err != nil {
+		return ctrl.Result{}, fmt.Errorf("re-reading group for failPlan: %w", err)
+	}
+	status := client.MergeFromWithOptions(group.DeepCopy(), client.MergeFromWithOptimisticLock{})
+
 	group.Status.Phase = seiv1alpha1.GroupPhaseDegraded
 	group.Status.Plan = nil
 	if group.Status.Rollout != nil {
@@ -118,7 +131,7 @@ func (r *SeiNodeDeploymentReconciler) failPlan(ctx context.Context, group *seiv1
 	r.Recorder.Event(group, corev1.EventTypeWarning, "PlanFailed", "Plan failed")
 	logger.Info("plan failed")
 
-	if err := r.updateStatus(ctx, group, statusBase); err != nil {
+	if err := r.updateStatus(ctx, group, status); err != nil {
 		return ctrl.Result{}, fmt.Errorf("updating status after plan failure: %w", err)
 	}
 	return planner.ResultRequeueImmediate, nil

internal/controller/nodedeployment/plan_test.go

@@ -78,8 +78,7 @@ func TestCompletePlan_ClearsRolloutInProgress(t *testing.T) {
 
 	r, c := newPlanTestReconciler(t, group, childNode)
 
-	statusBase := client.MergeFromWithOptions(group.DeepCopy(), client.MergeFromWithOptimisticLock{})
-	_, err := r.completePlan(ctx, group, statusBase)
+	_, err := r.completePlan(ctx, group)
 	g.Expect(err).NotTo(HaveOccurred())
 
 	fetched := &seiv1alpha1.SeiNodeDeployment{}
@@ -148,8 +147,7 @@ func TestFailPlan_ClearsRolloutInProgress(t *testing.T) {
 
 	r, c := newPlanTestReconciler(t, group, childRunning, childFailed, childFailed2)
 
-	statusBase := client.MergeFromWithOptions(group.DeepCopy(), client.MergeFromWithOptimisticLock{})
-	_, err := r.failPlan(ctx, group, statusBase)
+	_, err := r.failPlan(ctx, group)
 	g.Expect(err).NotTo(HaveOccurred())
 
 	fetched := &seiv1alpha1.SeiNodeDeployment{}