From 9ef9ac3db6346dfa3d93ead0af6f056d03b22bfe Mon Sep 17 00:00:00 2001 From: Nikola Jokic Date: Fri, 11 Sep 2026 10:57:12 +0200 Subject: [PATCH] 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{})) }) }