Merge remote-tracking branch 'origin/nikola-jokic-ers-actionable-revision' into nikola-jokic-ers-scale-up-after-cleanup

# Conflicts:
#	controllers/actions.github.com/ephemeralrunnerset_controller.go

controllers/actions.github.com/ephemeralrunnerset_controller.go

@@ -305,6 +305,16 @@ func (r *EphemeralRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl.R
 // object that was read. A cached read that still showed the field as 0 while the
 // API server held a recorded marker would produce a patch with no entry for the
 // field, silently leaving the stale marker in place.
+//
+// The patch carries an optimistic lock so that the re-fetch actually means
+// something. A plain merge patch has no resourceVersion precondition, so the API
+// server can never reject it as conflicting: RetryOnConflict would never fire,
+// and a patch computed from a stale read could move the applied revision
+// backwards, re-satisfying the spec > applied comparison above and deleting the
+// idle runners all over again. With the lock, the server accepts the write only
+// if the re-fetched object is still the live one, so a successful patch proves
+// the monotonicity check above was evaluated against live data. A stale attempt
+// conflicts and is retried or requeued instead of regressing the marker.
 func (r *EphemeralRunnerSetReconciler) patchAppliedActionableRevisionStatus(ctx context.Context, key types.NamespacedName, targetAppliedRevision int64) error {
 	return retry.RetryOnConflict(retry.DefaultBackoff, func() error {
 		var latest v1alpha1.EphemeralRunnerSet
@@ -333,7 +343,7 @@ func (r *EphemeralRunnerSetReconciler) patchAppliedActionableRevisionStatus(ctx
 		// pool.
 		latest.Status.FinishedRunnerCleanupPatchID = 0
 
-		return r.Status().Patch(ctx, &latest, client.MergeFrom(original))
+		return r.Status().Patch(ctx, &latest, client.MergeFromWithOptions(original, client.MergeFromWithOptimisticLock{}))
 	})
 }
 

controllers/actions.github.com/ephemeralrunnerset_revision_patch_test.go

@@ -0,0 +1,142 @@
+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"
+)
+
+// TestPatchAppliedActionableRevisionStatusUsesOptimisticLock pins the property
+// that makes the applied revision marker monotonic: the status patch must carry
+// a resourceVersion precondition.
+//
+// Without it the API server cannot reject the write as conflicting, so the
+// surrounding retry.RetryOnConflict never fires and the freshness check inside
+// it is unsound. Both the target revision and the re-fetched object come from
+// the cache-backed client, so a stale reconcile can compare a stale target
+// against an equally stale read, pass the check, and patch the applied revision
+// backwards. That re-satisfies the spec > applied comparison in Reconcile and
+// sends the controller through the idle and pending runner cleanup again.
+//
+// The property is asserted on the bytes the production code actually emits
+// rather than on an independently constructed patch, so the test cannot pass
+// while the reconciler builds its patch some other way. Reverting the patch
+// option to a plain client.MergeFrom must fail this test.
+func TestPatchAppliedActionableRevisionStatusUsesOptimisticLock(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{
+			ActionableRevision: 7,
+		},
+		Status: v1alpha1.EphemeralRunnerSetStatus{
+			AppliedActionableRevision: 2,
+		},
+	}
+
+	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,
+		Log:    logr.Discard(),
+		Scheme: scheme,
+	}
+
+	key := types.NamespacedName{Namespace: ephemeralRunnerSet.Namespace, Name: ephemeralRunnerSet.Name}
+	require.NoError(t, reconciler.patchAppliedActionableRevisionStatus(context.Background(), key, 7))
+
+	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 {
+			AppliedActionableRevision int64 `json:"appliedActionableRevision"`
+		} `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 moving the applied revision backwards, got %s",
+		string(capturedPatch),
+	)
+	assert.Equal(t, int64(7), emitted.Status.AppliedActionableRevision)
+
+	var updated v1alpha1.EphemeralRunnerSet
+	require.NoError(t, c.Get(context.Background(), key, &updated))
+	assert.Equal(t, int64(7), updated.Status.AppliedActionableRevision)
+}
+
+// TestPatchAppliedActionableRevisionStatusDoesNotMoveBackwards covers the guard
+// inside the retry: a reconcile carrying an older target revision must leave a
+// marker that has already advanced further alone.
+func TestPatchAppliedActionableRevisionStatusDoesNotMoveBackwards(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{
+			AppliedActionableRevision: 5,
+		},
+	}
+
+	c := fake.NewClientBuilder().
+		WithScheme(scheme).
+		WithObjects(ephemeralRunnerSet).
+		WithStatusSubresource(&v1alpha1.EphemeralRunnerSet{}).
+		Build()
+
+	reconciler := &EphemeralRunnerSetReconciler{
+		Client: c,
+		Log:    logr.Discard(),
+		Scheme: scheme,
+	}
+
+	key := types.NamespacedName{Namespace: ephemeralRunnerSet.Namespace, Name: ephemeralRunnerSet.Name}
+	require.NoError(t, reconciler.patchAppliedActionableRevisionStatus(context.Background(), key, 3))
+
+	var updated v1alpha1.EphemeralRunnerSet
+	require.NoError(t, c.Get(context.Background(), key, &updated))
+	assert.Equal(t, int64(5), updated.Status.AppliedActionableRevision)
+}