Confirm the finished runner cleanup marker outside the cache before scaling up

The scale-up suppression read Status.FinishedRunnerCleanupPatchID off the
EphemeralRunnerSet that Reconcile fetched through the manager's cached
client. That marker is written by the cleanup reconcile, and it is the
runner deletions performed by that same reconcile which trigger the next
one, so the follow-up reconcile is regularly served from an informer cache
that has not yet observed the controller's own status write. The marker
read as 0, the guard did not fire, and the controller created a
replacement runner for a job that had already finished. That is the exact
spurious scale up the guard exists to prevent, so it is a correctness
problem in a cluster and not only a flaky test.

Make the decision from authoritative state. A cached hit still short
circuits, because nothing ever clears the marker, so a hit cannot be a
false positive. Only a miss falls through to an uncached Get via a new
APIReader, which SetupWithManager fills in from mgr.GetAPIReader() so no
construction site can forget it. The extra read is confined to scale-up
decisions, where the controller is about to issue creates anyway.

The window was transient: updateStatus copies the marker from the
in-memory object and patches with MergeFrom, so a stale reconcile produces
no diff for that field and cannot clobber the recorded value.

The envtest spec that caught this only fails under CI load, so the
regression is pinned down directly instead. TestScaleUpServicedByFinished
RunnerCleanup drives the decision with a lagging cached client and an API
reader that already has the write, and asserts the suppression still
holds. Pointing the read back at the cached client fails that case and
only that case.

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

controllers/actions.github.com/ephemeralrunnerset_controller.go

@@ -52,6 +52,12 @@ type EphemeralRunnerSetReconciler struct {
 	client.Client
 	Log    logr.Logger
 	Scheme *runtime.Scheme
+	// APIReader reads straight from the API server, bypassing the manager's
+	// cache. It is needed where the controller has to observe a status field it
+	// wrote itself in an earlier reconcile, because the informer cache is not
+	// guaranteed to have caught up by the time the next reconcile runs.
+	// SetupWithManager fills this in from the manager when it is left unset.
+	APIReader client.Reader
 	ResourceBuilder
 }
 
@@ -235,7 +241,12 @@ func (r *EphemeralRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl.R
 			// The gap below Spec.Replicas is the one the cleanup above opened for
 			// this patch ID, not new demand. Wait for the listener to publish a
 			// fresh desired state before acting on it.
-			if ephemeralRunnerSet.Spec.PatchID > 0 && ephemeralRunnerSet.Status.FinishedRunnerCleanupPatchID == ephemeralRunnerSet.Spec.PatchID {
+			suppressed, err := r.scaleUpServicedByFinishedRunnerCleanup(ctx, req.NamespacedName, &ephemeralRunnerSet)
+			if err != nil {
+				log.Error(err, "failed to determine whether scale up was already serviced by finished runner cleanup")
+				return ctrl.Result{}, err
+			}
+			if suppressed {
 				log.Info("Skipping scale up until listener publishes a fresh desired state after finished runner cleanup", "patchID", ephemeralRunnerSet.Spec.PatchID)
 				return ctrl.Result{}, r.updateStatus(ctx, &ephemeralRunnerSet, ephemeralRunnersByState, log)
 			}
@@ -305,6 +316,43 @@ func (r *EphemeralRunnerSetReconciler) patchAppliedActionableRevisionStatus(ctx
 	})
 }
 
+// scaleUpServicedByFinishedRunnerCleanup reports whether the shortfall against
+// Spec.Replicas was created by this controller cleaning up finished runners for
+// the patch ID currently in the spec, rather than by new demand from the
+// listener.
+//
+// The marker is written by an earlier reconcile and then read back here, so the
+// cached copy handed to Reconcile cannot be trusted on its own: deleting the
+// finished runners triggers watch events that schedule the next reconcile, and
+// that reconcile can be served from an informer cache that has not yet observed
+// the controller's own status write. Reading a zero marker there would skip the
+// suppression and create exactly the unwanted runner this logic exists to
+// prevent.
+//
+// A cached hit is always safe, because nothing clears the marker, so only a miss
+// falls through to an uncached read. That confines the extra API call to
+// scale-up decisions, where the controller is about to issue creates anyway.
+func (r *EphemeralRunnerSetReconciler) scaleUpServicedByFinishedRunnerCleanup(ctx context.Context, key types.NamespacedName, ephemeralRunnerSet *v1alpha1.EphemeralRunnerSet) (bool, error) {
+	if ephemeralRunnerSet.Spec.PatchID == 0 {
+		return false, nil
+	}
+
+	if ephemeralRunnerSet.Status.FinishedRunnerCleanupPatchID == ephemeralRunnerSet.Spec.PatchID {
+		return true, nil
+	}
+
+	if r.APIReader == nil {
+		return false, errors.New("APIReader is not configured, cannot confirm the finished runner cleanup patch ID without reading through the cache")
+	}
+
+	var latest v1alpha1.EphemeralRunnerSet
+	if err := r.APIReader.Get(ctx, key, &latest); err != nil {
+		return false, fmt.Errorf("failed to read EphemeralRunnerSet without the cache: %w", err)
+	}
+
+	return latest.Status.FinishedRunnerCleanupPatchID == ephemeralRunnerSet.Spec.PatchID, nil
+}
+
 // patchFinishedRunnerCleanupPatchIDStatus records that finished runners were
 // deleted while serving patchID, so a later reconcile can tell the resulting gap
 // below Spec.Replicas apart from genuine new demand.
@@ -724,6 +772,10 @@ func (r *EphemeralRunnerSetReconciler) deleteEphemeralRunnerWithActionsClient(ct
 func (r *EphemeralRunnerSetReconciler) SetupWithManager(mgr ctrl.Manager, opts ...Option) error {
 	r.setSchemeIfUnset(r.Scheme)
 
+	if r.APIReader == nil {
+		r.APIReader = mgr.GetAPIReader()
+	}
+
 	return builderWithOptions(
 		ctrl.NewControllerManagedBy(mgr).
 			For(&v1alpha1.EphemeralRunnerSet{}).

controllers/actions.github.com/ephemeralrunnerset_scaleup_suppression_test.go

@@ -0,0 +1,107 @@
+package actionsgithubcom
+
+import (
+	"context"
+	"testing"
+
+	"github.com/actions/actions-runner-controller/apis/actions.github.com/v1alpha1"
+	"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"
+	"sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/controller-runtime/pkg/client/fake"
+)
+
+// TestScaleUpServicedByFinishedRunnerCleanup pins down the decision that
+// suppresses scale up after finished runners were cleaned up.
+//
+// The interesting case is the third one. The marker is written by one reconcile
+// and read back by the next, and the deletions performed by the first reconcile
+// are themselves what triggers the second. That next reconcile is regularly
+// served from an informer cache that has not yet observed the controller's own
+// status write, so the decision must not be made from the cached copy alone. An
+// envtest spec only hits that window under load, which is why this is asserted
+// directly instead.
+func TestScaleUpServicedByFinishedRunnerCleanup(t *testing.T) {
+	scheme := runtime.NewScheme()
+	require.NoError(t, v1alpha1.AddToScheme(scheme))
+
+	key := types.NamespacedName{Namespace: "test-ns", Name: "test-ers"}
+
+	newSet := func(specPatchID, statusCleanupPatchID int) *v1alpha1.EphemeralRunnerSet {
+		return &v1alpha1.EphemeralRunnerSet{
+			ObjectMeta: metav1.ObjectMeta{Namespace: key.Namespace, Name: key.Name},
+			Spec: v1alpha1.EphemeralRunnerSetSpec{
+				Replicas: 2,
+				PatchID:  specPatchID,
+			},
+			Status: v1alpha1.EphemeralRunnerSetStatus{
+				FinishedRunnerCleanupPatchID: statusCleanupPatchID,
+			},
+		}
+	}
+
+	newReader := func(objects ...client.Object) client.Client {
+		return fake.NewClientBuilder().WithScheme(scheme).WithObjects(objects...).Build()
+	}
+
+	t.Run("suppresses when the cached marker already matches the spec patch ID", func(t *testing.T) {
+		// A cached hit needs no confirmation, so the reconciler is deliberately
+		// left without an APIReader: reaching for one here would be a bug.
+		r := &EphemeralRunnerSetReconciler{}
+
+		suppressed, err := r.scaleUpServicedByFinishedRunnerCleanup(context.Background(), key, newSet(2, 2))
+		require.NoError(t, err)
+		assert.True(t, suppressed, "the cleanup for this patch ID is already recorded, so the shortfall is not new demand")
+	})
+
+	t.Run("does not suppress when neither the cache nor the API server records the patch ID", func(t *testing.T) {
+		r := &EphemeralRunnerSetReconciler{
+			Client:    newReader(newSet(2, 0)),
+			APIReader: newReader(newSet(2, 0)),
+		}
+
+		suppressed, err := r.scaleUpServicedByFinishedRunnerCleanup(context.Background(), key, newSet(2, 0))
+		require.NoError(t, err)
+		assert.False(t, suppressed, "no cleanup was performed for this patch ID, so the shortfall is genuine demand")
+	})
+
+	t.Run("suppresses when the cache is stale but the API server records the patch ID", func(t *testing.T) {
+		// This is the race: the cleanup reconcile wrote the marker and returned,
+		// and the reconcile triggered by its own deletions is still reading a
+		// pre-write cache. Client stands in for that lagging cache, APIReader for
+		// the API server that already has the write.
+		stale := newSet(2, 0)
+		r := &EphemeralRunnerSetReconciler{
+			Client:    newReader(newSet(2, 0)),
+			APIReader: newReader(newSet(2, 2)),
+		}
+
+		suppressed, err := r.scaleUpServicedByFinishedRunnerCleanup(context.Background(), key, stale)
+		require.NoError(t, err)
+		assert.True(t, suppressed, "the decision must come from the API server, not from a cache that has not caught up")
+	})
+
+	t.Run("never suppresses when the listener published patch ID zero", func(t *testing.T) {
+		// Patch ID 0 is the idle-at-minimum state the listener republishes
+		// without incrementing. Suppressing on it would let a scale set sit
+		// below its minimum indefinitely.
+		r := &EphemeralRunnerSetReconciler{
+			Client:    newReader(newSet(0, 0)),
+			APIReader: newReader(newSet(0, 0)),
+		}
+
+		suppressed, err := r.scaleUpServicedByFinishedRunnerCleanup(context.Background(), key, newSet(0, 0))
+		require.NoError(t, err)
+		assert.False(t, suppressed, "patch ID 0 must always be free to scale up")
+	})
+
+	t.Run("fails loudly when it cannot read past the cache", func(t *testing.T) {
+		r := &EphemeralRunnerSetReconciler{Client: newReader(newSet(2, 2))}
+
+		_, err := r.scaleUpServicedByFinishedRunnerCleanup(context.Background(), key, newSet(2, 0))
+		assert.Error(t, err, "a missing APIReader must surface rather than silently fall back to the cached marker")
+	})
+}