Lock the cleanup marker patch against a stale write

patchFinishedRunnerCleanupPatchIDStatus wrapped a plain merge patch in
retry.RetryOnConflict. A plain merge patch carries no resourceVersion
precondition, so the API server has nothing to reject: the write always
succeeds, the retry can never fire, and the surrounding machinery reads
as protection while providing none.

The exposure is worse here than for the applied revision, which at least
refuses to move backwards. This helper compares for equality and then
writes whatever patch ID the reconcile is carrying, so it is willing to
lower the marker. A reconcile serving an older patch ID can therefore
overwrite a marker recorded for a newer one, and the scale-up guard then
stops suppressing for the patch it actually serviced -- creating the
replacement runners this layer exists to prevent.

Re-fetching through the API reader narrows that window to the gap
between the read and the patch, but it cannot close it, because the
decision is only as fresh as the moment it was taken. The optimistic
lock is what makes the write conditional on that decision still holding;
a stale attempt now conflicts and is retried rather than silently
recording the wrong patch ID.

The test asserts on the patch bytes the reconciler actually emits rather
than on an independently constructed patch, so it cannot pass while the
production code builds its patch some other way. A second test covers
the early return, since an unconditional write would churn the status on
every reconcile for the same patch.

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

controllers/actions.github.com/ephemeralrunnerset_cleanup_patch_test.go

@@ -0,0 +1,145 @@
+package actionsgithubcom
+
+import (
+	"context"
+	"encoding/json"
+	"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"
+	"sigs.k8s.io/controller-runtime/pkg/client/fake"
+	"sigs.k8s.io/controller-runtime/pkg/client/interceptor"
+)
+
+// TestPatchFinishedRunnerCleanupPatchIDStatusUsesOptimisticLock pins the
+// resourceVersion precondition on the cleanup marker patch.
+//
+// The marker decides whether a shortfall below Spec.Replicas is suppressed, so a
+// write that lands on the wrong patch ID re-enables the spurious scale up this
+// layer exists to prevent. The helper sets the marker to whatever patch ID the
+// reconcile is carrying rather than only ever advancing it, so without the lock
+// the API server cannot reject a stale write, retry.RetryOnConflict never fires,
+// and a reconcile serving an older patch ID can overwrite a marker recorded for
+// a newer one.
+//
+// Asserted on the bytes the production code emits rather than on an
+// independently built patch, so it cannot pass while the reconciler constructs
+// its patch some other way. Reverting the option to a plain client.MergeFrom
+// must fail this test.
+func TestPatchFinishedRunnerCleanupPatchIDStatusUsesOptimisticLock(t *testing.T) {
+	scheme := runtime.NewScheme()
+	require.NoError(t, clientgoscheme.AddToScheme(scheme))
+	require.NoError(t, v1alpha1.AddToScheme(scheme))
+
+	ephemeralRunnerSet := &v1alpha1.EphemeralRunnerSet{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "test-ers",
+			Namespace: "default",
+		},
+		Spec: v1alpha1.EphemeralRunnerSetSpec{
+			PatchID: 4,
+		},
+	}
+
+	var capturedPatch []byte
+	c := fake.NewClientBuilder().
+		WithScheme(scheme).
+		WithObjects(ephemeralRunnerSet).
+		WithStatusSubresource(&v1alpha1.EphemeralRunnerSet{}).
+		WithInterceptorFuncs(interceptor.Funcs{
+			SubResourcePatch: func(ctx context.Context, clt client.Client, subResourceName string, obj client.Object, patch client.Patch, opts ...client.SubResourcePatchOption) error {
+				data, err := patch.Data(obj)
+				if err != nil {
+					return err
+				}
+				capturedPatch = data
+				return clt.Status().Patch(ctx, obj, patch, opts...)
+			},
+		}).
+		Build()
+
+	reconciler := &EphemeralRunnerSetReconciler{
+		Client:    c,
+		APIReader: c,
+		Log:       logr.Discard(),
+		Scheme:    scheme,
+	}
+
+	key := types.NamespacedName{Namespace: ephemeralRunnerSet.Namespace, Name: ephemeralRunnerSet.Name}
+	require.NoError(t, reconciler.patchFinishedRunnerCleanupPatchIDStatus(context.Background(), key, 4))
+
+	require.NotEmpty(t, capturedPatch, "expected the reconciler to emit a status patch")
+
+	var emitted struct {
+		Metadata struct {
+			ResourceVersion string `json:"resourceVersion"`
+		} `json:"metadata"`
+		Status struct {
+			FinishedRunnerCleanupPatchID int `json:"finishedRunnerCleanupPatchID"`
+		} `json:"status"`
+	}
+	require.NoError(t, json.Unmarshal(capturedPatch, &emitted))
+
+	assert.NotEmpty(
+		t,
+		emitted.Metadata.ResourceVersion,
+		"status patch must carry a resourceVersion precondition so a stale write is rejected instead of recording the wrong patch ID, got %s",
+		string(capturedPatch),
+	)
+	assert.Equal(t, 4, emitted.Status.FinishedRunnerCleanupPatchID)
+
+	var updated v1alpha1.EphemeralRunnerSet
+	require.NoError(t, c.Get(context.Background(), key, &updated))
+	assert.Equal(t, 4, updated.Status.FinishedRunnerCleanupPatchID)
+}
+
+// TestPatchFinishedRunnerCleanupPatchIDStatusIsIdempotent covers the early
+// return: a marker already recording this patch ID must not be rewritten, so
+// repeated reconciles for one patch do not churn the status.
+func TestPatchFinishedRunnerCleanupPatchIDStatusIsIdempotent(t *testing.T) {
+	scheme := runtime.NewScheme()
+	require.NoError(t, clientgoscheme.AddToScheme(scheme))
+	require.NoError(t, v1alpha1.AddToScheme(scheme))
+
+	ephemeralRunnerSet := &v1alpha1.EphemeralRunnerSet{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "test-ers",
+			Namespace: "default",
+		},
+		Status: v1alpha1.EphemeralRunnerSetStatus{
+			FinishedRunnerCleanupPatchID: 4,
+		},
+	}
+
+	patched := false
+	c := fake.NewClientBuilder().
+		WithScheme(scheme).
+		WithObjects(ephemeralRunnerSet).
+		WithStatusSubresource(&v1alpha1.EphemeralRunnerSet{}).
+		WithInterceptorFuncs(interceptor.Funcs{
+			SubResourcePatch: func(ctx context.Context, clt client.Client, subResourceName string, obj client.Object, patch client.Patch, opts ...client.SubResourcePatchOption) error {
+				patched = true
+				return clt.Status().Patch(ctx, obj, patch, opts...)
+			},
+		}).
+		Build()
+
+	reconciler := &EphemeralRunnerSetReconciler{
+		Client:    c,
+		APIReader: c,
+		Log:       logr.Discard(),
+		Scheme:    scheme,
+	}
+
+	key := types.NamespacedName{Namespace: ephemeralRunnerSet.Namespace, Name: ephemeralRunnerSet.Name}
+	require.NoError(t, reconciler.patchFinishedRunnerCleanupPatchIDStatus(context.Background(), key, 4))
+
+	assert.False(t, patched, "recording a patch ID the marker already holds must not emit a write")
+}

controllers/actions.github.com/ephemeralrunnerset_controller.go

@@ -402,6 +402,21 @@ func (r *EphemeralRunnerSetReconciler) scaleUpServicedByFinishedRunnerCleanup(ct
 // Like the applied revision above, this is written after the deletions succeed
 // and re-fetches the object inside the retry, so a conflicting write is never
 // resolved by replaying a status that predates the cleanup.
+//
+// The patch carries an optimistic lock for the same reason, and the exposure
+// here is if anything worse: the check below is an equality test rather than a
+// monotonicity test, so this helper is willing to move the marker to whatever
+// patch ID the reconcile is carrying, including backwards. Without a
+// resourceVersion precondition the API server cannot reject the write, so
+// RetryOnConflict can never fire and a reconcile serving an older patch ID can
+// overwrite a marker recorded for a newer one. The guard would then stop
+// suppressing for the patch ID that was actually serviced, and the controller
+// would create the replacement runners this layer exists to prevent.
+//
+// Re-fetching through the API reader narrows that window to the gap between the
+// read and the patch rather than closing it, because the decision is only as
+// fresh as the moment it was taken. The lock is what makes the write conditional
+// on that decision still holding.
 func (r *EphemeralRunnerSetReconciler) patchFinishedRunnerCleanupPatchIDStatus(ctx context.Context, key types.NamespacedName, patchID int) error {
 	return retry.RetryOnConflict(retry.DefaultBackoff, func() error {
 		var latest v1alpha1.EphemeralRunnerSet
@@ -420,7 +435,7 @@ func (r *EphemeralRunnerSetReconciler) patchFinishedRunnerCleanupPatchIDStatus(c
 		original := latest.DeepCopy()
 		latest.Status.FinishedRunnerCleanupPatchID = patchID
 
-		return r.Status().Patch(ctx, &latest, client.MergeFrom(original))
+		return r.Status().Patch(ctx, &latest, client.MergeFromWithOptions(original, client.MergeFromWithOptimisticLock{}))
 	})
 }