From e69d9e3c71a325972922ae6b640cef29c4237f9f Mon Sep 17 00:00:00 2001 From: Nikola Jokic Date: Fri, 11 Sep 2026 09:03:36 +0200 Subject: [PATCH 1/5] Move deletion of terminated runners after patch update Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- .../actions.github.com/ephemeralrunnerset_controller.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/controllers/actions.github.com/ephemeralrunnerset_controller.go b/controllers/actions.github.com/ephemeralrunnerset_controller.go index 2fc505f8..34ce10ed 100644 --- a/controllers/actions.github.com/ephemeralrunnerset_controller.go +++ b/controllers/actions.github.com/ephemeralrunnerset_controller.go @@ -217,14 +217,14 @@ func (r *EphemeralRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl.R // patch ID the cleanup belongs to and return, leaving the scaling decision // to the next reconcile, which sees the post-cleanup state. if len(ephemeralRunnersByState.finished) > 0 { - if err := r.deleteTerminatedEphemeralRunners(ctx, ephemeralRunnersByState.finished, log); err != nil { - log.Error(err, "failed to delete terminated ephemeral runners") - return ctrl.Result{}, err - } if err := r.patchFinishedRunnerCleanupPatchIDStatus(ctx, req.NamespacedName, ephemeralRunnerSet.Spec.PatchID); err != nil { log.Error(err, "failed to update finished runner cleanup patch ID status") return ctrl.Result{}, err } + if err := r.deleteTerminatedEphemeralRunners(ctx, ephemeralRunnersByState.finished, log); err != nil { + log.Error(err, "failed to delete terminated ephemeral runners") + return ctrl.Result{}, err + } ephemeralRunnerSet.Status.FinishedRunnerCleanupPatchID = ephemeralRunnerSet.Spec.PatchID log.Info("Finished ephemeral runners were cleaned up, deferring scaling decision") From f862d6f9cbf1fbd0d194b071a98abfbea2881f52 Mon Sep 17 00:00:00 2001 From: Nikola Jokic Date: Fri, 11 Sep 2026 09:04:34 +0200 Subject: [PATCH 2/5] Fix formatting in ephemeralrunnerset_controller_test.go Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- .../actions.github.com/ephemeralrunnerset_controller_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/controllers/actions.github.com/ephemeralrunnerset_controller_test.go b/controllers/actions.github.com/ephemeralrunnerset_controller_test.go index df3254e7..a2d4ccd1 100644 --- a/controllers/actions.github.com/ephemeralrunnerset_controller_test.go +++ b/controllers/actions.github.com/ephemeralrunnerset_controller_test.go @@ -627,7 +627,7 @@ var _ = Describe("Test EphemeralRunnerSet controller", func() { // confirm they are not deleted runnerList = new(v1alpha1.EphemeralRunnerList) - Eventually( + Consistently( func() (int, error) { err := listEphemeralRunnersAndRemoveFinalizers(ctx, k8sClient, runnerList, ephemeralRunnerSet.Namespace) if err != nil { From ec199b6d64d48bf8d16c6bf4c10d8442f336f53c Mon Sep 17 00:00:00 2001 From: Nikola Jokic Date: Fri, 11 Sep 2026 10:28:10 +0200 Subject: [PATCH 3/5] Guard the applied revision patch with an optimistic lock patchAppliedActionableRevisionStatus wrapped retry.RetryOnConflict around a plain client.MergeFrom status patch. A merge patch carries no resourceVersion precondition, so the API server can never reject the write as conflicting and the retry wrapper could never fire. Worse, the monotonicity check inside the retry was unsound. Both the target revision and the re-fetched object come from the cache-backed client, so a stale reconcile could compare a stale target against an equally stale read, pass the guard, and patch AppliedActionableRevision backwards. That re-satisfies Spec.ActionableRevision > Status.AppliedActionableRevision and sends the controller through the idle and pending runner cleanup again. MergeFromWithOptimisticLock stamps the re-fetched resourceVersion into the patch, so the server accepts it only if that object is still live. A successful patch therefore proves the guard was evaluated against live data, which is what makes the applied marker monotonic. A stale target can now only ever under-advance the marker, never regress it, and a later reconcile with fresh data completes the advance. At this layer the re-fetch inside the retry still uses the cached client, so a genuine conflict may refetch stale data, exhaust the backoff and return the conflict error. That requeues rather than writing a bad value, so it fails closed; it is only noisier than necessary. A later change makes the refetch authoritative so the retry can resolve the conflict in place. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../ephemeralrunnerset_controller.go | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/controllers/actions.github.com/ephemeralrunnerset_controller.go b/controllers/actions.github.com/ephemeralrunnerset_controller.go index e173cc31..31a6069f 100644 --- a/controllers/actions.github.com/ephemeralrunnerset_controller.go +++ b/controllers/actions.github.com/ephemeralrunnerset_controller.go @@ -259,6 +259,16 @@ 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 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 @@ -273,7 +283,7 @@ func (r *EphemeralRunnerSetReconciler) patchAppliedActionableRevisionStatus(ctx original := latest.DeepCopy() latest.Status.AppliedActionableRevision = targetAppliedRevision - return r.Status().Patch(ctx, &latest, client.MergeFrom(original)) + return r.Status().Patch(ctx, &latest, client.MergeFromWithOptions(original, client.MergeFromWithOptimisticLock{})) }) } From 3d5cf499133686b86af0b37efcd1cea7be573fd7 Mon Sep 17 00:00:00 2001 From: Nikola Jokic Date: Fri, 11 Sep 2026 10:37:45 +0200 Subject: [PATCH 4/5] Cover the applied revision patch preconditions The optimistic lock and the monotonicity guard in patchAppliedActionableRevisionStatus both prevent the applied revision marker from moving backwards, and neither was covered. Reproducing the lost update needs two reconciles racing on one object with a stale cached read, which is awkward to provoke deterministically. The observable property is cheaper: the emitted status patch must carry a resourceVersion precondition, since that is what lets the API server reject a stale write. The test intercepts SubResourcePatch and asserts on the bytes the reconciler actually emits, so it cannot pass while the patch is built some other way. Both assertions were mutation checked. Reverting the patch option to client.MergeFrom fails the precondition test, reporting the emitted patch as {"status":{"appliedActionableRevision":7}} with no resourceVersion. Disabling the >= guard fails the backwards test. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../ephemeralrunnerset_revision_patch_test.go | 142 ++++++++++++++++++ 1 file changed, 142 insertions(+) create mode 100644 controllers/actions.github.com/ephemeralrunnerset_revision_patch_test.go diff --git a/controllers/actions.github.com/ephemeralrunnerset_revision_patch_test.go b/controllers/actions.github.com/ephemeralrunnerset_revision_patch_test.go new file mode 100644 index 00000000..df3f6309 --- /dev/null +++ b/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) +} From 9ef9ac3db6346dfa3d93ead0af6f056d03b22bfe Mon Sep 17 00:00:00 2001 From: Nikola Jokic Date: Fri, 11 Sep 2026 10:57:12 +0200 Subject: [PATCH 5/5] Lock the cleanup marker patch against a stale write patchFinishedRunnerCleanupPatchIDStatus wrapped a plain merge patch in retry.RetryOnConflict. A plain merge patch carries no resourceVersion precondition, so the API server has nothing to reject: the write always succeeds, the retry can never fire, and the surrounding machinery reads as protection while providing none. The exposure is worse here than for the applied revision, which at least refuses to move backwards. This helper compares for equality and then writes whatever patch ID the reconcile is carrying, so it is willing to lower the marker. A reconcile serving an older patch ID can therefore overwrite a marker recorded for a newer one, and the scale-up guard then stops suppressing for the patch it actually serviced -- creating the replacement runners this layer exists to prevent. Re-fetching through the API reader narrows that window to the gap between the read and the patch, but it cannot close it, because the decision is only as fresh as the moment it was taken. The optimistic lock is what makes the write conditional on that decision still holding; a stale attempt now conflicts and is retried rather than silently recording the wrong patch ID. The test asserts on the patch bytes the reconciler actually emits rather than on an independently constructed patch, so it cannot pass while the production code builds its patch some other way. A second test covers the early return, since an unconditional write would churn the status on every reconcile for the same patch. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../ephemeralrunnerset_cleanup_patch_test.go | 145 ++++++++++++++++++ .../ephemeralrunnerset_controller.go | 17 +- 2 files changed, 161 insertions(+), 1 deletion(-) create mode 100644 controllers/actions.github.com/ephemeralrunnerset_cleanup_patch_test.go diff --git a/controllers/actions.github.com/ephemeralrunnerset_cleanup_patch_test.go b/controllers/actions.github.com/ephemeralrunnerset_cleanup_patch_test.go new file mode 100644 index 00000000..7b880ede --- /dev/null +++ b/controllers/actions.github.com/ephemeralrunnerset_cleanup_patch_test.go @@ -0,0 +1,145 @@ +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" +) + +// TestPatchFinishedRunnerCleanupPatchIDStatusUsesOptimisticLock pins the +// resourceVersion precondition on the cleanup marker patch. +// +// The marker decides whether a shortfall below Spec.Replicas is suppressed, so a +// write that lands on the wrong patch ID re-enables the spurious scale up this +// layer exists to prevent. The helper sets the marker to whatever patch ID the +// reconcile is carrying rather than only ever advancing it, so without the lock +// the API server cannot reject a stale write, retry.RetryOnConflict never fires, +// and a reconcile serving an older patch ID can overwrite a marker recorded for +// a newer one. +// +// Asserted on the bytes the production code emits rather than on an +// independently built patch, so it cannot pass while the reconciler constructs +// its patch some other way. Reverting the option to a plain client.MergeFrom +// must fail this test. +func TestPatchFinishedRunnerCleanupPatchIDStatusUsesOptimisticLock(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{ + PatchID: 4, + }, + } + + 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, + APIReader: c, + Log: logr.Discard(), + Scheme: scheme, + } + + key := types.NamespacedName{Namespace: ephemeralRunnerSet.Namespace, Name: ephemeralRunnerSet.Name} + require.NoError(t, reconciler.patchFinishedRunnerCleanupPatchIDStatus(context.Background(), key, 4)) + + 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 { + FinishedRunnerCleanupPatchID int `json:"finishedRunnerCleanupPatchID"` + } `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 recording the wrong patch ID, got %s", + string(capturedPatch), + ) + assert.Equal(t, 4, emitted.Status.FinishedRunnerCleanupPatchID) + + var updated v1alpha1.EphemeralRunnerSet + require.NoError(t, c.Get(context.Background(), key, &updated)) + assert.Equal(t, 4, updated.Status.FinishedRunnerCleanupPatchID) +} + +// 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. +func TestPatchFinishedRunnerCleanupPatchIDStatusIsIdempotent(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: 4, + }, + } + + patched := false + 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 { + patched = true + return clt.Status().Patch(ctx, obj, patch, opts...) + }, + }). + 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, 4)) + + assert.False(t, patched, "recording a patch ID the marker already holds must not emit a write") +} diff --git a/controllers/actions.github.com/ephemeralrunnerset_controller.go b/controllers/actions.github.com/ephemeralrunnerset_controller.go index d762737d..89763d1e 100644 --- a/controllers/actions.github.com/ephemeralrunnerset_controller.go +++ b/controllers/actions.github.com/ephemeralrunnerset_controller.go @@ -402,6 +402,21 @@ func (r *EphemeralRunnerSetReconciler) scaleUpServicedByFinishedRunnerCleanup(ct // Like the applied revision above, this is written after the deletions succeed // and re-fetches the object inside the retry, so a conflicting write is never // resolved by replaying a status that predates the cleanup. +// +// The patch carries an optimistic lock for the same reason, and the exposure +// here is if anything worse: the check below is an equality test rather than a +// monotonicity test, so this helper is willing to move the marker to whatever +// patch ID the reconcile is carrying, including backwards. Without a +// resourceVersion precondition the API server cannot reject the write, so +// RetryOnConflict can never fire and a reconcile serving an older patch ID can +// overwrite a marker recorded for a newer one. The guard would then stop +// suppressing for the patch ID that was actually serviced, and the controller +// would create the replacement runners this layer exists to prevent. +// +// Re-fetching through the API reader narrows that window to the gap between the +// 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. func (r *EphemeralRunnerSetReconciler) patchFinishedRunnerCleanupPatchIDStatus(ctx context.Context, key types.NamespacedName, patchID int) error { return retry.RetryOnConflict(retry.DefaultBackoff, func() error { var latest v1alpha1.EphemeralRunnerSet @@ -420,7 +435,7 @@ func (r *EphemeralRunnerSetReconciler) patchFinishedRunnerCleanupPatchIDStatus(c original := latest.DeepCopy() latest.Status.FinishedRunnerCleanupPatchID = patchID - return r.Status().Patch(ctx, &latest, client.MergeFrom(original)) + return r.Status().Patch(ctx, &latest, client.MergeFromWithOptions(original, client.MergeFromWithOptimisticLock{})) }) }