Clear the scale-up suppression marker when the revision advances

Status.FinishedRunnerCleanupPatchID was only ever set, never cleared, so
a marker recorded under one listener incarnation outlived the patch-ID
sequence it described. Applying a new actionable revision deletes the
idle and pending runners so they are rebuilt from the new spec, and it
restarts the listener. A restarted listener numbers its patches from 0
upwards and counts through every integer, so it does not merely risk
reusing the leftover value, it passes through it. If that reuse lands on
the reconcile that has to refill the pool, the guard suppresses exactly
the scale up the revision change asked for.

Clearing the marker where the applied revision advances is enough,
because the marker only ever means "the gap below Spec.Replicas was made
by cleaning up finished runners for this patch ID", and a spec change
invalidates that claim outright. That read now bypasses the cache: the
patch is a diff against the object that was read, so a cached copy
showing 0 while the server held a marker would emit no entry for the
field and leave the stale value behind.

Clearing the marker also breaks the invariant the cached fast path in
scaleUpServicedByFinishedRunnerCleanup rested on. That path was safe
only because a marker was never removed, so a cache hit could not be a
false positive. Now it can be: a lagging cache can show a marker the
server has already cleared, which suppresses the rebuild. The decision
is therefore always made against an uncached read. That costs one GET,
and only on reconciles that were about to issue creates anyway.

One window remains and is documented rather than papered over. A
listener that restarts without a spec change keeps the marker and still
renumbers from 0, so a collision is still likely. It costs one
suppressed reconcile, not an outage: the listener calls back into
scaling on every long-poll timeout rather than only on change, and an
idle set at its minimum publishes the collapsed patch ID 0, which is
never suppressed.

This was found while investigating the update-gha-runner-scale-set e2e
failure on the tip of the stack. It is a real defect, but it does not
explain that failure, whose cause remains open: the suppression here is
self-correcting within a long-poll cycle rather than terminal.

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

controllers/actions.github.com/ephemeralrunnerset_controller.go

@@ -299,10 +299,20 @@ func (r *EphemeralRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl.R
 // reconciler already has, because the cleanup can take long enough for that copy
 // to go stale, and a conflicting write must not be resolved by replaying an old
 // status.
+//
+// The read bypasses the cache because this also clears
+// FinishedRunnerCleanupPatchID, and the patch is computed as a diff against the
+// 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.
 func (r *EphemeralRunnerSetReconciler) patchAppliedActionableRevisionStatus(ctx context.Context, key types.NamespacedName, targetAppliedRevision int64) error {
 	return retry.RetryOnConflict(retry.DefaultBackoff, func() error {
 		var latest v1alpha1.EphemeralRunnerSet
-		if err := r.Get(ctx, key, &latest); err != nil {
+		reader := r.APIReader
+		if reader == nil {
+			reader = r.Client
+		}
+		if err := reader.Get(ctx, key, &latest); err != nil {
 			return err
 		}
 
@@ -313,6 +323,16 @@ func (r *EphemeralRunnerSetReconciler) patchAppliedActionableRevisionStatus(ctx
 		original := latest.DeepCopy()
 		latest.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 pending
+		// runners, so the shortfall that follows belongs to the new spec and must
+		// be filled. Worse, a spec change restarts the listener, and a restarted
+		// listener numbers its patches from 0 upwards, counting 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
+
 		return r.Status().Patch(ctx, &latest, client.MergeFrom(original))
 	})
 }
@@ -323,25 +343,36 @@ func (r *EphemeralRunnerSetReconciler) patchAppliedActionableRevisionStatus(ctx
 // 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.
+// cached copy handed to Reconcile cannot be trusted: 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. The decision is therefore always made against
+// an uncached read. That confines the extra API call to scale-up decisions,
+// where the controller is about to issue creates anyway.
 //
-// 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.
+// An earlier version short-circuited on a cached hit, on the reasoning that the
+// marker was only ever set and so a hit could never be a false positive. That
+// reasoning no longer holds: applying a new actionable revision clears the
+// marker, so a lagging cache can show a recorded marker that the API server has
+// already cleared, and trusting it would suppress exactly the scale up that
+// rebuilds the pool after a spec change.
+//
+// One window remains. A listener that restarts without a spec change keeps the
+// marker but starts its patch sequence again from 0 and counts up through every
+// integer, so it passes through the recorded value with near-certainty rather
+// than by coincidence. If that collision lands on a reconcile that needs to
+// scale up, that reconcile is suppressed.
+//
+// That is a hiccup rather than an outage. The listener calls back into scaling
+// on every long-poll timeout, not only when something changes, and once the set
+// is idle at its minimum with no job completed it publishes the collapsed patch
+// ID 0, which is never suppressed. So the shortfall is filled on the next
+// long-poll cycle.
 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")
 	}

controllers/actions.github.com/ephemeralrunnerset_scaleup_suppression_test.go

@@ -47,14 +47,15 @@ func TestScaleUpServicedByFinishedRunnerCleanup(t *testing.T) {
 		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{}
+	t.Run("suppresses when the API server records the spec patch ID", func(t *testing.T) {
+		r := &EphemeralRunnerSetReconciler{
+			Client:    newReader(newSet(2, 2)),
+			APIReader: newReader(newSet(2, 2)),
+		}
 
 		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")
+		assert.True(t, suppressed, "the cleanup for this patch ID is 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) {
@@ -69,10 +70,10 @@ func TestScaleUpServicedByFinishedRunnerCleanup(t *testing.T) {
 	})
 
 	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.
+		// 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)),
@@ -84,10 +85,27 @@ func TestScaleUpServicedByFinishedRunnerCleanup(t *testing.T) {
 		assert.True(t, suppressed, "the decision must come from the API server, not from a cache that has not caught up")
 	})
 
+	t.Run("does not suppress when the cache still shows a marker the API server has cleared", func(t *testing.T) {
+		// The mirror image of the case above, and the one that opens up once
+		// applying a new revision clears the marker. A cached hit is no longer
+		// self-evidently safe: here the cache still carries the marker from
+		// before the spec change while the API server has already cleared it, and
+		// trusting the cache would suppress the scale up that rebuilds the pool.
+		stale := newSet(2, 2)
+		r := &EphemeralRunnerSetReconciler{
+			Client:    newReader(newSet(2, 2)),
+			APIReader: newReader(newSet(2, 0)),
+		}
+
+		suppressed, err := r.scaleUpServicedByFinishedRunnerCleanup(context.Background(), key, stale)
+		require.NoError(t, err)
+		assert.False(t, suppressed, "a cleared marker on the API server must win over a stale cached one")
+	})
+
 	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.
+		// Patch ID 0 is the collapsed state the listener republishes on every
+		// long-poll timeout once the set is idle at its minimum. 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)),
@@ -105,3 +123,79 @@ func TestScaleUpServicedByFinishedRunnerCleanup(t *testing.T) {
 		assert.Error(t, err, "a missing APIReader must surface rather than silently fall back to the cached marker")
 	})
 }
+
+// TestPatchAppliedActionableRevisionStatusClearsFinishedRunnerCleanupPatchID
+// covers the marker's lifetime across a spec change.
+//
+// The marker is a patch ID, and patch IDs are only meaningful within one
+// listener incarnation. A spec change restarts the listener, which numbers its
+// patches from 0 upwards and so passes through any leftover value. Carrying the
+// marker across the revision boundary therefore suppresses the scale up that is
+// supposed to rebuild the pool the revision cleanup just deleted.
+func TestPatchAppliedActionableRevisionStatusClearsFinishedRunnerCleanupPatchID(t *testing.T) {
+	scheme := runtime.NewScheme()
+	require.NoError(t, v1alpha1.AddToScheme(scheme))
+
+	key := types.NamespacedName{Namespace: "test-ns", Name: "test-ers"}
+
+	newSet := func(appliedRevision int64, cleanupPatchID int) *v1alpha1.EphemeralRunnerSet {
+		return &v1alpha1.EphemeralRunnerSet{
+			ObjectMeta: metav1.ObjectMeta{Namespace: key.Namespace, Name: key.Name},
+			Status: v1alpha1.EphemeralRunnerSetStatus{
+				AppliedActionableRevision:    appliedRevision,
+				FinishedRunnerCleanupPatchID: cleanupPatchID,
+			},
+		}
+	}
+
+	newClient := func(object client.Object) client.Client {
+		return fake.NewClientBuilder().
+			WithScheme(scheme).
+			WithObjects(object).
+			WithStatusSubresource(&v1alpha1.EphemeralRunnerSet{}).
+			Build()
+	}
+
+	t.Run("clears the marker when the applied revision advances", func(t *testing.T) {
+		c := newClient(newSet(3, 7))
+		r := &EphemeralRunnerSetReconciler{Client: c, APIReader: c}
+
+		require.NoError(t, r.patchAppliedActionableRevisionStatus(context.Background(), key, 4))
+
+		var got v1alpha1.EphemeralRunnerSet
+		require.NoError(t, c.Get(context.Background(), key, &got))
+		assert.EqualValues(t, 4, got.Status.AppliedActionableRevision, "the applied revision should advance")
+		assert.Zero(t, got.Status.FinishedRunnerCleanupPatchID, "a marker from the previous patch sequence must not survive the spec change")
+	})
+
+	t.Run("decides against authoritative state rather than the cache", func(t *testing.T) {
+		// The helper both decides and diffs against the object it reads, so that
+		// read has to bypass the cache. Here the cache has already caught up to
+		// revision 4 while the API server has not, which is the shape a cache
+		// takes when it has observed a write the reconcile is about to redo: a
+		// cached read would conclude there is nothing to do and leave the stale
+		// marker in place.
+		authoritative := newClient(newSet(3, 7))
+		lagging := newClient(newSet(4, 7))
+		r := &EphemeralRunnerSetReconciler{Client: lagging, APIReader: authoritative}
+
+		require.NoError(t, r.patchAppliedActionableRevisionStatus(context.Background(), key, 4))
+
+		var got v1alpha1.EphemeralRunnerSet
+		require.NoError(t, lagging.Get(context.Background(), key, &got))
+		assert.Zero(t, got.Status.FinishedRunnerCleanupPatchID, "the clear must be computed against authoritative state")
+	})
+
+	t.Run("leaves the marker alone when the revision has already been applied", func(t *testing.T) {
+		// Nothing was cleaned up here, so there is no reason to disturb a marker
+		// that is still describing the current patch sequence.
+		c := newClient(newSet(4, 7))
+		r := &EphemeralRunnerSetReconciler{Client: c, APIReader: c}
+
+		require.NoError(t, r.patchAppliedActionableRevisionStatus(context.Background(), key, 4))
+
+		var got v1alpha1.EphemeralRunnerSet
+		require.NoError(t, c.Get(context.Background(), key, &got))
+		assert.EqualValues(t, 7, got.Status.FinishedRunnerCleanupPatchID, "an unchanged revision must not clear the marker")
+	})
+}