diff --git a/controllers/actions.github.com/ephemeralrunnerset_cleanup_patch_test.go b/controllers/actions.github.com/ephemeralrunnerset_cleanup_patch_test.go index 7b880ede..0cee25c9 100644 --- a/controllers/actions.github.com/ephemeralrunnerset_cleanup_patch_test.go +++ b/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. diff --git a/controllers/actions.github.com/ephemeralrunnerset_controller.go b/controllers/actions.github.com/ephemeralrunnerset_controller.go index 27efd043..b772f8d6 100644 --- a/controllers/actions.github.com/ephemeralrunnerset_controller.go +++ b/controllers/actions.github.com/ephemeralrunnerset_controller.go @@ -493,6 +493,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