From 0c17a55075686851292ff1e52f4fc18ef656c8bd Mon Sep 17 00:00:00 2001 From: Nikola Jokic Date: Sat, 12 Sep 2026 14:28:11 +0200 Subject: [PATCH] Guard Running phase transition with optimistic locking Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../v1alpha1/ephemeralrunner_types.go | 4 ++-- cmd/ghalistener/scaler/scaler.go | 16 ++++++++++++++++ .../ephemeralrunner_controller.go | 4 +++- 3 files changed, 21 insertions(+), 3 deletions(-) diff --git a/apis/actions.github.com/v1alpha1/ephemeralrunner_types.go b/apis/actions.github.com/v1alpha1/ephemeralrunner_types.go index 4c4a2ace..a38ae85f 100644 --- a/apis/actions.github.com/v1alpha1/ephemeralrunner_types.go +++ b/apis/actions.github.com/v1alpha1/ephemeralrunner_types.go @@ -188,8 +188,8 @@ const ( // EphemeralRunnerPhasePending is a phase set when the ephemeral runner is // being provisioned and is not yet online. EphemeralRunnerPhasePending EphemeralRunnerPhase = "Pending" - // EphemeralRunnerPhaseRunning is a phase set when the ephemeral runner is online and - // waiting for a job to execute. + // EphemeralRunnerPhaseRunning is a phase set by the listener when a job has been + // assigned to this ephemeral runner and the runner is executing it. EphemeralRunnerPhaseRunning EphemeralRunnerPhase = "Running" // EphemeralRunnerPhaseSucceeded is a phase set when the ephemeral runner // successfully executed the job and has been removed from the service. diff --git a/cmd/ghalistener/scaler/scaler.go b/cmd/ghalistener/scaler/scaler.go index f9d504f2..8e166f32 100644 --- a/cmd/ghalistener/scaler/scaler.go +++ b/cmd/ghalistener/scaler/scaler.go @@ -15,6 +15,7 @@ import ( "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/kubernetes" "k8s.io/client-go/rest" + "k8s.io/client-go/util/retry" ) type Option func(*Scaler) @@ -138,6 +139,15 @@ func (w *Scaler) HandleJobStarted(ctx context.Context, jobInfo *scaleset.JobStar w.dirty = true + // The promotion to Running is guarded by an optimistic lock on the resource version + // observed by the GET below, so a terminal phase written between the read and the + // patch is never clobbered. Conflicts are retried against freshly read state. + return retry.RetryOnConflict(retry.DefaultRetry, func() error { + return w.patchJobStarted(ctx, jobInfo) + }) +} + +func (w *Scaler) patchJobStarted(ctx context.Context, jobInfo *scaleset.JobStarted) error { // Fetch current EphemeralRunner to check phase and deletion status currentRunner := &v1alpha1.EphemeralRunner{} err := w.clientset.RESTClient(). @@ -179,6 +189,8 @@ func (w *Scaler) HandleJobStarted(ctx context.Context, jobInfo *scaleset.JobStar currentRunner.Status.Phase != v1alpha1.EphemeralRunnerPhaseSucceeded && currentRunner.Status.Phase != v1alpha1.EphemeralRunnerPhaseOutdated { patchRunner.Status.Phase = v1alpha1.EphemeralRunnerPhaseRunning + // Optimistic lock: reject the promotion if the runner changed since the GET. + patchRunner.ResourceVersion = currentRunner.ResourceVersion } patch, err := json.Marshal(patchRunner) @@ -209,6 +221,10 @@ func (w *Scaler) HandleJobStarted(ctx context.Context, jobInfo *scaleset.JobStar w.logger.Info("Ephemeral runner not found, skipping patching of ephemeral runner status", "runnerName", jobInfo.RunnerName) return nil } + if kerrors.IsConflict(err) { + w.logger.Info("Ephemeral runner changed while patching job info, retrying", "runnerName", jobInfo.RunnerName) + return err + } return fmt.Errorf("could not patch ephemeral runner status, patch JSON: %s, error: %w", string(mergePatch), err) } diff --git a/controllers/actions.github.com/ephemeralrunner_controller.go b/controllers/actions.github.com/ephemeralrunner_controller.go index ce903b1a..4ecdef54 100644 --- a/controllers/actions.github.com/ephemeralrunner_controller.go +++ b/controllers/actions.github.com/ephemeralrunner_controller.go @@ -860,6 +860,8 @@ func (r *EphemeralRunnerReconciler) updateRunStatusFromPod(ctx context.Context, // The controller no longer promotes the runner to Running. The listener owns that // transition and applies it when a job is assigned to this runner. The controller // still publishes the initial Pending phase while the runner pod is starting. + // The patch below is optimistically locked so a stale cached copy of this runner + // cannot undo the listener's transition to Running. phaseChanged := phase != ephemeralRunner.Status.Phase readyChanged := ready != ephemeralRunner.Status.Ready @@ -880,7 +882,7 @@ func (r *EphemeralRunnerReconciler) updateRunStatusFromPod(ctx context.Context, ephemeralRunner.Status.Reason = pod.Status.Reason ephemeralRunner.Status.Message = pod.Status.Message - if err := r.Status().Patch(ctx, ephemeralRunner, client.MergeFrom(original)); err != nil { + if err := r.Status().Patch(ctx, ephemeralRunner, client.MergeFromWithOptions(original, client.MergeFromWithOptimisticLock{})); err != nil { return fmt.Errorf("failed to update runner status for Phase/Reason/Message/Ready: %w", err) } r.publishEphemeralRunnerPhaseMetric(ephemeralRunner, ephemeralRunner.Status.Phase, log)