Use lazy copy to patch resources to ensure multiple modifications are applied to the base resource (#4580)

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

.mockery.yaml

@@ -14,4 +14,8 @@ packages:
       all: true
   github.com/actions/actions-runner-controller/controllers/actions.github.com:
     config:
-      all: true
+      all: false
+      include-interface-regex: ".*"
+      # deepCopyObject is a generic type constraint, not a collaborator, so it
+      # must not be mocked.
+      exclude-interface-regex: "^deepCopyObject$"

controllers/actions.github.com/autoscalinglistener_controller.go

@@ -78,7 +78,7 @@ func (r *AutoscalingListenerReconciler) Reconcile(ctx context.Context, req ctrl.
 	if err := r.Get(ctx, req.NamespacedName, &autoscalingListener); err != nil {
 		return ctrl.Result{}, client.IgnoreNotFound(err)
 	}
-	original := autoscalingListener.DeepCopy()
+	listener := newLazyCopy(&autoscalingListener)
 
 	if !autoscalingListener.DeletionTimestamp.IsZero() {
 		if !controllerutil.ContainsFinalizer(&autoscalingListener, autoscalingListenerFinalizerName) {
@@ -97,8 +97,8 @@ func (r *AutoscalingListenerReconciler) Reconcile(ctx context.Context, req ctrl.
 		}
 
 		log.Info("Removing finalizer")
-		if controllerutil.RemoveFinalizer(&autoscalingListener, autoscalingListenerFinalizerName) {
-			if err := r.Patch(ctx, &autoscalingListener, client.MergeFrom(original)); err != nil && !kerrors.IsNotFound(err) {
+		if controllerutil.RemoveFinalizer(listener.Mutate(), autoscalingListenerFinalizerName) {
+			if err := r.Patch(ctx, &autoscalingListener, listener.MergeFrom()); err != nil && !kerrors.IsNotFound(err) {
 				log.Error(err, "Failed to remove finalizer")
 				return ctrl.Result{}, err
 			}
@@ -109,8 +109,9 @@ func (r *AutoscalingListenerReconciler) Reconcile(ctx context.Context, req ctrl.
 		return ctrl.Result{}, nil
 	}
 
-	if controllerutil.AddFinalizer(&autoscalingListener, autoscalingListenerFinalizerName) {
-		if err := r.Patch(ctx, &autoscalingListener, client.MergeFrom(original)); err != nil {
+	if !controllerutil.ContainsFinalizer(&autoscalingListener, autoscalingListenerFinalizerName) {
+		controllerutil.AddFinalizer(listener.Mutate(), autoscalingListenerFinalizerName)
+		if err := r.Patch(ctx, &autoscalingListener, listener.MergeFrom()); err != nil {
 			log.Error(err, "Failed to add finalizer")
 			return ctrl.Result{}, err
 		}

controllers/actions.github.com/autoscalingrunnerset_controller.go

@@ -75,7 +75,7 @@ func (r *AutoscalingRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl
 	if err := r.Get(ctx, req.NamespacedName, &autoscalingRunnerSet); err != nil {
 		return ctrl.Result{}, client.IgnoreNotFound(err)
 	}
-	original := autoscalingRunnerSet.DeepCopy()
+	runnerSet := newLazyCopy(&autoscalingRunnerSet)
 
 	if !autoscalingRunnerSet.DeletionTimestamp.IsZero() {
 		if !controllerutil.ContainsFinalizer(&autoscalingRunnerSet, autoscalingRunnerSetFinalizerName) {
@@ -100,9 +100,9 @@ func (r *AutoscalingRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl
 			return ctrl.Result{}, err
 		}
 
-		if controllerutil.RemoveFinalizer(&autoscalingRunnerSet, autoscalingRunnerSetFinalizerName) {
+		if controllerutil.RemoveFinalizer(runnerSet.Mutate(), autoscalingRunnerSetFinalizerName) {
 			log.Info("Removing finalizer")
-			if err := r.Patch(ctx, &autoscalingRunnerSet, client.MergeFrom(original)); err != nil && !kerrors.IsNotFound(err) {
+			if err := r.Patch(ctx, &autoscalingRunnerSet, runnerSet.MergeFrom()); err != nil && !kerrors.IsNotFound(err) {
 				log.Error(err, "Failed to update autoscaling runner set without finalizer")
 				return ctrl.Result{}, err
 			}
@@ -131,10 +131,11 @@ func (r *AutoscalingRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl
 		return ctrl.Result{}, nil
 	}
 
-	if controllerutil.AddFinalizer(&autoscalingRunnerSet, autoscalingRunnerSetFinalizerName) {
+	if !controllerutil.ContainsFinalizer(&autoscalingRunnerSet, autoscalingRunnerSetFinalizerName) {
+		controllerutil.AddFinalizer(runnerSet.Mutate(), autoscalingRunnerSetFinalizerName)
 		log.Info("Adding finalizer")
 
-		if err := r.Patch(ctx, &autoscalingRunnerSet, client.MergeFrom(original)); err != nil {
+		if err := r.Patch(ctx, &autoscalingRunnerSet, runnerSet.MergeFrom()); err != nil {
 			log.Error(err, "Failed to update autoscaling runner set with finalizer")
 			return ctrl.Result{}, err
 		}

controllers/actions.github.com/ephemeralrunner_controller.go

@@ -94,7 +94,7 @@ func (r *EphemeralRunnerReconciler) Reconcile(ctx context.Context, req ctrl.Requ
 	if err := r.Get(ctx, req.NamespacedName, &ephemeralRunner); err != nil {
 		return ctrl.Result{}, client.IgnoreNotFound(err)
 	}
-	original := ephemeralRunner.DeepCopy()
+	runner := newLazyCopy(&ephemeralRunner)
 
 	if !ephemeralRunner.DeletionTimestamp.IsZero() {
 		r.publishEphemeralRunnerPhaseMetric(&ephemeralRunner, "", log)
@@ -116,9 +116,9 @@ func (r *EphemeralRunnerReconciler) Reconcile(ctx context.Context, req ctrl.Requ
 			}
 
 			log.Info("Runner is cleaned up from the service, removing finalizer")
-			if controllerutil.RemoveFinalizer(&ephemeralRunner, ephemeralRunnerActionsFinalizerName) {
+			if controllerutil.RemoveFinalizer(runner.Mutate(), ephemeralRunnerActionsFinalizerName) {
 				log.Info("Removed finalizer from ephemeral runner")
-				if err := r.Patch(ctx, &ephemeralRunner, client.MergeFrom(original)); err != nil {
+				if err := r.Patch(ctx, &ephemeralRunner, runner.MergeFrom()); err != nil {
 					log.Error(err, "Failed to update ephemeral runner after removing finalizer")
 					return ctrl.Result{}, err
 				}
@@ -143,9 +143,9 @@ func (r *EphemeralRunnerReconciler) Reconcile(ctx context.Context, req ctrl.Requ
 		}
 
 		log.Info("Removing finalizer")
-		if controllerutil.RemoveFinalizer(&ephemeralRunner, ephemeralRunnerFinalizerName) {
+		if controllerutil.RemoveFinalizer(runner.Mutate(), ephemeralRunnerFinalizerName) {
 			log.Info("Removed finalizer from ephemeral runner")
-			if err := r.Patch(ctx, &ephemeralRunner, client.MergeFrom(original)); client.IgnoreNotFound(err) != nil {
+			if err := r.Patch(ctx, &ephemeralRunner, runner.MergeFrom()); client.IgnoreNotFound(err) != nil {
 				log.Error(err, "Failed to update ephemeral runner after removing finalizer")
 				return ctrl.Result{}, err
 			}
@@ -171,17 +171,15 @@ func (r *EphemeralRunnerReconciler) Reconcile(ctx context.Context, req ctrl.Requ
 		return ctrl.Result{}, nil
 	}
 
-	addFinalizers := !controllerutil.ContainsFinalizer(&ephemeralRunner, ephemeralRunnerFinalizerName) || !controllerutil.ContainsFinalizer(&ephemeralRunner, ephemeralRunnerActionsFinalizerName)
-	if addFinalizers {
+	missingFinalizers := !controllerutil.ContainsFinalizer(&ephemeralRunner, ephemeralRunnerFinalizerName) ||
+		!controllerutil.ContainsFinalizer(&ephemeralRunner, ephemeralRunnerActionsFinalizerName)
+	if missingFinalizers {
 		log.Info("Adding finalizers")
-		var addedFinalizers bool
-		addedFinalizers = addedFinalizers || controllerutil.AddFinalizer(&ephemeralRunner, ephemeralRunnerFinalizerName)
-		addedFinalizers = addedFinalizers || controllerutil.AddFinalizer(&ephemeralRunner, ephemeralRunnerActionsFinalizerName)
-		if addedFinalizers {
-			if err := r.Patch(ctx, &ephemeralRunner, client.MergeFrom(original)); err != nil {
-				log.Error(err, "Failed to update with finalizer set")
-				return ctrl.Result{}, err
-			}
+		controllerutil.AddFinalizer(runner.Mutate(), ephemeralRunnerFinalizerName)
+		controllerutil.AddFinalizer(runner.Mutate(), ephemeralRunnerActionsFinalizerName)
+		if err := r.Patch(ctx, &ephemeralRunner, runner.MergeFrom()); err != nil {
+			log.Error(err, "Failed to update with finalizer set")
+			return ctrl.Result{}, err
 		}
 		log.Info("Successfully added finalizers")
 	}

controllers/actions.github.com/ephemeralrunnerset_advancing_revision_phase_test.go

@@ -0,0 +1,183 @@
+package actionsgithubcom
+
+import (
+	"context"
+	"testing"
+
+	"github.com/actions/actions-runner-controller/apis/actions.github.com/v1alpha1"
+	"github.com/go-logr/logr"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/types"
+	clientgoscheme "k8s.io/client-go/kubernetes/scheme"
+	"sigs.k8s.io/controller-runtime/pkg/client/fake"
+)
+
+// TestPatchAppliedActionableRevisionStatusClassifiesAgainstTheAdvancedRevision
+// pins the other half of the classifier contract: which revision is used when
+// the monotonicity guard actually fires.
+//
+// TestPatchAppliedActionableRevisionStatusIgnoresATargetBehindTheLiveRevision
+// covers the case where the target has fallen behind the live marker, so the
+// guard does not fire and the applied revision is unchanged. That case cannot
+// observe this one. The guard writes the advance to a copy of the status, so
+// only when it fires do the copy and the stored status hold different
+// revisions, and only then can the classifier be pointed at the wrong one.
+//
+// Reading the pre-advance revision rates a runner left over from the superseded
+// revision as current instead of staleOutdated, which sets the Outdated phase.
+// That phase is then persisted next to the freshly advanced revision, which is
+// the state this function exists to avoid: Reconcile's Outdated branch returns
+// before updateStatus, and this function only runs while spec > applied, so
+// nothing recomputes the phase and the set stays switched off until the next
+// spec change.
+func TestPatchAppliedActionableRevisionStatusClassifiesAgainstTheAdvancedRevision(t *testing.T) {
+	scheme := runtime.NewScheme()
+	require.NoError(t, clientgoscheme.AddToScheme(scheme))
+	require.NoError(t, v1alpha1.AddToScheme(scheme))
+
+	// The spec has moved to revision 2 while the status still records 1, so the
+	// guard below advances the marker and the two revisions differ.
+	ephemeralRunnerSet := &v1alpha1.EphemeralRunnerSet{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "test-ers",
+			Namespace: "default",
+		},
+		Spec: v1alpha1.EphemeralRunnerSetSpec{
+			ActionableRevision: 2,
+		},
+		Status: v1alpha1.EphemeralRunnerSetStatus{
+			AppliedActionableRevision: 1,
+			Phase:                     v1alpha1.EphemeralRunnerSetPhaseRunning,
+		},
+	}
+
+	// A runner built from revision 1 that the service rejected. Applying
+	// revision 2 deletes the idle and pending runners, but this one can survive
+	// the cleanup: the cleanup list is read through the cache and can be stale,
+	// which is the case the reviewer of this code identified.
+	//
+	// Its revision equals the pre-advance marker, so classifying against that
+	// value puts it in outdated rather than staleOutdated. Against the advanced
+	// revision it is correctly stale.
+	controllerRef := true
+	supersededRunner := &v1alpha1.EphemeralRunner{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "runner-from-revision-1",
+			Namespace: "default",
+			Annotations: map[string]string{
+				AnnotationKeyActionableRevision: "1",
+			},
+			OwnerReferences: []metav1.OwnerReference{
+				{
+					APIVersion: v1alpha1.GroupVersion.String(),
+					Kind:       "EphemeralRunnerSet",
+					Name:       "test-ers",
+					UID:        "test-uid",
+					Controller: &controllerRef,
+				},
+			},
+		},
+		Status: v1alpha1.EphemeralRunnerStatus{
+			Phase: v1alpha1.EphemeralRunnerPhaseOutdated,
+		},
+	}
+
+	fakeClient := fake.NewClientBuilder().
+		WithScheme(scheme).
+		WithObjects(ephemeralRunnerSet, supersededRunner).
+		WithStatusSubresource(&v1alpha1.EphemeralRunnerSet{}).
+		WithIndex(&v1alpha1.EphemeralRunner{}, resourceOwnerKey, newGroupVersionOwnerKindIndexer("EphemeralRunnerSet")).
+		Build()
+
+	reconciler := &EphemeralRunnerSetReconciler{
+		Client:    fakeClient,
+		APIReader: fakeClient,
+		Log:       logr.Discard(),
+		Scheme:    scheme,
+	}
+
+	key := types.NamespacedName{Namespace: "default", Name: "test-ers"}
+
+	require.NoError(t, reconciler.patchAppliedActionableRevisionStatus(context.Background(), key, 2))
+
+	var patched v1alpha1.EphemeralRunnerSet
+	require.NoError(t, fakeClient.Get(context.Background(), key, &patched))
+
+	assert.Equal(
+		t,
+		int64(2),
+		patched.Status.AppliedActionableRevision,
+		"the guard must advance the marker to the target revision",
+	)
+
+	assert.Equal(
+		t,
+		v1alpha1.EphemeralRunnerSetPhaseRunning,
+		patched.Status.Phase,
+		"the leftover runner reported Outdated against revision 1, which this call has just superseded, so it must be classified against the advanced revision and ignored: counting it against the pre-advance revision saves the Outdated phase alongside revision 2, and nothing recomputes the phase from there",
+	)
+}
+
+// TestPatchAppliedActionableRevisionStatusDoesNotPatchWhenNothingChanges pins
+// the other half of that contract: the call must issue no patch at all when the
+// status already says what it would set.
+//
+// This is the reason the object is not deep copied up front, and it is decided
+// by whether any mutation was actually made rather than by comparing a
+// separately built status value. Losing it would send an empty patch on every
+// reconcile that reaches this function, and each write bumps the resourceVersion
+// and wakes every watcher, so the regression is invisible in behaviour and
+// visible only in load.
+func TestPatchAppliedActionableRevisionStatusDoesNotPatchWhenNothingChanges(t *testing.T) {
+	scheme := runtime.NewScheme()
+	require.NoError(t, clientgoscheme.AddToScheme(scheme))
+	require.NoError(t, v1alpha1.AddToScheme(scheme))
+
+	// Already applied revision 3, already Running, marker already clear, and no
+	// child runners to move the phase. Every write below is therefore a no-op.
+	ephemeralRunnerSet := &v1alpha1.EphemeralRunnerSet{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "test-ers",
+			Namespace: "default",
+		},
+		Status: v1alpha1.EphemeralRunnerSetStatus{
+			AppliedActionableRevision: 3,
+			Phase:                     v1alpha1.EphemeralRunnerSetPhaseRunning,
+		},
+	}
+
+	fakeClient := fake.NewClientBuilder().
+		WithScheme(scheme).
+		WithObjects(ephemeralRunnerSet).
+		WithStatusSubresource(&v1alpha1.EphemeralRunnerSet{}).
+		WithIndex(&v1alpha1.EphemeralRunner{}, resourceOwnerKey, newGroupVersionOwnerKindIndexer("EphemeralRunnerSet")).
+		Build()
+
+	reconciler := &EphemeralRunnerSetReconciler{
+		Client:    fakeClient,
+		APIReader: fakeClient,
+		Log:       logr.Discard(),
+		Scheme:    scheme,
+	}
+
+	key := types.NamespacedName{Namespace: "default", Name: "test-ers"}
+
+	var before v1alpha1.EphemeralRunnerSet
+	require.NoError(t, fakeClient.Get(context.Background(), key, &before))
+
+	require.NoError(t, reconciler.patchAppliedActionableRevisionStatus(context.Background(), key, 3))
+
+	var after v1alpha1.EphemeralRunnerSet
+	require.NoError(t, fakeClient.Get(context.Background(), key, &after))
+
+	assert.Equal(
+		t,
+		before.ResourceVersion,
+		after.ResourceVersion,
+		"the status already matched, so no patch should have been sent: the resourceVersion moving means an empty patch was written anyway",
+	)
+	assert.Equal(t, before.Status, after.Status)
+}

controllers/actions.github.com/ephemeralrunnerset_controller.go

@@ -87,7 +87,7 @@ func (r *EphemeralRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl.R
 	if err := r.Get(ctx, req.NamespacedName, &ephemeralRunnerSet); err != nil {
 		return ctrl.Result{}, client.IgnoreNotFound(err)
 	}
-	original := ephemeralRunnerSet.DeepCopy()
+	runnerSet := newLazyCopy(&ephemeralRunnerSet)
 
 	// Requested deletion does not need reconciled.
 	if !ephemeralRunnerSet.DeletionTimestamp.IsZero() {
@@ -117,8 +117,8 @@ func (r *EphemeralRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl.R
 		}
 
 		log.Info("Removing finalizer")
-		if controllerutil.RemoveFinalizer(&ephemeralRunnerSet, EphemeralRunnerSetFinalizerName) {
-			if err := r.Patch(ctx, &ephemeralRunnerSet, client.MergeFrom(original)); err != nil {
+		if controllerutil.RemoveFinalizer(runnerSet.Mutate(), EphemeralRunnerSetFinalizerName) {
+			if err := r.Patch(ctx, &ephemeralRunnerSet, runnerSet.MergeFrom()); err != nil {
 				log.Error(err, "Failed to update ephemeral runner set with removed finalizer")
 				return ctrl.Result{}, err
 			}
@@ -130,9 +130,10 @@ func (r *EphemeralRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl.R
 	}
 
 	// Add finalizer if not present
-	if controllerutil.AddFinalizer(&ephemeralRunnerSet, EphemeralRunnerSetFinalizerName) {
+	if !controllerutil.ContainsFinalizer(&ephemeralRunnerSet, EphemeralRunnerSetFinalizerName) {
+		controllerutil.AddFinalizer(runnerSet.Mutate(), EphemeralRunnerSetFinalizerName)
 		log.Info("Adding finalizer")
-		if err := r.Patch(ctx, &ephemeralRunnerSet, client.MergeFrom(original)); err != nil {
+		if err := r.Patch(ctx, &ephemeralRunnerSet, runnerSet.MergeFrom()); err != nil {
 			log.Error(err, "Failed to update ephemeral runner set with new finalizer")
 			return ctrl.Result{}, err
 		}
@@ -344,23 +345,34 @@ func (r *EphemeralRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl.R
 // reader rather than the cache.
 func (r *EphemeralRunnerSetReconciler) patchAppliedActionableRevisionStatus(ctx context.Context, key types.NamespacedName, targetAppliedRevision int64) error {
 	return retry.RetryOnConflict(retry.DefaultBackoff, func() error {
-		var latest v1alpha1.EphemeralRunnerSet
+		var ephemeralRunnerSet v1alpha1.EphemeralRunnerSet
 		reader := r.APIReader
 		if reader == nil {
 			reader = r.Client
 		}
-		if err := reader.Get(ctx, key, &latest); err != nil {
+		if err := reader.Get(ctx, key, &ephemeralRunnerSet); err != nil {
 			return err
 		}
 
-		original := latest.DeepCopy()
+		// Mutations go through this rather than into a separate desired-status
+		// value. Writing to a copy while reading from the original is how the
+		// revision below came to be judged against a superseded value: the write
+		// moved and the read stayed, and because the read was textually unchanged
+		// nothing in the diff pointed at it. Mutate returns the live object, so
+		// every read below observes the writes above it, and the applied revision
+		// only ever lives in one place.
+		//
+		// It also leaves the object uncopied on the common path where nothing
+		// changes, which is why a plain DeepCopy is not taken here.
+		runnerSet := newLazyCopy(&ephemeralRunnerSet)
 
 		// Only an advance means the idle and pending runners were just deleted and
 		// the listener restarted. Guarding both writes on it keeps this callable
 		// as a plain "make sure status reflects revision N" without disturbing a
 		// marker that still describes the live patch sequence.
-		if latest.Status.AppliedActionableRevision < targetAppliedRevision {
-			latest.Status.AppliedActionableRevision = targetAppliedRevision
+		if ephemeralRunnerSet.Status.AppliedActionableRevision < targetAppliedRevision {
+			status := &runnerSet.Mutate().Status
+			status.AppliedActionableRevision = targetAppliedRevision
 
 			// The marker records a patch ID from the sequence that was current
 			// before this spec change. Applying a new revision deletes the idle and
@@ -370,7 +382,7 @@ func (r *EphemeralRunnerSetReconciler) patchAppliedActionableRevisionStatus(ctx
 			// through every integer. It therefore passes through a leftover marker
 			// value with near-certainty, and would suppress the very scale up that
 			// rebuilds the pool.
-			latest.Status.FinishedRunnerCleanupPatchID = 0
+			status.FinishedRunnerCleanupPatchID = 0
 		}
 
 		ephemeralRunnerList := new(v1alpha1.EphemeralRunnerList)
@@ -395,11 +407,11 @@ func (r *EphemeralRunnerSetReconciler) patchAppliedActionableRevisionStatus(ctx
 		// fail. A namespace can hold more than one scale set, so this does read
 		// runners that are not ours, but it only runs when a revision actually
 		// advances rather than on every reconcile.
-		if err := reader.List(ctx, ephemeralRunnerList, client.InNamespace(latest.Namespace)); err != nil {
+		if err := reader.List(ctx, ephemeralRunnerList, client.InNamespace(ephemeralRunnerSet.Namespace)); err != nil {
 			return fmt.Errorf("failed to list child ephemeral runners: %w", err)
 		}
 		ephemeralRunnerList.Items = slices.DeleteFunc(ephemeralRunnerList.Items, func(runner v1alpha1.EphemeralRunner) bool {
-			return !isControlledBy(&runner, "EphemeralRunnerSet", latest.Name)
+			return !isControlledBy(&runner, "EphemeralRunnerSet", ephemeralRunnerSet.Name)
 		})
 
 		// Judge the runners against the revision the set has now applied, rather
@@ -409,34 +421,52 @@ func (r *EphemeralRunnerSetReconciler) patchAppliedActionableRevisionStatus(ctx
 		// the Outdated phase immediately rather than waiting for the pre-update
 		// runners to be collected.
 		//
-		// After the guard above, this field is max(live, target), and the two
-		// differ in a case that matters. The caller reads the spec from the
-		// cache while this function re-reads the status from the API server, so
-		// a lagging reconcile can arrive with a target behind the live marker.
-		// Judging against that lower target would rate a runner left over from
-		// the superseded revision as current and flip a set that has already
-		// moved on back to Outdated. That phase is deliberately absorbing, so
-		// the set would then stay switched off until the next spec change.
-		state := newEphemeralRunnersByStates(ephemeralRunnerList, latest.Status.AppliedActionableRevision)
+		// This must stay below the guard, which is what makes the field read here
+		// max(live, target) rather than just the live value. The two differ in a
+		// case that matters in both directions.
+		//
+		// Reading a value behind the live marker rates a runner left over from a
+		// superseded revision as current and flips a set that has already moved on
+		// back to Outdated. The caller reads the spec from the cache while this
+		// function re-reads the status from the API server, so a lagging reconcile
+		// can arrive with a target behind the live marker; the guard is what stops
+		// that target being used.
+		//
+		// Reading a value behind the one being applied does the same thing to the
+		// advance itself. A runner missed by the cleanup, whose list is read
+		// through the cache, carries the pre-advance revision, so judging it
+		// against that revision counts it as current and saves Outdated alongside
+		// the freshly advanced marker.
+		//
+		// Neither is self-correcting. The Outdated phase is absorbing here:
+		// Reconcile returns on that path before reaching updateStatus, and this
+		// function only runs while spec is ahead of applied, so nothing recomputes
+		// the phase and the set stays switched off until the next spec change.
+		state := newEphemeralRunnersByStates(ephemeralRunnerList, ephemeralRunnerSet.Status.AppliedActionableRevision)
 
 		// Set the phase in both directions. This function returns early from
 		// Reconcile without reaching updateStatus, so leaving the phase untouched
 		// would let a stale value survive: a stale Running would hide genuinely
 		// outdated runners from the cleanup path, and a stale Outdated would keep
 		// the set switched off after the spec that caused it was replaced.
+		phase := v1alpha1.EphemeralRunnerSetPhaseRunning
 		if len(state.outdated) > 0 {
-			latest.Status.Phase = v1alpha1.EphemeralRunnerSetPhaseOutdated
-		} else {
-			latest.Status.Phase = v1alpha1.EphemeralRunnerSetPhaseRunning
+			phase = v1alpha1.EphemeralRunnerSetPhaseOutdated
+		}
+		if ephemeralRunnerSet.Status.Phase != phase {
+			runnerSet.Mutate().Status.Phase = phase
 		}
 
-		// Checked after every field above has been set, so that clearing the
-		// marker alone is still enough to issue the patch.
-		if original.Status == latest.Status {
+		// Every write above is guarded on the value actually changing, so an
+		// unmodified lazyCopy means the status already says what this call wanted
+		// it to say. Clearing the marker alone still counts as a change.
+		if !runnerSet.Modified() {
 			return nil
 		}
 
-		return r.Status().Patch(ctx, &latest, client.MergeFromWithOptions(original, client.MergeFromWithOptimisticLock{}))
+		// The lock covers the object this patch was computed against, so the
+		// snapshot has to be the one taken before the first mutation above.
+		return r.Status().Patch(ctx, &ephemeralRunnerSet, runnerSet.MergeFrom(client.MergeFromWithOptimisticLock{}))
 	})
 }
 
@@ -551,7 +581,6 @@ func (r *EphemeralRunnerSetReconciler) patchFinishedRunnerCleanupPatchIDStatus(c
 }
 
 func (r *EphemeralRunnerSetReconciler) updateStatus(ctx context.Context, ephemeralRunnerSet *v1alpha1.EphemeralRunnerSet, state *ephemeralRunnersByState, log logr.Logger) error {
-	original := ephemeralRunnerSet.DeepCopy()
 	var phase v1alpha1.EphemeralRunnerSetPhase
 	switch {
 	case len(state.outdated) > 0:
@@ -569,6 +598,7 @@ func (r *EphemeralRunnerSetReconciler) updateStatus(ctx context.Context, ephemer
 
 	// Update the status if needed.
 	if ephemeralRunnerSet.Status != desiredStatus {
+		original := ephemeralRunnerSet.DeepCopy()
 		ephemeralRunnerSet.Status = desiredStatus
 		if err := r.Status().Patch(ctx, ephemeralRunnerSet, client.MergeFrom(original)); err != nil {
 			log.Error(err, "Failed to update EphemeralRunnerSet status")

controllers/actions.github.com/lazycopy.go

@@ -0,0 +1,98 @@
+package actionsgithubcom
+
+import "sigs.k8s.io/controller-runtime/pkg/client"
+
+// deepCopyObject is satisfied by every generated API type pointer, as well as
+// by the built-in Kubernetes types.
+type deepCopyObject[T any] interface {
+	client.Object
+	DeepCopy() T
+}
+
+// lazyCopy defers the DeepCopy of a fetched object until the moment it is
+// actually about to be mutated.
+//
+// Reconcilers observe an object far more often than they change it, so taking
+// the snapshot up front means paying for a full deep copy on every reconcile
+// just to serve the rare patch. lazyCopy pays for it only on the reconciles
+// that patch.
+//
+// The snapshot must be taken before the first mutation, otherwise the merge
+// patch is computed against the already mutated object and comes out empty.
+// Routing a mutation through Mutate is what guarantees that ordering:
+//
+//	runner := newLazyCopy(&ephemeralRunner)
+//	if !controllerutil.ContainsFinalizer(&ephemeralRunner, name) {
+//		controllerutil.AddFinalizer(runner.Mutate(), name)
+//	}
+//	if runner.Modified() {
+//		err := r.Patch(ctx, &ephemeralRunner, runner.MergeFrom())
+//	}
+//
+// Callers must uphold that ordering themselves, because lazyCopy cannot
+// enforce it. The caller keeps the pointer it passed to newLazyCopy, and as the
+// example shows it goes on using that pointer to read the object and to address
+// the patch. Nothing stops it from writing through it as well. A write that
+// lands before the first Mutate is already present in the snapshot, so the
+// merge patch against that snapshot is empty and the write is silently dropped
+// rather than sent to the API server.
+//
+// So the invariant is: read through the original as much as you like, but make
+// every mutation that the patch should carry go through Mutate.
+//
+// "The patch" is the qualifier that matters for a type with a status
+// subresource, because there the object is two independently patchable
+// surfaces. A write to status cannot go missing from a patch that does not
+// carry status: the API server ignores status in the body of a merge patch to
+// the main resource, so a status write made outside Mutate is neither captured
+// by nor dropped from that patch. EphemeralRunnerSetReconciler.updateStatus
+// relies on this, writing Status directly while Reconcile holds a lazyCopy over
+// the same object and persisting it through its own Status().Patch. That reads
+// like a violation of the rule above and is not one.
+//
+// The reverse is not true. A lazyCopy guarding a status patch has the same
+// exposure to metadata writes, so the rule holds surface by surface rather than
+// object by object.
+//
+// A lazyCopy is not safe for concurrent use.
+type lazyCopy[T deepCopyObject[T]] struct {
+	obj      T
+	original T
+	copied   bool
+}
+
+// newLazyCopy returns a lazyCopy guarding obj. No copy is taken until the
+// first call to Mutate.
+func newLazyCopy[T deepCopyObject[T]](obj T) *lazyCopy[T] {
+	return &lazyCopy[T]{obj: obj}
+}
+
+// Mutate snapshots the object on its first call and returns the live object so
+// the caller can modify it. Every mutation that a later patch should carry must
+// go through Mutate.
+func (l *lazyCopy[T]) Mutate() T {
+	if !l.copied {
+		l.original = l.obj.DeepCopy()
+		l.copied = true
+	}
+	return l.obj
+}
+
+// Modified reports whether Mutate has been called, and therefore whether there
+// is anything to patch.
+func (l *lazyCopy[T]) Modified() bool {
+	return l.copied
+}
+
+// MergeFrom returns a merge patch against the snapshot taken by the first
+// Mutate call, with any of controller-runtime's merge options applied, such as
+// client.MergeFromWithOptimisticLock{}. It panics when called on an unmodified
+// lazyCopy, because there is no snapshot to diff against and the caller would
+// otherwise silently issue a patch computed from the live object against
+// itself. Guard it with Modified.
+func (l *lazyCopy[T]) MergeFrom(opts ...client.MergeFromOption) client.Patch {
+	if !l.copied {
+		panic("lazyCopy: MergeFrom called before Mutate")
+	}
+	return client.MergeFromWithOptions(l.original, opts...)
+}

controllers/actions.github.com/lazycopy_test.go

@@ -0,0 +1,113 @@
+package actionsgithubcom
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	corev1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"sigs.k8s.io/controller-runtime/pkg/client"
+)
+
+func TestLazyCopyDoesNotCopyUntilMutated(t *testing.T) {
+	pod := &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: "pod"}}
+	lazy := newLazyCopy(pod)
+
+	assert.False(t, lazy.Modified())
+	assert.Panics(t, func() { lazy.MergeFrom() })
+}
+
+func TestLazyCopySnapshotsBeforeTheFirstMutation(t *testing.T) {
+	pod := &corev1.Pod{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:        "pod",
+			Annotations: map[string]string{"key": "old"},
+		},
+	}
+	lazy := newLazyCopy(pod)
+
+	lazy.Mutate().Annotations["key"] = "new"
+	require.True(t, lazy.Modified())
+
+	data, err := lazy.MergeFrom().Data(pod)
+	require.NoError(t, err)
+	assert.JSONEq(t, `{"metadata":{"annotations":{"key":"new"}}}`, string(data))
+}
+
+func TestLazyCopySnapshotsOnlyOnce(t *testing.T) {
+	pod := &corev1.Pod{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:        "pod",
+			Annotations: map[string]string{"first": "old", "second": "old"},
+		},
+	}
+	lazy := newLazyCopy(pod)
+
+	lazy.Mutate().Annotations["first"] = "new"
+	// The second mutation must diff against the state before the first one,
+	// otherwise the earlier change is dropped from the patch.
+	lazy.Mutate().Annotations["second"] = "new"
+
+	data, err := lazy.MergeFrom().Data(pod)
+	require.NoError(t, err)
+	assert.JSONEq(t, `{"metadata":{"annotations":{"first":"new","second":"new"}}}`, string(data))
+}
+
+func TestLazyCopyMergeFromIsAMergePatch(t *testing.T) {
+	pod := &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: "pod"}}
+	lazy := newLazyCopy(pod)
+	lazy.Mutate().Labels = map[string]string{"key": "value"}
+
+	assert.Equal(t, client.MergeFrom(pod).Type(), lazy.MergeFrom().Type())
+}
+
+// Pins the hazard documented on lazyCopy: the caller keeps the pointer it
+// handed to newLazyCopy, so it can write through it without going via Mutate.
+// A write that lands before the first Mutate is already in the snapshot, so it
+// is absent from the patch and never reaches the API server. The type cannot
+// prevent this, which is why the ordering is a caller invariant rather than a
+// guarantee.
+func TestLazyCopyDropsWritesMadeBeforeTheFirstMutate(t *testing.T) {
+	pod := &corev1.Pod{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:        "pod",
+			Annotations: map[string]string{"smuggled": "old", "declared": "old"},
+		},
+	}
+	lazy := newLazyCopy(pod)
+
+	// Bypasses Mutate, so it is captured by the snapshot taken below.
+	pod.Annotations["smuggled"] = "new"
+
+	lazy.Mutate().Annotations["declared"] = "new"
+
+	data, err := lazy.MergeFrom().Data(pod)
+	require.NoError(t, err)
+	assert.JSONEq(t, `{"metadata":{"annotations":{"declared":"new"}}}`, string(data))
+}
+
+// The status patch in patchAppliedActionableRevisionStatus relies on being able
+// to attach an optimistic lock, so that a patch computed from a stale read is
+// rejected by the API server rather than silently moving the applied revision
+// backwards. Without the precondition the patch can never conflict, so the
+// surrounding RetryOnConflict would never fire.
+func TestLazyCopyMergeFromForwardsMergeOptions(t *testing.T) {
+	pod := &corev1.Pod{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:            "pod",
+			ResourceVersion: "42",
+		},
+	}
+	lazy := newLazyCopy(pod)
+	lazy.Mutate().Labels = map[string]string{"key": "value"}
+
+	data, err := lazy.MergeFrom(client.MergeFromWithOptimisticLock{}).Data(pod)
+	require.NoError(t, err)
+	assert.JSONEq(
+		t,
+		`{"metadata":{"labels":{"key":"value"},"resourceVersion":"42"}}`,
+		string(data),
+		"the lock is carried as a resourceVersion precondition in the patch body, taken from the snapshot rather than the mutated object",
+	)
+}