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