Record why the cleanup marker check is equality, not monotonicity

The neighbouring applied-revision helper refuses to move its marker
backwards, so the obvious tidy-up is to make this one match. That would
break it.

Applied revisions derive from metadata.generation and only ever climb.
Listener patch IDs do not. setDesiredWorkerState publishes 0 whenever
the set is idle at MinRunners with nothing dirty, restarts its sequence
from 0 when the listener restarts, and wraps explicitly at MaxInt32, so
Spec.PatchID legitimately moves down. The marker only means "the gap
below Spec.Replicas was created by cleaning up for exactly this patch
ID", so it has to follow the patch ID wherever it goes. A marker that
refused to descend would sit above every value the listener went on to
publish, and since the guard suppresses only on an exact match,
suppression would never fire again -- silently disabling the behaviour
this layer adds.

This also settles what the optimistic lock is and is not for. It cannot
make an older patch ID unwritable, because the retry re-reads and
re-applies the same argument, and recording the patch ID whose cleanup
actually happened is a true statement regardless of ordering. What it
prevents is a write decided against state that has since changed.

The test pins the descending case, so a future reader who reaches for
>= gets a failure rather than a silently inert guard.

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

controllers/actions.github.com/ephemeralrunnerset_cleanup_patch_test.go

@@ -100,6 +100,54 @@ func TestPatchFinishedRunnerCleanupPatchIDStatusUsesOptimisticLock(t *testing.T)
 	assert.Equal(t, 4, updated.Status.FinishedRunnerCleanupPatchID)
 }
 
+// TestPatchFinishedRunnerCleanupPatchIDStatusRecordsALowerPatchID pins the
+// equality check against being "tidied" into the >= monotonicity check its
+// neighbour uses.
+//
+// Applied revisions come from metadata.generation and only climb, but listener
+// patch IDs do not: the scaler publishes 0 whenever the set is idle at
+// MinRunners with nothing dirty, restarts its sequence from 0 on a listener
+// restart, and wraps explicitly at math.MaxInt32. A marker that refused to move
+// down would sit above every value the listener subsequently publishes, and
+// because the scale-up guard suppresses only on an exact match, suppression
+// would never fire again.
+func TestPatchFinishedRunnerCleanupPatchIDStatusRecordsALowerPatchID(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: 7,
+		},
+	}
+
+	c := fake.NewClientBuilder().
+		WithScheme(scheme).
+		WithObjects(ephemeralRunnerSet).
+		WithStatusSubresource(&v1alpha1.EphemeralRunnerSet{}).
+		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, 1))
+
+	var updated v1alpha1.EphemeralRunnerSet
+	require.NoError(t, c.Get(context.Background(), key, &updated))
+	assert.Equal(t, 1, updated.Status.FinishedRunnerCleanupPatchID,
+		"a restarted or collapsed patch sequence must be recorded, or suppression can never match Spec.PatchID again")
+}
+
 // 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.

controllers/actions.github.com/ephemeralrunnerset_controller.go

@@ -417,6 +417,24 @@ func (r *EphemeralRunnerSetReconciler) scaleUpServicedByFinishedRunnerCleanup(ct
 // 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.
+//
+// The check below is deliberately an equality test and must not be relaxed into
+// the >= monotonicity test the applied revision uses. Applied revisions derive
+// from metadata.generation and only ever climb, but listener patch IDs do not:
+// setDesiredWorkerState publishes 0 whenever the set is idle at MinRunners with
+// nothing dirty, restarts its sequence from 0 when the listener restarts, and
+// wraps explicitly at math.MaxInt32. So Spec.PatchID legitimately moves
+// backwards, and the marker has to follow it. Refusing to record a lower patch
+// ID would strand the marker above every value the listener goes on to publish,
+// and since the scale-up guard suppresses only on an exact match, suppression
+// would never fire again -- disabling the behaviour this layer exists to add.
+//
+// That is also why the lock is the right fix rather than a stricter comparison.
+// It cannot make an older patch ID unwritable, because the retry re-reads and
+// re-applies the same argument; recording the patch ID whose cleanup actually
+// happened is a true statement regardless of ordering, and the next cleanup
+// re-records. What the lock prevents is a write decided against state that has
+// since changed.
 func (r *EphemeralRunnerSetReconciler) patchFinishedRunnerCleanupPatchIDStatus(ctx context.Context, key types.NamespacedName, patchID int) error {
 	return retry.RetryOnConflict(retry.DefaultBackoff, func() error {
 		var latest v1alpha1.EphemeralRunnerSet