Guard Running phase transition with optimistic locking

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

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.

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)
 	}
 

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)